Bug 15260 - Option for extended loan with useDaysMode
Summary: Option for extended loan with useDaysMode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Holidays (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Andrew Isherwood
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks: 25781 22381 25850
  Show dependency tree / graph
 
Reported: 2015-11-27 14:23 UTC by Sally
Modified: 2022-11-11 10:32 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
The `useDaysMode` system preference has been enhanced to include an additional option. This allows libraries to dictate that if the library is closed on the usual due date, the loan period should be pushed forward to the next open day which is the same day of the week. For example : If an item should be due back on a Tuesday but that particular Tuesday is closed, then instead of it being due back the Wednesday (usual behaviour when due date is pushed forward to next open day), it would actually be due back the next available open Tuesday. **Note**: This preference setting only works in multiples of 7.
Version(s) released in:
19.11.00
Circulation function:

Attachments
Bug 15260: (follow-up) Fix misnamed variable (1014 bytes, patch)
2019-04-12 13:01 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Add new useDaysMode option "Dayweek" (3.85 KB, patch)
2019-04-16 11:16 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Modify description of Dayweek option (1.32 KB, patch)
2019-04-16 11:16 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Modify next/prev_open_day (6.28 KB, patch)
2019-04-16 11:16 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Implement weekly rollforward (2.81 KB, patch)
2019-04-16 11:16 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Add unit tests (10.18 KB, patch)
2019-04-16 11:16 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Fix calculation of add amount (2.59 KB, patch)
2019-04-16 11:16 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: (follow-up) Fix misnamed variable (1014 bytes, patch)
2019-04-16 11:16 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Add new useDaysMode option "Dayweek" (3.98 KB, patch)
2019-04-24 09:53 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Modify description of Dayweek option (1.45 KB, patch)
2019-04-24 09:53 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Modify next/prev_open_day (6.41 KB, patch)
2019-04-24 09:53 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Implement weekly rollforward (2.94 KB, patch)
2019-04-24 09:53 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Add unit tests (10.31 KB, patch)
2019-04-24 09:53 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Fix calculation of add amount (2.72 KB, patch)
2019-04-24 09:53 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: (follow-up) Fix misnamed variable (1.12 KB, patch)
2019-04-24 09:53 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: (follow-up) Fix bug in subtraction (943 bytes, patch)
2019-05-01 11:15 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Add new useDaysMode option "Dayweek" (4.02 KB, patch)
2019-05-06 17:44 UTC, Liz Rea 		Details | Diff | Splinter Review
Bug 15260: Modify description of Dayweek option (1.49 KB, patch)
2019-05-06 17:45 UTC, Liz Rea 		Details | Diff | Splinter Review
Bug 15260: Modify next/prev_open_day (6.46 KB, patch)
2019-05-06 17:45 UTC, Liz Rea 		Details | Diff | Splinter Review
Bug 15260: Implement weekly rollforward (2.98 KB, patch)
2019-05-06 17:45 UTC, Liz Rea 		Details | Diff | Splinter Review
Bug 15260: Add unit tests (10.36 KB, patch)
2019-05-06 17:45 UTC, Liz Rea 		Details | Diff | Splinter Review
Bug 15260: Fix calculation of add amount (2.76 KB, patch)
2019-05-06 17:45 UTC, Liz Rea 		Details | Diff | Splinter Review
Bug 15260: (follow-up) Fix misnamed variable (1.17 KB, patch)
2019-05-06 17:45 UTC, Liz Rea 		Details | Diff | Splinter Review
Bug 15260: (follow-up) Fix bug in subtraction (988 bytes, patch)
2019-05-06 17:46 UTC, Liz Rea 		Details | Diff | Splinter Review
Bug 15260: (QA follow-up) Fix updatedatabase message (1.11 KB, patch)
2019-05-06 17:46 UTC, Liz Rea 		Details | Diff | Splinter Review
Bug 15260: (follow-up) Add missing POD (919 bytes, patch)
2019-05-17 11:17 UTC, Andrew Isherwood 		Details | Diff | Splinter Review
Bug 15260: Add new useDaysMode option "Dayweek" (4.04 KB, patch)
2019-10-30 18:33 UTC, Josef Moravec 		Details | Diff | Splinter Review
Bug 15260: Modify description of Dayweek option (1.55 KB, patch)
2019-10-30 18:33 UTC, Josef Moravec 		Details | Diff | Splinter Review
Bug 15260: Modify next/prev_open_day (6.51 KB, patch)
2019-10-30 18:33 UTC, Josef Moravec 		Details | Diff | Splinter Review
Bug 15260: Implement weekly rollforward (3.04 KB, patch)
2019-10-30 18:34 UTC, Josef Moravec 		Details | Diff | Splinter Review
Bug 15260: Add unit tests (10.41 KB, patch)
2019-10-30 18:34 UTC, Josef Moravec 		Details | Diff | Splinter Review
Bug 15260: Fix calculation of add amount (2.82 KB, patch)
2019-10-30 18:34 UTC, Josef Moravec 		Details | Diff | Splinter Review
Bug 15260: (follow-up) Fix misnamed variable (1.22 KB, patch)
2019-10-30 18:34 UTC, Josef Moravec 		Details | Diff | Splinter Review
Bug 15260: (follow-up) Fix bug in subtraction (1.02 KB, patch)
2019-10-30 18:34 UTC, Josef Moravec 		Details | Diff | Splinter Review
Bug 15260: (QA follow-up) Fix updatedatabase message (1.16 KB, patch)
2019-10-30 18:34 UTC, Josef Moravec 		Details | Diff | Splinter Review
Bug 15260: (follow-up) Add missing POD (975 bytes, patch)
2019-10-30 18:34 UTC, Josef Moravec 		Details | Diff | Splinter Review
Show Obsolete (26) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sally 2015-11-27 14:23:50 UTC 
Could an option be introduced for holiday closures where the due date is set to be the next matching open day?  (Matching means Monday = Monday, Tuesday = Tuesday etc.)

There are currently 3 options with holiday dates using useDaysMode syspref:

1)  Circulation rules only = no change to expected due date
2)  Calendar to push the due date to next open day = due date changed to next open day 
3)  Calendar to skip all days the library is closed = extra days - equivalent to closed days - added 

