---- 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.
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)
Created attachment 5329 [details] [review] patch I've made a single-entry point for issuing rules. That is good. The bad ting is that the current implementation does not store nulls for numeric values, but 0. In those cases we have no way of knowing if 0 *is* the value, or we should fall back to the next more generic rule. What I have done is: * 0 is treated as null, ie no rule * when selecting rules order of preference is: borrower category, item type, branch All relevant parts of the system need to be tested. I tried Holds, Check Out and Renewals myself. If that is of any help, affected functions are: CanItemBeReserved CanBookBeIssued GetIssuingCharges AddIssue GetRenewCount CanBookBeRenewed AddRenewal CalcFine
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
Created attachment 6656 [details] [review] patch
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".
Created attachment 6915 [details] [review] patch
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.