Bug 19804 - Suspension calculation doesn't honor 'Suspension charging interval'
Summary: Suspension calculation doesn't honor 'Suspension charging interval'
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-13 09:15 UTC by Hugo Agud
Modified: 2018-04-12 11:59 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19804: Add new DB column issuingrules.suspension_chargeperiod (1.97 KB, patch)
2017-12-13 21:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19804: DBIC Schema changes (1.61 KB, patch)
2017-12-13 21:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19804: Add a 'Fine charging interval' for suspension days (6.38 KB, patch)
2017-12-13 21:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19804: Add the new column to the circ rules interface (3.85 KB, patch)
2017-12-13 21:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19804: Add new DB column issuingrules.suspension_chargeperiod (2.00 KB, patch)
2018-01-10 15:07 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 19804: DBIC Schema changes (1.64 KB, patch)
2018-01-10 15:07 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 19804: Add a 'Fine charging interval' for suspension days (6.37 KB, patch)
2018-01-10 15:07 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 19804: Add the new column to the circ rules interface (3.75 KB, patch)
2018-01-10 15:07 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 19804: Add a 'Fine charging interval' for suspension days (6.42 KB, patch)
2018-04-04 20:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19804: Add the new column to the circ rules interface (3.76 KB, patch)
2018-04-04 20:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19804: (follow-up) Fix wording and columns order (1.61 KB, patch)
2018-04-04 20:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19804: Add new DB column issuingrules.suspension_chargeperiod (2.05 KB, patch)
2018-04-07 10:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19804: DBIC Schema changes (1.70 KB, patch)
2018-04-07 10:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19804: Add a 'Fine charging interval' for suspension days (6.46 KB, patch)
2018-04-07 10:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19804: Add the new column to the circ rules interface (3.81 KB, patch)
2018-04-07 10:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19804: (follow-up) Fix wording and columns order (1.66 KB, patch)
2018-04-07 10:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19804: Fix test in GetHardDueDate.t (857 bytes, patch)
2018-04-09 20:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19804: Fix test in maxsuspensiondays.t (925 bytes, patch)
2018-04-09 20:40 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.