We use 21 days as our loan period.  When a due date falls on a closed day, we would like to offer a loan on the next matching open day - e.g. a 28 day loan, not a 22 day loan.  

If an item is borrowed on Monday 1st June, it would ordinarily be due on Monday 22nd June (21 day loan as standard).  If a library was closed on Monday 22nd June, we would like the due date to be Monday 29th June (28 day loan) - not Tuesday 23rd June (22 day loan).

This isn't just a 7 day push on because of Christmas holidays.  For example:  

Items borrowed on Friday 4th December should be due back Friday 25th December.
Friday 25th December is closed.  A 28 day loan would cause the items to be due back on Friday 1st January.  Friday 1st January is closed.  We would like the loan to be pushed to 35 days to the next Friday that the library is open = Friday 8th January, even though the library is open for business on non-matching (Monday, Tuesday, Wednesday etc) days between those dates.
Comment 1 Andrew Isherwood 2019-04-12 13:01:32 UTC 
Created attachment 87906 [details] [review]
Bug 15260: (follow-up) Fix misnamed variable

This was causing the amount to push by to be incorrectly calculated
Comment 2 Andrew Isherwood 2019-04-12 13:02:58 UTC 
Comment on attachment 87906 [details] [review]
Bug 15260: (follow-up) Fix misnamed variable

