Bug 14146 - Add option to add restriction period when checking-in several overdues for same patron
Summary: Add option to add restriction period when checking-in several overdues for sa...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-05 09:04 UTC by Carolina Andreu
Modified: 2017-04-02 16:18 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14146: Add the new pref CumulativeRestrictionPeriods (2.10 KB, patch)
2016-12-12 15:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14146: Add tests for AddReturn + CumulativeRestrictionPeriods (6.03 KB, patch)
2016-12-12 15:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14146: Add the ability to cumulate restriction periods (2.10 KB, patch)
2016-12-12 15:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14146: Clean a bit and make the code understandable (3.29 KB, patch)
2016-12-12 15:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14146: Add the new pref CumulativeRestrictionPeriods (3.22 KB, patch)
2016-12-13 10:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14146: Add tests for AddReturn + CumulativeRestrictionPeriods (6.08 KB, patch)
2017-02-17 11:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14146: Add the ability to cumulate restriction periods (2.10 KB, patch)
2017-02-17 11:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14146: Clean a bit and make the code understandable (3.29 KB, patch)
2017-02-17 11:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14146: Add the new pref CumulativeRestrictionPeriods (3.22 KB, patch)
2017-02-17 11:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14146: Add tests for AddReturn + CumulativeRestrictionPeriods (6.02 KB, patch)
2017-03-23 13:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14146: Add the ability to cumulate restriction periods (2.20 KB, patch)
2017-03-23 13:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14146: Clean a bit and make the code understandable (3.39 KB, patch)
2017-03-23 13:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14146: Add the new pref CumulativeRestrictionPeriods (3.31 KB, patch)
2017-03-23 13:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Carolina Andreu 2015-05-05 09:04:15 UTC
We've noticed that when checking in some items with overdues Koha only takes the first checked-in item to apply the restriction period to that borrower.

A borrower has three overdue items. We check-in the first of them, and Koha automatically sets a restriction period for that borrower. We check-in second and third item, but restriction period remains the same number of days, additional days of suspension are not accumulated to the restriction period.

The expected behaviour would be that Koha added days to the restriction period for every single overdue the borrower checked-in, whether the check-in date of the overdue items was.

Build 2015-03-27 on Windows 7, running Google Chrome and also tested with Mozilla Firefox and Internet Explorer.
Comment 1 Owen Leonard 2016-08-12 13:37:32 UTC
(In reply to Carolina Andreu from comment #0)
> We check-in
> second and third item, but restriction period remains the same number of
> days, additional days of suspension are not accumulated to the restriction
> period.

I find that this is still the way it works in master, but I wonder if it's not the expected behavior? Could this be a problem of differing interpretations of how "fine in days" is supposed to work?
Comment 2 Katrin Fischer 2016-11-30 08:36:02 UTC
I have just run some tests on Koha 3.22 - if the second item has been longer overdue, the restriction is recalculated to be longer. 

But it doesn't just add up the days for each returned item. I think that's how the feature is intended to work right now and the other would be a variation that coudl be an optional feature. Switching to enhancement.
Comment 3 Jonathan Druart 2016-11-30 10:56:26 UTC
(In reply to Katrin Fischer from comment #2)
> I have just run some tests on Koha 3.22 - if the second item has been longer
> overdue, the restriction is recalculated to be longer. 
> 
> But it doesn't just add up the days for each returned item. I think that's
> how the feature is intended to work right now and the other would be a
> variation that coudl be an optional feature. Switching to enhancement.

Confirmed, reading the code it's how it's intended to work.
Would it make more sense to add a global switch (syspref) or a new circulation rule?
Comment 4 Jonathan Druart 2016-12-12 15:33:11 UTC
Created attachment 58128 [details] [review]
Bug 14146: Add the new pref CumulativeRestrictionPeriods

Sponsored-by: Orex Digital
Comment 5 Jonathan Druart 2016-12-12 15:33:15 UTC
Created attachment 58129 [details] [review]
Bug 14146: Add tests for AddReturn + CumulativeRestrictionPeriods

Sponsored-by: Orex Digital
Comment 6 Jonathan Druart 2016-12-12 15:33:27 UTC
Created attachment 58130 [details] [review]
Bug 14146: Add the ability to cumulate restriction periods

At the moment the default behaviour is not to cumulate the restriction
periods but to apply the longest one.
This patch set creates a new syspref CumulativeRestrictionPeriods. If
on, the behaviour changes and the restriction periods are cumulated: the
days of the second restriction are added to the actual restriction
period.

We could add a new circulation rule instead of a syspref, but I am not
sure it's very useful to have such granularity for this behaviour (can
be changed if needed).

How it works:
Let's take 2 items, A and B.
A is returned with Na days late, and B Nb days late
The grace period is Ng and there is 1 day of suspension charge per day
of overdue
The suspension period is until day D = Na - Ng + Nb - Ng

I would have expected D = Na + Nb - Ng but it's how it worked before
this patch.

Test plan:
Create several overdue for a given patron
Do the checkins and confirm that the period are added if the pref is on.
If the pref is off, you should not get any changes in the existing behaviour.

Sponsored-by: Orex Digital
Comment 7 Jonathan Druart 2016-12-12 15:33:31 UTC
Created attachment 58131 [details] [review]
Bug 14146: Clean a bit and make the code understandable

The code was a bit weird and this patch cleans it a bit by renaming
variables and adding a variable.

Sponsored-by: Orex Digital
Comment 8 Jonathan Druart 2016-12-13 10:22:44 UTC
Created attachment 58149 [details] [review]
Bug 14146: Add the new pref CumulativeRestrictionPeriods

Sponsored-by: Orex Digital
Comment 9 Nick Clemens 2017-02-17 11:26:23 UTC
Conflict in the tests
Comment 10 Jonathan Druart 2017-02-17 11:55:52 UTC
Created attachment 60406 [details] [review]
Bug 14146: Add tests for AddReturn + CumulativeRestrictionPeriods

Sponsored-by: Orex Digital
Comment 11 Jonathan Druart 2017-02-17 11:55:57 UTC
Created attachment 60407 [details] [review]
Bug 14146: Add the ability to cumulate restriction periods

At the moment the default behaviour is not to cumulate the restriction
periods but to apply the longest one.
This patch set creates a new syspref CumulativeRestrictionPeriods. If
on, the behaviour changes and the restriction periods are cumulated: the
days of the second restriction are added to the actual restriction
period.

We could add a new circulation rule instead of a syspref, but I am not
sure it's very useful to have such granularity for this behaviour (can
be changed if needed).

