Bug 14522: Use Koha::Cache for accessing single_holidays()

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: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
This commit is contained in:
Mason James 2015-07-14 22:53:10 +12:00 committed by Tomas Cohen Arazi
parent 4a43c38205
commit 5d256e6b4b
5 changed files with 180 additions and 59 deletions

View File

@ -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;
}

View File

@ -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(
'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' );
}
$self->{single_holidays}{$branchcode} = DateTime::Set->from_datetimes( dates => $dates );
$single_holidays->{$branchcode} = $self->{single_holidays}{$branchcode};
return $single_holidays->{$branchcode};
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 @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 ;
}
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);

View File

@ -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');

View File

@ -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");

View File

@ -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') ;
}