Bug 19204

Summary: Fines in days restriction calculation is not taking calendar into account
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: christian.stelzenmueller, gaetan.boisson, gmcharlt, jonathan.druart, kyle.m.hall, martin.renvoize, niamh.walker-headon, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19804
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 13958    
Attachments: Bug 19204: Make the debarment date calculation depends on finesCalendar
Bug 19204: Clean the tests a bit to reuse the same pattern
Bug 19204: Add a test: holiday is expiration date
Bug 19204: Add tests for new days_mode parameter
Bug 19204: Make the debarment date calculation depends on finesCalendar
Bug 19204: Clean the tests a bit to reuse the same pattern
Bug 19204: Add a test: holiday is expiration date
Bug 19204: Add tests for new days_mode parameter
Bug 19204: Make the debarment date calculation depends on finesCalendar
Bug 19204: Clean the tests a bit to reuse the same pattern
Bug 19204: Add a test: holiday is expiration date
Bug 19204: Add tests for new days_mode parameter
Bug 19204: Make the debarment date calculation depends on finesCalendar
Bug 19204: Clean the tests a bit to reuse the same pattern
Bug 19204: Add a test: holiday is expiration date
Bug 19204: Add tests for new days_mode parameter

Description Katrin Fischer 2017-08-30 09:05:52 UTC
When using the fines in days feature, the restrictions will be calculated without taking holidays into account. So the restriction can end in the middle of a period the library is closed.

To test:
- Set finescalendar to 'not including days the library is closed'
- Set a circulation condition with no fine/maxfine, but fine days and max fine days instead
- Check out an item with a due date in the past
- Check the item in and verify the restriction date
- Clean the restriction
- Add holidays to your calendar on the calculated restriction date
- Check the item out again with the same due date in the past
- Check in the item again
- Verify the calculated restriction end date now is the same and a holiday.
Comment 1 Gaetan Boisson 2018-03-23 16:07:20 UTC
I guess this is a good thing. After discussing this with others we would suggest to make it work with useDaysMode, to be consistent with the library policy for computing return dates.
Comment 2 Katrin Fischer 2018-04-01 12:11:14 UTC
useDaysMode is used for calculating the due dates and usually not involved in the overdues process. I am not sure it's the right one. 

Maybe finesCalendar would be a better fit as it's already used to count the days overdue? So it would make sense to use it for determining the suspension date as well. With an update to the description. We could also have a new preference SuspensionCalendar mimicking the behaviour.
Comment 3 Katrin Fischer 2018-04-01 12:14:35 UTC
Actually looking at Bug 13909 - Suspension days calculation doesn't honour finesCalendar I think finesCalendar would be best. And update the description to say: Calculate fines and suspensions based on days overdue  ...
Comment 4 Tomás Cohen Arazi 2018-04-09 14:50:28 UTC
What is the status for this? Is anyone going to work on this for 18.05?
Comment 5 Katrin Fischer 2018-04-09 15:07:25 UTC
We are looking into it.
Comment 6 Jonathan Druart 2018-04-23 13:29:14 UTC
Created attachment 74726 [details] [review]
Bug 19204: Make the debarment date calculation depends on finesCalendar

This patchs adds the ability to calculate the end of the suspension date
(debarment date) using the finesCalendar syspref.
Prior to this patch it was never calculating without taking into account
the calendar.

calculated without taking holidays into account.
This was a problem because the restriction could end in the middle of a
period the library is closed.

Test plan:
- Set finescalendar to 'not including days the library is closed'
- Set a circulation condition with no fine/maxfine, but fine days and
max fine days instead
- Check out an item with a due date in the past
- Check the item in and verify the restriction date
- Clean the restriction

- Add holidays to your calendar on the calculated restriction date
- Check the item out again with the same due date in the past
- Check in the item again
- Verify the calculated restriction end date has changed, it's set to the day
after the holiday.

Fines in days restriction calculation is correctly taking calendar
into account.
Comment 7 Jonathan Druart 2018-04-23 13:29:17 UTC
Created attachment 74727 [details] [review]
Bug 19204: Clean the tests a bit to reuse the same pattern

The same pattern is used several times in test:
checkout, checkin, get the debarment, compare the dates and remove the
debarment.
Let's move that to a separate subroutine
Comment 8 Jonathan Druart 2018-04-23 13:29:21 UTC
Created attachment 74728 [details] [review]
Bug 19204: Add a test: holiday is expiration date
Comment 9 Jonathan Druart 2018-04-23 13:38:04 UTC
Created attachment 74730 [details] [review]
Bug 19204: Add tests for new days_mode parameter
Comment 10 Claire Gravely 2018-04-23 14:38:58 UTC
Created attachment 74733 [details] [review]
Bug 19204: Make the debarment date calculation depends on finesCalendar

This patchs adds the ability to calculate the end of the suspension date
(debarment date) using the finesCalendar syspref.
Prior to this patch it was never calculating without taking into account
the calendar.

