Bug 8367 - How long is a hold waiting for pickup at a more granular level
Summary: How long is a hold waiting for pickup at a more granular level
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 40 votes (vote)
Assignee: Aleisha Amohia
QA Contact: Katrin Fischer
URL:
Keywords:
: 19309 (view as bug list)
Depends on:
Blocks: 8362 10487 35460
  Show dependency treegraph
 
Reported: 2012-07-05 13:35 UTC by Paul Poulain
Modified: 2023-12-22 22:01 UTC (History)
41 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds a new value to the circulation rules: 'holds pickup period'. It overrides the value set in the `ReservesMaxPickUpDelay` system preference and allows setting different delays for specific item type, patron category, and library combinations.
Version(s) released in:
23.11.00


Attachments
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (23.22 KB, patch)
2012-09-11 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (24.74 KB, patch)
2012-10-31 13:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.57 KB, patch)
2012-11-21 16:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (24.77 KB, patch)
2012-11-21 17:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.57 KB, patch)
2012-11-21 17:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.63 KB, patch)
2012-11-22 09:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Add more granular level for ReservesMaxPickUpDelay (25.24 KB, patch)
2013-02-15 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.67 KB, patch)
2013-02-15 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (22.64 KB, patch)
2013-02-15 15:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (22.60 KB, patch)
2013-02-18 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (23.23 KB, patch)
2013-02-22 10:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (23.29 KB, patch)
2013-02-25 17:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.66 KB, patch)
2013-02-25 18:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (25.50 KB, patch)
2013-06-18 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.63 KB, patch)
2013-06-18 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (22.89 KB, patch)
2013-06-18 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.98 KB, patch)
2013-06-18 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.36 KB, patch)
2013-06-18 09:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.94 KB, patch)
2013-06-18 09:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.23 KB, patch)
2013-06-18 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.65 KB, patch)
2013-06-18 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup FIX a sql statement (bad conflict merge) (1.79 KB, patch)
2013-06-18 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.28 KB, patch)
2013-06-21 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.65 KB, patch)
2013-06-21 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Followup FIX a sql statement (bad conflict merge) (1.85 KB, patch)
2013-06-21 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.77 KB, patch)
2013-07-03 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Fix the maxpickupdate when we set waitingdate to NOW() (1.85 KB, patch)
2013-07-03 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Fix the maxpickupdate when we set waitingdate to NOW() (1.90 KB, patch)
2013-07-03 10:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.47 KB, patch)
2013-08-13 13:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.47 KB, patch)
2013-08-13 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup FIX a sql statement (bad conflict merge) (1.84 KB, patch)
2013-08-13 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (3.17 KB, patch)
2013-08-13 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Fix the maxpickupdate when we set waitingdate to NOW() (1.91 KB, patch)
2013-08-13 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (42.82 KB, patch)
2014-10-27 11:17 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (60.17 KB, patch)
2014-10-28 14:13 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (62.05 KB, patch)
2014-10-29 08:51 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
using this to obsolete previous patches (1 bytes, text/plain)
2014-10-29 08:56 UTC, Olli-Antti Kivilahti
Details
Bug 8367 - How long is a hold waiting for pickup at a more granular level (61.97 KB, patch)
2014-10-29 13:46 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (62.58 KB, patch)
2014-10-29 17:29 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (58.32 KB, patch)
2015-09-18 13:13 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (58.50 KB, patch)
2015-09-22 08:13 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (58.96 KB, patch)
2018-10-02 00:22 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm (26.22 KB, patch)
2018-10-02 00:25 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (58.96 KB, patch)
2018-10-02 01:28 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm (28.32 KB, patch)
2018-10-02 01:28 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Removed unneccessary warns and commented code (1.33 KB, patch)
2018-10-02 01:30 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir (5.03 KB, patch)
2018-10-02 22:10 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() (4.87 KB, patch)
2018-10-02 22:12 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled (917 bytes, patch)
2018-10-03 00:17 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref (1.37 KB, patch)
2018-10-03 05:01 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (58.19 KB, patch)
2019-08-21 23:41 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (60.47 KB, patch)
2019-08-21 23:45 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm (26.39 KB, patch)
2019-08-21 23:46 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Removed unneccessary warns and commented code (1.36 KB, patch)
2019-08-21 23:47 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm (26.39 KB, patch)
2019-08-21 23:47 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Removed unneccessary warns and commented code (1.36 KB, patch)
2019-08-21 23:49 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir (5.16 KB, patch)
2019-08-21 23:51 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() (4.41 KB, patch)
2019-08-21 23:52 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled (926 bytes, patch)
2019-08-21 23:53 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref (1.38 KB, patch)
2019-08-21 23:54 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: (follow-up) Updates to DBIx class files (8.68 KB, patch)
2019-08-21 23:55 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Fixes to make the reserves workflow work with new schema (2.85 KB, patch)
2019-08-21 23:56 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Fixing display of lastpickupdate (4.82 KB, patch)
2019-08-21 23:56 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (77.18 KB, patch)
2019-08-22 00:45 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() (5.56 KB, patch)
2019-08-22 00:46 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: (follow-up) Updates to DBIx class files (8.68 KB, patch)
2019-08-22 00:47 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate (7.43 KB, patch)
2019-08-22 00:47 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (76.57 KB, patch)
2019-09-19 09:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() (5.56 KB, patch)
2019-09-19 09:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 8367: (follow-up) Updates to DBIx class files (8.67 KB, patch)
2019-09-19 09:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate (7.42 KB, patch)
2019-09-19 09:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (13.84 KB, patch)
2020-04-21 22:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (13.87 KB, patch)
2020-11-05 23:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (13.76 KB, patch)
2021-02-25 00:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (16.77 KB, patch)
2021-03-10 03:31 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (16.77 KB, patch)
2021-03-10 09:54 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.33 KB, patch)
2021-09-07 07:13 UTC, David Cook
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.66 KB, patch)
2021-09-21 05:23 UTC, David Cook
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (17.05 KB, patch)
2022-03-02 21:05 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.34 KB, patch)
2022-03-02 21:05 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.67 KB, patch)
2022-03-02 21:05 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.51 KB, patch)
2022-03-02 21:05 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (3.73 KB, patch)
2022-03-14 03:35 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (898 bytes, patch)
2022-03-14 03:35 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (16.98 KB, patch)
2022-03-15 13:36 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.39 KB, patch)
2022-03-15 13:37 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.72 KB, patch)
2022-03-15 13:37 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.54 KB, patch)
2022-03-15 13:38 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (3.77 KB, patch)
2022-03-15 13:39 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (961 bytes, patch)
2022-03-15 13:39 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (17.38 KB, patch)
2022-04-07 21:53 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.39 KB, patch)
2022-04-07 21:53 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.73 KB, patch)
2022-04-07 21:53 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.62 KB, patch)
2022-04-07 21:53 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (3.80 KB, patch)
2022-04-07 21:54 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (955 bytes, patch)
2022-04-07 21:54 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (17.34 KB, patch)
2022-11-01 20:28 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.39 KB, patch)
2022-11-01 20:28 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.73 KB, patch)
2022-11-01 20:28 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.63 KB, patch)
2022-11-01 20:28 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (3.87 KB, patch)
2022-11-01 20:29 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (955 bytes, patch)
2022-11-01 20:29 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.44 KB, patch)
2022-11-01 20:29 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (17.73 KB, patch)
2022-11-21 02:50 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.39 KB, patch)
2022-11-21 02:50 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.72 KB, patch)
2022-11-21 02:52 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.61 KB, patch)
2022-11-21 02:52 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (3.86 KB, patch)
2022-11-21 02:52 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (957 bytes, patch)
2022-11-21 02:52 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.43 KB, patch)
2022-11-21 02:52 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.49 KB, patch)
2022-11-22 10:28 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 8367: (follow-up) possible to delete circ rule (768 bytes, patch)
2023-01-25 02:31 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) possible to delete circ rule (827 bytes, patch)
2023-01-25 07:21 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (17.76 KB, patch)
2023-03-06 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.44 KB, patch)
2023-03-06 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.77 KB, patch)
2023-03-06 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.66 KB, patch)
2023-03-06 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (3.91 KB, patch)
2023-03-06 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (1011 bytes, patch)
2023-03-06 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.54 KB, patch)
2023-03-06 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8367: (follow-up) possible to delete circ rule (881 bytes, patch)
2023-03-06 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8376: (QA follow-up) Update message on Hold waiting pickup report (2.33 KB, patch)
2023-03-06 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Improve display when value not set for rule (2.23 KB, patch)
2023-03-06 20:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (17.81 KB, patch)
2023-03-22 13:20 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.49 KB, patch)
2023-03-22 13:20 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.82 KB, patch)
2023-03-22 13:20 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.71 KB, patch)
2023-03-22 13:20 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (3.96 KB, patch)
2023-03-22 13:20 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (1.04 KB, patch)
2023-03-22 13:20 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.59 KB, patch)
2023-03-22 13:21 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) possible to delete circ rule (931 bytes, patch)
2023-03-22 13:21 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8376: (QA follow-up) Update message on Hold waiting pickup report (2.38 KB, patch)
2023-03-22 13:21 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Improve display when value not set for rule (2.28 KB, patch)
2023-03-22 13:21 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8376: (QA follow-up) Update message on Hold waiting pickup report (2.28 KB, patch)
2023-04-30 22:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Improve display when value not set for rule (2.28 KB, patch)
2023-04-30 22:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period fallback circulation rule (1.48 KB, patch)
2023-04-30 22:22 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (17.86 KB, patch)
2023-07-10 03:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.49 KB, patch)
2023-07-10 03:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.82 KB, patch)
2023-07-10 03:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.77 KB, patch)
2023-07-10 03:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (3.96 KB, patch)
2023-07-10 03:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (1.04 KB, patch)
2023-07-10 03:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.59 KB, patch)
2023-07-10 03:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (follow-up) possible to delete circ rule (933 bytes, patch)
2023-07-10 03:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report (2.28 KB, patch)
2023-07-10 03:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Improve display when value not set for rule (2.28 KB, patch)
2023-07-10 03:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period fallback circulation rule (1.48 KB, patch)
2023-07-10 03:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (17.94 KB, patch)
2023-10-14 14:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.55 KB, patch)
2023-10-14 14:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.88 KB, patch)
2023-10-14 14:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.82 KB, patch)
2023-10-14 14:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (4.01 KB, patch)
2023-10-14 14:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (1.09 KB, patch)
2023-10-14 14:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.64 KB, patch)
2023-10-14 14:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) possible to delete circ rule (988 bytes, patch)
2023-10-14 14:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report (2.33 KB, patch)
2023-10-14 14:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Improve display when value not set for rule (2.38 KB, patch)
2023-10-14 14:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period fallback circulation rule (1.53 KB, patch)
2023-10-14 14:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Fix QA script (22.78 KB, patch)
2023-10-14 14:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Make sure "Default" is not copied when editing circulation rules (2.37 KB, patch)
2023-10-14 14:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix for circ rules editor (1.90 KB, patch)
2023-10-18 01:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (17.99 KB, patch)
2023-10-18 19:09 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.59 KB, patch)
2023-10-18 19:09 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.93 KB, patch)
2023-10-18 19:09 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.87 KB, patch)
2023-10-18 19:09 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (4.06 KB, patch)
2023-10-18 19:09 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (1.14 KB, patch)
2023-10-18 19:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.69 KB, patch)
2023-10-18 19:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) possible to delete circ rule (1.01 KB, patch)
2023-10-18 19:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report (2.38 KB, patch)
2023-10-18 19:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Improve display when value not set for rule (2.42 KB, patch)
2023-10-18 19:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period fallback circulation rule (1.58 KB, patch)
2023-10-18 19:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Fix QA script (22.83 KB, patch)
2023-10-18 19:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix for circ rules editor (1.95 KB, patch)
2023-10-18 19:10 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (22.56 KB, patch)
2023-10-23 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.59 KB, patch)
2023-10-23 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.93 KB, patch)
2023-10-23 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.88 KB, patch)
2023-10-23 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (4.06 KB, patch)
2023-10-23 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (1.14 KB, patch)
2023-10-23 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.69 KB, patch)
2023-10-23 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) possible to delete circ rule (1.01 KB, patch)
2023-10-23 22:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report (2.38 KB, patch)
2023-10-23 22:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Improve display when value not set for rule (2.42 KB, patch)
2023-10-23 22:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period fallback circulation rule (1.58 KB, patch)
2023-10-23 22:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Fix QA script (22.94 KB, patch)
2023-10-23 22:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix for circ rules editor (1.95 KB, patch)
2023-10-23 22:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix for circ rules editor (2.41 KB, patch)
2023-10-24 00:15 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (22.62 KB, patch)
2023-10-29 10:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (1.65 KB, patch)
2023-10-29 10:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Make database update idempotent (across different MariaDB versions) (1.98 KB, patch)
2023-10-29 10:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits (5.93 KB, patch)
2023-10-29 10:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove pickup delay message from OPAC (4.12 KB, patch)
2023-10-29 10:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix tests (1.20 KB, patch)
2023-10-29 10:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Remove unnecessary code and atomic update (2.75 KB, patch)
2023-10-29 10:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) possible to delete circ rule (1.07 KB, patch)
2023-10-29 10:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report (2.44 KB, patch)
2023-10-29 10:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Improve display when value not set for rule (2.48 KB, patch)
2023-10-29 10:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period fallback circulation rule (1.64 KB, patch)
2023-10-29 10:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Fix QA script (23.00 KB, patch)
2023-10-29 10:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (follow-up) Fix for circ rules editor (2.46 KB, patch)
2023-10-29 10:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Fix broken tests (888 bytes, patch)
2023-11-02 13:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 8367: (QA follow-up) Fix broken tests in Reserves.t (1.11 KB, patch)
2023-11-02 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2012-07-05 13:35:33 UTC
The syspref reservemaxpickupdelay says how long a hold is waiting for patron pickup. This could be set at issuingrule level, to let a student of library A 4 days and a teacher of library B  10 days
Comment 1 Jonathan Druart 2012-09-11 12:55:29 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2012-10-29 15:57:26 UTC
I tried to manually fix the merge errors and ended up getting this error:

