See bug 21608 and bug 21611 - existing behvaiour from 17.11 is restored by those bugs - but it is possible we should consider broader options for what happens when a hold is reset from 'found' status
The issue is that when a hold is found the expiration date set by the patron (not needed after...) is overwritten by the latest possible pick-up date if that is after the expiration date. This means there is no way we can revert a hold from waiting in a clean way. Currently the expirationdate is kept when reverting, which means that we will have all reverted holds expire - for a short pick-up time set... they will expire very soon after reverting. I think the only thing we can do is set the expirationdate to blank.
*** Bug 23052 has been marked as a duplicate of this bug. ***
*** Bug 21932 has been marked as a duplicate of this bug. ***
Bug 21032 among others has suggested that we have a separate database field for the expiration date or the waiting date - I agree this is the right fix.
This is causing a lot of confusing in my library. It would be great to have this fixed. Thanks! Peggy - Dover Public Library NH
(In reply to Katrin Fischer from comment #4) > Bug 21032 among others has suggested that we have a separate database field > for the expiration date or the waiting date - I agree this is the right fix. I agree. There should be separate fields. Having a double use of the same field was a mistake, in my opinion. These dates need to be retained separately.
On master the expiration date can be edited/cleared once the hold is reverted.
(In reply to Katrin Fischer from comment #4) > Bug 21032 among others has suggested that we have a separate database field > for the expiration date or the waiting date - I agree this is the right fix. +1. This would be an ideal solution.
+1 this would be very helpful.
+1 for creating a separate field. This is how it worked in my previous ILS
We've just run into some issues with the expiration date not reverting when an awaiting copy was checked out to a different patron... Definitely would be better if the expiration date was cleared/reset than for it to hang on to the expiration date set when the first item was confirmed.
Created attachment 126160 [details] [review] Bug 21729: DB changes
Created attachment 126161 [details] [review] Bug 21729: DBIC schema changes
Created attachment 126162 [details] [review] Bug 21729: Keep expiration date set when placing a hold The expiration date picked by the patron (or librarian) when placing a hold is lost when a waiting hold is reverted. We need a separate DB field to store this value and restore it when needed: patron_expiration_date The new behaviours are now: Create a hold and specify an expiration date: expirationdate=patron_expiration_date Fill the hold: expiration_date is calculated expiration_date set to the calculated value or to patron_expiration_date if anterior patron_expiration_date not modified Revert the waiting status: expirationdate set back to patron_expiration_date Cancel expire reserves: if < expirationdate OR < patron_expiration_date Note: This change should not be needed but won't hurt
These patches are not ready for inclusion (missing test coverage) but I'd like to get a first iteration with testers to know if it will be answer your needs and the different use cases.
The system preference DefaultHoldExpirationdate puts a date into expirationdate but not your new patron_expiration_date field: expirationdate: 2021-10-23 patron_expiration_date: NULL Further, if I've set the DefaultHoldExpirationdate syspref to make holds expire in X days, but manually pick a non-X value when placing the hold, I get different values in the two fields. Here my syspref is set to 10 days, but I picked a nearer date when placing the hold. Koha shows 10/23 as the expiration date on the bib and borrower pages. expirationdate: 2021-10-23 patron_expiration_date: 2021-10-15 After I check the item in and set it to Waiting status, the patron_expiration_date does start to get enforced. (In this case, my patron_expiration_date ends up less than waitingdate+ReservesMaxPickUpDelay.) waitingdate: 2021-10-13 expirationdate: 2021-10-15 patron_expiration_date: 2021-10-15 From that point, my expiration date remains 10/15 if I revert the hold, recapture it, etc. Seems like we should make the syspref write its date into both expirationdate and patron_expiration.
Oh! It's gets very angry if you don't set an expiration date at all when placing the hold. I turned off DefaultHoldExpirationdate and made a hold without an expirationdate. I checked the item in and confirmed the hold but Koha didn't set ANY expiration date: found: W timestamp: 2021-10-13 12:34:01 itemnumber: 1 waitingdate: 2021-10-13 expirationdate: NULL patron_expiration_date: NULL And I get an error when I try to look at Holds Awaiting Pickup: Date::Calc::Date_to_Days(): not a valid date at /kohadevbox/koha/circ/waitingreserves.pl line 103
All that said, if I've created and captured a hold that started with an expiration date, reverting the hold's waiting status *is* successfully setting the expiration date back to what the patron selected when placing the hold! I tested both reversion via request.pl and via checkout of the item to another patron.
Created attachment 126203 [details] [review] Bug 21729: Keep expiration date set when placing a hold The expiration date picked by the patron (or librarian) when placing a hold is lost when a waiting hold is reverted. We need a separate DB field to store this value and restore it when needed: patron_expiration_date The new behaviours are now: Create a hold and specify an expiration date: expirationdate=patron_expiration_date Fill the hold: expiration_date is calculated expiration_date set to the calculated value or to patron_expiration_date if anterior patron_expiration_date not modified Revert the waiting status: expirationdate set back to patron_expiration_date Cancel expire reserves: if < expirationdate OR < patron_expiration_date Note: This change should not be needed but won't hurt
Thanks Andrew! I found some obvious issues in the previous patch and it will (hopefully) fix what you found. I haven't the time right now to retest the different combinations, but the patch is ready for testing again if you want to give it another try!
Created attachment 126258 [details] [review] Bug 21729: Simplify non-existent expiration date No change in behaviour expected by this patch.
Would like to sign off, should I use the test plan in bug 21932 ?
(In reply to Florian from comment #22) > Would like to sign off, should I use the test plan in bug 21932 ? Yes, that should work.
To test: APPLY PATCH 1 - create a hold with an expiration date sooner than the period defined by your MaxPickupDelay 2 - check your item in, triggering the hold, confirm the expiration date doesn't change 3 - revert your hold, confirm the expiration date doesn't change 4 - create a hold with an expiration date past the period defined by your MaxPickupDelay 5 - check your item in, triggering the hold, confirm the expiration date changes 6 - Revert your hold, confirm the expiration date reverts back to its original value 7 - create a hold with no expiration date 8 - check your item in, triggering the hold, confirm an expiration date is created 9 - revert your hold, confirm the expiration date is deleted
Created attachment 127797 [details] [review] Bug 21729: DB changes Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127798 [details] [review] Bug 21729: DBIC schema changes Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127799 [details] [review] Bug 21729: Keep expiration date set when placing a hold The expiration date picked by the patron (or librarian) when placing a hold is lost when a waiting hold is reverted. We need a separate DB field to store this value and restore it when needed: patron_expiration_date The new behaviours are now: Create a hold and specify an expiration date: expirationdate=patron_expiration_date Fill the hold: expiration_date is calculated expiration_date set to the calculated value or to patron_expiration_date if anterior patron_expiration_date not modified Revert the waiting status: expirationdate set back to patron_expiration_date Cancel expire reserves: if < expirationdate OR < patron_expiration_date Note: This change should not be needed but won't hurt Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127800 [details] [review] Bug 21729: Simplify non-existent expiration date No change in behaviour expected by this patch. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127867 [details] [review] Bug 29469: (bug 17600 follow-up) Fix tag approval/rejection from staff Usage: POSIX::exit(status) at /kohadevbox/koha/tags/review.pl line 72. at /usr/share/perl/5.28/Carp.pm line 289 Carp::croak('Usage: POSIX::exit(status)') called at /usr/lib/x86_64-linux-gnu/perl/5.28/POSIX.pm line 22 POSIX::usage('exit(status)') called at (eval 2066) line 3 Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 127868 [details] [review] Bug 29270: Expiration date must be in future Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 127869 [details] [review] Bug 29477: Flatpickr - default to 23:59 for the time part It was defaulting to 12:00 which was an unexpected change in behaviour caused by flatpickr switch. Test plan: On the circulation view, check some items out and play with the time part and "Remember for session". The behaviour must be correct, ie. the same as prior to flatpickr switch. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 127870 [details] [review] Bug 29270: start_for is actually date_to The attribute has been renamed in the meanwhile.. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Florian, to obsolete patches you need to click "Details", "edit details", tick "obsolete", then submit.
Created attachment 127877 [details] [review] Bug 21729: DB changes Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Florian Bontemps <florian.bontemps@biblibre.com>
Created attachment 127878 [details] [review] Bug 21729: DBIC schema changes Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Florian Bontemps <florian.bontemps@biblibre.com>
Created attachment 127879 [details] [review] Bug 21729: Keep expiration date set when placing a hold The expiration date picked by the patron (or librarian) when placing a hold is lost when a waiting hold is reverted. We need a separate DB field to store this value and restore it when needed: patron_expiration_date The new behaviours are now: Create a hold and specify an expiration date: expirationdate=patron_expiration_date Fill the hold: expiration_date is calculated expiration_date set to the calculated value or to patron_expiration_date if anterior patron_expiration_date not modified Revert the waiting status: expirationdate set back to patron_expiration_date Cancel expire reserves: if < expirationdate OR < patron_expiration_date Note: This change should not be needed but won't hurt Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Florian Bontemps <florian.bontemps@biblibre.com>
Created attachment 127880 [details] [review] Bug 21729: Simplify non-existent expiration date No change in behaviour expected by this patch. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Florian Bontemps <florian.bontemps@biblibre.com>
Apologies for the mess I caused, hopefully all should be good. Sidenote though : I was able to run the full test plan without the patches on master, and it worked as intented; so in essence I saw no differences with or without the patches. Maybe it was fixed elsewhere?
Any plans on adding the missing tests soon?
Here is the test plan I followed, from the 21932. To test flaws: 1) Place a first available hold for the next available item, and set the hold expires date to a short time, like tomorrow. 2) Make sure your ReservesMaxPickUpDelay setting is set for longer that a day. Say 7 days, for example. 3) View the hold. Note the expiration date is set. 4) Check in the item and trigger the hold. See that the item is waiting. Note the expiration is set to the patron's preferred expiration date. 5) Set the priority back to 1, and update the hold. Note that the hold is now an item specific hold instead of a next available hold. 6) Note the expiration stayed. This is okay in this instance. 7) Cancel the hold and start over. 8) Place a first available hold for the next available item, but do not set the hold expires date. 9) View the hold. Note the expiration date is not set. 10) Check in the item and trigger the hold. See that the item is waiting. Not the expiration date is set. 11) Set the priority back to 1, and update the hold. Note that the hold is now an item specific hold instead of a next available hold. 12) Note the expiration stayed. This is not okay in this instance. The patron's hold will disappear after the date!
Created attachment 128761 [details] [review] Bug 21729: Add tests
Created attachment 129090 [details] [review] Bug 21729: DB changes Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Florian Bontemps <florian.bontemps@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129091 [details] [review] Bug 21729: DBIC schema changes Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Florian Bontemps <florian.bontemps@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129092 [details] [review] Bug 21729: Keep expiration date set when placing a hold The expiration date picked by the patron (or librarian) when placing a hold is lost when a waiting hold is reverted. We need a separate DB field to store this value and restore it when needed: patron_expiration_date The new behaviours are now: Create a hold and specify an expiration date: expirationdate=patron_expiration_date Fill the hold: expiration_date is calculated expiration_date set to the calculated value or to patron_expiration_date if anterior patron_expiration_date not modified Revert the waiting status: expirationdate set back to patron_expiration_date Cancel expire reserves: if < expirationdate OR < patron_expiration_date Note: This change should not be needed but won't hurt Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Florian Bontemps <florian.bontemps@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129093 [details] [review] Bug 21729: Simplify non-existent expiration date No change in behaviour expected by this patch. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Florian Bontemps <florian.bontemps@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129094 [details] [review] Bug 21729: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All working as expected, code is clear. QA scripts happy, includes unit tests.. which read well and are passing. Passing QA
While pushing I will add semi-colon to dbrev, see Bug 29813 And I recreated DBIC shema changes.
This change in behavior needs a nice release note :)
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Oooo test suite broken ^^ I've created 2 new bug reports.