Bug 14522: Use Koha::Cache for accessing single_holidays()
authorMason James <mtj@kohaaloha.com>
Tue, 14 Jul 2015 10:53:10 +0000 (22:53 +1200)
committerTomas Cohen Arazi <tomascohen@unc.edu.ar>
Fri, 2 Oct 2015 14:41:29 +0000 (11:41 -0300)
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>
C4/Calendar.pm
Koha/Calendar.pm
t/Calendar.t
t/db_dependent/Holidays.t
tools/newHolidays.pl

index 6783f97..d8705af 100644 (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;
         }
index 27a02b5..6be7c16 100644 (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(
+    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);
index 8648968..758f60b 100755 (executable)
@@ -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');
index dc7e5a6..299893d 100755 (executable)
@@ -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");
 
index 678ee0d..23ad1bb 100755 (executable)
@@ -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') ;
 }