Bug 24138 - suspension miscalculated when Suspension charging interval bigger than 1 and Max. suspension duration is defined
Summary: suspension miscalculated when Suspension charging interval bigger than 1 and ...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-29 14:42 UTC by Hugo Agud
Modified: 2019-12-20 17:18 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24138: Move the calculation out of the sub (8.17 KB, patch)
2019-11-29 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24138: Fix calculation of suspension days when a limit is set (5.02 KB, patch)
2019-11-29 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24138: Move the calculation out of the sub (8.22 KB, patch)
2019-12-03 14:43 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 24138: Fix calculation of suspension days when a limit is set (5.07 KB, patch)
2019-12-03 14:43 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 24138: Move the calculation out of the sub (8.31 KB, patch)
2019-12-09 13:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24138: Fix calculation of suspension days when a limit is set (5.16 KB, patch)
2019-12-09 13:48 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 Hugo Agud 2019-11-29 14:42:35 UTC
When a circulation rule have a suspension charging interval bigger than one and also has defined a max suspension day, the fine is miscalculated.

The issue is that the max suspension day is affected by "suspension Charging interval" for example if we have a suspension interval of 15 and max suspension days of 365 , koha will only apply of 24 suspension day, it seems there a direct inversion relation between both variables.

reproduce plan.

set up a rule of 7 days suspension every 15 days with max suspension of 365 days
check out a book with due date 28/10/2018, and check in, you will see that the suspension is just 24 days

If you increase the max suspension day to 730, you will have a suspension of 48 days (aprox)

If you increase the max suspension day to (15x365) you will have the year suspension.


If you define 1 suspension day every day with max of 365 and you check out andcheck in the same dates, you will have the suspension of 365 days
Comment 1 Jonathan Druart 2019-11-29 15:28:42 UTC
Created attachment 95886 [details] [review]
Bug 24138: Move the calculation out of the sub

No changes expected here. For the next patch we are going to need to add
test and calculate the new debarment date. To ease the writing of these
tests the calculation is moved out of the existing subroutine.
Comment 2 Jonathan Druart 2019-11-29 15:28:46 UTC
Created attachment 95887 [details] [review]
Bug 24138: Fix calculation of suspension days when a limit is set

If there is a limit for the number of suspension days
(maxsuspensiondays), the calculation is wrong. We are reducing the
number of days before taking into account the suspension charging
interval.

For instance, a checkin is 1 year late and the circ rules are defined to
charge 7 days every 15 days.
It results in 365 * 7 / 15 days of suspension => 170 days

Before this patch the calculation was:
365 * 7 limited to 333, 333 / 15 => 22 days

Test plan:
Given the examples in the commit messages and the description of the bug
report, setup complex circulation rules and confirm that the debarment
dates are calculated correctly
Comment 3 Martin Renvoize 2019-11-29 15:57:54 UTC
Code looks solid, but it would be good to get Hugo's signoff that he believes it's working as expected before I QA it.
Comment 4 Hugo Agud 2019-12-03 11:39:24 UTC
I am checking the patch and it is working fine, BUT I have realized that fines only applies if Fine grace period is applied... it is mandatory add a value, independtl of the syspref of grace period.

from my point of view it is logical that you need to add a value, but as it is not a mandatory field perhaps it creates confussion?

On the other hand I do not know if this issue is related with this bug or not, I am ready to sign it
Comment 5 Jonathan Druart 2019-12-03 13:17:05 UTC
Hugo, this patch is only affecting suspension duration for patrons (debarment).
It should not affect the calculation of fines.
Comment 6 Hugo Agud 2019-12-03 14:43:19 UTC
Created attachment 95945 [details] [review]
Bug 24138: Move the calculation out of the sub

No changes expected here. For the next patch we are going to need to add
test and calculate the new debarment date. To ease the writing of these
tests the calculation is moved out of the existing subroutine.

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 7 Hugo Agud 2019-12-03 14:43:22 UTC
Created attachment 95946 [details] [review]
Bug 24138: Fix calculation of suspension days when a limit is set

If there is a limit for the number of suspension days
(maxsuspensiondays), the calculation is wrong. We are reducing the
number of days before taking into account the suspension charging
interval.

For instance, a checkin is 1 year late and the circ rules are defined to
charge 7 days every 15 days.
It results in 365 * 7 / 15 days of suspension => 170 days

Before this patch the calculation was:
365 * 7 limited to 333, 333 / 15 => 22 days

Test plan:
Given the examples in the commit messages and the description of the bug
report, setup complex circulation rules and confirm that the debarment
dates are calculated correctly

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 8 Marcel de Rooy 2019-12-06 11:15:06 UTC
my $suspension_days = $deltadays * $finedays;
finedays is a number, deltadays is not ?

