[Koha-bugs] [Bug 24194] Add system preference to disable the use of expiration dates for holds

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24194

--- Comment #17 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
Comment on attachment 110127
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110127
Bug 24194: Add DisableReserveExpiration system preference to hide expiration
date options for reserves

Review of attachment 110127:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=24194&attachment=110127)
-----------------------------------------------------------------

I had to fix a small conflict in the tests so I will upload an updated patch.
Tests pass :)

(In reply to David Nind from comment #16)
> - Everywhere else in the UI it refers to holds rather than reserves: should
> the system preference name by HideHoldExpiration instead?

1) I think it would be nice to rename it at this stage, but as you can see
below all the other prefs have Reserves in it too... so it might make it harder
to find. But in the pref description we should switch from reserves to holds.

> - For a patron, under Details > Holds the Expiration column is still shown.
> Should this be the case? (I'm not sure whether this is used for other
> holds/reserves things).

2) Hm, I think this question highlights the thing that is not clear here for me
about the intention of this patch and what the library has asked for. The
expiration date field is (sadly) used for 2 different purposes in the life
cycle of a hold:

a) Before the hold is filled, the date is used as "not needed after" indicating
that the hold can be cancelled after a certain date if not filled.
b) After the hold is filled, the date is used for "pick up until" with a
possible penalty if you don't or the hold going to the next patron with a hold
after.

>From reading the code, it looks like we are trying to remove the use of the
expiration date altogether here (see change to set_waiting) - so both use
cases/features. In this case I think more changes and tests would be needed:

- Hide it everywhere, expiration is still showing on the reserves page, the
details and checkouts tabs in the patron account, etc. (see David's comment)
- Move the pref from the 'interface' section to 'hold policy' so it appears
with the other prefs relying on the expiration date.
- Make the pref description a bit more explicit on its effects.
- Add notes to the system preferences relying on the hold expiration date:
ExpireReservesMaxPickUpDelay, ExpireReservesMaxPickUpDelayCharge, 
ExpireReservesOnHolidays, ReservesMaxPickUpDelay(?)
- Make sure misc/cronjobs/holds/cancel_expired_holds.pl doesn't cancel holds
once the pref has been switched.

Personally I'd like it a little better if we dealt with a) and b) separately,
starting with a) here. Both are separate features and mixing them in the db is
not good design (see bug 21729).

Aleisha, can you clarify?

3) There is a fail in the QA script:

 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
   FAIL   valid_template
                Attempt to reload Koha/Template/Plugin/Branches.pm aborted.
Compilation failed in require at
/usr/lib/x86_64-linux-gnu/perl5/5.24/Template/Plugins.pm line 206.

::: C4/Reserves.pm
@@ +2027,4 @@
>                             ->update({ priority => \'priority + 1' }, { no_triggers => 1 });
>  
>      ## Fix up the currently waiting reserve
> +    if ( C4::Context->preference('DisableReserveExpiration') ){

4) I think this is actually a general bug here - bug 21729 

On setting a hold to waiting, the expiration date changes from "not needed
after" to "pick up before". So when we revert the waiting status, we'd want to
reset to the date the patron chose as "not needed after", but it's already been
lost/overwritten. :(

Should it maybe always be set to undef right now?

:::
installer/data/mysql/atomicupdate/bug24194-add-DisableReserveExpiration-syspref.perl
@@ +1,3 @@
> +$DBversion = 'XXX';  # will be replaced by the RM
> +if( CheckVersion( $DBversion ) ) {
> +    $dbh->do(q{ INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES ( "DisableReserveExpiration", 0, NULL, "Disable the use of expiration date in reserves module.", "YesNo" ) });

Please make the reserves holds :)

::: installer/data/mysql/sysprefs.sql
@@ +163,4 @@
>  ('DefaultToLoggedInLibraryNoticesSlips',  '0', NULL ,  'If enabled,slips and notices editor will default to the logged in library''s rules, rather than the ''all libraries'' rules.',  'YesNo'),
>  ('DefaultToLoggedInLibraryOverdueTriggers',  '0', NULL ,  'If enabled, overdue status triggers editor will default to the logged in library''s rules, rather than the ''default'' rules.',  'YesNo'),
>  ('delimiter',';',';|tabulation|,|/|\\|#||','Define the default separator character for exporting reports','Choice'),
> +('DisableReserveExpiration', 0, NULL, 'Disable the use of expiration date in reserves module.', 'YesNo'),

Same here.

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
@@ +166,5 @@
> +            - pref: DisableReserveExpiration
> +              choices:
> +                  yes: Disable
> +                  no: "Don't disable"
> +            - the use of expiration dates in reserves.

And here.

-- 
You are receiving this mail because:
You are watching all bug changes.