The 'days' parameter (undef) to DateTime::Duration::new was an 'undef', which is not one of the allowed types: scalar
 at /usr/lib/perl5/DateTime/Duration.pm line 31
	DateTime::Duration::new(undef, 'days', undef) called at /usr/lib/perl5/DateTime.pm line 1640
	DateTime::add('DateTime=HASH(0xa1d3494)', 'days', undef) called at /home/oleonard/kohaclone/C4/Reserves.pm line 552
	C4::Reserves::GetMaxPickupDate('HASH(0xa11168c)', undef) called at /home/oleonard/kohaclone/circ/waitingreserves.pl line 107

Not sure if this is a real error or whether I made a mistake in merging?
Comment 3 Jonathan Druart 2012-10-31 13:39:04 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-10-31 13:41:23 UTC
(In reply to comment #2)

> Not sure if this is a real error or whether I made a mistake in merging?

Maybe both :)
The merge was not easy to resolve and errors exist in the previous patch.
Comment 5 Kyle M Hall 2012-11-21 14:17:00 UTC
This patch looks good in all cases except for waitingreserves.pl. Even though my hold is past its pickup window, it is showing in the waiting holds tab, and not in the holds over tab is it should be.
Comment 6 Jonathan Druart 2012-11-21 14:40:15 UTC
Hi Kyle,

What is the value filled for the "Available since-until" column ?

I have 2 holds and 1 by tab:
first have "19/11/2012 - 26/11/2012", second (over) "28/10/2012 - 04/11/2012".

And it is what is expected.
Comment 7 Kyle M Hall 2012-11-21 16:47:33 UTC
On circulation.pl it shows:
Item 1 waiting at Centerville until 11/20/2012

On waitingreserves.pl it shows:
11/19/2012 - 11/26/2012

so it appears to be using the new issue rules setting for circulation.pl, but it's still using the system preference for waitingreserves.pl

