Description
Hugo Agud
2019-11-29 14:42:35 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. 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! |