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

[21.01] Fix setCurrentHistoryId in legacy history panel context #11369

Merged

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Feb 14, 2021

If we create a new history in the legacy history the store does not get
updated, so
https://github.com/galaxyproject/galaxy/blame/3f47effa087f4049132da65643396d58a2be51d1/client/src/components/History/model/historyStore.js#L48
will always return the most recently used history.
This is a little silly in that we will load the history twice, but we
don't switch the history so often that it should matter.

If you want to test this, open the upload modal, close it, and switch to a new history, then upload a file. The dataset will appear in the old history. This fixes that.

If we create a new history in the legacy history the store does not get
updated, so
https://github.com/galaxyproject/galaxy/blame/3f47effa087f4049132da65643396d58a2be51d1/client/src/components/History/model/historyStore.js#L48
will always return the most recently used history.
This is a little silly in that we will load the history twice, but we
don't switch the history so often that it should matter.
@mvdbeek mvdbeek changed the title Fix setCurrentHistoryId in legacy history panel context [21.01] Fix setCurrentHistoryId in legacy history panel context Feb 14, 2021
@github-actions github-actions bot added this to the 21.05 milestone Feb 14, 2021
@Nerdinacan
Copy link
Contributor

Looks good to me, but let me check it out and play with a bit, see if I can find any hassles.

@Nerdinacan
Copy link
Contributor

This looks good, but I think we also want to remove the conditional gate in syncVuexToGalaxy because you will want the store monitor to work regardless of whether the beta panel is open or not, now that we are using the history store (via UserHistories) outside of the beta panel..

@mvdbeek
Copy link
Member Author

mvdbeek commented Feb 14, 2021

But I think that works correctly, if the beta history panel is open it sets the history on switch, and if it's not open it passes through the dispatch. I don't think there's another possibility ?

@Nerdinacan
Copy link
Contributor

Nerdinacan commented Feb 14, 2021

Well what if something elsewhere in the legacy code accesses Galaxy.currentHistory to change the current history from outside of the beta panel interface? I thought that was your use case?

If no such UI element exists, disregard my comment. :)

@Nerdinacan
Copy link
Contributor

I think maybe the list of histories might do something like this?
historylist

Here the old list changes the history with legacy code. I don't know how many other places might do the same, which is why I built the sync function.

@mvdbeek
Copy link
Member Author

mvdbeek commented Feb 14, 2021

Yeah, that's just what this fixes, except that switching histories in this legacy view also reloads the whole history panel, so it also worked without this fix. Switching the history even from legacy components nicely passes through the listener you wrote.
I'm inclined to think about it if we notice this to be a problem and let this be a "future us" problem

@Nerdinacan
Copy link
Contributor

I'm sold.

@dannon dannon merged commit efd8688 into galaxyproject:release_21.01 Feb 14, 2021
@mvdbeek mvdbeek deleted the fix_current_history_state branch March 1, 2021 08:42
@mvdbeek mvdbeek modified the milestones: 21.05, 21.01 Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants