Bug 25554 - Refactor rule kinds assignments in CirculationRules.pm
Summary: Refactor rule kinds assignments in CirculationRules.pm
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Peter Vashchuk
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-20 14:47 UTC by Peter Vashchuk
Modified: 2024-01-01 14:39 UTC (History)
5 users (show)

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


Attachments
Bug 25554: Refactor rule kinds assignments in CirculationRules.pm (7.52 KB, patch)
2020-05-20 15:02 UTC, Peter Vashchuk
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2020-05-20 14:47:56 UTC
Rule kinds in CirculationRules.pm have repeated code with the same values assigned many times. Refactored code is already in a patch that will be pushed promptly to this ticket.
Comment 1 Peter Vashchuk 2020-05-20 15:02:53 UTC
Created attachment 105154 [details] [review]
Bug 25554: Refactor rule kinds assignments in CirculationRules.pm

CirculationRules.pm have repeated code with the same values assigned many times to rule kinds and also same hardcoded values appear in foreach loop.
Refactored by creating arrays once and then assigning and looping their values.

Mentored-by: Andrew Nugged <nugged@gmail.com>
Comment 2 Jonathan Druart 2020-06-03 10:56:59 UTC
Hum, I am going to ask myself "what it scope2 again?" for years :)

I agree we could have that in a variable, but be more explicit:
 b_c, b_i, b_c_i
or, even better IMO:
 branch_category, branch_itemtype, branch_category_itemtype
Comment 3 Peter Vashchuk 2020-06-05 14:50:58 UTC
Yeah I thought about naming them similarly to what you proposed but it would undermine some decisions that were taken into consideration during refactor.

The thing is, we need to add another item to "scope3" for Koha Finland, if we name those arrays something like I already did (scope1, scope2, scope3) all it requires is to add another item to array and we’re done, but if we rename those arrays like you suggested (branch_category, branch_itemtype, branch_category_itemtype) it will complicate things if we ever need to add any other element to any of those arrays.
Comment 4 Katrin Fischer 2020-06-07 11:51:13 UTC
Hi Peter, can you explain about your future needs?

At the moment I am with Jonathan, I think readability is to be preferred, even if resulting in more code.
Comment 5 Andrii Nugged 2020-06-07 12:42:06 UTC
Here we need to have more filtering criteria for smart-rules. For Finnish libraries, we added "ccode" and "sleving_location", two extra smart rule filters
(but by the way also later we propose this for the community version)

so we had before:

    my $scope3 =
        [ 'branchcode', 'categorycode', 'itemtype' ];

but now we have in our case:

    my $scope3 =
        [ 'branchcode', 'categorycode', 'itemtype', 'ccode', 'shelving_location' ];

and because of having only a single variable, we don't need to change any variable names down in the file, nor copy-paste dozen times like it is now the same array copypasted everywhere.




But now proposal from Jonathan to name it something like "$branch_category_itemtype" will make this:

    my $branch_category_itemtype =
        [ 'branchcode', 'categorycode', 'itemtype' ];

becoming this (???):
 
    my $branch_category_itemtype_ccode_shelving_location =
        [ 'branchcode', 'categorycode', 'itemtype', 'ccode', 'shelving_location' ]; 

so when we add two extra items, we should again change all the code down below to the new variable name $branch_category_itemtype_ccode_shelving_location? Yikes!


That's why we voting for "$scope3",
because we have three scope sections, even logically separated: small one, middle, and full.

... Oh, or let's name it "$scope_full" if you ok?
Comment 6 Jonathan Druart 2020-06-08 08:37:46 UTC
What about the following?

$scopes => [
  {
    scope => [ qw( branchcode ) ],
    rules => [ qw( refund ) ],
  },
  {
    scope => [ qw( branchcode categorycode ) ],
    rules => [ qw( patron_maxissueqty patron_maxonsiteissueqty max_holds ... ) ],
  },
...
  {
    scope => [ qw( branchcode categorycode itemtype ) ],
    rules => [ qw( article_requests auto_renew cap_fine_to_replacement_price ...) ],
  },
];

ie you define using a different structure, then you "revert" it to generate the existing structure.

Don't implement it yet, I am not sure I agree with myself :D
Comment 7 Andrii Nugged 2020-06-08 18:58:54 UTC
But that's not only about the assignment of $RULE_KINDS on the beginning, $scope3 also used in the code further, in two places:

>+my $scope3 = [ 'branchcode', 'categorycode', 'itemtype' ];
...

>     my $order_by = $params->{order_by}
>-      // { -desc => [ 'branchcode', 'categorycode', 'itemtype' ] };
>+      // { -desc => $scope3 };
...

>     # Enforce scope; a rule should be set for its defined scope, no more, no less.
>-    foreach my $scope_level ( qw( branchcode categorycode itemtype ) ) {
>+    foreach my $scope_level ( @$scope3 ) {
>         if ( grep /$scope_level/, @{ $kind_info->{scope} } ) {

so just making pairs rules/scopes won't solve this, more advanced refactoring then needed. Let's think about this, yes (just thinking aloud here too :) ).