Bug 4530 - Circulation & fines rules aren't always applied from most specific to less specific
Summary: Circulation & fines rules aren't always applied from most specific to less sp...
Status: CLOSED WORKSFORME
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P2 major (vote)
Assignee: Srdjan Jankovic
QA Contact: Bugs List
URL:
Keywords:
: 6551 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-05-19 06:39 UTC by Frédéric Demians
Modified: 2020-01-06 20:17 UTC (History)
13 users (show)

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


Attachments
patch (24.10 KB, patch)
2011-09-07 02:26 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (23.39 KB, patch)
2011-12-08 03:53 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (22.98 KB, patch)
2011-12-22 00:39 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Circ/Fines Rules used for Main Library (64.78 KB, image/tiff)
2011-12-30 20:19 UTC, Melia Meggs
Details
Circ/Fines Rules used for South Branch (51.38 KB, image/tiff)
2011-12-30 20:19 UTC, Melia Meggs
Details
bug_4530: Rework issuing rules (23.04 KB, patch)
2012-02-07 16:02 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:28:15 UTC


---- Reported by frederic@tamil.fr 2010-05-19 06:39:48 ----

In some cases if specific rules aren't available, the most generic rule isn't applied.

For example, when fines in suspension days are defined only for default library, default item type and default patron category, and if a borrower returns a book several days after due date, he isn't suspended. It works only if suspension days is defined for a specific item type, and whatever what has been defined for the borrower specific category. If Koha is designed to work this way, 'suspension in days' must be removed from issuing rules matrix.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:28 UTC  ---

This bug was previously known as _bug_ 4530 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4530

Actual time not defined. Setting to 0.0
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.

Comment 1 Liz Rea 2011-01-21 16:03:26 UTC
I can confirm that this seems to be true: I had an itype that was only governed by the default rule, and the renewal rule wasn't applied (interface reported no renewals available, default rule stated that 2 renewals should be possible)
Comment 2 Srdjan Jankovic 2011-09-07 02:26:02 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2011-09-07 02:29:26 UTC
More comprehensive description of rule selection:
* Rules are chosen in order of preference borrower category, item type,
branch
* If value for certain rule is null, next rule in order is chosen
* 0 is treated as null
Comment 4 Brendan Gallagher 2011-11-04 11:13:57 UTC
Unable to apply to 3.6 - got patch failed.
Comment 5 Srdjan Jankovic 2011-11-07 05:47:03 UTC
I had no problems with 3.6.x. What should i do to replicate the problem please?
Comment 6 Nicole C. Engard 2011-11-11 14:59:04 UTC
This might help:



Applying: bug_4530: Rework issuing rules Use only one set of functions Database efficient rules fetch - only one query
/home/nengard/kohaclone/.git/rebase-apply/patch:150: trailing whitespace.
   
/home/nengard/kohaclone/.git/rebase-apply/patch:289: trailing whitespace.
                   FROM issues 
error: patch failed: C4/Circulation.pm:2306
error: C4/Circulation.pm: patch does not apply
Patch failed at 0001 bug_4530: Rework issuing rules Use only one set of functions Database efficient rules fetch - only one query
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/patch-rWv_7E.patch
nengard@debian:~/kohaclone$
Comment 7 Nicole C. Engard 2011-11-11 15:03:20 UTC
Tried again ... is this a pass or a fail?


Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: bug_4530: Rework issuing rules Use only one set of functions Database efficient rules fetch - only one query
Using index info to reconstruct a base tree...
<stdin>:150: trailing whitespace.
   
<stdin>:289: trailing whitespace.
                   FROM issues 
warning: 2 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging C4/Circulation.pm
Auto-merging C4/Members.pm
Auto-merging C4/Overdues.pm
Auto-merging C4/Reserves.pm
Auto-merging t/db_dependent/lib/KohaTest/Circulation.pm
nengard@debian:~/kohaclone$
Comment 8 Nicole C. Engard 2011-11-11 15:14:41 UTC
Okay it does apply.
Comment 9 Melia Meggs 2011-12-06 23:59:25 UTC
This patch doesn't apply for me.  This is what I got:

Applying: bug_4530: Rework issuing rules Use only one set of functions Database 
/home/melia/kohaclone/.git/rebase-apply/patch:150: trailing whitespace.
   
/home/melia/kohaclone/.git/rebase-apply/patch:289: trailing whitespace.
                   FROM issues 
error: patch failed: C4/Circulation.pm:74
error: C4/Circulation.pm: patch does not apply
error: patch failed: C4/Members.pm:660
error: C4/Members.pm: patch does not apply
Patch failed at 0001 bug_4530: Rework issuing rules Use only one set of function
e query
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/patch-_ZG0GE.patch
Comment 10 Chris Cormack 2011-12-07 00:10:04 UTC
CONFLICT (content): Merge conflict in C4/Members.pm

