Bug 32528 - Koha::Item->safe_to_delete should short-circuit earlier
Summary: Koha::Item->safe_to_delete should short-circuit earlier
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 32529
Blocks:
  Show dependency treegraph
 
Reported: 2022-12-23 19:53 UTC by Tomás Cohen Arazi
Modified: 2023-06-08 22:32 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.02


Attachments
Bug 32528: Mkae safe_to_delete exit earlier on error conditions (2.31 KB, patch)
2022-12-26 18:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32528: Mkae safe_to_delete exit earlier on error conditions (2.37 KB, patch)
2022-12-27 13:07 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32528: Change return logic (2.40 KB, patch)
2023-01-02 14:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32528: Change return logic (2.14 KB, patch)
2023-01-03 08:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32528: Make safe_to_delete exit earlier on error conditions (2.08 KB, patch)
2023-01-03 15:24 UTC, Jonathan Druart
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 2022-12-23 19:53:53 UTC
The method oges through all the possible blockers for item deletion, but only returns the last one. We could really return on the first one.

Plus: the 'book_reserved' error message is really about a having a waiting or in transit hold, the message could be more specific.
Comment 1 Tomás Cohen Arazi 2022-12-26 18:42:14 UTC
Koha::Item->safe_to_delete checks 5 conditions, most of which involve querying the DB. It should return right after one condition is met, but it keeps checking the rest of them.

This can be particularyl painful on batch deletions. And is incorrect regardless.
Comment 2 Tomás Cohen Arazi 2022-12-26 18:46:21 UTC
Created attachment 144842 [details] [review]
Bug 32528: Mkae safe_to_delete exit earlier on error conditions

This patch makes the `Koha::Item->safe_to_delete` method return right
after one of the checked conditions is met. This way we avoid further DB
queries and processing.

No behavior change is expected

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests STILL pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Nick Clemens (kidclamp) 2022-12-27 13:07:56 UTC
Created attachment 144847 [details] [review]
Bug 32528: Mkae safe_to_delete exit earlier on error conditions

This patch makes the `Koha::Item->safe_to_delete` method return right
after one of the checked conditions is met. This way we avoid further DB
queries and processing.

No behavior change is expected

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests STILL pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Martin Renvoize 2022-12-30 07:57:15 UTC
Do we care about the order in which these failure messages are returned here?

We effectively reverse their priority here by returning early.  If that matters we could re-arrange the code blocks and stick achieve the early returns.
Comment 5 Tomás Cohen Arazi 2022-12-30 11:43:46 UTC
(In reply to Martin Renvoize from comment #4)
> Do we care about the order in which these failure messages are returned here?
> 
> We effectively reverse their priority here by returning early.  If that
> matters we could re-arrange the code blocks and stick achieve the early
> returns.

All of them are blocking the action, so it feels correct.
Comment 6 Jonathan Druart 2023-01-02 14:22:30 UTC
Created attachment 144901 [details] [review]
Bug 32528: Change return logic

Back to 1 return statement
Prevent the Koha::Result::Boolean instanciation duplication
Comment 7 Jonathan Druart 2023-01-02 14:32:12 UTC
This patch seems to simplify and make the method more readable, but I guess it's a matter of taste. Feel free to obsolete it.
Comment 8 Jonathan Druart 2023-01-03 08:54:02 UTC
Created attachment 144925 [details] [review]
Bug 32528: Change return logic

Back to 1 return statement
Prevent the Koha::Result::Boolean instanciation duplication
Comment 9 Tomás Cohen Arazi 2023-01-03 14:11:24 UTC
(In reply to Jonathan Druart from comment #8)
> Created attachment 144925 [details] [review] [review]
> Bug 32528: Change return logic
> 
> Back to 1 return statement
> Prevent the Koha::Result::Boolean instanciation duplication

I agree with the follow-up.
Comment 10 Jonathan Druart 2023-01-03 15:24:13 UTC
Created attachment 144938 [details] [review]
Bug 32528: Make safe_to_delete exit earlier on error conditions

This patch makes the `Koha::Item->safe_to_delete` method return right
after one of the checked conditions is met. This way we avoid further DB
queries and processing.

No behavior change is expected

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests STILL pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Bug 32528: Change return logic

Back to 1 return statement
Prevent the Koha::Result::Boolean instanciation duplication

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Tomás Cohen Arazi 2023-01-11 23:55:59 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 12 Jacob O'Mara 2023-01-13 16:25:10 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 13 Lucas Gass 2023-01-24 18:32:06 UTC
Missing dependencies for 22.05.x, no backport