and then:
floor( $suspension_days->in_units('days') etc

suspension_days is just a number not a duration?

Does not look good to me.
Why do the tests pass ? Do we need more tests ?
Comment 9 Marcel de Rooy 2019-12-06 11:18:58 UTC
Why do we need a fine charging interval and a suspension interval btw ?
Comment 10 Hugo Agud 2019-12-06 11:23:21 UTC
because some libraries doesn't suspend 1 day a patron for every day delayed, as the same way that not all libraries charge 1 usd per every day delayed.

In fact I guess that the majority has "x" suspension days for every "y" delayed days, at least in Spain, I am not sure how it works in other countries.. it gives more flexibility to circulation rules

hope it helps!
Comment 11 Marcel de Rooy 2019-12-06 11:25:52 UTC
(In reply to Hugo Agud from comment #10)
> because some libraries doesn't suspend 1 day a patron for every day delayed,
> as the same way that not all libraries charge 1 usd per every day delayed.
> 
> In fact I guess that the majority has "x" suspension days for every "y"
> delayed days, at least in Spain, I am not sure how it works in other
> countries.. it gives more flexibility to circulation rules
> 
> hope it helps!

Hi Hugo
My question is about the two intervals. Why would you charge 1 usd per 2 days and 2 suspension days per another interval ?
Comment 12 Marcel de Rooy 2019-12-06 11:27:00 UTC
But is not really relevant to ask :)
Comment 13 Jonathan Druart 2019-12-06 13:08:06 UTC
(In reply to Marcel de Rooy from comment #8)
> my $suspension_days = $deltadays * $finedays;
> finedays is a number, deltadays is not ?
> 
> and then:
> floor( $suspension_days->in_units('days') etc
> 
> suspension_days is just a number not a duration?

I am not sure I understand your concern here.

perl -MDateTime::Duration -e 'my $dur = DateTime::Duration->new( days => 3 ); $dur *= 3; print $dur->in_units("days")' 

This is valid and will display 9
Comment 14 Jonathan Druart 2019-12-06 13:09:12 UTC
Also note that I am not modifying anything here, just moving up a code that is not at the correct place.
Comment 15 Marcel de Rooy 2019-12-09 13:11:00 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Marcel de Rooy from comment #8)
> > my $suspension_days = $deltadays * $finedays;
> > finedays is a number, deltadays is not ?
> > 
> > and then:
> > floor( $suspension_days->in_units('days') etc
> > 
> > suspension_days is just a number not a duration?
> 
> I am not sure I understand your concern here.
> 
> perl -MDateTime::Duration -e 'my $dur = DateTime::Duration->new( days => 3
> ); $dur *= 3; print $dur->in_units("days")' 
> 
> This is valid and will display 9

Which might be a bit confusing too, but okay. There must still be something wrong somewhere, my test does not work? Will have another look.
Comment 16 Marcel de Rooy 2019-12-09 13:29:38 UTC
This could be out of scope: But isnt it strange that calculating chargeable units does not look at FinesIncludeGracePeriod ?
I would expect fine and suspension period should follow the same algorithm.

But the 'test'
    $deltadays->subtract($grace)->is_positive()
removes the grace days from the suspension calculation..
Comment 17 Marcel de Rooy 2019-12-09 13:45:46 UTC
Using in_units from Duration is not entirely without risk btw.
Comment 18 Marcel de Rooy 2019-12-09 13:46:22 UTC
The main confusion must have been the grace subtraction. So I will pass qa.
Comment 19 Marcel de Rooy 2019-12-09 13:47:55 UTC
Created attachment 96100 [details] [review]
Bug 24138: Move the calculation out of the sub

No changes expected here. For the next patch we are going to need to add
test and calculate the new debarment date. To ease the writing of these
tests the calculation is moved out of the existing subroutine.

Signed-off-by: Hugo Agud <hagud@orex.es>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2019-12-09 13:48:00 UTC
Created attachment 96101 [details] [review]
Bug 24138: Fix calculation of suspension days when a limit is set

If there is a limit for the number of suspension days
(maxsuspensiondays), the calculation is wrong. We are reducing the
number of days before taking into account the suspension charging
interval.

For instance, a checkin is 1 year late and the circ rules are defined to
charge 7 days every 15 days.
It results in 365 * 7 / 15 days of suspension => 170 days

Before this patch the calculation was:
365 * 7 limited to 333, 333 / 15 => 22 days

Test plan:
Given the examples in the commit messages and the description of the bug
report, setup complex circulation rules and confirm that the debarment
dates are calculated correctly

Signed-off-by: Hugo Agud <hagud@orex.es>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Martin Renvoize 2019-12-10 16:18:23 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 22 Lucas Gass 2019-12-20 17:11:39 UTC
backported to 19.11.x for 19.11.01
Comment 23 Lucas Gass 2019-12-20 17:18:32 UTC
backported to 19.05.x for 19.05.06