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 stable --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: emily.lamancusa, kyle, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40062
GIT URL: Initiative type: ---
Sponsorship status: --- Crowdfunding goal: 0
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
This development refactors and existing function into a class method. The idea is that the code gets simplified, and also improve performance as the method acts on the already instantiated object instead of fetching it from the DB, internally.
Version(s) released in:
25.11.00,25.05.04
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.
Comment 13 Paul Derscheid 2025-09-20 08:58:06 UTC
Nice work everyone!

Pushed to 25.05.x