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

Multiview missing histories fix #4610

Merged
merged 3 commits into from Sep 14, 2017

Conversation

Projects
None yet
4 participants
@dannon
Copy link
Member

commented Sep 13, 2017

This fixes #4605 and probably #3519.

Fix in 4f93f88. Minor cruft cleanup, and swapping of commented out log statements to Galaxy.debug included in 1502e00.

dannon added some commits Sep 13, 2017

Allow fetchMore to actually specify a 0 offset when intentional. This…
… should fix the mysterious missing history in multiview bug.
Some cruft fixing and configuring of Galaxy.debug output instead of c…
…ommented out console.logs -- should help immensely for troubleshooting
@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

This doesn't fix #3519 unfortunately :(. I can reproduce this 100% of the time though, so if you want me to try something else that wouldn't be a problem.

@greenwoodma

This comment has been minimized.

Copy link

commented Sep 14, 2017

does fix #4605 though, thanks!

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

--- a/client/galaxy/scripts/mvc/base/controlled-fetch-collection.js
+++ b/client/galaxy/scripts/mvc/base/controlled-fetch-collection.js
@@ -57,6 +57,10 @@ var ControlledFetchCollection = Backbone.Collection.extend({
         // options.data.filters --> options.data.q, options.data.qv
         var filters = this._buildFetchFilters( options );
         Galaxy.debug( 'filters:', filters );
+        console.log('filters:');
+        console.log(filters);
+        filters = {'purged': "", 'deleted': "", 'encoded_id-in': ""};
+        console.log(filters);
         if( !_.isEmpty( filters ) ){
             _.extend( options.data, this._fetchFiltersToAjaxData( filters ) );
         }

is a hacky way that solves #3519 for me. And it's always the newest history that is missing if you make the current history a history that is not the newest history.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

this also works, and is probably closer to the real issue (concerning #3519):

--- a/client/galaxy/scripts/mvc/history/history-model.js
+++ b/client/galaxy/scripts/mvc/history/history-model.js
@@ -443,12 +443,12 @@ var HistoryCollection = _collectionSuper.extend( BASE_MVC.LoggableMixin ).extend
             xhr = _collectionSuper.prototype.fetchFirst.call( self, {
                 silent: true,
                 limit : 1,
-                filters: {
-                    // without these a deleted current history will return [] here and block the other xhr
-                    'purged'        : '',
-                    'deleted'       : '',
-                    'encoded_id-in' : this.currentHistoryId,
-                }
+                // filters: {
+                //     // without these a deleted current history will return [] here and block the other xhr
+                //     'purged'        : '',
+                //     'deleted'       : '',
+                //     'encoded_id-in' : this.currentHistoryId,
+                // }
             });
         }
         return xhr.then( function(){
@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

And empirically testing // without these a deleted current history will return [] here and block the other xhr doesn't reveal a problem, neither in the "saved histories" view nor in the "view all histories" view.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

This filter was introduced in 2a38b11, which first appeared in 16.10, which is consistent with #3519 (comment)

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

I've opened a PR to this branch here

@dannon

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

@mvdbeek Thanks! At first glance, that filter (at least with the current history id) seems like it'd be necessary; let me check out what's going on.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

That shouldn't hold up this PR in any way, every fix is a good fix. Just thought we could combine it.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

This actually also fixes #3519, I guess I must have done something stupid building the client during my tests.

@mvdbeek mvdbeek merged commit 9596926 into galaxyproject:dev Sep 14, 2017

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
api test Build finished. 292 tests run, 4 skipped, 0 failed.
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 45 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

Thanks so much @dannon !

@mvdbeek mvdbeek referenced this pull request Sep 14, 2017

Closed

History disappears #3519

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

Should 4f93f88 be backported to 16.10?

@dannon

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

Sure, I can do that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.