https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24194
--- Comment #17 from Katrin Fischer <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
