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.
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.
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>
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>
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.
(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.
Created attachment 144901 [details] [review] Bug 32528: Change return logic Back to 1 return statement Prevent the Koha::Result::Boolean instanciation duplication
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.
Created attachment 144925 [details] [review] Bug 32528: Change return logic Back to 1 return statement Prevent the Koha::Result::Boolean instanciation duplication
(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.
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>
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
Missing dependencies for 22.05.x, no backport