Bug 26498 - Add option to set a default expire date for holds at reservation time
Summary: Add option to set a default expire date for holds at reservation time
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Emmi Takkinen
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on:
Blocks: 29073 29074 29192
  Show dependency treegraph
 
Reported: 2020-09-21 09:12 UTC by Martin Renvoize
Modified: 2021-10-14 06:39 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This enhancement adds the option to set a default elapsed expiry date for holds at placement and when reservedate is updated.
Version(s) released in:
21.05.00


Attachments
Bug 26498: Set default expiration date for holds (7.80 KB, patch)
2020-11-30 13:44 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 26498: Set default expiration date for holds (7.99 KB, patch)
2020-12-18 14:02 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 26498: Set default expiration date for holds (8.06 KB, patch)
2020-12-23 10:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 26498: Set default expiration date for holds (8.09 KB, patch)
2021-02-22 01:51 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26498: Add POD for "store" (590 bytes, patch)
2021-03-02 07:49 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 26498: Set default expiration date for holds (8.16 KB, patch)
2021-03-02 17:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26498: Add POD for "store" (652 bytes, patch)
2021-03-02 17:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26498: Set default expiration date for holds (8.16 KB, patch)
2021-03-23 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26498: Add POD for "store" (652 bytes, patch)
2021-03-23 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26498: (QA follow-up) Add handling for update (6.59 KB, patch)
2021-03-23 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26498: Add default values (968 bytes, patch)
2021-04-07 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-09-21 09:12:42 UTC
Currently, when placing a hold the default is to leave the expiry date empty. It would be useful to have the option to set a default delay via a system preference such that if the patron/staff placing the hold doesn't fill the expire date, we set a default instead of leaving the hold without an expiration date.
Comment 1 Katrin Fischer 2020-09-21 09:33:48 UTC
Can you explain the use case a bit more? 

The expiration date is how long the hold will be "valid" in the system before it's filled (then it's pickup date...). So we would limit the holds to be only valid for x by default... And the users would see he date in their patron accounts?
Comment 2 Fiona Borthwick 2020-09-21 09:47:12 UTC
(In reply to Katrin Fischer from comment #1)
> Can you explain the use case a bit more? 
> 
> The expiration date is how long the hold will be "valid" in the system
> before it's filled (then it's pickup date...). So we would limit the holds
> to be only valid for x by default... And the users would see he date in
> their patron accounts?

Hi Katrin,

Holds can live on the system forever without being filled and we have customers who want to manage this. A member of staff or a user can manually enter an expiry date when placing the hold but if they do not, then the expiry date is blank which means that the hold never expires. We have customers who ask us to assign expiry dates to their holds. Then if they are not filled in that time period, the hold will expire.

So yes what you say here "we would limit the holds to be only valid for x by default..." is what we would like. I suppose it would be useful or the patrons to see this information in their accounts.

Hope this makes sense!
Comment 3 Katrin Fischer 2020-09-29 19:15:34 UTC
Hi Fiona, that makes sense now - thx.
Comment 4 Emmi Takkinen 2020-11-30 13:44:43 UTC
Created attachment 114057 [details] [review]
Bug 26498: Set default expiration date for holds

Koha doesn't automatically set expiration date for holds
so they can live in the system forever. This patch adds
new sysprefs to control setting of default expiration
date for holds.

To test:
1. Apply patch (and update database).
2. Enable syspref "DefaultHoldExpirationdate" and set some values
to "DefaultHoldExpirationdatePeriod" and
"DefaultHoldExpirationdateUnitOfTime".
3. Add some holds for patron from staff client or OPAC.
=> Holds expiration date should be set according your settings
4. Disable "DefaultHoldExpirationdate" and add new holds.
=> Expiration date shouldn't be set.

Also prove prove t/db_dependent/Hold.t

Sponsored-by: Koha-Suomi Oy
Comment 5 Andrew Isherwood 2020-12-10 14:23:42 UTC
Hi Emmi

I tried to sign this off but ran into a few problems, sorry!

1. prove -v t/db_dependent/Hold.t fails with:
t/db_dependent/Hold.t .. 1/34 found extra parameters: []# Looks like your test exited with 2 just after 1.
t/db_dependent/Hold.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 33/34 subtests 

Test Summary Report
-------------------
t/db_dependent/Hold.t (Wstat: 512 Tests: 1 Failed: 0)
  Non-zero exit status: 2
  Parse errors: Bad plan.  You planned 34 tests but ran 1.
Files=1, Tests=1,  4 wallclock secs ( 0.04 usr  0.01 sys +  3.28 cusr  0.42 csys =  3.75 CPU)
Result: FAIL

