Description
Sally
2015-11-27 14:23:50 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 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 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". Created attachment 88053 [details] [review] Bug 15260: Modify description of Dayweek option 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. 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. 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 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. Created attachment 88059 [details] [review] Bug 15260: (follow-up) Fix misnamed variable This was causing the amount to push by to be incorrectly calculated 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 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 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 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 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 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 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 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 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 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 Great catch Liz, thanks! That was a nasty one! Thank the unit tests :) 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 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 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 89388 [details] [review] Bug 15260: (QA follow-up) Fix updatedatabase message Signed-off-by: Liz Rea <wizzyrea@gmail.com> only FQA for the qa tool that might be a false positive, fix that up and it can go back to SO. :) Liz Created attachment 89885 [details] [review] Bug 15260: (follow-up) Add missing POD As per comment #33 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Nice work! Pushed to master for 19.11.00 +my $dayweek_categorycode = 'C'; +my $dayweek_itemtype = 'MX'; +my $dayweek_branchcode = 'FPL'; That must not be hardcoded. (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? 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. |