[Bug 32528] New: Enhance Koha::Item->safe_to_delete
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Bug ID: 32528 Summary: Enhance Koha::Item->safe_to_delete Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |32529 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32529 [Bug 32529] Holds in processing should block item deletion -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com, | |tomascohen@gmail.com Summary|Enhance |Koha::Item->safe_to_delete |Koha::Item->safe_to_delete |should short-circuit | |earlier Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 144842 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144842&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144842|0 |1 is obsolete| | --- Comment #3 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 144847 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144847&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 144901 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144901&action=edit Bug 32528: Change return logic Back to 1 return statement Prevent the Koha::Result::Boolean instanciation duplication -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144901|0 |1 is obsolete| | --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 144925 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144925&action=edit Bug 32528: Change return logic Back to 1 return statement Prevent the Koha::Result::Boolean instanciation duplication -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 --- Comment #9 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #8)
Created attachment 144925 [details] [review] Bug 32528: Change return logic
Back to 1 return statement Prevent the Koha::Result::Boolean instanciation duplication
I agree with the follow-up. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144847|0 |1 is obsolete| | Attachment #144925|0 |1 is obsolete| | --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 144938 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144938&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.05. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Jacob O'Mara <jacob.omara@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.05.00 |23.05.00,22.11.02 released in| | Status|Pushed to master |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 --- Comment #12 from Jacob O'Mara <jacob.omara@ptfs-europe.com> --- Nice work, thanks everyone! Pushed to 22.11.x for the next release. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32528 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #13 from Lucas Gass <lucas@bywatersolutions.com> --- Missing dependencies for 22.05.x, no backport -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon@bugs.koha-community.org