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.
(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?
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.
(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?
Created attachment 58128 [details] [review] Bug 14146: Add the new pref CumulativeRestrictionPeriods Sponsored-by: Orex Digital
Created attachment 58129 [details] [review] Bug 14146: Add tests for AddReturn + CumulativeRestrictionPeriods Sponsored-by: Orex Digital
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
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
Created attachment 58149 [details] [review] Bug 14146: Add the new pref CumulativeRestrictionPeriods Sponsored-by: Orex Digital
Conflict in the tests
Created attachment 60406 [details] [review] Bug 14146: Add tests for AddReturn + CumulativeRestrictionPeriods Sponsored-by: Orex Digital
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
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
Created attachment 60409 [details] [review] Bug 14146: Add the new pref CumulativeRestrictionPeriods Sponsored-by: Orex Digital
I do not see a signoff on those patches?
QA: Still looking here btw
+ # 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)
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>
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>
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>
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>
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.
(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...
(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.
(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?
Pushed to master for 17.05, thanks Jonathan!
This won't get ported back to 16.11.x as it is an enhancement.