How it works:
Let's take 2 items, A and B.
A is returned with Na days late, and B Nb days late
The grace period is Ng and there is 1 day of suspension charge per day
of overdue
The suspension period is until day D = Na - Ng + Nb - Ng

I would have expected D = Na + Nb - Ng but it's how it worked before
this patch.

Test plan:
Create several overdue for a given patron
Do the checkins and confirm that the period are added if the pref is on.
If the pref is off, you should not get any changes in the existing behaviour.

Sponsored-by: Orex Digital
Comment 12 Jonathan Druart 2017-02-17 11:56:01 UTC
Created attachment 60408 [details] [review]
Bug 14146: Clean a bit and make the code understandable

The code was a bit weird and this patch cleans it a bit by renaming
variables and adding a variable.

Sponsored-by: Orex Digital
Comment 13 Jonathan Druart 2017-02-17 11:56:06 UTC
Created attachment 60409 [details] [review]
Bug 14146: Add the new pref CumulativeRestrictionPeriods

Sponsored-by: Orex Digital
Comment 14 Marcel de Rooy 2017-03-23 11:23:54 UTC
I do not see a signoff on those patches?
Comment 15 Marcel de Rooy 2017-03-23 11:52:58 UTC
QA: Still looking here btw
Comment 16 Marcel de Rooy 2017-03-23 12:05:23 UTC
+    # FIXME Is it right? I'd have expected 5 * 2 - 1 instead
+    # Same for the others
+    my $expected_expiration = output_pref(
+        {
+            dt         => dt_from_string->add( days => ( 5 - 1 ) * 2 ),

If I look to the code:
my $grace =           DateTime::Duration->new( $unit => $issuing_rule->firstremind );
        my $deltadays = DateTime::Duration->new(             days => $chargeable_units         );
        if ( $deltadays->subtract($grace)->is_positive() ) {
            my $suspension_days = $deltadays * $finedays;
Then I would expect indeed (5-1)*2.
Why would you expect 5*2-1 ?
First you do deltadays - grace (=1) and then you multiply with finedays (2)
Comment 17 Marcel de Rooy 2017-03-23 13:23:19 UTC
Created attachment 61535 [details] [review]
Bug 14146: Add tests for AddReturn + CumulativeRestrictionPeriods

Sponsored-by: Orex Digital

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2017-03-23 13:23:24 UTC
Created attachment 61536 [details] [review]
Bug 14146: Add the ability to cumulate restriction periods

At the moment the default behaviour is not to cumulate the restriction
periods but to apply the longest one.
This patch set creates a new syspref CumulativeRestrictionPeriods. If
on, the behaviour changes and the restriction periods are cumulated: the
days of the second restriction are added to the actual restriction
period.

We could add a new circulation rule instead of a syspref, but I am not
sure it's very useful to have such granularity for this behaviour (can
be changed if needed).

How it works:
Let's take 2 items, A and B.
A is returned with Na days late, and B Nb days late
The grace period is Ng and there is 1 day of suspension charge per day
of overdue
The suspension period is until day D = Na - Ng + Nb - Ng

I would have expected D = Na + Nb - Ng but it's how it worked before
this patch.

Test plan:
Create several overdue for a given patron
Do the checkins and confirm that the period are added if the pref is on.
If the pref is off, you should not get any changes in the existing behaviour.

Sponsored-by: Orex Digital

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2017-03-23 13:23:28 UTC
Created attachment 61537 [details] [review]
Bug 14146: Clean a bit and make the code understandable

The code was a bit weird and this patch cleans it a bit by renaming
variables and adding a variable.

Sponsored-by: Orex Digital

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2017-03-23 13:23:33 UTC
Created attachment 61538 [details] [review]
Bug 14146: Add the new pref CumulativeRestrictionPeriods

Sponsored-by: Orex Digital

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2017-03-23 13:24:02 UTC
QA Comment:

[1] We now assume that the max suspension days should be applied on the individual suspension, not on the accumulation. Is that a logical choice?
No blocker, just mentioning it.

[2] The unit test does not return the case where you would return first the 10 day book and then the 5 day book. This might be interesting to add since formerly the second return would not make a difference.
Note: I tested this case in the interface and it works.
Comment 22 Jonathan Druart 2017-03-23 18:43:31 UTC
(In reply to Marcel de Rooy from comment #16)
> +    # FIXME Is it right? I'd have expected 5 * 2 - 1 instead
> +    # Same for the others
> +    my $expected_expiration = output_pref(
> +        {
> +            dt         => dt_from_string->add( days => ( 5 - 1 ) * 2 ),
> 
> If I look to the code:
> my $grace =           DateTime::Duration->new( $unit =>
> $issuing_rule->firstremind );
>         my $deltadays = DateTime::Duration->new(             days =>
> $chargeable_units         );
>         if ( $deltadays->subtract($grace)->is_positive() ) {
>             my $suspension_days = $deltadays * $finedays;
> Then I would expect indeed (5-1)*2.
> Why would you expect 5*2-1 ?
> First you do deltadays - grace (=1) and then you multiply with finedays (2)

Yes but I was wondering if it is really what we expect.
If a checkin is 5 days late, with 2 days of fine per day of overdue and 1 day of grace: I think it would make more sense to apply the grace on the total of fine days, then 5 * 2 - 1.

(In reply to Marcel de Rooy from comment #21)
> QA Comment:
> 
> [1] We now assume that the max suspension days should be applied on the
> individual suspension, not on the accumulation. Is that a logical choice?
> No blocker, just mentioning it.

I do not get it, what do you mean?
There is now a pref to choose the behavior you want to apply.

> [2] The unit test does not return the case where you would return first the
> 10 day book and then the 5 day book. This might be interesting to add since
> formerly the second return would not make a difference.
> Note: I tested this case in the interface and it works.

Hum maybe, but looking at the current code it seems hard to change it that much to break that condition. I would have added it if I did not have to c/p ~70 lines of code...
Comment 23 Marcel de Rooy 2017-03-24 07:10:54 UTC
(In reply to Jonathan Druart from comment #22)
> Yes but I was wondering if it is really what we expect.
> If a checkin is 5 days late, with 2 days of fine per day of overdue and 1
> day of grace: I think it would make more sense to apply the grace on the
> total of fine days, then 5 * 2 - 1.

I would stick to the current policy. Subtract the grace period first. You should not be fined over the grace period. Which you do if you do 5*2-1.
Comment 24 Marcel de Rooy 2017-03-24 07:15:15 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Marcel de Rooy from comment #21)
> > [1] We now assume that the max suspension days should be applied on the
> > individual suspension, not on the accumulation. Is that a logical choice?
> > No blocker, just mentioning it.
> 
> I do not get it, what do you mean?
> There is now a pref to choose the behavior you want to apply.

If you return 10 books which result in say 3 fine days each. Is 30.
But the max suspension days is say 5.
The current logic compares 10 times 3 with 5 and says ok.
My question was: Should it be applied to the accumulation. So should we say when it reaches 6 with the second book: Max reached?
Comment 25 Kyle M Hall 2017-03-31 10:54:24 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 26 Katrin Fischer 2017-04-02 16:18:49 UTC
This won't get ported back to 16.11.x as it is an enhancement.