We should add the option to include a warm-up period to add before a booking and a cool-down to add after a booking in the bookings system. This would allow for late returns and any maintenance that may need to take place before the next booking is due. This would also give us something clear to bind on for check-ins to trigger a 'ready for collection' type notice to the patron who booked the materials.
*** Bug 35670 has been marked as a duplicate of this bug. ***
Quoting Esther Melander: This is perhaps the beginning of a list of circulation rules needed for bookings. To replicate: 1. Find an item and create a booking to start tomorrow for Patron 1. 2. Check out the same item to Patron 2 today. The bookings work correctly in that the item checked out today is also due today. However, if that item actually went out the door with Patron 2 the chances of getting it back in time for the booking is zero. A circulation rule is needed to prevent circulation within so many days of a booking and/or a circulation alert is needed for staff to not let the book circulate because of a pending booking.
Work in progress for this is available at https://github.com/PTFS-Europe/koha/tree/bug_34440. I'm still refining the last pieces of it but will be asking for feedback imminently. The best way to help it continue to move forward at the moment however is to look at the dependencies and help those work their way through signoff and QA.
Created attachment 167948 [details] [review] Bug 34440: Add lead and trail time to circulation rules
Created attachment 167949 [details] [review] Bug 34440: Take preparation period into account at checkout This patch ensures we catch checkouts that would interfer with the preparation period set for an existing booking by adding said period to the bookings search. We also reduce the proposed due date for the checkout to be confirmed by the preparation period for the item. Test plan 1) Set a booking preparation period for an itemtype 2) Add a booking for an item with that itemtype to fall beyond the due date that the item would be assigned if you were to check it out now to your test user, but within the number of days you've set for your preparation period. 3) Attempt to check the item out to your test user. 4) Note that we catch the booking and warn and that the latest return date suggested take the preparation period into account.
Created attachment 167950 [details] [review] Bug 34440: Add itemtype selection to bookings modal Whilst we still don't need to know the specific item we will need to know the itemtype to be able to fetch the correct circulation rules for adjusting the datepicker with lead and trail times. This patch adds such a picker option. You should be able to select the item or itemtype in either order.. selecting itemtype first will limit the items list to options available (and 'Any item').. upon selecting a specific item the itemtype selector should become disabled and reflect the itemtype of the item selected.
Created attachment 167951 [details] [review] Bug 34440: Add handling for lead/trail display This patch adds code to insert classes into the flatpickr for lead and trail time on bookings and includes some scss additions to highlight said periods in display.
Created attachment 167952 [details] [review] Bug 34440: Add isSameOrBefore dayjs plugin
Created attachment 167953 [details] [review] Bug 34440: Simplify modal code using isSameOr* functions
Created attachment 167954 [details] [review] Bug 34440: Use circulation_rules for buffer timing
Created attachment 167955 [details] [review] Bug 34440: (TEMP) Comment 'rules' param Right now, the 'multi' flag used in the 'rules' query param in bug 36641 doesn't work as expected.
Created attachment 168941 [details] [review] Bug 34440: Add lead and trail time to circulation rules
Created attachment 168942 [details] [review] Bug 34440: Take preparation period into account at checkout This patch ensures we catch checkouts that would interfer with the preparation period set for an existing booking by adding said period to the bookings search. We also reduce the proposed due date for the checkout to be confirmed by the preparation period for the item. Test plan 1) Set a booking preparation period for an itemtype 2) Add a booking for an item with that itemtype to fall beyond the due date that the item would be assigned if you were to check it out now to your test user, but within the number of days you've set for your preparation period. 3) Attempt to check the item out to your test user. 4) Note that we catch the booking and warn and that the latest return date suggested take the preparation period into account.
Created attachment 168943 [details] [review] Bug 34440: Add itemtype selection to bookings modal Whilst we still don't need to know the specific item we will need to know the itemtype to be able to fetch the correct circulation rules for adjusting the datepicker with lead and trail times. This patch adds such a picker option. You should be able to select the item or itemtype in either order.. selecting itemtype first will limit the items list to options available (and 'Any item').. upon selecting a specific item the itemtype selector should become disabled and reflect the itemtype of the item selected.
Created attachment 168944 [details] [review] Bug 34440: Add handling for lead/trail display This patch adds code to insert classes into the flatpickr for lead and trail time on bookings and includes some scss additions to highlight said periods in display.
Created attachment 168945 [details] [review] Bug 34440: Add isSameOrBefore dayjs plugin
Created attachment 168946 [details] [review] Bug 34440: Simplify modal code using isSameOr* functions
Created attachment 168947 [details] [review] Bug 34440: Use circulation_rules for buffer timing
Created attachment 168948 [details] [review] Bug 34440: (TEMP) Comment 'rules' param Right now, the 'multi' flag used in the 'rules' query param in bug 36641 doesn't work as expected.
Created attachment 168949 [details] [review] Bug 34440: Update for change to /circulation_rules We updated the preceeding bug that introducing the /circulation_rules endpoint such that it always returns an array. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168998 [details] [review] Bug 34440: Add lead and trail time to circulation rules
Created attachment 168999 [details] [review] Bug 34440: Take preparation period into account at checkout This patch ensures we catch checkouts that would interfer with the preparation period set for an existing booking by adding said period to the bookings search. We also reduce the proposed due date for the checkout to be confirmed by the preparation period for the item. Test plan 1) Set a booking preparation period for an itemtype 2) Add a booking for an item with that itemtype to fall beyond the due date that the item would be assigned if you were to check it out now to your test user, but within the number of days you've set for your preparation period. 3) Attempt to check the item out to your test user. 4) Note that we catch the booking and warn and that the latest return date suggested take the preparation period into account.
Created attachment 169000 [details] [review] Bug 34440: Add itemtype selection to bookings modal Whilst we still don't need to know the specific item we will need to know the itemtype to be able to fetch the correct circulation rules for adjusting the datepicker with lead and trail times. This patch adds such a picker option. You should be able to select the item or itemtype in either order.. selecting itemtype first will limit the items list to options available (and 'Any item').. upon selecting a specific item the itemtype selector should become disabled and reflect the itemtype of the item selected.
Created attachment 169001 [details] [review] Bug 34440: Add handling for lead/trail display This patch adds code to insert classes into the flatpickr for lead and trail time on bookings and includes some scss additions to highlight said periods in display.
Created attachment 169002 [details] [review] Bug 34440: Add isSameOrBefore dayjs plugin
Created attachment 169003 [details] [review] Bug 34440: Simplify modal code using isSameOr* functions
Created attachment 169004 [details] [review] Bug 34440: Use circulation_rules for buffer timing
Created attachment 169005 [details] [review] Bug 34440: Update for change to /circulation_rules We updated the preceeding bug that introducing the /circulation_rules endpoint such that it always returns an array. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169006 [details] [review] Bug 34440: (follow-up) Enable flatpickr only once rules are populated
Created attachment 169007 [details] [review] Bug 34440: Highlight overlaping lead and trail periods This adds a further distinction between a lead/trail period and whether it would conflict with an existing booking or not.
I need some testing and feedback on this one at this point. At booking time, this is mostly visual.. i.e. we allow bookings to take place even though the lead or trail times may be conflicting. We highlight that an overlap is taking place by darkening the hinted area's for lead and trail times. I considered making this require an override but haven't added that code at this time. I've also not prevented these lead/trail overlaps at the server level.
I am moving through the test plan in a sandbox and will follow up tomorrow there. Re: "I need some testing and feedback on this one at this point. At booking time, this is mostly visual.. i.e. we allow bookings to take place even though the lead or trail times may be conflicting. We highlight that an overlap is taking place by darkening the hinted area's for lead and trail times. I considered making this require an override but haven't added that code at this time. I've also not prevented these lead/trail overlaps at the server level." Our use case for prep time/warm up and cool down is that the bookable item may need to travel from one branch to another for pickup, or it may need to be cleaned or otherwise re-set before another patron can use it. The prep time functions to ensure that the patron who booked the item cannot have it out these days, but no one else can reserve it at this time either. We have over 100 staff that may place bookings, so if we do not prevent bookings conflicting with warm up/cool down days there is a high chance of confusion/bookings not ready for pickup on the start date. Hope that helps!
Could I ask for more parameters on step 2 of the test plan? Add a booking for an item with that itemtype to fall beyond the due date that the item would be assigned if you were to check it out now to your test user, but within the number of days you've set for your preparation period. Am I correct in thinking that could be: I set an item type with 2 days of prep time on either end. I set up a booking where the prep time starts Monday, 7/22, the actual booking begins Wednesday, 7/24, and ends Monday, 7/29. The prep time at the end goes until Wednesday, 7/31. Try to check out the item with the booking to a patron on 7/22, and see if the warning describes pops up?
Created attachment 169571 [details] [review] Bug 34440: Add lead and trail time to circulation rules
Created attachment 169572 [details] [review] Bug 34440: Take preparation period into account at checkout This patch ensures we catch checkouts that would interfer with the preparation period set for an existing booking by adding said period to the bookings search. We also reduce the proposed due date for the checkout to be confirmed by the preparation period for the item. Test plan 1) Set a booking preparation period for an itemtype 2) Add a booking for an item with that itemtype to fall beyond the due date that the item would be assigned if you were to check it out now to your test user, but within the number of days you've set for your preparation period. 3) Attempt to check the item out to your test user. 4) Note that we catch the booking and warn and that the latest return date suggested take the preparation period into account.
Created attachment 169573 [details] [review] Bug 34440: Add itemtype selection to bookings modal Whilst we still don't need to know the specific item we will need to know the itemtype to be able to fetch the correct circulation rules for adjusting the datepicker with lead and trail times. This patch adds such a picker option. You should be able to select the item or itemtype in either order.. selecting itemtype first will limit the items list to options available (and 'Any item').. upon selecting a specific item the itemtype selector should become disabled and reflect the itemtype of the item selected.
Created attachment 169574 [details] [review] Bug 34440: Add handling for lead/trail display This patch adds code to insert classes into the flatpickr for lead and trail time on bookings and includes some scss additions to highlight said periods in display.
Created attachment 169575 [details] [review] Bug 34440: Add isSameOrBefore dayjs plugin
Created attachment 169576 [details] [review] Bug 34440: Simplify modal code using isSameOr* functions
Created attachment 169577 [details] [review] Bug 34440: Use circulation_rules for buffer timing
Created attachment 169578 [details] [review] Bug 34440: Update for change to /circulation_rules We updated the preceeding bug that introducing the /circulation_rules endpoint such that it always returns an array. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169579 [details] [review] Bug 34440: (follow-up) Enable flatpickr only once rules are populated
Created attachment 169580 [details] [review] Bug 34440: Highlight overlaping lead and trail periods This adds a further distinction between a lead/trail period and whether it would conflict with an existing booking or not.
Created attachment 169581 [details] [review] Bug 34440: Enforce lead/trail restrictions This patch adds rule enforcement into the user interface. When attempting to make a booking now, instead of just visually displaying the lead and trail period and highlighting when an overlap appears, we now block the ability to select a date when such an overlap case is found.
Thanks for the feedback Kristi. I've now added the code to block bookings that would infringe the lead/trail period. Currently, it only takes into account the additional periods for the item you are booking and not the surrounding bookings, however.. I think I may need to update it to take more into account. i.e it currently acts like: booking 1 --||-- lead | booking 2 | trail -- ||-- booking 3 Should it be the following in your opinion: booking 1 --| trail --||-- lead | booking 2 | trail -- ||-- lead | booking 3
(In reply to Kristi from comment #33) > Could I ask for more parameters on step 2 of the test plan? > > Add a booking for an item with that itemtype to fall beyond the due date > that the item would be assigned if you were to check it out now to your test > user, but within the number of days you've set for your preparation period. > > Am I correct in thinking that could be: > I set an item type with 2 days of prep time on either end. > I set up a booking where the prep time starts Monday, 7/22, the actual > booking begins Wednesday, 7/24, and ends Monday, 7/29. The prep time at the > end goes until Wednesday, 7/31. > > Try to check out the item with the booking to a patron on 7/22, and see if > the warning describes pops up? I'd be more than happy to have a call.. this is all rather involved so maybe a little video chat would help? It sounds like you've understood it correctly though. I envisage 'prep time = lead time' and it's about what you may need to do to prepare the item for the next user and 'trail time' to be a buffer for 'late returns' in the way I've written it.
Created attachment 169587 [details] [review] Bug 34440: Enforce lead/trail restrictions This patch adds rule enforcement into the user interface. When attempting to make a booking now, instead of just visually displaying the lead and trail period and highlighting when an overlap appears, we now block the ability to select a date when such an overlap case is found.
Created attachment 169588 [details] [review] Bug 34440: Enforce lead/trail restrictions This patch adds rule enforcement into the user interface. When attempting to make a booking now, instead of just visually displaying the lead and trail period and highlighting when an overlap appears, we now block the ability to select a date when such an overlap case is found.
Created attachment 170032 [details] [review] Bug 34440: Add lead and trail time to circulation rules
Created attachment 170033 [details] [review] Bug 34440: Take preparation period into account at checkout This patch ensures we catch checkouts that would interfer with the preparation period set for an existing booking by adding said period to the bookings search. We also reduce the proposed due date for the checkout to be confirmed by the preparation period for the item. Test plan 1) Set a booking preparation period for an itemtype 2) Add a booking for an item with that itemtype to fall beyond the due date that the item would be assigned if you were to check it out now to your test user, but within the number of days you've set for your preparation period. 3) Attempt to check the item out to your test user. 4) Note that we catch the booking and warn and that the latest return date suggested take the preparation period into account.
Created attachment 170034 [details] [review] Bug 34440: Add itemtype selection to bookings modal Whilst we still don't need to know the specific item we will need to know the itemtype to be able to fetch the correct circulation rules for adjusting the datepicker with lead and trail times. This patch adds such a picker option. You should be able to select the item or itemtype in either order.. selecting itemtype first will limit the items list to options available (and 'Any item').. upon selecting a specific item the itemtype selector should become disabled and reflect the itemtype of the item selected.
Created attachment 170035 [details] [review] Bug 34440: Add handling for lead/trail display This patch adds code to insert classes into the flatpickr for lead and trail time on bookings and includes some scss additions to highlight said periods in display.
Created attachment 170036 [details] [review] Bug 34440: Add isSameOrBefore dayjs plugin
Created attachment 170037 [details] [review] Bug 34440: Simplify modal code using isSameOr* functions
Created attachment 170038 [details] [review] Bug 34440: Use circulation_rules for buffer timing
Created attachment 170039 [details] [review] Bug 34440: Update for change to /circulation_rules We updated the preceeding bug that introducing the /circulation_rules endpoint such that it always returns an array. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 170040 [details] [review] Bug 34440: (follow-up) Enable flatpickr only once rules are populated
Created attachment 170041 [details] [review] Bug 34440: Highlight overlaping lead and trail periods This adds a further distinction between a lead/trail period and whether it would conflict with an existing booking or not.
Created attachment 170042 [details] [review] Bug 34440: Enforce lead/trail restrictions This patch adds rule enforcement into the user interface. When attempting to make a booking now, instead of just visually displaying the lead and trail period and highlighting when an overlap appears, we now block the ability to select a date when such an overlap case is found.
Created attachment 170046 [details] [review] Bug 34440: Add lead and trail time to circulation rules Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170047 [details] [review] Bug 34440: Take preparation period into account at checkout This patch ensures we catch checkouts that would interfer with the preparation period set for an existing booking by adding said period to the bookings search. We also reduce the proposed due date for the checkout to be confirmed by the preparation period for the item. Test plan 1) Set a booking preparation period for an itemtype 2) Add a booking for an item with that itemtype to fall beyond the due date that the item would be assigned if you were to check it out now to your test user, but within the number of days you've set for your preparation period. 3) Attempt to check the item out to your test user. 4) Note that we catch the booking and warn and that the latest return date suggested take the preparation period into account. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170048 [details] [review] Bug 34440: Add itemtype selection to bookings modal Whilst we still don't need to know the specific item we will need to know the itemtype to be able to fetch the correct circulation rules for adjusting the datepicker with lead and trail times. This patch adds such a picker option. You should be able to select the item or itemtype in either order.. selecting itemtype first will limit the items list to options available (and 'Any item').. upon selecting a specific item the itemtype selector should become disabled and reflect the itemtype of the item selected. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170049 [details] [review] Bug 34440: Add handling for lead/trail display This patch adds code to insert classes into the flatpickr for lead and trail time on bookings and includes some scss additions to highlight said periods in display. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170050 [details] [review] Bug 34440: Add isSameOrBefore dayjs plugin Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170051 [details] [review] Bug 34440: Simplify modal code using isSameOr* functions Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170052 [details] [review] Bug 34440: Use circulation_rules for buffer timing Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170053 [details] [review] Bug 34440: Update for change to /circulation_rules We updated the preceeding bug that introducing the /circulation_rules endpoint such that it always returns an array. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170054 [details] [review] Bug 34440: (follow-up) Enable flatpickr only once rules are populated Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170055 [details] [review] Bug 34440: Highlight overlaping lead and trail periods This adds a further distinction between a lead/trail period and whether it would conflict with an existing booking or not. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170056 [details] [review] Bug 34440: Enforce lead/trail restrictions This patch adds rule enforcement into the user interface. When attempting to make a booking now, instead of just visually displaying the lead and trail period and highlighting when an overlap appears, we now block the ability to select a date when such an overlap case is found. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
(In reply to Martin Renvoize from comment #45) > Thanks for the feedback Kristi. > > I've now added the code to block bookings that would infringe the lead/trail > period. > > Currently, it only takes into account the additional periods for the item > you are booking and not the surrounding bookings, however.. I think I may > need to update it to take more into account. > > i.e it currently acts like: > > booking 1 --||-- lead | booking 2 | trail -- ||-- booking 3 > > Should it be the following in your opinion: > > booking 1 --| trail --||-- lead | booking 2 | trail -- ||-- lead | booking 3 I discussed this with Kristi and she agree's that it should be the combination of periods as proposed in the example above. A- | TA >|< LB | -B- | TB >|< LC | -C I'll look at adding a following to deal with that ASAP
Created attachment 170589 [details] [review] Bug 34440: Add lead and trail time to circulation rulesa Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170590 [details] [review] Bug 34440: Take preparation period into account at checkout This patch ensures we catch checkouts that would interfer with the preparation period set for an existing booking by adding said period to the bookings search. We also reduce the proposed due date for the checkout to be confirmed by the preparation period for the item. Test plan 1) Set a booking preparation period for an itemtype 2) Add a booking for an item with that itemtype to fall beyond the due date that the item would be assigned if you were to check it out now to your test user, but within the number of days you've set for your preparation period. 3) Attempt to check the item out to your test user. 4) Note that we catch the booking and warn and that the latest return date suggested take the preparation period into account. Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170591 [details] [review] Bug 34440: Add itemtype selection to bookings modal Whilst we still don't need to know the specific item we will need to know the itemtype to be able to fetch the correct circulation rules for adjusting the datepicker with lead and trail times. This patch adds such a picker option. You should be able to select the item or itemtype in either order.. selecting itemtype first will limit the items list to options available (and 'Any item').. upon selecting a specific item the itemtype selector should become disabled and reflect the itemtype of the item selected. Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170592 [details] [review] Bug 34440: Add handling for lead/trail display This patch adds code to insert classes into the flatpickr for lead and trail time on bookings and includes some scss additions to highlight said periods in display. Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170593 [details] [review] Bug 34440: Add isSameOrBefore dayjs plugin Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170594 [details] [review] Bug 34440: Simplify modal code using isSameOr* functions Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170595 [details] [review] Bug 34440: Use circulation_rules for buffer timing Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170596 [details] [review] Bug 34440: Update for change to /circulation_rules We updated the preceeding bug that introducing the /circulation_rules endpoint such that it always returns an array. Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170597 [details] [review] Bug 34440: (follow-up) Enable flatpickr only once rules are populated Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170598 [details] [review] Bug 34440: Highlight overlaping lead and trail periods This adds a further distinction between a lead/trail period and whether it would conflict with an existing booking or not. Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170599 [details] [review] Bug 34440: Enforce lead/trail restrictions This patch adds rule enforcement into the user interface. When attempting to make a booking now, instead of just visually displaying the lead and trail period and highlighting when an overlap appears, we now block the ability to select a date when such an overlap case is found. Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Kristi Krueger <KKRUEGER@cuyahogalibrary.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170600 [details] [review] Bug 34440: Unit test for change to Koha::Item This adds a unit test for the new preparation_period handling introduced into the find_booking method of Koha::Item. Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Created attachment 170603 [details] [review] Bug 34440: Unit test for change to Koha::Item This adds a unit test for the new preparation_period handling introduced into the find_booking method of Koha::Item. Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 170610 [details] [review] Bug 34440: (QA follow-up) Tidy changes to t/db_dependent/Koha/Item.t
Something went wrong here when attaching the patches. I will try to fix, but please double check. Please don't forget to obsolete patches manually where you updated the commit message. Bug 34440 - Add warm-up and cool-down periods to bookings Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 34440: Add lead and trail time to circulation rules Applying: Bug 34440: Add lead and trail time to circulation rulesa Using index info to reconstruct a base tree... M Koha/CirculationRules.pm M admin/smart-rules.pl M installer/onboarding.pl M koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt ******No changes -- Patch already applied. Applying: Bug 34440: Take preparation period into account at checkout Applying: Bug 34440: Add itemtype selection to bookings modal Applying: Bug 34440: Add handling for lead/trail display Applying: Bug 34440: Add isSameOrBefore dayjs plugin Applying: Bug 34440: Simplify modal code using isSameOr* functions Applying: Bug 34440: Use circulation_rules for buffer timing Applying: Bug 34440: Update for change to /circulation_rules Applying: Bug 34440: (follow-up) Enable flatpickr only once rules are populated Applying: Bug 34440: Highlight overlaping lead and trail periods Applying: Bug 34440: Enforce lead/trail restrictions Applying: Bug 34440: Unit test for change to Koha::Item Applying: Bug 34440: (QA follow-up) Tidy changes to t/db_dependent/Koha/Item.t
Notes from RM testing :) 1) Terminology The code uses trail and lead, the GUI uses preparation and precaution, the bug description uses warm-up and cool-down periods. I have to admit all were new to me :) Can we standardize a little at least between GUI and bug? 2) Item type or Itemtype? The label on the bookings form used Itemtype, but then we also have: + placeholder: __("Item type"), We actually don't have that resolved in the terminology list either. Asking on Mattermost, something more to standardize! 2) GUI I have been wondering if adding "days" behind the input fields might help usability a bit or not. 3) Release notes please!!
Pushed for 24.11! Well done everyone, thank you!
It took me a long time to notice that this patchset was the culprit! https://jenkins.koha-community.org/job/Koha_Main/3019/consoleFull t/db_dependent/selenium/opac_holds.t is failing with 17:36:02 koha_1 | # Failed test 'Detail page - Place hold button should be absent. Authenticated - onshelfholds If all unavailable' 17:36:02 koha_1 | # at t/db_dependent/selenium/opac_holds.t line 402. 17:36:02 koha_1 | # got: 'Place hold' 17:36:02 koha_1 | # expected: 'Print' 17:36:02 koha_1 | # Looks like you failed 1 test of 7. 17:36:02 koha_1 | 17:36:02 koha_1 | # Failed test 'detail page' 17:36:02 koha_1 | # at t/db_dependent/selenium/opac_holds.t line 308. 17:36:02 koha_1 | # Looks like you failed 1 test of 5. 17:36:02 koha_1 | 17:36:02 koha_1 | # Failed test 'authenticated' 17:36:02 koha_1 | # at t/db_dependent/selenium/opac_holds.t line 310. Then lot of other tests, but I think we should focus on the first failure. You can recreate using: root@kohadevbox:koha$ perl ../misc4dev/run_tests.pl --run-all-tests Easier if you replace this line: - @prove_files = map { chomp ; $_ } qx{ ( find t/db_dependent/selenium -name '*.t' -not -name '00-onboarding.t' | sort ) ; ( find t xt -name '*.t' -not -path "t/db_dependent/selenium/*" | shuf ) }; + @prove_files = qw( t/db_dependent/selenium/01-installation.t t/db_dependent/selenium/opac_holds.t ); I have no idea what's going on, neither why it fails, but it fails :)
If I comment, in t/db_dependent/selenium/opac_holds.t: reset_data() that is after detail_page_hold_button_absent('Authenticated - onshelfholds If all unavailable'); Then cleanup_data();, I can see the problem on the UI It seems that opac/opac-detail.pl 701 $can_holds_be_placed = 702 $can_holds_be_placed || $patron && IsAvailableForItemLevelRequest( $item, $patron, undef ); This is 1 when it should be 0. IF I go to admin/smart-rules.pl, edit and save (ie. set default to circ rules), the test will pass!
(In reply to Jonathan Druart from comment #90) > If I comment, in t/db_dependent/selenium/opac_holds.t: > > reset_data() that is after > detail_page_hold_button_absent('Authenticated - onshelfholds If all > unavailable'); > > Then cleanup_data();, I can see the problem on the UI > > It seems that opac/opac-detail.pl > 701 $can_holds_be_placed = > 702 $can_holds_be_placed || $patron && > IsAvailableForItemLevelRequest( $item, $patron, undef ); > > This is 1 when it should be 0. > > IF I go to admin/smart-rules.pl, edit and save (ie. set default to circ > rules), the test will pass! I believe Martin is back at work this week, so hope he can help to shed some light on this!
Created attachment 170951 [details] [review] Bug 34440: bookings_lead_period and bookings_trail_period cant be set at categorycode level Follow Joubu's test plan. It should no longer fail. Error from eval Koha::CirculationRules->set_rules($params) at onboarding.pl was: set_rule cannot set 'bookings_trail_period' for a 'categorycode'! at /kohadevbox/koha/installer/onboarding.pl line 301.
Thanks Pedro, great work. I honestly have no idea how those lines got added in the first place to onboarding.. I don't remember adding them and certainly didn't test that.
Follow-up pushed.. lets see how Jenkins does now.
Thanks Pedro, nice one. It drove me crazy! I didn't noticed the error in the logs!
Reading the code: koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js 149 leadDays = rules.bookings_lead_period; 150 trailDays = rules.n; 819 const leadStart = startDate 820 ? startDate.subtract(leadDays, "day") 821 : hoverDate.subtract(leadDays, "day"); 822 const leadEnd = startDate ? startDate : hoverDate; I think we should test if startDate is defined. Then also if bookings_lead_period and bookings_trail_period are defined. I see, at least, "Uncaught TypeError: startDate is null" when no start date is selected.