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 <nick@quecheelibrary.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

diff --git a/C4/Search.pm b/C4/Search.pm
index 54ef9ec..932d04b 100644 (file)
--- 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 (file)
--- 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');
 };