Bug 40058 - Move RevertWaitingStatus to Koha::Hold->revert_waiting()
Summary: Move RevertWaitingStatus to Koha::Hold->revert_waiting()
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 40055
Blocks: 17728 40671
  Show dependency treegraph
 
Reported: 2025-06-03 13:01 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-10-17 13:53 UTC (History)
4 users (show)

See Also:
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:


Attachments
Bug 40058: Add Koha::Hold->revert_waiting() (11.50 KB, patch)
2025-06-03 15:12 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40058: Remove RevertWaitingStatus() (5.98 KB, patch)
2025-06-03 15:12 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40058: Adjust existing tests (12.97 KB, patch)
2025-06-03 15:12 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40058: Add Koha::Hold->revert_waiting() (11.54 KB, patch)
2025-06-04 11:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 40058: Remove RevertWaitingStatus() (6.03 KB, patch)
2025-06-04 11:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 40058: Adjust existing tests (13.01 KB, patch)
2025-06-04 11:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 40058: Add Koha::Hold->revert_waiting() (11.42 KB, patch)
2025-08-01 10:57 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40058: Remove RevertWaitingStatus() (6.10 KB, patch)
2025-08-01 10:57 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40058: Adjust existing tests (13.49 KB, patch)
2025-08-01 10:57 UTC, Martin Renvoize (ashimema)
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-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