Bug 8418 - Koha::Calendar is_holiday ignores all user data
Summary: Koha::Calendar is_holiday ignores all user data
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P1 - high critical (vote)
Assignee: Bugs List
QA Contact: Mason James
URL:
Keywords:
: 6398 (view as bug list)
Depends on:
Blocks: 6398 8110 8348
  Show dependency treegraph
 
Reported: 2012-07-11 13:25 UTC by Dobrica Pavlinusic
Modified: 2019-06-27 09:24 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays (886 bytes, patch)
2012-07-11 13:30 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8418 - fix dt->truncate( to => 'day' ) (952 bytes, patch)
2012-07-11 14:00 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8418 - fix dt->truncate( to => 'day' ) (1.24 KB, patch)
2012-07-11 16:44 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8418 - single_holidays DateTime::Set (3.35 KB, patch)
2012-07-11 16:45 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays (995 bytes, patch)
2012-07-13 09:25 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8418 - fix dt->truncate( to => 'day' ) (1.29 KB, patch)
2012-07-13 09:26 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8418 - correctly mock test data (1.14 KB, patch)
2012-07-14 09:51 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays (1.29 KB, patch)
2012-07-14 10:04 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8418 - single_holidays DateTime::Set (3.43 KB, patch)
2012-07-14 10:12 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8418 - single_holidays DateTime::Set (3.49 KB, patch)
2012-07-19 18:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays (1.35 KB, patch)
2012-07-19 18:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Proposed Patch (943 bytes, patch)
2012-08-14 14:47 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 8418: load repeating holidays hash correctly (984 bytes, patch)
2012-08-14 23:27 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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. ***