I have my rule set to 1 day, and my system pref set to 7.
Comment 8 Kyle M Hall 2012-11-21 16:59:18 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-11-21 17:16:34 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-11-21 17:16:51 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-11-22 09:19:52 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2012-11-22 09:23:46 UTC
(In reply to comment #10)
> Created attachment 13604 [details] [review]
> Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup

Good catch Kyle, Thanks!

It would be worth having an other SO for these patches
Comment 13 Kyle M Hall 2012-12-14 16:13:51 UTC
It's been a month without another signer. I think it's time to move these patches to QA.
Comment 14 Paul Poulain 2012-12-14 16:22:12 UTC
(In reply to comment #13)
> It's been a month without another signer. I think it's time to move these
> patches to QA.

agreed (and agreed also because now QAers are supposed to test the patch)
Comment 15 Marcel de Rooy 2013-02-15 08:15:09 UTC
QA: Looking at this now. Rebasing it first..
Comment 16 Marcel de Rooy 2013-02-15 08:16:10 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2013-02-15 08:16:23 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2013-02-15 09:29:41 UTC
QA Comment:
I like the feature! But IMO it needs some more attention before pushing it: 

Issue while resolving merge conflict in circ/waitingreserves.pl: 
The dateformat template var has been removed in master (was in the patch) but is still used in one line of template (waitingreserves.pl). Please check. 

IMO it is confusing that the old pref exists besides the new column in issuing rules. I would suggest simplification by obsoleting pref ReservesMaxPickUpDelay.
At this time behavior is not so clear anymore. Note that there is no comment either on the preferences form that this syspref could be overruled by issuing rules.
And the other way around: If no rule is found, the pref still holds. Does the user still understand that from the rules form? 

GetMaxPickupDate: It may be worth to consider if it would be easier to add a maxpickupdate to the reserves table instead of recalculating it "everywhere".
GetMaxPickupDelay: Only called by GetMaxPickupDate. 
A serious issue IMO is the FIXME in CancelExpiredReserves. Instead of calling GetMaxPickupDelay, you just use the pref value!
I understand that from performance point of view, but this is "killing" the feature.
When using the cron job to remove holds, the new pickup delay should be used not the old one.
Note that you could easily resolve this by looking at a maxpickupdate when adding this to the reserve table and calculating it at confirm hold time. 

Minor representation issue: In the opac user form I see the max pickup date. Would it be consistent to show it on staff catalogue detail too (Holds tab)? 

Setting to Failed QA, mainly for the cancelling holds issue.
Comment 19 Marcel de Rooy 2013-02-15 09:33:54 UTC
Just another thought:
If you would remove the pref, please add a db change to copy it into the new column of the existing issuing rules.
Comment 20 Jonathan Druart 2013-02-15 15:47:50 UTC
(In reply to comment #18)
> QA Comment:
> Note that you could easily resolve this by looking at a maxpickupdate when
> adding this to the reserve table and calculating it at confirm hold time. 

Hi Marcel,
Thanks for QAing this patch!
It is a great idea even though it was not so easy :)
A patch is coming.
Comment 21 Jonathan Druart 2013-02-15 15:48:14 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-02-15 17:02:32 UTC
Questions, does this take into account days the library is closed? It would make sense for the expiration day for pickups to skip to the next open day if they land on a holiday. This is something that needed has needed to be addressed for some time now.
Comment 23 Marcel de Rooy 2013-02-18 09:25:04 UTC
(In reply to comment #20)
> It is a great idea even though it was not so easy :)
Yes, I see. In circulation nothing seems to be easy..
Will try to have a look at your patches again later this week.

Just a question from quickly glancing through your code. In CancelExpiredReserves:
+        my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) - TO_DAYS( waitingdate ) > maxpickupdate AND found = 'W' AND priority = 0";
Does that work? Looks to me that it doesn't. Shouldn't you just look for maxpickupdate < now?
Comment 24 Marcel de Rooy 2013-02-18 09:27:11 UTC
(In reply to comment #22)
> Questions, does this take into account days the library is closed? It would
> make sense for the expiration day for pickups to skip to the next open day
> if they land on a holiday. This is something that needed has needed to be
> addressed for some time now.

Looks like that this report does not do that. I agree that it should, but you could say that it is beyond scope here? Followup or additional report? 
But this report does pave the way. We only have to refine the routine where maxpickupdate is calculated now.
Comment 25 Katrin Fischer 2013-02-18 09:41:22 UTC
Hm, there is a downside to the new column too - we loose a bit of flexibility. Say a library closes for some time and changes the pickup delay to be longer, so the holds don't go stale - with the patch in effect they would have to do a database update to those holds. It's only one use case I have come up with so far. There might be other disadvantages/advantages of both ways of doing it.
Comment 26 Jonathan Druart 2013-02-18 12:24:23 UTC
(In reply to comment #23)
> Just a question from quickly glancing through your code. In
> CancelExpiredReserves:
> +        my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) -
> TO_DAYS( waitingdate ) > maxpickupdate AND found = 'W' AND priority = 0";
> Does that work? Looks to me that it doesn't. Shouldn't you just look for
> maxpickupdate < now?

Yes, of course!
(new patch coming)

(In reply to comment #24)
> (In reply to comment #22)
> > Questions, does this take into account days the library is closed? It would
> > make sense for the expiration day for pickups to skip to the next open day
> > if they land on a holiday. This is something that needed has needed to be
> > addressed for some time now.
> 
> Looks like that this report does not do that. I agree that it should, but
> you could say that it is beyond scope here? Followup or additional report? 
> But this report does pave the way. We only have to refine the routine where
> maxpickupdate is calculated now.

Yes, that can be developed later without investing too much effort.


(In reply to comment #25)
> Hm, there is a downside to the new column too - we loose a bit of
> flexibility. Say a library closes for some time and changes the pickup delay
> to be longer, so the holds don't go stale - with the patch in effect they
> would have to do a database update to those holds. It's only one use case I
> have come up with so far. There might be other disadvantages/advantages of
> both ways of doing it.

You are right. But do you think many librarians often change this value?
I don't see how to manage the feature and this use case.
We can imagine a script that modifies directly the max pickup date in DB according some variables.
Comment 27 Jonathan Druart 2013-02-18 12:24:38 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2013-02-18 12:25:50 UTC
(In reply to comment #25)
> Hm, there is a downside to the new column too - we loose a bit of
> flexibility. Say a library closes for some time and changes the pickup delay
> to be longer, so the holds don't go stale - with the patch in effect they
> would have to do a database update to those holds. It's only one use case I
> have come up with so far. There might be other disadvantages/advantages of
> both ways of doing it.

What is Koha doing if I adjust the loan period? It does not change the due dates in the system. The due date was set with the active guidelines of that moment. You could say: Doing the same for holds is consistent :)

If the library closes, they should stop confirming holds in due time, limiting the number of waiting holds. Pending holds are no problem. The pickup date should be calculated at confirm hold time.
They could also stop the cron job for cancelling holds until they processed the holds queue after reopening? Would they really need a db update?
Comment 29 Katrin Fischer 2013-02-18 12:35:17 UTC
As I said - I think thre are arguments for both approaches.  :)

Only I am thinking that this would not work:
>If the library closes, they should stop confirming holds in due time, limiting >the number of waiting holds. Pending holds are no problem. The pickup date >should be calculated at confirm hold time.

While that might, but requires you have access to the system or your support vendor lets you do it:
>They could also stop the cron job for cancelling holds until they processed the >holds queue after reopening? Would they really need a db update?
Comment 30 Katrin Fischer 2013-02-18 14:33:59 UTC
Ah sorry, I missed half the comments:

@Jonathan: No, I don't think it happens often that the value changes. I just have been thinking about the consequences of not recalculating. I didn't think about the calendar - that would be a solution (see bug 8735)
Maybe making it possible to change the date from the interface (out of scope here) would be nice too at some point.
Comment 31 Marcel de Rooy 2013-02-22 07:21:32 UTC
QA: Looking again here now..
Comment 32 Marcel de Rooy 2013-02-22 08:33:58 UTC
Preliminary comments:
Report Holds awaiting pickup:
A DateTime object can only be compared to another DateTime object (DateTime=HASH(0x53d7020), 2013-02-22). at /usr/share/koha/testclone/circ/waitingreserves.pl line 110

Another serious issue: During upgrade all reserves need a maxpickupdate. Now they are null.

Before I would signoff and pass qa, it would be very useful to have another signoff on this one. After testing your changes thorougly, another external signoff (maybe from Kyle?) is welcome too. The nature of these patches asks for more extensive testing. Please note that a more detailed test plan could help.

For now setting status to Failed QA.
Comment 33 Marcel de Rooy 2013-02-22 09:17:22 UTC
Please have a look too at bug 5786.
Comment 34 Jonathan Druart 2013-02-22 10:52:10 UTC
(In reply to comment #32)
> Preliminary comments:
> Report Holds awaiting pickup:
> A DateTime object can only be compared to another DateTime object
> (DateTime=HASH(0x53d7020), 2013-02-22). at
> /usr/share/koha/testclone/circ/waitingreserves.pl line 110
> 
> Another serious issue: During upgrade all reserves need a maxpickupdate. Now
> they are null.

Fixed.


> Before I would signoff and pass qa, it would be very useful to have another
> signoff on this one. After testing your changes thorougly, another external
> signoff (maybe from Kyle?) is welcome too. The nature of these patches asks
> for more extensive testing.

Yes! I tested some cases but there are so many specific cases, it is difficult to test everything alone. 

> Please note that a more detailed test plan could help.

Since the patch affects all the pages related to holds, I don't know how to write a complete test plan.

(In reply to comment #33)
> Please have a look too at bug 5786.

I did. I think my patch is closer to be pushed to master than bug 5786.
I didn't find where the new column issuingrules.reservesmaxpickupdelay is used elsewhere than in the issuingrules management (add/remove in DB + in tt files).
Comment 35 Jonathan Druart 2013-02-22 10:52:47 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2013-02-25 17:24:24 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2013-02-25 18:19:04 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2013-02-25 18:23:31 UTC
(In reply to comment #37)
> Created attachment 15659 [details] [review] [review]
> Bug 8367: Skip to next open day if max pickup date is a holiday

I should note this also fixes a bug, where the call to GetReservesFromItemnumber assumed it would receive a reserve hashref, but it does not.
Comment 39 Owen Leonard 2013-03-19 01:03:11 UTC
The last patch needs a commit message with a test plan.
Comment 40 Jonathan Druart 2013-06-14 13:22:04 UTC
(In reply to comment #39)
> The last patch needs a commit message with a test plan.

Kyle, since this patch is from you, could you provide a test plan please?
Comment 41 Kyle M Hall 2013-06-17 17:06:00 UTC
(In reply to comment #40)
> (In reply to comment #39)
> > The last patch needs a commit message with a test plan.
> 
> Kyle, since this patch is from you, could you provide a test plan please?

Sure!

1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup location
4) Edit the calendar for the given location, and make the pickup date a holiday
4) View waitingreserves.pl, the last pickup date should now be the increased by one day

I would add this to the patch, but at the moment the first patch isn't applying.
Comment 42 Jonathan Druart 2013-06-18 08:30:05 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2013-06-18 08:30:13 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2013-06-18 08:30:17 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2013-06-18 08:30:21 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2013-06-18 08:31:19 UTC
I rebased 4 patches and added a test plan for the last one.
Thanks Kyle!
Comment 47 Jonathan Druart 2013-06-18 09:17:00 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2013-06-18 09:17:06 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2013-06-18 09:18:29 UTC
I squashed the 3 first patches, it has become too difficult to rebase them...
It needs a SO.
Comment 50 Jonathan Druart 2013-06-18 09:25:53 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2013-06-18 09:25:58 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2013-06-18 09:56:22 UTC
Note: Unit tests for GetMaxPickupDate are provided by bug 10487 (depends on 10289)
Comment 53 Kyle M Hall 2013-06-18 12:01:26 UTC
When I tried to create my first rule on a fresh database with the first patch only, I got this error:

smart-rules.pl: DBD::mysql::st execute failed: called with 18 bind variables when 17 are needed at /home/koha/kohaclone/admin/smart-rules.pl line 137., referer: http://staff.kohadev/cgi-bin/koha/admin/smart-rules.pl
Comment 54 Jonathan Druart 2013-06-18 13:14:52 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2013-06-21 12:49:10 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2013-06-21 12:49:27 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2013-06-21 12:49:47 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2013-06-24 14:58:51 UTC
(In reply to Kyle M Hall from comment #56)
> Created attachment 19181 [details] [review] [review]
> Bug 8367: Skip to next open day if max pickup date is a holiday

Kyle,
I don't understand how this patch can worked. The maxpickupdate is stored in the DB, when adding or updating an hold.
The patch works but not with this test plan.
Comment 59 Kyle M Hall 2013-07-02 16:42:26 UTC
You're right, I think the test plan should be:

Test plan:
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
holiday
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold should now be the increased by one day
Comment 60 Jonathan Druart 2013-07-03 09:55:23 UTC
Kyle, I agree with your test plan. I am signing off your patch. But I think I found a bug, could you take a look at this new patch please?
Comment 61 Jonathan Druart 2013-07-03 09:55:48 UTC Comment hidden (obsolete)
Comment 62 Jonathan Druart 2013-07-03 09:55:58 UTC Comment hidden (obsolete)
Comment 63 Katrin Fischer 2013-07-03 10:10:15 UTC
Hmm, I think NOW() is a mysqlism. I know we probably have lots of those in the codebase already, but maybe worth trying not to add more and find best practices for replacing them? 

Mtj suggested using CURRENT_TIMESTAMP() instead on IRC:
http://dev.mysql.com/doc/refman/5.1/en/date-and-time-functions.html
Comment 64 Jonathan Druart 2013-07-03 10:16:45 UTC
(In reply to Katrin Fischer from comment #63)
> Hmm, I think NOW() is a mysqlism. I know we probably have lots of those in
> the codebase already, but maybe worth trying not to add more and find best
> practices for replacing them? 
> 
> Mtj suggested using CURRENT_TIMESTAMP() instead on IRC:
> http://dev.mysql.com/doc/refman/5.1/en/date-and-time-functions.html

I don't add it, it already exists.
Comment 65 Kyle M Hall 2013-07-03 10:21:19 UTC Comment hidden (obsolete)
Comment 66 Katrin Fischer 2013-07-03 10:22:30 UTC
Ok, maybe we could take a look at the NOW()s another time.
Comment 67 Kyle M Hall 2013-07-03 10:26:42 UTC
(In reply to Katrin Fischer from comment #66)
> Ok, maybe we could take a look at the NOW()s another time.

Yes, I would suggest filing a bug report to take care of any NOW()'s in the codebase. It seems likely however, that the upcoming DBIx::Class switchover will take care of those anyway.
Comment 68 Katrin Fischer 2013-08-03 17:54:14 UTC
Hi Kyle,
there is a conflict in Reserves.pm I am not sure how to fix - could you take a look please? I think it's because your patch for using reserve_id is in now, so it might need a little work there.
Comment 69 Jonathan Druart 2013-08-13 13:10:14 UTC Comment hidden (obsolete)
Comment 70 Jonathan Druart 2013-08-13 13:11:08 UTC Comment hidden (obsolete)
Comment 71 Jonathan Druart 2013-08-13 13:11:16 UTC Comment hidden (obsolete)
Comment 72 Jonathan Druart 2013-08-13 13:11:22 UTC Comment hidden (obsolete)
Comment 73 Jonathan Druart 2013-08-13 13:11:28 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2013-08-13 13:12:09 UTC
(In reply to Katrin Fischer from comment #68)
> Hi Kyle,
> there is a conflict in Reserves.pm I am not sure how to fix - could you take
> a look please? I think it's because your patch for using reserve_id is in
> now, so it might need a little work there.

Conflicts solved.
Comment 75 Katrin Fischer 2013-09-22 12:26:43 UTC
Hi Jonathan,

I am sorry, there are a couple of new conflicts now in this patch:
Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 8367: Add more granular level for ReservesMaxPickUpDelay
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-user.pl
CONFLICT (content): Merge conflict in opac/opac-user.pl
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8367: Add more granular level for ReservesMaxPickUpDelay
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-8367-Add-more-granular-level-for-ReservesMaxPi-BWSyFY.patch
Comment 76 Agnes Rivers-Moore 2014-10-01 19:45:09 UTC
We would like the option to have a shorter pickup period for some item types.
Comment 77 Paul Poulain 2014-10-02 08:03:15 UTC
(In reply to Agnes Rivers-Moore from comment #76)
> We would like the option to have a shorter pickup period for some item types.

Sorry, but this patch does not apply anymore, and should be rewritten almost from scratch because another patch has been pushed before this one, that make what Jonathan made invalid.

So, without funding to re-do the work, we (BibLibre) will not work on it anymore, sorry.
Comment 78 Olli-Antti Kivilahti 2014-10-24 18:17:29 UTC
Hi there gentlemen and esteemed gentle lady!

I just finished refactoring this feature to master and I am looking forward to sharing the results ASAP I get it better tested.

Our bookmobiles are suffering from constantly canceled/billed reserves and this feature solves a big issue for them.

Unfortunately this feature breaks a lot of our in-house work, but such is life :)

I hope you can sharpen your "Sign off"-daggers and start stabbing, presuming you or your client is looking forward to getting this feature to master.
Comment 79 Olli-Antti Kivilahti 2014-10-27 11:17:04 UTC Comment hidden (obsolete)
Comment 80 Olli-Antti Kivilahti 2014-10-27 11:21:25 UTC
Making unit tests for the smart-rules.pl is rather tedious with the lack of proper getters/setters for rules. Need to introduce a C4::SmartRules.pm to access them more conveniently. I hope this is not getting way out of scope for this feture?
Comment 81 Katrin Fischer 2014-10-27 11:23:50 UTC
Hm, introducing a new module just for testing seems a bit out of scope :) Have you taken a look at some of the other tests for recently added features? I think there are some for the new automatic renewal and maybe others that could be useful.
Comment 82 Olli-Antti Kivilahti 2014-10-28 14:13:34 UTC Comment hidden (obsolete)
Comment 83 Olli-Antti Kivilahti 2014-10-29 08:51:26 UTC Comment hidden (obsolete)
Comment 84 Olli-Antti Kivilahti 2014-10-29 08:56:18 UTC Comment hidden (obsolete)
Comment 85 Olli-Antti Kivilahti 2014-10-29 13:46:07 UTC Comment hidden (obsolete)
Comment 86 Paola Rossi 2014-10-29 16:39:44 UTC
Kind Olli-Antti Kivilahti,

I've applied the patch against master 3.17.00.035.

I've upgraded the DB from a one that had been filled with some holds.

After having applied, by mysql command line, on the DB I saw the new field "lastpickupdate" of the table "reserves" as required; in all the holds this new field was set to NULL.

About this step:
4. In the 4 templates, you can see the 'Last pickup date' for an hold
   (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt,
   opac-user.tt)

in circ/circulation.tt (CIRC, Check out, Holds) and members/moremember.tt (Patrons, patron X, Details, Holds) there was the title "Last pickup date" of the new column (the 2-nd from the left) of the table, but the datas in the cells below it were the titles. All the other datas on the right were shifted on the left by 1 column, and the last column "Delete?" of the table had nothing in the cells below it.

I created a new hold for a patron without holds, and the problem was the same on both the *.tt .

About opac-user.tt, I logged in to OPAC as a patron with n holds. Here ("your summary", tag "Holds (n)") I didn't see any new column.

So I pass the patch to "Failed QA" status.
Comment 87 Olli-Antti Kivilahti 2014-10-29 17:29:17 UTC Comment hidden (obsolete)
Comment 88 Olli-Antti Kivilahti 2014-10-29 17:34:11 UTC
(In reply to Paola Rossi from comment #86)
> Kind Olli-Antti Kivilahti,
> 
Thank you so much for testing this patch Ms. or Mrs. Rossi!

> I've applied the patch against master 3.17.00.035.
> 
> I've upgraded the DB from a one that had been filled with some holds.
> 
> After having applied, by mysql command line, on the DB I saw the new field
> "lastpickupdate" of the table "reserves" as required; in all the holds this
> new field was set to NULL.
> 
Sorry, my test plans were not complete. I added the following clause:
0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF
   'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves
   won't get UPDATEd with the new lastpickupdate-values.

> About this step:
> 4. In the 4 templates, you can see the 'Last pickup date' for an hold
>    (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt,
>    opac-user.tt)
> 
> in circ/circulation.tt (CIRC, Check out, Holds) and members/moremember.tt
> (Patrons, patron X, Details, Holds) there was the title "Last pickup date"
> of the new column (the 2-nd from the left) of the table, but the datas in
> the cells below it were the titles. All the other datas on the right were
> shifted on the left by 1 column, and the last column "Delete?" of the table
> had nothing in the cells below it.
> 
> I created a new hold for a patron without holds, and the problem was the
> same on both the *.tt .
> 
Are you sure you used the english language? I have encountered this issue mostly with not-up-to-date language templates. I retested this feature and was unable to replicate your report.

> About opac-user.tt, I logged in to OPAC as a patron with n holds. Here
> ("your summary", tag "Holds (n)") I didn't see any new column.
> 
Sorry, updated the TEST PLAN as follows:
5. In opac-user.pl the last pickup date is displayed in the status-column if
   available.

> So I pass the patch to "Failed QA" status.
Thanks for testing! Hope to hear from you soon :)
Comment 89 Paola Rossi 2014-10-30 16:30:17 UTC
(In reply to Olli-Antti Kivilahti from comment #88)
> [...] Hope to hear from you soon 

Thanks, Olli-Antti, for your kind feedbacks.

Today I've applied the patch against master 3.17.00.043.

After having applied, I've created a hold reserving an item that was checked out. So the reserve/hold's status became "W" after checking in it. On the browser I refreshed the cache (Ctrl r, Ctrl F5): so I saw the hold by the 4 *.tt as required in step 4.

In particular: on OPAC, for a logged patron with holds, the lastpickupdate was in the "until" part of the "Status" column of the hold.
On circ/waitingreserves.tt (Home › Circulation › Holds awaiting pickup) the lastpickupdate was in the "until" part of the first column "Available" of "n Hold(s) waiting" tag.
 
The tables with holds with NULL lastpickupdate (holds created reserving items not checked out, as allowed by syspref AllowOnShelfHolds set to "Allow") were shown OK too.

So I pass the patch back to "Needs Signoff" status.

I've changed the syspref 'ReservesMaxPickUpDelay' from the default 0 value, thank you again.
Comment 90 Olli-Antti Kivilahti 2014-10-30 16:56:00 UTC
(In reply to Paola Rossi from comment #89)
> (In reply to Olli-Antti Kivilahti from comment #88)
> > [...] Hope to hear from you soon 
> 
> Thanks, Olli-Antti, for your kind feedbacks.
> 
> Today I've applied the patch against master 3.17.00.043.
> 
> After having applied, I've created a hold reserving an item that was checked
> out. So the reserve/hold's status became "W" after checking in it. On the
> browser I refreshed the cache (Ctrl r, Ctrl F5): so I saw the hold by the 4
> *.tt as required in step 4.
> 
> In particular: on OPAC, for a logged patron with holds, the lastpickupdate
> was in the "until" part of the "Status" column of the hold.
> On circ/waitingreserves.tt (Home › Circulation › Holds awaiting pickup) the
> lastpickupdate was in the "until" part of the first column "Available" of "n
> Hold(s) waiting" tag.
>  
> The tables with holds with NULL lastpickupdate (holds created reserving
> items not checked out, as allowed by syspref AllowOnShelfHolds set to
> "Allow") were shown OK too.
> 
> So I pass the patch back to "Needs Signoff" status.
> 
> I've changed the syspref 'ReservesMaxPickUpDelay' from the default 0 value,
> thank you again.

Nice to hear from you!

Is there anything here that prevents your from signing this off?
I believe this feature needs more than one sign off to be considered fit for QA and yours would be a great start!
Comment 91 Paola Rossi 2014-11-05 10:53:49 UTC
[I beg you pardon for the length of this comment, my delay in answering and possible inaccuracies.]

I've applied the patch against master 3.17.00.044 [HEAD 10860].

To test the patch IMHO on ADMIN it is better:
- (as required by the test plan) kept the value of syspref "ReservesMaxPickupDelay" > 0
- set the syspref "EnhancedMessagingPreferences" to ON (default is OFF)

To test the patch IMHO on ADMIN it could be better:
- kept the default value "Don't allow" to the syspref "AllowOnShelfHolds"
- set the "Charge" of the item-types to 0.00 (default 5.00) for all types
- [remember to set the circulation rules]
- set the syspref "KohaAdminEmailAddress" to a value not filtered by antispam's own emailing-system [to receive notices' email]
- [remeber to refresh the cache [Ctrl r - Ctrl F5] after seeing master's behaviour].

At my tests, the RESERVESLIP and HOLD notices are allowed to be cloned to your specific library, but I saw that koha uses only the "ALL LIBRARIES" notices, and never the cloned ones.
So I changed those 2 notices to test the 2 parts of the test plan about LETTER.

To test the part of the test plan about the RESERVESLIP notice, i.e. see below:
*** TEST: LETTER PLACEHOLDER ***,
IMHO the functions to test are "Confirm"/"Print and confirm", on cheching in a record already on hold by another patron.

To test the last part of the test plan about the HOLD notice, i.e. see below:
*** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION ****,
if I'm not wrong :
- set the notice's email [i.e. "Primary email"] of the test patron to your own email
- if this patron had a found/status W hold, you'll receive a mail soon after:
perl ./misc/cronjobs/advance_notices.pl  -c
perl ./misc/cronjobs/process_message_queue.pl

IMHO it would be better explicitly adding a further last part to the test plan about the provided:
t/db_dependent/Holds.t
[perhaps other parts of the preamble could need a testing action too.]

At my tests the upgrade of the DB is OK.

The 4 *.tt are OK.
NB1.: on ADMIN, in the "Circ and fines issuing rules", perhaps the new column "Holds wait for pickup (day)" could be better set at the right of "Holds allowed (count)", at the right of all the 4 columns about renewal, and not among these 4 ones.

NB2.: on CIRC, on waitingreserves.pl (="Holds awaiting pickup"), on the "Holds over" tag, the present string:
"Holds listed here have been awaiting pickup for more than days." could be modified. In master it was:
"Holds listed here have been awaiting pickup for more than X days." where X was the value of the syspref "ReservesMaxPickUpDelay" which has been removed by this patch.

NB3.: On opac-user.pl, for a logged in user with W holds, the W holds had not neigher the "Cancel" icon nor the "Suspend" icon. [I didn't check the master about this behaviour, sorry. If this patch had changed the behaviour, this regression would be on error, and this point will be added to the errors' list.]

PROBLEMS:

NB4.: [MASTER] at my tests the "W" found/status of a hold/reserve is set in only 1 case:
it is set to a hold, that previously had been opened on a biblio record which was already checked out (found=NULL), and then checked in (found=W) without "Ignore" submit. [I'd like to have a feedback about it, please, because all my tests were about this assumption.]
NB5.: [MASTER] So, at my tests, the waitingdate of a hold/reserve can be only the current date, i.e. the current day on which I check-in a biblio record. [I'd like to have a feedback about it, please, because all my tests were about this assumption.]

1) I saw no reserves.lastpickupdate nor the reserves.waitingdate on RESERVESLIP notice. But I think that this problem is out of this bug, because the master too behaves so. [On the contrary, on master (and after applying too) the reserves.reservesnotes is correctly shown by the notice.]
2) Whilst a check out lasts 1 more day for an intervening holiday, the hold's period doesn't: I'd like to have a feedback about this, please. If the W hold would last 1 more day, the patch would be in error.
3) On upgrading the DB, by syspref "ReservesMaxPickupDelay" > 0, all the holds for all the patrons' categories and item types would receive the same lasting period. Whilst the patch, once applied, could assign a different value to each of them. I think this remarkable to get coherent DB datas.
4) The found/status "W" meaning is about the lasting period available to the patron to pick up the document. But when a "Transfer" is needed from the check-out' library, to the hold's library, koha [master] doesn't set the found/status to "W", but to NULL/T: I think that this behaviour could be evaluated, that it can be a master's bug, and, if this is the case, that this patch could be tested after having resolved this bug on a DB with such cases.
5)  prove t/db_dependent/Holds.t
t/db_dependent/Holds.t .. 1/46
#   Failed test 'GetLastPickupDate(): Not using Calendar'
#   at t/db_dependent/Holds.t line 436.
#          got: '2014-11-10'
#     expected: '2014-11-05'
 
#   Failed test 'GetLastPickupDate(): Moving lastpickupdate over holidays, but not affected by them'
#   at t/db_dependent/Holds.t line 448.
#          got: '2014-11-10'
#     expected: '2014-11-05'
 
#   Failed test 'MoveWaitingdate(): Moving to past'
#   at t/db_dependent/Holds.t line 401.
#          got: ''
#     expected: '1'
 
#   Failed test 'Waiting reserve with lastpickupdate for 2014-11-04 totally canceled'
#   at t/db_dependent/Holds.t line 367.
#          got: '1'
#     expected: '0'
# Looks like you failed 4 tests of 46.
t/db_dependent/Holds.t .. Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/46 subtests
 
Test Summary Report
-------------------
t/db_dependent/Holds.t (Wstat: 1024 Tests: 46 Failed: 4)
  Failed tests:  37, 39, 41, 44
  Non-zero exit status: 4
Files=1, Tests=46,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.91 cusr  0.29 csys =  1.22 CPU)
Result: FAIL

For the 5-th problem I pass the patch to "Failed QA" status.
Comment 92 Paola Rossi 2014-11-06 13:21:45 UTC
I beg your pardon for the following comment, further in addition to the 91-th comment.

It's about the syspref "AllowOnShelfHolds". As the manual 3.16 says:
"Asks: ___ hold requests to be placed on items that are not checked out."

At my tests, the default value is "Don't allow" instead of the manual's default "Allow". The "Allow" status is the most interesting one, of course.

[on MASTER]
A) About a multi branch system, with "Transfer" from L1 to L2 ["if the library system is a multi branch system and patrons used the hold system to request items from other libraries"], the 3 status/found (of the just created hold, of the transferred hold by L1, and also after cancelling the Transfer by L2's receiving] are all NULL. IMHO the status/found of the hold could be "W" after cancelling.
B) About reserving from home ["if the library wanted to allow users to place holds on items [NOT OUT!] from home"], IMHO the status/found of the hold could be "W", but at my tests it's NULL.

If A) and B) be bugs, and fixed, all these new "W" cases of a hold should be tested in this patch 8367 too.
Comment 93 Paola Rossi 2014-12-23 16:14:09 UTC
I've tried to apply the patch against master 3.19.00.002

Applying: Bug 8367 - How long is a hold waiting for pickup at a more granular level
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Holds.t
Auto-merging svc/holds
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/holds.js
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging circ/waitingreserves.pl
Auto-merging C4/Reserves.pm
Auto-merging C4/Letters.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8367 - How long is a hold waiting for pickup at a more granular level

So I pass the patch to "Patch doesn't apply" status.
Comment 94 Joonas Kylmälä 2015-09-18 07:23:22 UTC
I'll start working on this and I decided to create a new patch and implement the same changes on it.
Comment 95 Joonas Kylmälä 2015-09-18 13:13:21 UTC Comment hidden (obsolete)
Comment 96 Joonas Kylmälä 2015-09-22 08:13:08 UTC
Created attachment 42754 [details] [review]
Bug 8367 - How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.
Comment 97 Joonas Kylmälä 2015-09-22 08:16:57 UTC
After this patch gets to master, the system preference ExpireReservesMaxPickUpDelay could to be changed to something like ExpireReservesAfterLastPickupDay. I guess it is then more descriptive. There was about 15-20 files depending on ExpireReservesMaxPickUpDelay preference, so it would be better to do maybe a new bug report for it?
Comment 98 I'm just a bot 2016-03-06 21:50:06 UTC
fatal: previous rebase directory .git/rebase-apply still exists but mbox given. at ./getter.pl line 196.
Comment 99 Hugh Rundle 2017-12-29 00:24:03 UTC
Is anyone still working on this? Being able to provide extended hold dates (e.g. for items set aside for our housebound patrons) would be very useful.
Comment 100 Alex Buckley 2018-10-02 00:21:56 UTC
Hi there

I have gone through and rebased this, master had progressed quite a lot since this patch was added and so I had to make a few assumptions during the rebase. 

Please let me know if changes need to be made as a result of the rebase. 

I have also corrected the commit messages to the format 'Bug XXXX: '

I have also written two other patches which correct template issues, and remove the check for the value of the 'ReserveMaxPickupDelay' syspref in Koha/Hold.pm in the set_waiting() function. 

This check is unneccessary as the main patch on this bug report removes the ReserveMaxPickupDelay syspref, and so I replaced this with a retrieval of the appropriate issuingrule and setting the exprationdate = waitingdate + issuingrule.lastpickupwait 

I am currently working on the unit tests t/db_dependent/Hold.t and so I am assigning this the status of 'Assigned' until I have attached a commit with the corrected t/db_dependent/Hold.t
Comment 101 Alex Buckley 2018-10-02 00:22:50 UTC
Created attachment 79790 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Conflicts:
	installer/data/mysql/updatedatabase.pl
Comment 102 Alex Buckley 2018-10-02 00:25:12 UTC
Created attachment 79791 [details] [review]
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm

Fixed waitingreserves.tt template so the borrowernumber is
successfully passed to waitingreserves.pl upon cancelling a hold, so
users can now successfully cancel holds from the waitingreserves.pl
interface.

Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this
syspref is removed in the previous patch. Instead the
expirationdate is calculated the same way as the lastpickupdatedate that is
retrieving the holdpickupwait value for the appropriate issuingrule and then
adding that onto the waiting date.

Same test plan as main patch

Sponsored-By: Brimbank Library, Australia
Comment 103 Alex Buckley 2018-10-02 01:28:15 UTC
Created attachment 79798 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Conflicts:
	installer/data/mysql/updatedatabase.pl
Comment 104 Alex Buckley 2018-10-02 01:28:39 UTC
Created attachment 79799 [details] [review]
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm

Fixed waitingreserves.tt template so the borrowernumber is
successfully passed to waitingreserves.pl upon cancelling a hold, so
users can now successfully cancel holds from the waitingreserves.pl
interface.

Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this
syspref is removed in the previous patch. Instead the
expirationdate is calculated the same way as the lastpickupdatedate that is
retrieving the holdpickupwait value for the appropriate issuingrule and then
adding that onto the waiting date.

Same test plan as main patch

Sponsored-By: Brimbank Library, Australia
Comment 105 Alex Buckley 2018-10-02 01:30:34 UTC
Created attachment 79800 [details] [review]
Bug 8367: Removed unneccessary warns and commented code

Sponsored-By: Brimbank Library, Australia
Comment 106 Alex Buckley 2018-10-02 22:10:59 UTC
Created attachment 79853 [details] [review]
Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir
Comment 107 Alex Buckley 2018-10-02 22:12:50 UTC
Created attachment 79854 [details] [review]
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves()

Added check for holidays to expirationdate calculation in
Koha/Hold.pm so the expirationdate respects holidays as the
lastpickupdate does.

Changed C4/Reserve/CancelExpiredReserves() to check the
reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds
if they have not been picked up within the time period specified in the
hold pickup delay defined in the issuing rules (which is added to
reserves.waitingdate to calculate the reserves.lastpickupdate) AND the
ExpireReservesMaxPickUpDelay syspref is enabled.

Sponsored-By: Brimbank Library, Australia
Comment 108 Alex Buckley 2018-10-03 00:17:40 UTC
Created attachment 79855 [details] [review]
Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled

Sponsored-By: Brimbank Library, Australia
Comment 109 Alex Buckley 2018-10-03 05:01:37 UTC
Created attachment 79859 [details] [review]
Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref

Sponsored-By: Brimbank Library, Australia
Comment 110 Katrin Fischer 2019-04-28 13:38:30 UTC
Hi Alex, is assigned the correct status here?
Comment 111 Laurie 2019-06-06 16:47:09 UTC
Is this bug still in process? We could really use the granularity to assign different wait items on different item types. Thank you
Comment 112 Hayley Pelham 2019-08-21 23:41:34 UTC
Created attachment 92412 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Conflicts:
	installer/data/mysql/updatedatabase.pl
Comment 113 Hayley Pelham 2019-08-21 23:45:07 UTC
Created attachment 92413 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Conflicts:
	installer/data/mysql/updatedatabase.pl
Comment 114 Hayley Pelham 2019-08-21 23:46:02 UTC
Created attachment 92414 [details] [review]
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm

Fixed waitingreserves.tt template so the borrowernumber is
successfully passed to waitingreserves.pl upon cancelling a hold, so
users can now successfully cancel holds from the waitingreserves.pl
interface.

Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this
syspref is removed in the previous patch. Instead the
expirationdate is calculated the same way as the lastpickupdatedate that is
retrieving the holdpickupwait value for the appropriate issuingrule and then
adding that onto the waiting date.

Same test plan as main patch

Sponsored-By: Brimbank Library, Australia
Comment 115 Hayley Pelham 2019-08-21 23:47:22 UTC
Created attachment 92415 [details] [review]
Bug 8367: Removed unneccessary warns and commented code

Sponsored-By: Brimbank Library, Australia
Comment 116 Hayley Pelham 2019-08-21 23:47:54 UTC
Created attachment 92416 [details] [review]
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm

Fixed waitingreserves.tt template so the borrowernumber is
successfully passed to waitingreserves.pl upon cancelling a hold, so
users can now successfully cancel holds from the waitingreserves.pl
interface.

Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this
syspref is removed in the previous patch. Instead the
expirationdate is calculated the same way as the lastpickupdatedate that is
retrieving the holdpickupwait value for the appropriate issuingrule and then
adding that onto the waiting date.

Same test plan as main patch

Sponsored-By: Brimbank Library, Australia
Comment 117 Hayley Pelham 2019-08-21 23:49:21 UTC
Created attachment 92417 [details] [review]
Bug 8367: Removed unneccessary warns and commented code

Sponsored-By: Brimbank Library, Australia
Comment 118 Hayley Pelham 2019-08-21 23:51:37 UTC
Created attachment 92418 [details] [review]
Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir
Comment 119 Hayley Pelham 2019-08-21 23:52:30 UTC
Created attachment 92419 [details] [review]
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves()

Added check for holidays to expirationdate calculation in
Koha/Hold.pm so the expirationdate respects holidays as the
lastpickupdate does.

Changed C4/Reserve/CancelExpiredReserves() to check the
reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds
if they have not been picked up within the time period specified in the
hold pickup delay defined in the issuing rules (which is added to
reserves.waitingdate to calculate the reserves.lastpickupdate) AND the
ExpireReservesMaxPickUpDelay syspref is enabled.

Sponsored-By: Brimbank Library, Australia
Comment 120 Hayley Pelham 2019-08-21 23:53:04 UTC
Created attachment 92420 [details] [review]
Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled

Sponsored-By: Brimbank Library, Australia
Comment 121 Hayley Pelham 2019-08-21 23:54:25 UTC
Created attachment 92421 [details] [review]
Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref

Sponsored-By: Brimbank Library, Australia
Comment 122 Hayley Pelham 2019-08-21 23:55:27 UTC
Created attachment 92422 [details] [review]
Bug 8367: (follow-up) Updates to DBIx class files

Ran the update_dbix_class_files.pl script to regenerate the
Koha/Schema/Result/ files again to match the database changes

Making the atomicupdate file idempotent

Sponsored-by: Brimbank Library, Australia
Authored-by: Hayley Mapley and Alex Buckley
Comment 123 Hayley Pelham 2019-08-21 23:56:22 UTC
Created attachment 92423 [details] [review]
Bug 8367: Fixes to make the reserves workflow work with new schema

Sponsored-by: Brimbank library, Australia
Comment 124 Hayley Pelham 2019-08-21 23:56:58 UTC
Created attachment 92424 [details] [review]
Bug 8367: Fixing display of lastpickupdate

Changed circ/waitingreserves.pl to correctly display lastpickupdate in
Circulation>Holds awaiting pickup.
Changed includes/holds-table.inc to display lastpickupdate in
opac-user.pl in Holds tab.

Sponsored-by: Brimbank District Council
Authored-by: Hayley Mapley and Alex Buckley (Catalyst IT)
Comment 125 Hayley Pelham 2019-08-22 00:45:48 UTC
Created attachment 92425 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the following 3 templates of staff client, you can see the 'Last pickup date' for a hold
   -circ/circulation.pl,
    Circulation>Check outs>Search for Patron>Holds tab
   -circ/waitingreserves.pl
    Circulation>Holds awaiting pickup
   -members/moremember.pl
   Patrons>Search for patron>Holds tab
5. In opac-user.pl the last pickup date is displayed in the Status column if available.
6. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Create and trap a hold
2) View waitingreserves.pl (Circulation>Holds awaiting pickup), note the last pickup date, and the pickup
   location
3) Cancel this hold
4) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
5) Repeat step 2
6) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD_SLIP, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower and print the slip. Verify that the
lastpickupdate is actually (currentdate + the 'Holds pickup wait' in
        your issuing rules) in days.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Originally Authored-by: Joonas Kylmälä <j.kylmala@gmail.com>
Taken up by: Hayley Mapley and Alex Buckley (Catalyst IT)
Sponsored-by: Brimbank District Council
Comment 126 Hayley Pelham 2019-08-22 00:46:14 UTC
Created attachment 92426 [details] [review]
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves()

Added check for holidays to expirationdate calculation in
Koha/Hold.pm so the expirationdate respects holidays as the
lastpickupdate does.

Changed C4/Reserve/CancelExpiredReserves() to check the
reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds
if they have not been picked up within the time period specified in the
hold pickup delay defined in the issuing rules (which is added to
reserves.waitingdate to calculate the reserves.lastpickupdate) AND the
ExpireReservesMaxPickUpDelay syspref is enabled.

Sponsored-By: Brimbank Library, Australia
Comment 127 Hayley Pelham 2019-08-22 00:47:14 UTC
Created attachment 92427 [details] [review]
Bug 8367: (follow-up) Updates to DBIx class files

Ran the update_dbix_class_files.pl script to regenerate the
Koha/Schema/Result/ files again to match the database changes

Making the atomicupdate file idempotent

Sponsored-by: Brimbank Library, Australia
Authored-by: Hayley Mapley and Alex Buckley
Comment 128 Hayley Pelham 2019-08-22 00:47:38 UTC
Created attachment 92428 [details] [review]
Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate

Changed circ/waitingreserves.pl to correctly display lastpickupdate in
Circulation>Holds awaiting pickup.
Changed includes/holds-table.inc to display lastpickupdate in
opac-user.pl in Holds tab.

Sponsored-by: Brimbank District Council
Authored-by: Hayley Mapley and Alex Buckley (Catalyst IT)
Comment 129 Stefan Berndtsson 2019-08-30 13:29:44 UTC
There seems to be a bug in Koha/Hold.pm, set_waiting() where GetReservesControlBranch() is called as a method rather than a function causing the package name to become the first argument.


This line:

my $controlbranch = C4::Reserves->GetReservesControlBranch( $item, $patron );

Should be:

my $controlbranch = C4::Reserves::GetReservesControlBranch( $item, $patron );


...if I'm not mistaken. The current code causes an error with checkin on the pickup library for the book.
Comment 130 David Gustafsson 2019-09-19 09:22:26 UTC
Created attachment 92958 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the following 3 templates of staff client, you can see the 'Last pickup date' for a hold
   -circ/circulation.pl,
    Circulation>Check outs>Search for Patron>Holds tab
   -circ/waitingreserves.pl
    Circulation>Holds awaiting pickup
   -members/moremember.pl
   Patrons>Search for patron>Holds tab
5. In opac-user.pl the last pickup date is displayed in the Status column if available.
6. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Create and trap a hold
2) View waitingreserves.pl (Circulation>Holds awaiting pickup), note the last pickup date, and the pickup
   location
3) Cancel this hold
4) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
5) Repeat step 2
6) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD_SLIP, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower and print the slip. Verify that the
lastpickupdate is actually (currentdate + the 'Holds pickup wait' in
        your issuing rules) in days.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Originally Authored-by: Joonas Kylmälä <j.kylmala@gmail.com>
