Bug 14293

Summary: Error in the calculation of the suspension of users per day
Product: Koha Reporter: MetaBiblioteca Colombia <felipe.gomez3>
Component: CirculationAssignee: Thibaud Guillot <thibaud.guillot>
Status: In Discussion --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P4 CC: arthur.suzuki, david, felipe.gomez3, gmcharlt, jonathan.druart, katrin.fischer, koha, kyle.m.hall, kyle, m.de.rooy, martin.renvoize, mtompset, niamh.walker-headon, nick, thibaud.guillot, tomascohen
Version: MainKeywords: needs_rfc
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14975
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23382
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 14293: The suspension should be calculated in days
Bug 14293: The suspension should be calculated in days
Bug 14293: The suspension should be calculated in days
Bug 14293: Add more tests and improve get_chargeable_units
Bug 14293: The suspension should be calculated in days
Bug 14293: The suspension should be calculated in days
Bug 14293: Add more tests and improve get_chargeable_units
Bug 14293: The suspension should be calculated in days
Bug 14293: Add more tests and improve get_chargeable_units
Bug 14293: Add more tests and fix floor/ceil
Bug 14293: The suspension should be calculated in days
Bug 14293: Add more tests and improve get_chargeable_units
Bug 14293: Add more tests and fix floor/ceil
Bug 14293: The suspension should be calculated in days
Bug 14293: Rebasing tests
Bug 14293: The suspension should be calculated in days
Bug 14293: Rebasing tests
Bug 14293: The suspension should be calculated in days
Bug 14293: The suspension should be calculated in days
Bug 14293: The suspension should be calculated in days
Bug 14293: (QA follow-up) Remove copy and paste test

Description MetaBiblioteca Colombia 2015-05-29 05:39:18 UTC
Currently the system is miscalculating the day suspension when a loan is made hours. Add 24 days for each day in fines.

The problem occurs when changed in version 3.16.X

my $deltadays = $calendar->days_between( $dt_due, $dt_today );

by
my $chargeable_units = C4::Overdues::get_chargeable_units($unit, $dt_due, $dt_today, $branchcode);

In version 3.18.X, 3.20.X

And the line is not necessary:

$finedays = $finedays * 24 if ( $unit eq 'hours' );

Which is generating an error in the calculation of the suspension days

______________________________________
Spanish
Error en el calculo de la suspensión de usuarios por Días

Actualmente el sistema se encuentra calculando mal los días de suspención cuando se hace un préstamo por Horas. Agrega 24 Días por cada día de multa.

El problema aparece Cuando cambiaron  en la versión 3.16.X
my $deltadays = $calendar->days_between( $dt_due, $dt_today );

Por  
my $chargeable_units = C4::Overdues::get_chargeable_units($unit, $dt_due, $dt_today, $branchcode);
En la versión 3.18.X, 3.20.X

Ya no se hace necesario la línea:

$finedays = $finedays * 24 if ( $unit eq 'hours' );

