Bug 24026

Summary: Wrong parameters in Koha/Templates/Plugin/CirculationRules.pm and smart-rules.tt
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: System AdministrationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, kelly, kyle, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.05
Circulation function:
Bug Depends on: 22847    
Bug Blocks:    
Attachments: Bug 24026: (bug 22847 follow-up) Remove c/p issues
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones)
Bug 24026: (bug 22847 follow-up) Remove c/p issues
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones)
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones)
Bug 24026: (bug 22847 follow-up) Remove c/p issues
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones)

Description Nick Clemens (kidclamp) 2019-11-12 18:08:07 UTC
The searches in the module use $branchcode for all three parameters, looks like a copy/paste issue

  29     $branchcode   = undef if $branchcode eq q{}   or $branchcode eq q{*};
  30     $categorycode = undef if $categorycode eq q{} or $branchcode eq q{*};
  31     $itemtype     = undef if $itemtype eq q{}     or $branchcode eq q{*};

The admin page uses 'Get' instead of 'Search' for maxisseqty/maxonsiteissueqty - this returns the 'effective' rule and so may return the wrong rule if things are not set

 139                             <td>
 140                                 [% SET rule_value = CirculationRules.Get( rule.branchcode, rule.categorycode, rule.itemtype, 'maxissueqty' ) %]
 141                                 [% IF rule_value || rule_value == "0"  %]
 142                                     [% rule_value | html %]
 143                                 [% ELSE %]
 144                                     <span>Unlimited</span>
 145                                 [% END %]
 146                             </td>

We discovered this from upgrade issues resulting from bug 23813
Comment 1 Jonathan Druart 2019-11-13 11:28:42 UTC
ARG! Impossible, I never copy paste, never!
Comment 2 Jonathan Druart 2019-11-13 11:38:28 UTC
Created attachment 95365 [details] [review]
Bug 24026: (bug 22847 follow-up) Remove c/p issues
Comment 3 Jonathan Druart 2019-11-13 11:38:32 UTC
Created attachment 95366 [details] [review]
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones)

See bug 22847. We should never call ->Search in this template, we want
to display the values in the DB, not the calculated ones
Comment 4 Jonathan Druart 2019-11-13 11:39:38 UTC
Upping severity.
Comment 5 Nick Clemens (kidclamp) 2019-11-13 11:52:48 UTC
Created attachment 95369 [details] [review]
Bug 24026: (bug 22847 follow-up) Remove c/p issues

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens (kidclamp) 2019-11-13 11:52:51 UTC
Created attachment 95370 [details] [review]
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones)

See bug 22847. We should never call ->Search in this template, we want
to display the values in the DB, not the calculated ones

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens (kidclamp) 2019-11-13 11:53:54 UTC
(In reply to Jonathan Druart from comment #3)
> See bug 22847. We should never call ->Search in this template, we want
> to display the values in the DB, not the calculated ones

We should never call 'Get', we should always use 'Search' - I think just the comment was wrong
Comment 8 Nick Clemens (kidclamp) 2019-11-13 11:59:53 UTC
To test:
Add a standard rule for all libraries with maxisseqty and maxonsiteissueqty set to some value, say 5

Add several itemtype or categorycode specific rules, leaving max checkouts/onsite as blank to get unlimited

Set an itemtype or cateogory specific rule for a branch as well.

Remove the specific rules from the DB:
delete from circulation_rules WHERE itemtype IS NOT NULL OR categorycode IS NOT NULL;

Note that all rules for all libraries use the 'All/All' checkout limit
Note that the specific library does as well

Apply patch

unset rules appear  as unlimited now

Note:thigns still won't work in this state, as we are introducing bad data, but it helps highlight the issue
Comment 9 Jonathan Druart 2019-11-13 12:19:46 UTC
Created attachment 95373 [details] [review]
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones)

See bug 22847. We should never call ->Get in this template, we want
to display the values in the DB, not the calculated ones

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Jonathan Druart 2019-11-13 12:20:07 UTC
(In reply to Nick Clemens from comment #7)
> (In reply to Jonathan Druart from comment #3)
> > See bug 22847. We should never call ->Search in this template, we want
> > to display the values in the DB, not the calculated ones
> 
> We should never call 'Get', we should always use 'Search' - I think just the
> comment was wrong

Move confusion, more fun!
Comment 11 Martin Renvoize (ashimema) 2019-11-13 13:50:20 UTC
Created attachment 95386 [details] [review]
Bug 24026: (bug 22847 follow-up) Remove c/p issues

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize (ashimema) 2019-11-13 13:50:24 UTC
Created attachment 95387 [details] [review]
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones)

See bug 22847. We should never call ->Get in this template, we want
to display the values in the DB, not the calculated ones

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize (ashimema) 2019-11-13 14:04:10 UTC
Nice work!

Pushed to master for 19.11.00
Comment 14 Fridolin Somers 2019-11-15 08:53:34 UTC
Pushed to 19.05.x for 19.05.05