Looks to be the problem
Comment 11 Srdjan Jankovic 2011-12-08 03:53:19 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2011-12-21 20:25:49 UTC
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: bug_4530: Rework issuing rules
/home/katrin/kohaclone/.git/rebase-apply/patch:150: trailing whitespace.
   
/home/katrin/kohaclone/.git/rebase-apply/patch:314: trailing whitespace.
                   FROM issues 
warning: 2 lines add whitespace errors.
Using index info to reconstruct a base tree...
<stdin>:150: trailing whitespace.
   
<stdin>:314: trailing whitespace.
                   FROM issues 
warning: 2 lines applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging C4/Circulation.pm
CONFLICT (content): Merge conflict in C4/Circulation.pm
Auto-merging C4/Members.pm
Auto-merging C4/Overdues.pm
Auto-merging C4/Reserves.pm
Failed to merge in the changes.
Patch failed at 0001 bug_4530: Rework issuing rules
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Comment 13 Srdjan Jankovic 2011-12-22 00:39:51 UTC Comment hidden (obsolete)
Comment 14 Melia Meggs 2011-12-30 20:14:34 UTC
This patch applies, and I have attempted to test, but I need some help with testing this one because it's pretty complicated.

Here's what I did.  (Srdjan has pointed out that this test plan is too simple, but I thought it'd be at least a place to start).  I set up circ/fines rules to look like this:

For Main Branch
Patron Category | Item Type | Checkouts | Fine Amt | Suspension | Renewals | Holds
       Child                 All                 6             .50               3                  3             3 
       Child               Book               5              .50               2                  2             2 
       All                     All                3               .50               0                  0             0 

For South Branch
Patron Category | Item Type | Checkouts | Fine Amt | Suspension | Renewals | Holds
       Child                Book              4               .50              1                   1             1 


Then what I would expect to happen is: 
  -  If a child patron from the main branch tries to checkout a DVD, he can check out up to 6, with 3 days fines suspension, 3 renewals and 3 holds. 
  -  If a child patron from the main branch tries to checkout a book, he can check out up to 5, with 2 days fines suspension, 2 renewals and 2 holds. 
  -  If a child patron from the South branch tries to checkout a book, he can check out up to 4, with 1 day fines suspension, 1 renewal and 1 hold. 
  -  If a child patron from the East branch tries to checkout a compact disc, he can check out up to 3, with no fines suspension, no renewals and no holds. 

Here are my results
  -  If a child patron from the main branch tries to checkout a DVD, he can check out up to 6 - FAIL? (I checked out 7 with no problem.)
with 3 days fines suspension - DIDN'T TEST (didn't know how to test suspension)
3 renewals - PASS
and 3 holds - NOT SURE (for some reason I couldn't place on anything while testing, so I think something else was screwing that up... so I can't say one way or the other about holds)
  -  If a child patron from the main branch tries to checkout a book, he can check out up to 5 - PASS
with 2 days fines suspension - DIDN'T TEST
2 renewals - PASS
and 2 holds - NOT SURE
  -  If a child patron from the South branch tries to checkout a book, he can check out up to 4 - FAIL? (I checked out 5 with no problem.)
with 1 day fines suspension - DIDN'T TEST
1 renewal - NOT SURE
and 1 hold - NOT SURE
  -  If a child patron from the East branch tries to checkout a compact disc, he can check out up to 3 - FAIL? (I checked out 5 with no problem.)
with no fines suspension - DIDN'T TEST
no renewals - DIDN'T TEST
and no holds - NOT SURE

Sorry, those aren't very useful test results.  But if someone could help me fix the holes in that test plan, I'd be happy to do more testing.
Comment 15 Melia Meggs 2011-12-30 20:18:47 UTC
I am attaching screenshots of the circ/fines rules that I used since you can't read the previous note very easily.
Comment 16 Melia Meggs 2011-12-30 20:19:15 UTC
Created attachment 6985 [details]
Circ/Fines Rules used for Main Library
Comment 17 Melia Meggs 2011-12-30 20:19:36 UTC
Created attachment 6986 [details]
Circ/Fines Rules used for South Branch
Comment 18 Kyle M Hall 2012-02-07 16:00:38 UTC
I've done a lot of testing on this. It appears to be working correctly. I created a multitude of circ rules and used warns and Data::Dumper to see which rule would be picked each time. They rule chosen was correct in each instance.
Comment 19 Kyle M Hall 2012-02-07 16:02:34 UTC
Created attachment 7482 [details] [review]
bug_4530: Rework issuing rules

Use only one set of functions
Database efficient rules fetch - only one query

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Jared Camins-Esakov 2012-02-10 15:03:41 UTC
*** Bug 6551 has been marked as a duplicate of this bug. ***
Comment 21 Paul Poulain 2012-02-17 17:18:48 UTC
QA comments/questions

