Web client: Show Clearable holds list shows holds that can not be Cleared with the "Clear these holds" process.

[Dale Rigney]

Tested on Evergreen 3.2 and 3.1.10

When you click on the Holds shelf "Show Clearable Holds" tab the holds that show up include holds where the shelf_expire_time < now(). The xul client only shows holds with a shelf_expire_time < 'today'. The clear holds process only looks at holds where the shelf_expire_time < 'today'. The end results is the Show clearable Holds tab in the web client is showing holds that are not "Clearable" with the "Clear these Holds" Process. To test you can do the following:

1) Find a hold where the shelf_expire_time is earlier in the day or create one by setting the shelf_expire_time = now().
2) in the web client go to Circulation -> Holds Shelf and click on the "Show Clear Holds". You should be able to see the hold there.
3) in the xul client go to Circulation -> Browse Hold shelf and click on the View Clearable holds box. You will not see the hold listed there.
4) In the Web client click on the box for the hold in question and click on the "Clear These Holds" button.
5) Check in the item you will see the "This item should be routed to the Public Holds Shelf" window pop up for the same hold in question showing the hold was not cleared.

The web client should only show holds that are expired the day before the process is ran.

[Steve Callender (stevecallender)]

Fix is here,

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/Callender/hold_clear_list_shelf_expire_date

This change pre-loads the hold data with holds that expired less than today, not less than datetime today in order to match what the actual clear hold shelf function does. This will prevent holds from showing that won't actually clear.

It looks like this issue first showed up in 3.1.11 so it can be backported to anything above that release.

[Mike Rylander (mrylander)]

Hi Steve, and good work finding the place to change.

I think the change will need to be more complicated. One of the possible use cases right now is the ability to have very short hold shelf expire times -- think of a 4-hour window to come get some high-demand object from the desk.

We can't really base that on anything circ related, such as fine interval, since the circ doesn't exist yet, so I think the best option is probably a YAOUS that indicates whether the shelf lifetime is date-granular or precise. Then the library can decide. We already pull two other org settings for use in this query, so a third shouldn't materially impact performance.

There's also the opportunity to provide a tiny optimization by using a literal in the query ('today'::timestamptz or 'now'::timestamptz for date-granular or precise, respectively) rather than functions.

Thoughts?

[Josh Stompro (u-launchpad-stompro-org)]

EG 3.3.4 - just had this behavior reported today. The shelf expire report just happened to have been run after the time the pull list items had been checked in 10 days ago. "None of these expired holds will clear today Josh!"

If one of the goals is to allow shorter expiration periods, shouldn't we be changing the 'today' in
https://git.evergreen-ils.org/?p=Evergreen.git;a=blob;f=Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm;hb=HEAD#l2215

to 'now' so that all holds that have expired get cleared?

I wonder if a better spot to control precise vs day granularity is when the shelf_expire_time gets set, that way the decision doesn't need to be checked in two spots, one for view clearable holds and one for clearing the holds.

The set_hold_shelf_expire_time seems like a good spot to check an org unit setting. How about 'circ.holds.shelf_expire_push_end' "Push the shelf expire date to the end of the day it falls on. If not set, shelf_expire_time will be set precisely based on the default shelf expire interval". Set to true by default to match xul client behavior.

https://git.evergreen-ils.org/?p=Evergreen.git;a=blob;f=Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm;hb=HEAD#l1124

There is already an example there for pushing the expire time to the end of the last closed day.

This would allow locations that like the old behavior of giving customers the whole day that their holds expire to make it in, to continue with that behavior. If an organization had a special org unit with a hold shelf for high demand items and they only wanted to give someone 4 hours to pickup, and they cleared the shelf every hour, then that should work fine also.

If this seems like an ok approach, I can work on it.

Josh

[John Amundson (jamundson)]

I think the library setting makes sense here, with the default behavior following the XUL.

The underlying need to me, though, it just making sure the holds that do appear on the clear list can be cleared.

Regardless of if the setting is set to precise or date, holds need to be clearable with the "Clear These Holds" button on the hold shelf AND the Checkin Modifier "Clear Holds Shelf"

[Josh Stompro (u-launchpad-stompro-org)]

I just noticed that the web client - (3.3.4) holds shelf - edit holds dates doesn't allow specifying the exact time, just the day. So if a location ever does want to use short hold shelf expire intervals, that may need to be adjusted also.

And I also noticed that bug 1829295 involves making some assumptions about hold shelf expire intervals being specified in days, which could conflict with a goal to allow short hold shelf intervals.
Josh

[Benjamin Murphy (benjamin.murphy)]

We've rolled out the patch for this for our instance of Evergreen. Is this targeted for any forthcoming versions of Evergreen?

[Terran McCanna (tmccanna)]

Adding the needsdiscussion tag since it sounds like it may need some fine tuning.

[Josh Stompro (u-launchpad-stompro-org)]

Benjamin, it hasn't been targeted since there is still discussion going on about how exactly to solve it.

Summary:

Steve's fix changes which holds are shown for the clear hold shelf function, which matches the XUL behavior.

Mike brought up the fact that some locations may want a shorter than a day expire interval, which wouldn't work with just going back to the old logic.

Maybe this should be committed with Steve's fix, and a new bug created for allowing shorter than a day hold shelf expire intervals, since that seems like a new feature, while Steve's fix seems like a bug fix to me.

Josh

[Josh Stompro (u-launchpad-stompro-org)]

Just in case there is consensus to apply Steve's fix now, here is a signoff branch.

user/stompro/lp1819540_hold_clear_list_shelf_expire_date_signoff

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

I added testing steps, and changed the date(now()) to 'today'::timestamptz

When I run explain analyze the 'today'::timestamptz does seem faster.

egdbprod=# explain analyze select date(now());
                                     QUERY PLAN
------------------------------------------------------------------------------------
 Result (cost=0.00..0.01 rows=1 width=0) (actual time=0.009..0.009 rows=1 loops=1)
 Planning time: 0.018 ms
 Execution time: 0.023 ms
(3 rows)

egdbprod=# explain analyze select 'today'::timestamptz;
                                     QUERY PLAN
------------------------------------------------------------------------------------
 Result (cost=0.00..0.01 rows=1 width=0) (actual time=0.001..0.001 rows=1 loops=1)
 Planning time: 0.009 ms
 Execution time: 0.009 ms
(3 rows)

Josh

[Jennifer Pringle (jpringle-u)]

+1 to Josh's suggestion "Maybe this should be committed with Steve's fix, and a new bug created for allowing shorter than a day hold shelf expire intervals, since that seems like a new feature, while Steve's fix seems like a bug fix to me."

[John Amundson (jamundson)]

I also agree with Josh. Fix the bug now, and try to improve it later. Our libraries find this bug quite frustrating.

[Deborah Luchenbill (deborah)]

+1 to Josh's suggestion, too.

[Jason Stephenson (jstephenson)]

I'm removing the needsdiscussion tag, assigning this bug to John Amundson who will look at it next week, and targeting this bug for 3.3, 3.4, and 3.5 branches.

I agree that this is a bug fix and adding less than daily hold shelf expire times is a new feature deserving of its own bug.

[John Amundson (jamundson)]

This is looking good to me on my test server, as well. I'll add my signoff to the mix.

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

[Jason Stephenson (jstephenson)]

Thanks, everyone. I have finally gotten around to pushing this to master, rel_3_5, rel_3_4, and rel_3_3.