Bug 40672 - `desk_id` not cleared when `revert_found()` called
Summary: `desk_id` not cleared when `revert_found()` called
Status: Needs documenting
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: Marcel de Rooy
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-10-10 13:50 UTC (History)
6 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,25.05.04
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
Bug 40672: Reset desk_id when reverting waiting holds (6.22 KB, patch)
2025-08-29 08:16 UTC, Marcel de Rooy
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>
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