Bug 4530

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)

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

Unable to apply to 3.6 - got patch failed.

I had no problems with 3.6.x. What should i do to replicate the problem please?

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$

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$

Okay it does apply.

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

CONFLICT (content): Merge conflict in C4/Members.pm

Looks to be the problem

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".

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.

I am attaching screenshots of the circ/fines rules that I used since you can't read the previous note very easily.

Created attachment 6985 [details]
Circ/Fines Rules used for Main Library

Created attachment 6986 [details]
Circ/Fines Rules used for South Branch

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.

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>

*** Bug 6551 has been marked as a duplicate of this bug. ***

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

> 
> === 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.

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...

Is there still a problem with the application of generic/specific circulation rules?

M. Saby

Still valid?

This seems to be working now.