Bug 8418

Summary: Koha::Calendar is_holiday ignores all user data
Product: Koha Reporter: Dobrica Pavlinusic <dpavlin>
Component: CirculationAssignee: Bugs List <koha-bugs>
Status: CLOSED FIXED QA Contact: Mason James <mtj>
Severity: critical    
Priority: P1 - high CC: chris, colin.campbell, dpavlin, gmcharlt, katrin.fischer, koha.sekjal, kyle.m.hall, kyle, leonardobareno, mtj, oleonard, paul.poulain
Version: 3.8   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8251
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 6398, 8110, 8348    
Attachments: Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays
Bug 8418 - fix dt->truncate( to => 'day' )
Bug 8418 - fix dt->truncate( to => 'day' )
Bug 8418 - single_holidays DateTime::Set
Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays
Bug 8418 - fix dt->truncate( to => 'day' )
Bug 8418 - correctly mock test data
Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays
Bug 8418 - single_holidays DateTime::Set
Bug 8418 - single_holidays DateTime::Set
Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays
Proposed Patch
Bug 8418: load repeating holidays hash correctly

Description Dobrica Pavlinusic 2012-07-11 13:25:37 UTC

    
Comment 1 Dobrica Pavlinusic 2012-07-11 13:30:26 UTC Comment hidden (obsolete)
Comment 2 Dobrica Pavlinusic 2012-07-11 14:00:50 UTC Comment hidden (obsolete)
Comment 3 Dobrica Pavlinusic 2012-07-11 16:44:17 UTC Comment hidden (obsolete)
Comment 4 Dobrica Pavlinusic 2012-07-11 16:45:02 UTC Comment hidden (obsolete)
Comment 5 Dobrica Pavlinusic 2012-07-11 16:49:08 UTC
This bug duplicates some parts of following bugs which are not signed off:

Bug 8110 - Fines accruing on closed days
Bug 8348 - useDaysMode syspref does not apply

It modifies only Koha::Calendar code, making is simper and faster (saving one SQL query as side-effect) and basically brings back Koha behaviour before we decided to move C4::Calendar into Koha::Calendar and broke stuff.
Comment 6 Chris Cormack 2012-07-13 09:25:02 UTC Comment hidden (obsolete)
Comment 7 Chris Cormack 2012-07-13 09:26:23 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2012-07-13 10:15:31 UTC
The tests now fail

Before the patches
 perl t/Kalendar.t                            
1..9
ok 1 - use Koha::Calendar;
ok 2 - Calendar class returned isa Koha::Calendar
ok 3 - Sunday is a closed day
ok 4 - Monday is not a closed day
ok 5 - month/day closed day test
ok 6 - special closed day test
ok 7 - open day test
ok 8 - addDate skips closed Sunday
ok 9 - Negative call to addDate

Applying the first patch
perl t/Kalendar.t                                     
1..9
ok 1 - use Koha::Calendar;
ok 2 - Calendar class returned isa Koha::Calendar
ok 3 - Sunday is a closed day
ok 4 - Monday is not a closed day
not ok 5 - month/day closed day test
#   Failed test 'month/day closed day test'
#   at t/Kalendar.t line 54.
#          got: '0'
#     expected: '1'
ok 6 - special closed day test
ok 7 - open day test
ok 8 - addDate skips closed Sunday
ok 9 - Negative call to addDate
# Looks like you failed 1 test of 9.


Applying the 2nd patch
Still failing 5

Applying the final one
 perl t/Kalendar.t                                     
1..9
ok 1 - use Koha::Calendar;
ok 2 - Calendar class returned isa Koha::Calendar
ok 3 - Sunday is a closed day
ok 4 - Monday is not a closed day
not ok 5 - month/day closed day test
#   Failed test 'month/day closed day test'
#   at t/Kalendar.t line 54.
#          got: '0'
#     expected: '1'
not ok 6 - special closed day test
#   Failed test 'special closed day test'
#   at t/Kalendar.t line 55.
#          got: '0'
#     expected: '1'
ok 7 - open day test
ok 8 - addDate skips closed Sunday
ok 9 - Negative call to addDate
# Looks like you failed 2 tests of 9

I can fix the test for the first fail

By changing this
-    $self->{day_month_closed_days} = { 6 => { 16 => 1, } };
+    $self->{day_month_closed_days} = { 16 => { 6 => 1, } };


But the test still fails for special holidays, I think this is actually broken.

Also are we certain everywhere is specifying holidays as day month, not month day as the test and code was before this change?

Its too late in the night for me to go much further with this, I hope someone else has a chance to look at it
Comment 9 Dobrica Pavlinusic 2012-07-14 09:51:35 UTC Comment hidden (obsolete)
Comment 10 Dobrica Pavlinusic 2012-07-14 10:04:47 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2012-07-14 10:09:35 UTC
Hi Dobrica, 

I am a bit worried about Chris' question:

>Also are we certain everywhere is specifying holidays as day month, not month >day as the test and code was before this change?

