Issues Placing Holds from the Patron Record, Angular and AngularJS

Branch for testing: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbriem/lp1996818_holds_from_patron_record

This clears the patron hold target cookie when navigating from the staff/catalog route, when the window is closed, and when holds are successfully placed.

This also changes the AngularJS patron interface to set the same session cookie as the Angular interface.

The comments on bug 1949226 mentioned "same search" as the ideal persistence, but that may be too restrictive if staff need multiple searches to find an item or if they want to place multiple holds using the bucket interface. I think successfully placing a hold might be a better proxy for a "session" and might be more consistent given, when not initiating a hold from the patron interface and instead searching for a patron in the catalog, that patron's data is cleared after holds are successfully placed.

We've applied this to 3.10 with production data and it works great!

My sign off is at: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mccanna/lp1996818_holds_from_patron_record_signoff

I've reviewed this patch and have a couple reservations:

[1] There are issues if you start a search-to-hold, then open individual titles in separate tabs. Consider:

a. Select a patron and click the place hold button.
b. Catalog component comes up indicating that you're intending to place a hold.
c. Do a search.
d. Open one of the results in another tab, then realize that it's not the title you're looking for, then close it.
e. As a consequence, the session cookie gets deleted but the catalog service in the original table is still in search-for-hold mode - and you can in fact place a hold.
f. But if you instead open a second title from the result set in a new tab, that new tab is not in search-to-hold mode because the cookie is gone.

That's inconsistent and hard to explain to a staff user, but arguably is still an improvement over the current state of affairs. Doing more work to coordinate between tabs might smooth this out, but need not hold up some version of this patch.

However:

[2] I'm not sure I agree that placing a hold should end the search-to-hold session, as it prevents somebody from readily placing multiple holds for a patron in a single session. Consequently, at the moment I'd be more in favor of a version of the patch that omits the change to hold.component.ts.

Now that we've had this in place for a little while, we have had staff complaining that it loses the patron's info after placing the first hold. So, while retaining it for one hold is better than none, they would like it to be retained longer as in Galen's point [2] above.

Branch for testing: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbriem/lp1996818_broadcast_patron_hold_target

AngularJS Place Hold button sets the same cookie as Angular. This branch also clears the cookie and broadcasts to all catalog tabs to remove the hold target when:
- the Clear button for the hold target is pressed
- the hold interface loads a different patron
- a different Angular route loads
- AngularJS app starts (left the Angular context)

When a catalog tab is closed, the cookie clears and a broadcast is sent in case there are any open catalog tabs, so those tabs can restore it.

If you are testing, in addition to Angular ng build you'll need to do npm run build for AngularJS as well.

As an alternative, I also force pushed my old branch to remove the hold.component.ts aspect as Galen suggested, in case I've over-engineered this and there's still disagreement on how this should work.

I've had a difficult time wrapping my head around the parameters of this feature and still feel it would be better accomplished from the reverse, and would point anyone interested in simply bringing up the previous patron from the hold interface to bug 2009725.

We're testing this at CW MARS. Even if we sign off, I'll hold off on pushing this to see if there is any discussion about Dan's comment #5.

I tried cherry-picking Dan's latest patch on a server runnging 3.10.2 and it does not build. I get the following errors while builing the Angular staff client:

Error: src/app/staff/catalog/catalog.component.ts:5:18 - error TS2305: Module '"rxjs"' has no exported member 'takeUntil'.

5 import {Subject, takeUntil} from 'rxjs';
                   ~~~~~~~~~

Error: src/app/staff/catalog/catalog.service.ts:12:21 - error TS2305: Module '"rxjs"' has no exported member 'tap'.

12 import {Observable, tap} from 'rxjs';

It DOES build on a server with the lastest main Evergreen branch.

I know this bug has not been targeted, but we do see it on 3.10, so it looks like we'll have to update the rxjs package on 3.10 if this fix goes in.

I treid updating rxjs on a 3.10.2 server with this branch, and I get different errors now:

Error: src/app/staff/acq/provider/acq-provider.component.ts:161:24 - error TS2554: Expected 1 arguments, but got 0.

161 this.destroyed.next();
                           ~~~~~~

  node_modules/rxjs/dist/types/internal/Subject.d.ts:32:10
    32 next(value: T): void;
                ~~~~~~~~
    An argument for 'value' was not provided.

Error: src/app/staff/acq/search/acq-search.component.ts:135:24 - error TS2554: Expected 1 arguments, but got 0.

135 this.destroyed.next();
                           ~~~~~~

  node_modules/rxjs/dist/types/internal/Subject.d.ts:32:10
    32 next(value: T): void;
                ~~~~~~~~
    An argument for 'value' was not provided.

Error: src/app/staff/share/op-change/op-change.component.ts:111:15 - error TS2345: Argument of type '(_: any) => Promise<any>' is not assignable to parameter of type 'TeardownLogic'.
  Type '(_: any) => Promise<any>' is not assignable to type '() => void'.

111 ).add(_ => this.auth.undoOpChange());
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It looks like this patch will not work for Evergreen 3.10. I have not tried it with 3.11.

I tested this on a server set up by Jason running the latest Evergreen main branch, and I'm giving it my tentative sign off:

I have tested this code and consent to signing off on it with my name, John Amundson, and my email address, <email address hidden>.

From a staff perspective, the feature works well. The patron's information was retained when placing multiple holds in the same tab and even when opening records in multiple tabs (which is an improvement even from the previous behavior). The patron's information only seems to clear when performing one of the actions listed in Dan's comment.

Based on Dan's and Jason's comments, though, others may want to test or chime in.

I also tried it and it worked for me on main. I'm concerned about it not applying to 3.10 and the fix for that is also not obvious.

It does look like it will work for 3.11, since package.json is the same for rel_3_11 and main for now.

Since John has signed off, I'll unassign myself from the bug, and let others have a look.

Jason & John, thanks for taking time to look at this. I should've tested on 3.10 instead of just main.

I added another commit to the tip of https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbriem/lp1996818_broadcast_patron_hold_target

This imports from rxjs/operators so it will work with both RxJS 6 & 7 and, while I'm not seeing the Subject error, there's no reason it has to be void, so it's now null and passes in a null parameter.

If operators are removed from rxjs/operators some version down the line I guess we'll have to refactor that throughout the application anyway.

Thanks for the update, Dan! I'll give it another look on Monday. We'd really like to see a fix for this make it into 3.10.

We have had some time to review Dan's updated branch at CW MARS yesterday and today. It works on 3.10.2 as well as master thanks to Dan's latest updates.

I've pushed a signoff branch with both John Amundson's and my signoff added to

user/dyrcona/lp1996818_broadcast_patron_hold_target-signoff (https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1996818_broadcast_patron_hold_target-
signoff)

I'm not pushing this to the main repsository right now in case anyone wants to discuss the approach taken as mentioned by Dan in comment #11. I have no issues with it.

The code could also stadnd more scrutiny. Just because it works for CW MARS doesn't mean it necessarily works for eveyone.

Tested and pushed down to rel_3_10. Thanks, Dan and Jason!