Taken up by: Hayley Mapley and Alex Buckley (Catalyst IT)
Sponsored-by: Brimbank District Council
Comment 131 David Gustafsson 2019-09-19 09:22:31 UTC
Created attachment 92959 [details] [review]
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves()

Added check for holidays to expirationdate calculation in
Koha/Hold.pm so the expirationdate respects holidays as the
lastpickupdate does.

Changed C4/Reserve/CancelExpiredReserves() to check the
reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds
if they have not been picked up within the time period specified in the
hold pickup delay defined in the issuing rules (which is added to
reserves.waitingdate to calculate the reserves.lastpickupdate) AND the
ExpireReservesMaxPickUpDelay syspref is enabled.

Sponsored-By: Brimbank Library, Australia
Comment 132 David Gustafsson 2019-09-19 09:22:37 UTC
Created attachment 92960 [details] [review]
Bug 8367: (follow-up) Updates to DBIx class files

Ran the update_dbix_class_files.pl script to regenerate the
Koha/Schema/Result/ files again to match the database changes

Making the atomicupdate file idempotent

Sponsored-by: Brimbank Library, Australia
Authored-by: Hayley Mapley and Alex Buckley
Comment 133 David Gustafsson 2019-09-19 09:22:43 UTC
Created attachment 92961 [details] [review]
Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate

