Description
Dobrica Pavlinusic
2012-07-11 13:25:37 UTC
Created attachment 10769 [details] [review] Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays This is simple fix since day_month_closed_days hash has wrong arguments order. Created attachment 10770 [details] [review] Bug 8418 - fix dt->truncate( to => 'day' ) Created attachment 10773 [details] [review] Bug 8418 - fix dt->truncate( to => 'day' ) Created attachment 10774 [details] [review] Bug 8418 - single_holidays DateTime::Set We where cargo-culting code from C4::Calendar, but in the process code executed same SQL query with isexceptional flag set each time. This basically disabled all entries on calendar without it, which is somewhat of a problem, since users currently can't add entry which does have isexception bit set. There is really no need for two separate DateTime::Set objects, since code for them is same if we remove isexception from query. 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. Created attachment 10810 [details] [review] Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays This is simple fix since day_month_closed_days hash has wrong arguments order. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> http://bugs.koha-community.org/show_bug.cgi?id=8414 Created attachment 10811 [details] [review] Bug 8418 - fix dt->truncate( to => 'day' ) Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> 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 Created attachment 10838 [details] [review] Bug 8418 - correctly mock test data Created attachment 10839 [details] [review] Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays This is simple fix since day_month_closed_days hash has wrong arguments order and it's not used outside Koha::Calenar code. 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?
Created attachment 10840 [details] [review] Bug 8418 - single_holidays DateTime::Set We where cargo-culting code from C4::Calendar, but in the process code executed same SQL query with isexceptional flag set each time. This basically disabled all entries on calendar without it, which is somewhat of a problem, since users currently can't add entry which does have isexception bit set. There is really no need for two separate DateTime::Set objects, since code for them is same if we remove isexception from query. 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. 'coding guidelines' and 'shouldn't' make me worry again ;) It shouldn't. This module respect them :-) Created attachment 11025 [details] [review] Bug 8418 - single_holidays DateTime::Set We where cargo-culting code from C4::Calendar, but in the process code executed same SQL query with isexceptional flag set each time. This basically disabled all entries on calendar without it, which is somewhat of a problem, since users currently can't add entry which does have isexception bit set. There is really no need for two separate DateTime::Set objects, since code for them is same if we remove isexception from query. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 11026 [details] [review] Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays This is simple fix since day_month_closed_days hash has wrong arguments order and it's not used outside Koha::Calenar code. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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? isn't this patch somewhere redundant with bug 8486 ? Created attachment 11611 [details] [review] Proposed Patch The repeating holidays bug is still there. It should be corrected if the structure is setup correctly on load from the database as the TEST bersion of the object does. This fixes it in a single place unlike the previous which addressed it in the access method and meant that the test mock had to change to conform 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> 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 There is a further related data loading bug I've entered it as bug 8656 with a patch 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; } The non loading of exception holidays is addressed by the patch on bug 8656 > 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 Patch pushed to master Pushed to 3.8.x will be in 3.8.5 *** Bug 6398 has been marked as a duplicate of this bug. *** |