Bug 40296

Summary: Bookings that are checked out do not have status updated to completed
Product: Koha Reporter: Lucas Gass (lukeg) <lucas>
Component: CirculationAssignee: Martin Renvoize (ashimema) <martin.renvoize>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: cubingguy714, gmcharlt, kyle.m.hall, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36789
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function: Bookings
Bug Depends on: 37601    
Bug Blocks:    
Attachments: Bug 40296: Mark booking as completed when issuing
Bug 40296: Mark booking as completed when issuing to booking patron
Bug 40296: (follow-up) Fix booking patron ownership and cancellation logic
Bug 40296: Add unit tests for booking status handling

Description Lucas Gass (lukeg) 2025-07-02 16:42:12 UTC
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
Comment 1 Lucas Gass (lukeg) 2025-07-02 17:58:17 UTC
It looks like this is because a booking that is checked out never has the status changed to 'completed'.
Comment 2 Lucas Gass (lukeg) 2025-07-02 20:18:29 UTC
Created attachment 183717 [details] [review]
Bug 40296: Mark booking as completed when issuing
Comment 3 Martin Renvoize (ashimema) 2025-07-03 10:55:32 UTC
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>
Comment 4 Martin Renvoize (ashimema) 2025-07-03 10:55:34 UTC
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
Comment 5 Martin Renvoize (ashimema) 2025-07-03 10:55:36 UTC
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
Comment 6 Eric Garcia 2025-07-03 17:09:25 UTC
Test B fails. Booking status remains as "new" instead of being updated to "cancelled" when checked out to Patron B.