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

[23.2] Fix tools missing in panel bug #17192

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Dec 14, 2023

Some tools would be filtered out from the tool panel needlessly when organizing the ToolPanel in Panels/utilities.ts.

This was caused due to the mistake of filtering out cases where tools had the same exact name+description, which is something we shouldn't have done because different id = different tool.

For example, because of this bug, we had FastQC Read Quality reports missing, although, Porechop adapter trimmer for Oxford Nanopore reads was found in the panel, even though both tools had multiple occurrences in the toolsList with same name+desc. What caused this:

  • first we were (erroneously) removing all repetitions after the first occurrence (referenced by first found tool id) of such a tool in the toolsList (... we would in fact include one occurrence in toolsById ...)
  • then, we would use the api/tool_panels/default structure to display the tool panel, where in the case of FastQC, we didn't have the latest tool id in toolsById, and in the case of Porechop we just happened to store the latest tool id (by chance of the ordering of the toolsList) in toolsById

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

Some tools would be filtered out from the tool panel needlessly
when organizing the `ToolPanel` in `Panels/utilities.ts`.
@dannon
Copy link
Member

dannon commented Dec 14, 2023

Given the tests aren't changed I'm assuming there was insufficient (or nonexistent) test coverage for the duplicate filtering that was mentioned in the comment -- is this something we need to enhance (either here or dev) to make sure these corner cases are all covered and we're not going back and forth on fixes?

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Dec 14, 2023

Given the tests aren't changed I'm assuming there was insufficient (or nonexistent) test coverage for the duplicate filtering that was mentioned in the comment -- is this something we need to enhance (either here or dev) to make sure these corner cases are all covered and we're not going back and forth on fixes?

The "filtering out tools with the exact same name+description" was something we didn't want done at the front end at all in the first place, and the actual problem was fixed at the backend in #17036 , so I guess no tests were/are needed for that in this case? I believe this was just a case of removing this unnecessary filtering in the front end

@dannon
Copy link
Member

dannon commented Dec 14, 2023

Got it, so for the example where tool id mytool changes to mytool_redux with the exact same name/desc, we just show both in the panel but always link to 'latest' based on the lineages now?

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Dec 14, 2023

Got it, so for the example where tool id mytool changes to mytool_redux with the exact same name/desc, we just show both in the panel but always link to 'latest' based on the lineages now?

Yes, we route to ...&version=latest always, although this was a fix made earlier for older/newer versions having different names in different sections.
We would also get the case where tools can have the same exact name+desc (but different ids of course) but we don't want to filter out these tools, since we treat two tools with different ids as different tools.
(I should have removed this bit of code earlier, my bad on that...)

@martenson
Copy link
Member

good job tracking it down 👍

@martenson martenson merged commit c4c8d7b into galaxyproject:release_23.2 Dec 14, 2023
26 of 27 checks passed
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