Bug 21443

Summary: Add ability to exclude holidays when calculating rentals fees by time period
Product: Koha Reporter: Kyle M Hall <kyle>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, gmcharlt, katrin.fischer, kyle.m.hall, mfuerst
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Allows to configure on item type level, if the calendar will be taken into account when calculating hourly or daily rental fees.
Version(s) released in:
20.05.00
Bug Depends on: 20912    
Bug Blocks: 26341    
Attachments: Bug 21443: Update database
Bug 21443: Update Schema [DO NOT PUSH]
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period
Bug 21443: Unit tests
Bug 21443: Update database
Bug 21443: Update database
Bug 21443: Update Schema [DO NOT PUSH]
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period
Bug 21443: Unit tests
Bug 21443: Mark new column as boolean in schema file
Bug 21443: Remove the dependency on finesCalendar
Bug 21443: Mark new columns as boolean in schema file
Bug 21443: Remove the dependency on finesCalendar
Bug 21443: Update database
Bug 21443: Update Schema [DO NOT PUSH]
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period
Bug 21443: Unit tests
Bug 21443: Mark new columns as boolean in schema file
Bug 21443: Remove the dependency on finesCalendar
Bug 21443: Update database
Bug 21443: Update Schema [DO NOT PUSH]
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period
Bug 21443: Unit tests
Bug 21443: Mark new columns as boolean in schema file
Bug 21443: Remove the dependency on finesCalendar
Bug 21443: (RM follow-up) Fix failing tests

Description Kyle M Hall 2018-09-27 16:47:55 UTC
It would be a nice addition if we could have the option to not count holidays when creating time period based rental fees.
Comment 1 Martha Fuerst 2019-08-06 13:53:39 UTC
Agreed with Kyle. It would be nice to include rental fees in holiday exceptions. We like being able to allow this exception for fines, and having to turn it off in order to accomodate rental fees is not ideal for us, since the patron still has the use of the material for the holiday (and should pay for it) vs. the library being closed and accruing a fine.
Comment 2 Katrin Fischer 2019-08-13 05:58:38 UTC
A preference like RentalFeeCalendar maybe? (matching finesCalendar and OverdueNoticeCalendar)
Comment 3 Martha Fuerst 2019-08-13 13:01:49 UTC
That sounds like it would work for us, Katrin!
Comment 4 Kyle M Hall 2020-03-09 17:37:37 UTC
I have discussed this with the sponsor of the original development and we agree that a syspref does not provide fine enough control. Instead, it makes sense to include this control in the itemtype where the daily/hourly fees are also set.
Comment 5 Kyle M Hall 2020-03-09 17:37:58 UTC
Created attachment 100402 [details] [review]
Bug 21443: Update database
Comment 6 Kyle M Hall 2020-03-09 17:38:07 UTC
Created attachment 100403 [details] [review]
Bug 21443: Update Schema [DO NOT PUSH]
Comment 7 Kyle M Hall 2020-03-09 17:38:10 UTC
Created attachment 100404 [details] [review]
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period
Comment 8 Kyle M Hall 2020-03-09 17:38:12 UTC
Created attachment 100405 [details] [review]
Bug 21443: Unit tests
Comment 9 Kyle M Hall 2020-03-09 17:43:27 UTC
Test Plan:
1) Apply this patch set
2) Restart all the things!
3) Set finesCalendar to 'noFinesWhenClosed'
4) Set up daily/hourly rental fees for an itemtype
5) Add a holiday during the rental period
4) Verify the rental fee by checking out an item
5) Uncheck the "Daily/Hourly rental fees use calendar" checkbox
6) Perform another checkout
7) This time, the rental fee should be increased by one day's fee, as it will not longer include the holiday.
Comment 10 Kyle M Hall 2020-03-16 12:57:29 UTC
Created attachment 100744 [details] [review]
Bug 21443: Update database

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Comment 11 Kyle M Hall 2020-03-16 12:59:14 UTC
Created attachment 100745 [details] [review]
Bug 21443: Update database

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Comment 12 Kyle M Hall 2020-03-16 12:59:23 UTC
Created attachment 100746 [details] [review]
Bug 21443: Update Schema [DO NOT PUSH]

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Comment 13 Kyle M Hall 2020-03-16 12:59:26 UTC
Created attachment 100747 [details] [review]
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Comment 14 Kyle M Hall 2020-03-16 12:59:29 UTC
Created attachment 100748 [details] [review]
Bug 21443: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Comment 15 Katrin Fischer 2020-03-22 16:00:27 UTC
Hi Kyle, 

giving this a go:

