[Bug 41268] New: Circulation rules script has many conditionals
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 Bug ID: 41268 Summary: Circulation rules script has many conditionals Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org Focusing on delete-branch-item-rule operation we have four code paths: 1 - if the branch is '*' and itemtype is '*' 2 - if branch is '*' and itemtype is specific 3 - If branch is specific and itemtype is '*' 4 - if branch is specific and itemtype is specified The only difference in these being that we pass 'undef' instead of '*' when star is set. I don't undertstand why we can't set a variable to 'undef' if necessary? -- 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=41268 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 --- Comment #1 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 189689 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189689&action=edit Bug 41268: use variables rather than conditionals -- 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=41268 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com, | |kyle@bywatersolutions.com, | |martin.renvoize@openfifth.c | |o.uk, tomascohen@gmail.com Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- 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=41268 Eric Phetteplace <ephetteplace@cca.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ephetteplace@cca.edu --- Comment #2 from Eric Phetteplace <ephetteplace@cca.edu> --- I'm not sure what the testing procedures are for refactors like this, but the tests pass and I tried creating then deleting some random policies and that worked, too. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 Eric Phetteplace <ephetteplace@cca.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189689|0 |1 is obsolete| | --- Comment #3 from Eric Phetteplace <ephetteplace@cca.edu> --- Created attachment 192116 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192116&action=edit Bug 41268: use variables rather than conditionals Testing plan: - Apply patch - Run tests - Go to admin > circ and fine rules - Create a few Holds and bookings policies - For All Libraries - For a specific library - Delete the policies - (optional) a simple `SELECT * FROM circulation_rules` can confirm policies were created/deleted correctly Signed-off-by: Eric Phetteplace <ephetteplace@cca.edu> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 Eric Phetteplace <ephetteplace@cca.edu> 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=41268 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192116|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 192343 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192343&action=edit Bug 41268: Use variables rather than conditionals Testing plan: - Apply patch - Run tests - Go to admin > circ and fine rules - Create a few Holds and bookings policies - For All Libraries - For a specific library - Delete the policies - (optional) a simple `SELECT * FROM circulation_rules` can confirm policies were created/deleted correctly Signed-off-by: Eric Phetteplace <ephetteplace@cca.edu> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 192344 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192344&action=edit Bug 41268: (QA follow-up) Apply same simplification to remaining operations This follow-up applies the same variable-based simplification pattern to the remaining operations in smart-rules.pl: - cud-set-branch-defaults: 2 code paths reduced to 1 - cud-add-branch-cat: 4 code paths reduced to 1 - cud-add-open-article-requests-limit: 4 code paths reduced to 1 - cud-del-open-article-requests-limit: 4 code paths reduced to 1 - cud-add-branch-item: 4 code paths reduced to 1 Note: cud-delete-branch-cat was NOT simplified because it contains additional business logic (setting itemtype rules only when deleting the default categorycode), which is not simply a matter of converting '*' to undef. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- I'm happy to PQA here, but my follow-up should probably have a pair of eyes on it for final go ahead. I'd be comfortable with Nick confirming -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 Nick Clemens (kidclamp) <nick@bywatersolutions.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=41268 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192343|0 |1 is obsolete| | Attachment #192344|0 |1 is obsolete| | --- Comment #7 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 192382 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192382&action=edit Bug 41268: Use variables rather than conditionals Testing plan: - Apply patch - Run tests - Go to admin > circ and fine rules - Create a few Holds and bookings policies - For All Libraries - For a specific library - Delete the policies - (optional) a simple `SELECT * FROM circulation_rules` can confirm policies were created/deleted correctly Signed-off-by: Eric Phetteplace <ephetteplace@cca.edu> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> 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=41268 --- Comment #8 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 192383 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192383&action=edit Bug 41268: (QA follow-up) Apply same simplification to remaining operations This follow-up applies the same variable-based simplification pattern to the remaining operations in smart-rules.pl: - cud-set-branch-defaults: 2 code paths reduced to 1 - cud-add-branch-cat: 4 code paths reduced to 1 - cud-add-open-article-requests-limit: 4 code paths reduced to 1 - cud-del-open-article-requests-limit: 4 code paths reduced to 1 - cud-add-branch-item: 4 code paths reduced to 1 Note: cud-delete-branch-cat was NOT simplified because it contains additional business logic (setting itemtype rules only when deleting the default categorycode), which is not simply a matter of converting '*' to undef. 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=41268 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.05.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 --- Comment #9 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 26.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 Jacob O'Mara <jacob.omara@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|26.05.00 |26.05.00,25.11.03 released in| | Status|Pushed to main |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=41268 --- Comment #10 from Jacob O'Mara <jacob.omara@openfifth.co.uk> --- Thanks all, pushed to 25.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41268 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED CC| |fridolin.somers@biblibre.co | |m Status|Pushed to stable |RESOLVED --- Comment #11 from Fridolin Somers <fridolin.somers@biblibre.com> --- No behavior change so lets not backport to 25.05.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org