Bugzilla – Attachment 144626 Details for
Bug 32476
Add caching for relatively expensive patron methods
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 32476: Add caching for relatively expensive patron methods
Bug-32476-Add-caching-for-relatively-expensive-pat.patch (text/plain), 10.48 KB, created by
David Gustafsson
on 2022-12-15 17:58:38 UTC
(
hide
)
Description:
Bug 32476: Add caching for relatively expensive patron methods
Filename:
MIME Type:
Creator:
David Gustafsson
Created:
2022-12-15 17:58:38 UTC
Size:
10.48 KB
patch
obsolete
>From 299f21a181cbe4de5077655a9e4078abbea3d74d Mon Sep 17 00:00:00 2001 >From: David Gustafsson <david.gustafsson@ub.gu.se> >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/Patrons.t all pass > >Sponsored-by: Gothenburg University Library >--- > C4/Circulation.pm | 13 ++++--- > Koha/Patron.pm | 67 +++++++++++++++++++++++++++++++---- > t/db_dependent/Koha/Patrons.t | 41 +++++++++++++++++---- > 3 files changed, 101 insertions(+), 20 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/Patron.pm b/Koha/Patron.pm >index 96a72a393a..428f6127ae 100644 >--- a/Koha/Patron.pm >+++ b/Koha/Patron.pm >@@ -762,6 +762,35 @@ sub is_debarred { > return; > } > >+=head3 dateexpiry >+ >+Override Patron::dateexpiry to invalidate Patron::is_expired cache. >+ >+=cut >+ >+sub dateexpiry { >+ my $self = shift @_; >+ if (@_) { >+ $self->{is_expired_cache} = undef; >+ } >+ 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->{is_expired_cache} = undef; >+ } >+ return $self->SUPER::set(@_); >+} >+ > =head3 is_expired > > my $is_expired = $patron->is_expired; >@@ -771,11 +800,23 @@ 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 //= {}; >+ if ($params->{cache} && defined $self->{is_expired_cache}) { >+ return $self->{is_expired_cache}; >+ } >+ my $is_expired = >+ $self->dateexpiry && >+ $self->dateexpiry !~ '^9999' && >+ dt_from_string( $self->dateexpiry ) < dt_from_string->truncate( to => 'day' ); >+ $is_expired = $is_expired ? 1 : 0; >+ if ($params->{cache}) { >+ $self->{is_expired_cache} = $is_expired; >+ } >+ else { >+ $self->{is_expired_cache} = undef; >+ } >+ return $is_expired; > } > > =head3 password_expired >@@ -979,9 +1020,21 @@ Returns the number of patron's overdues > =cut > > sub has_overdues { >- my ($self) = @_; >+ my ($self, $params) = @_; >+ $params //= {}; >+ >+ if ($params->{cache} && defined $self->{has_overdues_cache}) { >+ return $self->{has_overdues_cache}; >+ } > my $dtf = Koha::Database->new->schema->storage->datetime_parser; >- return $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime( dt_from_string() ) } })->count; >+ my $has_overdues = $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime( dt_from_string() ) } })->count; >+ if ($params->{cache}) { >+ $self->{has_overdues_cache} = $has_overdues; >+ } >+ else { >+ $self->{has_overdues_cache} = undef; >+ } >+ return $has_overdues; > } > > =head3 track_login >diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t >index b1adccde3d..4bcf626899 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 => 10; > > my $item_1 = $builder->build_sample_item; > my $retrieved_patron = Koha::Patrons->find( $new_patron_1->borrowernumber ); >@@ -273,26 +273,55 @@ 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->{has_overdues_cache}, 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, ); >+ ok( !defined $retrieved_patron->{has_overdues_cache}, 'has_overdues cache has been invalidated' ); >+ # Cache is set >+ is( $retrieved_patron->has_overdues({ cache => 1}), 1, ); >+ is ( $retrieved_patron->{has_overdues_cache}, 1, 'has_overdues cache has been set' ); >+ # Cache is used >+ is( $retrieved_patron->has_overdues({ cache => 1}), 1, ); >+ > $issue->delete(); > }; > > subtest 'is_expired' => sub { >- plan tests => 4; >+ plan tests => 13; > 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' ); >+ >+ # 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->{is_expired_cache}, 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, 1, 'Patron should be considered expired if dateexpiry is yesterday'); >+ is( $patron->is_expired({ cache => 1 }), 1, 'Patron should be considered expired if dateexpiry is yesterday, cache has been invalidated' ); >+ is( $patron->{is_expired_cache}, 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->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; >+ ok( !defined $patron->{is_expired_cache}, 'Cache has been invalidated after calling is_expired without caching enabled' ); > > $patron->delete; > }; >-- >2.35.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 32476
:
144626
|
144627
|
144744
|
144770
|
144771
|
145036
|
145037
|
147145
|
147148
|
150778
|
150779
|
157241
|
157258
|
157259
|
162537
|
162538
|
162539