Description
Kyle M Hall (khall)
2018-09-27 16:47:55 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. A preference like RentalFeeCalendar maybe? (matching finesCalendar and OverdueNoticeCalendar) That sounds like it would work for us, Katrin! 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. Created attachment 100402 [details] [review] Bug 21443: Update database Created attachment 100403 [details] [review] Bug 21443: Update Schema [DO NOT PUSH] Created attachment 100404 [details] [review] Bug 21443: Add ability to exclude holidays when calculating rentals fees by time period Created attachment 100405 [details] [review] Bug 21443: Unit tests 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. 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> 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> 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> 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> 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> 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. Created attachment 101428 [details] [review] Bug 21443: Mark new column as boolean in schema file Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> Created attachment 101430 [details] [review] Bug 21443: Mark new columns as boolean in schema file Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> 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> 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> 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> 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> Created attachment 101462 [details] [review] Bug 21443: Mark new columns as boolean in schema file Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> 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> 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> 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> 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> 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> 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> Nice work everyone! Pushed to master for 20.05 Created attachment 101631 [details] [review] Bug 21443: (RM follow-up) Fix failing tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice work everyone! Pushed to master for 20.05 enhancement not backported to 19.11.x |