Bug 19804

Summary: Suspension calculation doesn't honor 'Suspension charging interval'
Product: Koha Reporter: Hugo Agud <hagud>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, martin.renvoize, niamh.walker-headon, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19204
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 19804: Add new DB column issuingrules.suspension_chargeperiod
Bug 19804: DBIC Schema changes
Bug 19804: Add a 'Fine charging interval' for suspension days
Bug 19804: Add the new column to the circ rules interface
Bug 19804: Add new DB column issuingrules.suspension_chargeperiod
Bug 19804: DBIC Schema changes
Bug 19804: Add a 'Fine charging interval' for suspension days
Bug 19804: Add the new column to the circ rules interface
Bug 19804: Add a 'Fine charging interval' for suspension days
Bug 19804: Add the new column to the circ rules interface
Bug 19804: (follow-up) Fix wording and columns order
Bug 19804: Add new DB column issuingrules.suspension_chargeperiod
Bug 19804: DBIC Schema changes
Bug 19804: Add a 'Fine charging interval' for suspension days
Bug 19804: Add the new column to the circ rules interface
Bug 19804: (follow-up) Fix wording and columns order
Bug 19804: Fix test in GetHardDueDate.t
Bug 19804: Fix test in maxsuspensiondays.t

Description Hugo Agud 2017-12-13 09:15:44 UTC
When a user is returning an item with overudue, the suspension is calculated in days according 'suspensions in day' it doesn't calculate the 'Fine charging interval' that I guess is used for € fines, but doesn't apply to suspensions in day.

Not all libraries has a relation > 1 to days and suspensions, for example now it is not possible to define 7 days of suspension for every 14.

It should be nice if Koha also consider the 'Fine charging interval' for suspensions in days and library could decide to change the suspension, every 1, 2 ... days
Comment 1 Jonathan Druart 2017-12-13 21:06:17 UTC
Created attachment 69783 [details] [review]
Bug 19804: Add new DB column issuingrules.suspension_chargeperiod
Comment 2 Jonathan Druart 2017-12-13 21:06:21 UTC
Created attachment 69784 [details] [review]
Bug 19804: DBIC Schema changes
Comment 3 Jonathan Druart 2017-12-13 21:06:24 UTC
Created attachment 69785 [details] [review]
Bug 19804: Add a 'Fine charging interval' for suspension days

We already have a chargeperiod (Fine charging interval) value which is
taken into account for fine ($) for not for the suspension period.

This patch adds a new column suspension_chargeperiod (Fine day charging
interval) to add the same behaviour when a suspension is calculated.

Test plan:
Add overdue item and play with the circulation rules (and the calendar).
The suspension period must be correctly calculated.
Please provide the different tests you made.
Comment 4 Jonathan Druart 2017-12-13 21:06:28 UTC
Created attachment 69786 [details] [review]
Bug 19804: Add the new column to the circ rules interface
Comment 5 Hugo Agud 2018-01-10 13:24:26 UTC
Good morning

I have insall the patch into kohadev, I have checked out and checked in several items with different configuration and it works.. 

I have created a rule of 1 suspension day for every 2 delay

Case 1
Cumulative on
Calendar without holidays--> works as espected
Calendar with holiday -->  work as expected


Cumulative off
Calendar without holidays --> works as expected
Calendar with holidays -->  works as expected

I missed something to check, if not I will sign it asap
Comment 6 Hugo Agud 2018-01-10 15:07:48 UTC
Created attachment 70404 [details] [review]
Bug 19804: Add new DB column issuingrules.suspension_chargeperiod

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 7 Hugo Agud 2018-01-10 15:07:51 UTC
Created attachment 70405 [details] [review]
Bug 19804: DBIC Schema changes

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 8 Hugo Agud 2018-01-10 15:07:54 UTC
Created attachment 70406 [details] [review]
Bug 19804: Add a 'Fine charging interval' for suspension days

We already have a chargeperiod (Fine charging interval) value which is
taken into account for fine ($) for not for the suspension period.

This patch adds a new column suspension_chargeperiod (Fine day charging
interval) to add the same behaviour when a suspension is calculated.

Test plan:
Add overdue item and play with the circulation rules (and the calendar).
The suspension period must be correctly calculated.
Please provide the different tests you made.

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 9 Hugo Agud 2018-01-10 15:07:57 UTC
Created attachment 70407 [details] [review]
Bug 19804: Add the new column to the circ rules interface

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 10 Tomás Cohen Arazi 2018-02-15 12:04:30 UTC
Jonathan, the atomicupdate should set an initial value for existing rules, based on chargeperiod.
Comment 11 Jonathan Druart 2018-02-15 15:07:12 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> Jonathan, the atomicupdate should set an initial value for existing rules,
> based on chargeperiod.

It's done already, "DEFAULT "1"'
Comment 12 Katrin Fischer 2018-04-04 06:04:39 UTC
Some tiny things:

- number of tests needs to be 114 for them to pass
- the headings at the top and the bottom row of the circulation rules table don't match up
- Maybe rename "Fine day charging interval" to "Suspension charging interval" to match a bit more with the other related columns?
- Adding a see also for bug 19204 - it's obvious here too, that calendar is ignored.

