Bug 25112 - Koha::CirculationRules->set_rules should handle scopes per rule
Summary: Koha::CirculationRules->set_rules should handle scopes per rule
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Lari Taskula
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-11 17:36 UTC by Lari Taskula
Modified: 2024-01-01 14:39 UTC (History)
3 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25112: Add a failing test for CirculationRules.t (3.47 KB, patch)
2020-04-11 18:00 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 25112: Make set_rules() handle rule scopes (1.97 KB, patch)
2020-04-11 18:00 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 25112: Add a failing test for CirculationRules.t (3.63 KB, patch)
2020-06-12 10:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25112: Make set_rules() handle rule scopes (2.12 KB, patch)
2020-06-12 10:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25112: Add a failing test for CirculationRules.t (3.46 KB, patch)
2020-07-31 15:44 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 25112: Make set_rules() handle rule scopes (1.97 KB, patch)
2020-07-31 15:44 UTC, Lari Taskula
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2020-04-11 17:36:17 UTC
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.
Comment 1 Lari Taskula 2020-04-11 18:00:27 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
Comment 2 Lari Taskula 2020-04-11 18:00:30 UTC
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
Comment 3 Lari Taskula 2020-04-11 18:11:00 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2020-06-12 09:58:07 UTC
I think that's expected.
That may make the caller codes confusing. Why do you need that?
Comment 5 Joonas Kylmälä 2020-06-12 10:11:06 UTC
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>
Comment 6 Joonas Kylmälä 2020-06-12 10:11:11 UTC
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>
Comment 7 Lari Taskula 2020-06-16 08:54:18 UTC
(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
  }
})
Comment 8 Jonathan Druart 2020-06-16 09:22:08 UTC
Yes it shorten the code, but it makes it confusing in my opinion.

Waiting for someone from QA to get their opinion.
Comment 9 Lari Taskula 2020-07-31 15:44:23 UTC
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
Comment 10 Lari Taskula 2020-07-31 15:44:27 UTC
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
Comment 11 Joonas Kylmälä 2020-08-24 08:49:40 UTC
Lari, can you clarify the bug status? It is now Signed Off but the patches were resubmitted without signed-off-by line.
Comment 12 Joonas Kylmälä 2020-08-24 10:03:40 UTC
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.
Comment 13 Lari Taskula 2020-08-24 17:39:12 UTC
(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.
Comment 14 Katrin Fischer 2024-01-01 14:39:43 UTC
Hos does this relate to bug 25554?