Changed circ/waitingreserves.pl to correctly display lastpickupdate in
Circulation>Holds awaiting pickup.
Changed includes/holds-table.inc to display lastpickupdate in
opac-user.pl in Holds tab.

Sponsored-by: Brimbank District Council
Authored-by: Hayley Mapley and Alex Buckley (Catalyst IT)
Comment 134 David Gustafsson 2019-09-19 09:29:25 UTC
It seems like this block was incorrectly removed, probably as part of recent rebase:

-                [% UNLESS ( borrowers ) %]
-                    [% IF borrowernumber and patron %]
-                            <div class="col-sm-2 col-sm-pull-10">
-                                <aside>
-                            [% INCLUDE 'circ-menu.inc' %]
-                                </aside>
-                            </div> <!-- /.col-sm-2 col-sm-pull-10 -->
-                    [% END %]
-                [% END %]

This makes the sidebar menu disappear on the circulation page. Removed this change from the "Bug 8367: How long is a hold waiting for pickup at a more granular level"-commit.
Comment 135 Stefan Berndtsson 2019-09-24 09:21:37 UTC
I'm really unclear of the relationship between the two columns lastpickupdate and expirationdate from this patch.

This patch introduces lastpickupdate, but set_waiting() sets expirationdate. Cancelling of an expired hold is then checked against lastpickupdate (used to be expirationdate).

All of the rest of the Koha code that is not affected by this patch uses expirationdate.

Why is lastpickupdate there? Why is cancelling comparing against something that set_waiting() didn't set?
Comment 136 Stefan Berndtsson 2019-09-24 10:57:58 UTC
This patch also explicitly sets 'W' without checking if it should be transferred or not.

To be more specific, it sets 'T' for transfer, then immediately after sets 'W' anyway, using an added ModReserveStatus() call.
Comment 137 Aleisha Amohia 2020-04-21 22:40:18 UTC
Created attachment 103425 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay
4) Place a hold on a biblio
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t

I've also added this to the TalkingTech_itiva_outbound.pl cronjob, I'm
not sure how to test this. Please test properly if you can, otherwise
maybe put some warns in and confirm the correct expiration date is
calculated.

Sponsored-by: Catalyst IT
Comment 138 David Nind 2020-09-28 10:32:21 UTC
Patch doesn't apply 8-(.. 

(The status was changed earlier in the month - not sure whether just changing the status sends a message or not).
Comment 139 Aleisha Amohia 2020-09-28 20:38:59 UTC
(In reply to David Nind from comment #138)
> Patch doesn't apply 8-(.. 
> 
> (The status was changed earlier in the month - not sure whether just
> changing the status sends a message or not).

It does! :)

This is on my list to get to when I have time.
Comment 140 Aleisha Amohia 2020-11-05 23:21:03 UTC
Created attachment 113211 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay
4) Place a hold on a biblio
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t

I've also added this to the TalkingTech_itiva_outbound.pl cronjob, I'm
not sure how to test this. Please test properly if you can, otherwise
maybe put some warns in and confirm the correct expiration date is
calculated.

Sponsored-by: Catalyst IT
Comment 141 Emmi Takkinen 2020-12-01 12:42:09 UTC
Patch doesn't apply.
Comment 142 Aleisha Amohia 2021-02-25 00:21:27 UTC
Created attachment 117284 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay
4) Place a hold on a biblio
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t

I've also added this to the TalkingTech_itiva_outbound.pl cronjob, I'm
not sure how to test this. Please test properly if you can, otherwise
maybe put some warns in and confirm the correct expiration date is
calculated.

Sponsored-by: Catalyst IT
Comment 143 Emmi Takkinen 2021-02-25 12:37:03 UTC
According to the bug 9004 you can test TalkingTech_itiva_outbound.pl by running: perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE. First enable "TalkingTechItivaPhoneNotification" and add content to HOLD slips Phone (itiva) part. However it doesn't seem to take holidays into account. Also there's a warning: 

> "my" variable $item masks earlier declaration in same scope at misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl line 347.
Comment 144 Kelly McElligott 2021-03-04 16:27:40 UTC
I love this feature!  While testing, I did find something that I think would need to change to make this work more efficiently- currently when a library is using their 'Holds Awaiting Pickup' report, there are two tabs.  Current Holds and Holds that are Watinging over X number of days.  This X is pulled from the system preference, ReservesMaxPickUpDelay.  However, now that Koha is giving the ability to have different pickup delays, could we change the tab to say " Holds Past Expiration Date"?  I also would love to see the expiration date to this report - however, I can file a separate bug for this if this is out of the scope.  

In addition, this should be documented that the Pickup Location is what is driving the rule- so where the patron chooses to pick the hold up is what rule this will follow. 

Finally, I am not sure about the itiva cron- I also saw the same issue as Emmi did while running this.
Comment 145 Joonas Kylmälä 2021-03-05 08:14:45 UTC
(In reply to Kelly McElligott from comment #144)
> I love this feature!  While testing, I did find something that I think would
> need to change to make this work more efficiently- currently when a library
> is using their 'Holds Awaiting Pickup' report, there are two tabs.  Current
> Holds and Holds that are Watinging over X number of days.  This X is pulled
> from the system preference, ReservesMaxPickUpDelay.  However, now that Koha
> is giving the ability to have different pickup delays, could we change the
> tab to say " Holds Past Expiration Date"?  I also would love to see the
> expiration date to this report - however, I can file a separate bug for this
> if this is out of the scope.  

This is really great feature idea and our librarians have been asking for it too! :) I would probably file it as separate bug, as it already now applies to the current solution, doesn't it? And maybe we would want to show the holds past expiration day on a day by day basis, like holds expired 2020-03-03, 2020-03-04 and so on (up until like 5 opening days? Or some configuration value for how many days to show?).
Comment 146 Aleisha Amohia 2021-03-07 19:39:15 UTC
Failing QA because of the itiva cron, will come back to this
Comment 147 Kelly McElligott 2021-03-08 12:25:05 UTC
I would say to keep it to two different tabs, one with active holds and ones that are past their expiration date,  maybe something like, Holds Past Expiration Date.  If we have more than 2 tabs, that could potentially be a lot of clicking for librarians to check every tab!
Comment 148 Aleisha Amohia 2021-03-10 03:31:28 UTC
Created attachment 118017 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT
Comment 149 Emmi Takkinen 2021-03-10 09:54:19 UTC
Created attachment 118021 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 150 Rhonda Kuiper 2021-05-19 20:33:42 UTC
What is the status of this fix?  We are eager to have this functionality for our hold lockers.  
Rhonda Kuiper
Comment 151 Katrin Fischer 2021-05-24 11:28:10 UTC
(In reply to Rhonda Kuiper from comment #150)
> What is the status of this fix?  We are eager to have this functionality for
> our hold lockers.  
> Rhonda Kuiper

At the moment this is waiting for QA. But as we are just about to release 21.05 it might wait a little longer while we focus on fixing bugs in the upcoming release.
Comment 152 Jonathan Druart 2021-05-24 12:01:13 UTC
Rhonda, it could help QA to add another signoff on the patch :)
Comment 153 Rhonda Kuiper 2021-05-28 16:11:57 UTC
I started the signoff and everything worked great on the staff side. The problem I saw was on step 8 of the test plan.  When I went to put a biblio on hold the message on the hold screen said "When your item is ready for pickup, you will have days to pick it up from this library".  There was no number before the 'days'.  I tried changing the library but the message still read the same.
Also, when I set a holiday within the pickup time, I put an item on hold and the expiration date didn't change (extend for one more day).
I didn't get to step 13 with the talking tech test.
Not sure what status to change this to...
Rhonda K.
Comment 154 Katrin Fischer 2021-05-29 15:24:51 UTC
If you think you found a problem, which it reads like, setting "Failed QA" is always the way to go. It usually alerts the dev to check what's going on. It's really a status for everyone to use.
Comment 155 David Cook 2021-09-07 00:47:03 UTC
We're interested in this one too. I might be able to take a look at this one.
Comment 156 David Cook 2021-09-07 06:54:01 UTC
The bug_8367-add_holds_pickup_period_circrule.perl script is not idempotent. It shouldn't re-add the rule on subsequent runs.
Comment 157 David Cook 2021-09-07 07:13:51 UTC
Created attachment 124568 [details] [review]
Bug 8367: Make database update idempotent
Comment 158 David Cook 2021-09-07 07:23:47 UTC
I'm finding this test plan a bit difficult to follow.

What does "7) Revert the waiting status" mean?

--

Repeating Rhonda's mention that the pickup message doesn't change for "8) Re-place the hold on the biblio on the OPAC. Notice that when you change the pick up location, the number of days in the pickup message below the dropdown changes based on the circ rules." 

Not seeing any relevant Javascript errors. I'll look into that next...
Comment 159 David Cook 2021-09-07 07:27:55 UTC
Comment on attachment 118021 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

