Bug 24026 - Wrong parameters in Koha/Templates/Plugin/CirculationRules.pm and smart-rules.tt
Summary: Wrong parameters in Koha/Templates/Plugin/CirculationRules.pm and smart-rules.tt
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 22847
Blocks:
  Show dependency treegraph
 
Reported: 2019-11-12 18:08 UTC by Nick Clemens
Modified: 2019-11-15 08:53 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.05


Attachments
Bug 24026: (bug 22847 follow-up) Remove c/p issues (1.65 KB, patch)
2019-11-13 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones) (2.10 KB, patch)
2019-11-13 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24026: (bug 22847 follow-up) Remove c/p issues (1.70 KB, patch)
2019-11-13 11:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones) (2.16 KB, patch)
2019-11-13 11:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones) (2.15 KB, patch)
2019-11-13 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24026: (bug 22847 follow-up) Remove c/p issues (1.76 KB, patch)
2019-11-13 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24026: (bug 22847 follow-up) Display correctly max[onsite]issueqty (not the propagated ones) (2.22 KB, patch)
2019-11-13 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 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 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 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 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 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 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 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 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