=== point 1 (and a question) ===
(In reply to comment #3)
> More comprehensive description of rule selection:
> * Rules are chosen in order of preference borrower category, item type,
> branch
> * If value for certain rule is null, next rule in order is chosen
> * 0 is treated as null

does it mean that, if I set the general rule to 7 documents for 21 days, and set a rule for 
CHILDREN / DVD to 0 it will be treated as NULL and the general rule will be applied ?
If yes, it's a big change in the behaviour, and, imo, a big mistake = libraries want to define such rules.
0 should mean 0. I agree we should handle "empty" as "rule unused at this level"

=== point 2 ===
If I understand well your code, the GetIssuingRules relies on
            ." order by branchcode desc, categorycode desc, itemtype desc";
and the rule returned is the result ordered 1st in this query, right.
Reading what's written on smart-rules.pl:
    same library, same patron type, same item type
    same library, same patron type, all item types
    same library, all patron types, same item type
    same library, all patron types, all item types
    all libraries, same patron type, same item type
    all libraries, same patron type, all item types
    all libraries, all patron types, same item type
    all libraries, all patron types, all item types

it's consistent with the order.

=== point 3 ===
In CanItemBeReserved, you removed the SQL that was
-                               categorycode DESC, 
-                               itemtype     DESC, 
-                               branchcode   DESC;"
in favor of GetIssuingRuleRecord, that is 
            ." order by branchcode desc, categorycode desc, itemtype desc";

It mean that the behaviour will change depending on how you have setup your rules, isn't it ?
I don't say I disagree, I say it must be clearly stated/advertised, and this patch should not be pushed in 3.6.
(however, I think it's more consistent, so it's good)

=== point 4 ===
small point : SQL must have reserved words in capital
so 
            ." ORDER BY branchcode DESC, categorycode DESC, itemtype DESC";
and not 
            ." order by branchcode desc, categorycode desc, itemtype desc";

=== point 5 ===
This patch also change the fines/hold/renewal behaviour, isn't it ? More tests must be done in this area

Conclusion:
* marking failed QA for point 4 and until point 1 is addressed. I'll also ask in // for one of BibLibre librarians to investigate this bug/patch
Comment 22 Srdjan Jankovic 2012-02-19 23:22:58 UTC
> 
> === point 1 (and a question) ===
> (In reply to comment #3)
> > More comprehensive description of rule selection:
> > * Rules are chosen in order of preference borrower category, item type,
> > branch
> > * If value for certain rule is null, next rule in order is chosen
> > * 0 is treated as null
> 
> does it mean that, if I set the general rule to 7 documents for 21 days, and
> set a rule for 
> CHILDREN / DVD to 0 it will be treated as NULL and the general rule will be
> applied ?
> If yes, it's a big change in the behaviour, and, imo, a big mistake = libraries
> want to define such rules.
> 0 should mean 0. I agree we should handle "empty" as "rule unused at this
> level"

I agree, it is just how it was before, so I did not want to change that. Please advise best course of action.


> 
> === point 4 ===
> small point : SQL must have reserved words in capital
> so 
>             ." ORDER BY branchcode DESC, categorycode DESC, itemtype DESC";
> and not 
>             ." order by branchcode desc, categorycode desc, itemtype desc";

Probably used some of the existing code, but will change that, it's not a big issue.
Comment 23 Gaetan Boisson 2012-03-02 11:47:44 UTC
I think we really need 0 and NULL to be treated differently. It is important for instance to define that a type of document cannot be issued to a certain category of patrons, without falling back on a more generic rule that would allow it.
Sometimes we also want to define that 0 books of that type can be issued to a patron category, with a loan period of X days : if we force the loan on an exceptional basis, the length of the loan will already be defined.

Here is my test plan so far: i have defined 8 rules, matching the 8 possibilities. For every rule i put the same value in each field:

8 for same library, same patron type, same item type
7 for same library, same patron type, all item types
6 for same library, all patron types, same item type
5 for same library, all patron types, all item types
4 for all libraries, same patron type, same item type
3 for all libraries, same patron type, all item types
2 for all libraries, all patron types, same item type
1 for all libraries, all patron types, all item types

the i tweak to put NULL (or 0 at the moment) at different places, loan some books and see what happens.

So far one thing that turned out not to work out was the number of renewals : it falls straight back to rule 3, and if i void rule 3 it falls back to rule 1, ignoring rule 2.

There are a lot of possibilities to test and i am not done yet...
Comment 24 Mathieu Saby 2013-06-18 07:53:10 UTC
Is there still a problem with the application of generic/specific circulation rules?

M. Saby
Comment 25 Marc Véron 2015-12-20 17:48:29 UTC
Still valid?
Comment 26 Liz Rea 2019-04-10 15:23:01 UTC
This seems to be working now.