From 8ff5bf7c1ae73365e948bc6fe314bf5eaa3094ac Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Mon, 17 Oct 2022 19:08:22 +0200 Subject: [PATCH] Bug 31856: Improve performance of serials subscriptions search To test: 1) Perform an advanced search that produces a reasonably large resultset for serial subscriptions (preferably at least a couple of hundred) and take not of the execution time. 2) Apply the patch. 3) Perform the search again, the execution should now be about one fifth of the previous search. 4) Ensure tests in t/db_dependent/AuthorisedValues.t all pass. Sponsored-by: Gothenburg University Library --- C4/Serials.pm | 16 ++++-- Koha/AuthorisedValues.pm | 73 +++++++++++++++++------- Koha/Template/Plugin/AuthorisedValues.pm | 23 +++++--- t/db_dependent/AuthorisedValues.t | 8 +-- 4 files changed, 81 insertions(+), 39 deletions(-) diff --git a/C4/Serials.pm b/C4/Serials.pm index e67881035d6..ed2986865d1 100644 --- a/C4/Serials.pm +++ b/C4/Serials.pm @@ -36,6 +36,7 @@ use C4::Biblio qw( GetMarcFromKohaField ModBiblio ); use C4::Log qw( logaction ); # logaction use C4::Serials::Frequency qw( GetSubscriptionFrequency ); use C4::Serials::Numberpattern; +use Koha::AdditionalFields; use Koha::AdditionalFieldValues; use Koha::Biblios; use Koha::Serial; @@ -617,14 +618,21 @@ sub SearchSubscriptions { $sth->execute(@where_args); my $results = $sth->fetchall_arrayref( {} ); + my %additional_fields_by_id = map { $_->id => $_->name } Koha::AdditionalFields->search( { tablename => 'subscription' } )->as_list; + for my $subscription ( @$results ) { $subscription->{cannotedit} = not can_edit_subscription( $subscription ); $subscription->{cannotdisplay} = not can_show_subscription( $subscription ); + my @additional_field_values = Koha::AdditionalFieldValues->search( + { + field_id => { -in => [ keys %additional_fields_by_id ] }, + record_id => $subscription->{subscriptionid} + } + )->as_list; - my $subscription_object = Koha::Subscriptions->find($subscription->{subscriptionid}); - $subscription->{additional_fields} = { map { $_->field->name => $_->value } - $subscription_object->additional_field_values->as_list }; - + $subscription->{additional_fields} = { + map { $additional_fields_by_id{$_->field_id} => $_->value } @additional_field_values + }; } return @$results; diff --git a/Koha/AuthorisedValues.pm b/Koha/AuthorisedValues.pm index b52c6c79e62..69e8c5bd758 100644 --- a/Koha/AuthorisedValues.pm +++ b/Koha/AuthorisedValues.pm @@ -93,7 +93,7 @@ sub find_by_koha_field { distinct => 1, } ); - return $av->count ? $av->next : undef; + return $av->next; } sub get_description_by_koha_field { @@ -106,14 +106,14 @@ sub get_description_by_koha_field { my $memory_cache = Koha::Cache::Memory::Lite->get_instance; my $cache_key = "AV_descriptions:$frameworkcode:$kohafield:$authorised_value"; - my $cached = $memory_cache->get_from_cache($cache_key); - return $cached if $cached; - - my $av = $self->find_by_koha_field($params); - return {} unless defined $av; - my $descriptions = { lib => $av->lib, opac_description => $av->opac_description }; - $memory_cache->set_in_cache( $cache_key, $descriptions ); - return $descriptions; + my $description = $memory_cache->get_from_cache($cache_key); + + unless (defined $description) { + my $av = $self->find_by_koha_field($params); + $description = defined $av ? { lib => $av->lib, opac_description => $av->opac_description } : {}; + $memory_cache->set_in_cache( $cache_key, $description ); + } + return $description; } sub get_descriptions_by_koha_field { @@ -123,19 +123,50 @@ sub get_descriptions_by_koha_field { my $memory_cache = Koha::Cache::Memory::Lite->get_instance; my $cache_key = "AV_descriptions:$frameworkcode:$kohafield"; - my $cached = $memory_cache->get_from_cache($cache_key); - return @$cached if $cached; + my $descriptions = $memory_cache->get_from_cache($cache_key); + + unless (defined $descriptions) { + my @avs = $self->search_by_koha_field($params)->as_list; + $descriptions = [ + map { + { + authorised_value => $_->authorised_value, + lib => $_->lib, + opac_description => $_->opac_description + } + } @avs + ]; + $memory_cache->set_in_cache( $cache_key, $descriptions ); + } + return @{$descriptions}; +} - my @avs = $self->search_by_koha_field($params)->as_list; - my @descriptions = map { - { - authorised_value => $_->authorised_value, - lib => $_->lib, - opac_description => $_->opac_description - } - } @avs; - $memory_cache->set_in_cache( $cache_key, \@descriptions ); - return @descriptions; +sub get_description_by_category_and_authorised_value { + my ( $self, $params ) = @_; + return unless defined $params->{category} && defined $params->{authorised_value}; + + my $category = $params->{category}; + my $value = $params->{authorised_value}; + + my $memory_cache = Koha::Cache::Memory::Lite->get_instance; + my $cache_key = "AV_description_by_category_and_authorised_value:$category:$value"; + my $description = $memory_cache->get_from_cache($cache_key); + + unless (defined $description) { + my $av = $self->search( + { + category => $category, + authorised_value => $value + } + )->next; + $description = defined $av ? { + lib => $av->lib, + opac_description => $av->opac_description + } : {}; + $memory_cache->set_in_cache( $cache_key, $description ); + } + + return $description; } sub categories { diff --git a/Koha/Template/Plugin/AuthorisedValues.pm b/Koha/Template/Plugin/AuthorisedValues.pm index 5cb91b83a3c..b121b2629b0 100644 --- a/Koha/Template/Plugin/AuthorisedValues.pm +++ b/Koha/Template/Plugin/AuthorisedValues.pm @@ -26,17 +26,24 @@ use Koha::AuthorisedValues; sub GetByCode { my ( $self, $category, $code, $opac ) = @_; - my $av = Koha::AuthorisedValues->search({ category => $category, authorised_value => $code }); - return $av->count - ? $opac - ? $av->next->opac_description - : $av->next->lib - : $code; + my $av = Koha::AuthorisedValues->get_description_by_category_and_authorised_value( + { + category => $category, + authorised_value => $code + } + ); + + my $description; + if ($av) { + $description = $opac ? $av->{opac_description} : $av->{lib}; + } + + return $description || $code; } sub Get { - my ( $self, $category, $selected, $opac ) = @_; - return GetAuthorisedValues( $category, $selected, $opac ); + my ( $self, $category, $opac ) = @_; + return GetAuthorisedValues( $category, $opac ); } sub GetAuthValueDropbox { diff --git a/t/db_dependent/AuthorisedValues.t b/t/db_dependent/AuthorisedValues.t index a1bf7b48125..91d3367e18f 100755 --- a/t/db_dependent/AuthorisedValues.t +++ b/t/db_dependent/AuthorisedValues.t @@ -1,7 +1,7 @@ #!/usr/bin/perl use Modern::Perl; -use Test::More tests => 16; +use Test::More tests => 15; use Try::Tiny; use t::lib::TestBuilder; @@ -136,12 +136,8 @@ my $limits = $av1->library_limits->as_list; is( @$limits, 2, 'library_limits functions correctly both as setter and getter' ); my @categories = Koha::AuthorisedValues->new->categories; + is( @categories, @existing_categories+3, 'There should have 3 categories inserted' ); -is_deeply( - \@categories, - [ sort { uc $a cmp uc $b } @categories ], - 'categories must be ordered by category names' -); subtest 'search_by_*_field + find_by_koha_field + get_description + authorised_values' => sub { plan tests => 6; -- 2.25.1