New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix resync remote repos #4113

Merged
merged 6 commits into from Jun 9, 2018

Conversation

Projects
None yet
3 participants
@stsewd
Member

stsewd commented May 18, 2018

Fix #4101

I'll re-minify the js after see if this is the correct solution p:

@@ -282,6 +282,8 @@ function ProjectImportView(instance, config) {
.then(function (data) {
self.get_organizations();
self.get_accounts();
// re-sync
self.filter_by(self.filter_by());

This comment has been minimized.

@stsewd

stsewd May 18, 2018

Member

I'm not sure if this is the proper way of doing this, but I didn't find anything on the docs. But I tested this and works as expected.

This comment has been minimized.

@humitos

humitos May 22, 2018

Member

Did you test it locally? The line looks fine to me.

Don't we need to re-request the projects also and then apply the filter?

This comment has been minimized.

@stsewd

stsewd May 22, 2018

Member

Did you test it locally? The line looks fine to me.

Yes, I tested locally, it works.

Don't we need to re-request the projects also and then apply the filter?

When the filter is set, a new request is made https://github.com/rtfd/readthedocs.org/pull/4113/files#diff-fd74ba503cde92cdf9a406ce7da32f2eL210.

@@ -282,6 +282,8 @@ function ProjectImportView(instance, config) {
.then(function (data) {
self.get_organizations();
self.get_accounts();
// re-sync

This comment has been minimized.

@humitos

humitos May 22, 2018

Member

It's not a re-sync but a filter by previously selected filter

This comment has been minimized.

@stsewd

stsewd May 22, 2018

Member

But this is a hack to trigger the re-sync

This comment has been minimized.

@humitos

humitos May 22, 2018

Member

yeah, you are right. So, maybe it's better to improve the comment explaining why we are doing this here and how it works behind the scenes.

@@ -282,6 +282,8 @@ function ProjectImportView(instance, config) {
.then(function (data) {
self.get_organizations();
self.get_accounts();
// re-sync
self.filter_by(self.filter_by());

This comment has been minimized.

@humitos

humitos May 22, 2018

Member

Did you test it locally? The line looks fine to me.

Don't we need to re-request the projects also and then apply the filter?

@agjohnson

I'm not sure about my change, but there is likely a more dedicated mechanism for notifying subscribers of changes or forcing re-evaluation of the observable.

@@ -282,6 +282,9 @@ function ProjectImportView(instance, config) {
.then(function (data) {
self.get_organizations();
self.get_accounts();
// This will trigger a new
// request to re-sync the repos
self.filter_by(self.filter_by());

This comment has been minimized.

@agjohnson

agjohnson May 22, 2018

Contributor

I believe the observable should have a notifySubscribers method that we can call, instead of resetting the variable like this to notify subscribing observables.

This comment has been minimized.

@stsewd

stsewd May 22, 2018

Member

I'll try to take a deeper look to the docs and see what I can find.

@@ -282,6 +282,9 @@ function ProjectImportView(instance, config) {
.then(function (data) {
self.get_organizations();
self.get_accounts();
// This will trigger a new
// request to re-sync the repos
self.filter_by.valueHasMutated();

This comment has been minimized.

@stsewd

stsewd May 22, 2018

Member

I took this from https://stackoverflow.com/questions/16261689/force-knockout-to-mark-an-observable-as-changed-even-if-focus-is-still-in-the-f#16261761, still not sure if this is the correct way, but I tested it and works.

This comment has been minimized.

@agjohnson

agjohnson Jun 8, 2018

Contributor

This change looks better.

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Jun 8, 2018

This will need a rebase and regeneration of the static assets.

@agjohnson agjohnson added this to the 2.6 milestone Jun 8, 2018

@agjohnson agjohnson added the Bug label Jun 8, 2018

@stsewd

This comment has been minimized.

Member

stsewd commented Jun 8, 2018

Done

@agjohnson agjohnson merged commit 04ce7bb into rtfd:master Jun 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:fix-resync branch Jun 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment