Nick, there is no difference between the 2 snippets you paste :) Ha ok, "this should fire a warning" :) We could change the different occurrences of
if defined($loanlength) && $loanlength->{issuelength};
with
if defined($loanlength) && defined $loanlength->{issuelength};
To take into account the "0".
And do not default to 0 when defining the circ rules but NULL instead.
The only problem is that these behavior changes could be considered as a bug for existing installations.
I suppose a syspref then? CircRuleUndefinedCheckout: Yes/No/Warn CircRuleUndefinedCheckoutPeriod: default(21) So default is 'Yes/Allow' for 21 days? (In reply to Jonathan Druart from comment #3) > We could change the different occurrences of > if defined($loanlength) && $loanlength->{issuelength}; > with > if defined($loanlength) && defined $loanlength->{issuelength}; > To take into account the "0". > And do not default to 0 when defining the circ rules but NULL instead. > > The only problem is that these behavior changes could be considered as a bug > for existing installations. I would consider it more an enhancement than as a blocker (In reply to Jonathan Druart from comment #5) > I would consider it more an enhancement than as a blocker I could go for a lesser bug status, but I definitely think the expectation of a library setting the checkout period to zero is not a fall back to default rules or 21 days, I think it is expected that the item will not be checked out I would definitely call this a bug. We were scratching our heads trying to figure out where the 21 days came from. What would be preferable is when something like this is overridden and does not have a defined loan period it should prompt you. Something like: This item does not have a set loan period. When would you like it due? If a due date is specified already by staff, it should show that date as a suggestion, otherwise show today's date. Staff could alter the date on that prompt or just use the date shown. Better yet, when asked if you want to override, it could say: Do you want to override with a due date of xx/xx/xxxx? YES / NO Again, defaulting to today, or the specified due date if set. Christopher, would you agree with the suggestion I made on comment 3? (In reply to Jonathan Druart from comment #9) > Christopher, would you agree with the suggestion I made on comment 3? I can't. The code is beyond my understanding. I don't know any of Koha's internal plumbing. (In reply to Christopher Brannon from comment #10) > (In reply to Jonathan Druart from comment #9) > > Christopher, would you agree with the suggestion I made on comment 3? > > I can't. The code is beyond my understanding. I don't know any of Koha's > internal plumbing. Basically it means that - 0 will be taken into account for 'Loan period' (issuelength) - and if not filled in the circ rules NULL will be inserted in the DB (instead of 0, which is the current behavior). (In reply to Jonathan Druart from comment #11) > Basically it means that > - 0 will be taken into account for 'Loan period' (issuelength) > - and if not filled in the circ rules NULL will be inserted in the DB > (instead of 0, which is the current behavior). And what is the behavior of NULL? If 0 is being taken into account, then that is great. But I think we need to make clear what NULL will do. (In reply to Christopher Brannon from comment #12) > (In reply to Jonathan Druart from comment #11) > > > Basically it means that > > - 0 will be taken into account for 'Loan period' (issuelength) > > - and if not filled in the circ rules NULL will be inserted in the DB > > (instead of 0, which is the current behavior). > > And what is the behavior of NULL? > > If 0 is being taken into account, then that is great. But I think we need > to make clear what NULL will do. Nothing, another rule will be checked. And 0 will be returned (instead of 21) if no default rule is defined. That's why I have linked bug 13576 and bug 8361, a default rule has to be defined. I'd consider that outside the scope of this bug. (In reply to Jonathan Druart from comment #13) > Nothing, another rule will be checked. > And 0 will be returned (instead of 21) if no default rule is defined. > That's why I have linked bug 13576 and bug 8361, a default rule has to be > defined. I'd consider that outside the scope of this bug. Then I am in agreement with the change in comment 3. :) Thanks for walking me through it. *** Bug 13576 has been marked as a duplicate of this bug. *** Created attachment 50013 [details] [review] Bug 15757: Add one test for GetLoanLength The usual call (3 params) of this subroutine was not tested. Created attachment 50014 [details] [review] Bug 15757: Make GetLoanLength defaults to 0 instead of 21 GetLoanLength arbitrary defaulted to 21. The expected behavior seems to be to default on 0 (loan will be dued today). IMPORTANT NOTE: This patch will introduce a change in the behaviors for configuration with a 0 in issuelength. Before this patch, the rule with a issuelength==0 was skipped, now it's used! Test plan: 1/ Do not define any rule: the due date will be today (before this patch was +21 days) 2/ Define some rules which does not match the patron category, itemtype or branchcode: the due date will be today (before this patch was +21 days). 3/ Modify a rule to match the checkout and set issuelength=0: the due date will be today (before this patch, the rule was skipped) 4/ Modify this rule and set the issuelength to something > 0: the due date will be adjusted (same behavior as before this patch) Created attachment 50015 [details] [review] Bug 15757: Make issuelength default to null instead of 0 When editing circ rules, if the Loan period "issuelength" is not defined (empty string), the default value was 0, not it's inserted in the DB as NULL. Test plan: 1/ Create or edit a circ rule 2/ Do not fill the Loan period column 3/ Save => Without this patch, the value was 0 => With this patch it's now an empty string (in the DB it's set to NULL) Important: Please test this patch deeply! @RMaint: Do NOT backport this patch set. Comment on attachment 50015 [details] [review] Bug 15757: Make issuelength default to null instead of 0 Review of attachment 50015 [details] [review]: ----------------------------------------------------------------- ::: admin/smart-rules.pl @@ +140,4 @@ > $maxonsiteissueqty =~ s/\s//g; > $maxonsiteissueqty = undef if $maxonsiteissueqty !~ /^\d+/; > my $issuelength = $input->param('issuelength'); > + $issuelength = $issuelength eq q{} ? undef : $issuelength; Change 142 to: my $issuelength = $input->param('issuelength') // q{}; -- to avoid noise triggered when parameter is undef. Comment on attachment 50013 [details] [review] Bug 15757: Add one test for GetLoanLength Review of attachment 50013 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Circulation_Issuingrule.t @@ +113,4 @@ > maxissueqty => 5, > maxonsiteissueqty => 4, > finedays => 0, > + lengthunit => 'days', Why change to days? @@ -113,4 @@ > maxissueqty => 5, > maxonsiteissueqty => 4, > finedays => 0, > - lengthunit => 'Null', I think the point was undef, but Data::Dumper puts out 'Null' Comment on attachment 50014 [details] [review] Bug 15757: Make GetLoanLength defaults to 0 instead of 21 Review of attachment 50014 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Circulation_Issuingrule.t @@ +113,5 @@ > +is_deeply( > + C4::Circulation::GetLoanLength(), > + $default, > + "Without parameters, GetLoanLength returns hardcoded values" > +); This is an added test. If the number of tests to run is set, then you forgot to update it. @@ +360,3 @@ > is_deeply( > C4::Circulation::GetLoanLength(), > + $default, Nice clean up! (In reply to M. Tompsett from comment #22) > Comment on attachment 50014 [details] [review] [review] > Bug 15757: Make GetLoanLength defaults to 0 instead of 21 > > Review of attachment 50014 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: t/db_dependent/Circulation_Issuingrule.t > @@ +113,5 @@ > > +is_deeply( > > + C4::Circulation::GetLoanLength(), > > + $default, > > + "Without parameters, GetLoanLength returns hardcoded values" > > +); > > This is an added test. If the number of tests to run is set, then you forgot > to update it. Don't know where does it come from, it's certainly a c/p issue. The number of expected tests is 10 and 11 are run. Created attachment 50029 [details] [review] Bug 15757: Make GetLoanLength defaults to 0 instead of 21 GetLoanLength arbitrary defaulted to 21. The expected behavior seems to be to default on 0 (loan will be dued today). IMPORTANT NOTE: This patch will introduce a change in the behaviors for configuration with a 0 in issuelength. Before this patch, the rule with a issuelength==0 was skipped, now it's used! Test plan: 1/ Do not define any rule: the due date will be today (before this patch was +21 days) 2/ Define some rules which does not match the patron category, itemtype or branchcode: the due date will be today (before this patch was +21 days). 3/ Modify a rule to match the checkout and set issuelength=0: the due date will be today (before this patch, the rule was skipped) 4/ Modify this rule and set the issuelength to something > 0: the due date will be adjusted (same behavior as before this patch) Please provide follow-ups for what you think useful. Created attachment 50131 [details] [review] [ISGNED-OFF] Bug 15757: Add one test for GetLoanLength The usual call (3 params) of this subroutine was not tested. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 50132 [details] [review] [SIGNED-OFF] Bug 15757: Make issuelength default to null instead of 0 When editing circ rules, if the Loan period "issuelength" is not defined (empty string), the default value was 0, not it's inserted in the DB as NULL. Test plan: 1/ Create or edit a circ rule 2/ Do not fill the Loan period column 3/ Save => Without this patch, the value was 0 => With this patch it's now an empty string (in the DB it's set to NULL) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works as described. No errors Created attachment 50133 [details] [review] [SIGNED-OFF] Bug 15757: Make GetLoanLength defaults to 0 instead of 21 GetLoanLength arbitrary defaulted to 21. The expected behavior seems to be to default on 0 (loan will be dued today). IMPORTANT NOTE: This patch will introduce a change in the behaviors for configuration with a 0 in issuelength. Before this patch, the rule with a issuelength==0 was skipped, now it's used! Test plan: 1/ Do not define any rule: the due date will be today (before this patch was +21 days) 2/ Define some rules which does not match the patron category, itemtype or branchcode: the due date will be today (before this patch was +21 days). 3/ Modify a rule to match the checkout and set issuelength=0: the due date will be today (before this patch, the rule was skipped) 4/ Modify this rule and set the issuelength to something > 0: the due date will be adjusted (same behavior as before this patch) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works ok, checked 1-4 All test pass No koha-qa errors Created attachment 50513 [details] [review] Bug 15757: Add one test for GetLoanLength The usual call (3 params) of this subroutine was not tested. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 50514 [details] [review] Bug 15757: Make issuelength default to null instead of 0 When editing circ rules, if the Loan period "issuelength" is not defined (empty string), the default value was 0, not it's inserted in the DB as NULL. Test plan: 1/ Create or edit a circ rule 2/ Do not fill the Loan period column 3/ Save => Without this patch, the value was 0 => With this patch it's now an empty string (in the DB it's set to NULL) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works as described. No errors Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 50515 [details] [review] Bug 15757: Make GetLoanLength defaults to 0 instead of 21 GetLoanLength arbitrary defaulted to 21. The expected behavior seems to be to default on 0 (loan will be dued today). IMPORTANT NOTE: This patch will introduce a change in the behaviors for configuration with a 0 in issuelength. Before this patch, the rule with a issuelength==0 was skipped, now it's used! Test plan: 1/ Do not define any rule: the due date will be today (before this patch was +21 days) 2/ Define some rules which does not match the patron category, itemtype or branchcode: the due date will be today (before this patch was +21 days). 3/ Modify a rule to match the checkout and set issuelength=0: the due date will be today (before this patch, the rule was skipped) 4/ Modify this rule and set the issuelength to something > 0: the due date will be adjusted (same behavior as before this patch) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works ok, checked 1-4 All test pass No koha-qa errors Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 50516 [details] [review] Bug 15757 [QA Followup] - Having EDI configured will make unit tests fail Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Pushed to Master - Should be in the May 2016 release! Thanks! Patches pushed to 3.22.x, will be in 3.22.6 |
A library reported recently that an item with circulation period set to 0 for all patron categories with a few exceptions set was checked out and was given a checkout period of 21 days. This comes from the last lines in the GetLoanLength subroutine: 1475 # if no rule is set => 21 days (hardcoded) 1476 return { 1477 issuelength => 21, 1478 renewalperiod => 21, 1479 lengthunit => 'days', 1480 }; 1481 This should fire a1475 # if no rule is set => 21 days (hardcoded) 1476 return { 1477 issuelength => 21, 1478 renewalperiod => 21, 1479 lengthunit => 'days', 1480 }; 1481 warning and/or stop the checkout rather than defaulting.