Can you provide a quick follow up?
Comment 13 Jonathan Druart 2018-04-04 20:37:16 UTC
Created attachment 73665 [details] [review]
Bug 19804: Add a 'Fine charging interval' for suspension days

We already have a chargeperiod (Fine charging interval) value which is
taken into account for fine ($) for not for the suspension period.

This patch adds a new column suspension_chargeperiod (Fine day charging
interval) to add the same behaviour when a suspension is calculated.

Test plan:
Add overdue item and play with the circulation rules (and the calendar).
The suspension period must be correctly calculated.
Please provide the different tests you made.

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 14 Jonathan Druart 2018-04-04 20:37:22 UTC
Created attachment 73666 [details] [review]
Bug 19804: Add the new column to the circ rules interface

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 15 Jonathan Druart 2018-04-04 20:37:27 UTC
Created attachment 73667 [details] [review]
Bug 19804: (follow-up) Fix wording and columns order
Comment 16 Jonathan Druart 2018-04-04 20:37:58 UTC
(In reply to Katrin Fischer from comment #12)
> Some tiny things:
> 
> - number of tests needs to be 114 for them to pass

Patch amended.

> - the headings at the top and the bottom row of the circulation rules table
> don't match up
> - Maybe rename "Fine day charging interval" to "Suspension charging
> interval" to match a bit more with the other related columns?

Done, thanks!
Comment 17 Katrin Fischer 2018-04-06 06:33:07 UTC
Hi Joubu,

running out of time for testing this morning, but have 2 questions I want to note:

1) For the fine charging interval we multiply:
Days overdue / fine charging interval * fine

The equivalence would then be:
Days overdue / suspension charging interval * suspension in days

But that's not quite what this does, is it?

2) I have backdated an item on checkout to be immediately overdue, but on return no restriction was created. Can you add a more detailed test plan?
Comment 18 Jonathan Druart 2018-04-06 19:09:46 UTC
(In reply to Katrin Fischer from comment #17)
> Hi Joubu,
> 
> running out of time for testing this morning, but have 2 questions I want to
> note:
> 
> 1) For the fine charging interval we multiply:
> Days overdue / fine charging interval * fine
> 
> The equivalence would then be:
> Days overdue / suspension charging interval * suspension in days
> 
> But that's not quite what this does, is it?

Yes it is what it does, take a look at the tests:
1795     # We want to charge 2 days every day, without grace
1796     # With 5 days of overdue: 5 * Z

(Z is a typo for 2 I guess..)

1814     # We want to charge 2 days every 2 days, without grace
1815     # With 5 days of overdue: (5 * 2) / 2

1836     # We want to charge 2 days every 3 days, with 1 day of grace
1837     # With 5 days of overdue: ((5-1) / 3 ) * 2


> 2) I have backdated an item on checkout to be immediately overdue, but on
> return no restriction was created. Can you add a more detailed test plan?

Today is April 6th, I set:
Suspension in days (day): 3
Suspension charging interval: 2

Which means: charge 3 days every 2 days.

Check an item in with a due date on April 2nd (4 days of overdue)

We are expecting: 3*4/2 = 6 days of suspension

And I get: "Restricted: Patron's account is restricted until 12/04/2018"

\o/
Comment 19 Katrin Fischer 2018-04-07 10:50:22 UTC
It was me, the devbox had the wrong time and I made a mistake in the circulation conditions... *sigh* - But the good news is the patch working :)
Comment 20 Katrin Fischer 2018-04-07 10:51:25 UTC
Created attachment 73830 [details] [review]
Bug 19804: Add new DB column issuingrules.suspension_chargeperiod

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2018-04-07 10:51:28 UTC
Created attachment 73831 [details] [review]
Bug 19804: DBIC Schema changes

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2018-04-07 10:51:32 UTC
Created attachment 73832 [details] [review]
Bug 19804: Add a 'Fine charging interval' for suspension days

We already have a chargeperiod (Fine charging interval) value which is
taken into account for fine ($) for not for the suspension period.

This patch adds a new column suspension_chargeperiod (Fine day charging
interval) to add the same behaviour when a suspension is calculated.

Test plan:
Add overdue item and play with the circulation rules (and the calendar).
The suspension period must be correctly calculated.
Please provide the different tests you made.

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2018-04-07 10:51:36 UTC
Created attachment 73833 [details] [review]
Bug 19804: Add the new column to the circ rules interface

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2018-04-07 10:51:41 UTC
Created attachment 73834 [details] [review]
Bug 19804: (follow-up) Fix wording and columns order

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Jonathan Druart 2018-04-09 19:16:23 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 26 Jonathan Druart 2018-04-09 20:34:17 UTC
Created attachment 73894 [details] [review]
Bug 19804: Fix test in GetHardDueDate.t
Comment 27 Jonathan Druart 2018-04-09 20:40:51 UTC
Created attachment 73895 [details] [review]
Bug 19804: Fix test in maxsuspensiondays.t

We do not want a random value of suspension_chargeperiod from
TestBuilder
Comment 28 Jonathan Druart 2018-04-09 20:45:20 UTC
Last two patches pushed to master.