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?
Created attachment 189689 [details] [review] Bug 41268: use variables rather than conditionals
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.
Created attachment 192116 [details] [review] 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>
Created attachment 192343 [details] [review] 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>
Created attachment 192344 [details] [review] 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.
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
Created attachment 192382 [details] [review] 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>
Created attachment 192383 [details] [review] 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>
Nice work everyone! Pushed to main for 26.05