From 75a01a9bd378c873f7acb7be31d9ee1aaa5332c1 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Wed, 20 Aug 2014 11:39:27 -0300 Subject: [PATCH] [PASSED QA] Bug 12788: (followup) minor optimization with proper tests This patch removes the $facets_info calculation from the _get_facets_data_from_record sub so it is not done for each record. It introduces a new sub, _get_facets_info that is called from the getRecords loop, that does the job only once. To test: - Apply on top of the previous patches - Run $ prove -v t/db_dependent/Search.t => SUCCESS: _get_facets_info gets tested and it passes for both MARC21 and UNIMARC. Facets rendering should remain unchaged on the UI. - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens Signed-off-by: David Cook Signed-off-by: Katrin Fischer --- C4/Search.pm | 29 ++++++++++++++++++++---- t/db_dependent/Search.t | 60 +++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/C4/Search.pm b/C4/Search.pm index 54ef9ec..932d04b 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -520,7 +520,8 @@ sub getRecords { next; } - _get_facets_data_from_record( $marc_record, $facets, $facets_counter, $facets_info ); + _get_facets_data_from_record( $marc_record, $facets, $facets_counter ); + $facets_info = _get_facets_info( $facets ); } } @@ -656,7 +657,7 @@ sub getRecords { C4::Search::_get_facets_data_from_record( $marc_record, $facets, $facets_counter ); Internal function that extracts facets information from a MARC::Record object -and populates $facets_counter and $facets_info for using in getRecords. +and populates $facets_counter for using in getRecords. $facets is expected to be filled with C4::Koha::getFacets output (i.e. the configured facets for Zebra). @@ -665,7 +666,7 @@ facets for Zebra). sub _get_facets_data_from_record { - my ( $marc_record, $facets, $facets_counter, $facets_info ) = @_; + my ( $marc_record, $facets, $facets_counter ) = @_; for my $facet (@$facets) { @@ -692,10 +693,30 @@ sub _get_facets_data_from_record { } } } - # update $facets_info so we know what facet categories need to be rendered + } +} + +=head2 _get_facets_info + + my $facets_info = C4::Search::_get_facets_info( $facets ) + +Internal function that extracts facets information and properly builds +the data structure needed to render facet labels. + +=cut + +sub _get_facets_info { + + my $facets = shift; + + my $facets_info = {}; + + for my $facet ( @$facets ) { $facets_info->{ $facet->{ idx } }->{ label_value } = $facet->{ label }; $facets_info->{ $facet->{ idx } }->{ expanded } = $facet->{ expanded }; } + + return $facets_info; } sub pazGetRecords { diff --git a/t/db_dependent/Search.t b/t/db_dependent/Search.t index 11bbba9..a10b71b 100644 --- a/t/db_dependent/Search.t +++ b/t/db_dependent/Search.t @@ -129,6 +129,8 @@ $contextmodule->mock('preference', sub { return '--'; } elsif ($pref eq 'DisplayLibraryFacets') { return 'holding'; + } elsif ($pref eq 'UNIMARCAuthorsFacetsSeparator') { + return '--'; } else { warn "The syspref $pref was requested but I don't know what to say; this indicates that the test requires updating" unless $pref =~ m/(XSLT|item|branch|holding|image)/i; @@ -859,7 +861,6 @@ if ( $indexing_mode eq 'dom' ) { # Test facet calculation my $facets_counter = {}; - my $facets_info = {}; my $facets = C4::Koha::getFacets(); # Create a record with a 100$z field my $marc_record = MARC::Record->new; @@ -869,7 +870,7 @@ if ( $indexing_mode eq 'dom' ) { [ '100', 'z', ' ', a => 'Tomasito' ], [ '245', ' ', ' ', a => 'First try' ] ); - C4::Search::_get_facets_data_from_record($marc_record, $facets, $facets_counter,$facets_info); + C4::Search::_get_facets_data_from_record( $marc_record, $facets, $facets_counter ); is_deeply( { au => { 'Cohen Arazi, Tomas' => 1 } }, $facets_counter, "_get_facets_data_from_record doesn't count 100\$z (Bug 12788)"); $marc_record = MARC::Record->new; @@ -879,10 +880,33 @@ if ( $indexing_mode eq 'dom' ) { [ '100', 'z', ' ', a => 'Tomasito' ], [ '245', ' ', ' ', a => 'Second try' ] ); - C4::Search::_get_facets_data_from_record($marc_record, $facets, $facets_counter, $facets_info); + C4::Search::_get_facets_data_from_record( $marc_record, $facets, $facets_counter ); is_deeply( { au => { 'Cohen Arazi, Tomas' => 2 } }, $facets_counter, "_get_facets_data_from_record correctly counts author facet twice"); + # Test _get_facets_info + my $facets_info = C4::Search::_get_facets_info( $facets ); + my $expected_facets_info_marc21 = { + 'au' => { 'expanded' => undef, + 'label_value' => "Authors" }, + 'holdingbranch' => { 'expanded' => undef, + 'label_value' => "HoldingLibrary" }, + 'itype' => { 'expanded' => undef, + 'label_value' => "ItemTypes" }, + 'location' => { 'expanded' => undef, + 'label_value' => "Location" }, + 'se' => { 'expanded' => undef, + 'label_value' => "Series" }, + 'su-geo' => { 'expanded' => undef, + 'label_value' => "Places" }, + 'su-to' => { 'expanded' => undef, + 'label_value' => "Topics" }, + 'su-ut' => { 'expanded' => undef, + 'label_value' => "Titles" } + }; + is_deeply( $facets_info, $expected_facets_info_marc21, + "_get_facets_info returns the correct data"); + cleanup(); } @@ -954,26 +978,48 @@ sub run_unimarc_search_tests { ); is($count, 24, 'UNIMARC authorities: hits on any starts with "jean"'); + # Test _get_facets_info + my $facets = C4::Koha::getFacets(); + my $facets_info = C4::Search::_get_facets_info( $facets ); + my $expected_facets_info_unimarc = { + 'au' => { 'expanded' => undef, + 'label_value' => "Authors" }, + 'holdingbranch' => { 'expanded' => undef, + 'label_value' => "HoldingLibrary" }, + 'location' => { 'expanded' => undef, + 'label_value' => "Location" }, + 'se' => { 'expanded' => undef, + 'label_value' => "Series" }, + 'su-geo' => { 'expanded' => undef, + 'label_value' => "Places" }, + 'su-to' => { 'expanded' => undef, + 'label_value' => "Topics" }, + 'su-ut' => { 'expanded' => undef, + 'label_value' => "Titles" } + }; + is_deeply( $facets_info, $expected_facets_info_unimarc, + "_get_facets_info returns the correct data"); + cleanup(); } subtest 'MARC21 + GRS-1' => sub { - plan tests => 108; + plan tests => 109; run_marc21_search_tests('grs1'); }; subtest 'MARC21 + DOM' => sub { - plan tests => 108; + plan tests => 109; run_marc21_search_tests('dom'); }; subtest 'UNIMARC + GRS-1' => sub { - plan tests => 13; + plan tests => 14; run_unimarc_search_tests('grs1'); }; subtest 'UNIMARC + DOM' => sub { - plan tests => 13; + plan tests => 14; run_unimarc_search_tests('dom'); }; -- 1.9.1