Skip to content
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
Merged

Fix resync remote repos #4113

merged 6 commits into from Jun 9, 2018

Conversation

@stsewd
Copy link
Member

@stsewd 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());
Copy link
Member Author

@stsewd stsewd May 18, 2018

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.

Copy link
Member

@humitos humitos May 22, 2018

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?

Copy link
Member Author

@stsewd stsewd May 22, 2018

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
Copy link
Member

@humitos humitos May 22, 2018

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

Copy link
Member Author

@stsewd stsewd May 22, 2018

But this is a hack to trigger the re-sync

Copy link
Member

@humitos humitos May 22, 2018

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());
Copy link
Member

@humitos humitos May 22, 2018

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?

Copy link
Contributor

@agjohnson agjohnson left a comment

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());
Copy link
Contributor

@agjohnson agjohnson May 22, 2018

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

Copy link
Member Author

@stsewd stsewd May 22, 2018

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();
Copy link
Member Author

@stsewd stsewd May 22, 2018

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.

Copy link
Contributor

@agjohnson agjohnson Jun 8, 2018

This change looks better.

@agjohnson
Copy link
Contributor

@agjohnson 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
Copy link
Member Author

@stsewd stsewd commented Jun 8, 2018

Done

@agjohnson agjohnson merged commit 04ce7bb into readthedocs:master Jun 9, 2018
1 check passed
@stsewd stsewd deleted the fix-resync branch Jun 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants