Shelf expire date doesn't respect closed dates

[James Fournie]

Shelf expire date has some closed-date handling code here:
https://github.com/evergreen-library-system/Evergreen/blob/3e3ec8ec68bdc49eb43a6dedc187c3c90a3813cc/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm#L1112

However, it appears both from looking at the code and from testing that this code will only bump the expire date if the expire date falls on a closed date.

I would expect this to exclude all closed dates from the count.

For example, expire time = 3 days, I'm closed Saturday, Sunday. Item is captured on Friday. The expire date will become Monday. I would expect it to be wednesday, as Saturday and Sunday the library is closed so there's no reasonable opportunity to pick up the hold.

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

I think this is a duplicate of a bug I filed 2 years ago. LP#1665395 "Wishlist: Default hold shelf expire interval count open days only".

Also somewhat related is LP#1665400 "Hold Targeter - Respect weekly schedule as well as closed holidays."

It seems like there could be some common code for handling open days that includes an org unit's weekly schedule along with closed days from the closed dates editor.

[Terran McCanna (tmccanna)]

I'm marking this as Wishlist with needsdiscussion since it would be a significant change in behavior from calendar days to open business days.

[Jeff Godin (jgodin)]

This may indeed be a duplicate of bug 1665395, as Josh stated above, though I'm not certain I follow the description in that bug enough to be certain.

We'd have strong opposition to this change unless it was behind something like an org unit setting.

[Terran McCanna (tmccanna)]

+1 to an org unit setting.

[Bill Erickson (berick)]

As a heads up, I have a big chunk of code running locally that addresses this bug and bug 1665395. There are some assumptions and TODO's that will need addressing and it will need some general cleanup, but I can at least get it published in case anyone wants to run with it in the meantime...

[Bill Erickson (berick)]

Lightly cleaned patch from internal code:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1829295-shelf-expire-honors-closed-and-hours

The main assumption that needs some consideration is that hold shelf expire intervals are assumed to be defined in days (not hours, weeks, etc.). We probably need a separate setting which explicitly defines the expire interval as a day count (e.g. circ.holds.default_shelf_expire_days). This simplified the code a good bit, and I'm guessing this is the most common use case.

In short, if an org unit has any open hours on a given day (starting 'tomorrow'), it counts as "1 day" of the expire interval.

[Jennifer Weston (jweston)]

Bill - Does the patch from 2019 use an org unit setting? It would be great to revisit this discussion in 2021 to provide the option to libraries (especially with days of operation changing frequently as we experienced in 2020).

[Bill Erickson (berick)]

Jennifer, it currently uses the existing 'circ.holds.default_shelf_expire_interval' setting, however I'd like to add a new org setting that explicitly stores the value as a number of days instead of an interval value -- or maybe teach the code to map the interval value to a number of days. Either way, a simple addition to the code.

[Jennifer Weston (jweston)]

Thanks, Bill. Another question Note #6 says "In short, if an org unit has any open hours on a given day (starting 'tomorrow'), it counts as "1 day" of the expire interval." Is that actually how the patch will work or is it a restatement of this wishlist discussion? There has been some discussion about calculating expiration with calendar days versus open business days and interest in being able to allow the library to choose their preferred method. We've had multiple inquiries lately asking to count only open days (in 3.4 which is using calendar days) so I'm trying to provide an update on the ongoing discussions.

[Bill Erickson (berick)]

Hi Jennifer, the note describes how the patch works. If a branch has any hours of operation for a given day, regardless of the duration, it counts as 1 day of the configured days (assuming no closed dates for that day).

[Jennifer Weston (jweston)]

Thanks again, Bill.

[Jennifer Weston (jweston)]

Confirming patch was tested with successful results by multiple libraries on 3.4. Patch works as expected to count only open days. Moving forward with plans to apply patch later this month with same libraries with upgrade to 3.6. Noting the patch replaces the calculation using calendar days and there is not yet an option to choose a preferred method using an org unit setting.

[Jason Boyer (jboyer)]

Any opinions on merging this and waiting to see if anyone actually wants a setting to disable it? I would think if you want to offer patrons X days to pick up a hold it would be best if you were open all X days.

[Terran McCanna (tmccanna)]

PINES has a few corner cases where a service outlet might only be open one or two days per week, so it would be nice to disable it for them to avoid having a book sitting on their hold shelf for a month or two, but IMO that shouldn't delay moving this forward. That could be a new wish list request.

[Jeff Godin (jgodin)]

From the four people who have noted a need for this behavior change to be optional, does an org unit setting seem appropriate here?

That seems a better fit than a global/internal flag here.

An org unit setting as a boolean for opting in to the "old" (current) behavior, documented in the release notes? Sites upgrading would gain the new functionality unless they took action to prevent it.

Since this is something we would need to remove locally if it were committed as-is without a setting to disable, I'm willing to put a branch together so that this can be merged.

Does the above sound reasonable?

[Terran McCanna (tmccanna)]

+1 to an org unit setting