Bug 41268 - Circulation rules script has many conditionals
Summary: Circulation rules script has many conditionals
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-11-18 21:54 UTC by Nick Clemens (kidclamp)
Modified: 2026-02-06 22:20 UTC (History)
5 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
26.05.00
Circulation function:


Attachments
Bug 41268: use variables rather than conditionals (3.53 KB, patch)
2025-11-18 21:55 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 41268: use variables rather than conditionals (3.88 KB, patch)
2026-01-28 04:22 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 41268: Use variables rather than conditionals (3.94 KB, patch)
2026-02-02 21:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41268: (QA follow-up) Apply same simplification to remaining operations (14.06 KB, patch)
2026-02-02 21:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41268: Use variables rather than conditionals (4.00 KB, patch)
2026-02-03 13:05 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 41268: (QA follow-up) Apply same simplification to remaining operations (14.12 KB, patch)
2026-02-03 13:05 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2025-11-18 21:54:14 UTC
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?
Comment 1 Nick Clemens (kidclamp) 2025-11-18 21:55:10 UTC
Created attachment 189689 [details] [review]
Bug 41268: use variables rather than conditionals
Comment 2 Eric Phetteplace 2026-01-28 02:44:33 UTC
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.
Comment 3 Eric Phetteplace 2026-01-28 04:22:32 UTC
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>
Comment 4 Martin Renvoize (ashimema) 2026-02-02 21:49:49 UTC
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>
Comment 5 Martin Renvoize (ashimema) 2026-02-02 21:49:50 UTC
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.
Comment 6 Martin Renvoize (ashimema) 2026-02-03 09:31:05 UTC
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
Comment 7 Nick Clemens (kidclamp) 2026-02-03 13:05:36 UTC
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>
Comment 8 Nick Clemens (kidclamp) 2026-02-03 13:05:38 UTC
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>
Comment 9 Lucas Gass (lukeg) 2026-02-06 22:20:59 UTC
Nice work everyone!

Pushed to main for 26.05