Summary: | Koha::Item->safe_to_delete should short-circuit earlier | ||
---|---|---|---|
Product: | Koha | Reporter: | Tomás Cohen Arazi <tomascohen> |
Component: | Architecture, internals, and plumbing | Assignee: | Tomás Cohen Arazi <tomascohen> |
Status: | RESOLVED FIXED | QA Contact: | Testopia <testopia> |
Severity: | minor | ||
Priority: | P5 - low | CC: | jonathan.druart, lucas, martin.renvoize, nick, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
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
|
|
Bug Depends on: | 32529 | ||
Bug Blocks: | |||
Attachments: |
Bug 32528: Mkae safe_to_delete exit earlier on error conditions
Bug 32528: Mkae safe_to_delete exit earlier on error conditions Bug 32528: Change return logic Bug 32528: Change return logic Bug 32528: Make safe_to_delete exit earlier on error conditions |
Description
Tomás Cohen Arazi
2022-12-23 19:53:53 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. 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 |