Bug 40672

Summary: `desk_id` not cleared when `revert_found()` called
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Hold requestsAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Needs documenting --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: emily.lamancusa, fridolin.somers, gmcharlt, lisette, m.de.rooy, tomascohen
Version: Main   
Hardware: All   
OS: All   
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,25.05.04
Circulation function:
Bug Depends on: 40671, 24412    
Bug Blocks:    
Attachments: Bug 40672: Reset desk_id when reverting waiting holds
Bug 40672: Reset desk_id when reverting waiting holds
Bug 40672: Reset desk_id when reverting waiting holds

Description Tomás Cohen Arazi (tcohen) 2025-08-19 14:28:06 UTC
When `ModReserveAffect` is used to set a waiting status for a hold, if the session has a `desk_id` set, then the hold is marked as waiting and the `desk_id` is set for the hold.

Neither the original `RevertWaitingStatus` or the new `revert_found` methods reset the `desk_id`.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-08-19 14:50:29 UTC
Created attachment 185543 [details] [review]
Bug 40672: Reset desk_id when reverting waiting holds

This patch adds desk_id reset functionality to the revert_found method,
but only for waiting holds where desk_id is actually relevant.

Changes:
* Modified revert_found() to conditionally reset desk_id
* Only resets desk_id for waiting holds (found=W)
* Leaves desk_id unchanged for in transit (T) and in processing (P) holds
* Added comprehensive test coverage for all scenarios
* Updated POD documentation to explain desk_id behavior

The logic is conservative and only clears desk_id when it makes sense:
waiting holds have their desk assignment cleared since they are no longer
waiting at a specific desk, while transit and processing holds remain
unchanged as they do not use desk_id.

To test:
1. Apply patch
2. Run tests:
   $ ktd --shell
  k$ cd /kohadevbox/koha
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: All tests pass including new desk_id handling tests
3. Verify all hold test suites still pass:
  k$ prove t/db_dependent/Koha/Hold*.t
=> SUCCESS: All hold-related tests pass
4. Sign off :-D
Comment 2 Tomás Cohen Arazi (tcohen) 2025-08-19 14:52:18 UTC
I haven't found anything obvious for not doing it. My main goal is to fully document the behavior for this methods and make things explicit. If something's not correct we will fix the tests and change the code. But this seems about right.
Comment 3 Emily Lamancusa (emlam) 2025-08-19 19:06:50 UTC
Created attachment 185567 [details] [review]
Bug 40672: Reset desk_id when reverting waiting holds

This patch adds desk_id reset functionality to the revert_found method,
but only for waiting holds where desk_id is actually relevant.

Changes:
* Modified revert_found() to conditionally reset desk_id
* Only resets desk_id for waiting holds (found=W)
* Leaves desk_id unchanged for in transit (T) and in processing (P) holds
* Added comprehensive test coverage for all scenarios
* Updated POD documentation to explain desk_id behavior

The logic is conservative and only clears desk_id when it makes sense:
waiting holds have their desk assignment cleared since they are no longer
waiting at a specific desk, while transit and processing holds remain
unchanged as they do not use desk_id.

To test:
1. Apply patch
2. Run tests:
   $ ktd --shell
  k$ cd /kohadevbox/koha
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: All tests pass including new desk_id handling tests
3. Verify all hold test suites still pass:
  k$ prove t/db_dependent/Koha/Hold*.t
=> SUCCESS: All hold-related tests pass
4. Sign off :-D

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 4 Marcel de Rooy 2025-08-29 08:16:20 UTC
I love adding tests, but think we need some balance for maintainability too.
We are changing one line (code):
+                    ( $self->is_waiting ? ( desk_id => undef ) : () ),
And we are adding 80+ lines in the test.

I am passing QA for this one, but we are out of balance here (imho)..
Comment 5 Marcel de Rooy 2025-08-29 08:16:52 UTC
Created attachment 185886 [details] [review]
Bug 40672: Reset desk_id when reverting waiting holds

This patch adds desk_id reset functionality to the revert_found method,
but only for waiting holds where desk_id is actually relevant.

Changes:
* Modified revert_found() to conditionally reset desk_id
* Only resets desk_id for waiting holds (found=W)
* Leaves desk_id unchanged for in transit (T) and in processing (P) holds
* Added comprehensive test coverage for all scenarios
* Updated POD documentation to explain desk_id behavior

The logic is conservative and only clears desk_id when it makes sense:
waiting holds have their desk assignment cleared since they are no longer
waiting at a specific desk, while transit and processing holds remain
unchanged as they do not use desk_id.

To test:
1. Apply patch
2. Run tests:
   $ ktd --shell
  k$ cd /kohadevbox/koha
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: All tests pass including new desk_id handling tests
3. Verify all hold test suites still pass:
  k$ prove t/db_dependent/Koha/Hold*.t
=> SUCCESS: All hold-related tests pass
4. Sign off :-D

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Tomás Cohen Arazi (tcohen) 2025-08-29 11:52:28 UTC
(In reply to Marcel de Rooy from comment #4)
> I love adding tests, but think we need some balance for maintainability too.
> We are changing one line (code):
> +                    ( $self->is_waiting ? ( desk_id => undef ) : () ),
> And we are adding 80+ lines in the test.
> 
> I am passing QA for this one, but we are out of balance here (imho)..

I agree, I should've put the desk_id tests within the existing tests, right?
Comment 7 Lucas Gass (lukeg) 2025-08-29 17:10:37 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 8 Paul Derscheid 2025-09-20 08:59:23 UTC
Nice work everyone!

Pushed to 25.05.x
Comment 9 Fridolin Somers 2025-10-10 13:50:00 UTC
Depends on Bug 40671 not in 24.11.x