[Bug 24138] New: suspension miscalculated when Suspension charging interval bigger than 1 and Max. suspension duration is defined
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Bug ID: 24138 Summary: suspension miscalculated when Suspension charging interval bigger than 1 and Max. suspension duration is defined Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: hagud@orex.es QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 95886 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95886&action=edit 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 95887 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95887&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org Severity|enhancement |major Assignee|koha-bugs@lists.koha-commun |jonathan.druart@bugs.koha-c |ity.org |ommunity.org -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #4 from Hugo Agud <hagud@orex.es> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Hugo, this patch is only affecting suspension duration for patrons (debarment). It should not affect the calculation of fines. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Hugo Agud <hagud@orex.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95886|0 |1 is obsolete| | --- Comment #6 from Hugo Agud <hagud@orex.es> --- Created attachment 95945 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95945&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Hugo Agud <hagud@orex.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95887|0 |1 is obsolete| | --- Comment #7 from Hugo Agud <hagud@orex.es> --- Created attachment 95946 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95946&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Hugo Agud <hagud@orex.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|martin.renvoize@ptfs-europe |m.de.rooy@rijksmuseum.nl |.com | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Why do we need a fine charging interval and a suspension interval btw ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #10 from Hugo Agud <hagud@orex.es> --- 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- But is not really relevant to ask :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Also note that I am not modifying anything here, just moving up a code that is not at the correct place. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Using in_units from Duration is not entirely without risk btw. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- The main confusion must have been the grace subtraction. So I will pass qa. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95945|0 |1 is obsolete| | --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 96100 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96100&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95946|0 |1 is obsolete| | --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 96101 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96101&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |19.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|19.05.00 |20.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable CC| |lucas@bywatersolutions.com --- Comment #22 from Lucas Gass <lucas@bywatersolutions.com> --- backported to 19.11.x for 19.11.01 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable --- Comment #23 from Lucas Gass <lucas@bywatersolutions.com> --- backported to 19.05.x for 19.05.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 --- Comment #24 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 103193 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103193&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24138 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103193|0 |1 is obsolete| | --- Comment #25 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Comment on attachment 103193 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103193 Bug 25138: (follow-up) Point of sale should use library instead of branch Bug number typo! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org