2. In your test plan, you say "Holds expiration date should be set according your settings", this doesn't appear to be happening. The expiry date is not being populated according to the value in the DefaultHoldExpirationdatePeriod syspref. Maybe I'm misunderstanding the test plan? You can see what I mean here: https://nimb.ws/Sw048R

3. In the video I manually selected a date and the hold was created, however, if I leave the date selection blank I receive an error page (the relevant part of which is here https://nimb.ws/pWzZNO), I note the error is the same as the one in the failing test.

Sorry to report bad news!
Comment 6 Emmi Takkinen 2020-12-11 09:54:30 UTC
 (In reply to Andrew Isherwood from comment #5)
> Hi Emmi
> 
> I tried to sign this off but ran into a few problems, sorry!
> 
> 1. prove -v t/db_dependent/Hold.t fails with:
> t/db_dependent/Hold.t .. 1/34 found extra parameters: []# Looks like your
> test exited with 2 just after 1.
> t/db_dependent/Hold.t .. Dubious, test returned 2 (wstat 512, 0x200)
> Failed 33/34 subtests 
> 
> Test Summary Report
> -------------------
> t/db_dependent/Hold.t (Wstat: 512 Tests: 1 Failed: 0)
>   Non-zero exit status: 2
>   Parse errors: Bad plan.  You planned 34 tests but ran 1.
> Files=1, Tests=1,  4 wallclock secs ( 0.04 usr  0.01 sys +  3.28 cusr  0.42
> csys =  3.75 CPU)
> Result: FAIL
> 
> 2. In your test plan, you say "Holds expiration date should be set according
> your settings", this doesn't appear to be happening. The expiry date is not
> being populated according to the value in the
> DefaultHoldExpirationdatePeriod syspref. Maybe I'm misunderstanding the test
> plan? You can see what I mean here: https://nimb.ws/Sw048R
> 
> 3. In the video I manually selected a date and the hold was created,
> however, if I leave the date selection blank I receive an error page (the
> relevant part of which is here https://nimb.ws/pWzZNO), I note the error is
> the same as the one in the failing test.
> 
> Sorry to report bad news!

Hi Andrew and thanks for your feedback.

1. There is something wrong with the tests but mine fail for other reason(s):
"attempting to use non-existent class sorting routine CIG" followed by bunch of "Use of uninitialized value $cn_class in concatenation", then fail to insert into special_holidays and lastly "The method age is not covered by tests!". But they fail also on master.  

2. I should have been more clear with my test plan, sorry! These sysprefs have effect only if expiration date is left blank. As Fiona above said: "A member of staff or a user can manually enter an expiry date when placing the hold but if they do not, then the expiry date is blank which means that the hold never expires."         

3. Hmm, I'm unable to repeat this. I'll set this back to assigned and see if I can figure this out.
Comment 7 Emmi Takkinen 2020-12-18 14:02:06 UTC
Created attachment 114524 [details] [review]
Bug 26498: Set default expiration date for holds

Koha doesn't automatically set expiration date for holds
so they can live in the system forever. This patch adds
new sysprefs to control setting of default expiration
date for holds. Note that expiration date is only set
if input field for it is left blank.

To test:
1. Apply patch and update database.
2. Enable syspref "DefaultHoldExpirationdate" and set some values
to "DefaultHoldExpirationdatePeriod" and
"DefaultHoldExpirationdateUnitOfTime".
3. Add some holds for patron from staff client or OPAC and leave
expiration date input field blank.
=> Holds expiration date should be set according your settings
4. Disable "DefaultHoldExpirationdate" and repeat step 3.
=> Expiration date shouldn't be set.

Also prove prove t/db_dependent/Hold.t

Sponsored-by: Koha-Suomi Oy
Comment 8 Emmi Takkinen 2020-12-18 14:08:26 UTC
Either "DefaultHoldExpirationdatePeriod" or "DefaultHoldExpirationdateUnitOfTime" was left undefined causing Datetime error. Added missing default values for all sysprefs and clarified test plan.
Comment 9 Andrew Isherwood 2020-12-23 10:21:59 UTC
Created attachment 114663 [details] [review]
Bug 26498: Set default expiration date for holds

Koha doesn't automatically set expiration date for holds
so they can live in the system forever. This patch adds
new sysprefs to control setting of default expiration
date for holds. Note that expiration date is only set
if input field for it is left blank.

To test:
1. Apply patch and update database.
2. Enable syspref "DefaultHoldExpirationdate" and set some values
to "DefaultHoldExpirationdatePeriod" and
"DefaultHoldExpirationdateUnitOfTime".
3. Add some holds for patron from staff client or OPAC and leave
expiration date input field blank.
=> Holds expiration date should be set according your settings
4. Disable "DefaultHoldExpirationdate" and repeat step 3.
=> Expiration date shouldn't be set.

Also prove prove t/db_dependent/Hold.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Comment 10 Victor Grousset/tuxayo 2021-02-22 01:51:48 UTC
Created attachment 117127 [details] [review]
Bug 26498: Set default expiration date for holds

Koha doesn't automatically set expiration date for holds
so they can live in the system forever. This patch adds
new sysprefs to control setting of default expiration
date for holds. Note that expiration date is only set
if input field for it is left blank.

To test:
1. Apply patch and update database.
2. Enable syspref "DefaultHoldExpirationdate" and set some values
to "DefaultHoldExpirationdatePeriod" and
"DefaultHoldExpirationdateUnitOfTime".
3. Add some holds for patron from staff client or OPAC and leave
expiration date input field blank.
=> Holds expiration date should be set according your settings
4. Disable "DefaultHoldExpirationdate" and repeat step 3.
=> Expiration date shouldn't be set.

Also prove prove t/db_dependent/Hold.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 11 Victor Grousset/tuxayo 2021-02-22 01:54:44 UTC
QA tools:
   FAIL	  pod coverage
		POD is missing for 'store'

Fixed a typo and phrasing : settimg default expiration => setting the default expiration

Other than that, it work (hence the signoff) and the code looks good.
Comment 12 Victor Grousset/tuxayo 2021-02-22 01:58:58 UTC
> POD is missing for 'store'

Something like that would do:

-Overloaded I<store> method to apply DefaultHoldExpirationdate
Comment 13 Emmi Takkinen 2021-03-02 07:49:45 UTC
Created attachment 117472 [details] [review]
Bug 26498: Add POD for "store"
Comment 14 Martin Renvoize 2021-03-02 17:24:08 UTC
Created attachment 117542 [details] [review]
Bug 26498: Set default expiration date for holds

Koha doesn't automatically set expiration date for holds
so they can live in the system forever. This patch adds
new sysprefs to control setting of default expiration
date for holds. Note that expiration date is only set
if input field for it is left blank.

To test:
1. Apply patch and update database.
2. Enable syspref "DefaultHoldExpirationdate" and set some values
to "DefaultHoldExpirationdatePeriod" and
"DefaultHoldExpirationdateUnitOfTime".
3. Add some holds for patron from staff client or OPAC and leave
expiration date input field blank.
=> Holds expiration date should be set according your settings
4. Disable "DefaultHoldExpirationdate" and repeat step 3.
=> Expiration date shouldn't be set.

Also prove prove t/db_dependent/Hold.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2021-03-02 17:24:12 UTC
Created attachment 117543 [details] [review]
Bug 26498: Add POD for "store"

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-03-02 17:24:52 UTC
Works well, code reads well, QA scripts are happy and tests make sense and pass..

Passing QA
Comment 17 Jonathan Druart 2021-03-03 16:01:22 UTC
The store method should be improved to set expirationdate only if reservedate has been modified, to avoid unnecessary processing.
Comment 18 Emmi Takkinen 2021-03-04 09:54:13 UTC
(In reply to Jonathan Druart from comment #17)
> The store method should be improved to set expirationdate only if
> reservedate has been modified, to avoid unnecessary processing.

Could you explain a little? Currently expirationdate is set only if "DefaultHoldExpirationdate" and it's not defined or empty. Which is true only when hold is first time made so I'm failing to see logic behind checking reservedate in case of modifications. 

However I now noticed that if "AllowHoldDateInFuture" is set and reservedate is modified expirationdate remains the same. That should be imo be fixed.
Comment 19 Jonathan Druart 2021-03-04 10:07:30 UTC
(In reply to Emmi Takkinen from comment #18)
> (In reply to Jonathan Druart from comment #17)
> > The store method should be improved to set expirationdate only if
> > reservedate has been modified, to avoid unnecessary processing.
> 
> Could you explain a little? Currently expirationdate is set only if
> "DefaultHoldExpirationdate" and it's not defined or empty. Which is true
> only when hold is first time made so I'm failing to see logic behind
> checking reservedate in case of modifications. 
> 
> However I now noticed that if "AllowHoldDateInFuture" is set and reservedate
> is modified expirationdate remains the same. That should be imo be fixed.

You need to set expiration only if reservedate has been passed/modified.
Koha::Item->store will give you some example (search for %updated_columns).

If you have:
  my $hold = Koha::Holds->find(42);
  $hold->notes("some notes")->store;
you don't want to rebuild expirationdate.

Let me know if you need more details.
Comment 20 Emmi Takkinen 2021-03-09 07:22:14 UTC
(In reply to Jonathan Druart from comment #19)
> (In reply to Emmi Takkinen from comment #18)
> > (In reply to Jonathan Druart from comment #17)
> > > The store method should be improved to set expirationdate only if
> > > reservedate has been modified, to avoid unnecessary processing.
> > 
> > Could you explain a little? Currently expirationdate is set only if
> > "DefaultHoldExpirationdate" and it's not defined or empty. Which is true
> > only when hold is first time made so I'm failing to see logic behind
> > checking reservedate in case of modifications. 
> > 
> > However I now noticed that if "AllowHoldDateInFuture" is set and reservedate
> > is modified expirationdate remains the same. That should be imo be fixed.
> 
> You need to set expiration only if reservedate has been passed/modified.
> Koha::Item->store will give you some example (search for %updated_columns).
> 
> If you have:
>   my $hold = Koha::Holds->find(42);
>   $hold->notes("some notes")->store;
> you don't want to rebuild expirationdate.
> 
> Let me know if you need more details.

OK I see what you mean. I'll look into this and provide patch later.
Comment 21 Martin Renvoize 2021-03-22 17:35:23 UTC
Good catch Jonathan.. do you need some help here Emmi.. or are you already on the case?
Comment 22 Emmi Takkinen 2021-03-23 13:29:43 UTC
(In reply to Martin Renvoize from comment #21)
> Good catch Jonathan.. do you need some help here Emmi.. or are you already
> on the case?

I've been a little busy lately so I haven't had time to work with this. Go ahead and feel free to look into this if you want :)
Comment 23 Martin Renvoize 2021-03-23 15:52:31 UTC
Created attachment 118676 [details] [review]
Bug 26498: Set default expiration date for holds

Koha doesn't automatically set expiration date for holds
so they can live in the system forever. This patch adds
new sysprefs to control setting of default expiration
date for holds. Note that expiration date is only set
if input field for it is left blank.

To test:
1. Apply patch and update database.
2. Enable syspref "DefaultHoldExpirationdate" and set some values
to "DefaultHoldExpirationdatePeriod" and
"DefaultHoldExpirationdateUnitOfTime".
3. Add some holds for patron from staff client or OPAC and leave
expiration date input field blank.
=> Holds expiration date should be set according your settings
4. Disable "DefaultHoldExpirationdate" and repeat step 3.
=> Expiration date shouldn't be set.

Also prove prove t/db_dependent/Hold.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2021-03-23 15:52:35 UTC
Created attachment 118677 [details] [review]
Bug 26498: Add POD for "store"

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2021-03-23 15:52:38 UTC
Created attachment 118678 [details] [review]
Bug 26498: (QA follow-up) Add handling for update

This follow-up adds handling for updates as well as the create action as
highlighted by Jonathans comments.  We also adds tests for these cases.
Comment 26 Jonathan Druart 2021-04-07 12:33:19 UTC
Created attachment 119278 [details] [review]
Bug 26498: Add default values

Just in case
Comment 27 Jonathan Druart 2021-04-07 13:18:59 UTC
Don't we actually want this to be a circulation rule to have more granularity?
Comment 28 Jonathan Druart 2021-04-07 14:38:14 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 29 Fridolin Somers 2021-04-19 09:58:54 UTC
Enhancement not pushed to 20.11.x
Comment 30 Tadas 2021-06-16 10:51:22 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2021-10-13 13:33:21 UTC
Any good reasons to not have '! $self->expiration' instead of the not defined, eq '' checks?

+    if ( C4::Context->preference('DefaultHoldExpirationdate')
+        and ( not defined $self->expirationdate or $self->expirationdate eq '' ) ){
Comment 32 Emmi Takkinen 2021-10-14 06:09:53 UTC
(In reply to Jonathan Druart from comment #31)
> Any good reasons to not have '! $self->expiration' instead of the not
> defined, eq '' checks?
> 
> +    if ( C4::Context->preference('DefaultHoldExpirationdate')
> +        and ( not defined $self->expirationdate or $self->expirationdate eq
> '' ) ){

Hmm, most likely I have first used just not defined, then after testing realized we also need to check empty values and just added it without remembering !$self->expirationdate. So probably no good reasons there.
Comment 33 Jonathan Druart 2021-10-14 06:39:09 UTC
I've attached a patch on bug 21729 ("Simplify non-existent expiration date")