From 4b477b7e851a65df5e9c753bda697fc8d77302c5 Mon Sep 17 00:00:00 2001 From: Mark Tompsett Date: Fri, 4 Mar 2016 17:04:37 -0500 Subject: [PATCH] Bug 15870: Follow-up of filter and tests This patch: - improves perlcritic messages in the filter and tests. - changes should display logic to should hide logic to simplify filter. - perltidies the scripts - debugs the issues outstanding on the comprehensive tests provided in the second commit. Signed-off-by: Hector Castro Signed-off-by: Tomas Cohen Arazi --- Koha/Filter/MARC/ViewPolicy.pm | 101 ++++++++++++--------- t/db_dependent/Filter_MARC_ViewPolicy.t | 151 +++++++++++++++++++------------- 2 files changed, 152 insertions(+), 100 deletions(-) diff --git a/Koha/Filter/MARC/ViewPolicy.pm b/Koha/Filter/MARC/ViewPolicy.pm index e28ade7..14df201 100644 --- a/Koha/Filter/MARC/ViewPolicy.pm +++ b/Koha/Filter/MARC/ViewPolicy.pm @@ -44,7 +44,9 @@ use C4::Biblio; use base qw(Koha::RecordProcessor::Base); our $NAME = 'MARC_ViewPolicy'; -our $VERSION = '3.23'; # Master version I hope it gets in. +our $VERSION = '3.23'; # Master version I hope it gets in. + +use constant FIRST_NONCONTROL_TAG => 10; # tags < 10 are control tags. =head1 SUBROUTINES/METHODS @@ -81,7 +83,7 @@ sub filter { my $result = $current_record->clone(); my $interface = $self->{options}->{interface} // 'opac'; my $frameworkcode = $self->{options}->{frameworkcode} // q{}; - my $display = _should_display_on_interface(); + my $hide = _should_hide_on_interface(); my $marcsubfieldstructure = GetMarcStructure( 0, $frameworkcode ); @@ -95,7 +97,7 @@ sub filter { { field => $field, marcsubfieldstructure => $marcsubfieldstructure, - display => $display, + hide => $hide, interface => $interface, result => $result } @@ -117,24 +119,22 @@ sub _filter_field { my $field = $parameter->{field}; my $marcsubfieldstructure = $parameter->{marcsubfieldstructure}; - my $display = $parameter->{display}; + my $hide = $parameter->{hide}; my $interface = $parameter->{interface}; my $result = $parameter->{result}; my $tag = $field->tag(); - if ( $tag >= 10 ) { + if ( $tag >= FIRST_NONCONTROL_TAG ) { foreach my $subpairs ( $field->subfields() ) { my ( $subtag, $value ) = @{$subpairs}; - # visibility is a "level" (-7 to +7), default to 0 + # visibility is a "level" (-9 to +9), default to 0 + # -8 is flagged, and 9/-9 are not implemented. my $visibility = $marcsubfieldstructure->{$tag}->{$subtag}->{hidden}; $visibility //= 0; - my $hidden; - if ( $display->{$interface}->{$visibility} ) { - $hidden = 0; - } - else { + if ( $hide->{$interface}->{$visibility} ) { + # deleting last subfield doesn't delete field, so # this detects that case to delete the field. if ( scalar $field->subfields() <= 1 ) { @@ -147,19 +147,16 @@ sub _filter_field { } } - # tags less than 10 don't have subfields, use @ trick. + # control tags don't have subfields, use @ trick. else { - # visibility is a "level" (-7 to +7), default to 0 + # visibility is a "level" (-9 to +9), default to 0 + # -8 is flagged, and 9/-9 are not implemented. my $visibility = $marcsubfieldstructure->{$tag}->{q{@}}->{hidden}; $visibility //= 0; - my $hidden; - if ( $display->{$interface}->{$visibility} ) { - $hidden = 0; - } - else { - $hidden = 1; + if ( $hide->{$interface}->{$visibility} ) { $result->delete_fields($field); } + } return; } @@ -174,36 +171,62 @@ sub initialize { return; } -sub _should_display_on_interface { - my $display = { +# Copied and modified from 3.10.x help file +# marc_subfields_structure.hidden +# allows you to select from 19 possible visibility conditions, 17 of which are implemented. They are the following: +# -9 => Future use +# -8 => Flag +# -7 => OPAC !Intranet !Editor Collapsed +# -6 => OPAC Intranet !Editor !Collapsed +# -5 => OPAC Intranet !Editor Collapsed +# -4 => OPAC !Intranet !Editor !Collapsed +# -3 => OPAC !Intranet Editor Collapsed +# -2 => OPAC !Intranet Editor !Collapsed +# -1 => OPAC Intranet Editor Collapsed +# 0 => OPAC Intranet Editor !Collapsed +# 1 => !OPAC Intranet Editor Collapsed +# 2 => !OPAC !Intranet Editor !Collapsed +# 3 => !OPAC !Intranet Editor Collapsed +# 4 => !OPAC Intranet Editor !Collapsed +# 5 => !OPAC !Intranet !Editor Collapsed +# 6 => !OPAC Intranet !Editor !Collapsed +# 7 => !OPAC Intranet !Editor Collapsed +# 8 => !OPAC !Intranet !Editor !Collapsed +# 9 => Future use +# ( ! means 'not visible' or in the case of Collapsed 'not Collapsed') + +sub _should_hide_on_interface { + my $hide = { opac => { - 0 => 1, - -1 => 1, - -2 => 1, - -3 => 1, - -4 => 1, - -5 => 1, - -6 => 1, - -7 => 1, + '-8' => 1, + '1' => 1, + '2' => 1, + '3' => 1, + '4' => 1, + '5' => 1, + '6' => 1, + '7' => 1, + '8' => 1, }, intranet => { - -6 => 1, - -5 => 1, - -1 => 1, - 0 => 1, - 1 => 1, - 4 => 1, - 6 => 1, - 7 => 1, + '-8' => 1, + '-7' => 1, + '-4' => 1, + '-3' => 1, + '-2' => 1, + '2' => 1, + '3' => 1, + '5' => 1, + '8' => 1, }, }; - return $display; + return $hide; } =head1 DIAGNOSTICS $ prove -v t/RecordProcessor.t - $ prove -v t/db_dependent/RecordProcessor_ViewPolicy.t + $ prove -v t/db_dependent/Filter_MARC_ViewPolicy.t =head1 CONFIGURATION AND ENVIRONMENT diff --git a/t/db_dependent/Filter_MARC_ViewPolicy.t b/t/db_dependent/Filter_MARC_ViewPolicy.t index 43e24c3..09e567b 100644 --- a/t/db_dependent/Filter_MARC_ViewPolicy.t +++ b/t/db_dependent/Filter_MARC_ViewPolicy.t @@ -3,6 +3,10 @@ # This file is part of Koha. # # Copyright 2015 Mark Tompsett +# - Initial commit, perlcritic clean-up, and +# debugging +# Copyright 2016 Tomas Cohen Arazi +# - Expansion of test cases to be comprehensive # # Koha is free software; you can redistribute it and/or modify it # under the terms of the GNU General Public License as published by @@ -25,49 +29,60 @@ use List::MoreUtils qw/any/; use MARC::Record; use MARC::Field; use C4::Context; +use Koha::Cache qw/flush_all/; use Koha::Database; +use English qw/-no_match_vars/; + +$OUTPUT_AUTOFLUSH = 1; BEGIN { use_ok('Koha::RecordProcessor'); } -my $dbh = C4::Context->dbh; +sub run_hiding_tests { -my $database = Koha::Database->new(); -my $schema = $database->schema(); -$dbh->{RaiseError} = 1; + my $interface = shift; -my @valid_hidden_values = ( - '-7', '-6', '-5', '-4', '-3', '-2', '-1', '0', - '1', '2', '3', '4', '5', '6', '7', '8' -); + # TODO: -8 is Flagged, which doesn't seem used. + # -9 and +9 are supposedly valid future values + # according to older documentation in 3.10.x + my @valid_hidden_values = + ( -8, -7, -6, -5, -4, -3, -2, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8 ); -my $hidden = { - opac => [ '1', '2', '3', '4', '5', '6', '7', '8' ], - intranet => [ '-7', '-4', '-3', '-2', '2', '3', '5', '8' ] -}; + my $hidden = { + 'opac' => [ -8, 1, 2, 3, 4, 5, 6, 7, 8 ], + 'intranet' => [ -8, -7, -4, -3, -2, 2, 3, 5, 8 ] + }; -sub run_hiding_tests { + my $dbh = C4::Context->dbh; - my $interface = shift; + my $database = Koha::Database->new(); + my $schema = $database->schema(); + $dbh->{RaiseError} = 1; - # foreach my $hidden_value ( @{ $hidden->{ $interface } } ) { - foreach my $hidden_value ( @valid_hidden_values ) { + #$ENV{'DEBUG'} = '1'; # Turn on debugging. - $schema->storage->txn_begin(); + foreach my $hidden_value (@valid_hidden_values) { - my $sth = $dbh->prepare(" - UPDATE marc_subfield_structure SET hidden=? - WHERE tagfield='020' OR - tagfield='008'; - "); - $sth->execute( $hidden_value ); + $schema->storage->txn_begin(); - my $processor = Koha::RecordProcessor->new({ - schema => 'MARC', - filters => ( 'ViewPolicy' ), - options => { interface => $interface } - }); + my $update_sql = + q{UPDATE marc_subfield_structure SET hidden=? } + . q{WHERE tagfield='020' OR } + . q{ tagfield='008';}; + my $sth = $dbh->prepare($update_sql); + $sth->execute($hidden_value); + + my $cache = Koha::Cache->get_instance(); + $cache->flush_all(); # easy way to ensure DB is queried again. + + my $processor = Koha::RecordProcessor->new( + { + schema => 'MARC', + filters => ('ViewPolicy'), + options => { interface => $interface } + } + ); is( ref( $processor->filters->[0] ), @@ -76,74 +91,88 @@ sub run_hiding_tests { ); # Create a fresh record - my $record = create_marc_record(); + my $sample_record = create_marc_record(); + my $unfiltered_record = $sample_record->clone(); + # Apply filters - my $filtered_record = $processor->process( $record ); + my $filtered_record = $processor->process($sample_record); + # Data fields + if ( any { $_ == $hidden_value } @{ $hidden->{$interface} } ) { - if ( any { $_ eq $hidden_value } @{ $hidden->{ $interface } }) { # Subfield and controlfield are set to be hidden - - is( $filtered_record->field('020'), undef, + is( $filtered_record->field('020'), + undef, "Data field has been deleted because of hidden=$hidden_value" ); - is( $record->field('020'), undef, - "Data field has been deleted in the original record because of hidden=$hidden_value" ); + isnt( $unfiltered_record->field('020'), undef, +"Data field has been deleted in the original record because of hidden=$hidden_value" + ); + # Control fields have a different behaviour in code is( $filtered_record->field('008'), undef, - "Control field has been deleted because of hidden=$hidden_value" ); - is( $record->field('008'), undef, - "Control field has been deleted in the original record because of hidden=$hidden_value" ); + "Control field has been deleted because of hidden=$hidden_value" + ); + isnt( $unfiltered_record->field('008'), undef, +"Control field has been deleted in the original record because of hidden=$hidden_value" + ); - } else { + ok( $filtered_record && $unfiltered_record, 'Records exist' ); + } + else { isnt( $filtered_record->field('020'), undef, - "Data field hasn't been deleted because of hidden=$hidden_value" ); - isnt( $record->field('020'), undef, - "Data field hasn't been deleted in the original record because of hidden=$hidden_value" ); + "Data field hasn't been deleted because of hidden=$hidden_value" + ); + isnt( $unfiltered_record->field('020'), undef, +"Data field hasn't been deleted in the original record because of hidden=$hidden_value" + ); + # Control fields have a different behaviour in code isnt( $filtered_record->field('008'), undef, - "Control field hasn't been deleted because of hidden=$hidden_value" ); - isnt( $record->field('008'), undef, - "Control field hasn't been deleted in the original record because of hidden=$hidden_value" ); +"Control field hasn't been deleted because of hidden=$hidden_value" + ); + isnt( $unfiltered_record->field('008'), undef, +"Control field hasn't been deleted in the original record because of hidden=$hidden_value" + ); + + is_deeply( $filtered_record, $unfiltered_record, + 'Records are the same' ); } - is_deeply( $filtered_record, $record, - "Records are the same" ); - $schema->storage->txn_rollback(); } + return; } sub create_marc_record { - my $isbn = '0590353403'; - my $title = 'Foundation'; - my $record = MARC::Record->new; - my @fields = ( - MARC::Field->new( '003', 'AR-CdUBM'), - MARC::Field->new( '008', '######suuuu####ag_||||__||||_0||_|_uuu|d'), - MARC::Field->new( '020', q{}, q{}, 'a' => $isbn ), - MARC::Field->new( '245', q{}, q{}, 'a' => $title ) + my $isbn = '0590353403'; + my $title = 'Foundation'; + my $marc_record = MARC::Record->new; + my @fields = ( + MARC::Field->new( '003', 'AR-CdUBM' ), + MARC::Field->new( '008', '######suuuu####ag_||||__||||_0||_|_uuu|d' ), + MARC::Field->new( '020', q{}, q{}, 'a' => $isbn ), + MARC::Field->new( '245', q{}, q{}, 'a' => $title ), ); - $record->insert_fields_ordered( @fields ); + $marc_record->insert_fields_ordered(@fields); - return $record; + return $marc_record; } subtest 'Koha::Filter::MARC::ViewPolicy opac tests' => sub { - plan tests => 96; + plan tests => 102; run_hiding_tests('opac'); }; subtest 'Koha::Filter::MARC::ViewPolicy intranet tests' => sub { - plan tests => 96; + plan tests => 102; run_hiding_tests('intranet'); }; - 1; -- 2.7.4