[Bug 40672] New: `desk_id` not cleared when `revert_found()` called
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Bug ID: 40672 Summary: `desk_id` not cleared when `revert_found()` called Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, lisette@bywatersolutions.com, tomascohen@gmail.com 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`. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185543 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185543&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |40671 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40671 [Bug 40671] Expand Koha::Hold->revert_waiting to handle all found statuses -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |24412 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24412 [Bug 24412] Attach waiting hold to desk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185543|0 |1 is obsolete| | --- Comment #3 from Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 185567 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185567&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |emily.lamancusa@montgomeryc | |ountymd.gov -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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).. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|Trivial patch |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185567|0 |1 is obsolete| | --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 185886 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185886&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 --- Comment #6 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 --- Comment #7 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|25.11.00 |25.11.00,25.05.04 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 --- Comment #8 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Nice work everyone! Pushed to 25.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Needs documenting CC| |fridolin.somers@biblibre.co | |m --- Comment #9 from Fridolin Somers <fridolin.somers@biblibre.com> --- Depends on Bug 40671 not in 24.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40672 Bug 40672 depends on bug 40671, which changed state. Bug 40671 Summary: Expand Koha::Hold->revert_waiting to handle all found statuses https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40671 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org