Bug 21729 - When reverting a hold the expirationdate should be reset
Summary: When reverting a hold the expirationdate should be reset
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal with 20 votes (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords: release-notes-needed
: 21932 23052 (view as bug list)
Depends on: 21608
Blocks: 29975 29976
  Show dependency treegraph
 
Reported: 2018-10-30 19:09 UTC by Nick Clemens
Modified: 2022-08-17 20:39 UTC (History)
21 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 21729: DB changes (4.01 KB, patch)
2021-10-13 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21729: DBIC schema changes (3.32 KB, patch)
2021-10-13 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21729: Keep expiration date set when placing a hold (6.38 KB, patch)
2021-10-13 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21729: Keep expiration date set when placing a hold (6.92 KB, patch)
2021-10-13 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21729: Simplify non-existent expiration date (1.31 KB, patch)
2021-10-14 06:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21729: DB changes (4.07 KB, patch)
2021-11-18 13:24 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 21729: DBIC schema changes (3.38 KB, patch)
2021-11-18 13:24 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 21729: Keep expiration date set when placing a hold (6.98 KB, patch)
2021-11-18 13:24 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 21729: Simplify non-existent expiration date (1.38 KB, patch)
2021-11-18 13:24 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29469: (bug 17600 follow-up) Fix tag approval/rejection from staff (1.31 KB, patch)
2021-11-19 14:24 UTC, Florian
Details | Diff | Splinter Review
Bug 29270: Expiration date must be in future (1.27 KB, patch)
2021-11-19 14:24 UTC, Florian
Details | Diff | Splinter Review
Bug 29477: Flatpickr - default to 23:59 for the time part (1.45 KB, patch)
2021-11-19 14:24 UTC, Florian
Details | Diff | Splinter Review
Bug 29270: start_for is actually date_to (1.44 KB, patch)
2021-11-19 14:25 UTC, Florian
Details | Diff | Splinter Review
Bug 21729: DB changes (4.13 KB, patch)
2021-11-19 15:06 UTC, Florian
Details | Diff | Splinter Review
Bug 21729: DBIC schema changes (3.44 KB, patch)
2021-11-19 15:06 UTC, Florian
Details | Diff | Splinter Review
Bug 21729: Keep expiration date set when placing a hold (7.05 KB, patch)
2021-11-19 15:06 UTC, Florian
Details | Diff | Splinter Review
Bug 21729: Simplify non-existent expiration date (1.44 KB, patch)
2021-11-19 15:06 UTC, Florian
Details | Diff | Splinter Review
Bug 21729: Add tests (4.69 KB, patch)
2021-12-20 15:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21729: DB changes (4.20 KB, patch)
2022-01-06 14:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21729: DBIC schema changes (3.51 KB, patch)
2022-01-06 14:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21729: Keep expiration date set when placing a hold (7.11 KB, patch)
2022-01-06 14:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21729: Simplify non-existent expiration date (1.50 KB, patch)
2022-01-06 14:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21729: Add tests (4.75 KB, patch)
2022-01-06 14:13 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-10-30 19:09:07 UTC
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
Comment 1 Katrin Fischer 2018-10-30 19:13:22 UTC
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.
Comment 2 Andrew Fuerste-Henry 2019-06-05 13:45:42 UTC
*** Bug 23052 has been marked as a duplicate of this bug. ***
Comment 3 Katrin Fischer 2019-07-13 10:11:00 UTC
*** Bug 21932 has been marked as a duplicate of this bug. ***
Comment 4 Katrin Fischer 2019-07-13 10:12:09 UTC
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.
Comment 5 Peggy Thrasher 2020-01-13 21:34:13 UTC
This is causing a lot of confusing in my library.  It would be great to have this fixed.
Thanks!
Peggy - Dover Public Library NH
Comment 6 Christopher Brannon 2020-01-13 21:45:26 UTC
(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.
Comment 7 Lisette Scheer 2020-02-18 20:02:06 UTC
On master the expiration date can be edited/cleared once the hold is reverted.
Comment 8 Sally 2020-11-10 10:37:18 UTC
(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.
Comment 9 Rebecca Coert 2021-04-13 15:56:33 UTC
+1 this would be very helpful.
Comment 10 Klas Blomberg 2021-07-27 11:52:56 UTC
+1 for creating a separate field.
This is how it worked in my previous ILS
Comment 11 Chris Slone 2021-08-05 15:08:33 UTC
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.
Comment 12 Jonathan Druart 2021-10-13 09:38:04 UTC
Created attachment 126160 [details] [review]
Bug 21729: DB changes
Comment 13 Jonathan Druart 2021-10-13 09:38:09 UTC
Created attachment 126161 [details] [review]
Bug 21729: DBIC schema changes
Comment 14 Jonathan Druart 2021-10-13 09:38:14 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2021-10-13 09:38:58 UTC
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.
Comment 16 Andrew Fuerste-Henry 2021-10-13 12:32:07 UTC
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.
Comment 17 Andrew Fuerste-Henry 2021-10-13 12:36:42 UTC
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
Comment 18 Andrew Fuerste-Henry 2021-10-13 12:38:21 UTC
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.
Comment 19 Jonathan Druart 2021-10-13 13:55:44 UTC
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
Comment 20 Jonathan Druart 2021-10-13 13:57:31 UTC
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!
Comment 21 Jonathan Druart 2021-10-14 06:38:53 UTC
Created attachment 126258 [details] [review]
Bug 21729: Simplify non-existent expiration date

No change in behaviour expected by this patch.
Comment 22 Florian 2021-11-17 14:49:26 UTC
Would like to sign off, should I use the test plan in bug 21932 ?
Comment 23 Jonathan Druart 2021-11-18 08:06:06 UTC
(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.
Comment 24 Andrew Fuerste-Henry 2021-11-18 13:15:12 UTC
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
Comment 25 Andrew Fuerste-Henry 2021-11-18 13:24:18 UTC
Created attachment 127797 [details] [review]
Bug 21729: DB changes

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 26 Andrew Fuerste-Henry 2021-11-18 13:24:23 UTC
Created attachment 127798 [details] [review]
Bug 21729: DBIC schema changes

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 27 Andrew Fuerste-Henry 2021-11-18 13:24:28 UTC
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>
Comment 28 Andrew Fuerste-Henry 2021-11-18 13:24:32 UTC
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>
Comment 29 Florian 2021-11-19 14:24:48 UTC Comment hidden (obsolete)
Comment 30 Florian 2021-11-19 14:24:53 UTC Comment hidden (obsolete)
Comment 31 Florian 2021-11-19 14:24:58 UTC Comment hidden (obsolete)
Comment 32 Florian 2021-11-19 14:25:03 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2021-11-19 14:32:36 UTC
Florian, to obsolete patches you need to click "Details", "edit details", tick "obsolete", then submit.
Comment 34 Florian 2021-11-19 15:06:33 UTC
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>
Comment 35 Florian 2021-11-19 15:06:38 UTC
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>
Comment 36 Florian 2021-11-19 15:06:44 UTC
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>
Comment 37 Florian 2021-11-19 15:06:49 UTC
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>
Comment 38 Florian 2021-11-19 15:10:22 UTC
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?
Comment 39 Tomás Cohen Arazi 2021-12-17 15:20:16 UTC
Any plans on adding the missing tests soon?
Comment 40 Florian 2021-12-20 12:47:54 UTC
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!
Comment 41 Jonathan Druart 2021-12-20 15:04:11 UTC
Created attachment 128761 [details] [review]
Bug 21729: Add tests
Comment 42 Martin Renvoize 2022-01-06 14:12:43 UTC
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>
Comment 43 Martin Renvoize 2022-01-06 14:12:49 UTC
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>
Comment 44 Martin Renvoize 2022-01-06 14:12:54 UTC
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>
Comment 45 Martin Renvoize 2022-01-06 14:12:59 UTC
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>
Comment 46 Martin Renvoize 2022-01-06 14:13:05 UTC
Created attachment 129094 [details] [review]
Bug 21729: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2022-01-06 14:13:56 UTC
All working as expected, code is clear.

QA scripts happy, includes unit tests.. which read well and are passing.

Passing QA
Comment 48 Fridolin Somers 2022-01-28 20:12:49 UTC
While pushing I will add semi-colon to dbrev, see Bug 29813

And I recreated DBIC shema changes.
Comment 49 Fridolin Somers 2022-01-28 20:14:48 UTC
This change in behavior needs a nice release note :)
Comment 50 Fridolin Somers 2022-01-28 21:12:31 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 51 Fridolin Somers 2022-01-29 00:49:51 UTC
Oooo test suite broken ^^
I've created 2 new bug reports.