>From 12eecbfc9f52184475299caf02a8efae8e86f5ba Mon Sep 17 00:00:00 2001
>From: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
>Date: Fri, 12 Apr 2019 12:31:31 +0100
>Subject: [PATCH] Bug 15260: (follow-up) Fix misnamed variable
>
>This was causing the amount to push by to be incorrectly calculated
>---
> Koha/Calendar.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm
>index 47c1728575..932b7f1c1e 100644
>--- a/Koha/Calendar.pm
>+++ b/Koha/Calendar.pm
>@@ -218,7 +218,7 @@ sub addDays {
>                 my $dow = $base_date->day_of_week;
>                 my $days = $days_duration->in_units('days');
>                 # Is it a period based on weeks
>-                my $push_amt = $days_duration % 7 == 0 ?
>+                my $push_amt = $days % 7 == 0 ?
>                     $self->get_push_amt($base_date) : 1;
>                 if ( $days_duration->is_negative() ) {
>                     $base_date = $self->prev_open_days($base_date, $push_amt);
>-- 
>2.11.0
Comment 3 Andrew Isherwood 2019-04-16 11:16:41 UTC 
Created attachment 88052 [details] [review]
Bug 15260: Add new useDaysMode option "Dayweek"

This patch adds a new "Dayweek" option to the useDaysMode syspref, it
has the description: "the calendar to push the due date to the next open
day on the same weekday".
Comment 4 Andrew Isherwood 2019-04-16 11:16:43 UTC 
Created attachment 88053 [details] [review]
Bug 15260: Modify description of Dayweek option
Comment 5 Andrew Isherwood 2019-04-16 11:16:46 UTC 
Created attachment 88054 [details] [review]
Bug 15260: Modify next/prev_open_day

We require next_open_day & prev_open_day to be more flexible. We could
create a separate sub, but that's not very DRY given that they'll do
pretty much the same thing.

So next_open_day becomes next_open_days and prev_open_day becomes
prev_open_days and both functions accept an additional parameter which
determines how many days they add or subtract.

All calls of these two functions have been modified accordingly.
Comment 6 Andrew Isherwood 2019-04-16 11:16:49 UTC 
Created attachment 88055 [details] [review]
Bug 15260: Implement weekly rollforward

This patch will, when appropriate, roll due dates forward by full weeks,
rather than single days.
Comment 7 Andrew Isherwood 2019-04-16 11:16:52 UTC 
Created attachment 88056 [details] [review]
Bug 15260: Add unit tests

This patch adds unit tests of the CalcDueDate function to ensure that
the useDaysMode "Dayweek" mode works as expected
Comment 8 Andrew Isherwood 2019-04-16 11:16:54 UTC 
Created attachment 88057 [details] [review]
Bug 15260: Fix calculation of add amount

In a sequence of closed days, we should take into account the nature
of each closed day as we encounter it in order to calculate the amount
to add to reach the next potential closed date. We are now doing this.
Comment 9 Andrew Isherwood 2019-04-16 11:16:57 UTC 
Created attachment 88059 [details] [review]
Bug 15260: (follow-up) Fix misnamed variable

This was causing the amount to push by to be incorrectly calculated
Comment 10 Andrew Isherwood 2019-04-16 12:11:43 UTC 
Test plan:

Set up
------

- Apply patches
- Set the useDaysMode syspref to "Calculate the due date using the calendar to push the due date to the next open matching weekday for weekly loan periods, or the next open day otherwise"
- In order to test this functionality, you will need items with weekly (or multiples of) loan period (e.g. 7, 14, 21 days etc)

Test 1 - One week roll forward
------------------------------

Here we test that loans and renewals roll forward one week if the expected due date is closed.

- Identify the loan and renewal periods of the item you intend testing and add a temporary closure day to the calendar on the due date for issues and renewals
- Issue the item
=> TEST: Observe the item's due date rolled forward to one week later than the expected issue due date
- Renew the item
=> TEST: Observe the item's due date rolled forward to one week later than the expected renewal due date

Test 2 - Multiple week roll forward
-----------------------------------

Here we test that loans and renewals continue to roll forward if there are subsequent closures on the day of the week of the due date

- Identify the loan and renewal periods of the item you intend testing and add a closure temporary day to the calendar on the due date for issues and renewals, and also to the two subsequent weeks on the same day
- Issue the item
=> TEST: Observe the item's due date rolled forward to one week later than the lastclosure day for that day of the week
- Renew the item
=> TEST: Observe the item's due date rolled forward to one week later than the lastclosure day for that day of the week

Test 3 - Permanent closures
---------------------------

Here we test that the functionality doesn't try to continue looking for an open day on the same day of the week as the expected due date if the library has a permanent closure on that day of the week, instead it rolls forward a single day

- Identify the loan and renewal periods of the item you intend testing and add a recurring holiday on the day of the week of the expected due date
- Issue the item
=> TEST: Observe that the item's due date only rolled forward a single day from the original due date as it was unable to roll forward a week or multiple weeks
- Add a recurring holiday on the day of the week of the item's new due date
- Renew the item
=> TEST:  Observe that the item's due date only rolled forward a single day from the original renewal due date as it was unable to roll forward a week or multiple weeks
Comment 11 Andrew Isherwood 2019-04-24 09:53:40 UTC 
Created attachment 88607 [details] [review]
Bug 15260: Add new useDaysMode option "Dayweek"

This patch adds a new "Dayweek" option to the useDaysMode syspref, it
has the description: "the calendar to push the due date to the next open
day on the same weekday".

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council
Comment 12 Andrew Isherwood 2019-04-24 09:53:43 UTC 
Created attachment 88608 [details] [review]
Bug 15260: Modify description of Dayweek option

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council
Comment 13 Andrew Isherwood 2019-04-24 09:53:46 UTC 
Created attachment 88609 [details] [review]
Bug 15260: Modify next/prev_open_day

We require next_open_day & prev_open_day to be more flexible. We could
create a separate sub, but that's not very DRY given that they'll do
pretty much the same thing.

So next_open_day becomes next_open_days and prev_open_day becomes
prev_open_days and both functions accept an additional parameter which
determines how many days they add or subtract.

All calls of these two functions have been modified accordingly.

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council
Comment 14 Andrew Isherwood 2019-04-24 09:53:49 UTC 
Created attachment 88610 [details] [review]
Bug 15260: Implement weekly rollforward

This patch will, when appropriate, roll due dates forward by full weeks,
rather than single days.

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council
Comment 15 Andrew Isherwood 2019-04-24 09:53:52 UTC 
Created attachment 88611 [details] [review]
Bug 15260: Add unit tests

This patch adds unit tests of the CalcDueDate function to ensure that
the useDaysMode "Dayweek" mode works as expected

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council
Comment 16 Andrew Isherwood 2019-04-24 09:53:55 UTC 
Created attachment 88612 [details] [review]
Bug 15260: Fix calculation of add amount

In a sequence of closed days, we should take into account the nature
of each closed day as we encounter it in order to calculate the amount
to add to reach the next potential closed date. We are now doing this.

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council
Comment 17 Andrew Isherwood 2019-04-24 09:53:58 UTC 
Created attachment 88613 [details] [review]
Bug 15260: (follow-up) Fix misnamed variable

This was causing the amount to push by to be incorrectly calculated

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council
Comment 18 Liz Rea 2019-04-24 18:01:47 UTC 
Hi, before I got into functional testing I tried the unit tests, and got a failure:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove -v t/db_dependent/Circulation/CalcDateDue.t 
t/db_dependent/Circulation/CalcDateDue.t .. 
1..17
ok 1 - use C4::Circulation;
ok 2 - date expiry
ok 3 - date expiry with useDaysMode to noDays
ok 4 - date expiry should be 2013-01-01 -1 day
not ok 5 - date expiry should be 2013-01-01 -2 day

#   Failed test 'date expiry should be 2013-01-01 -2 day'
#   at t/db_dependent/Circulation/CalcDateDue.t line 80.
#          got: '2013-01-02T23:59:00'
#     expected: '2012-12-30T23:59:00'
ok 6 - date expiry ( 9 + 10 )
ok 7 - date expiry ( 9 + 5 )
ok 8 - useDaysMode = Dayweek, no closed days, issue date expiry ( start + 10 )
ok 9 - useDaysMode = Dayweek, no closed days, renewal date expiry ( start + 5 )
ok 10 - useDaysMode = Dayweek, closed on due date, 10 day loan (should not trigger 7 day roll forward), issue date expiry ( start + 10  + 1 )
ok 11 - useDaysMode = Dayweek, closed on due date, 5 day renewal (should not trigger 7 day roll forward), renewal date expiry ( start + 5  + 1 )
ok 12 - useDaysMode = Dayweek, closed on due date, 14 day loan (should trigger 7 day roll forward), issue date expiry ( start + 21 )
ok 13 - useDaysMode = Dayweek, closed on due date, 7 day renewal (should trigger 7 day roll forward), renewal date expiry ( start + 14 )
ok 14 - useDaysMode = Dayweek, closed on due date and two subequent due dates, 14 day loan (should trigger 2 x 7 day roll forward), issue date expiry ( start + 28 )
ok 15 - useDaysMode = Dayweek, closed on due date and three subsequent due dates, 7 day renewal (should trigger 3 x 7 day roll forward), issue date expiry ( start + 28 )
ok 16 - useDaysMode = Dayweek, due on Saturday, closed on Saturdays, 14 day loan (should trigger 1 day roll forward), issue date expiry ( start + 15 )
ok 17 - useDaysMode = Dayweek, due on Saturday, closed on Saturdays, 7 day renewal (should trigger 1 day roll forward), issue date expiry ( start + 8 )
# Looks like you failed 1 test of 17.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/17 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation/CalcDateDue.t (Wstat: 256 Tests: 17 Failed: 1)
  Failed test:  5
  Non-zero exit status: 1
Files=1, Tests=17,  3 wallclock secs ( 0.03 usr  0.01 sys +  1.45 cusr  0.52 csys =  2.01 CPU)
Result: FAIL

I have decided not to continue on with functional testing until the tests pass.

Cheers,
Liz
Comment 19 Andrew Isherwood 2019-05-01 11:15:09 UTC 
Created attachment 89178 [details] [review]
Bug 15260: (follow-up) Fix bug in subtraction

prev_open_days can receive either positive or negative numbers and
then does the right thing with them. However, we weren't also doing this
with the number it receives back from get_push_amt
Comment 20 Andrew Isherwood 2019-05-01 11:15:38 UTC 
Great catch Liz, thanks! That was a nasty one!
Comment 21 Liz Rea 2019-05-01 20:49:59 UTC 
Thank the unit tests :)
Comment 22 Liz Rea 2019-05-06 17:40:30 UTC 
Hi,

This mostly looks good - tests pass now and I didn't get any failures on due date calculation using this system preference, and existing functionality seems good. 

This may very well be a false positive but could you check out this QA tool failure: 

 FAIL	Koha/Calendar.pm
   FAIL	  pod coverage
		POD is missing for get_push_amt

I'll fix up the other QA tool concerns in a couple of follow ups (namely commit messages having malformed headers and the DB update not having a nice message) :)

Cheers,
Liz
Comment 23 Liz Rea 2019-05-06 17:41:53 UTC 
Accidentally sent this as a private comment, sorry for the double up.

Hi,

This mostly looks good - tests pass now and I didn't get any failures on due date calculation using this system preference, and existing functionality seems good. 

This may very well be a false positive but could you check out this QA tool failure: 

 FAIL	Koha/Calendar.pm
   FAIL	  pod coverage
		POD is missing for get_push_amt

I'll fix up the other QA tool concerns in a couple of follow ups (namely commit messages having malformed headers and the DB update not having a nice message) :)

Cheers,
Liz
Comment 24 Liz Rea 2019-05-06 17:44:53 UTC 
Created attachment 89380 [details] [review]
Bug 15260: Add new useDaysMode option "Dayweek"

This patch adds a new "Dayweek" option to the useDaysMode syspref, it
has the description: "the calendar to push the due date to the next open
day on the same weekday".

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 25 Liz Rea 2019-05-06 17:45:17 UTC 
Created attachment 89381 [details] [review]
Bug 15260: Modify description of Dayweek option

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 26 Liz Rea 2019-05-06 17:45:24 UTC 
Created attachment 89382 [details] [review]
Bug 15260: Modify next/prev_open_day

We require next_open_day & prev_open_day to be more flexible. We could
create a separate sub, but that's not very DRY given that they'll do
pretty much the same thing.

So next_open_day becomes next_open_days and prev_open_day becomes
prev_open_days and both functions accept an additional parameter which
determines how many days they add or subtract.

All calls of these two functions have been modified accordingly.

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 27 Liz Rea 2019-05-06 17:45:29 UTC 
Created attachment 89383 [details] [review]
Bug 15260: Implement weekly rollforward

This patch will, when appropriate, roll due dates forward by full weeks,
rather than single days.

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 28 Liz Rea 2019-05-06 17:45:38 UTC 
Created attachment 89384 [details] [review]
Bug 15260: Add unit tests

This patch adds unit tests of the CalcDueDate function to ensure that
the useDaysMode "Dayweek" mode works as expected

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 29 Liz Rea 2019-05-06 17:45:47 UTC 
Created attachment 89385 [details] [review]
Bug 15260: Fix calculation of add amount

In a sequence of closed days, we should take into account the nature
of each closed day as we encounter it in order to calculate the amount
to add to reach the next potential closed date. We are now doing this.

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 30 Liz Rea 2019-05-06 17:45:55 UTC 
Created attachment 89386 [details] [review]
Bug 15260: (follow-up) Fix misnamed variable

This was causing the amount to push by to be incorrectly calculated

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 31 Liz Rea 2019-05-06 17:46:05 UTC 
Created attachment 89387 [details] [review]
Bug 15260: (follow-up) Fix bug in subtraction

prev_open_days can receive either positive or negative numbers and
then does the right thing with them. However, we weren't also doing this
with the number it receives back from get_push_amt

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 32 Liz Rea 2019-05-06 17:46:18 UTC 
Created attachment 89388 [details] [review]
Bug 15260: (QA follow-up) Fix updatedatabase message

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 33 Liz Rea 2019-05-07 19:51:36 UTC 
only FQA for the qa tool that might be a false positive, fix that up and it can go back to SO. :)

