What should happen when a librarian specifies a due date that matches a holiday and useDaysMode=Datedue (Calculate the due date using the calendar to push the due date to the next open day)? My guess is that the due date should be fixed to match the next open day.
Created attachment 12444 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour useDaysMode=Datedue wasn't used as advertised in the docs. Added a next_open_day sub to Koha::Calendar and some tests for it. - AddIssue was fixed for the case $datedue was defined in circ/circulation.pl - CalcDateDue was modified to be consistent with the intended behaviour Regards To+ Sponsored-by: Universidad Nacional de Córdoba
Created attachment 12445 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour useDaysMode=Datedue wasn't used as advertised in the docs. Added a next_open_day sub to Koha::Calendar and some tests for it. - AddIssue was fixed for the case $datedue was defined in circ/circulation.pl - CalcDateDue was modified to be consistent with the intended behaviour Regards To+ Sponsored-by: Universidad Nacional de Córdoba
This patch needs two sysprefs set like this: useDaysMode=Datedue SpecifyDueDate=1 Test plan: - On circulation specify a due date that matches a holiday, the due day should be automatically set to the next open day. - Checkout an item so that the due date (without taking the calendar into account) matches a holiday, the final due date should be set to the next open day.
Created attachment 12446 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour Forgot to check availability of some variables. Fixed. Regards To+ Sponsored-by: Universidad Nacional de Córdoba
Created attachment 12448 [details] [review] useDaysMode='Calendar' should make date due be the next open day too. Sponsored-by: Universidad Nacional de Córdoba
Created attachment 12449 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour useDaysMode=Datedue wasn't used as advertised in the docs. Added a next_open_day sub to Koha::Calendar and some tests for it. - AddIssue was fixed for the case $datedue was defined in circ/circulation.pl - CalcDateDue was modified to be consistent with the intended behaviour Regards To+ Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Elliott Davis <elliott@bywatersolutions.com>
Created attachment 12527 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour (3.8.x) useDaysMode=Datedue wasn't used as advertised in the docs. Added a next_open_day sub to Koha::Calendar and some tests for it. - AddIssue was fixed for the case $datedue was defined in circ/circulation.pl - CalcDateDue was modified to be consistent with the intended behaviour Regards To+ Sponsored-by: Universidad Nacional de Córdoba
Created attachment 12600 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour (3.8.x) useDaysMode=Datedue wasn't used as advertised in the docs. Added a next_open_day sub to Koha::Calendar and some tests for it. - AddIssue was fixed for the case $datedue was defined in circ/circulation.pl - CalcDateDue was modified to be consistent with the intended behaviour Regards To+ Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Federico Rinaudo <frinaudo@infocpt.com.ar>
(In reply to comment #6) > Created attachment 12449 [details] [review] > Bug 8800 - useDaysMode=Datedue wrong behaviour > > Sponsored-by: Universidad Nacional de Córdoba > Signed-off-by: Elliott Davis <elliott@bywatersolutions.com> patch (applied on master) looks good, passing QA... $ koha-qa.pl -c 1 testing 1 commit(s) (applied to commit 6c18204) * a74b490 Bug 8800 - useDaysMode=Datedue wrong behaviour C4/Circulation.pm Koha/Calendar.pm t/Calendar.t * C4/Circulation.pm OK * Koha/Calendar.pm OK * t/Calendar.t OK
(additionnal QA comment, this patch could have a big side effect, i'll push it becase it's a bugfix, but I double check !) This patch fixes a problem in 2 different places: * in AddIssue, if the datedue is provided, Koha calculate the next open day (except if useDaysMode is set to Calendar). This sounds logical = the librarian entering a date may have forgotten that it's a closed date, and it's useful that Koha calculates the next open date (this part of the patch could be considered as an ENH) * in CalcDueDate, called if the datedue is not provided, the patch adds a sub to calculate the next open day if the syspref is set to Datedue. But, from a QA pov, i'm not sure this patch is written in the proper way: in Koha::Calendar.pm, the method addDate is here to calculate the datedue. The next_open_day added in this patch is a duplicate of what addDate is supposed to do. In fact, the addDate already includes calculation: } elsif ( $days_mode eq 'Calendar' ) { if ( $unit eq 'hours' ) { $base_date->add_duration($add_duration); while ( $self->is_holiday($base_date) ) { $base_date->add_duration($day_dur); } } else { my $days = abs $add_duration->in_units('days'); while ($days) { $base_date->add_duration($day_dur); if ( $self->is_holiday($base_date) ) { next; } else { --$days; } } } If it does not work, it should be fixed here. Overall, failing QA, sorry: the patch may fix the problem, but it's improperly coded.
Created attachment 12919 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited) useDaysMode=Datedue wasn't used as advertised in the docs. Added next_open_day and prev_open_day subs to Koha::Calendar and some tests for them. - CalcDateDue was modified to be consistent with the intended behaviour - Koha::Calendar->addDate was rewritten in a more sane way (also split into addHours and addDays for convenience). Regards To+ Sponsored-by: Universidad Nacional de Córdoba
perl t/Calendar.t 2012-11-10 08:23:06 christopher pts/2 1..28 ok 1 - use Koha::Calendar; ok 2 - use C4::Calendar; ok 3 - Calendar class returned isa Koha::Calendar ok 4 - Sunday is a closed day ok 5 - Monday is not a closed day ok 6 - month/day closed day test ok 7 - special closed day test ok 8 - open day test ok 9 - addDate skips closed Sunday ok 10 - Negative call to addDate ok 11 - days_between calculates correctly ok 12 - Simple Single Day Add (Calendar) ok 13 - Add 7 days Calendar mode ok 14 - Add 7 days Datedue mode ok 15 - Add 7 days Days mode ok 16 - is holiday for the next test ok 17 - Date should be the same after is_holiday ok 18 - test larger intervals ok 19 - test positive intervals ok 20 - test parameter order not relevant ok 21 - days_between calculates correctly ok 22 - holiday correctly recognized ok 23 - multiple holidays correctly recognized ok 24 - Next open day correctly calculated ok 25 - Previous open day correctly calculated ok 1 - Single day add (Datedue, matches holiday, shift) ok 2 - Two days add, skips holiday (Datedue) 1..2 ok 26 - useDaysMode=Datedue ok 1 - Single day add (Calendar) 1..1 ok 27 - useDaysMode=Calendar ok 1 - Single day add (Days) 1..1 ok 28 - useDaysMode=Days
Created attachment 13367 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited) useDaysMode=Datedue wasn't used as advertised in the docs. Added next_open_day and prev_open_day subs to Koha::Calendar and some tests for them. - CalcDateDue was modified to be consistent with the intended behaviour - Koha::Calendar->addDate was rewritten in a more sane way (also split into addHours and addDays for convenience). Regards To+ Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Increased the number of tests to 28
QA comment * all tests passes, * koha-qa.pl is OK * looking at the code, it seems to properly fix the problem. Thanks a lot Tomas ! passed QA
Created attachment 13402 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited) useDaysMode=Datedue wasn't used as advertised in the docs. Added next_open_day and prev_open_day subs to Koha::Calendar and some tests for them. - CalcDateDue was modified to be consistent with the intended behaviour - Koha::Calendar->addDate was rewritten in a more sane way (also split into addHours and addDays for convenience). Regards To+ Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Increased the number of tests to 28 Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Could I please have a detailed test plan? As I understand it, I should be taking the following steps to test this: 1) Create a holiday, say on November 14. 2) Try to check out a book, manually setting the due date to November 14. 3) See that the book is due November 15. No matter what I said useDaysMode to, the book is due November 14. I also tried setting a 2-day loan rule, and the due date was still November 14. Am I doing something wrong?
I ommited the manual-date-auto-fix thing as Paul said it should have its own enh bug. This just makes usedaysmode=datedue work as expected. Automatic calculation.
Okay, that makes sense. What is the exact procedure I should use for testing the automatic calculation?
Created attachment 13447 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited) useDaysMode=Datedue wasn't used as advertised in the docs. Added next_open_day and prev_open_day subs to Koha::Calendar and some tests for them. - Koha::Calendar->addDate was rewritten in a more sane way (also split into addHours and addDays for convenience). - Fixed a bug introduced in Bug 8966 regarding dt truncation and dtSets->contains - Minor docs typos - Use the passed Calendar mode or default to 'Calendar' in Koha::Calendar->_mockinit. - Tests I'm writing some db-dependent tests for is_holiday, and hopefully for CalcDateDue so any rewrite/followup doesn't break things. Regards To+ Sponsored-by: Universidad Nacional de Córdoba
Testing: - Set useDaysMode = Datedue - Set circulation rules so the datedue matches a holiday (calculated in straight days) - Proceed to checkout: - Without the patch the due date matches the straight days calculation - With the patch the due date matches the next open day
Created attachment 13571 [details] test plan and results of testing Hi Tomas, in my tests your patch results in a great improvement. There is only one exception - the exceptions. Before your patch the functionality is totally broken and I think we need to fix this before release. I can't be trusted much around calendars and timezones, so I wrote all my test cases up in a PDF document - hope it's understandable and helpful. Note: I didn't test with hourly loans, because I am not totally sure how it should work.
On bug 9074 I added some db-dependent tests that expose exceptions do not work. I also filled a bug for exceptions (bug 9078)
Created attachment 13591 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited) useDaysMode=Datedue wasn't used as advertised in the docs. Added next_open_day and prev_open_day subs to Koha::Calendar and some tests for them. - Koha::Calendar->addDate was rewritten in a more sane way (also split into addHours and addDays for convenience). - Fixed a bug introduced in Bug 8966 regarding dt truncation and dtSets->contains - Minor docs typos - Use the passed Calendar mode or default to 'Calendar' in Koha::Calendar->_mockinit. - Tests I'm writing some db-dependent tests for is_holiday, and hopefully for CalcDateDue so any rewrite/followup doesn't break things. Regards To+ Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Tested in conjunction with the patch for bug 9078.
Created attachment 13605 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited) useDaysMode=Datedue wasn't used as advertised in the docs. Added next_open_day and prev_open_day subs to Koha::Calendar and some tests for them. - Koha::Calendar->addDate was rewritten in a more sane way (also split into addHours and addDays for convenience). - Fixed a bug introduced in Bug 8966 regarding dt truncation and dtSets->contains - Minor docs typos - Use the passed Calendar mode or default to 'Calendar' in Koha::Calendar->_mockinit. - Tests I'm writing some db-dependent tests for is_holiday, and hopefully for CalcDateDue so any rewrite/followup doesn't break things. Regards To+ Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 13619 [details] test plan and testing results (2) The new patch scores full points in my tests.
Created attachment 13620 [details] [review] Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)
QA comments: * passes koha-qa.pl * t/Calendar.pm is successfull * very very good test plan & tests results, kf++++ * nothing spotted in the code quality itself passed QA !
This patch has been pushed to master.
Patch pushed to branch 3.10.x
Pushed to 3.8.x will be in 3.8.8
Patches do not apply to 3.6.x, please reopen and submit a patch for 3.6.x if necessary. Thanks!