La cual está generando un error en el cálculo de la suspensión por días.
Comment 1 Jonathan Druart 2015-09-04 10:02:58 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2015-09-11 23:06:52 UTC
Created attachment 42492 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 3 Nick Clemens 2015-09-11 23:08:12 UTC
Test plan worked as expected with one question: if they are hours overdue on an hourly loan, and not a day overdue, it seems to not suspend them.  Should it?
Comment 4 Kyle M Hall 2015-10-23 12:59:27 UTC
(In reply to Nick Clemens from comment #3)
> Test plan worked as expected with one question: if they are hours overdue on
> an hourly loan, and not a day overdue, it seems to not suspend them.  Should
> it?

I suppose the question is, should hourly loans suspend in days the number of hours overdue? If an hourly loan is 3 hours late, should the patron be suspended for 3 days? If so, changes are needed.

This patch makes it so that hourly loans are suspended based on the number of *days* the item is overdue. Anything less than 24 hours rounds down to 0 days. Should hourly loans suspend for a minimum of 1 day if they are overdue by less than 24 hours?

I think the logic of how hourly loan suspension should work needs to be decided before it can be decided if this patch is correct, or if it needs additional changes.
Comment 5 Koha Team University Lyon 3 2019-02-07 10:00:53 UTC
The present path doesn't apply anymore :

t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t
CONFLICT (content): Merge conflict in t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t
Auto-merging C4/Circulation.pm
CONFLICT (content): Merge conflict in C4/Circulation.pm
error: Failed to merge in the changes.

To give a feedback to Kyle'question :
> I suppose the question is, should hourly loans suspend in days the number of hours overdue? If an hourly loan is 3 hours late, should the patron be suspended for 3 days? If so, changes are needed.

In our library, we don't want hours late to be change in days suspension. 
If "Suspension in days (day)" = 1, we want that 2h or 2mn late calculate 1 day of suspension.
If there more than 24h, then it will be 2 days of suspension.

I hope that this bug could be correct easily.

Sonia
Comment 6 Jonathan Druart 2019-02-16 17:36:00 UTC
Created attachment 85210 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 7 Jonathan Druart 2019-02-16 17:36:11 UTC
Created attachment 85211 [details] [review]
Bug 14293: Add more tests and improve get_chargeable_units

Not sure at all about this change, especially because it could modify
CalcFine
Comment 8 Jonathan Druart 2019-02-16 17:37:03 UTC
Sonia, please give this new version a try.
Comment 9 Arthur Suzuki 2019-02-18 16:36:24 UTC
This new patch do suspend the user until next day from the first minute overdue up until 24h late (non-inclusive).

Then suspends for one more day for each period of 24h overdue.

IMHO it is quite fair that one who is overdue for the night and brings back yesterday's hourly loans on the next morning may not borrow new books on the same day.

If someone take a hourly loan and only brings back the book on the next morning then one might not get suspended for let say 10 days if one is 10 periods of 1h overdue.

This behaviour is same whatever the hourly loan period is (tried with 1h and 6h), which I guess is the expected/desired behaviour.
Comment 10 Arthur Suzuki 2019-02-18 16:37:15 UTC
Created attachment 85251 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 11 Arthur Suzuki 2019-02-18 16:38:03 UTC
Created attachment 85252 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 12 Arthur Suzuki 2019-02-18 17:39:26 UTC
Meh, guess I broke up something while trying to sign the patches, only one of them is signed and I cannot apply the patches anymore using git-bz...
I wonder what I screwed up here :/
Comment 13 Arthur Suzuki 2019-02-18 17:53:44 UTC
Created attachment 85255 [details] [review]
Bug 14293: Add more tests and improve get_chargeable_units

Not sure at all about this change, especially because it could modify
CalcFine

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 14 Arthur Suzuki 2019-02-20 19:43:14 UTC
Created attachment 85407 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 15 Arthur Suzuki 2019-02-20 19:44:06 UTC
Created attachment 85408 [details] [review]
Bug 14293: Add more tests and improve get_chargeable_units

Not sure at all about this change, especially because it could modify
CalcFine

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 16 Arthur Suzuki 2019-02-20 19:49:32 UTC
Ok, patches signed properly after been applied to the right version (master).
Comment 17 Koha Team University Lyon 3 2019-02-25 15:26:50 UTC
Hello,
I've begun the tests but there's a case there the calculation is wrong.
If you have this circulation rules for an hourly loan : 
Suspension in days (day) = 1 -> Suspension charging interval = 2

With few hours late, no suspension are charged : it should be 2 days.
With one day + few hours late, the suspension is 2 days, it should be 4 days.


(but it works for Suspension in days (day) = 1 -> Suspension charging interval = 1)

Sonia
Comment 18 Katrin Fischer 2019-02-25 17:49:01 UTC
(In reply to Koha Team Lyon 3 from comment #17)
> Hello,
> I've begun the tests but there's a case there the calculation is wrong.
> If you have this circulation rules for an hourly loan : 
> Suspension in days (day) = 1 -> Suspension charging interval = 2
> 
> With few hours late, no suspension are charged : it should be 2 days.
> With one day + few hours late, the suspension is 2 days, it should be 4 days.
> 
> 
> (but it works for Suspension in days (day) = 1 -> Suspension charging
> interval = 1)
> 
> Sonia

Should it not be the other way around? suspension in days 2 and charging interval  1 for the results you describe?
Comment 19 Niamh 2019-02-25 17:56:12 UTC
Is this because the suspensions only operate in days but the loan rule is in hours? So if the item is returned on the same day Koha does not recognise it as overdue... especially if the cron job only runs overnight?
Comment 20 Jonathan Druart 2019-02-27 16:32:00 UTC
Could you please list the different situations you have tested (circulation rules values) then what you would have expected and what you get?

With OK and NOK in front of each cases.
Comment 21 Jonathan Druart 2019-02-27 17:00:06 UTC
Created attachment 85788 [details] [review]
Bug 14293: Add more tests and fix floor/ceil
Comment 22 Jonathan Druart 2019-02-27 17:00:27 UTC
My tests cover the following situations:

* 2 days of suspension every day (finedays = 2, suspension_chargeperiod = 1)
  - 20 days late => 40 days of suspension
  - 1 day late => 2 days of suspension
  - 2 hours late => 2 days of suspension
  - 2 hours before => 0 day of suspension

* 1 day of suspension every 2 days (finedays = 1, suspension_chargeperiod = 2)
  - 20 days late => 10 days of suspension
  - 1 day late => 1 day of suspension (was 2 before the last patch)
  - 2 hours late => 1 day of suspension (was 2 before the last patch)
  - 2 hours before => 0 day of suspension
Comment 23 Koha Team University Lyon 3 2019-02-28 08:42:45 UTC Comment hidden (obsolete)
Comment 24 Koha Team University Lyon 3 2019-02-28 08:51:25 UTC
Hello Katrin, you're right. I have to buy a new brain. I've begun my tests with the right logic and I don't know why, I swapped...

Jonathan, before redoing the tests,I'm not sure we are on the same logic for the case "1 day of suspension every 2 days (finedays = 1, suspension_chargeperiod = 2)"

  - 20 days late => 10 days of suspension : OK

  - 1 day late => 1 day of suspension (was 2 before the last patch)
-> Should we begin to add charge before 2 days late ?
I would say :
  * 1 day late => 0 day of suspension (was 2 before the last patch)
  * 2 days late => 1 day of suspension

  - 2 hours late => 1 day of suspension (was 2 before the last patch)
The same here 
   * 2 hours late => 0 day of suspension (was 2 before the last patch)
   * 2 hours late + 1 day => 1 day of supension

  - 2 hours before => 0 day of suspension : OK

In my opinion, if suspension_chargeperiod = 2, we should not charge on the first day... but, I'm not sure of the behaviour in the current version of Koha.

Other opinions on that?
Comment 25 Jonathan Druart 2019-02-28 13:51:07 UTC
(In reply to Koha Team Lyon 3 from comment #24)
> In my opinion, if suspension_chargeperiod = 2, we should not charge on the
> first day... but, I'm not sure of the behaviour in the current version of
> Koha.

Did not test neither remember. But my feeling is that: if you are late, you pay. If you do not want this behavior you could use the grace.

Before continuing on this maybe we should ask on the list?
Comment 26 Katrin Fischer 2019-03-01 06:56:54 UTC
(In reply to Jonathan Druart from comment #25)
> (In reply to Koha Team Lyon 3 from comment #24)
> > In my opinion, if suspension_chargeperiod = 2, we should not charge on the
> > first day... but, I'm not sure of the behaviour in the current version of
> > Koha.
> 
> Did not test neither remember. But my feeling is that: if you are late, you
> pay. If you do not want this behavior you could use the grace.
> 
> Before continuing on this maybe we should ask on the list?

I think we could use the "when to charge" parameter for this as it's the same problem as with fines. It allows to charge at the beginning of the interval (charge first day) or at the end (charge after 2 days). This way the behavior would be configurable and we even have the option already.

What do you think?
Comment 27 Arthur Suzuki 2019-03-18 13:46:54 UTC
(In reply to Katrin Fischer from comment #26)
> I think we could use the "when to charge" parameter for this as it's the
> same problem as with fines. It allows to charge at the beginning of the
> interval (charge first day) or at the end (charge after 2 days). This way
> the behavior would be configurable and we even have the option already.
> 
> What do you think?

Agreed :)
Comment 28 Jonathan Druart 2019-04-29 02:32:38 UTC
So, what's next? Waiting something from me?
Comment 29 Koha Team University Lyon 3 2019-05-02 13:56:42 UTC
Hi,
I've tested when patrons are charged with days of suspension in a 18.11 version. If the setting "Suspension charging interval" is 2, they are not charged if they are only one day late. They are charged if they are 2 days late.

I think that we shouldn't change it with this bug. Libraries are certainly used to this behaviour and to my opinion, this rules should be kept when calculting suspension for loans in hours.

Sonia
Comment 30 Arthur Suzuki 2019-06-23 22:33:41 UTC
I guess we could move on this bugs since anyone agreed that this solves at least the hourly loan issue.
From what I understood following the discussion, there is a bigger issue with the whole fine calculation rules but I'd say it's not in the same scope and shall be documented in another bugzilla.
Comment 31 Mark Tompsett 2019-06-24 15:30:28 UTC
Created attachment 90955 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 32 Mark Tompsett 2019-06-24 15:30:32 UTC
Created attachment 90956 [details] [review]
Bug 14293: Add more tests and improve get_chargeable_units

Not sure at all about this change, especially because it could modify
CalcFine

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 33 Mark Tompsett 2019-06-24 15:30:35 UTC
Created attachment 90957 [details] [review]
Bug 14293: Add more tests and fix floor/ceil

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 34 Mark Tompsett 2019-06-24 15:32:22 UTC
(In reply to Arthur Suzuki from comment #30)
> I guess we could move on this bugs since anyone agreed that this solves at
> least the hourly loan issue.
> From what I understood following the discussion, there is a bigger issue
> with the whole fine calculation rules but I'd say it's not in the same scope
> and shall be documented in another bugzilla.

Given this comment, and the tests actually testing the changes. I figured it needed to be signed off to move forward.
Comment 35 Jonathan Druart 2019-06-24 15:41:28 UTC
I understand from comment 29 that Sonia disagrees with the behavior of this patch.
Comment 36 Kyle M Hall 2019-06-28 12:37:40 UTC
This new algorithm for calculating due dates in days derived from hours will result in different due dates than what is currently expected:

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 28/45
    #   Failed test 'Test at line 1886'
    #   at t/db_dependent/Circulation.t line 93.
    #          got: '2019-07-01'
    #     expected: '2019-06-30'
    # Looks like you failed 1 test of 21.
t/db_dependent/Circulation.t .. 29/45
#   Failed test 'AddReturn + suspension_chargeperiod'
#   at t/db_dependent/Circulation.t line 1976.
t/db_dependent/Circulation.t .. 45/45 # Looks like you failed 1 test of 45.
t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/45 subtests

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 256 Tests: 45 Failed: 1)
  Failed test:  29
  Non-zero exit status: 1
Files=1, Tests=45, 28 wallclock secs ( 0.08 usr  0.01 sys + 19.32 cusr  3.82 csys = 23.23 CPU)
Result: FAIL
Comment 37 Koha Team University Lyon 3 2019-08-30 08:22:32 UTC
Hi,
Yes Jonathan, I'm not really fond of the behavior proposed because it changes the current calculation of suspension days for the loan in days. 
I don't want to block the resolution of this issue, but if we keep the correction proposed, We just have to be aware that we should explain that there's a change and that people have to check their circulation rules if they use the Suspension charging interval. 
But for the moment, I'm not sure a lot of libraries are using it.... 
Sonia
Comment 38 Katrin Fischer 2019-09-02 06:30:51 UTC
(In reply to Koha Team Lyon 3 from comment #37)
> But for the moment, I'm not sure a lot of libraries are using it.... 
> Sonia

We have 2 or 3 libraries using it currently (one is in Spain, it appears common there too).

I haven't read through all the bug to see what it changes, just wanted to leave a note that this is probably a wider used feature.
Comment 39 Niamh 2019-09-25 15:53:46 UTC
We are using Suspensions only. We don't 'charge fines'. This miscalculation has a local impact for us where if a restricted loan or short loan item is 5 hours overdue there is no suspension for the patron who kept it over, where as if we were using fines they would be penalised. So we are definitely interested in getting this sorted.
Comment 40 Arthur Suzuki 2019-10-08 09:32:57 UTC
Hi,
Patch applies just fine on master however when I try to return an issue I get an infinite loading and no restriction to the user.
Also I think there is an error in C4/Circulation.pm line 2270, floor has been changed to ceil and I suspect this affect day loans as well (at least it changes the behaviour as mentioned by Sonia earlier).
Arthur
Comment 41 Arthur Suzuki 2019-10-08 09:38:20 UTC
Hi,
Patch applies just fine on master however when I try to return an issue I get an infinite loading and no restriction to the user.
Also I think there is an error in C4/Circulation.pm line 2270, floor has been changed to ceil and I suspect this affect day loans as well (at least it changes the behaviour as mentioned by Sonia earlier).
Arthur
Comment 42 Tomás Cohen Arazi 2021-03-06 00:12:14 UTC
(In reply to Jonathan Druart from comment #35)
> I understand from comment 29 that Sonia disagrees with the behavior of this
> patch.

Sonia's concern can be addressed by adding an option, analogous to fines' 'When to charge', and defaulting it to end of the interval, of course.
Comment 43 Katrin Fischer 2023-08-05 11:29:18 UTC
(In reply to Tomás Cohen Arazi from comment #42)
> (In reply to Jonathan Druart from comment #35)
> > I understand from comment 29 that Sonia disagrees with the behavior of this
> > patch.
> 
> Sonia's concern can be addressed by adding an option, analogous to fines'
> 'When to charge', and defaulting it to end of the interval, of course.

I think we could even use the one used for fines now: "When to charge"
Comment 44 Thibaud Guillot 2023-11-17 10:18:09 UTC
Created attachment 159075 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 45 Thibaud Guillot 2023-11-17 10:18:12 UTC
Created attachment 159076 [details] [review]
Bug 14293: Rebasing tests

--Including:--

    Bug 14293: Add more tests and improve get_chargeable_units

    Not sure at all about this change, especially because it could modify
    CalcFine

    Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

    Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

    Bug 14293: Add more tests and fix floor/ceil

    Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 46 Thibaud Guillot 2023-11-17 10:21:11 UTC
Hello,

I'll try to move this patch forward, I rebased the patch first before adding the "when to charge" option so that everyone can test it under fresh master conditions. Thanks for your feedback
Comment 47 David Nind 2023-11-20 00:28:48 UTC
Created attachment 159128 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 48 David Nind 2023-11-20 00:28:52 UTC
Created attachment 159129 [details] [review]
Bug 14293: Rebasing tests

--Including:--

    Bug 14293: Add more tests and improve get_chargeable_units

    Not sure at all about this change, especially because it could modify
    CalcFine

    Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

    Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

    Bug 14293: Add more tests and fix floor/ceil

    Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 49 David Nind 2023-11-20 00:37:23 UTC
I've tested and signed off. But feel free to change back to "Needs signoff" if this isn't sufficient.

(I'm always nervous about testing circulation and fine rules - "Here be dragons".)

Testing notes using KTD:

1. Set the "All All" rule as follows:
   - Unit: change from Days to Hours
   - Suspension in days (day): change from blank to 2
   - Max. suspension duration (day): left blank
   - Suspension charging interval: 1

2. Items checked out on 20 November 2023.

3. Results before the patch was applied (I set the due date for an item to 11/09/2023). When checked in, the message says:
   "[Patron name] is now debarred until 06/05/2055."

4. After the patch was applied, the message on check in says: "[Patron name] is now debarred until 12/10/2023."
   - Matches what was expected: difference between due date and checked in date = 10; x2 = 20 days = 12/10/2023

5. I then set "Suspension in days (day)" to 1. The message on check in says: "[Patron name] is now debarred until 11/30/2023."
   - Matches what was expected: difference between due date and checked in date = 10; x1 = 10 days = 11/30/2023

6. The tests pass: prove t/db_dependent/Circulation/maxsuspensiondays.t
Comment 50 Marcel de Rooy 2023-11-24 07:38:08 UTC
 FAIL
   FAIL   git manipulation
                The file has been added and deleted in the same patchset

 OK     C4/Circulation.pm

 WARN   C4/Overdues.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 122, now: 125)

 WARN   t/db_dependent/Circulation/maxsuspensiondays.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 52, now: 77)


Processing additional checks

        * Signed-off-by line '    Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>' seems invalid
        * Signed-off-by line '    Signed-off-by: Mark Tompsett <mtompset@hotmail.com>' seems invalid
        * Signed-off-by line '    Signed-off-by: Mark Tompsett <mtompset@hotmail.com>' seems invalid

Running tests (1)
        * Proving /usr/share/koha/t/db_dependent/Circulation/maxsuspensiondays.t KO!
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Incorrect datetime value: 'SCALAR(0x5595a6847398)' for column `koha_myclone`.`borrowers`.`updated_on` at row 1 at /usr/share/koha/Koha/Object.pm line 170
 Invalid value passed, borrowers.updated_on=SCALAR(0x5595a6847398) expected type is datetime # Looks like your test exited with 11 before it could output anything.
 /usr/share/koha/t/db_dependent/Circulation/maxsuspensiondays.t ..
Dubious, test returned 11 (wstat 2816, 0xb00)
 Failed 4/4 subtests
Comment 51 Thibaud Guillot 2023-11-27 15:43:03 UTC
Created attachment 159277 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly
Comment 52 Thibaud Guillot 2023-11-27 15:45:52 UTC
(In reply to Marcel de Rooy from comment #50)
>  FAIL
>    FAIL   git manipulation
>                 The file has been added and deleted in the same patchset
> 
>  OK     C4/Circulation.pm
> 
>  WARN   C4/Overdues.pm
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 122, now: 125)
> 
>  WARN   t/db_dependent/Circulation/maxsuspensiondays.t
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 52, now: 77)
> 
> 
> Processing additional checks
> 
>         * Signed-off-by line '    Signed-off-by: Arthur Suzuki
> <arthur.suzuki@biblibre.com>' seems invalid
>         * Signed-off-by line '    Signed-off-by: Mark Tompsett
> <mtompset@hotmail.com>' seems invalid
>         * Signed-off-by line '    Signed-off-by: Mark Tompsett
> <mtompset@hotmail.com>' seems invalid
> 
> Running tests (1)
>         * Proving
> /usr/share/koha/t/db_dependent/Circulation/maxsuspensiondays.t KO!
> DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st
> execute failed: Incorrect datetime value: 'SCALAR(0x5595a6847398)' for
> column `koha_myclone`.`borrowers`.`updated_on` at row 1 at
> /usr/share/koha/Koha/Object.pm line 170
>  Invalid value passed, borrowers.updated_on=SCALAR(0x5595a6847398) expected
> type is datetime # Looks like your test exited with 11 before it could
> output anything.
>  /usr/share/koha/t/db_dependent/Circulation/maxsuspensiondays.t ..
> Dubious, test returned 11 (wstat 2816, 0xb00)
>  Failed 4/4 subtests

Thanks for your feedback, I've rebased it and hope it's ok now for the order and bad commit messages.

For the tests I don't understand because it was and still is okay on my instance.
Comment 53 David Nind 2023-12-03 22:14:09 UTC
Created attachment 159504 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: David Nind <david@davidnind.com>
Comment 54 David Nind 2023-12-03 22:17:24 UTC
Testing notes (using koha-testing-docker):

1. Tests pass for me: prove t/db_dependent/Circulation/maxsuspensiondays.t

2. Use the same test plan as in comment#49

Maybe the code changes made need to be tidied using Perltidy? (https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL1:_Perltidy and https://wiki.koha-community.org/wiki/Perltidy)
Comment 55 Marcel de Rooy 2024-01-12 10:25:51 UTC
Looking here now
Comment 56 Marcel de Rooy 2024-01-12 10:56:15 UTC
Created attachment 160924 [details] [review]
Bug 14293: The suspension should be calculated in days

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Marcel de Rooy 2024-01-12 10:56:18 UTC
Created attachment 160925 [details] [review]
Bug 14293: (QA follow-up) Remove copy and paste test

The comment is in contradiction with the code.
The test has been copied. Not needed to repeat it.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 58 Marcel de Rooy 2024-01-12 10:58:39 UTC
QA Comment:
Code changes look good to me. Fixed test in a follow-up.
The only thing remaining is the discussion about the rounding (changing from floor to ceil).

I will pass QA if we change it back for current behavior.
If we do not want to change it, please provide evidence for community consensus about that change.
You could do so by mailing the lists or putting on the dev meeting agenda, etc.

Putting on ID for now.