Liz
Comment 34 Andrew Isherwood 2019-05-17 11:17:38 UTC 
Created attachment 89885 [details] [review]
Bug 15260: (follow-up) Add missing POD

As per comment #33
Comment 35 Josef Moravec 2019-10-30 18:33:44 UTC 
Created attachment 94880 [details] [review]
Bug 15260: Add new useDaysMode option "Dayweek"

This patch adds a new "Dayweek" option to the useDaysMode syspref, it
has the description: "the calendar to push the due date to the next open
day on the same weekday".

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 36 Josef Moravec 2019-10-30 18:33:49 UTC 
Created attachment 94881 [details] [review]
Bug 15260: Modify description of Dayweek option

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 37 Josef Moravec 2019-10-30 18:33:55 UTC 
Created attachment 94882 [details] [review]
Bug 15260: Modify next/prev_open_day

We require next_open_day & prev_open_day to be more flexible. We could
create a separate sub, but that's not very DRY given that they'll do
pretty much the same thing.

So next_open_day becomes next_open_days and prev_open_day becomes
prev_open_days and both functions accept an additional parameter which
determines how many days they add or subtract.

All calls of these two functions have been modified accordingly.

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 38 Josef Moravec 2019-10-30 18:34:00 UTC 
Created attachment 94883 [details] [review]
Bug 15260: Implement weekly rollforward

