To recreate: 0. I am using k-t-d record #32, which includes 3 items. 1. Create a booking starting today. To do so I had to create one in the UI for tomorrow and then run: update bookings set start_date = '2025-07-02 06:00:00'; 2. Create a 2nd booking, choosing 'Any item'. I did 7/3 - 7/11. It should work 3. Now try to create another booking for 'Any item' with the same dates. You will get a 'Failure' at the top of the modal. The API response 'booking would conflict' I think that items that are checked out are being double counted in check_booking
It looks like this is because a booking that is checked out never has the status changed to 'completed'.
Created attachment 183717 [details] [review] Bug 40296: Mark booking as completed when issuing
Created attachment 183743 [details] [review] Bug 40296: Mark booking as completed when issuing to booking patron This patch adds logic to automatically mark a patron's booking as 'completed' when they check out the item they have booked. When AddIssue is called, the system now: - Checks if there's a conflicting booking for the checkout period - Marks the booking status as 'completed' if found This ensures booking status accurately reflects when items are actually checked out to fulfill bookings. Test plan: 1. Enable bookings system preference 2. Create a bookable item (set bookable = 1 in items table or via item editor) 3. Create a booking for a patron for that item with start date = today, end date = 7 days from now 4. Go to circulation and check out the item to the same patron who made the booking 5. Check the booking status in the database or via the bookings interface 6. Verify the booking status has changed from 'new' to 'completed' 7. Confirm the checkout was successful and shows normally in patron's checkouts list Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 183744 [details] [review] Bug 40296: (follow-up) Fix booking patron ownership and cancellation logic This follow-up addresses critical issues in the initial implementation: 1. **Patron Ownership Check**: Only mark bookings as 'completed' when the patron checking out the item is the same patron who made the booking 2. **Smart Cancellation**: When another patron checks out an item that conflicts with an existing booking: - Cancel the booking only if checkout overlaps with actual booking period - Respect lead/trail periods - don't cancel if conflict is only during preparation periods (allows librarian override scenarios) 3. **Date Logic**: Proper overlap detection using start_date and end_date rather than periods extended by lead time This prevents incorrect booking completion and handles librarian override scenarios appropriately. Test plan: 1. Apply previous patch and this follow-up 2. Enable bookings and create a bookable item Test A - Patron's own booking completion: 3. Create booking for Patron A: start = today, end = +7 days 4. Check out item to Patron A (due date = +5 days) 5. Verify booking status = 'completed' Test B - Another patron's booking cancellation: 6. Return the item, create new booking for Patron A: start = +2 days, end = +6 days 7. Check out item to Patron B (due date = +4 days) - may need override 8. Verify Patron A's booking status = 'cancelled' Test C - Lead period override scenario: 9. Return item, set bookings_lead_period circulation rule = 3 days 10. Create booking for Patron A: start = +5 days, end = +8 days 11. Check out item to Patron B (due date = +4 days) - conflicts with lead period 12. Override the booking conflict warning 13. Verify Patron A's booking status remains 'new' (not cancelled) 14. This respects librarian's decision that item will return in time Test D - No booking scenario: 15. Return item, ensure no active bookings exist 16. Check out item to any patron 17. Verify checkout works normally without errors
Created attachment 183745 [details] [review] Bug 40296: Add unit tests for booking status handling This patch adds test coverage for the new booking status management in AddIssue: Test scenarios covered: - Patron checking out their own booked item (marks as completed) - Different patron checking out booked item during actual booking period (cancels booking) - Checkout conflicts only during lead period (preserves booking, respects librarian decision) - Checkout period encompassing entire booking period (cancels booking) - No booking present (normal operation) Tests ensure proper booking lifecycle management and validate that librarian override scenarios work as intended. To test: 1. Apply all patches 2. Run: prove t/db_dependent/Circulation.t 3. Verify all booking status handling scenarios pass The test plans now provide: - Step-by-step instructions for manual testing - Multiple scenarios covering different use cases - Clear expectations for what should happen in each case - Practical workflow that staff would actually follow - Edge cases like lead period conflicts and overrides
Test B fails. Booking status remains as "new" instead of being updated to "cancelled" when checked out to Patron B.
Ooops.. that test plan includes functionality that doesn't yet exist.. the override option! I'll update the test plan.. the code is ready for the override option later, but for now we don't need to test it as it's not actually exposed in the UI yet.
Yes, we'd want to see a complete workflow for Bookings that mirrors the behaviour of the holds workflow. This is what library staff and library users will expect, i.e. check out of the item completes the booking so that we can then report and monitor bookings in the way we do holds.
Created attachment 183820 [details] [review] Bug 40296: Mark booking as completed when issuing to booking patron This patch adds logic to automatically mark a patron's booking as 'completed' when they check out the item they have booked. When AddIssue is called, the system now: - Checks if there's a conflicting booking for the checkout period - Marks the booking status as 'completed' if found This ensures booking status accurately reflects when items are actually checked out to fulfill bookings. Test plan: 1. Enable bookings system preference 2. Create a bookable item (set bookable = 1 in items table or via item editor) 3. Create a booking for a patron for that item with start date = today, end date = 7 days from now 4. Go to circulation and check out the item to the same patron who made the booking 5. Check the booking status in the database or via the bookings interface 6. Verify the booking status has changed from 'new' to 'completed' 7. Confirm the checkout was successful and shows normally in patron's checkouts list Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 183821 [details] [review] Bug 40296: (follow-up) Fix booking patron ownership and cancellation logic This follow-up addresses critical issues in the initial implementation: 1. **Patron Ownership Check**: Only mark bookings as 'completed' when the patron checking out the item is the same patron who made the booking 2. **Smart Cancellation**: When another patron checks out an item that conflicts with an existing booking: - Cancel the booking only if checkout overlaps with actual booking period - Respect lead/trail periods - don't cancel if conflict is only during preparation periods (allows librarian override scenarios) 3. **Date Logic**: Proper overlap detection using start_date and end_date rather than periods extended by lead time This prevents incorrect booking completion and handles librarian override scenarios appropriately. Test plan: 1. Apply previous patch and this follow-up 2. Enable bookings and create a bookable item Test A - Patron's own booking completion: 3. Create booking for Patron A: start = today, end = +7 days 4. Check out item to Patron A (due date = +5 days) 5. Verify booking status = 'completed' Test B - Lead period override scenario: 11. Return item, set bookings_lead_period circulation rule = 3 days 12. Create booking for Patron A: start = +5 days, end = +8 days 13. Check out item to Patron B (due date = +4 days) - conflicts with lead period 14. Override the booking conflict warning 15. Verify Patron A's booking status remains 'new' (not cancelled) 16. This respects librarian's decision that item will return in time Test C - No booking scenario: 17. Return item, ensure no active bookings exist 18. Check out item to any patron 19. Verify checkout works normally without errors
Created attachment 183822 [details] [review] Bug 40296: Add unit tests for booking status handling This patch adds test coverage for the new booking status management in AddIssue: Test scenarios covered: - Patron checking out their own booked item (marks as completed) - Different patron checking out booked item during actual booking period (cancels booking) - Checkout conflicts only during lead period (preserves booking, respects librarian decision) - Checkout period encompassing entire booking period (cancels booking) - No booking present (normal operation) Tests ensure proper booking lifecycle management and validate that librarian override scenarios work as intended. To test: 1. Apply all patches 2. Run: prove t/db_dependent/Circulation.t 3. Verify all booking status handling scenarios pass The test plans now provide: - Step-by-step instructions for manual testing - Multiple scenarios covering different use cases - Clear expectations for what should happen in each case - Practical workflow that staff would actually follow - Edge cases like lead period conflicts and overrides
*** Bug 36789 has been marked as a duplicate of this bug. ***
Test A Fails Booking status is still "new" after checking out to the same patron
Created attachment 185383 [details] [review] Bug 40296: Mark booking as completed when issuing to booking patron This patch adds logic to automatically mark a patron's booking as 'completed' when they check out the item they have booked. When AddIssue is called, the system now: - Checks if there's a conflicting booking for the checkout period - Marks the booking status as 'completed' if found This ensures booking status accurately reflects when items are actually checked out to fulfill bookings. Test plan: 1. Enable bookings system preference 2. Create a bookable item (set bookable = 1 in items table or via item editor) 3. Create a booking for a patron for that item with start date = today, end date = 7 days from now 4. Go to circulation and check out the item to the same patron who made the booking 5. Check the booking status in the database or via the bookings interface 6. Verify the booking status has changed from 'new' to 'completed' 7. Confirm the checkout was successful and shows normally in patron's checkouts list Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 185384 [details] [review] Bug 40296: (follow-up) Fix booking patron ownership and cancellation logic This follow-up addresses critical issues in the initial implementation: 1. **Patron Ownership Check**: Only mark bookings as 'completed' when the patron checking out the item is the same patron who made the booking 2. **Smart Cancellation**: When another patron checks out an item that conflicts with an existing booking: - Cancel the booking only if checkout overlaps with actual booking period - Respect lead/trail periods - don't cancel if conflict is only during preparation periods (allows librarian override scenarios) 3. **Date Logic**: Proper overlap detection using start_date and end_date rather than periods extended by lead time This prevents incorrect booking completion and handles librarian override scenarios appropriately. Test plan: 1. Apply previous patch and this follow-up 2. Enable bookings and create a bookable item Test A - Patron's own booking completion: 3. Create booking for Patron A: start = today, end = +7 days 4. Check out item to Patron A (due date = +5 days) 5. Verify booking status = 'completed' Test B - Lead period override scenario: 11. Return item, set bookings_lead_period circulation rule = 3 days 12. Create booking for Patron A: start = +5 days, end = +8 days 13. Check out item to Patron B (due date = +4 days) - conflicts with lead period 14. Override the booking conflict warning 15. Verify Patron A's booking status remains 'new' (not cancelled) 16. This respects librarian's decision that item will return in time Test C - No booking scenario: 17. Return item, ensure no active bookings exist 18. Check out item to any patron 19. Verify checkout works normally without errors
Created attachment 185385 [details] [review] Bug 40296: Add unit tests for booking status handling This patch adds test coverage for the new booking status management in AddIssue: Test scenarios covered: - Patron checking out their own booked item (marks as completed) - Different patron checking out booked item during actual booking period (cancels booking) - Checkout conflicts only during lead period (preserves booking, respects librarian decision) - Checkout period encompassing entire booking period (cancels booking) - No booking present (normal operation) Tests ensure proper booking lifecycle management and validate that librarian override scenarios work as intended. To test: 1. Apply all patches 2. Run: prove t/db_dependent/Circulation.t 3. Verify all booking status handling scenarios pass The test plans now provide: - Step-by-step instructions for manual testing - Multiple scenarios covering different use cases - Clear expectations for what should happen in each case - Practical workflow that staff would actually follow - Edge cases like lead period conflicts and overrides
Created attachment 185386 [details] [review] Bug 40296: (QA follow-up) Increment test count to include Test::NoWarnings
Created attachment 185387 [details] [review] Bug 40296: Mark booking as completed when issuing to booking patron This patch adds logic to automatically mark a patron's booking as 'completed' when they check out the item they have booked. When AddIssue is called, the system now: - Checks if there's a conflicting booking for the checkout period - Marks the booking status as 'completed' if found This ensures booking status accurately reflects when items are actually checked out to fulfill bookings. Test plan: 1. Enable bookings system preference 2. Create a bookable item (set bookable = 1 in items table or via item editor) 3. Create a booking for a patron for that item with start date = today, end date = 7 days from now 4. Go to circulation and check out the item to the same patron who made the booking 5. Check the booking status in the database or via the bookings interface 6. Verify the booking status has changed from 'new' to 'completed' 7. Confirm the checkout was successful and shows normally in patron's checkouts list Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 185388 [details] [review] Bug 40296: (follow-up) Fix booking patron ownership and cancellation logic This follow-up addresses critical issues in the initial implementation: 1. **Patron Ownership Check**: Only mark bookings as 'completed' when the patron checking out the item is the same patron who made the booking 2. **Smart Cancellation**: When another patron checks out an item that conflicts with an existing booking: - Cancel the booking only if checkout overlaps with actual booking period - Respect lead/trail periods - don't cancel if conflict is only during preparation periods (allows librarian override scenarios) 3. **Date Logic**: Proper overlap detection using start_date and end_date rather than periods extended by lead time This prevents incorrect booking completion and handles librarian override scenarios appropriately. Test plan: 1. Apply previous patch and this follow-up 2. Enable bookings and create a bookable item Test A - Patron's own booking completion: 3. Create booking for Patron A: start = today, end = +7 days 4. Check out item to Patron A (due date = +5 days) 5. Verify booking status = 'completed' Test B - Lead period override scenario: 11. Return item, set bookings_lead_period circulation rule = 3 days 12. Create booking for Patron A: start = +5 days, end = +8 days 13. Check out item to Patron B (due date = +4 days) - conflicts with lead period 14. Override the booking conflict warning 15. Verify Patron A's booking status remains 'new' (not cancelled) 16. This respects librarian's decision that item will return in time Test C - No booking scenario: 17. Return item, ensure no active bookings exist 18. Check out item to any patron 19. Verify checkout works normally without errors Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 185389 [details] [review] Bug 40296: Add unit tests for booking status handling This patch adds test coverage for the new booking status management in AddIssue: Test scenarios covered: - Patron checking out their own booked item (marks as completed) - Different patron checking out booked item during actual booking period (cancels booking) - Checkout conflicts only during lead period (preserves booking, respects librarian decision) - Checkout period encompassing entire booking period (cancels booking) - No booking present (normal operation) Tests ensure proper booking lifecycle management and validate that librarian override scenarios work as intended. To test: 1. Apply all patches 2. Run: prove t/db_dependent/Circulation.t 3. Verify all booking status handling scenarios pass The test plans now provide: - Step-by-step instructions for manual testing - Multiple scenarios covering different use cases - Clear expectations for what should happen in each case - Practical workflow that staff would actually follow - Edge cases like lead period conflicts and overrides Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 185390 [details] [review] Bug 40296: (QA follow-up) Increment test count to include Test::NoWarnings Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Rebased onto current upstream/main and passed QA. Sorry for the bloat, forgot to sign off on the first attachment. I followed the test plan and got the expected results, added a commit to increment the test count for Test::NoWarnings (caused test failure on qa). Awesome change, thanks Martin.
Nice work everyone! Pushed to main for 25.11
t/db_dependent/Circulation/Returns.t is now failing: # Failed test 'Item without itemtype set raises warning on AddIssue' # at /kohadevbox/koha/t/db_dependent/Circulation/Returns.t line 147. # found warning: item-level_itypes set but no itemtype set for item (1100) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 1100. # found warning: item-level_itypes set but no itemtype set for item (1100) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 1100. # found warning: item-level_itypes set but no itemtype set for item (1100) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 1100. # found warning: item-level_itypes set but no itemtype set for item (1100) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 1100. # expected to find warning: (?^u:^item-level_itypes set but no itemtype set for item) # expected to find warning: (?^u:^item-level_itypes set but no itemtype set for item) # expected to find warning: (?^u:^item-level_itypes set but no itemtype set for item) ok 3 - biblio-level itype recorded on statistics for return as a fallback for null item-level itype # Looks like you failed 1 test of 3. not ok 1 - AddReturn logging on statistics table (item-level_itypes=1)
Created attachment 185454 [details] [review] Bug 40296: Fix failing Returns.t test
follow-up pushed to main