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

refactor 'select all/clear all selections' methods out of GridBar, into Grid #1740

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

labkey-chrisj
Copy link
Contributor

@labkey-chrisj labkey-chrisj commented Dec 1, 2023

Rationale

While investigating failures in biologics test automation, I noticed that GridBar.clearAllSelections has an issue that causes it to no-op silently if there isn't a button containing 'Clear' already present. Thinking it would be a simple matter of waiting for it to appear, I added that wait- and discovered that the check for its presence was looking within the GridBar, when the 'Clear' button is now located outside of that, in the 'selection-status' element, which contains the select-all and clear-all buttons.

Investigating further, I noted that QueryGrid.clearAllSelections checks to see if the current grid is a GridPanel (versus being a QueryGridPanel)- and only calls into GridBar.clearAllSelections if it is the latter. (IIRC, QueryGridPanel was to be deprecated).

If QGP is in fact deprecated (@labkey-alan, is this the case?) we should probably remove GridBar's ``clearAllSelections method now and refactor its usages to QueryGrid's clearAllSelections.
If not, the change in this PR updates GridBar's clearAllSelections to function regardless of whether the current grid is a QueryGrid, by finding the 'Clear' button in the QueryGrid's componentElement

Related Pull Requests

related changes in biologics
https://github.com/LabKey/biologics/pull/2554

Changes

find the button in the QueryGrid component instead of the GridBar

Copy link
Member

@labkey-tchad labkey-tchad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There has to be a better solution to this. We expect places to call clearAllSelections just in case there's a selection. 90% of the time these should no-op but this change will add a 5 second delay to each of these calls.
Most callers shouldn't need to dig into the grid bar.
e.g. grid.clearAllSelections(); instead of grid.getGridBar().clearAllSelections();
Not least of all because the QueryGrid version wraps it in a doAndWaitForUpdate.

@labkey-alan
Copy link
Contributor

QueryGridPanel no longer exists, so yes, you should be able to get rid of the old code.

@labkey-chrisj
Copy link
Contributor Author

OK, I've taken the liberty to:

  • take out the checks for QueryGridPanels,
  • remove the GridBar select-all/deselect-all methods
  • refactor usages of the GridBar methods to instead use their corresponding methods on the Grid

This addresses the concern that we'll wait too long for situations that don't call for it- also removes some code that's been effectively dead for some time

@labkey-chrisj
Copy link
Contributor Author

Biologics results here
SM results here

@labkey-chrisj labkey-chrisj changed the title change search scope for 'clear' button to avoid silent no-op refactor 'select all/clear all selections' methods out of GridBar, into Grid Dec 4, 2023
@labkey-chrisj labkey-chrisj merged commit 527655b into develop Dec 5, 2023
1 check passed
@labkey-chrisj labkey-chrisj deleted the fb_clearSelectionsNoOp branch December 5, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants