Bug 24159 - Allow daysMode for calculating due and renewal dates to be set at the circulation rules level
Summary: Allow daysMode for calculating due and renewal dates to be set at the circula...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 18936
Blocks: 15523 31235 25896 27026
  Show dependency treegraph
 
Reported: 2019-12-03 19:37 UTC by Jonathan Druart
Modified: 2022-08-01 14:41 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This new enhancement is adding the ability to make hourly loan returned on a closed day, if checked out on the same close day. The useDaysMode system preference has been moved to a circulation rule to add more flexibility in the calculation of the due date.
Version(s) released in:
20.11.00


Attachments
Bug 24159: Move useDaysMode pref to circulation rules (6.07 KB, patch)
2020-02-28 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Throw an exception if days_mode option is not given when needed (4.20 KB, patch)
2020-02-28 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Add UI changes (8.77 KB, patch)
2020-02-28 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Adjust tests (2.84 KB, patch)
2020-02-28 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Add new tests for get_useDaysMode_effective_value (3.84 KB, patch)
2020-02-28 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Move useDaysMode pref to circulation rules (6.07 KB, patch)
2020-03-12 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Throw an exception if days_mode option is not given when needed (4.20 KB, patch)
2020-03-12 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Add UI changes (8.77 KB, patch)
2020-03-12 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Adjust tests (2.84 KB, patch)
2020-03-12 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Add new tests for get_useDaysMode_effective_value (3.84 KB, patch)
2020-03-12 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Set days_mode according to circ rules in 3 other places (5.24 KB, patch)
2020-03-12 08:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Use days_mode circ rule to calculate the dropbox date (1.39 KB, patch)
2020-03-12 08:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Fix one occurrence in updatedatabase.pl (1.24 KB, patch)
2020-03-12 08:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Test (25.69 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2020-03-19 17:06 UTC, Paul Thornton
Details
Test (25.69 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2020-03-19 17:07 UTC, Paul Thornton
Details
Test 24/3/20 (186.41 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2020-03-24 12:38 UTC, Simon Perry
Details
Bug 24159: Fix shift in the header (961 bytes, patch)
2020-03-24 17:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Test 25/3/20 (102.88 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2020-03-25 12:21 UTC, Simon Perry
Details
testing 270320 (87.67 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2020-03-27 15:35 UTC, Simon Perry
Details
Bug 24159 test 31 March (38.08 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2020-03-31 15:57 UTC, Simon Perry
Details
Sandbox error (24.01 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2020-04-02 15:31 UTC, Simon Perry
Details
Final patch testing (155.39 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2020-04-08 11:19 UTC, Simon Perry
Details
Bug 24159: Move useDaysMode pref to circulation rules (6.12 KB, patch)
2020-04-08 11:47 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24159: Throw an exception if days_mode option is not given when needed (4.25 KB, patch)
2020-04-08 11:47 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24159: Add UI changes (8.82 KB, patch)
2020-04-08 11:47 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24159: Adjust tests (2.89 KB, patch)
2020-04-08 11:47 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24159: Add new tests for get_useDaysMode_effective_value (3.90 KB, patch)
2020-04-08 11:47 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24159: Set days_mode according to circ rules in 3 other places (5.30 KB, patch)
2020-04-08 11:47 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24159: Use days_mode circ rule to calculate the dropbox date (1.44 KB, patch)
2020-04-08 11:48 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24159: Fix one occurrence in updatedatabase.pl (1.29 KB, patch)
2020-04-08 11:48 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24159: Fix shift in the header (1014 bytes, patch)
2020-04-08 11:48 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24159: Set days_mode according to circ rules in 3 other places (5.21 KB, patch)
2020-05-02 14:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Use days_mode circ rule to calculate the dropbox date (1.43 KB, patch)
2020-05-02 14:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Fix one occurrence in updatedatabase.pl (1.24 KB, patch)
2020-05-02 14:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Fix shift in the header (961 bytes, patch)
2020-05-02 14:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Set days_mode according to circ rules in 3 other places (5.26 KB, patch)
2020-05-02 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Use days_mode circ rule to calculate the dropbox date (1.48 KB, patch)
2020-05-02 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Fix one occurrence in updatedatabase.pl (1.29 KB, patch)
2020-05-02 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Fix shift in the header (1014 bytes, patch)
2020-05-02 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24159: Move useDaysMode pref to circulation rules (6.18 KB, patch)
2020-06-18 12:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Throw an exception if days_mode option is not given when needed (4.31 KB, patch)
2020-06-18 12:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Move useDaysMode pref to circulation rules (6.18 KB, patch)
2020-06-18 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Throw an exception if days_mode option is not given when needed (4.31 KB, patch)
2020-06-18 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Add UI changes (8.88 KB, patch)
2020-06-18 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Adjust tests (2.96 KB, patch)
2020-06-18 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Add new tests for get_useDaysMode_effective_value (3.96 KB, patch)
2020-06-18 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Set days_mode according to circ rules in 3 other places (5.32 KB, patch)
2020-06-18 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Use days_mode circ rule to calculate the dropbox date (1.55 KB, patch)
2020-06-18 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Fix one occurrence in updatedatabase.pl (1.35 KB, patch)
2020-06-18 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: Fix shift in the header (1.05 KB, patch)
2020-06-18 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24159: (QA follow-up) Make terminology consistent (18.39 KB, patch)
2020-06-18 14:56 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 Jonathan Druart 2019-12-03 19:37:19 UTC
Scenario:
 * The library has a 4wk loan item type and a 1hr loan item type.
 * The library would like 1hr loan item types to be issued and returned on a day marked as closed in the circ calendar, but only allow 4wk loan types to be returned on an open day.

Currently, if a 1hr loan is issued on a closed day, the due date is pushed back to the next open day, overriding the duration of the loan.

If the usedaysmode syspref is set to "Circulation rules only" due dates for 1hr loan items are calculated at 1hr later as defined in the circ rules, however 4wk loan due dates can potentially be set to closed days.
Comment 1 Jonathan Druart 2020-02-28 14:14:25 UTC
Created attachment 99758 [details] [review]
Bug 24159: Move useDaysMode pref to circulation rules

Moving the useDaysMode system preference to a circulation rule will add
much more flexibility in the calculation of the due date.

The initial request was to make hourly loan returned on closed when
(when checked out on the same close day).
To do so we do not want to take into account the calendar.
However the calendar need to be taken into account for other loan item types.

Other scenarios are possible, for instance depending on the branch.

This patchset will add a new "Days mode" column (next to "Loan period")
to the circulation rules page, with the different values of the
"useDaysMode" system preference + a "default" value, to default to the
system preference value.

Test plan:
- Define a long loan item type (like 10 days) that will use the calendar
(or default to the pref value, if the pref is not set to "ignore the
calendar")
- and a hourly loan (like 2 hours) that will ignore the calendar
- Create items with those item types
- Mark today as a closed day
- Check the items out
=> The hourly loan is due the same day
=> The other loan is due on an open day

QA note:
There is the need to force the "days_mode" option when Koha::Calendar is
initiated for the due date calculation. To make sure devs will not
forget it, the methods that need have it defined will throw an
exception.

Sponsored-by: Institute of Technology Carlow
Comment 2 Jonathan Druart 2020-02-28 14:14:29 UTC
Created attachment 99759 [details] [review]
Bug 24159: Throw an exception if days_mode option is not given when needed

Sponsored-by: Institute of Technology Carlow
Comment 3 Jonathan Druart 2020-02-28 14:14:32 UTC
Created attachment 99760 [details] [review]
Bug 24159: Add UI changes

Sponsored-by: Institute of Technology Carlow
Comment 4 Jonathan Druart 2020-02-28 14:14:35 UTC
Created attachment 99761 [details] [review]
Bug 24159: Adjust tests

Sponsored-by: Institute of Technology Carlow
Comment 5 Jonathan Druart 2020-02-28 14:14:38 UTC
Created attachment 99762 [details] [review]
Bug 24159: Add new tests for get_useDaysMode_effective_value

Sponsored-by: Institute of Technology Carlow
Comment 6 Jonathan Druart 2020-03-12 08:09:24 UTC
Created attachment 100608 [details] [review]
Bug 24159: Move useDaysMode pref to circulation rules

Moving the useDaysMode system preference to a circulation rule will add
much more flexibility in the calculation of the due date.

The initial request was to make hourly loan returned on closed when
(when checked out on the same close day).
To do so we do not want to take into account the calendar.
However the calendar need to be taken into account for other loan item types.

Other scenarios are possible, for instance depending on the branch.

This patchset will add a new "Days mode" column (next to "Loan period")
to the circulation rules page, with the different values of the
"useDaysMode" system preference + a "default" value, to default to the
system preference value.

Test plan:
- Define a long loan item type (like 10 days) that will use the calendar
(or default to the pref value, if the pref is not set to "ignore the
calendar")
- and a hourly loan (like 2 hours) that will ignore the calendar
- Create items with those item types
- Mark today as a closed day
- Check the items out
=> The hourly loan is due the same day
=> The other loan is due on an open day

QA note:
There is the need to force the "days_mode" option when Koha::Calendar is
initiated for the due date calculation. To make sure devs will not
forget it, the methods that need have it defined will throw an
exception.

Sponsored-by: Institute of Technology Carlow
Comment 7 Jonathan Druart 2020-03-12 08:09:31 UTC
Created attachment 100609 [details] [review]
Bug 24159: Throw an exception if days_mode option is not given when needed

Sponsored-by: Institute of Technology Carlow
Comment 8 Jonathan Druart 2020-03-12 08:09:39 UTC
Created attachment 100610 [details] [review]
Bug 24159: Add UI changes

Sponsored-by: Institute of Technology Carlow
Comment 9 Jonathan Druart 2020-03-12 08:09:47 UTC
Created attachment 100611 [details] [review]
Bug 24159: Adjust tests

Sponsored-by: Institute of Technology Carlow
Comment 10 Jonathan Druart 2020-03-12 08:09:57 UTC
Created attachment 100612 [details] [review]
Bug 24159: Add new tests for get_useDaysMode_effective_value

Sponsored-by: Institute of Technology Carlow
Comment 11 Jonathan Druart 2020-03-12 08:10:06 UTC
Created attachment 100613 [details] [review]
Bug 24159: Set days_mode according to circ rules in 3 other places

There are 3 other occurrences where the new circ rule can be used:
 * C4::Circulation::checkHighHolds
 * Koha::Hold->set_waiting
 * misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl

Test plan:
* checkHighHolds
Enable decreaseLoanHighHolds and fill decreaseLoanHighHoldsDuration
Setup things to hit a "high demand" alert with a shortened due date
Check an item out
=> The due date must be recalculated depending on the circ rule useDaysMode.

* set_waiting
Set ExcludeHolidaysFromMaxPickUpDelay to "1" (note that there is currently
a bug in the description of the syspref, see bug 22381 comment 19)
Mark a hold waiting
The expiration date should have been set depending on the value of the
circ rule.

* TalkingTech cronjob
Cannot test this
Comment 12 Jonathan Druart 2020-03-12 08:10:15 UTC
Created attachment 100614 [details] [review]
Bug 24159: Use days_mode circ rule to calculate the dropbox date

Calculate the drop box date depending on the circ rule. Note that only
the branchcode is taken into account here. Is that correct?

Test plan:
Use the drop box date feature and confirm that it takes into account the
calendar, depending on the circ rule.
Comment 13 Jonathan Druart 2020-03-12 08:10:26 UTC
Created attachment 100615 [details] [review]
Bug 24159: Fix one occurrence in updatedatabase.pl

Here we just want to get the value from the syspref, the circ rule does
not exist yet.
Comment 14 Paul Thornton 2020-03-19 17:06:40 UTC
Created attachment 101053 [details]
Test
Comment 15 Paul Thornton 2020-03-19 17:07:11 UTC
Created attachment 101054 [details]
Test
Comment 16 Paul Thornton 2020-03-19 17:08:39 UTC
Hello Jonathan
I'm coordinating testing with Simon Perry, and he raised the following issue:

"I’ve hit problems with this bug. I created a test bibliographic record with two items records attached (one as an hourly loan and one as a long loan – see attachment for details), set yesterday as a closed day in the calendar and issued both items to a patron. I hadn’t changed the UseDaysMode syspref, so I wanted to check the items in, change the syspref and then reissue. I’ve found that I can’t check either item in and am getting a string of error messages when I try to check in (attached). Note that I deleted the closed day entry in the calendar for yesterday to see if it would make a difference to checkin – it didn’t work.
Do you want to investigate or create a new sandbox so that I can retest?"
Comment 17 Jonathan Druart 2020-03-19 18:01:29 UTC
Hi Paul,

Can you confirm you were testing with all the patches applied?

Mark reported me this error by email, but that should have been fixed by the patches I attached here on March 12th.
Comment 18 Paul Thornton 2020-03-23 14:58:35 UTC
Hello Jonathan
I touched base with Simon and he says the best thing would be if PTFS can re-send the link to the sandbox for this bug and he can retest. Can you organise that please?
Comment 19 Simon Perry 2020-03-24 12:38:00 UTC
Created attachment 101556 [details]
Test 24/3/20

Attached is testing completed today. I tested all 4 options available in the useDaysMode syspref. Only for the Ignore and Use calendar to push due date to next pen matching weekday.... syspref settings did the checkout due date/hour remain on the days of checkout. This wouldn't work for our live system settings as noted.
Regards, Simon
Comment 20 Jonathan Druart 2020-03-24 17:13:04 UTC
Hello Simon,

This development moves the system preference to the circulation rules, for more granularity.

On the circulation rules you should see a new column "Days mode" with 5 different options.

In your case, I think what you need is:
Set the syspref to "Push the due date to next open day"
Define a circulation rule for Hourly loan and select "Ignore the Calendar" in the "Days mode" column.

Note that there is a small cosmetic bug on the circulation table, the header of the table says "Unit" instead of "Days mode". The footer is correct but there is a shift in the header.
Comment 21 Jonathan Druart 2020-03-24 17:13:59 UTC
Created attachment 101649 [details] [review]
Bug 24159: Fix shift in the header
Comment 22 Simon Perry 2020-03-25 12:21:16 UTC
Created attachment 101698 [details]
Test 25/3/20

Hi Jonathan, thanks for clarification. I just retested with today set as closed day in Calendar, Days mode in hourly circulation rule set to 'Calendar' (the dropdown menu for the Days Mode only says 'Calendar' not 'Ignore the Calendar' btw) and useDaysMode syspref set to 'Use the Calendar to push the due date to next due day.' When I checked out an hourly loan item, the due date was moved on to tomorrow, not retained as today's. Please see screenshot attachment for details. Do you want me to test with other options set for useDaysMode etc.?
Simon
Comment 23 Jonathan Druart 2020-03-27 07:11:13 UTC
Simon, I think I understand where the confusion comes from.

The values in the circulation rules have the following meaning:

Days:     Ignore the calendar
Calendar: Use the calendar to skip days the library is closed
Datedue:  Use the calendar to push the due date to the next open day
Dayweek:  Use the calendar to push the due date to the next open matching weekday for weekly loan periods, or the next open day otherwise

They are the "internal values" of the syspref.
It's not ideal but we cannot use the long descriptions in the dropdown list in the circ rules table, that will be too wide.

It seems that it is not enough but to avoid the confusion there is a tooltip that is displayed (over the cursor on the cell): https://snipboard.io/We6flu.jpg it cannot be seen on the screenshot, but the cursor is on "Days".

If you have anything else to suggest to make it less confusing, let me know!
Comment 24 Simon Perry 2020-03-27 11:13:28 UTC
Thanks for clarification Jonathan, from your comments it looks like I should be selecting the 'Days' (ignore calendar) for the hourly loan. Am I right in assuming that the DaysMode column in the Circulation rules table will override what is set in the useDaysMode syspref? And therefore if the Days Mode column in the Circ. rules table is set to 'Default' it will use whatever value is set for the useDaysMode syspref?

Tooltip is useful and I guess the other thing is to ensure it is well documented in the Koha circulation help. I'll retest and feedback results.
Regards, Simon
Comment 25 Jonathan Druart 2020-03-27 11:44:43 UTC
(In reply to Simon Perry from comment #24)
> Thanks for clarification Jonathan, from your comments it looks like I should
> be selecting the 'Days' (ignore calendar) for the hourly loan. Am I right in
> assuming that the DaysMode column in the Circulation rules table will
> override what is set in the useDaysMode syspref? And therefore if the Days
> Mode column in the Circ. rules table is set to 'Default' it will use
> whatever value is set for the useDaysMode syspref?

Yes, that's exactly how it is supposed to work!
Comment 26 Simon Perry 2020-03-27 15:35:25 UTC
Created attachment 102009 [details]
testing 270320

Hi Jonathan,
I've now tested with UsedaysMode set to 'Push due date to next open day' and Days Mode set to 'Days' for the hourly loan rule. Checkout for an hourly loan item type with today set as a library closed day correctly retained today as the due date with an hour's loan allowed. See screenshots of test. I'd like to do some more testing with different useDaysMode syspref values before signing off. Unfortunately the sandbox has got very slow for some reason this afternoon (in fact I cannot connect to it right now). Maybe it could do with a refresh? I'll check again on Monday.
Regards, Simon
Comment 27 Simon Perry 2020-03-31 15:57:33 UTC
Created attachment 102148 [details]
Bug 24159 test 31 March

Hi Jonathan,

Just started testing again. I had the sandbox set to the following configuration:

(1)	useDaysMode syspref –  Use Calendar to push due date to the next open day
Today – closed day in calendar
Circulation rules hourly loan Days Mode column – days (ignore calendar)

Result
Hourly loan – correct loan due date same 1hr loan
Long loan -correct due date 6/4/20

the hourly loan checked out correctly retaining today's date and advancing due time by 1 hour from checkout, but when I tried to check the hourly loan and the long loan I got a checkin error again. See attachment for details. Not sure if this is just sandbox playing up or something else but thought you should know.
Regards, Simon
Comment 28 Jonathan Druart 2020-03-31 16:11:24 UTC
Thanks for reporting that Simon, that seems a sandbox-related bug. I will ask Martin.
Comment 29 Martin Renvoize 2020-04-01 09:27:46 UTC
(In reply to Jonathan Druart from comment #28)
> Thanks for reporting that Simon, that seems a sandbox-related bug. I will
> ask Martin.

PTFS-E Sandboxes updated to use a newer MariaDB version with the auto-increment fix.
Comment 30 Simon Perry 2020-04-02 15:31:49 UTC
Created attachment 102303 [details]
Sandbox error

Hi,
I've just tried to complete testing but am getting an error when I try to add a new bib record or item for testing. See attached for details.
Thanks, Simon
Comment 31 Jonathan Druart 2020-04-03 11:40:40 UTC
Hello Simon, We have been fighting with an annoying bugs the last 2 days. The error you saw is not related to these patches, but bug 25040 (but you do not need to care about, it's technical).

We fixed it in the sandbox manually. I do not know if you have been told to "refresh the schema" before trying the sandbox, but you should not until bug 25040 is pushed to master.

Basically the sandbox is ready for testing, biblio and item creation is ok now.
Comment 32 Simon Perry 2020-04-06 08:58:43 UTC
Hi,
I lost contact with http://staff-bug24159.sandboxes.ptfs-europe.co.uk/ on Friday afternoon. I still can't connect today. I just want to run a couple more tests before signing off so could access be restored please.
Thanks, Simon
Comment 33 Simon Perry 2020-04-08 11:19:26 UTC
Created attachment 102554 [details]
Final patch testing

Hi,
I've completed final testing (details attached for information). I tried a few different settings in the Circulation Days Mode column just to check that other options worked as I would expect. Now signing off bug.
Regards, Simon
Comment 34 PTFS Europe Sandboxes 2020-04-08 11:47:43 UTC
Created attachment 102555 [details] [review]
Bug 24159: Move useDaysMode pref to circulation rules

Moving the useDaysMode system preference to a circulation rule will add
much more flexibility in the calculation of the due date.

The initial request was to make hourly loan returned on closed when
(when checked out on the same close day).
To do so we do not want to take into account the calendar.
However the calendar need to be taken into account for other loan item types.

Other scenarios are possible, for instance depending on the branch.

This patchset will add a new "Days mode" column (next to "Loan period")
to the circulation rules page, with the different values of the
"useDaysMode" system preference + a "default" value, to default to the
system preference value.

Test plan:
- Define a long loan item type (like 10 days) that will use the calendar
(or default to the pref value, if the pref is not set to "ignore the
calendar")
- and a hourly loan (like 2 hours) that will ignore the calendar
- Create items with those item types
- Mark today as a closed day
- Check the items out
=> The hourly loan is due the same day
=> The other loan is due on an open day

QA note:
There is the need to force the "days_mode" option when Koha::Calendar is
initiated for the due date calculation. To make sure devs will not
forget it, the methods that need have it defined will throw an
exception.

Sponsored-by: Institute of Technology Carlow

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 35 PTFS Europe Sandboxes 2020-04-08 11:47:46 UTC
Created attachment 102556 [details] [review]
Bug 24159: Throw an exception if days_mode option is not given when needed

Sponsored-by: Institute of Technology Carlow

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 36 PTFS Europe Sandboxes 2020-04-08 11:47:49 UTC
Created attachment 102557 [details] [review]
Bug 24159: Add UI changes

Sponsored-by: Institute of Technology Carlow

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 37 PTFS Europe Sandboxes 2020-04-08 11:47:52 UTC
Created attachment 102558 [details] [review]
Bug 24159: Adjust tests

Sponsored-by: Institute of Technology Carlow

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 38 PTFS Europe Sandboxes 2020-04-08 11:47:55 UTC
Created attachment 102559 [details] [review]
Bug 24159: Add new tests for get_useDaysMode_effective_value

Sponsored-by: Institute of Technology Carlow

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 39 PTFS Europe Sandboxes 2020-04-08 11:47:58 UTC
Created attachment 102560 [details] [review]
Bug 24159: Set days_mode according to circ rules in 3 other places

There are 3 other occurrences where the new circ rule can be used:
 * C4::Circulation::checkHighHolds
 * Koha::Hold->set_waiting
 * misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl

Test plan:
* checkHighHolds
Enable decreaseLoanHighHolds and fill decreaseLoanHighHoldsDuration
Setup things to hit a "high demand" alert with a shortened due date
Check an item out
=> The due date must be recalculated depending on the circ rule useDaysMode.

* set_waiting
Set ExcludeHolidaysFromMaxPickUpDelay to "1" (note that there is currently
a bug in the description of the syspref, see bug 22381 comment 19)
Mark a hold waiting
The expiration date should have been set depending on the value of the
circ rule.

* TalkingTech cronjob
Cannot test this

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 40 PTFS Europe Sandboxes 2020-04-08 11:48:02 UTC
Created attachment 102561 [details] [review]
Bug 24159: Use days_mode circ rule to calculate the dropbox date

Calculate the drop box date depending on the circ rule. Note that only
the branchcode is taken into account here. Is that correct?

Test plan:
Use the drop box date feature and confirm that it takes into account the
calendar, depending on the circ rule.

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 41 PTFS Europe Sandboxes 2020-04-08 11:48:05 UTC
Created attachment 102562 [details] [review]
Bug 24159: Fix one occurrence in updatedatabase.pl

Here we just want to get the value from the syspref, the circ rule does
not exist yet.

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 42 PTFS Europe Sandboxes 2020-04-08 11:48:08 UTC
Created attachment 102563 [details] [review]
Bug 24159: Fix shift in the header

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 43 Jonathan Druart 2020-04-08 12:04:34 UTC
Thanks for the test session, Simon!
Comment 44 Katrin Fischer 2020-05-02 13:45:00 UTC
This no longer applies - can you please rebase?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24159: Move useDaysMode pref to circulation rules
Applying: Bug 24159: Throw an exception if days_mode option is not given when needed
Applying: Bug 24159: Add UI changes
Applying: Bug 24159: Adjust tests
Applying: Bug 24159: Add new tests for get_useDaysMode_effective_value
Applying: Bug 24159: Set days_mode according to circ rules in 3 other places
error: sha1 information is lacking or useless (C4/Circulation.pm).
error: could not build fake ancestor
Comment 45 Jonathan Druart 2020-05-02 14:07:35 UTC
Created attachment 104197 [details] [review]
Bug 24159: Set days_mode according to circ rules in 3 other places

There are 3 other occurrences where the new circ rule can be used:
 * C4::Circulation::checkHighHolds
 * Koha::Hold->set_waiting
 * misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl

Test plan:
* checkHighHolds
Enable decreaseLoanHighHolds and fill decreaseLoanHighHoldsDuration
Setup things to hit a "high demand" alert with a shortened due date
Check an item out
=> The due date must be recalculated depending on the circ rule useDaysMode.

* set_waiting
Set ExcludeHolidaysFromMaxPickUpDelay to "1" (note that there is currently
a bug in the description of the syspref, see bug 22381 comment 19)
Mark a hold waiting
The expiration date should have been set depending on the value of the
circ rule.

* TalkingTech cronjob
Cannot test this
Comment 46 Jonathan Druart 2020-05-02 14:07:39 UTC
Created attachment 104198 [details] [review]
Bug 24159: Use days_mode circ rule to calculate the dropbox date

Calculate the drop box date depending on the circ rule. Note that only
the branchcode is taken into account here. Is that correct?

Test plan:
Use the drop box date feature and confirm that it takes into account the
calendar, depending on the circ rule.
Comment 47 Jonathan Druart 2020-05-02 14:07:43 UTC
Created attachment 104199 [details] [review]
Bug 24159: Fix one occurrence in updatedatabase.pl

Here we just want to get the value from the syspref, the circ rule does
not exist yet.
Comment 48 Jonathan Druart 2020-05-02 14:07:47 UTC
Created attachment 104200 [details] [review]
Bug 24159: Fix shift in the header
Comment 49 Jonathan Druart 2020-05-02 14:13:45 UTC
Created attachment 104201 [details] [review]
Bug 24159: Set days_mode according to circ rules in 3 other places

There are 3 other occurrences where the new circ rule can be used:
 * C4::Circulation::checkHighHolds
 * Koha::Hold->set_waiting
 * misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl

Test plan:
* checkHighHolds
Enable decreaseLoanHighHolds and fill decreaseLoanHighHoldsDuration
Setup things to hit a "high demand" alert with a shortened due date
Check an item out
=> The due date must be recalculated depending on the circ rule useDaysMode.

* set_waiting
Set ExcludeHolidaysFromMaxPickUpDelay to "1" (note that there is currently
a bug in the description of the syspref, see bug 22381 comment 19)
Mark a hold waiting
The expiration date should have been set depending on the value of the
circ rule.

* TalkingTech cronjob
Cannot test this

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 50 Jonathan Druart 2020-05-02 14:13:50 UTC
Created attachment 104202 [details] [review]
Bug 24159: Use days_mode circ rule to calculate the dropbox date

Calculate the drop box date depending on the circ rule. Note that only
the branchcode is taken into account here. Is that correct?

Test plan:
Use the drop box date feature and confirm that it takes into account the
calendar, depending on the circ rule.

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 51 Jonathan Druart 2020-05-02 14:13:54 UTC
Created attachment 104203 [details] [review]
Bug 24159: Fix one occurrence in updatedatabase.pl

Here we just want to get the value from the syspref, the circ rule does
not exist yet.

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 52 Jonathan Druart 2020-05-02 14:13:59 UTC
Created attachment 104204 [details] [review]
Bug 24159: Fix shift in the header

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Comment 53 Martin Renvoize 2020-06-18 12:50:52 UTC
Created attachment 106007 [details] [review]
Bug 24159: Move useDaysMode pref to circulation rules

Moving the useDaysMode system preference to a circulation rule will add
much more flexibility in the calculation of the due date.

The initial request was to make hourly loan returned on closed when
(when checked out on the same close day).
To do so we do not want to take into account the calendar.
However the calendar need to be taken into account for other loan item types.

Other scenarios are possible, for instance depending on the branch.

This patchset will add a new "Days mode" column (next to "Loan period")
to the circulation rules page, with the different values of the
"useDaysMode" system preference + a "default" value, to default to the
system preference value.

Test plan:
- Define a long loan item type (like 10 days) that will use the calendar
(or default to the pref value, if the pref is not set to "ignore the
calendar")
- and a hourly loan (like 2 hours) that will ignore the calendar
- Create items with those item types
- Mark today as a closed day
- Check the items out
=> The hourly loan is due the same day
=> The other loan is due on an open day

QA note:
There is the need to force the "days_mode" option when Koha::Calendar is
initiated for the due date calculation. To make sure devs will not
forget it, the methods that need have it defined will throw an
exception.

Sponsored-by: Institute of Technology Carlow
Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2020-06-18 12:50:57 UTC
Created attachment 106008 [details] [review]
Bug 24159: Throw an exception if days_mode option is not given when needed

Sponsored-by: Institute of Technology Carlow
Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2020-06-18 14:56:02 UTC
Created attachment 106013 [details] [review]
Bug 24159: Move useDaysMode pref to circulation rules

Moving the useDaysMode system preference to a circulation rule will add
much more flexibility in the calculation of the due date.

The initial request was to make hourly loan returned on closed when
(when checked out on the same close day).
To do so we do not want to take into account the calendar.
However the calendar need to be taken into account for other loan item types.

Other scenarios are possible, for instance depending on the branch.

This patchset will add a new "Days mode" column (next to "Loan period")
to the circulation rules page, with the different values of the
"useDaysMode" system preference + a "default" value, to default to the
system preference value.

Test plan:
- Define a long loan item type (like 10 days) that will use the calendar
(or default to the pref value, if the pref is not set to "ignore the
calendar")
- and a hourly loan (like 2 hours) that will ignore the calendar
- Create items with those item types
- Mark today as a closed day
- Check the items out
=> The hourly loan is due the same day
=> The other loan is due on an open day

QA note:
There is the need to force the "days_mode" option when Koha::Calendar is
initiated for the due date calculation. To make sure devs will not
forget it, the methods that need have it defined will throw an
exception.

Sponsored-by: Institute of Technology Carlow
Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2020-06-18 14:56:14 UTC
Created attachment 106014 [details] [review]
Bug 24159: Throw an exception if days_mode option is not given when needed

Sponsored-by: Institute of Technology Carlow
Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2020-06-18 14:56:26 UTC
Created attachment 106015 [details] [review]
Bug 24159: Add UI changes

Sponsored-by: Institute of Technology Carlow
Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Martin Renvoize 2020-06-18 14:56:31 UTC
Created attachment 106016 [details] [review]
Bug 24159: Adjust tests

Sponsored-by: Institute of Technology Carlow
Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 59 Martin Renvoize 2020-06-18 14:56:35 UTC
Created attachment 106017 [details] [review]
Bug 24159: Add new tests for get_useDaysMode_effective_value

Sponsored-by: Institute of Technology Carlow
Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 60 Martin Renvoize 2020-06-18 14:56:39 UTC
Created attachment 106018 [details] [review]
Bug 24159: Set days_mode according to circ rules in 3 other places

There are 3 other occurrences where the new circ rule can be used:
 * C4::Circulation::checkHighHolds
 * Koha::Hold->set_waiting
 * misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl

Test plan:
* checkHighHolds
Enable decreaseLoanHighHolds and fill decreaseLoanHighHoldsDuration
Setup things to hit a "high demand" alert with a shortened due date
Check an item out
=> The due date must be recalculated depending on the circ rule useDaysMode.

* set_waiting
Set ExcludeHolidaysFromMaxPickUpDelay to "1" (note that there is currently
a bug in the description of the syspref, see bug 22381 comment 19)
Mark a hold waiting
The expiration date should have been set depending on the value of the
circ rule.

* TalkingTech cronjob
Cannot test this

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 61 Martin Renvoize 2020-06-18 14:56:43 UTC
Created attachment 106019 [details] [review]
Bug 24159: Use days_mode circ rule to calculate the dropbox date

Calculate the drop box date depending on the circ rule. Note that only
the branchcode is taken into account here. Is that correct?

Test plan:
Use the drop box date feature and confirm that it takes into account the
calendar, depending on the circ rule.

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Martin Renvoize 2020-06-18 14:56:46 UTC
Created attachment 106020 [details] [review]
Bug 24159: Fix one occurrence in updatedatabase.pl

Here we just want to get the value from the syspref, the circ rule does
not exist yet.

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Martin Renvoize 2020-06-18 14:56:50 UTC
Created attachment 106021 [details] [review]
Bug 24159: Fix shift in the header

Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 64 Martin Renvoize 2020-06-18 14:56:54 UTC
Created attachment 106022 [details] [review]
Bug 24159: (QA follow-up) Make terminology consistent

This patch changes the new circulation rule that's introduced from
useDaysMode to daysmode to improve consistency with other rule names.

We also update the accessors and code using them to reflect the new
term.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 65 Martin Renvoize 2020-06-18 15:05:17 UTC
This works well and passes the qa script.

I have added a trivial followup to update the new rule name and introduced accessor to more closely match the existing rules and methods for consistency.

I would love to see some future work, and with any luck I can contribute it, to further 'simplify' the Koha::CirculationRules module to make it more self-consistent but that work doesn't need to happen here.

Passing QA
Comment 66 Katrin Fischer 2020-06-20 15:25:38 UTC
Hi, 

would you agree that this is not a bug? It appears to add new configuration options in the circulation rules to allow covering more uses cases - this is valid but I'd see it more as an enhancement.

I'd also love to see a more descriptive bug title that tells us more about what this patchset does.

I really like the default value mentioned in the patch description that allows fallback to the pref - I was worried a bit about having to set yet another value!
Comment 67 Katrin Fischer 2020-06-20 15:26:52 UTC
Suggestion for the bug title:

"Move useDaysMode preference for calculating due dates to the circulation rules"
Comment 68 Martin Renvoize 2020-06-22 07:40:34 UTC
(In reply to Katrin Fischer from comment #67)
> Suggestion for the bug title:
> 
> "Move useDaysMode preference for calculating due dates to the circulation
> rules"

I agree it's an enhancement as it stands... as for the title how about:

"Allow daysMode to be set at the circulation rules level"

I think that still conveys the right message but also hints that the original system preference is intact?
Comment 69 Katrin Fischer 2020-06-22 10:29:38 UTC
I sneaked in a few more words :)
Comment 70 Jonathan Druart 2020-06-25 09:12:28 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 71 Andrii Nugged 2020-06-29 20:54:52 UTC
Small (below trivial :P) postfix: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25896
Comment 72 Lucas Gass 2020-07-13 15:50:52 UTC
not backporting enhancement to 20.05.x series