Merge from onprc19.1 & onprc19.1Prod r.63271 to 65949

[labkey-bpatel]

Rationale

Merge changes from onprc19.1 r.63271 to 65757
Merge changes from onprc19.1Prod r. 65762 to 65949

Related Pull Requests

LabKey/onprcEHRModules#32
LabKey/platform#1626
LabKey/ehrModules#74
LabKey/DiscvrLabKeyModules#64
https://github.com/LabKey/sampleManagement/pull/381
https://github.com/LabKey/biologics/pull/712
https://github.com/LabKey/labbook/pull/53
LabKey/wnprc-modules#38

[labkey-jeckels]

One bit of comment cleanup, but otherwise looks good.

[bbimber]

What's the background on this? Wouldnt calling clearSelected() substantially change behavior from before?

[labkey-jeckels]

This has to do with how sticky selections are. This change clears the selection, but holds on to the selection values at the time and uses them for the action the user performed. This is similar to passing "true" as the value for clearSelection in the server-side DataRegion.getSelected() code.

https://www.labkey.org/ONPRC/Support%20Tickets/issues-details.view?issueId=33957

[bbimber]

OK, I understand now. This type of issue is why in my modules I have complete stopped using the async getSelected() API. I exclusively use getChecked(). While I understand there were specific use cases driving the persistence of selection, it is simply wrong way too often. most of the time when it's wrong the issue is subtle, non-mutable, and really doesnt matter. The breeding group issue you point out is a genuine bug and annoying, but it's not like it's deleting or editing records. The fact that LK's delete button uses getSelected honestly scares me. There simply isnt enough visible feedback either. There is the 'selection XX of YY records' banner; however, that doesnt reliably show up.

I have a few thoughts:

1. This is being picky, but it doesnt feel like the right separation of concerns to put the onus to clear selection in the method getDataRegionWhereClause(). At minimum it's putting the onus for clear() in a really obscure way into code, which to me feels like the DataRegion API is wrong.

2. I 100% agree that in a sizable number of usages the desired behavior after calling DataRegion.getSelected() is to clear selection. Why not have a toggle on that API for "clearSelection: true" then? I dont think this would actually solve all the instances in which I believe getSelected() returns results that are not what the user intended, but forcing a more frequent purge of selected records would at least reduce the number of times LK gets it wrong.

3. All usage I immediately see for this method is in EHR module (i dont have all the client EHRs checked out). Not linking this refector to this PR is OK; however, since I think EHR is the only user of this method, I'd just as soon refactor this into EHR.utils. I'd put LDK.DataRegionUtil.bulkEditHandler as a static method in BulkEditWindow, and then we get rid of DataRegionUtils.js. If we do this, I care less about the guts of getDataRegionWhereClause().

I'm not sure how much you believe me on the fact that getSelected() extremely commonly doesnt match the user's intentions. A while ago I put logging into our modules to log instances when getChecked() didnt match getSelected() on various button handles - answer is quite often they dont. Again, I acknowledge there are legitimate use cases for persistence; however, i really want to emphasize that LK is getting this wrong all the time, and the only reason it hasnt become more of an issue is either that usually low consequence (like showing an extra group or downloading a few more rows into an excel file). People probably often just assume they checked boxes wrong.

Writing this, I would really prefer the persistence was more of an opt-in behavior; however, I assume for legacy reasons that's not going to happen. Anyway, this is a long response, but #2 above seems like a genuinely useful and small change (outside this PR). I would like to undertake #3, which should be a separate PR.

[labkey-jeckels]

Agreed that the code that's part of this merge is clunky in terms of selection state clearing. I've opened an issue to myself to add the new argument, which is analogous to the DataRegionSelection.getSelected() clearSelection argument (as you probably know).

https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=41705

We've had many discussions on how to handle the selection state. The general feedback we've received (though not unanimous) from different groups is that they prefer the cross-page selection handling. See more here, if you haven't already:

https://www.labkey.org/issues/home/Developer/issues/details.view?issueId=37818

But of course one of the biggest sources of confusion is when the UI behaves inconsistently, both in terms of clearing the selection and in terms of cross-page selections.

[bbimber]

I think the selection stickiness/not-stickiness is a lingering problem in LabKey, but it's well beyond the scope of this PR. If there is more internal LK discussion about these behaviors I'd maybe just these use cases in that discussion:

1. What if the user has two windows/tabs open showing the same grid, doing different things? I'd argue there's a reasonable chance they would want different selections in each window. I'm not saying there isnt some scenario in which the person wanted them to be the same, but there's many when you might not.

2. Discussion of actions like 'select all' might make sense for a smallish sample set. What if your table has 15m records? Any discussion around selection stickiness should consider both small table use-cases and enormous tables.

3. I'd argue one problem is that there isnt consistent enough transparency and user-facing info on what is selected. I havent studied this a lot, but my sense is that the DR bar summarizing selection (which is reasonable) isn't consistently appearing.

4. A few tweaks might mitigate some of the excessive stickiness. Like we discussed, more actions should probably clear selection by default, and building that option into the API might help. I dont know the current rules, but a more aggressive timeout for selection might be useful (i.e. if there is no selection change in X mins, clear it).

5. It might be useful to think about the scope of the selected list. I believe it is currently keyed by session/schemaName/queryName? I understand why it's done that way, but perhaps there might be ways to scope this, at least in some usages, using DR name or adding another ID to that key? I realize there are reasons that is tricky with page loads. Maybe adding something about the page's base URL into that scope?

6. I really want to emphasize this point: under most conditions if LK is sticky on selection and returns something broader than the user intended, much of the time the user probably either doesnt notice or assumes they screwed up. I did that until I began paying more attention, and I'm a fairly savvy user. This doesnt mean overly sticky selection is acceptable - I'm just saying that users might observe it more in one direction than the other.

[labkey-jeckels]

@bbimber thanks for your quick responses. Sorry it's taken me so long to loop back on this. I think with my latest commit we're in agreement on the contents of this PR, with some followup items identified.