[Bug 26498] New: Add option to set a default expire date for holds at reservation time
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Bug ID: 26498 Summary: Add option to set a default expire date for holds at reservation time Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fiona.borthwick@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #2 from Fiona Borthwick <fiona.borthwick@ptfs-europe.com> --- (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! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #3 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Fiona, that makes sense now - thx. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=24194 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Emmi Takkinen <emmi.takkinen@outlook.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emmi.takkinen@outlook.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Emmi Takkinen <emmi.takkinen@outlook.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |emmi.takkinen@outlook.com |ity.org | --- Comment #4 from Emmi Takkinen <emmi.takkinen@outlook.com> --- Created attachment 114057 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114057&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sally.healey@cheshireshared | |services.gov.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Andrew Isherwood <andrew.isherwood@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew.isherwood@ptfs-europ | |e.com --- Comment #5 from Andrew Isherwood <andrew.isherwood@ptfs-europe.com> --- 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED --- Comment #6 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114057|0 |1 is obsolete| | --- Comment #7 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 114524 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114524&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #8 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Either "DefaultHoldExpirationdatePeriod" or "DefaultHoldExpirationdateUnitOfTime" was left undefined causing Datetime error. Added missing default values for all sysprefs and clarified test plan. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Andrew Isherwood <andrew.isherwood@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Andrew Isherwood <andrew.isherwood@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114524|0 |1 is obsolete| | --- Comment #9 from Andrew Isherwood <andrew.isherwood@ptfs-europe.com> --- Created attachment 114663 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114663&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114663|0 |1 is obsolete| | --- Comment #10 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 117127 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117127&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net QA Contact|testopia@bugs.koha-communit |victor@tuxayo.net |y.org | --- Comment #11 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #12 from Victor Grousset/tuxayo <victor@tuxayo.net> ---
POD is missing for 'store'
Something like that would do: -Overloaded I<store> method to apply DefaultHoldExpirationdate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This enhancement adds the release notes| |option to set a default | |elapsed expiry date for | |holds at placement time. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #13 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 117472 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117472&action=edit Bug 26498: Add POD for "store" -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117127|0 |1 is obsolete| | --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117542 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117542&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117472|0 |1 is obsolete| | --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117543 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117543&action=edit Bug 26498: Add POD for "store" Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Works well, code reads well, QA scripts are happy and tests make sense and pass.. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- The store method should be improved to set expirationdate only if reservedate has been modified, to avoid unnecessary processing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #18 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- (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 are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #20 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Good catch Jonathan.. do you need some help here Emmi.. or are you already on the case? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #22 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117542|0 |1 is obsolete| | --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118676 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118676&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117543|0 |1 is obsolete| | --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118677 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118677&action=edit Bug 26498: Add POD for "store" Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118678 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118678&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the|This enhancement adds the |This enhancement adds the release notes|option to set a default |option to set a default |elapsed expiry date for |elapsed expiry date for |holds at placement time. |holds at placement and when | |reservedate is updated. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #26 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 119278 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119278&action=edit Bug 26498: Add default values Just in case -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #27 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Don't we actually want this to be a circulation rule to have more granularity? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |21.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #29 from Fridolin Somers <fridolin.somers@biblibre.com> --- Enhancement not pushed to 20.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Circulation |Hold requests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #30 from Tadas <tadas.kaupa@gmail.com> --- Comment on attachment 114663 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114663 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
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29073 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29073 [Bug 29073] Hold expiration added to new holds when DefaultHoldExpirationdate turned off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29074 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29074 [Bug 29074] DefaultHoldExpirationdatePeriod blank value interpreted as zero -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29192 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29192 [Bug 29192] DefaultHoldExpirationDate sends a DateTime object to logs -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #31 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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 '' ) ){ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #32 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=21729 --- Comment #33 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I've attached a patch on bug 21729 ("Simplify non-existent expiration date") -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #34 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 --- Comment #35 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26498 Andrew Isherwood <bugzilla@warmlight.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|bugzilla@warmlight.co.uk | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org