From e36f4b1b760c5b5d0579413703ef313d3b4b897b Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Thu, 29 Sep 2022 17:18:01 +0200 Subject: [PATCH] Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass Sponsored-by: Gothenburg University Library --- C4/Circulation.pm | 13 ++++--- Koha/Object.pm | 67 +++++++++++++++++++++++++++++++++++ Koha/Patron.pm | 52 ++++++++++++++++++++++----- t/db_dependent/Koha/Patrons.t | 56 +++++++++++++++++++++++++---- 4 files changed, 167 insertions(+), 21 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 336ce9490a..07d5371577 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -841,7 +841,7 @@ sub CanBookBeIssued { $issuingimpossible{DEBARRED} = 1; } - if ( $patron->is_expired ) { + if ( $patron->is_expired({ cache => 1 }) ) { $issuingimpossible{EXPIRED} = 1; } @@ -927,7 +927,7 @@ sub CanBookBeIssued { else { $issuingimpossible{USERBLOCKEDWITHENDDATE} = $debarred_date; } - } elsif ( my $num_overdues = $patron->has_overdues ) { + } elsif ( my $num_overdues = $patron->has_overdues({ cache => 1 }) ) { ## patron has outstanding overdue loans if ( C4::Context->preference("OverduesBlockCirc") eq 'block'){ $issuingimpossible{USERBLOCKEDOVERDUE} = $num_overdues; @@ -1737,7 +1737,6 @@ sub AddIssue { if ( $accumulate_charge > 0 ) { AddIssuingCharge( $issue, $accumulate_charge, 'RENT_DAILY' ); $charge += $accumulate_charge; - $item_unblessed->{charge} = $charge; } } @@ -2542,7 +2541,7 @@ sub MarkIssueReturned { # Remove any OVERDUES related debarment if the borrower has no overdues if ( C4::Context->preference('AutoRemoveOverduesRestrictions') && $patron->debarred - && !$patron->has_overdues + && !$patron->has_overdues({ cache => 1 }) && @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) } ) { DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' }); @@ -2898,7 +2897,7 @@ sub CanBookBeRenewed { my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing'); $patron = Koha::Patrons->find($borrowernumber); # FIXME Is this really useful? my $restricted = $patron->is_debarred; - my $hasoverdues = $patron->has_overdues; + my $hasoverdues = $patron->has_overdues({ cache => 1 }); if ( $restricted and $restrictionblockrenewing ) { return ( 0, 'restriction'); @@ -3173,7 +3172,7 @@ sub AddRenewal { # Remove any OVERDUES related debarment if the borrower has no overdues if ( $patron && $patron->is_debarred - && ! $patron->has_overdues + && ! $patron->has_overdues({ cache => 1 }) && @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) } ) { DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' }); @@ -4385,7 +4384,7 @@ sub _CanBookBeAutoRenewed { } ); - if ( $patron->category->effective_BlockExpiredPatronOpacActions and $patron->is_expired ) { + if ( $patron->is_expired({ cache => 1 }) && $patron->category->effective_BlockExpiredPatronOpacActions ) { return 'auto_account_expired'; } diff --git a/Koha/Object.pm b/Koha/Object.pm index b4b5717c34..512b4fa3f6 100644 --- a/Koha/Object.pm +++ b/Koha/Object.pm @@ -80,6 +80,7 @@ sub new { } $self->{_messages} = []; + $self->{_cache} = {}; croak("No _type found! Koha::Object must be subclassed!") unless $class->_type(); @@ -88,6 +89,72 @@ sub new { } +=head3 Koha::Object->_cache_get(); + +Koha::Object->_cache_get($cache_key); + +=cut + +sub _cache_get { + my ($self, $key) = @_; + return $self->{_cache}->{$key}; +} + +=head3 Koha::Object->_cache_set(); + +Koha::Object->_cache_set($cache_key, $value); + +=cut + +sub _cache_set { + my ($self, $key, $value) = @_; + $self->{_cache}->{$key} = $value; +} + + +=head3 Koha::Object->_cache_clear($cache_key); + +Koha::Object->_cache_clear($cache_key); +Koha::Object->_cache_clear(); + +=cut + +sub _cache_clear { + my ($self, $key) = @_; + if (defined $key) { + delete $self->{_cache}->{$key}; + } + else { + $self->{_cache} = {}; + } +} + +=head3 Koha::Object->_maybe_cached($cache_key); + +Koha::Object->_maybe_cached($use_cache, $cache_key, $getter); + +=cut + +sub _maybe_cached { + my ($self, $use_cache, $cache_key, $getter) = @_; + my $value; + + if ($use_cache) { + $value = $self->_cache_get($cache_key); + return $value if defined $value; + } + + $value = $getter->(); + if ($use_cache) { + $self->_cache_set($cache_key, $value); + } + else { + $self->_cache_clear($cache_key); + } + + return $value; +} + =head3 Koha::Object->_new_from_dbic(); my $object = Koha::Object->_new_from_dbic($dbic_row); diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 96a72a393a..8d8ad99821 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -762,20 +762,52 @@ sub is_debarred { return; } +=head3 dateexpiry + +Override Patron::dateexpiry to invalidate Patron::is_expired cache. + +=cut + +sub dateexpiry { + my $self = shift @_; + if (@_) { + $self->_cache_clear('is_expired'); + } + return $self->SUPER::dateexpiry(@_); +} + +=head3 set + +Override Patron::set to invalidate Patron::is_expired cache if dateexpiry is set. + +=cut + +sub set { + my $self = shift @_; + my ($properties) = @_; + if (exists $properties->{dateexpiry}) { + $self->_cache_clear('is_expired'); + } + return $self->SUPER::set(@_); +} + =head3 is_expired my $is_expired = $patron->is_expired; +my $is_expired = $patron->is_expired({ cache => 1}); Returns 1 if the patron is expired or 0; =cut sub is_expired { - my ($self) = @_; - return 0 unless $self->dateexpiry; - return 0 if $self->dateexpiry =~ '^9999'; - return 1 if dt_from_string( $self->dateexpiry ) < dt_from_string->truncate( to => 'day' ); - return 0; + my ($self, $params) = @_; + $params //= {}; + return $self->_maybe_cached($params->{cache}, 'is_expired', sub { + return $self->dateexpiry && + $self->dateexpiry !~ '^9999' && + dt_from_string( $self->dateexpiry ) < dt_from_string->truncate( to => 'day' ) ? 1 : 0; + }); } =head3 password_expired @@ -973,15 +1005,19 @@ sub renew_account { =head3 has_overdues my $has_overdues = $patron->has_overdues; +my $has_overdues = $patron->has_overdues({ cache => 1 }); Returns the number of patron's overdues =cut sub has_overdues { - my ($self) = @_; - my $dtf = Koha::Database->new->schema->storage->datetime_parser; - return $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime( dt_from_string() ) } })->count; + my ($self, $params) = @_; + $params //= {}; + return $self->_maybe_cached($params->{cache}, 'has_overdues', sub { + my $dtf = Koha::Database->new->schema->storage->datetime_parser; + return $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime( dt_from_string() ) } })->count; + }); } =head3 track_login diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index b1adccde3d..35a74c077b 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -264,7 +264,7 @@ subtest 'siblings' => sub { }; subtest 'has_overdues' => sub { - plan tests => 3; + plan tests => 12; my $item_1 = $builder->build_sample_item; my $retrieved_patron = Koha::Patrons->find( $new_patron_1->borrowernumber ); @@ -273,26 +273,70 @@ subtest 'has_overdues' => sub { my $tomorrow = DateTime->today( time_zone => C4::Context->tz() )->add( days => 1 ); my $issue = Koha::Checkout->new({ borrowernumber => $new_patron_1->id, itemnumber => $item_1->itemnumber, date_due => $tomorrow, branchcode => $library->{branchcode} })->store(); is( $retrieved_patron->has_overdues, 0, ); + + # Cache is set + is( $retrieved_patron->has_overdues({ cache => 1}), 0, ); + is( $retrieved_patron->_cache_get('has_overdues'), 0, 'has_overdues cache has been set'); + # Cache is used + is( $retrieved_patron->has_overdues({ cache => 1}), 0, ); + $issue->delete(); my $yesterday = DateTime->today(time_zone => C4::Context->tz())->add( days => -1 ); $issue = Koha::Checkout->new({ borrowernumber => $new_patron_1->id, itemnumber => $item_1->itemnumber, date_due => $yesterday, branchcode => $library->{branchcode} })->store(); $retrieved_patron = Koha::Patrons->find( $new_patron_1->borrowernumber ); is( $retrieved_patron->has_overdues, 1, ); + is( $retrieved_patron->_cache_get('has_overdues'),, undef, 'has_overdues cache has been invalidated' ); + # Cache is set + is( $retrieved_patron->has_overdues({ cache => 1}), 1, ); + is ( $retrieved_patron->_cache_get('has_overdues'),, 1, 'has_overdues cache has been set' ); + # Cache is used + is( $retrieved_patron->has_overdues({ cache => 1}), 1, ); + $issue->delete(); + + # Cache is used (with stale entry, flushing cache must be handled manually as automatic invalidation is + # complex and probably not worth the effort) + is( $retrieved_patron->has_overdues({ cache => 1}), 1, ); + + # Cache has been invalidated + is( $retrieved_patron->has_overdues, 0, ); }; subtest 'is_expired' => sub { - plan tests => 4; + plan tests => 14; my $patron = $builder->build({ source => 'Borrower' }); $patron = Koha::Patrons->find( $patron->{borrowernumber} ); $patron->dateexpiry( undef )->store->discard_changes; - is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is not set'); + is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is not set' ); $patron->dateexpiry( dt_from_string )->store->discard_changes; - is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is today'); + is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is today' ); $patron->dateexpiry( dt_from_string->add( days => 1 ) )->store->discard_changes; - is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is tomorrow'); + is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is tomorrow' ); $patron->dateexpiry( dt_from_string->add( days => -1 ) )->store->discard_changes; - is( $patron->is_expired, 1, 'Patron should be considered expired if dateexpiry is yesterday'); + is( $patron->is_expired, 1, 'Patron should be considered expired if dateexpiry is yesterday' ); + + + # Test is_expired cache + $patron->dateexpiry( undef )->store->discard_changes; + # Set cache + is( $patron->is_expired({ cache => 1 }), 0, 'Patron should not be considered expired if dateexpiry is not set, with caching enabled' ); + is( $patron->_cache_get('is_expired'), 0, 'is_expired cache has been set'); + is( $patron->is_expired({ cache => 1 }), 0, 'Patron should not be considered expired if dateexpiry is not set, with caching enabled and cache hit' ); + $patron->dateexpiry( dt_from_string->add( days => -1 ) )->store->discard_changes; + is( $patron->is_expired({ cache => 1 }), 1, 'Patron should be considered expired if dateexpiry is yesterday, cache has been invalidated' ); + is( $patron->_cache_get('is_expired'), 1, 'is_expired cache has been set'); + is( $patron->is_expired({ cache => 1 }), 1, 'Patron should be considered expired if dateexpiry is yesterday, on cache hit' ); + + $patron->{dateexpiry} = undef; + # Checking cache is really utilized by setting dateexpiry property without using accessor method + is( $patron->is_expired({ cache => 1 }), 1, 'Cache is used with stale value' ); + + $patron->set({ dateexpiry => undef })->store->discard_changes; + is( $patron->is_expired({ cache => 1 }), 0, 'Patron should not be considered expired if dateexpiry is not set, cache has been invalidated' ); + is( $patron->is_expired({ cache => 1 }), 0, 'Patron should not be considered expired if dateexpiry is not set, on cache hit' ); + + $patron->is_expired; + is( $patron->_cache_get('is_expired'), undef , 'Cache has been invalidated after calling is_expired without caching enabled' ); $patron->delete; }; -- 2.35.1