Bug 40671 - Expand Koha::Hold->revert_waiting to handle all found statuses
Summary: Expand Koha::Hold->revert_waiting to handle all found statuses
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 40058
Blocks: 21572 40672
  Show dependency treegraph
 
Reported: 2025-08-19 13:55 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-10-10 12:39 UTC (History)
5 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 40671: Refactor Koha::Hold->revert_waiting into revert_found (8.97 KB, patch)
2025-08-19 14:09 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40671: Update method calls from revert_waiting to revert_found (5.19 KB, patch)
2025-08-19 14:10 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40671: Refactor Koha::Hold->revert_waiting into revert_found (9.04 KB, patch)
2025-08-19 18:31 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 40671: Update method calls from revert_waiting to revert_found (5.25 KB, patch)
2025-08-19 18:31 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 40671: Refactor Koha::Hold->revert_waiting into revert_found (9.13 KB, patch)
2025-08-29 08:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40671: Update method calls from revert_waiting to revert_found (5.34 KB, patch)
2025-08-29 08:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40671: (QA follow-up) Fix hardcoded currency symbol (1.70 KB, patch)
2025-08-29 08:06 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 13:55:36 UTC
The original method -RevertWaitingStatus- was actually used to revert any `found` status, not just waiting. The `Koha::Hold->revert_waiting()` method implemented on bug 40058 throws an exception if the hold is not waiting. This causes errors in a couple places (reverting in transit holds, reverting in processing holds, etc).

We need to document things better and cover all the use cases with tests to avoid this. Will do my best.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-08-19 14:09:59 UTC
Created attachment 185541 [details] [review]
Bug 40671: Refactor Koha::Hold->revert_waiting into revert_found

This patch refactors the revert_waiting method to handle all found statuses
instead of just waiting holds, making it more versatile and consistent.

Changes:
* Renamed method from revert_waiting() to revert_found()
* Expanded functionality to handle all found statuses (W, T, P)
* Updated exception logic to check is_found instead of is_waiting
* Changed exception message from hold_not_waiting to hold_not_found
* Added comprehensive tests for all found statuses
* Updated POD documentation to reflect new behavior

The method now reverts any hold with a found status back to a regular
hold with priority 1, regardless of whether it was waiting, in transit,
or in processing.

To test:
1. Apply the patches
2. Run tests:
   $ ktd --shell
  k$ cd /kohadevbox/koha
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: All tests pass including new subtests for T and P statuses
3. Verify all hold test suites pass:
  k$ prove t/db_dependent/Koha/Hold*.t
=> SUCCESS: All hold-related tests pass
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2025-08-19 14:10:02 UTC
Created attachment 185542 [details] [review]
Bug 40671: Update method calls from revert_waiting to revert_found

This patch updates all existing calls to the renamed method throughout
the codebase to maintain compatibility.

Changes:
* Updated C4::Circulation calls to use revert_found()
* Updated C4::Reserves calls to use revert_found()
* Updated test files to use revert_found()
* Maintained all existing functionality and behavior

All existing functionality is preserved - this is purely a method
name change to reflect the expanded capability.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Emily Lamancusa (emlam) 2025-08-19 18:31:21 UTC
Created attachment 185562 [details] [review]
Bug 40671: Refactor Koha::Hold->revert_waiting into revert_found

This patch refactors the revert_waiting method to handle all found statuses
instead of just waiting holds, making it more versatile and consistent.

Changes:
* Renamed method from revert_waiting() to revert_found()
* Expanded functionality to handle all found statuses (W, T, P)
* Updated exception logic to check is_found instead of is_waiting
* Changed exception message from hold_not_waiting to hold_not_found
* Added comprehensive tests for all found statuses
* Updated POD documentation to reflect new behavior

The method now reverts any hold with a found status back to a regular
hold with priority 1, regardless of whether it was waiting, in transit,
or in processing.

To test:
1. Apply the patches
2. Run tests:
   $ ktd --shell
  k$ cd /kohadevbox/koha
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: All tests pass including new subtests for T and P statuses
3. Verify all hold test suites pass:
  k$ prove t/db_dependent/Koha/Hold*.t
=> SUCCESS: All hold-related tests pass
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 4 Emily Lamancusa (emlam) 2025-08-19 18:31:23 UTC
Created attachment 185563 [details] [review]
Bug 40671: Update method calls from revert_waiting to revert_found

This patch updates all existing calls to the renamed method throughout
the codebase to maintain compatibility.

Changes:
* Updated C4::Circulation calls to use revert_found()
* Updated C4::Reserves calls to use revert_found()
* Updated test files to use revert_found()
* Maintained all existing functionality and behavior

All existing functionality is preserved - this is purely a method
name change to reflect the expanded capability.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 5 Marcel de Rooy 2025-08-29 07:41:36 UTC
Looking here
Comment 6 Marcel de Rooy 2025-08-29 07:42:19 UTC
        * Proving /usr/share/koha/t/db_dependent/SIP/Transaction.t FAIL

        #   Failed test 'The fine is displayed on checkin when show_outstanding_amount is enabled'
        #   at /usr/share/koha/t/db_dependent/SIP/Transaction.t line 706.
         #          got: 'You owe 12.00yqsA for this item.'
        #     expected: 'You owe $12.00 for this item.'
         # Looks like you failed 1 test of 2.
Comment 7 Marcel de Rooy 2025-08-29 08:05:22 UTC
Will fix that one here now. Stumbled over it somewhere else recently too..
Comment 8 Marcel de Rooy 2025-08-29 08:06:38 UTC
Created attachment 185883 [details] [review]
Bug 40671: Refactor Koha::Hold->revert_waiting into revert_found

This patch refactors the revert_waiting method to handle all found statuses
instead of just waiting holds, making it more versatile and consistent.

Changes:
* Renamed method from revert_waiting() to revert_found()
* Expanded functionality to handle all found statuses (W, T, P)
* Updated exception logic to check is_found instead of is_waiting
* Changed exception message from hold_not_waiting to hold_not_found
* Added comprehensive tests for all found statuses
* Updated POD documentation to reflect new behavior

The method now reverts any hold with a found status back to a regular
hold with priority 1, regardless of whether it was waiting, in transit,
or in processing.

To test:
1. Apply the patches
2. Run tests:
   $ ktd --shell
  k$ cd /kohadevbox/koha
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: All tests pass including new subtests for T and P statuses
3. Verify all hold test suites pass:
  k$ prove t/db_dependent/Koha/Hold*.t
=> SUCCESS: All hold-related tests pass
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2025-08-29 08:06:39 UTC
Created attachment 185884 [details] [review]
Bug 40671: Update method calls from revert_waiting to revert_found

This patch updates all existing calls to the renamed method throughout
the codebase to maintain compatibility.

Changes:
* Updated C4::Circulation calls to use revert_found()
* Updated C4::Reserves calls to use revert_found()
* Updated test files to use revert_found()
* Maintained all existing functionality and behavior

All existing functionality is preserved - this is purely a method
name change to reflect the expanded capability.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2025-08-29 08:06:42 UTC
Created attachment 185885 [details] [review]
Bug 40671: (QA follow-up) Fix hardcoded currency symbol

It could be before or after the amount. Assuming that it does not
contain digits itself ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Lucas Gass (lukeg) 2025-08-29 16:47:16 UTC
Nice work everyone!

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

Pushed to 25.05.x
Comment 13 Fridolin Somers 2025-10-10 12:39:33 UTC
Depends on Bug 40058 not in 24.11.x