Bug 15757 - Hard coded due loan/renewal period of 21 days if no circ rule found in C4::Circulation::GetLoanLength
Summary: Hard coded due loan/renewal period of 21 days if no circ rule found in C4::Ci...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12787
  Show dependency treegraph
 
Reported: 2016-02-08 14:14 UTC by Nick Clemens
Modified: 2019-06-27 09:24 UTC (History)
8 users (show)

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


Attachments
Bug 15757: Add one test for GetLoanLength (2.69 KB, patch)
2016-04-07 09:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15757: Make GetLoanLength defaults to 0 instead of 21 (5.87 KB, patch)
2016-04-07 09:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15757: Make issuelength default to null instead of 0 (1.26 KB, patch)
2016-04-07 09:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15757: Make GetLoanLength defaults to 0 instead of 21 (5.74 KB, patch)
2016-04-07 13:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
[ISGNED-OFF] Bug 15757: Add one test for GetLoanLength (2.76 KB, patch)
2016-04-11 19:25 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15757: Make issuelength default to null instead of 0 (1.36 KB, patch)
2016-04-11 19:26 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15757: Make GetLoanLength defaults to 0 instead of 21 (5.87 KB, patch)
2016-04-11 19:26 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15757: Add one test for GetLoanLength (2.81 KB, patch)
2016-04-21 14:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15757: Make issuelength default to null instead of 0 (1.40 KB, patch)
2016-04-21 14:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15757: Make GetLoanLength defaults to 0 instead of 21 (5.91 KB, patch)
2016-04-21 14:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15757 [QA Followup] - Having EDI configured will make unit tests fail (917 bytes, patch)
2016-04-21 14:21 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 Nick Clemens 2016-02-08 14:14:26 UTC
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.
Comment 1 Jonathan Druart 2016-02-15 17:52:41 UTC
Nick, there is no difference between the 2 snippets you paste :)
Comment 2 Jonathan Druart 2016-02-15 17:53:18 UTC
Ha ok, "this should fire a warning" :)
Comment 3 Jonathan Druart 2016-02-15 18:01:49 UTC
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.
Comment 4 Nick Clemens 2016-02-18 16:20:33 UTC
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.
Comment 5 Jonathan Druart 2016-02-19 15:45:06 UTC
I would consider it more an enhancement than as a blocker
Comment 6 Nick Clemens 2016-02-29 13:07:37 UTC
(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
Comment 7 Christopher Brannon 2016-04-05 19:16:36 UTC
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.
Comment 8 Christopher Brannon 2016-04-05 19:18:54 UTC
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.
Comment 9 Jonathan Druart 2016-04-06 10:58:18 UTC
Christopher, would you agree with the suggestion I made on comment 3?
Comment 10 Christopher Brannon 2016-04-06 14:11:18 UTC
(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.
Comment 11 Jonathan Druart 2016-04-06 14:22:16 UTC
(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).
Comment 12 Christopher Brannon 2016-04-06 14:47:33 UTC
(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.
Comment 13 Jonathan Druart 2016-04-06 15:09:35 UTC
(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.
Comment 14 Christopher Brannon 2016-04-06 15:10:48 UTC
(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.
Comment 15 Magnus Enger 2016-04-07 08:57:21 UTC
*** Bug 13576 has been marked as a duplicate of this bug. ***
Comment 16 Jonathan Druart 2016-04-07 09:42:31 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-04-07 09:42:34 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-04-07 09:42:38 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-04-07 09:43:57 UTC
Important: Please test this patch deeply!

@RMaint: Do NOT backport this patch set.
Comment 20 Mark Tompsett 2016-04-07 13:08:47 UTC
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 21 Mark Tompsett 2016-04-07 13:13:57 UTC
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 22 Mark Tompsett 2016-04-07 13:16:03 UTC
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!
Comment 23 Jonathan Druart 2016-04-07 13:37:00 UTC
(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.
Comment 24 Jonathan Druart 2016-04-07 13:40:00 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2016-04-07 13:41:20 UTC
Please provide follow-ups for what you think useful.
Comment 26 Bernardo Gonzalez Kriegel 2016-04-11 19:25:56 UTC Comment hidden (obsolete)
Comment 27 Bernardo Gonzalez Kriegel 2016-04-11 19:26:04 UTC Comment hidden (obsolete)
Comment 28 Bernardo Gonzalez Kriegel 2016-04-11 19:26:12 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2016-04-21 14:21:03 UTC
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>
Comment 30 Kyle M Hall 2016-04-21 14:21:13 UTC
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>
Comment 31 Kyle M Hall 2016-04-21 14:21:16 UTC
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>
Comment 32 Kyle M Hall 2016-04-21 14:21:19 UTC
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>
Comment 33 Brendan Gallagher 2016-04-22 00:25:30 UTC
Pushed to Master - Should be in the May 2016 release!  Thanks!
Comment 34 Julian Maurice 2016-04-22 11:43:04 UTC
Patches pushed to 3.22.x, will be in 3.22.6