https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25112 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25112 --- Comment #2 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 102764 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102764&action=edit Bug 25112: Make set_rules() handle rule scopes Koha::CirculationRules->set_rules is currently too complicated to use. Right now, in order to specify multiple rules at once, they must all be rules that accept the same set of scopes. Otherwise we can get this type of errors: 1/9 set_rule cannot set 'holds_per_record' for a 'checkout_type'! at t/db_dependent/Circulation/GetHardDueDate.t line 215. Validating scopes at set_rule() is good, but set_rules() should examine each rule and pass the correct scope to set_rule() instead of passing all scopes to every rule like it is doing now. To test: 1. prove t/db_dependent/Koha/CirculationRules.t 2. Observe success Sponsored-by: The National Library of Finland -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25112 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lari.taskula@hypernova.fi -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25112 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I think that's expected. That may make the caller codes confusing. Why do you need that? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25112 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102763|0 |1 is obsolete| | Attachment #102764|0 |1 is obsolete| | --- Comment #5 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 105787 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=105787&action=edit Bug 25112: Add a failing test for CirculationRules.t To reproduce the problem, do: 1. prove t/db_dependent/Koha/CirculationRules.t 2. Observe a failing test set_rule cannot set 'holdallowed' for a 'categorycode'! at t/db_dependent/Koha/CirculationRules.t line 304. Sponsored-by: The National Library of Finland Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25112 --- Comment #7 from Lari Taskula <lari.taskula@hypernova.fi> --- (In reply to Jonathan Druart from comment #4)

I think that's expected. That may make the caller codes confusing. Why do you need that?

This may be a matter of preference but I would expect "set_rules" method to take all rules at once regardless of the developer having to study which set of rules use the same set of scopes and then determine how many calls are necessary. It should simply process all given rules with the scopes that they are able to utilize. It reduces code repetition and makes adding new/modifying scopes of existing rules easier because you don't have to split any of the set_rules methods in parts. A good example is in smart-rules.pl where we are repeating the calls multiple times due to current restriction of set_rules. It looks bad and is quite confusing to make changes to, especially when adding a new scope. I know this page will eventually be removed but for the sake of future usability I propose this change. For visualization, here is the worst case with the current scopes and the result after my proposed change: Koha::CirculationRules->set_rules({ branchcode => 'CPL', rules => { refund => 1 } }) Koha::CirculationRules->set_rules({ branchcode => 'CPL', categorycode => 'PT', rules => { patron_maxissueqty => 1 } }) Koha::CirculationRules->set_rules({ branchcode => 'CPL', itemtype => 'BK', rules => { holdallowed => 1 } }) Koha::CirculationRules->set_rules({ branchcode => 'CPL', categorycode => 'PT' itemtype => 'BK', rules => { article_requests => 1 } }) vs proposed change: Koha::CirculationRules->set_rules({ branchcode => 'CPL', categorycode => 'PT' itemtype => 'BK', rules => { refund => 1 patron_maxissueqty => 1 holdallowed => 1, article_requests => 1 } }) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25112 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #105787|0 |1 is obsolete| | --- Comment #9 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 107650 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107650&action=edit Bug 25112: Add a failing test for CirculationRules.t To reproduce the problem, do: 1. prove t/db_dependent/Koha/CirculationRules.t 2. Observe a failing test set_rule cannot set 'holdallowed' for a 'categorycode'! at t/db_dependent/Koha/CirculationRules.t line 304. Sponsored-by: The National Library of Finland -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25112 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi --- Comment #11 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Lari, can you clarify the bug status? It is now Signed Off but the patches were resubmitted without signed-off-by line. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25112 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|25089 | --- Comment #13 from Lari Taskula <lari.taskula@hypernova.fi> --- (In reply to Joonas Kylmälä from comment #12)

I agree with Jonathan that this change would make the code confusing and also I think it would make it error prone because the coder could think the scope is more restricted than what it in reality will be. I wonder if there would be any other way to make the rule setting more intuitive. Thanks for the feedback Jonathan and Joonas. It seems we need more opinions and some support before this can move forward. I'll leave this to "In Discussion" status.

Since this is not a critical change, the dependency chain that it was built for, and is currently part of, can use the original set_rules() behavior. So I'm removing Bug 25089 from the blocker list. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25089 [Bug 25089] Add checkout_type to circulation rules -- You are receiving this mail because: You are watching all bug changes.