This patch will, when appropriate, roll due dates forward by full weeks,
rather than single days.

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 39 Josef Moravec 2019-10-30 18:34:05 UTC 
Created attachment 94884 [details] [review]
Bug 15260: Add unit tests

This patch adds unit tests of the CalcDueDate function to ensure that
the useDaysMode "Dayweek" mode works as expected

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 40 Josef Moravec 2019-10-30 18:34:10 UTC 
Created attachment 94885 [details] [review]
Bug 15260: Fix calculation of add amount

In a sequence of closed days, we should take into account the nature
of each closed day as we encounter it in order to calculate the amount
to add to reach the next potential closed date. We are now doing this.

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 41 Josef Moravec 2019-10-30 18:34:16 UTC 
Created attachment 94886 [details] [review]
Bug 15260: (follow-up) Fix misnamed variable

This was causing the amount to push by to be incorrectly calculated

Sponsored-by: Cheshire West and Chester and Cheshire East Councils
Sponsored-by: Newcastle City Council
Sponsored-by: Sefton Council

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 42 Josef Moravec 2019-10-30 18:34:21 UTC 
Created attachment 94887 [details] [review]
Bug 15260: (follow-up) Fix bug in subtraction

prev_open_days can receive either positive or negative numbers and
then does the right thing with them. However, we weren't also doing this
with the number it receives back from get_push_amt

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 43 Josef Moravec 2019-10-30 18:34:27 UTC 
Created attachment 94888 [details] [review]
Bug 15260: (QA follow-up) Fix updatedatabase message

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 44 Josef Moravec 2019-10-30 18:34:32 UTC 
Created attachment 94889 [details] [review]
Bug 15260: (follow-up) Add missing POD

As per comment #33

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 45 Martin Renvoize (ashimema) 2019-10-31 11:57:44 UTC 
Nice work!

Pushed to master for 19.11.00
Comment 46 Jonathan Druart 2020-01-22 12:44:39 UTC 
+my $dayweek_categorycode = 'C';
+my $dayweek_itemtype = 'MX';
+my $dayweek_branchcode = 'FPL';


That must not be hardcoded.
Comment 47 Martin Renvoize (ashimema) 2020-01-22 12:58:00 UTC 
(In reply to Jonathan Druart from comment #46)
> +my $dayweek_categorycode = 'C';
> +my $dayweek_itemtype = 'MX';
> +my $dayweek_branchcode = 'FPL';
> 
> 
> That must not be hardcoded.

I only see it hardcoded in a test.. can you explain what issue you perceive?
Comment 48 Jonathan Druart 2020-01-22 13:41:13 UTC 
categorycode=C does not exist, working on 18936 that will add a FK on the circulation rule, the test failed.

I fixed it there (replaced C with K), but it would be good to have it create all the data it needs, not depend on existing data.