Bug 21443 - Add ability to exclude holidays when calculating rentals fees by time period
Summary: Add ability to exclude holidays when calculating rentals fees by time period
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: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 20912
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-27 16:47 UTC by Kyle M Hall
Modified: 2020-05-16 14:01 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
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


Attachments
Bug 21443: Update database (2.81 KB, patch)
2020-03-09 17:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Update Schema [DO NOT PUSH] (1.97 KB, patch)
2020-03-09 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period (6.05 KB, patch)
2020-03-09 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Unit tests (2.91 KB, patch)
2020-03-09 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Update database (2.91 KB, patch)
2020-03-16 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Update database (2.91 KB, patch)
2020-03-16 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Update Schema [DO NOT PUSH] (2.07 KB, patch)
2020-03-16 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period (6.18 KB, patch)
2020-03-16 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Unit tests (3.01 KB, patch)
2020-03-16 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Mark new column as boolean in schema file (971 bytes, patch)
2020-03-23 12:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Remove the dependency on finesCalendar (6.19 KB, patch)
2020-03-23 12:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Mark new columns as boolean in schema file (1.01 KB, patch)
2020-03-23 12:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Remove the dependency on finesCalendar (6.19 KB, patch)
2020-03-23 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21443: Update database (2.90 KB, patch)
2020-03-23 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Update Schema [DO NOT PUSH] (2.05 KB, patch)
2020-03-23 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period (6.05 KB, patch)
2020-03-23 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Unit tests (2.99 KB, patch)
2020-03-23 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Mark new columns as boolean in schema file (1016 bytes, patch)
2020-03-23 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Remove the dependency on finesCalendar (6.17 KB, patch)
2020-03-23 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Update database (2.96 KB, patch)
2020-03-23 15:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Update Schema [DO NOT PUSH] (2.11 KB, patch)
2020-03-23 15:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period (6.10 KB, patch)
2020-03-23 15:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Unit tests (3.05 KB, patch)
2020-03-23 15:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Mark new columns as boolean in schema file (1.05 KB, patch)
2020-03-23 15:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: Remove the dependency on finesCalendar (6.23 KB, patch)
2020-03-23 15:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21443: (RM follow-up) Fix failing tests (7.60 KB, patch)
2020-03-24 16:28 UTC, Martin Renvoize
Details | Diff | Splinter Review

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