Summary: | Koha::CirculationRules->set_rules should handle scopes per rule | ||
---|---|---|---|
Product: | Koha | Reporter: | Lari Taskula <lari.taskula> |
Component: | Architecture, internals, and plumbing | Assignee: | Lari Taskula <lari.taskula> |
Status: | In Discussion --- | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | jonathan.druart, joonas.kylmala, lari.taskula |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25554 | ||
Change sponsored?: | Sponsored | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Bug 25112: Add a failing test for CirculationRules.t
Bug 25112: Make set_rules() handle rule scopes Bug 25112: Add a failing test for CirculationRules.t Bug 25112: Make set_rules() handle rule scopes Bug 25112: Add a failing test for CirculationRules.t Bug 25112: Make set_rules() handle rule scopes |
Description
Lari Taskula
2020-04-11 17:36:17 UTC
Created attachment 102763 [details] [review] 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 Created attachment 102764 [details] [review] 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 (In reply to Lari Taskula from comment #1) > Created attachment 102763 [details] [review] [review] > 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 I think that's expected. That may make the caller codes confusing. Why do you need that? Created attachment 105787 [details] [review] 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> Created attachment 105788 [details] [review] 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 Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.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 } }) Yes it shorten the code, but it makes it confusing in my opinion. Waiting for someone from QA to get their opinion. Created attachment 107650 [details] [review] 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 Created attachment 107651 [details] [review] 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 Lari, can you clarify the bug status? It is now Signed Off but the patches were resubmitted without signed-off-by line. Hi, I understood now that it was in Signed off status because it was waiting QA comment. 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. (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. Hos does this relate to bug 25554? |