Staff catalog lets staff pick invalid pickup locations

[Benjamin Kalish]

When placing a hold in the staff catalog staff may pick any of the values in the Pickup Location selection list by using the autocomplete feature. Although invalid locations are greyed out and cannot be selected using the mouse, staff can easily select these by typing the first few letters and hitting the tab key. This can easily lead to holds with nonsense pickup locations or to holds that can't be placed without any obvious reason.

Observed in Evergreen 3-7-3.

[Terran McCanna (tmccanna)]

Confirmed in 3.8

[Benjamin Kalish (bkalish)]

I should add that there are two separate problems here:
1. The interface should not let staff pick invalid locations
2. Attempts to place holds with invalid locations should fail

Fixing either problem would be an improvement, but ideally we would fix both.

[John Amundson (jamundson)]

Also noting that if "OPAC: Org Unit is not a hold pickup library" is set to TRUE for a library, but the staff place a hold for a patron that defaults to this library, staff can still place the hold without warning.

If staff navigate away from the library, they can't select it again because it's grayed out, but this doesn't stop the initial default selection, which still allows the hold to be placed.

[Terran McCanna (tmccanna)]

I inadvertently submitted a duplicate bug, so copying and pasting my description from that one here:

****************

3.12 - This is a follow up remaining bug to the fixes already made in these:

https://bugs.launchpad.net/evergreen/+bug/1908743
https://bugs.launchpad.net/evergreen/+bug/1477154
https://bugs.launchpad.net/evergreen/+bug/2043127

The remaining issue is in the Angular staff catalog only - if patron has a default pickup library set to BR1 and then BR1 is disabled as a pickup library, the hold placement screen will still default to BR1 and allow it to be saved when the hold request is submitted. (Note, it does NOT allow BR1 to be selected from the dropdown, it is only a problem when it is already the default.)

[Steven Mayo (stmayo)]

You can use the tab completion portion of this bug to use a visible but disabled org anywhere the angular org selector is used, not just when placing a hold. I used it to create a workstation at SYS2, for example.

[Steven Mayo (stmayo)]

After some more looking, it appears that this is happening because angular bootstrap typeahead (ngbtypeahead) does not actually support displaying disabled options. Our current workaround is manually fetching the options to apply the disabled class to them. This stops you from clicking on them to select them but doesn't prevent ngbtypeahead's internal logic from considering them active - in fact the moment you focus on the org selector ngbtypeahead gives CONS the active class, there's just no CSS for active AND disabled.
And, ngbtypeahead is handling the logic of arrow, tab, and enter keys while the org_selector is focused. As far as I'm aware, that means we can't get to their keypress handlers to add the check either.
So, unfortunately, without a complete rewrite of org-select.component to not use ngbtypeahead, we can't stop the user from focusing on a disabled org. We can detect it after they do, but we can't prevent it.

I'll work on disabling the place hold button when a disabled org is selected on this specific screen. We'll have to double check the results everywhere the org-select.component is used, which is a source of tech debt, but as I understand a redesign of the org-selector is on the horizon anyway.

[Stephanie Leary (stephanieleary)]

Mike and I ran into similar issues with the keyboard interactions while working on the upcoming combobox-based MARC editor, and we have some changes in that branch that might make it easier to add some logic around disabled entries. That will be forthcoming early next year.

I have been sorely tempted to rewrite combobox entirely using the demo at the end of this article: https://adamsilver.io/blog/building-an-accessible-autocomplete-control/

... but we use it for an awful lot of things, some of which I'm not totally familiar with yet.

The org selector's labeling logic has changed quite a bit in the last few weeks, but I don't plan on making further changes to it unless something else comes up.

[Steven Mayo (stmayo)]

Added a check to the holds screen in the staff client that disables the button when an invalid pickup lib is selected. Not completely happy with this since it still allows the user to perform an invalid action (bad UX, grr!) but it should stop the invalid holds from getting placed at least.

Also should modify the org selector to show as invalid when selecting an invalid pickup lib.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/smayo/lp2000270-Staff_Catalog_Allows_Invalid_Hold_Pickup_Locations

Will still use pullrequest tag because the behavior is better than before but I'll want to revisit this once the the keyboard interaction workarounds are made public. Perhaps as a different bug?

[Andrea Neiman (aneiman)]

Does the thing with no apparent explosions! I agree, it's an improvement over current behavior and thus I'm giving it a signoff.

Tested on terran-main.gapines.org for BSW March 2024.

I consent to signing off on this code with my name, Andrea Buntz Neiman, and my email, <email address hidden>.

[Michele Morgan (mmorgan)]

Works as advertised, a definite improvement over current behavior.

Pushed to main and rel_3_12. It did not merge cleanly to rel_3_11, so I have not pushed it there.

Thanks Steven and Andrea!

[Galen Charlton (gmc)]

Pushed a follow-up to main and rel_3_12 to fix lint and to mark the area where there is uncertainly about proper integration with Angular Forms with a FIXME.

Michele, you also did not include your sign-off when you committed. EDIT: Apologies, I see it now.