Bug 40296 - Bookings that are checked out do not have status updated to completed
Summary: Bookings that are checked out do not have status updated to completed
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Martin Renvoize (ashimema)
QA Contact: Paul Derscheid
URL:
Keywords: additional_work_needed
Depends on: 37601
Blocks: 36789 40656
  Show dependency treegraph
 
Reported: 2025-07-02 16:42 UTC by Lucas Gass (lukeg)
Modified: 2025-08-15 15:53 UTC (History)
7 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function: Bookings


Attachments
Bug 40296: Mark booking as completed when issuing (1.45 KB, patch)
2025-07-02 20:18 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 40296: Mark booking as completed when issuing to booking patron (2.44 KB, patch)
2025-07-03 10:55 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40296: (follow-up) Fix booking patron ownership and cancellation logic (4.37 KB, patch)
2025-07-03 10:55 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40296: Add unit tests for booking status handling (6.87 KB, patch)
2025-07-03 10:55 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40296: Mark booking as completed when issuing to booking patron (2.44 KB, patch)
2025-07-07 06:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40296: (follow-up) Fix booking patron ownership and cancellation logic (4.12 KB, patch)
2025-07-07 06:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40296: Add unit tests for booking status handling (6.87 KB, patch)
2025-07-07 06:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40296: Mark booking as completed when issuing to booking patron (2.19 KB, patch)
2025-08-14 11:18 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40296: (follow-up) Fix booking patron ownership and cancellation logic (4.12 KB, patch)
2025-08-14 11:18 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40296: Add unit tests for booking status handling (6.65 KB, patch)
2025-08-14 11:18 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40296: (QA follow-up) Increment test count to include Test::NoWarnings (742 bytes, patch)
2025-08-14 11:18 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40296: Mark booking as completed when issuing to booking patron (2.25 KB, patch)
2025-08-14 11:19 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40296: (follow-up) Fix booking patron ownership and cancellation logic (4.18 KB, patch)
2025-08-14 11:19 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40296: Add unit tests for booking status handling (6.71 KB, patch)
2025-08-14 11:19 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40296: (QA follow-up) Increment test count to include Test::NoWarnings (801 bytes, patch)
2025-08-14 11:19 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40296: Fix failing Returns.t test (967 bytes, patch)
2025-08-15 14:42 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 Comment hidden (obsolete)
Comment 3 Martin Renvoize (ashimema) 2025-07-03 10:55:32 UTC Comment hidden (obsolete)
Comment 4 Martin Renvoize (ashimema) 2025-07-03 10:55:34 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize (ashimema) 2025-07-03 10:55:36 UTC Comment hidden (obsolete)
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.
Comment 7 Martin Renvoize (ashimema) 2025-07-04 14:58:34 UTC
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.
Comment 8 Hannah Dunne-Howrie 2025-07-04 15:15:57 UTC
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.
Comment 9 Martin Renvoize (ashimema) 2025-07-07 06:08:05 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize (ashimema) 2025-07-07 06:08:07 UTC
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
Comment 11 Martin Renvoize (ashimema) 2025-07-07 06:08:09 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize (ashimema) 2025-07-12 14:31:04 UTC
*** Bug 36789 has been marked as a duplicate of this bug. ***
Comment 13 Eric Garcia 2025-07-15 15:31:30 UTC
Test A Fails

Booking status is still "new" after checking out to the same patron
Comment 14 Paul Derscheid 2025-08-14 11:18:41 UTC
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>
Comment 15 Paul Derscheid 2025-08-14 11:18:43 UTC
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
Comment 16 Paul Derscheid 2025-08-14 11:18:46 UTC
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
Comment 17 Paul Derscheid 2025-08-14 11:18:48 UTC
Created attachment 185386 [details] [review]
Bug 40296: (QA follow-up) Increment test count to include Test::NoWarnings
Comment 18 Paul Derscheid 2025-08-14 11:19:26 UTC
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>
Comment 19 Paul Derscheid 2025-08-14 11:19:29 UTC
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>
Comment 20 Paul Derscheid 2025-08-14 11:19:31 UTC
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>
Comment 21 Paul Derscheid 2025-08-14 11:19:34 UTC
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>
Comment 22 Paul Derscheid 2025-08-14 11:21:49 UTC
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.
Comment 23 Lucas Gass (lukeg) 2025-08-14 22:09:24 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 24 Lucas Gass (lukeg) 2025-08-15 14:29:10 UTC
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)
Comment 25 Lucas Gass (lukeg) 2025-08-15 14:42:42 UTC
Created attachment 185454 [details] [review]
Bug 40296: Fix failing Returns.t test
Comment 26 Lucas Gass (lukeg) 2025-08-15 15:53:33 UTC
follow-up pushed to main