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.
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?
(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!
Hi Fiona, that makes sense now - thx.
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
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!
(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.
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
Either "DefaultHoldExpirationdatePeriod" or "DefaultHoldExpirationdateUnitOfTime" was left undefined causing Datetime error. Added missing default values for all sysprefs and clarified test plan.
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>
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>
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.
> POD is missing for 'store' Something like that would do: -Overloaded I<store> method to apply DefaultHoldExpirationdate
Created attachment 117472 [details] [review] Bug 26498: Add POD for "store"
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>
Created attachment 117543 [details] [review] Bug 26498: Add POD for "store" Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works well, code reads well, QA scripts are happy and tests make sense and pass.. Passing QA
The store method should be improved to set expirationdate only if reservedate has been modified, to avoid unnecessary processing.
(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.
(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.
(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.
Good catch Jonathan.. do you need some help here Emmi.. or are you already on the case?
(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 :)
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>
Created attachment 118677 [details] [review] Bug 26498: Add POD for "store" Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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.
Created attachment 119278 [details] [review] Bug 26498: Add default values Just in case
Don't we actually want this to be a circulation rule to have more granularity?
Pushed to master for 21.05, thanks to everybody involved!
Enhancement not pushed to 20.11.x
Comment on attachment 114663 [details] [review] Bug 26498: Set default expiration date for holds >From 9fcb4bc1baf12a5471bec6997af4ac41da9a3db7 Mon Sep 17 00:00:00 2001 >From: Emmi Takkinen <emmi.takkinen@outlook.com> >Date: Thu, 26 Nov 2020 14:59:42 +0200 >Subject: [PATCH] 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> >--- > Koha/Hold.pm | 15 ++++++++++++ > ...Set_default_expiration_date_for_holds.perl | 12 ++++++++++ > installer/data/mysql/mandatory/sysprefs.sql | 3 +++ > .../admin/preferences/circulation.pref | 18 +++++++++++++++ > t/db_dependent/Hold.t | 23 ++++++++++++++++++- > 5 files changed, 70 insertions(+), 1 deletion(-) > create mode 100644 installer/data/mysql/atomicupdate/Bug-26498-Set_default_expiration_date_for_holds.perl > >diff --git a/Koha/Hold.pm b/Koha/Hold.pm >index 7754691238..33f474cfba 100644 >--- a/Koha/Hold.pm >+++ b/Koha/Hold.pm >@@ -505,6 +505,21 @@ sub cancel { > return $self; > } > >+sub store { >+ my ($self) = @_; >+ >+ if ( C4::Context->preference('DefaultHoldExpirationdate') >+ and ( not defined $self->expirationdate or $self->expirationdate eq '' ) ){ >+ >+ my $period = C4::Context->preference('DefaultHoldExpirationdatePeriod'); >+ my $timeunit = C4::Context->preference('DefaultHoldExpirationdateUnitOfTime'); >+ >+ $self->expirationdate( dt_from_string( $self->reservedate )->add( $timeunit => $period ) ); >+ } >+ >+ $self = $self->SUPER::store; >+} >+ > =head3 _move_to_old > > my $is_moved = $hold->_move_to_old; >diff --git a/installer/data/mysql/atomicupdate/Bug-26498-Set_default_expiration_date_for_holds.perl b/installer/data/mysql/atomicupdate/Bug-26498-Set_default_expiration_date_for_holds.perl >new file mode 100644 >index 0000000000..0fc61b50e4 >--- /dev/null >+++ b/installer/data/mysql/atomicupdate/Bug-26498-Set_default_expiration_date_for_holds.perl >@@ -0,0 +1,12 @@ >+$DBversion = 'XXX'; # will be replaced by the RM >+if( CheckVersion( $DBversion ) ) { >+ # you can use $dbh here like: >+ # $dbh->do( "ALTER TABLE biblio ADD COLUMN badtaste int" ); >+ >+ $dbh->do(q{INSERT IGNORE INTO systempreferences (variable,value,options,explanation,type) VALUES ('DefaultHoldExpirationdate','0','','Automatically set default expiration date for holds','YesNo') }); >+ $dbh->do(q{INSERT IGNORE INTO systempreferences (variable,value,options,explanation,type) VALUES ('DefaultHoldExpirationdatePeriod','0','','How long into the future default expiration date is set to be.','integer') }); >+ $dbh->do(q{INSERT IGNORE INTO systempreferences (variable,value,options,explanation,type) VALUES ('DefaultHoldExpirationdateUnitOfTime','days','days|months|years','Which unit of time is used when settimg default expiration date. ','choice') }); >+ >+ # Always end with this (adjust the bug info) >+ NewVersion( $DBversion, 26498, "Bug 26498 - Add option to set a default expire date for holds at reservation time"); >+} >\ No newline at end of file >diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql >index 2ea427bdd2..5b6587ee05 100644 >--- a/installer/data/mysql/mandatory/sysprefs.sql >+++ b/installer/data/mysql/mandatory/sysprefs.sql >@@ -151,6 +151,9 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` > ('decreaseLoanHighHoldsValue',NULL,'','Specifies a threshold for the minimum number of holds needed to trigger a reduction in loan duration (used with decreaseLoanHighHolds)','Integer'), > ('DefaultClassificationSource','ddc',NULL,'Default classification scheme used by the collection. E.g., Dewey, LCC, etc.','ClassSources'), > ('DefaultCountryField008','','','Fill in the default country code for field 008 Range 15-17 of MARC21 - Place of publication, production, or execution. See <a href=\"http://www.loc.gov/marc/countries/countries_code.html\">MARC Code List for Countries</a>','Free'), >+('DefaultHoldExpirationdate','0','','Automatically set expiration date for holds','YesNo'), >+('DefaultHoldExpirationdatePeriod','0','','How long into the future default expiration date is set to be.','integer'), >+('DefaultHoldExpirationdateUnitOfTime','days','days|months|years','Which unit of time is used when settimg default expiration date. ','choice'), > ('DefaultLanguageField008','','','Fill in the default language for field 008 Range 35-37 of MARC21 records (e.g. eng, nor, ger, see <a href=\"http://www.loc.gov/marc/languages/language_code.html\">MARC Code List for Languages</a>)','Free'), > ('DefaultLongOverdueSkipLostStatuses', '', NULL, 'Skip these lost statuses by default in longoverdue.pl', 'Free'), > ('DefaultLongOverdueChargeValue', '', NULL, "Charge a lost item to the borrower's account when the LOST value of the item changes to n.", 'integer'), >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref >index 9bd26d5aa1..430aaaa25a 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref >@@ -893,6 +893,24 @@ Circulation: > - pref: UpdateItemWhenLostFromHoldList > type: textarea > syntax: text/x-yaml >+ - >+ - pref: DefaultHoldExpirationdate >+ default: 0 >+ choices: >+ yes: Set >+ no: Don't set >+ - default expiration date for holds automatically. >+ - If enabled, set expiration date >+ - pref: DefaultHoldExpirationdatePeriod >+ default: 0 >+ class: integer >+ - pref: DefaultHoldExpirationdateUnitOfTime >+ default: days >+ choices: >+ days: days >+ months: months >+ years: years >+ - from reserve date. > Interlibrary loans: > - > - pref: ILLModule >diff --git a/t/db_dependent/Hold.t b/t/db_dependent/Hold.t >index 5b71b64292..c86807e3a9 100755 >--- a/t/db_dependent/Hold.t >+++ b/t/db_dependent/Hold.t >@@ -29,7 +29,7 @@ use Koha::Item; > use Koha::DateUtils; > use t::lib::TestBuilder; > >-use Test::More tests => 33; >+use Test::More tests => 34; > use Test::Exception; > use Test::Warn; > >@@ -142,6 +142,27 @@ ok( !$hold->is_at_destination(), "Waiting hold where hold branchcode is not the > $item->holdingbranch( $branches[1]->{branchcode} ); > ok( $hold->is_at_destination(), "Waiting hold where hold branchcode is the same as the item's holdingbranch is at destination" ); > >+# Test setting expiration date >+t::lib::Mocks::mock_preference( 'DefaultHoldExpirationdate', 1 ); >+t::lib::Mocks::mock_preference( 'DefaultHoldExpirationdatePeriod', 2 ); >+t::lib::Mocks::mock_preference( 'DefaultHoldExpirationdateUnitOfTime', 'years' ); >+ >+my $hold_2 = Koha::Hold->new( >+ { >+ biblionumber => $biblionumber, >+ itemnumber => $item->id(), >+ reservedate => '2020-11-30', >+ waitingdate => '2020-11-30', >+ borrowernumber => $borrower->{borrowernumber}, >+ branchcode => $branches[1]->{branchcode}, >+ suspend => 0, >+ } >+); >+$hold_2->store(); >+ >+my $expected_date = dt_from_string( $hold_2->reservedate )->add( years => 2); >+is($hold_2->expirationdate->ymd, $expected_date->ymd,'Expiration date is correctly set.'); >+ > $schema->storage->txn_rollback(); > > subtest "delete() tests" => sub { >-- >2.20.1
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 '' ) ){
(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.
I've attached a patch on bug 21729 ("Simplify non-existent expiration date")
Is there any reason why the hold end date on the opac-reserve form is not filled with the default date? Note that this field is available via OPACAllowHoldDateInFuture. And even, if you enable the DefaultHoldExpiration prefs, should you not show the hold end date although OPACAllowHoldDateInFuture might be off ? How do we alert the user on the default end date for a hold now?
(In reply to Marcel de Rooy from comment #34) > Is there any reason why the hold end date on the opac-reserve form is not > filled with the default date? > Note that this field is available via OPACAllowHoldDateInFuture. > Tested and default date was set nicely. > And even, if you enable the DefaultHoldExpiration prefs, should you not show > the hold end date although OPACAllowHoldDateInFuture might be off ? > Hmm, to me it seems that hold end date field is hidden only if AllowHoldDateInFuture syspref is set off. And if I understand correctly end date field in opac should only be shown if both AllowHoldDateInFuture _and_ OPACAllowHoldDateInFuture are enabled. So it seems OPACAllowHoldDateInFuture isn't working as it should? > How do we alert the user on the default end date for a hold now? Good point. We don't currently by my knowledge. Maybe we need one?