Description
Aleisha Amohia
2019-12-09 20:23:41 UTC
Created attachment 96120 [details] [review] Bug 24194: Add HideReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In a third tab, open the intranet and set HideReserveExpiration system preference to 'Hide' 5) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 6) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. Sponsored-by: Horowhenua Library Trust Created attachment 96121 [details] [review] Bug 24194: Add HideReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In a third tab, open the intranet and set HideReserveExpiration system preference to 'Hide' 5) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 6) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. Sponsored-by: Horowhenua Library Trust Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Should not it be hidden locally in CSS or JS instead? Created attachment 98048 [details] [review] Bug 24194: Add HideReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set HideReserveExpiration system preference to 'Hide' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. Sponsored-by: Horowhenua Library Trust (In reply to Jonathan Druart from comment #3) > Should not it be hidden locally in CSS or JS instead? It turns out the library wants to not use expiration dates for reserves at all, so the latest patch now includes not setting an expiration date when setting a reserve as waiting, and nullifying an expiration date when reverting waiting status. Back to needs signoff, with a new test plan. (In reply to Aleisha Amohia from comment #5) > (In reply to Jonathan Druart from comment #3) > > Should not it be hidden locally in CSS or JS instead? > > It turns out the library wants to not use expiration dates for reserves at > all, so the latest patch now includes not setting an expiration date when > setting a reserve as waiting, and nullifying an expiration date when > reverting waiting status. Back to needs signoff, with a new test plan. Hi Aleisha, can you please update the bug title? Sounds like quite a different thing now! (In reply to Katrin Fischer from comment #6) > (In reply to Aleisha Amohia from comment #5) > > (In reply to Jonathan Druart from comment #3) > > > Should not it be hidden locally in CSS or JS instead? > > > > It turns out the library wants to not use expiration dates for reserves at > > all, so the latest patch now includes not setting an expiration date when > > setting a reserve as waiting, and nullifying an expiration date when > > reverting waiting status. Back to needs signoff, with a new test plan. > > Hi Aleisha, can you please update the bug title? Sounds like quite a > different thing now! Fair enough Katrin, have updated! Created attachment 98368 [details] [review] Bug 24194: Add HideReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set HideReserveExpiration system preference to 'Hide' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> I worked through the test plan and I think everything works as expected. But it may require sign-off or review from someone who is more familiar with holds/reserves! It's much more than "hide"/"show". I think we should rename the pref and update its description to explain a bit more this new enhancement. Created attachment 100927 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set DisableReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Does not apply. Needs a rebase I don't see any unit test that covers changes in the subs RevertWaitingStatus andset_waiting. I think t/db_dependent/Reserves.t and t/db_dependent/Hold.t should be updated to take into account the new system preference. Created attachment 110082 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set DisableReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Created attachment 110127 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set DisableReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Hi Aleisha. Everything worked for me a per the test plan, and the tests passed, so I have signed off. Just a couple of things I noted, which are probably more for the QA team than me 8-) : - Everywhere else in the UI it refers to holds rather than reserves: should the system preference name by HideHoldExpiration instead? - 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). David Comment on attachment 110127 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves Review of attachment 110127 [details] [review]: ----------------------------------------------------------------- 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. Created attachment 111443 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set DisableReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 113284 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set DisableReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 113285 [details] [review] Bug 24914: (follow-up) Clarifying sysprefs, hiding columns, and more - change syspref description to say 'holds' instead of reserves - clarify descriptions of related sysprefs - hide expiration column on reserves page - move syspref to 'hold policy' section - fix cancel_expired_holds.pl cron to consider syspref - set expirationdate to undef always Regarding to terminology https://wiki.koha-community.org/wiki/Terminology We prefer hold instead of reserve. New preference should be named 'DisableHoldExpiration' no ? (In reply to Fridolin Somers from comment #21) > Regarding to terminology https://wiki.koha-community.org/wiki/Terminology > We prefer hold instead of reserve. > > New preference should be named 'DisableHoldExpiration' no ? Hi Fridolin, Katrin has already answered this in the comment above: 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. Ok super ;) Patch no longer applies 8-( Created attachment 116626 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set DisableReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 116627 [details] [review] Bug 24914: (follow-up) Clarifying sysprefs, hiding columns, and more - change syspref description to say 'holds' instead of reserves - clarify descriptions of related sysprefs - hide expiration column on reserves page - move syspref to 'hold policy' section - fix cancel_expired_holds.pl cron to consider syspref - set expirationdate to undef always I was looking at this and getting ready to submit a follow-up with some additional corrections (Holds history page, the holds tab on the checkout page, etc) but I find that it feels really convoluted to work with a system preference which is "negative." DisableReserveExpiration equals 1 to turn something off rather than turn something on. I know this has been through a lot of revisions already but can it be switched to be "ReserveExpiration" and have it be enabled by default? Created attachment 117016 [details] [review] Bug 24194: (follow-up) Renaming syspref to ReserveExpiration Created attachment 117085 [details] [review] Bug 24194: (follow-up) Correct some missing instances This patch fixes some instances where hold expiration dates are set or referenced. To test, apply the patch and test the following pages with the ReserveExpiration preference both on and off to confirm that expiration dates are shown or hidden correctly. Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Created attachment 117089 [details] [review] Bug 24194: (follow-up) Link references to new preference As was done in Bug 27263, references to one system preference in another system preference's description should be a link back to the mentioned preference. To test, apply the patch go to Administration -> Global system preferences. - Search for "ReserveExpiration." - Besides the preference itself, you should get four other preferences which have mentions of "ReserveExpiration" in their descriptions. - Confirm that each mention in the description is a link to a search for "ReserveExpiration." Created attachment 117097 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set DisableReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Created attachment 117098 [details] [review] Bug 24914: (follow-up) Clarifying sysprefs, hiding columns, and more - change syspref description to say 'holds' instead of reserves - clarify descriptions of related sysprefs - hide expiration column on reserves page - move syspref to 'hold policy' section - fix cancel_expired_holds.pl cron to consider syspref - set expirationdate to undef always https://bugs.koha-community.org/show_bug.cgi?id=24194 Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 117099 [details] [review] Bug 24194: (follow-up) Renaming syspref to ReserveExpiration Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 117100 [details] [review] Bug 24194: (follow-up) Correct some missing instances This patch fixes some instances where hold expiration dates are set or referenced. To test, apply the patch and test the following pages with the ReserveExpiration preference both on and off to confirm that expiration dates are shown or hidden correctly. Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 117101 [details] [review] Bug 24194: (follow-up) Link references to new preference As was done in Bug 27263, references to one system preference in another system preference's description should be a link back to the mentioned preference. To test, apply the patch go to Administration -> Global system preferences. - Search for "ReserveExpiration." - Besides the preference itself, you should get four other preferences which have mentions of "ReserveExpiration" in their descriptions. - Confirm that each mention in the description is a link to a search for "ReserveExpiration." Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> ReserveExpiration should be moved out of set_waiting() method because it makes using set_waiting method really confusing because you are really expecting it to only set the waiting status and not do much more, documentation and testing of the function becomes also more difficult. The best way to do this would be to block adding expiration date already when creating a new hold. So please add the necessary code to OPAC, Staff and REST. For OPAC and Staff I see there is already some code, REST is missing completely. Also if I'm not mistaken the reason you added ReserveExpiration to set_waiting() is because of ReservesMaxPickUpDelay not working really well – what you could do here is do a DB upgrade so that ReservesMaxPickUpDelay value '' => 0 so then when ReservesMaxPickUpDelay is not an integer value it would simply not add expiration date. (In reply to Joonas Kylmälä from comment #36) > Also if I'm not mistaken the reason you added ReserveExpiration > to set_waiting() is because of ReservesMaxPickUpDelay not working really > well – what you could do here is do a DB upgrade so that > ReservesMaxPickUpDelay value '' => 0 so then when ReservesMaxPickUpDelay is > not an integer value it would simply not add expiration date. Alternatively we could probably check if ExpireReservesMaxPickUpDelay is enabled and only then add the expiration date. And for added robustness, you might wanna throw exception if somebody tries to set the expirationdate field for Koha::Hold. (In reply to Joonas Kylmälä from comment #36) > ReserveExpiration should be moved out of set_waiting() method because it > makes using set_waiting method really confusing because you are really > expecting it to only set the waiting status and not do much more, > documentation and testing of the function becomes also more difficult. I'm not sure I understand what you mean here, expiration date logic is already in the set_waiting method before my patch. I think moving that logic out is beyond the scope of this work. > The best way to do this would be to block adding expiration date already when > creating a new hold. So please add the necessary code to OPAC, Staff and > REST. For OPAC and Staff I see there is already some code, REST is missing > completely. What code is missing for OPAC and staff? I will add it to REST in a follow-up. > Also if I'm not mistaken the reason you added ReserveExpiration > to set_waiting() is because of ReservesMaxPickUpDelay not working really > well – what you could do here is do a DB upgrade so that > ReservesMaxPickUpDelay value '' => 0 so then when ReservesMaxPickUpDelay is > not an integer value it would simply not add expiration date. I added it to set_waiting because the code already exists there - when we set a hold as waiting it may have a specified shelf time for awaiting pickup. Created attachment 118258 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set DisableReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 118259 [details] [review] Bug 24914: (follow-up) Clarifying sysprefs, hiding columns, and more - change syspref description to say 'holds' instead of reserves - clarify descriptions of related sysprefs - hide expiration column on reserves page - move syspref to 'hold policy' section - fix cancel_expired_holds.pl cron to consider syspref - set expirationdate to undef always https://bugs.koha-community.org/show_bug.cgi?id=24194 Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118260 [details] [review] Bug 24194: (follow-up) Renaming syspref to ReserveExpiration Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 118261 [details] [review] Bug 24194: (follow-up) Correct some missing instances This patch fixes some instances where hold expiration dates are set or referenced. To test, apply the patch and test the following pages with the ReserveExpiration preference both on and off to confirm that expiration dates are shown or hidden correctly. Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 118262 [details] [review] Bug 24194: (follow-up) Link references to new preference As was done in Bug 27263, references to one system preference in another system preference's description should be a link back to the mentioned preference. To test, apply the patch go to Administration -> Global system preferences. - Search for "ReserveExpiration." - Besides the preference itself, you should get four other preferences which have mentions of "ReserveExpiration" in their descriptions. - Confirm that each mention in the description is a link to a search for "ReserveExpiration." Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 118263 [details] [review] Bug 24194: (follow-up) Use ReservesExpiration in other places (In reply to Aleisha Amohia from comment #39) > (In reply to Joonas Kylmälä from comment #36) > > ReserveExpiration should be moved out of set_waiting() method because it > > makes using set_waiting method really confusing because you are really > > expecting it to only set the waiting status and not do much more, > > documentation and testing of the function becomes also more difficult. > > I'm not sure I understand what you mean here, expiration date logic is > already in the set_waiting method before my patch. I think moving that logic > out is beyond the scope of this work. I mean just moving the ReserveExpiration syspref code you added. The rest of the code is already in OPAC/INTRANET/REST (because that is required due the nature of it) so having this check there wouldn't add any extra code. Also please see below for more explanation. Also note that this DB migration would be needed then: > > ReservesMaxPickUpDelay value '' => 0 so then when ReservesMaxPickUpDelay is > > not an integer value it would simply not add expiration date. > > The best way to do this would be to block adding expiration date already when > > creating a new hold. So please add the necessary code to OPAC, Staff and > > REST. For OPAC and Staff I see there is already some code, REST is missing > > completely. > > What code is missing for OPAC and staff? I will add it to REST in a > follow-up. You had the code to hide the HTML but if someone submits the request directly it will work but the expiration date is silently dropped. I noticed you handled it now this way as well in the REST API in the follow-up, IMHO there should be error given (e.g. raise the previously mentioned exception in Koha::Hold). Silently discarding data is going to lead to surprises for our API users especially! Marking "Failed QA" based on comment 46. Created attachment 128301 [details] [review] Bug 24194: Add DisableReserveExpiration system preference to hide expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set DisableReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 128302 [details] [review] Bug 24914: (follow-up) Clarifying sysprefs, hiding columns, and more - change syspref description to say 'holds' instead of reserves - clarify descriptions of related sysprefs - hide expiration column on reserves page - move syspref to 'hold policy' section - fix cancel_expired_holds.pl cron to consider syspref - set expirationdate to undef always https://bugs.koha-community.org/show_bug.cgi?id=24194 Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 128303 [details] [review] Bug 24194: (follow-up) Renaming syspref to ReserveExpiration Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 128304 [details] [review] Bug 24194: (follow-up) Correct some missing instances This patch fixes some instances where hold expiration dates are set or referenced. To test, apply the patch and test the following pages with the ReserveExpiration preference both on and off to confirm that expiration dates are shown or hidden correctly. Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 128305 [details] [review] Bug 24194: (follow-up) Link references to new preference As was done in Bug 27263, references to one system preference in another system preference's description should be a link back to the mentioned preference. To test, apply the patch go to Administration -> Global system preferences. - Search for "ReserveExpiration." - Besides the preference itself, you should get four other preferences which have mentions of "ReserveExpiration" in their descriptions. - Confirm that each mention in the description is a link to a search for "ReserveExpiration." Signed-off-by: David Nind <david@davidnind.com> https://bugs.koha-community.org/show_bug.cgi?id=24914 Signed-off-by: David Nind <david@davidnind.com> Created attachment 128306 [details] [review] Bug 24194: (follow-up) Use ReservesExpiration in other places Created attachment 128307 [details] [review] Bug 24194: (follow-up) Use ExpireReservesMaxPickUpDelay when setting hold waiting If ExpireReservesMaxPickUpDelay is disabled then we don't need to set an expiration date when setting a hold as waiting Created attachment 128308 [details] [review] Bug 24194: (follow-up) Throw an error if expiration date used Created attachment 140646 [details] [review] Bug 24194: Add ReserveExpiration system preference to disable expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set ReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t 12) Go to Admin -> System preferences and search for ReserveExpiration. Confirm related system preferences have a reference to ReserveExpiration and the links all work 13) Text the cancel_expired_holds.pl cronjob and confirm it works with ReserveExpiration enabled, and does nothing when disabled 14) Confirm all instances where hold expiration dates are set or referenced are shown or hidden correctly, depending on ReserveExpiration Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. - Circulation -> Holds awaiting pickup Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Created attachment 140647 [details] [review] Bug 24194: (follow-up) Throw an error if expiration date used Signed-off-by: Laura Escamilla <lauraescamilla@bywatersolutions.com> Created attachment 144076 [details] [review] Bug 24194: Add ReserveExpiration system preference to disable expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set ReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t 12) Go to Admin -> System preferences and search for ReserveExpiration. Confirm related system preferences have a reference to ReserveExpiration and the links all work 13) Text the cancel_expired_holds.pl cronjob and confirm it works with ReserveExpiration enabled, and does nothing when disabled 14) Confirm all instances where hold expiration dates are set or referenced are shown or hidden correctly, depending on ReserveExpiration Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. - Circulation -> Holds awaiting pickup Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Created attachment 147527 [details] [review] Bug 24194: Add ReserveExpiration system preference to disable expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set ReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t 12) Go to Admin -> System preferences and search for ReserveExpiration. Confirm related system preferences have a reference to ReserveExpiration and the links all work 13) Text the cancel_expired_holds.pl cronjob and confirm it works with ReserveExpiration enabled, and does nothing when disabled 14) Confirm all instances where hold expiration dates are set or referenced are shown or hidden correctly, depending on ReserveExpiration Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. - Circulation -> Holds awaiting pickup Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Created attachment 147528 [details] [review] Bug 24194: (follow-up) Throw an error if expiration date used Created attachment 150799 [details] [review] Bug 24194: Add ReserveExpiration system preference to disable expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set ReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t 12) Go to Admin -> System preferences and search for ReserveExpiration. Confirm related system preferences have a reference to ReserveExpiration and the links all work 13) Text the cancel_expired_holds.pl cronjob and confirm it works with ReserveExpiration enabled, and does nothing when disabled 14) Confirm all instances where hold expiration dates are set or referenced are shown or hidden correctly, depending on ReserveExpiration Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. - Circulation -> Holds awaiting pickup Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nicolas Giraud <nicolas.giraud@inlibro.com> Created attachment 150800 [details] [review] Bug 24194: (follow-up) Throw an error if expiration date used Signed-off-by: Nicolas Giraud <nicolas.giraud@inlibro.com> Hi Nicolas. Since you have signed off, you should change the status to Signed Off. That way you will get credit on the dashboard (https://dashboard.koha-community.org/). David Hi David, I've just modified it, sorry ! No problem! We all do this every now and again. Created attachment 159060 [details] [review] Bug 24194: Add ReserveExpiration system preference to disable expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set ReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t 12) Go to Admin -> System preferences and search for ReserveExpiration. Confirm related system preferences have a reference to ReserveExpiration and the links all work 13) Text the cancel_expired_holds.pl cronjob and confirm it works with ReserveExpiration enabled, and does nothing when disabled 14) Confirm all instances where hold expiration dates are set or referenced are shown or hidden correctly, depending on ReserveExpiration Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. - Circulation -> Holds awaiting pickup Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nicolas Giraud <nicolas.giraud@inlibro.com> Created attachment 159061 [details] [review] Bug 24194: (follow-up) Throw an error if expiration date used Signed-off-by: Nicolas Giraud <nicolas.giraud@inlibro.com> Created attachment 167796 [details] [review] Bug 24194: Add ReserveExpiration system preference to disable expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set ReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t 12) Go to Admin -> System preferences and search for ReserveExpiration. Confirm related system preferences have a reference to ReserveExpiration and the links all work 13) Text the cancel_expired_holds.pl cronjob and confirm it works with ReserveExpiration enabled, and does nothing when disabled 14) Confirm all instances where hold expiration dates are set or referenced are shown or hidden correctly, depending on ReserveExpiration Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. - Circulation -> Holds awaiting pickup Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nicolas Giraud <nicolas.giraud@inlibro.com> Created attachment 167797 [details] [review] Bug 24194: (follow-up) Throw an error if expiration date used Signed-off-by: Nicolas Giraud <nicolas.giraud@inlibro.com> I rebased the patches, but there are a lot of issues reported by the QA tool so I'm setting to 'Failed QA' Created attachment 169174 [details] [review] Bug 24194: Making the qa script happy Tidied up a bunch there are still a couple of perltidy complaints, but I am running out of time today so attaching this patch so it doesn't get lost No more test fails, or failing patterns Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Applying: Bug 24194: Add ReserveExpiration system preference to disable expiration date options for reserves Using index info to reconstruct a base tree... M C4/ILSDI/Services.pm M C4/Reserves.pm M Koha/Hold.pm M Koha/REST/V1/Holds.pm M installer/data/mysql/mandatory/sysprefs.sql M koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref M koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt M koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt M koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-print.tt M koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt M koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt M koha-tmpl/intranet-tmpl/prog/js/holds.js M koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt M t/db_dependent/Reserves.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Reserves.t CONFLICT (content): Merge conflict in t/db_dependent/Reserves.t Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc Auto-merging koha-tmpl/intranet-tmpl/prog/js/holds.js Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-print.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-print.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc Auto-merging installer/data/mysql/mandatory/sysprefs.sql Auto-merging Koha/REST/V1/Holds.pm Auto-merging Koha/Hold.pm Auto-merging C4/Reserves.pm Auto-merging C4/ILSDI/Services.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 24194: Add ReserveExpiration system preference to disable expiration date options for reserves Created attachment 174650 [details] [review] Bug 24194: Add ReserveExpiration system preference to disable expiration date options for reserves To test: 1) Update database 2) Go to place a hold on any biblio in the staff intranet and confirm you can see the 'Hold expires on date' field. 3) In another tab, go to place a hold on any biblio in the OPAC and confirm you can see the 'Hold not needed after' field as an option. 4) In yet another tab, open the staff intranet and place a reserve for a user. Check it in and set the reserve as waiting. Notice that an expiration date has now been generated for this reserve. 5) Attempt to check out the item you reserved to some other borrower. Revert waiting status. Notice that the expiration date that was generated remains. 6) Go to Administration -> system preferences and set ReserveExpiration system preference to 'Disable' 7) Refresh the hold request page in the intranet. Confirm the expiration date field disappears. 8) Refresh the hold request page in the OPAC. Confirm the expiration date field disappears. 9) Place another reserve. Check it in and set the reserve as waiting. Notice that no expiration date was generated for this reserve. 10) Attempt to check out the item you reserved to some other borrower. Revert waiting status. The expiration date should remain null. 11) Confirm tests pass t/db_dependent/Hold.t t/db_dependent/Reserves.t 12) Go to Admin -> System preferences and search for ReserveExpiration. Confirm related system preferences have a reference to ReserveExpiration and the links all work 13) Text the cancel_expired_holds.pl cronjob and confirm it works with ReserveExpiration enabled, and does nothing when disabled 14) Confirm all instances where hold expiration dates are set or referenced are shown or hidden correctly, depending on ReserveExpiration Pages in the staff interface: - Check out to a patron and check the holds tab - View patron details and check the holds tab - Patron hold history - Patron details -> Print -> Print summary - Place a hold on a title which already has at least one hold on it, check the table of existing holds. - Circulation -> Holds awaiting pickup Pages in the OPAC: - Log in to the OPAC as a patron with holds - Your summary -> Holds tab - Your holds history (OPACHoldsHistory must be enabled). Sponsored-by: Horowhenua Library Trust Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nicolas Giraud <nicolas.giraud@inlibro.com> Created attachment 174651 [details] [review] Bug 24194: (follow-up) Throw an error if expiration date used Signed-off-by: Nicolas Giraud <nicolas.giraud@inlibro.com> Created attachment 174652 [details] [review] Bug 24194: Making the qa script happy Tidied up a bunch there are still a couple of perltidy complaints, but I am running out of time today so attaching this patch so it doesn't get lost No more test fails, or failing patterns Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Right now if you have the DefaultHoldExpirationdate preference set and disable holds expirations Koha will still set a default expiration date. That's probably not what we want? I see that there are other preferences which don't apply if holds expirations are disabled.
Also, could have have the preference called "HoldsExpirations." Although we have plenty of preferences which use "reserve," I still think it's better to use the correct terminology for new preferences.
Otherwise everything looks like it works according to the plan, although I didn't find that this was true:
> 4) In yet another tab, open the staff intranet and place a reserve for a
> user. Check it in and set the reserve as waiting. Notice that an
> expiration date has now been generated for this reserve.
I only saw an expiration date getting generated if DefaultHoldExpirationdate was set. Not sure if that's a quirk of my test system or if I'm missing something?
|