From 19c8e4ee46e41cf4b01b25000ebd623aa85c5660 Mon Sep 17 00:00:00 2001 From: Mason James Date: Tue, 14 Jul 2015 22:53:10 +1200 Subject: [PATCH] Bug 14522 - Use Koha::Cache for accessing single_holidays() Content-Type: text/plain; charset="utf-8" this patch adds Koha::Cache functionality to the 'single_holidays' table it is a performance patch for the problem described in BZ14315, only http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14315 it replaces slooow DateTime holiday objects with simple Ymd strings (19991230), then stores the strings in an @array using Koha::Cache it does not attempt to add caching to all holiday tables - just the single_holidays table (at this stage on my test (master-cd9a827); nytprof showed a time reduction of the single_holidays() sub - from 61.7s to 587ms here are some before/after nytprof runs, (really on master-cd9a827, not 3.20) http://x1.kohaaloha.com/i/nyt-bz14522-before/home-mason-g-k-3-20-x-Koha-Calendar-pm-1485-line.html#237 http://x1.kohaaloha.com/i/nyt-bz14522-after/home-mason-g-k-3-20-x-Koha-Calendar-pm-1485-line.html#280 to test... 1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday rows) 2/ add a loong circ rule for an itemtype (my rule has 140 days) 3/ checkout an item to a user (took me 67 secs) apply patch... 4/ return item 5/ repeats steps 1..3, (took me 6 secs) 6/ add/change/delete some various single_holidays, via Home->Tools->Calendar ensure that your various changes have indeed saved correctly for extra points... 7/ run tests t/Calendar.t and t/db_dependent/Holidays.t, with all tests pass OK sudo koha-shell -c ' export PERL5LIB=/home/mason/g/k/master ; \ cd /home/mason/g/k/master ; perl t/Calendar.t ; perl t/db_dependent/Holidays.t ' testkoha 8/ run QA tool, with all tests pass OK sudo koha-shell -c ' \ export KOHA_CONF=/etc/koha/sites/mayo2/koha-conf.xml \ export PERL5LIB=/home/mason/g/k/master:/home/mason/qa-test-tools/ ; \ cd /home/mason/g/k/master ; perl /home/mason/qa-test-tools/koha-qa.pl -c 1 ' testkoha Signed-off-by: Mason James --- C4/Calendar.pm | 56 ++++++++++++++++++--- Koha/Calendar.pm | 125 ++++++++++++++++++++++++++++++++++------------ t/Calendar.t | 37 ++++++++------ t/db_dependent/Holidays.t | 9 ++-- tools/newHolidays.pl | 8 +++ 5 files changed, 178 insertions(+), 57 deletions(-) diff --git a/C4/Calendar.pm b/C4/Calendar.pm index dfee6b3..1ed7c63 100644 --- a/C4/Calendar.pm +++ b/C4/Calendar.pm @@ -23,8 +23,10 @@ use Carp; use Date::Calc qw( Date_to_Days Today); use C4::Context; +use Koha::Cache; use constant ISO_DATE_FORMAT => "%04d-%02d-%02d"; + =head1 NAME C4::Calendar::Calendar - Koha module dealing with holidays. @@ -262,7 +264,6 @@ C<$description> Is the description to store for the holiday formed by $year/$mon sub insert_single_holiday { my $self = shift @_; my %options = @_; - @options{qw(year month day)} = ( $options{date} =~ m/(\d+)-(\d+)-(\d+)/o ) if $options{date} && !$options{day}; @@ -272,7 +273,14 @@ sub insert_single_holiday { $insertHoliday->execute( $self->{branchcode}, $options{day},$options{month},$options{year}, $isexception, $options{title}, $options{description}); $self->{'single_holidays'}->{"$options{year}/$options{month}/$options{day}"}{title} = $options{title}; $self->{'single_holidays'}->{"$options{year}/$options{month}/$options{day}"}{description} = $options{description}; + + + # changed the 'single_holidays' table, lets force/reset its cache + my $cache = Koha::Cache->get_instance(); + $cache->clear_from_cache( 'single_holidays') ; + return $self; + } =head2 insert_exception_holiday @@ -310,6 +318,11 @@ sub insert_exception_holiday { $insertException->execute( $self->{branchcode}, $options{day},$options{month},$options{year}, $isexception, $options{title}, $options{description}); $self->{'exception_holidays'}->{"$options{year}/$options{month}/$options{day}"}{title} = $options{title}; $self->{'exception_holidays'}->{"$options{year}/$options{month}/$options{day}"}{description} = $options{description}; + + # changed the 'single_holidays' table, lets force/reset its cache + my $cache = Koha::Cache->get_instance(); + $cache->clear_from_cache( 'single_holidays') ; + return $self; } @@ -398,10 +411,18 @@ sub ModSingleholiday { my $dbh = C4::Context->dbh(); my $isexception = 0; - my $updateHoliday = $dbh->prepare("UPDATE special_holidays SET title = ?, description = ? WHERE day = ? AND month = ? AND year = ? AND branchcode = ? AND isexception = ?"); + + my $updateHoliday = $dbh->prepare(" +UPDATE special_holidays SET title = ?, description = ? + WHERE day = ? AND month = ? AND year = ? AND branchcode = ? AND isexception = ?"); $updateHoliday->execute($options{title},$options{description},$options{day},$options{month},$options{year},$self->{branchcode},$isexception); $self->{'single_holidays'}->{"$options{year}/$options{month}/$options{day}"}{title} = $options{title}; $self->{'single_holidays'}->{"$options{year}/$options{month}/$options{day}"}{description} = $options{description}; + + # changed the 'single_holidays' table, lets force/reset its cache + my $cache = Koha::Cache->get_instance(); + $cache->clear_from_cache( 'single_holidays') ; + return $self; } @@ -433,10 +454,17 @@ sub ModExceptionholiday { my $dbh = C4::Context->dbh(); my $isexception = 1; - my $updateHoliday = $dbh->prepare("UPDATE special_holidays SET title = ?, description = ? WHERE day = ? AND month = ? AND year = ? AND branchcode = ? AND isexception = ?"); - $updateHoliday->execute($options{title},$options{description},$options{day},$options{month},$options{year},$self->{branchcode},$isexception); + my $updateHoliday = $dbh->prepare(" +UPDATE special_holidays SET title = ?, description = ? + WHERE day = ? AND month = ? AND year = ? AND branchcode = ? AND isexception = ?"); + $updateHoliday->execute($options{title},$options{description},$options{day},$options{month},$options{year},$self->{branchcode},$isexception); $self->{'exception_holidays'}->{"$options{year}/$options{month}/$options{day}"}{title} = $options{title}; $self->{'exception_holidays'}->{"$options{year}/$options{month}/$options{day}"}{description} = $options{description}; + + # changed the 'single_holidays' table, lets force/reset its cache + my $cache = Koha::Cache->get_instance(); + $cache->clear_from_cache( 'single_holidays') ; + return $self; } @@ -465,7 +493,7 @@ sub delete_holiday { # Verify what kind of holiday that day is. For example, if it is # a repeatable holiday, this should check if there are some exception - # for that holiday rule. Otherwise, if it is a regular holiday, it´s + # for that holiday rule. Otherwise, if it is a regular holiday, it´s # ok just deleting it. my $dbh = C4::Context->dbh(); @@ -512,6 +540,11 @@ sub delete_holiday { } } } + + # changed the 'single_holidays' table, lets force/reset its cache + my $cache = Koha::Cache->get_instance(); + $cache->clear_from_cache( 'single_holidays') ; + return $self; } =head2 delete_holiday_range @@ -537,6 +570,11 @@ sub delete_holiday_range { my $dbh = C4::Context->dbh(); my $sth = $dbh->prepare("DELETE FROM special_holidays WHERE (branchcode = ?) AND (day = ?) AND (month = ?) AND (year = ?)"); $sth->execute($self->{branchcode}, $options{day}, $options{month}, $options{year}); + + # changed the 'single_holidays' table, lets force/reset its cache + my $cache = Koha::Cache->get_instance(); + $cache->clear_from_cache( 'single_holidays') ; + } =head2 delete_holiday_range_repeatable @@ -585,6 +623,10 @@ sub delete_exception_holiday_range { my $dbh = C4::Context->dbh(); my $sth = $dbh->prepare("DELETE FROM special_holidays WHERE (branchcode = ?) AND (isexception = 1) AND (day = ?) AND (month = ?) AND (year = ?)"); $sth->execute($self->{branchcode}, $options{day}, $options{month}, $options{year}); + + # changed the 'single_holidays' table, lets force/reset its cache + my $cache = Koha::Cache->get_instance(); + $cache->clear_from_cache( 'single_holidays') ; } =head2 isHoliday @@ -606,7 +648,7 @@ sub isHoliday { $month=$month+0; $year=$year+0; $day=$day+0; - my $weekday = &Date::Calc::Day_of_Week($year, $month, $day) % 7; + my $weekday = &Date::Calc::Day_of_Week($year, $month, $day) % 7; my $weekDays = $self->get_week_days_holidays(); my $dayMonths = $self->get_day_month_holidays(); my $exceptions = $self->get_exception_holidays(); @@ -617,7 +659,7 @@ sub isHoliday { if ((exists($weekDays->{$weekday})) || (exists($dayMonths->{"$month/$day"})) || (exists($singles->{"$year/$month/$day"}))) { - return 1; + return 1; } else { return 0; } diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm index 27a02b5..6be7c16 100644 --- a/Koha/Calendar.pm +++ b/Koha/Calendar.pm @@ -7,6 +7,7 @@ use DateTime; use DateTime::Set; use DateTime::Duration; use C4::Context; +use Koha::Cache; use Carp; sub new { @@ -57,7 +58,9 @@ sub _init { # is allowing this with the expectation that prior to release of # 3.16, bug 8089 will be fixed and we can switch the caching over # to Koha::Cache. -our ( $exception_holidays, $single_holidays ); + +our ( $exception_holidays, $single_holidays ); ## no need for $single_holidays now, surely? + sub exception_holidays { my ( $self ) = @_; my $dbh = C4::Context->dbh; @@ -87,31 +90,67 @@ sub exception_holidays { } sub single_holidays { - my ( $self ) = @_; - my $dbh = C4::Context->dbh; + my ( $self, $date ) = @_; my $branchcode = $self->{branchcode}; - if ( $single_holidays->{$branchcode} ) { - $self->{single_holidays}{$branchcode} = $single_holidays->{$branchcode}; - return $single_holidays->{$branchcode}; - } - my $single_holidays_sth = $dbh->prepare( + my $cache = Koha::Cache->get_instance(); + my $single_holidays = $cache->get_from_cache('single_holidays'); + +=c +$single_holidays looks like this.. + +\ { + CPL [ + [0] 20131122, + [1] 20131123, + [2] 20131124 + ], + MPL [ + [0] 20131122, + [1] 20131123, + [2] 20131124 + ] +} + +=cut + + unless ($single_holidays) { + my $dbh = C4::Context->dbh; + $single_holidays = {}; + + # push holidays for each branch + my $branches_sth = + $dbh->prepare('SELECT distinct(branchcode) FROM special_holidays'); + $branches_sth->execute(); + while ( my $br = $branches_sth->fetchrow ) { + my $single_holidays_sth = $dbh->prepare( 'SELECT day, month, year FROM special_holidays WHERE branchcode = ? AND isexception = 0' - ); - $single_holidays_sth->execute( $branchcode ); - my $dates = []; - while ( my ( $day, $month, $year ) = $single_holidays_sth->fetchrow ) { - push @{$dates}, - DateTime->new( - day => $day, - month => $month, - year => $year, - time_zone => C4::Context->tz() - )->truncate( to => 'day' ); + ); + $single_holidays_sth->execute($branchcode); + + my @ymd_arr; + while ( my ( $day, $month, $year ) = + $single_holidays_sth->fetchrow ) + { + my $dt = DateTime->new( + day => $day, + month => $month, + year => $year, + time_zone => C4::Context->tz() + )->truncate( to => 'day' ); + push @ymd_arr, $dt->ymd(''); + } + $single_holidays->{$br} = \@ymd_arr; + } # br + $cache->set_in_cache( 'single_holidays', $single_holidays, + 76800 ) #24 hrs ; } - $self->{single_holidays}{$branchcode} = DateTime::Set->from_datetimes( dates => $dates ); - $single_holidays->{$branchcode} = $self->{single_holidays}{$branchcode}; - return $single_holidays->{$branchcode}; + my $holidays = ( $single_holidays->{$branchcode} ); + for my $hols (@$holidays ) { + return 1 if ( $date == $hols ) #match ymds; + } + return 0; } + sub addDate { my ( $self, $startdate, $add_duration, $unit ) = @_; @@ -193,6 +232,7 @@ sub addDays { # Datedue, then use the calendar to push # the date to the next open day if holiday if ( $self->is_holiday($base_date) ) { + if ( $days_duration->is_negative() ) { $base_date = $self->prev_open_day($base_date); } else { @@ -207,12 +247,14 @@ sub addDays { sub is_holiday { my ( $self, $dt ) = @_; + my $localdt = $dt->clone(); my $day = $localdt->day; my $month = $localdt->month; $localdt->truncate( to => 'day' ); + if ( $self->exception_holidays->contains($localdt) ) { # exceptions are not holidays return 0; @@ -234,7 +276,8 @@ sub is_holiday { return 1; } - if ( $self->single_holidays->contains($localdt) ) { + my $ymd = $localdt->ymd('') ; + if ($self->single_holidays( $ymd ) == 1 ) { return 1; } @@ -340,19 +383,28 @@ sub clear_weekly_closed_days { return; } -sub add_holiday { - my $self = shift; - my $new_dt = shift; - my @dt = $self->single_holidays->as_list; - push @dt, $new_dt; - my $branchcode = $self->{branchcode}; - $self->{single_holidays}{$branchcode} = - DateTime::Set->from_datetimes( dates => \@dt ); - $single_holidays->{$branchcode} = $self->{single_holidays}{$branchcode}; - return; +sub add_dummy_holiday { + my ( $self, $new_dt ) = @_; + + my $cache = Koha::Cache->get_instance(); + my $single_holidays = $cache->get_from_cache('single_holidays'); + + # add a dummy holiday to the holiday cache... + my $ymd = $new_dt->ymd(''); + $single_holidays->{'MPL'} = [$ymd]; + $cache->set_in_cache( 'single_holidays', $single_holidays, 76800 ); + + # ...but *dont* reset the cache, as this holiday was not really written to the db + # its only used to mock a holiday insert for 1 test in t/db_dependent/Holidays.t + + # is( $koha_calendar->is_holiday($custom_holiday), 0, '2013-11-10 does not start off as a holiday' ); + # $koha_calendar->add_dummy_holiday($custom_holiday ); + # is( $koha_calendar->is_holiday($custom_holiday), 1, 'able to add holiday for testing' ); + } + 1; __END__ @@ -429,6 +481,13 @@ Currently unit is only used to invoke Staffs return Monday at 10 am rule this parameter will be removed when issuingrules properly cope with that +=head2 single_holidays + +my $rc = $self->single_holidays( $ymd ); + +Passed a $date in Ymd (yyyymmdd) format - returns 1 if date is a single_holiday, or 0 if not. + + =head2 is_holiday $yesno = $calendar->is_holiday($dt); diff --git a/t/Calendar.t b/t/Calendar.t index 8648968..758f60b 100755 --- a/t/Calendar.t +++ b/t/Calendar.t @@ -4,9 +4,10 @@ use strict; use warnings; use DateTime; use DateTime::Duration; -use Test::More tests => 35; +use Test::More tests => 34; use Test::MockModule; use DBD::Mock; +use Koha::Cache; use Koha::DateUtils; BEGIN { @@ -14,7 +15,7 @@ BEGIN { # This was the only test C4 had # Remove when no longer used - use_ok('C4::Calendar'); + #use_ok('C4::Calendar'); # not used anymore? } my $module_context = new Test::MockModule('C4::Context'); @@ -66,20 +67,35 @@ my $holidays_session = DBD::Mock::Session->new('holidays_session' => ( [ 11, 11, 2012 ] # sunday exception ] }, - { # single holidays + + { # single holidays1 + statement => "SELECT distinct(branchcode) FROM special_holidays", + results => [ + [ 'branchcode' ], + [ 'MPL'] + ] + }, + + { # single holidays2 statement => "SELECT day, month, year FROM special_holidays WHERE branchcode = ? AND isexception = 0", results => [ [ 'day', 'month', 'year' ], [ 1, 6, 2011 ], # single holiday [ 4, 7, 2012 ] ] - } + }, )); # Initialize the global $dbh variable my $dbh = C4::Context->dbh(); # Apply the mock session $dbh->{ mock_session } = $holidays_session; + + +my $cache = Koha::Cache->get_instance(); +$cache->clear_from_cache( 'single_holidays') ; + + # 'MPL' branch is arbitrary, is not used at all but is needed for initialization my $cal = Koha::Calendar->new( branchcode => 'MPL' ); @@ -134,7 +150,6 @@ my $day_after_christmas = DateTime->new( day => 26 ); # for testing negative addDate - { # Syspref-agnostic tests is ( $saturday->day_of_week, 6, '\'$saturday\' is actually a saturday (6th day of week)'); is ( $sunday->day_of_week, 7, '\'$sunday\' is actually a sunday (7th day of week)'); @@ -148,7 +163,6 @@ my $day_after_christmas = DateTime->new( is ( $cal->is_holiday($sunday_exception), 0, 'Exception holiday is not a closed day test' ); } - { # Bugzilla #8966 - is_holiday truncates referenced date my $later_dt = DateTime->new( # Monday year => 2012, @@ -164,7 +178,6 @@ my $day_after_christmas = DateTime->new( cmp_ok( $later_dt, 'eq', '2012-09-17T17:30:00', 'bz-8966 (2/2) Date should be the same after is_holiday' ); } - { # Bugzilla #8800 - is_holiday should use truncated date for 'contains' call my $single_holiday_time = DateTime->new( year => 2011, @@ -179,12 +192,12 @@ my $day_after_christmas = DateTime->new( 'bz-8800 is_holiday should truncate the date for holiday validation' ); } - my $one_day_dur = DateTime::Duration->new( days => 1 ); my $two_day_dur = DateTime::Duration->new( days => 2 ); my $seven_day_dur = DateTime::Duration->new( days => 7 ); - my $dt = dt_from_string( '2012-07-03','iso' ); + my $dt = dt_from_string( '2012-07-03','iso' ); #tuesday + my $test_dt = DateTime->new( # Monday year => 2012, month => 7, @@ -202,7 +215,6 @@ my $day_after_christmas = DateTime->new( time_zone => 'Europe/London', ); - { ## 'Datedue' tests $module_context->unmock('preference'); @@ -218,7 +230,7 @@ my $day_after_christmas = DateTime->new( $cal = Koha::Calendar->new( branchcode => 'MPL' ); - is($cal->addDate( $dt, $one_day_dur, 'days' ), + is($cal->addDate( $dt, $one_day_dur, 'days' ), # tuesday dt_from_string('2012-07-05','iso'), 'Single day add (Datedue, matches holiday, shift)' ); @@ -243,11 +255,8 @@ my $day_after_christmas = DateTime->new( cmp_ok( $cal->days_between( $later_dt, $test_dt )->in_units('days'), '==', 40, 'Test parameter order not relevant (Days)' ); - - } - { ## 'Calendar' tests' $module_context->unmock('preference'); diff --git a/t/db_dependent/Holidays.t b/t/db_dependent/Holidays.t index dc7e5a6..299893d 100755 --- a/t/db_dependent/Holidays.t +++ b/t/db_dependent/Holidays.t @@ -3,12 +3,13 @@ use warnings; use 5.010; use DateTime; use DateTime::TimeZone; +use Test::More tests => 12; use C4::Context; -use Koha::DateUtils; -use Test::More tests => 12; use C4::Branch; +use Koha::DateUtils; + BEGIN { use_ok('Koha::Calendar'); } BEGIN { use_ok('C4::Calendar'); } @@ -91,8 +92,9 @@ my $custom_holiday = DateTime->new( month => 11, day => 12, ); + is( $koha_calendar->is_holiday($custom_holiday), 0, '2013-11-10 does not start off as a holiday' ); -$koha_calendar->add_holiday($custom_holiday); +$koha_calendar->add_dummy_holiday($custom_holiday ); is( $koha_calendar->is_holiday($custom_holiday), 1, 'able to add holiday for testing' ); my $today = dt_from_string(); @@ -103,6 +105,7 @@ C4::Calendar->new( branchcode => 'CPL' )->insert_single_holiday( title => "$today", description => "$today", ); + is( Koha::Calendar->new( branchcode => 'CPL' )->is_holiday( $today ), 1, "Today is a holiday for CPL" ); is( Koha::Calendar->new( branchcode => 'MPL' )->is_holiday( $today ), 0, "Today is not a holiday for MPL"); diff --git a/tools/newHolidays.pl b/tools/newHolidays.pl index 678ee0d..23ad1bb 100755 --- a/tools/newHolidays.pl +++ b/tools/newHolidays.pl @@ -1,4 +1,6 @@ #!/usr/bin/perl +#FIXME: add a license +#FIXME: perltidy this file use strict; use warnings; @@ -8,6 +10,8 @@ use CGI qw ( -utf8 ); use C4::Auth; use C4::Output; +use Koha::Cache; + use C4::Calendar; use DateTime; @@ -82,6 +86,7 @@ if($allbranches) { print $input->redirect("/cgi-bin/koha/tools/holidays.pl?branch=$originalbranchcode&calendardate=$calendardate"); +#FIXME: move add_holiday() to a better place sub add_holiday { ($newoperation, $branchcode, $weekday, $day, $month, $year, $title, $description) = @_; my $calendar = C4::Calendar->new(branchcode => $branchcode); @@ -141,4 +146,7 @@ sub add_holiday { } } } + # we updated the single_holidays table, so wipe its cache + my $cache = Koha::Cache->get_instance(); + $cache->clear_from_cache( 'single_holidays') ; } -- 2.1.4