Bug 8800

Summary: SpecifyDueDate && useDaysMode=Datedue wrong behaviour
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: CirculationAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Mason James <mtj>
Severity: blocker    
Priority: P5 - low CC: chris, elliott, gmcharlt, jcamins, katrin.fischer, kyle.m.hall, kyle, liz, mtj, paul.poulain
Version: 3.8   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 9074, 9078    
Attachments: Bug 8800 - useDaysMode=Datedue wrong behaviour
Bug 8800 - useDaysMode=Datedue wrong behaviour
Bug 8800 - useDaysMode=Datedue wrong behaviour
useDaysMode='Calendar' should make date due be the next open day too.
Bug 8800 - useDaysMode=Datedue wrong behaviour
Bug 8800 - useDaysMode=Datedue wrong behaviour (3.8.x)
Bug 8800 - useDaysMode=Datedue wrong behaviour (3.8.x)
Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)
Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)
Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)
Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)
test plan and results of testing
Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)
Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)
test plan and testing results (2)
Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)

Description Tomás Cohen Arazi 2012-09-21 12:42:43 UTC
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.
Comment 1 Tomás Cohen Arazi 2012-09-21 17:50:53 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2012-09-21 17:57:27 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2012-09-21 18:25:12 UTC
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.
Comment 4 Tomás Cohen Arazi 2012-09-21 18:43:20 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2012-09-21 19:31:47 UTC Comment hidden (obsolete)
Comment 6 Elliott Davis 2012-09-21 20:42:46 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2012-09-26 13:48:21 UTC Comment hidden (obsolete)
Comment 8 Federico Rinaudo 2012-09-28 17:47:30 UTC Comment hidden (obsolete)
Comment 9 Mason James 2012-10-03 22:28:18 UTC
(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
Comment 10 Paul Poulain 2012-10-08 14:22:09 UTC
(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.
Comment 11 Tomás Cohen Arazi 2012-10-18 19:43:47 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2012-11-09 19:26:00 UTC
 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
Comment 13 Chris Cormack 2012-11-09 19:42:45 UTC Comment hidden (obsolete)
Comment 14 Paul Poulain 2012-11-12 17:16:09 UTC
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
Comment 15 Paul Poulain 2012-11-12 17:16:30 UTC Comment hidden (obsolete)
Comment 16 Jared Camins-Esakov 2012-11-13 00:56:03 UTC
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?
Comment 17 Tomás Cohen Arazi 2012-11-13 01:47:54 UTC
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.
Comment 18 Jared Camins-Esakov 2012-11-13 01:49:44 UTC
Okay, that makes sense. What is the exact procedure I should use for testing the automatic calculation?
Comment 19 Tomás Cohen Arazi 2012-11-14 14:33:02 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2012-11-14 14:38:06 UTC
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
Comment 21 Katrin Fischer 2012-11-21 07:27:50 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2012-11-21 11:52:39 UTC
On bug 9074 I added some db-dependent tests that expose exceptions do not work. I also filled a bug for exceptions (bug 9078)
Comment 23 Kyle M Hall 2012-11-21 14:55:11 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2012-11-21 14:56:23 UTC
Tested in conjunction with the patch for bug 9078.
Comment 25 Kyle M Hall 2012-11-21 17:25:28 UTC Comment hidden (obsolete)
Comment 26 Katrin Fischer 2012-11-21 22:05:15 UTC
Created attachment 13619 [details]
test plan and testing results (2)

The new patch scores full points in my tests.
Comment 27 Katrin Fischer 2012-11-21 22:11:00 UTC
Created attachment 13620 [details] [review]
Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)
Comment 28 Paul Poulain 2012-11-22 08:55:51 UTC
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 !
Comment 29 Jared Camins-Esakov 2012-11-22 14:27:23 UTC
This patch has been pushed to master.
Comment 30 Paul Poulain 2012-11-22 15:41:48 UTC
Patch pushed to branch 3.10.x
Comment 31 Chris Cormack 2012-11-26 00:29:20 UTC
Pushed to 3.8.x will be in 3.8.8
Comment 32 Liz Rea 2012-11-28 01:44:18 UTC
Patches do not apply to 3.6.x, please reopen and submit a patch for 3.6.x if necessary. 

Thanks!