calculated without taking holidays into account.
This was a problem because the restriction could end in the middle of a
period the library is closed.

Test plan:
- Set finescalendar to 'not including days the library is closed'
- Set a circulation condition with no fine/maxfine, but fine days and
max fine days instead
- Check out an item with a due date in the past
- Check the item in and verify the restriction date
- Clean the restriction

- Add holidays to your calendar on the calculated restriction date
- Check the item out again with the same due date in the past
- Check in the item again
- Verify the calculated restriction end date has changed, it's set to the day
after the holiday.

Fines in days restriction calculation is correctly taking calendar
into account.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 11 Claire Gravely 2018-04-23 14:39:01 UTC
Created attachment 74734 [details] [review]
Bug 19204: Clean the tests a bit to reuse the same pattern

The same pattern is used several times in test:
checkout, checkin, get the debarment, compare the dates and remove the
debarment.
Let's move that to a separate subroutine

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 12 Claire Gravely 2018-04-23 14:39:05 UTC
Created attachment 74735 [details] [review]
Bug 19204: Add a test: holiday is expiration date

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 13 Claire Gravely 2018-04-23 14:39:09 UTC
Created attachment 74736 [details] [review]
Bug 19204: Add tests for new days_mode parameter

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 14 Katrin Fischer 2018-04-24 06:39:00 UTC
Created attachment 74777 [details] [review]
Bug 19204: Make the debarment date calculation depends on finesCalendar

This patchs adds the ability to calculate the end of the suspension date
(debarment date) using the finesCalendar syspref.
Prior to this patch it was never calculating without taking into account
the calendar.

calculated without taking holidays into account.
This was a problem because the restriction could end in the middle of a
period the library is closed.

Test plan:
- Set finescalendar to 'not including days the library is closed'
- Set a circulation condition with no fine/maxfine, but fine days and
max fine days instead
- Check out an item with a due date in the past
- Check the item in and verify the restriction date
- Clean the restriction

- Add holidays to your calendar on the calculated restriction date
- Check the item out again with the same due date in the past
- Check in the item again
- Verify the calculated restriction end date has changed, it's set to the day
after the holiday.

Fines in days restriction calculation is correctly taking calendar
into account.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2018-04-24 06:39:04 UTC
Created attachment 74778 [details] [review]
Bug 19204: Clean the tests a bit to reuse the same pattern

The same pattern is used several times in test:
checkout, checkin, get the debarment, compare the dates and remove the
debarment.
Let's move that to a separate subroutine

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2018-04-24 06:39:09 UTC
Created attachment 74779 [details] [review]
Bug 19204: Add a test: holiday is expiration date

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2018-04-24 06:39:13 UTC
Created attachment 74780 [details] [review]
Bug 19204: Add tests for new days_mode parameter

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2018-04-24 06:40:04 UTC
Won't QA this (conflict of interest) - can someone else please take a look?
Comment 19 Tomás Cohen Arazi 2018-04-24 12:41:36 UTC
Created attachment 74798 [details] [review]
Bug 19204: Make the debarment date calculation depends on finesCalendar

This patchs adds the ability to calculate the end of the suspension date
(debarment date) using the finesCalendar syspref.
Prior to this patch it was never calculating without taking into account
the calendar.

calculated without taking holidays into account.
This was a problem because the restriction could end in the middle of a
period the library is closed.

Test plan:
- Set finescalendar to 'not including days the library is closed'
- Set a circulation condition with no fine/maxfine, but fine days and
max fine days instead
- Check out an item with a due date in the past
- Check the item in and verify the restriction date
- Clean the restriction

- Add holidays to your calendar on the calculated restriction date
- Check the item out again with the same due date in the past
- Check in the item again
- Verify the calculated restriction end date has changed, it's set to the day
after the holiday.

Fines in days restriction calculation is correctly taking calendar
into account.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2018-04-24 12:41:56 UTC
Created attachment 74799 [details] [review]
Bug 19204: Clean the tests a bit to reuse the same pattern

The same pattern is used several times in test:
checkout, checkin, get the debarment, compare the dates and remove the
debarment.
Let's move that to a separate subroutine

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2018-04-24 12:42:03 UTC
Created attachment 74800 [details] [review]
Bug 19204: Add a test: holiday is expiration date

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2018-04-24 12:42:15 UTC
Created attachment 74801 [details] [review]
Bug 19204: Add tests for new days_mode parameter

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Katrin Fischer 2018-04-25 08:09:12 UTC
Hi Jonathan, can you please add a Sponsored-by line before pushing? This was sponsored by the Goethe-Intsitut.
Comment 24 Katrin Fischer 2018-04-25 09:09:14 UTC
Without typo: Goethe-Institut
Comment 25 Jonathan Druart 2018-04-25 14:50:52 UTC
Pushed to master for 18.05, thanks to everybody involved!