1) QA Tools fail:

 FAIL	installer/data/mysql/kohastructure.sql
   FAIL	  tinyint_has_boolean_flag
		The new column itemtypes.rentalcharge_daily_calendar is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Itemtype.pm), see the SQL12 coding guideline
		The new column itemtypes.rentalcharge_hourly_calendar is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Itemtype.pm), see the SQL12 coding guideline


2) Usability

I am a bit worried about usability:

Say: finescalender is set to ignore holidays.
And on the itemtype I check:
Daily rentals use calendar

But, if I understand correctly, they won't - because it falls back to finescalendar which is set to ignore holidays. 

This seems potentially confusing to me, as you can't see what finescalendar is set to and you have to read the 'fine print' (hint) and look it up to determine the actual behaviour.

Instead, maybe we could label it:
Use finescalendar setting. If not checked, holidays will be excluded when calculating rental charges. (please feel free to rephrase!)

We could also include the value of finescalendar somewhere in the template maybe?

Alternatively... should we remove the dependeny on finescalendar and make just the itemtype setting relevant?

then we could just have:
- ignore calendar
- exclude holidays

And it would be immediately visible from the item type configuration page, what rule will apply.
Comment 16 Kyle M Hall 2020-03-23 12:09:16 UTC
Created attachment 101428 [details] [review]
Bug 21443: Mark new column as boolean in schema file

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2020-03-23 12:09:45 UTC
Created attachment 101429 [details] [review]
Bug 21443: Remove the dependency on finesCalendar

Considering that the the use of finesCalendar for this calculation is
already a binary choice, it makes sense to remove the use of
finesCalendar here. It is an uneccessary complication that could
introduce confusion. Allowing this new setting to directly control
the behavior makes it clear and obvious what is going on.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2020-03-23 12:11:52 UTC
Created attachment 101430 [details] [review]
Bug 21443: Mark new columns as boolean in schema file

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2020-03-23 12:12:02 UTC
Created attachment 101431 [details] [review]
Bug 21443: Remove the dependency on finesCalendar

Considering that the the use of finesCalendar for this calculation is
already a binary choice, it makes sense to remove the use of
finesCalendar here. It is an uneccessary complication that could
introduce confusion. Allowing this new setting to directly control
the behavior makes it clear and obvious what is going on.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Katrin Fischer 2020-03-23 15:22:35 UTC
Created attachment 101458 [details] [review]
Bug 21443: Update database

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Comment 21 Katrin Fischer 2020-03-23 15:22:39 UTC
Created attachment 101459 [details] [review]
Bug 21443: Update Schema [DO NOT PUSH]

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Comment 22 Katrin Fischer 2020-03-23 15:22:44 UTC
Created attachment 101460 [details] [review]
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Comment 23 Katrin Fischer 2020-03-23 15:22:48 UTC
Created attachment 101461 [details] [review]
Bug 21443: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Comment 24 Katrin Fischer 2020-03-23 15:22:53 UTC
Created attachment 101462 [details] [review]
Bug 21443: Mark new columns as boolean in schema file

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Katrin Fischer 2020-03-23 15:22:56 UTC
Created attachment 101463 [details] [review]
Bug 21443: Remove the dependency on finesCalendar

Considering that the the use of finesCalendar for this calculation is
already a binary choice, it makes sense to remove the use of
finesCalendar here. It is an uneccessary complication that could
introduce confusion. Allowing this new setting to directly control
the behavior makes it clear and obvious what is going on.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Katrin Fischer 2020-03-23 15:23:38 UTC
Created attachment 101464 [details] [review]
Bug 21443: Update database

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2020-03-23 15:23:42 UTC
Created attachment 101465 [details] [review]
Bug 21443: Update Schema [DO NOT PUSH]

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2020-03-23 15:23:45 UTC
Created attachment 101466 [details] [review]
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2020-03-23 15:23:48 UTC
Created attachment 101467 [details] [review]
Bug 21443: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2020-03-23 15:23:52 UTC
Created attachment 101468 [details] [review]
Bug 21443: Mark new columns as boolean in schema file

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2020-03-23 15:23:55 UTC
Created attachment 101469 [details] [review]
Bug 21443: Remove the dependency on finesCalendar

Considering that the the use of finesCalendar for this calculation is
already a binary choice, it makes sense to remove the use of
finesCalendar here. It is an uneccessary complication that could
introduce confusion. Allowing this new setting to directly control
the behavior makes it clear and obvious what is going on.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Martin Renvoize 2020-03-24 11:33:39 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 33 Martin Renvoize 2020-03-24 16:28:31 UTC
Created attachment 101631 [details] [review]
Bug 21443: (RM follow-up) Fix failing tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2020-03-24 16:29:32 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 35 Joy Nelson 2020-04-03 21:46:49 UTC
enhancement not backported to 19.11.x