Bugzilla – Attachment 48768 Details for
Bug 15870
Add Filter for MARC to respect visibility settings
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 15870 - Follow-up of filter and tests
Bug-15870---Follow-up-of-filter-and-tests.patch (text/plain), 14.08 KB, created by
Mark Tompsett
on 2016-03-08 06:19:23 UTC
(
hide
)
Description:
Bug 15870 - Follow-up of filter and tests
Filename:
MIME Type:
Creator:
Mark Tompsett
Created:
2016-03-08 06:19:23 UTC
Size:
14.08 KB
patch
obsolete
>From 6c915b83a897f1b754d79673e0ed9b415e7927fa Mon Sep 17 00:00:00 2001 >From: Mark Tompsett <mtompset@hotmail.com> >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. >--- > 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.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 15870
:
48260
|
48261
|
48346
|
48628
|
48766
|
48767
|
48768
|
48769
|
48812
|
48813
|
48814
|
48815
|
49013
|
49035
|
49534
|
49535
|
49536
|
49537
|
49538