From 62d3a286c16b97c288af54bfd4b64bffbb5f6090 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Wed, 20 Aug 2014 00:33:40 -0300 Subject: [PATCH] Bub 12788: (regression test) refactor facet extraction code to allow testing This patch refactors the facet extraction loop into a proper function. The loop is changed so the MARC::Record objects are created only once instead of the old/current behaviour: once for each defined facet (in C4::Koha::getFacets). To test: - Apply the patch => SUCCESS: verify facets functionality remains unchanged. - Run: $ prove -v t/db_dependent/Search.t => SUCCESS: tests for _get_facets_data_from_record fail, because 100$a is considered for fields with indicator 1=z (field added by IncludeSeeFromInSearches syspref). - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens Signed-off-by: David Cook Signed-off-by: Katrin Fischer Works as described, passes tests and QA script. Signed-off-by: Tomas Cohen Arazi --- C4/Search.pm | 104 ++++++++++++++++++++++++---------------- t/db_dependent/Search.t | 30 +++++++++++- 2 files changed, 91 insertions(+), 43 deletions(-) diff --git a/C4/Search.pm b/C4/Search.pm index c58692bdad..e90ecc9774 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -340,8 +340,8 @@ sub getRecords { my $results_hashref = (); # Initialize variables for the faceted results objects - my $facets_counter = (); - my $facets_info = (); + my $facets_counter = {}; + my $facets_info = {}; my $facets = getFacets(); my $facets_maxrecs = C4::Context->preference('maxRecordsForFacets')||20; @@ -499,51 +499,29 @@ sub getRecords { } $results_hashref->{ $servers[ $i - 1 ] } = $results_hash; -# Fill the facets while we're looping, but only for the biblioserver and not for a scan + # Fill the facets while we're looping, but only for the + # biblioserver and not for a scan if ( !$scan && $servers[ $i - 1 ] =~ /biblioserver/ ) { - my $jmax = - $size > $facets_maxrecs ? $facets_maxrecs : $size; - for my $facet (@$facets) { - for ( my $j = 0 ; $j < $jmax ; $j++ ) { + my $jmax = $size > $facets_maxrecs + ? $facets_maxrecs + : $size; - my $marc_record = new_record_from_zebra ( - 'biblioserver', - $results[ $i - 1 ]->record($j)->raw() - ); - - if ( ! defined $marc_record ) { - warn "ERROR DECODING RECORD - $@: " . - $results[ $i - 1 ]->record($j)->raw(); - next; - } - - my @used_datas = (); + for ( my $j = 0 ; $j < $jmax ; $j++ ) { - foreach my $tag ( @{ $facet->{tags} } ) { + my $marc_record = new_record_from_zebra ( + 'biblioserver', + $results[ $i - 1 ]->record($j)->raw() + ); - # avoid first line - my $tag_num = substr( $tag, 0, 3 ); - my $subfield_letters = substr( $tag, 3 ); - # Removed when as_string fixed - my @subfields = $subfield_letters =~ /./sg; - - my @fields = $marc_record->field($tag_num); - foreach my $field (@fields) { - my $data = $field->as_string( $subfield_letters, $facet->{sep} ); + if ( ! defined $marc_record ) { + warn "ERROR DECODING RECORD - $@: " . + $results[ $i - 1 ]->record($j)->raw(); + next; + } - unless ( grep { /^\Q$data\E$/ } @used_datas ) { - push @used_datas, $data; - $facets_counter->{ $facet->{idx} }->{$data}++; - } - } # fields - } # field codes - } # records - $facets_info->{ $facet->{idx} }->{label_value} = - $facet->{label}; - $facets_info->{ $facet->{idx} }->{expanded} = - $facet->{expanded}; - } # facets + _get_facets_data_from_record( $marc_record, $facets, $facets_counter, $facets_info ); + } } # warn "connection ", $i-1, ": $size hits"; @@ -673,6 +651,50 @@ sub getRecords { return ( undef, $results_hashref, \@facets_loop ); } +=head2 _get_facets_data_from_record + + 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 for using in getRecords. + +$facets is expected to be filled with C4::Koha::getFacets output (i.e. the configured +facets for Zebra). + +=cut + +sub _get_facets_data_from_record { + + my ( $marc_record, $facets, $facets_counter, $facets_info ) = @_; + + for my $facet (@$facets) { + + my @used_datas = (); + + foreach my $tag ( @{ $facet->{ tags } } ) { + + # avoid first line + my $tag_num = substr( $tag, 0, 3 ); + my $subfield_letters = substr( $tag, 3 ); + # Removed when as_string fixed + my @subfields = $subfield_letters =~ /./sg; + + my @fields = $marc_record->field( $tag_num ); + foreach my $field (@fields) { + my $data = $field->as_string( $subfield_letters, $facet->{ sep } ); + + unless ( grep { /^\Q$data\E$/ } @used_datas ) { + push @used_datas, $data; + $facets_counter->{ $facet->{ idx } }->{ $data }++; + } + } + } + # update $facets_info so we know what facet categories need to be rendered + $facets_info->{ $facet->{ idx } }->{ label_value } = $facet->{ label }; + $facets_info->{ $facet->{ idx } }->{ expanded } = $facet->{ expanded }; + } +} + sub pazGetRecords { my ( $koha_query, $simple_query, $sort_by_ref, $servers_ref, diff --git a/t/db_dependent/Search.t b/t/db_dependent/Search.t index cadfedb72d..11bbba9c98 100644 --- a/t/db_dependent/Search.t +++ b/t/db_dependent/Search.t @@ -857,6 +857,32 @@ if ( $indexing_mode eq 'dom' ) { getRecords('ccl=( AND )', '', ['title_az'], [ 'biblioserver' ], '20', 0, undef, \%branches, \%itemtypes, 'ccl', undef) } qr/WARNING: query problem with/, 'got warning instead of crash when attempting to run invalid query (bug 9578)'; + # 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; + $marc_record->add_fields( + [ '001', '1234' ], + [ '100', ' ', ' ', a => 'Cohen Arazi, Tomas' ], + [ '100', 'z', ' ', a => 'Tomasito' ], + [ '245', ' ', ' ', a => 'First try' ] + ); + C4::Search::_get_facets_data_from_record($marc_record, $facets, $facets_counter,$facets_info); + 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; + $marc_record->add_fields( + [ '001', '1234' ], + [ '100', ' ', ' ', a => 'Cohen Arazi, Tomas' ], + [ '100', 'z', ' ', a => 'Tomasito' ], + [ '245', ' ', ' ', a => 'Second try' ] + ); + C4::Search::_get_facets_data_from_record($marc_record, $facets, $facets_counter, $facets_info); + is_deeply( { au => { 'Cohen Arazi, Tomas' => 2 } }, $facets_counter, + "_get_facets_data_from_record correctly counts author facet twice"); + cleanup(); } @@ -932,12 +958,12 @@ sub run_unimarc_search_tests { } subtest 'MARC21 + GRS-1' => sub { - plan tests => 106; + plan tests => 108; run_marc21_search_tests('grs1'); }; subtest 'MARC21 + DOM' => sub { - plan tests => 106; + plan tests => 108; run_marc21_search_tests('dom'); }; -- 2.39.5