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
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.
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
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.
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
Hugo, this patch is only affecting suspension duration for patrons (debarment). It should not affect the calculation of fines.
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>
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>
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 ?
Why do we need a fine charging interval and a suspension interval btw ?
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!
(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 ?
But is not really relevant to ask :)
(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
Also note that I am not modifying anything here, just moving up a code that is not at the correct place.
(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.
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..
Using in_units from Duration is not entirely without risk btw.
The main confusion must have been the grace subtraction. So I will pass qa.
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>
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>
Nice work everyone! Pushed to master for 20.05
backported to 19.11.x for 19.11.01
backported to 19.05.x for 19.05.06
Created attachment 103193 [details] [review] Bug 25138: (follow-up) Point of sale should use library instead of branch Fixes a misssed occurence on the pay.tt page. To test: - Don't have a cash register set up (or you have archived the cash register for the library) - Go to 'Point of sale' from the main staff interface page - Verify library/branch in the note shown: You must have at least one cash register associated with this library before you can record payments.
Comment on attachment 103193 [details] [review] Bug 25138: (follow-up) Point of sale should use library instead of branch Bug number typo!