Bug 40058

Summary: Move RevertWaitingStatus to Koha::Hold->revert_waiting()
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Pushed to main --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: emily.lamancusa, kyle, martin.renvoize, tomascohen
Version: MainKeywords: additional_work_needed
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40062
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:
Bug Depends on: 40055    
Bug Blocks: 17728, 40671    
Attachments: Bug 40058: Add Koha::Hold->revert_waiting()
Bug 40058: Remove RevertWaitingStatus()
Bug 40058: Adjust existing tests
Bug 40058: Add Koha::Hold->revert_waiting()
Bug 40058: Remove RevertWaitingStatus()
Bug 40058: Adjust existing tests
Bug 40058: Add Koha::Hold->revert_waiting()
Bug 40058: Remove RevertWaitingStatus()
Bug 40058: Adjust existing tests

Description Tomás Cohen Arazi (tcohen) 2025-06-03 13:01:30 UTC
This method should be moved to the Koha:: namespace.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-06-03 15:12:15 UTC
Created attachment 182924 [details] [review]
Bug 40058: Add Koha::Hold->revert_waiting()

This patch adds a new method to the `Koha::Hold` class. The method
follows the original implementation in C4::Reserves.

Tests are added to cover all the code paths.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass :-D
3. Sign off :-D
Comment 2 Tomás Cohen Arazi (tcohen) 2025-06-03 15:12:17 UTC
Created attachment 182925 [details] [review]
Bug 40058: Remove RevertWaitingStatus()
Comment 3 Tomás Cohen Arazi (tcohen) 2025-06-03 15:12:20 UTC
Created attachment 182926 [details] [review]
Bug 40058: Adjust existing tests
Comment 4 David Nind 2025-06-04 11:22:36 UTC
Created attachment 182941 [details] [review]
Bug 40058: Add Koha::Hold->revert_waiting()

This patch adds a new method to the `Koha::Hold` class. The method
follows the original implementation in C4::Reserves.

Tests are added to cover all the code paths.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass :-D
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2025-06-04 11:22:38 UTC
Created attachment 182942 [details] [review]
Bug 40058: Remove RevertWaitingStatus()

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2025-06-04 11:22:41 UTC
Created attachment 182943 [details] [review]
Bug 40058: Adjust existing tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Martin Renvoize (ashimema) 2025-08-01 10:57:47 UTC
Created attachment 184980 [details] [review]
Bug 40058: Add Koha::Hold->revert_waiting()

This patch adds a new method to the `Koha::Hold` class. The method
follows the original implementation in C4::Reserves.

Tests are added to cover all the code paths.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass :-D
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 8 Martin Renvoize (ashimema) 2025-08-01 10:57:49 UTC
Created attachment 184981 [details] [review]
Bug 40058: Remove RevertWaitingStatus()

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 9 Martin Renvoize (ashimema) 2025-08-01 10:57:52 UTC
Created attachment 184982 [details] [review]
Bug 40058: Adjust existing tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 10 Lucas Gass (lukeg) 2025-08-14 22:09:16 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 11 Emily Lamancusa (emlam) 2025-08-19 13:00:57 UTC
I'm afraid this has introduced a few new bugs - there are a few places in the code where RevertWaitingStatus was being used to revert other found statuses, which now throw an error 500:

- in request.tt, the "Revert waiting status" button's text changes to "Revert transit status" if the hold is in-transit, but under the hood it's the same code that calls revert_waiting either way

- when checking out an item with a found hold, if the user chooses to revert the status of the hold and check out the item, revert_waiting gets called regardless of which found status the item has

Any reason not to name the method revert_found and explicitly allow it to revert any found status?
Comment 12 Tomás Cohen Arazi (tcohen) 2025-08-19 13:14:16 UTC
(In reply to Emily Lamancusa (emlam) from comment #11)
> I'm afraid this has introduced a few new bugs - there are a few places in
> the code where RevertWaitingStatus was being used to revert other found
> statuses, which now throw an error 500:
> 
> - in request.tt, the "Revert waiting status" button's text changes to
> "Revert transit status" if the hold is in-transit, but under the hood it's
> the same code that calls revert_waiting either way
> 
> - when checking out an item with a found hold, if the user chooses to revert
> the status of the hold and check out the item, revert_waiting gets called
> regardless of which found status the item has
> 
> Any reason not to name the method revert_found and explicitly allow it to
> revert any found status?

Good catch. This is why we need 100% tests coverage in the code. I'll provide a follow-up in a separate bug.