So maybe he patches are breaking things in other places now? Is there a way to check/make sure?
Comment 12 Dobrica Pavlinusic 2012-07-14 10:12:23 UTC Comment hidden (obsolete)
Comment 13 Dobrica Pavlinusic 2012-07-14 10:20:47 UTC
Rest of the code doesn't use (In reply to comment #11)
> Hi Dobrica, 
> 
> So maybe he patches are breaking things in other places now? Is there a way
> to check/make sure?

I did that before I started modifing code.

git grep day_month_closed_days

is quite useful to get all places in code without need to write elaborate grep statements. To be honest, I switched to git grep from my own hand-crafted greps in this bug, so I'm sharing my excitement here :-)

According to our coding guidelines, other code shouldn't access values directly in Koha namespace anyway, which is probably only reason why we are mocking test
data within module itself (which was strange to me at first, and is probably reason why I broke it).

I cleaned up two patches which to include correct mocking of test data.
Comment 14 Katrin Fischer 2012-07-14 10:53:36 UTC
'coding guidelines' and 'shouldn't' make me worry again ;)
Comment 15 Dobrica Pavlinusic 2012-07-16 18:09:29 UTC
It shouldn't. This module respect them :-)
Comment 16 Kyle M Hall 2012-07-19 18:35:10 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2012-07-19 18:36:25 UTC Comment hidden (obsolete)
Comment 18 Ian Walls 2012-07-20 17:18:38 UTC
Patch 1:  correct 'to' value of truncate function is 'day', not 'days', but the patch no longer applies.

Patch 2:  looks to be removing 'singleholiday' and 'exceptionholiday' references in favour of 'specialholiday'.  Also, patch does not apply anymore.

Patch 3:  swaps around hash from 'month' => 'day' to 'day' => 'month', which seems backwards as far as granularity goes.  I remember us switching this for a reason... was that reason half-baked?  Or did we just not switch it in enough places?
Comment 19 Paul Poulain 2012-08-02 14:56:31 UTC
isn't this patch somewhere redundant with bug 8486 ?
Comment 20 Colin Campbell 2012-08-14 14:47:30 UTC Comment hidden (obsolete)
Comment 21 Chris Cormack 2012-08-14 23:27:10 UTC
Created attachment 11624 [details] [review]
Bug 8418: load repeating holidays hash correctly

The repeating holidays structure was being loaded in
a different order to how it was tested

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 22 Chris Cormack 2012-08-14 23:29:15 UTC
perl t/Calendar.t                      2012-08-15 11:28:50 chrisc pts/26
1..21
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 - test larger intervals
ok 17 - test positive intervals
ok 18 - test parameter order not relevant
ok 19 - days_between calculates correctly
ok 20 - holiday correctly recognized
ok 21 - multiple holidays correctly recognized
Comment 23 Colin Campbell 2012-08-20 14:39:28 UTC
There is a further related data loading bug I've entered it as bug 8656 with a patch
Comment 24 L. Bareño 2012-09-06 04:15:55 UTC
1) Current code in ./Koha/Calendar.pm is ignoring single holidays (those whose special_holidays.isexception == 0)

(line numbers and code from master at this message's date)

./Koha/Calendar.pm

line 68:
- $special->execute( $branch, 1 );
+ $special->execute( $branch, 0 );


2) exception_holidays are not holidays, and (please you people tell me if the following interpretation I'm doing is correct) these must disable any other repeatable or single holiday.

lines 141-165 (suggested code):
sub is_holiday {
    my ( $self, $dt ) = @_;
    if ( $self->{exception_holidays}->contains($dt) ) {
        return 0;
    }
    my $dow = $dt->day_of_week;
    if ( $dow == 7 ) {
        $dow = 0;
    }
    if ( $self->{weekly_closed_days}->[$dow] == 1 ) {
        return 1;
    }
    $dt->truncate( to => 'day' );
    my $day   = $dt->day;
    my $month = $dt->month;
    if ( exists $self->{day_month_closed_days}->{$month}->{$day} ) {
        return 1;
    }
    if ( $self->{single_holidays}->contains($dt) ) {
        return 1;
    }

    # damn have to go to work after all
    return 0;
}
Comment 25 Colin Campbell 2012-09-06 07:05:35 UTC
The non loading of exception holidays is addressed by the patch on bug 8656
Comment 26 Mason James 2012-09-14 05:17:09 UTC
> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>


patch looks good, passing QA...

$ koha-qa.pl

testing 1 commit(s) (applied to commit 60790d0)
 * 1230d06 Bug 8418: load repeating holidays hash correctly
      Koha/Calendar.pm

* Koha/Calendar.pm                                                         OK
Comment 27 Paul Poulain 2012-09-14 12:53:24 UTC
Patch pushed to master
Comment 28 Chris Cormack 2012-09-15 20:00:05 UTC
Pushed to 3.8.x will be in 3.8.5
Comment 29 Tomás Cohen Arazi 2012-09-20 17:36:20 UTC
*** Bug 6398 has been marked as a duplicate of this bug. ***