Description
Tomás Cohen Arazi (tcohen)
2012-09-21 12:42:43 UTC
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! |