Review of attachment 118021 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
@@ +486,5 @@
>          $(radios).first().attr("checked", "checked");
>      }
>  
> +    function calculate_delay_days(){
> +        var selected_branch = $("#branch_1").val();

This #branch_1 is wrong, as the HTML is populated using "branch_[% bibitemloo.biblionumber | html %]". The current code would only ever work for biblio 1.
Comment 160 David Cook 2021-09-07 07:40:20 UTC
Comment on attachment 118021 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

Review of attachment 118021 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
@@ +272,5 @@
>                                          </ul>
>  
> +                                        [% IF pickup_delays %]
> +                                            <p id="pickup_delay" style="display:none;">When your item is ready for pickup, you will have <span id="pickup_delay_days"></span> days to pick it up from this library.</p>
> +                                        [% END %]

This doesn't display for any bibs but the first one if you have multiple bibs to place holds on.

@@ +490,5 @@
> +        var selected_branch = $("#branch_1").val();
> +        [% FOREACH branchcode IN pickup_delays.keys %]
> +            var branchcode = "[% branchcode | html %]";
> +            if ( branchcode == selected_branch ) {
> +                $("#pickup_delay_days").text([% pickup_delays.$branchcode | html %]);

#pickup_delay_days can't work because it's not biblio-specific, so this will break if you're placing holds on more than 1 bib at a time.

@@ +499,4 @@
>      $(document).ready(function() {
> +        calculate_delay_days();
> +        $("#pickup_delay").show();
> +        $("#branch_1").change(function(){

This has the same selector problem because it's hard-coded to biblionumber 1.

::: opac/opac-reserve.pl
@@ +694,5 @@
> +    });
> +    $hold_pickup_delay_by_branch{$branch->branchcode} = $rule->rule_value if ( $rule and $rule->rule_value );
> +}
> +
> +$template->param( pickup_delays => \%hold_pickup_delay_by_branch );

"pickup_delays" is a confusing name, and it looks like there's no consideration of ReservesMaxPickUpDelay here, even though Hold.pm seems to take it into consideration... which could lead to confusing displays.
Comment 161 David Cook 2021-09-07 08:06:00 UTC
I don't think this patch takes into account C4::Reserves::CancelExpiredReserves which gets called by the cronjob /usr/share/koha/bin/cronjobs/holds/cancel_expired_holds.pl
Comment 162 David Cook 2021-09-16 02:21:28 UTC
Actually, I don't think we really need the changes to opac-reserve.pl and opac-reserve.tt. Those changes just add complexity. While they give patrons a little bit more information about when the hold expires, I don't know that we need to give that information at that point.
Comment 163 David Cook 2021-09-16 02:29:21 UTC
(In reply to David Cook from comment #161)
> I don't think this patch takes into account
> C4::Reserves::CancelExpiredReserves which gets called by the cronjob
> /usr/share/koha/bin/cronjobs/holds/cancel_expired_holds.pl

Actually, that's irrelevant. I was mixing up "ReservesMaxPickUpDelay" and "ExpireReservesMaxPickUpDelay". The description of "ExpireReservesMaxPickUpDelay" is actually a bit misleading and isn't relevant after all (since a user could nominate their own expiry regardless of ReservesMaxPickUpDelay).
Comment 164 David Cook 2021-09-21 05:08:19 UTC
(In reply to David Cook from comment #157)
> Created attachment 124568 [details] [review] [review]
> Bug 8367: Make database update idempotent

This query works on MariaDB 10.1.48 (used by koha-testing-docker), but it's not working for MariaDB 10.3.29 out in the wild on Debian 10...
Comment 165 David Cook 2021-09-21 05:23:38 UTC
Created attachment 125087 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.
Comment 166 Emmi Takkinen 2022-01-19 10:32:11 UTC
What's the status of this bug? Is it still Failed QA or could it be changed as Needs Signoff?
Comment 167 Aleisha Amohia 2022-03-02 21:05:16 UTC
Created attachment 131299 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 168 Aleisha Amohia 2022-03-02 21:05:25 UTC
Created attachment 131300 [details] [review]
Bug 8367: Make database update idempotent
Comment 169 Aleisha Amohia 2022-03-02 21:05:34 UTC
Created attachment 131301 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.
Comment 170 Aleisha Amohia 2022-03-02 21:05:44 UTC
Created attachment 131302 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits
Comment 171 Emmi Takkinen 2022-03-07 09:30:12 UTC
Sorry but I'm unable to repeat step 8, since number of days doesn't change when I change library. It seems number is based on "Standard rules for all libraries", not individual rules as described. After deleting standard rule, number of days is set by ReservesMaxPickUpDelay. 

Also tests fail for me:

not ok 12 - Expiration date should be set to today + 3 based on circulation rules
#   Failed test 'Expiration date should be set to today + 3 based on circulation rules'
#   at t/db_dependent/Holds/WaitingReserves.t line 244.
#          got: '2022-03-07'
#     expected: '2022-03-10'
not ok 13 - Expiration date should be set to today + 4 based on circulation rules and including a holiday
#   Failed test 'Expiration date should be set to today + 4 based on circulation rules and including a holiday'
#   at t/db_dependent/Holds/WaitingReserves.t line 266.
#          got: '2022-03-07'
#     expected: '2022-03-12'

Other than these, this seems to be working as described.
Comment 172 Aleisha Amohia 2022-03-14 03:35:40 UTC
Created attachment 131646 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.
Comment 173 Aleisha Amohia 2022-03-14 03:35:49 UTC
Created attachment 131647 [details] [review]
Bug 8367: (follow-up) Fix tests
Comment 174 Emmi Takkinen 2022-03-15 13:36:42 UTC
Created attachment 131718 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 175 Emmi Takkinen 2022-03-15 13:37:11 UTC
Created attachment 131719 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 176 Emmi Takkinen 2022-03-15 13:37:48 UTC
Created attachment 131720 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 177 Emmi Takkinen 2022-03-15 13:38:25 UTC
Created attachment 131722 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 178 Emmi Takkinen 2022-03-15 13:39:00 UTC
Created attachment 131723 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 179 Emmi Takkinen 2022-03-15 13:39:34 UTC
Created attachment 131724 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 180 Emmi Takkinen 2022-03-15 13:41:40 UTC
Tested, works as described.
Comment 181 Aleisha Amohia 2022-04-07 21:53:31 UTC
Created attachment 133089 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 182 Aleisha Amohia 2022-04-07 21:53:40 UTC
Created attachment 133090 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 183 Aleisha Amohia 2022-04-07 21:53:49 UTC
Created attachment 133091 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 184 Aleisha Amohia 2022-04-07 21:53:57 UTC
Created attachment 133092 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 185 Aleisha Amohia 2022-04-07 21:54:05 UTC
Created attachment 133093 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 186 Aleisha Amohia 2022-04-07 21:54:15 UTC
Created attachment 133094 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 187 Nick Clemens 2022-08-19 11:20:26 UTC
Hi Aleisha,

This looks great, but I found a few issues:

1 - Needs a minor rebase on top of article request rules
2 - The changes to iTiva outbound should be rebased on 31356 - essentially they aren't needed as we should use the already calculated expiration date
3 - I think the atomic update can simply be dropped - if the rule is not defined, it won't be used, no need to set it as blank
4 - When saving a more specific rule it is saved as blank, this still makes it the effective rule
    i.e.
    - Set a default rule with "Holds pickup period" as 25
    - Set syspref  ReservesMaxPickUpDelay to 1
    - Set an branch specific all/all rule leaving "Holds pickup period" blank
    - Place two holds - one for pickup at branch with specific rule, one at another
    - Sign in as the correct libraries and check in items and confirm holds
    - Branch with specific rule - hold gets 1 day delay
    - Other branch - hold gets 25 day delay
Comment 188 Katrin Fischer 2022-10-03 15:14:30 UTC
*** Bug 19309 has been marked as a duplicate of this bug. ***
Comment 189 Aleisha Amohia 2022-11-01 20:28:20 UTC
Created attachment 142897 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 190 Aleisha Amohia 2022-11-01 20:28:33 UTC
Created attachment 142898 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 191 Aleisha Amohia 2022-11-01 20:28:44 UTC
Created attachment 142899 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 192 Aleisha Amohia 2022-11-01 20:28:54 UTC
Created attachment 142900 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 193 Aleisha Amohia 2022-11-01 20:29:04 UTC
Created attachment 142901 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 194 Aleisha Amohia 2022-11-01 20:29:14 UTC
Created attachment 142902 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 195 Aleisha Amohia 2022-11-01 20:29:25 UTC
Created attachment 142903 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update
Comment 196 Emmi Takkinen 2022-11-11 09:36:45 UTC
Patches no longer apply.

Applying: Bug 8367: Add holds_pickup_period circulation rule
Using index info to reconstruct a base tree...
M	Koha/Hold.pm
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
M	opac/opac-reserve.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-reserve.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging Koha/Hold.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 8367: Add holds_pickup_period circulation rule
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-8367-Add-holdspickupperiod-circulation-rule-PsnpwK.patch
Comment 197 David Cook 2022-11-15 05:15:41 UTC
I'll try to keep my eyes upon to QA this one when it's ready.
Comment 198 Emmi Takkinen 2022-11-15 10:23:22 UTC
Note that there is a bug here similar to bug 30694 but with holds_pickup_period.
Comment 199 Aleisha Amohia 2022-11-21 02:50:48 UTC
Created attachment 144097 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 200 Aleisha Amohia 2022-11-21 02:50:57 UTC
Created attachment 144098 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 201 Aleisha Amohia 2022-11-21 02:52:16 UTC
Created attachment 144099 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 202 Aleisha Amohia 2022-11-21 02:52:27 UTC
Created attachment 144100 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 203 Aleisha Amohia 2022-11-21 02:52:38 UTC
Created attachment 144101 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 204 Aleisha Amohia 2022-11-21 02:52:47 UTC
Created attachment 144102 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 205 Aleisha Amohia 2022-11-21 02:52:56 UTC
Created attachment 144103 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update
Comment 206 Emmi Takkinen 2022-11-22 10:28:57 UTC
Created attachment 144146 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 207 Emmi Takkinen 2022-11-22 10:35:13 UTC
Tested this again and everything seems to work as described. However circulation rule with "Holds pickup period (day)" column filled cannot be deleted. There is a similar problem with column "Decreased loan period for high holds" (bug 30694) where rule isn't included to deletion in smart-rules.pl. Not sure what is a proper status for this, is it "Patch doesn't apply" or "Failed QA"?
Comment 208 Katrin Fischer 2022-11-22 10:38:34 UTC
(In reply to Emmi Takkinen from comment #207)
> Tested this again and everything seems to work as described. However
> circulation rule with "Holds pickup period (day)" column filled cannot be
> deleted. There is a similar problem with column "Decreased loan period for
> high holds" (bug 30694) where rule isn't included to deletion in
> smart-rules.pl. Not sure what is a proper status for this, is it "Patch
> doesn't apply" or "Failed QA"?

Hi Emmi, probably only needs a small follow-up, but Failed QA would be the correct one for now. Thx for linking the other bug already, that's a good clue.
Comment 209 Emmi Takkinen 2022-11-22 12:33:31 UTC
Settig this as Failed QA for reason mentioned in comment 207.
Comment 210 Aleisha Amohia 2023-01-25 02:31:54 UTC
Created attachment 145644 [details] [review]
Bug 8367: (follow-up) possible to delete circ rule
Comment 211 Emmi Takkinen 2023-01-25 07:21:17 UTC
Created attachment 145657 [details] [review]
Bug 8367: (follow-up) possible to delete circ rule

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 212 Nick Clemens 2023-03-06 20:06:15 UTC
Created attachment 147798 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 213 Nick Clemens 2023-03-06 20:06:21 UTC
Created attachment 147799 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 214 Nick Clemens 2023-03-06 20:06:26 UTC
Created attachment 147800 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 215 Nick Clemens 2023-03-06 20:06:32 UTC
Created attachment 147801 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 216 Nick Clemens 2023-03-06 20:06:37 UTC
Created attachment 147802 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 217 Nick Clemens 2023-03-06 20:06:43 UTC
Created attachment 147803 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 218 Nick Clemens 2023-03-06 20:06:48 UTC
Created attachment 147804 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 219 Nick Clemens 2023-03-06 20:06:53 UTC
Created attachment 147805 [details] [review]
Bug 8367: (follow-up) possible to delete circ rule

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 220 Nick Clemens 2023-03-06 20:06:59 UTC
Created attachment 147806 [details] [review]
Bug 8376: (QA follow-up) Update message on Hold waiting pickup report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 221 Nick Clemens 2023-03-06 20:07:04 UTC
Created attachment 147807 [details] [review]
Bug 8367: (QA follow-up) Improve display when value not set for rule

When a rule is not set for a speficic line, the value in the syspref
will be used. This should be made clear in the interface.

To test:
1 - Add a new rule for a specific item type
2 - Do not fill the 'Hold pickup period'
3 - Confirm that the rule defaults to the system preference
4 - Hover on 'Default' in the circ rules under 'Hold pickup period'
5 - Confirm the text is readable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 222 Nick Clemens 2023-03-06 20:11:07 UTC
Moving back to Signed off, need another pair of eyes here

After talking to educators here, they believe if a line is left blank, we should default back to the system preference, and I added a line to make that clear.

If this is the case, however, we do need to insert the blank value into the DB for all existing rules - otherwise rules will fall back to the most specific rule when undefined, and will use the system preference when blank - so we could end up having two behaviors

The patches should be squashed once a decision is made
Comment 223 David Cook 2023-03-08 01:45:43 UTC
I'm interested in this one, so planning to take a look. Have a few other things I need to do first, but just a FYI
Comment 224 ByWater Sandboxes 2023-03-22 13:20:31 UTC
Created attachment 148536 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 225 ByWater Sandboxes 2023-03-22 13:20:36 UTC
Created attachment 148537 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 226 ByWater Sandboxes 2023-03-22 13:20:41 UTC
Created attachment 148538 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 227 ByWater Sandboxes 2023-03-22 13:20:46 UTC
Created attachment 148539 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 228 ByWater Sandboxes 2023-03-22 13:20:51 UTC
Created attachment 148540 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 229 ByWater Sandboxes 2023-03-22 13:20:56 UTC
Created attachment 148541 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 230 ByWater Sandboxes 2023-03-22 13:21:00 UTC
Created attachment 148542 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 231 ByWater Sandboxes 2023-03-22 13:21:05 UTC
Created attachment 148543 [details] [review]
Bug 8367: (follow-up) possible to delete circ rule

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 232 ByWater Sandboxes 2023-03-22 13:21:10 UTC
Created attachment 148544 [details] [review]
Bug 8376: (QA follow-up) Update message on Hold waiting pickup report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=8367
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 233 ByWater Sandboxes 2023-03-22 13:21:15 UTC
Created attachment 148545 [details] [review]
Bug 8367: (QA follow-up) Improve display when value not set for rule

When a rule is not set for a speficic line, the value in the syspref
will be used. This should be made clear in the interface.

To test:
1 - Add a new rule for a specific item type
2 - Do not fill the 'Hold pickup period'
3 - Confirm that the rule defaults to the system preference
4 - Hover on 'Default' in the circ rules under 'Hold pickup period'
5 - Confirm the text is readable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 234 Aleisha Amohia 2023-04-27 22:20:57 UTC
There are lots of signoffs on these patches, can this be marked Passed QA?
Comment 235 Nick Clemens 2023-04-28 10:48:25 UTC
(In reply to Aleisha Amohia from comment #234)
> There are lots of signoffs on these patches, can this be marked Passed QA?

Hi Aleisha,

It appears I was wrong in comment 187, see comment 222

It is a question of how the fallback should work, so we either need a DB update, or to change the expectation for when a rule is not set
Comment 236 Katrin Fischer 2023-04-28 17:01:37 UTC
(In reply to Nick Clemens from comment #235)
> (In reply to Aleisha Amohia from comment #234)
> > There are lots of signoffs on these patches, can this be marked Passed QA?
> 
> Hi Aleisha,
> 
> It appears I was wrong in comment 187, see comment 222
> 
> It is a question of how the fallback should work, so we either need a DB
> update, or to change the expectation for when a rule is not set

It sounds like this needs a little bit more work?
Comment 237 Aleisha Amohia 2023-04-30 22:21:50 UTC
Created attachment 150437 [details] [review]
Bug 8376: (QA follow-up) Update message on Hold waiting pickup report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=8367
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 238 Aleisha Amohia 2023-04-30 22:21:57 UTC
Created attachment 150438 [details] [review]
Bug 8367: (QA follow-up) Improve display when value not set for rule

When a rule is not set for a speficic line, the value in the syspref
will be used. This should be made clear in the interface.

To test:
1 - Add a new rule for a specific item type
2 - Do not fill the 'Hold pickup period'
3 - Confirm that the rule defaults to the system preference
4 - Hover on 'Default' in the circ rules under 'Hold pickup period'
5 - Confirm the text is readable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 239 Aleisha Amohia 2023-04-30 22:22:03 UTC
Created attachment 150439 [details] [review]
Bug 8367: Add holds_pickup_period fallback circulation rule
Comment 240 Aleisha Amohia 2023-04-30 22:23:27 UTC
(In reply to Katrin Fischer from comment #236)
> (In reply to Nick Clemens from comment #235)
> > (In reply to Aleisha Amohia from comment #234)
> > > There are lots of signoffs on these patches, can this be marked Passed QA?
> > 
> > Hi Aleisha,
> > 
> > It appears I was wrong in comment 187, see comment 222
> > 
> > It is a question of how the fallback should work, so we either need a DB
> > update, or to change the expectation for when a rule is not set
> 
> It sounds like this needs a little bit more work?

Attached an atomic update patch, not sure how it affects the bug status.
Comment 241 Aleisha Amohia 2023-07-10 03:12:20 UTC
Created attachment 153235 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 242 Aleisha Amohia 2023-07-10 03:12:27 UTC
Created attachment 153237 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 243 Aleisha Amohia 2023-07-10 03:12:34 UTC
Created attachment 153238 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 244 Aleisha Amohia 2023-07-10 03:12:40 UTC
Created attachment 153239 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 245 Aleisha Amohia 2023-07-10 03:12:47 UTC
Created attachment 153240 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 246 Aleisha Amohia 2023-07-10 03:12:55 UTC
Created attachment 153241 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 247 Aleisha Amohia 2023-07-10 03:13:02 UTC
Created attachment 153242 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 248 Aleisha Amohia 2023-07-10 03:13:09 UTC
Created attachment 153243 [details] [review]
Bug 8367: (follow-up) possible to delete circ rule

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 249 Aleisha Amohia 2023-07-10 03:13:16 UTC
Created attachment 153244 [details] [review]
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=8367
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 250 Aleisha Amohia 2023-07-10 03:13:23 UTC
Created attachment 153245 [details] [review]
Bug 8367: (QA follow-up) Improve display when value not set for rule

When a rule is not set for a speficic line, the value in the syspref
will be used. This should be made clear in the interface.

To test:
1 - Add a new rule for a specific item type
2 - Do not fill the 'Hold pickup period'
3 - Confirm that the rule defaults to the system preference
4 - Hover on 'Default' in the circ rules under 'Hold pickup period'
5 - Confirm the text is readable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 251 Aleisha Amohia 2023-07-10 03:13:30 UTC
Created attachment 153246 [details] [review]
Bug 8367: Add holds_pickup_period fallback circulation rule
Comment 252 Katrin Fischer 2023-10-09 22:20:20 UTC
Working on this one, but will not finish today.
Comment 253 Katrin Fischer 2023-10-14 14:44:23 UTC
Created attachment 157138 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 254 Katrin Fischer 2023-10-14 14:44:30 UTC
Created attachment 157139 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 255 Katrin Fischer 2023-10-14 14:44:37 UTC
Created attachment 157140 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 256 Katrin Fischer 2023-10-14 14:44:44 UTC
Created attachment 157141 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 257 Katrin Fischer 2023-10-14 14:44:52 UTC
Created attachment 157142 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 258 Katrin Fischer 2023-10-14 14:44:59 UTC
Created attachment 157143 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 259 Katrin Fischer 2023-10-14 14:45:06 UTC
Created attachment 157144 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 260 Katrin Fischer 2023-10-14 14:45:13 UTC
Created attachment 157145 [details] [review]
Bug 8367: (follow-up) possible to delete circ rule

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 261 Katrin Fischer 2023-10-14 14:45:21 UTC
Created attachment 157146 [details] [review]
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=8367
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 262 Katrin Fischer 2023-10-14 14:45:29 UTC
Created attachment 157147 [details] [review]
Bug 8367: (QA follow-up) Improve display when value not set for rule

When a rule is not set for a speficic line, the value in the syspref
will be used. This should be made clear in the interface.

To test:
1 - Add a new rule for a specific item type
2 - Do not fill the 'Hold pickup period'
3 - Confirm that the rule defaults to the system preference
4 - Hover on 'Default' in the circ rules under 'Hold pickup period'
5 - Confirm the text is readable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 263 Katrin Fischer 2023-10-14 14:45:37 UTC
Created attachment 157148 [details] [review]
Bug 8367: Add holds_pickup_period fallback circulation rule

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 264 Katrin Fischer 2023-10-14 14:45:46 UTC
Created attachment 157149 [details] [review]
Bug 8367: (QA follow-up) Fix QA script

* Add +x to atomic database update file
* Perltidy
* Add spans to rewritten tab label for translatability

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 265 Katrin Fischer 2023-10-14 14:45:54 UTC
Created attachment 157150 [details] [review]
Bug 8367: Make sure "Default" is not copied when editing circulation rules

When editing a circulation rules the "Default" was copied into the
input field. This would cause problems on saving and especially with
translations. So now the field will be empty and appear as "Default"
after saving as we already do it for other values with "unlimited".

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 266 Katrin Fischer 2023-10-14 15:11:54 UTC
This is almost there, but we need a small fix for the circulation rules editor.

When a rule is edited and the Pickup period is "Default" (was saved as empty before), the "Default" shows up in the input field. This is bad as it will break on save and also when translated.

We need to make it show up as empty when the rule is edited. And as 0 when 0 was entered.

My patch didn't work and created a display issue at the beginning of the table (second column).

Aleisha, could you have a go please?
Comment 267 Aleisha Amohia 2023-10-18 01:40:08 UTC
Created attachment 157298 [details] [review]
Bug 8367: (follow-up) Fix for circ rules editor

Make sure holds pickup period field is empty when editing a 'default' rule.
Comment 268 Aleisha Amohia 2023-10-18 01:41:11 UTC
(In reply to Katrin Fischer from comment #266)
> This is almost there, but we need a small fix for the circulation rules
> editor.
> 
> When a rule is edited and the Pickup period is "Default" (was saved as empty
> before), the "Default" shows up in the input field. This is bad as it will
> break on save and also when translated.
> 
> We need to make it show up as empty when the rule is edited. And as 0 when 0
> was entered.
> 
> My patch didn't work and created a display issue at the beginning of the
> table (second column).
> 
> Aleisha, could you have a go please?

I don't know what the difference is between your patch that didn't work and the one I just attached, but it looks good to me!
Comment 269 ByWater Sandboxes 2023-10-18 19:09:36 UTC
Created attachment 157343 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 270 ByWater Sandboxes 2023-10-18 19:09:41 UTC
Created attachment 157344 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 271 ByWater Sandboxes 2023-10-18 19:09:47 UTC
Created attachment 157345 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 272 ByWater Sandboxes 2023-10-18 19:09:52 UTC
Created attachment 157346 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 273 ByWater Sandboxes 2023-10-18 19:09:57 UTC
Created attachment 157347 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 274 ByWater Sandboxes 2023-10-18 19:10:03 UTC
Created attachment 157348 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 275 ByWater Sandboxes 2023-10-18 19:10:08 UTC
Created attachment 157349 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 276 ByWater Sandboxes 2023-10-18 19:10:13 UTC
Created attachment 157350 [details] [review]
Bug 8367: (follow-up) possible to delete circ rule

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 277 ByWater Sandboxes 2023-10-18 19:10:18 UTC
Created attachment 157351 [details] [review]
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=8367
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 278 ByWater Sandboxes 2023-10-18 19:10:24 UTC
Created attachment 157352 [details] [review]
Bug 8367: (QA follow-up) Improve display when value not set for rule

When a rule is not set for a speficic line, the value in the syspref
will be used. This should be made clear in the interface.

To test:
1 - Add a new rule for a specific item type
2 - Do not fill the 'Hold pickup period'
3 - Confirm that the rule defaults to the system preference
4 - Hover on 'Default' in the circ rules under 'Hold pickup period'
5 - Confirm the text is readable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 279 ByWater Sandboxes 2023-10-18 19:10:29 UTC
Created attachment 157354 [details] [review]
Bug 8367: Add holds_pickup_period fallback circulation rule

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 280 ByWater Sandboxes 2023-10-18 19:10:34 UTC
Created attachment 157355 [details] [review]
Bug 8367: (QA follow-up) Fix QA script

* Add +x to atomic database update file
* Perltidy
* Add spans to rewritten tab label for translatability

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 281 ByWater Sandboxes 2023-10-18 19:10:39 UTC
Created attachment 157356 [details] [review]
Bug 8367: (follow-up) Fix for circ rules editor

Make sure holds pickup period field is empty when editing a 'default' rule.

Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 282 Katrin Fischer 2023-10-23 21:33:38 UTC
Kelly, it looks like you forgot to switch the status here and on some other bugs, please double check when signing off!
Comment 283 Katrin Fischer 2023-10-23 22:16:39 UTC
Hi Aleisha, 

I rebased the patch set (will re-apply my patches here), because bug 25393 was pushed before yours, also adding a new circulation condition to the table.

This means we need to adjust the column from 31 to 32, but I didn't write the follow-up for that, because I found what broke the behavior:

* Turn on UnseenRenewals system preference

It throws the column count...and then we are back at the default copying into the input field. 

Could you try to fix that?
Comment 284 Katrin Fischer 2023-10-23 22:18:15 UTC
Created attachment 157697 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 285 Katrin Fischer 2023-10-23 22:18:22 UTC
Created attachment 157698 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 286 Katrin Fischer 2023-10-23 22:18:29 UTC
Created attachment 157699 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 287 Katrin Fischer 2023-10-23 22:18:36 UTC
Created attachment 157700 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 288 Katrin Fischer 2023-10-23 22:18:43 UTC
Created attachment 157701 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 289 Katrin Fischer 2023-10-23 22:18:50 UTC
Created attachment 157702 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 290 Katrin Fischer 2023-10-23 22:18:57 UTC
Created attachment 157703 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 291 Katrin Fischer 2023-10-23 22:19:04 UTC
Created attachment 157704 [details] [review]
Bug 8367: (follow-up) possible to delete circ rule

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 292 Katrin Fischer 2023-10-23 22:19:11 UTC
Created attachment 157705 [details] [review]
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=8367
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 293 Katrin Fischer 2023-10-23 22:19:18 UTC
Created attachment 157706 [details] [review]
Bug 8367: (QA follow-up) Improve display when value not set for rule

When a rule is not set for a speficic line, the value in the syspref
will be used. This should be made clear in the interface.

To test:
1 - Add a new rule for a specific item type
2 - Do not fill the 'Hold pickup period'
3 - Confirm that the rule defaults to the system preference
4 - Hover on 'Default' in the circ rules under 'Hold pickup period'
5 - Confirm the text is readable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 294 Katrin Fischer 2023-10-23 22:19:25 UTC
Created attachment 157707 [details] [review]
Bug 8367: Add holds_pickup_period fallback circulation rule

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 295 Katrin Fischer 2023-10-23 22:19:33 UTC
Created attachment 157708 [details] [review]
Bug 8367: (QA follow-up) Fix QA script

* Add +x to atomic database update file
* Perltidy
* Add spans to rewritten tab label for translatability

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 296 Katrin Fischer 2023-10-23 22:19:40 UTC
Created attachment 157709 [details] [review]
Bug 8367: (follow-up) Fix for circ rules editor

Make sure holds pickup period field is empty when editing a 'default' rule.

Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 297 Aleisha Amohia 2023-10-24 00:15:15 UTC
Created attachment 157715 [details] [review]
Bug 8367: (follow-up) Fix for circ rules editor

Make sure holds pickup period field is empty when editing a 'default' rule.
Comment 298 Aleisha Amohia 2023-10-24 00:16:37 UTC
(In reply to Katrin Fischer from comment #283)
> Hi Aleisha, 
> 
> I rebased the patch set (will re-apply my patches here), because bug 25393
> was pushed before yours, also adding a new circulation condition to the
> table.
> 
> This means we need to adjust the column from 31 to 32, but I didn't write
> the follow-up for that, because I found what broke the behavior:
> 
> * Turn on UnseenRenewals system preference
> 
> It throws the column count...and then we are back at the default copying
> into the input field. 
> 
> Could you try to fix that?

Hi Katrin, I've done a new fix for this. I noticed the UnseenRenewals circ rule breaks a bunch of other rules in the same way, so maybe we can adopt this new way of identifying those columns in another bug report.
Comment 299 Katrin Fischer 2023-10-29 10:16:19 UTC
Created attachment 158023 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay. Set a few
different numbers for different branches as well.
4) Place a hold on a biblio from the staff client.
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio on the OPAC. Notice that when you
change the pick up location, the number of days in the pickup message
below the dropdown changes based on the circ rules.
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t
13) Test Talking Tech:
13a) Enable TalkingTechItivaPhoneNotification
13b) Go to Tools -> Notices & slips. Add content to the HOLD phone
(itiva) notice.
13c) In your terminal, run perl
/path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o
~/itiva.tmp -w 0 --type=RESERVE

Sponsored-by: Catalyst IT

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 300 Katrin Fischer 2023-10-29 10:16:27 UTC
Created attachment 158024 [details] [review]
Bug 8367: Make database update idempotent

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 301 Katrin Fischer 2023-10-29 10:16:34 UTC
Created attachment 158025 [details] [review]
Bug 8367: Make database update idempotent (across different MariaDB versions)

The previous query seemed to work on MariaDB 10.1.48 (used by koha-testing-docker),
but it didn't work on MariaDB 10.3.29 (default in Debian 10 as of this writing),
but this patched query works on both indempotently.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 302 Katrin Fischer 2023-10-29 10:16:42 UTC
Created attachment 158026 [details] [review]
Bug 8367: (follow-up) Fix pickup delay text on OPAC and other bits

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 303 Katrin Fischer 2023-10-29 10:16:50 UTC
Created attachment 158027 [details] [review]
Bug 8367: (follow-up) Remove pickup delay message from OPAC

It adds unnecessary complexity and information.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 304 Katrin Fischer 2023-10-29 10:16:58 UTC
Created attachment 158028 [details] [review]
Bug 8367: (follow-up) Fix tests

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 305 Katrin Fischer 2023-10-29 10:17:07 UTC
Created attachment 158029 [details] [review]
Bug 8367: (follow-up) Remove unnecessary code and atomic update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 306 Katrin Fischer 2023-10-29 10:17:16 UTC
Created attachment 158030 [details] [review]
Bug 8367: (follow-up) possible to delete circ rule

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 307 Katrin Fischer 2023-10-29 10:17:24 UTC
Created attachment 158031 [details] [review]
Bug 8367: (QA follow-up) Update message on Hold waiting pickup report

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=8367
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 308 Katrin Fischer 2023-10-29 10:17:32 UTC
Created attachment 158032 [details] [review]
Bug 8367: (QA follow-up) Improve display when value not set for rule

When a rule is not set for a speficic line, the value in the syspref
will be used. This should be made clear in the interface.

To test:
1 - Add a new rule for a specific item type
2 - Do not fill the 'Hold pickup period'
3 - Confirm that the rule defaults to the system preference
4 - Hover on 'Default' in the circ rules under 'Hold pickup period'
5 - Confirm the text is readable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 309 Katrin Fischer 2023-10-29 10:17:40 UTC
Created attachment 158033 [details] [review]
Bug 8367: Add holds_pickup_period fallback circulation rule

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 310 Katrin Fischer 2023-10-29 10:17:48 UTC
Created attachment 158034 [details] [review]
Bug 8367: (QA follow-up) Fix QA script

* Add +x to atomic database update file
* Perltidy
* Add spans to rewritten tab label for translatability

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 311 Katrin Fischer 2023-10-29 10:17:55 UTC
Created attachment 158035 [details] [review]
Bug 8367: (follow-up) Fix for circ rules editor

Make sure holds pickup period field is empty when editing a 'default' rule.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 312 Katrin Fischer 2023-10-29 10:20:10 UTC
(In reply to Aleisha Amohia from comment #298)
> > This means we need to adjust the column from 31 to 32, but I didn't write
> > the follow-up for that, because I found what broke the behavior:
> > 
> > * Turn on UnseenRenewals system preference
> > 
> > It throws the column count...and then we are back at the default copying
> > into the input field. 
> > 
> > Could you try to fix that?
> 
> Hi Katrin, I've done a new fix for this. I noticed the UnseenRenewals circ
> rule breaks a bunch of other rules in the same way, so maybe we can adopt
> this new way of identifying those columns in another bug report.

Hi Aleisha, thanks for that! I failed to come up with the right syntax :)
I have filed a new bug to deal with the pre-existing problem for the other circulation rules:

Bug 35182 - Activating "UnseenRenewals" causes problems in circulation rules editor

Happy to pass QA now.
Comment 313 Tomás Cohen Arazi 2023-11-01 21:03:34 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 314 Tomás Cohen Arazi 2023-11-02 13:22:23 UTC
Created attachment 158254 [details] [review]
Bug 8367: (QA follow-up) Fix broken tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 315 Tomás Cohen Arazi 2023-11-02 13:40:03 UTC
Created attachment 158255 [details] [review]
Bug 8367: (QA follow-up) Fix broken tests in Reserves.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 316 Fridolin Somers 2023-11-10 01:50:36 UTC
Enhancement not pushed to 23.05.x