Bug 40672 - `desk_id` not cleared when `revert_found()` called
Summary: `desk_id` not cleared when `revert_found()` called
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on: 40671 24412
Blocks:
  Show dependency treegraph
 
Reported: 2025-08-19 14:28 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-08-19 19:07 UTC (History)
4 users (show)

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


Attachments
Bug 40672: Reset desk_id when reverting waiting holds (6.05 KB, patch)
2025-08-19 14:50 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40672: Reset desk_id when reverting waiting holds (6.12 KB, patch)
2025-08-19 19:06 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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>