From 608ddc2fa93e085a7094dd6649cbb894cd15c75c Mon Sep 17 00:00:00 2001 From: Mark Tompsett Date: Tue, 14 Jan 2014 11:31:36 -0500 Subject: [PATCH] Bug 11592 - opac scripts do not respect marc tag visibility Added two functions to C4/Biblio: GetFilteredOpacBiblio and GetOpacHideMARC. GetFilteredOpacBiblio returns a MARC::Record stripped of all the fields and subfields which are supposed to be hidden in OPAC. GetOpacHideMARC returns a hash of whether a particular key (eg. title, subtitle, etc.) is hidden. A value of 0 means no, a value of 1 means hidden. Made GetCOinSBiblio function handle hiding of 245$a more gracefully. Also, modified GetMarcSubjects to not generate an array entry of empty values. Tweaked C4/XSLT.pm to delete a field, if there weren't any subfields to actually update with. Properly hid 245$a, in the case that there is no visibility for OPAC, for opac-MARCdetail. opac-detail now filters the MARC::Record according to the hidden value in marc_subfield_structure properly. It also corrects a couple minor issues with a parameter not passed being used in a concatenation. How the subtitle values calculation works changed. It is now based on marc_subfield_structure and not fieldmapping. And, the GetBiblioData hash that generates template variables is now filtered by the results of a GetOpacHideMARC call. The opac-showmarc page now converts the MARCXML record to a MARC::Record to easily filter using GetFilteredOpacBiblio, which is then turned back into MARCXML for processing. The opac-export script now filters the biblio record to match what is currently displayed. Added tests to t/db_dependent/Biblio.t to test the functionality of the two functions added into C4::Biblio. Added a GetFilteredOpacBiblio call into opac-ISBDdetail page, so that it should be filtered. TEST PLAN --------- 1) Backup DB 2) Go to any OPAC detail page and note the biblio number. 3) Log into the staff client and determine the framework code for that biblio number. 4) The steps should be done with OPACXSLTDetailsDisplay set to blank. 5) Home -> Koha administration -> MARC bibliographic framework -> MARC structure (for the matching framework) 6) On the OPAC detail page, click MARC view 7) In the staff client, for every tag listed in the OPAC -> Subfields -> click the first link Then running through all the tabs, click Advanced constraints and uncheck OPAC visibility. Then click Save Changes 8) Refresh the opac-MARCdetail page in OPAC -- what you hid should be mostly hidden TITLE will still display, even if you hide 245$a! 9) Click Normal view, and all the hidden things are still mostly showing! 10) The steps should be done with OPACXSLTDetailsDisplay set to default (or some custom one?). 11) Refresh the opac-detail page. Still mostly showing all the hidden things. 12) Click MARC view, and everything should be hidden... 13) Until you click the 'view plain' link. 14) Apply the patch. 15) Run the Koha QA test tool. 16) Refresh the opac-MARCdetail page. Title should hide now. 17) Click the 'view plain' link. -- LDR and 999$c and 999$d were displaying for me. I realized that I hadn't hidden 999, because the opac-MARCdetail page doesn't display it. LDR is the only known leak. 18) Click Normal view 19) Now all the opac-detail page should hide things just like the opac-MARCdetail page. 20) In the staff client, change the OPACXSLTDetailsDisplay to a blank value. 21) Recheck opac-detail and opac-MARC detail pages again, including the view plain link, and everything should hidden similarly. -- NOTE: LDR is the only known leak. NOTE: 952 fields are treated separately, so I don't believe they could be hidden in the table using the MARC visibility. The goal was hiding properly things above the items. 22) And lastly, attempt to Save record in the various formats using the dropdown and clicking Go. -- The results should be filtered. 23) Click on ISBD view to ensure nothing broke. 24) Restore DB, because hiding 000,003,005,008,020,040,245,etc. are not useful across an entire framework. Signed-off-by: Robin Sheat Signed-off-by: Kyle M Hall --- C4/Biblio.pm | 108 ++++++++++++++++++++++++++++++++++++++++++++++- C4/XSLT.pm | 17 +++++--- opac/opac-ISBDdetail.pl | 6 ++- opac/opac-MARCdetail.pl | 3 +- opac/opac-detail.pl | 17 +++++++- opac/opac-export.pl | 9 ++-- opac/opac-showmarc.pl | 6 +++ t/db_dependent/Biblio.t | 64 ++++++++++++++++++++++++++-- 8 files changed, 212 insertions(+), 18 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index f671828..e9657d2 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -79,6 +79,8 @@ BEGIN { &GetMarcSeries &GetMarcHosts GetMarcUrls + GetOpacHideMARC + GetFilteredOpacBiblio &GetUsedMarcStructure &GetXmlBiblio &GetCOinSBiblio @@ -1207,6 +1209,107 @@ sub GetUsedMarcStructure { return $sth->fetchall_arrayref( {} ); } +=head2 GetOpacHideMARC + +Return a hash reference of whether a field, built from +kohafield and tag, is hidden in OPAC (1) or not (0). + + my $OpacHideMARC = GetOpacHideMARC($frameworkcode); + + if ($OpacHideMARC->{'stocknumber'}==1) { + print "Hidden!\n"; + } + +C<$OpacHideMARC> is a ref to a hash which contains a series +of key value pairs indicating if that field (key) is +hidden (value == 1) or not (value == 0). + +C<$frameworkcode> is the framework code. + +=cut + +sub GetOpacHideMARC { + my ( $frameworkcode ) = shift || ''; + my $dbh = C4::Context->dbh; + my $sth = $dbh->prepare("SELECT kohafield AS field, tagfield AS tag, hidden FROM marc_subfield_structure WHERE kohafield>'' AND frameworkcode=? ORDER BY field, tagfield;"); + my $rv = $sth->execute($frameworkcode); + my %opachidemarc; + my $data = $sth->fetchall_hashref( [ qw(field tag) ] ); + foreach my $fullfield (keys %{$data}) { + my @tmpsplit = split(/\./,$fullfield); + my $field = $tmpsplit[-1]; + foreach my $tag (keys %{$data->{$fullfield}}) { + if ($data->{$fullfield}->{$tag}->{'hidden'}>0) { + $opachidemarc{ $field } = 1; + } + elsif ( !exists($opachidemarc{ $field }) ) { + $opachidemarc{ $field } = 0; + } + } + } + return \%opachidemarc; +} + +=head2 GetFilteredOpacBiblio + +Return a MARC::Record which has been filtered based +on the corresponding marc_subfield_structure records +for the given framework by excluding hidden>0. + + my $filtered_record = GetFilteredOpacBiblio($record,$frameworkcode); + +C<$record> is a MARC::Record. + +C<$frameworkcode> is the framework code. + +=cut + +sub GetFilteredOpacBiblio { + my ( $record ) = shift; + if (!$record) { return $record; } + my $filtered_record = $record->clone; + + my ( $frameworkcode ) = shift || ''; + + my $marcsubfieldstructure = GetMarcStructure(0,$frameworkcode); + #if ($marcsubfieldstructure->{'000'}->{'@'}->{hidden}>0) { + # LDR field is excluded from $record->fields(). + # if we hide it here, the MARCXML->MARC::Record->MARCXML transformation blows up. + #} + foreach my $fields ($filtered_record->fields()) { + my $tag = $fields->tag(); + if ($tag>=10) { + foreach my $subpairs ($fields->subfields()) { + my ($subtag,$value) = @$subpairs; + my $hidden = $marcsubfieldstructure->{$tag}->{$subtag}->{hidden}; + $hidden //= 0; + $hidden = ($hidden<=0) ? 0 : 1; + if ($hidden) { + # deleting last subfield doesn't delete field, so + # this detects that case to delete the field. + if (scalar $fields->subfields() <= 1) { + $filtered_record->delete_fields($fields); + } + else { + $fields->delete_subfield( code => $subtag ); + } + } + } + } + # tags less than 10 don't have subfields, use @ trick. + else { + my $hidden = $marcsubfieldstructure->{$tag}->{'@'}->{hidden}; + $hidden //= 0; + $hidden = ($hidden<=0) ? 0 : 1; + if ($hidden) { + $filtered_record->delete_fields($fields); + } + } + } + + return $filtered_record; +} + =head2 GetMarcFromKohaField ($MARCfield,$MARCsubfield)=GetMarcFromKohaField($kohafield,$frameworkcode); @@ -1427,7 +1530,8 @@ sub GetCOinSBiblio { $oauthors .= "&rft.au=$au"; } } - $title = "&rft." . $titletype . "title=" . $record->subfield( '245', 'a' ); + $title = $record->subfield( '245', 'a' ) || ''; + $title = "&rft." . $titletype . "title=" . $title; $subtitle = $record->subfield( '245', 'b' ) || ''; $title .= $subtitle; if ($titletype eq 'a') { @@ -1878,7 +1982,7 @@ sub GetMarcSubjects { push @marcsubjects, { MARCSUBJECT_SUBFIELDS_LOOP => \@subfields_loop, authoritylink => $authoritylink, - }; + } if $authoritylink || @subfields_loop; } return \@marcsubjects; diff --git a/C4/XSLT.pm b/C4/XSLT.pm index 242278d..edb826c 100644 --- a/C4/XSLT.pm +++ b/C4/XSLT.pm @@ -92,12 +92,17 @@ sub transformMARCXML4XSLT { if $av->{ $tag }->{ $letter }; push( @new_subfields, $letter, $value ); } - $field ->replace_with( MARC::Field->new( - $tag, - $field->indicator(1), - $field->indicator(2), - @new_subfields - ) ); + if (@new_subfields) { + $field ->replace_with( MARC::Field->new( + $tag, + $field->indicator(1), + $field->indicator(2), + @new_subfields + ) ); + } + else { + $record->delete_fields($field); + } } } } diff --git a/opac/opac-ISBDdetail.pl b/opac/opac-ISBDdetail.pl index 4bf42f6..24655bb 100755 --- a/opac/opac-ISBDdetail.pl +++ b/opac/opac-ISBDdetail.pl @@ -93,11 +93,13 @@ if (scalar @items >= 1) { } } -my $record = GetMarcBiblio($biblionumber); -if ( ! $record ) { +my $unfiltered_record = GetMarcBiblio($biblionumber); +if ( ! $unfiltered_record ) { print $query->redirect("/cgi-bin/koha/errors/404.pl"); exit; } +my $record = GetFilteredOpacBiblio($unfiltered_record,GetFrameworkCode($biblionumber)); + # some useful variables for enhanced content; # in each case, we're grabbing the first value we find in # the record and normalizing it diff --git a/opac/opac-MARCdetail.pl b/opac/opac-MARCdetail.pl index aaa565d..8a999dd 100755 --- a/opac/opac-MARCdetail.pl +++ b/opac/opac-MARCdetail.pl @@ -98,9 +98,10 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( } ); +my ($bt_tag,$bt_subtag) = GetMarcFromKohaField('biblio.title',$itemtype); $template->param( bibliotitle => $biblio->{title}, -); +) if $tagslib->{$bt_tag}->{$bt_subtag}->{hidden} <= 0; # get biblionumbers stored in the cart my @cart_list; diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl index e97ac91..ed003e2 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -122,6 +122,10 @@ if (C4::Context->preference('OpacSuppression')) { $template->param( biblionumber => $biblionumber ); +# Filter out the things to hide in OPAC. +my $frameworkcode = GetFrameworkCode($biblionumber); +$record = GetFilteredOpacBiblio($record,$frameworkcode); + # get biblionumbers stored in the cart my @cart_list; @@ -517,6 +521,7 @@ if ( C4::Context->preference('HighlightOwnItemsOnOPAC') ) { } my $dat = &GetBiblioData($biblionumber); +my $OpacHideMARC = GetOpacHideMARC($dat->{'frameworkcode'}); my $itemtypes = GetItemTypes(); # imageurl: @@ -676,7 +681,16 @@ my $marcsubjctsarray = GetMarcSubjects($record,$marcflavour); my $marcseriesarray = GetMarcSeries ($record,$marcflavour); my $marcurlsarray = GetMarcUrls ($record,$marcflavour); my $marchostsarray = GetMarcHosts($record,$marcflavour); -my $subtitle = GetRecordValue('subtitle', $record, GetFrameworkCode($biblionumber)); +my ($st_tag,$st_subtag) = GetMarcFromKohaField('bibliosubtitle.subtitle',$dat->{'frameworkcode'}); +my $subtitle; +if ($st_tag && $st_subtag) { + my @subtitles = $record->subfield($st_tag,$st_subtag); + $subtitle = \@subtitles if @subtitles; +} +if ($subtitle && @$subtitle) { + my @subtitles = map { { subfield => $_ } } @$subtitle; + $subtitle = \@subtitles; +} $template->param( MARCNOTES => $marcnotesarray, @@ -726,6 +740,7 @@ if (C4::Context->preference("AlternateHoldingsField") && scalar @items == 0) { } foreach ( keys %{$dat} ) { + next if ( $OpacHideMARC->{$_} ); $template->param( "$_" => defined $dat->{$_} ? $dat->{$_} : '' ); } diff --git a/opac/opac-export.pl b/opac/opac-export.pl index e707896..dbfd9b9 100755 --- a/opac/opac-export.pl +++ b/opac/opac-export.pl @@ -32,9 +32,12 @@ my $op=$query->param("op")||''; #op=export is currently the only use my $format=$query->param("format")||'utf8'; my $biblionumber = $query->param("bib")||0; $biblionumber = int($biblionumber); -my ($marc, $error)= ('',''); +my ($unfiltered_marc, $error, $frameworkcode)= ('','',''); + +$unfiltered_marc = GetMarcBiblio($biblionumber, 1) if $biblionumber; +$frameworkcode = GetFrameworkCode($biblionumber) if $biblionumber; +my $marc = GetFilteredOpacBiblio($unfiltered_marc,$frameworkcode); -$marc = GetMarcBiblio($biblionumber, 1) if $biblionumber; if(!$marc) { print $query->redirect("/cgi-bin/koha/errors/404.pl"); exit; @@ -56,7 +59,7 @@ elsif ($format =~ /ris/) { $format = 'ris'; } elsif ($format =~ /bibtex/) { - $marc = marc2bibtex(C4::Biblio::GetMarcBiblio($biblionumber),$biblionumber); + $marc = marc2bibtex($marc,$biblionumber); $format = 'bibtex'; } elsif ($format =~ /dc/) { diff --git a/opac/opac-showmarc.pl b/opac/opac-showmarc.pl index 8cce54d..3494c30 100755 --- a/opac/opac-showmarc.pl +++ b/opac/opac-showmarc.pl @@ -60,6 +60,12 @@ if(!ref $record) { if ($view eq 'card' || $view eq 'html') { my $xmlrecord= $importid? $record->as_xml(): GetXmlBiblio($biblionumber); + if (!$importid && $view eq 'html') { + my $filtered_record = MARC::Record->new_from_xml($xmlrecord); + my $frameworkcode = GetFrameworkCode($biblionumber); + $filtered_record = GetFilteredOpacBiblio($filtered_record,$frameworkcode); + $xmlrecord = $filtered_record->as_xml(); + } my $xslfile; my $xslfilename; my $htdocs = C4::Context->config('opachtdocs'); diff --git a/t/db_dependent/Biblio.t b/t/db_dependent/Biblio.t index 3c0a159..ca869dc 100755 --- a/t/db_dependent/Biblio.t +++ b/t/db_dependent/Biblio.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 3; +use Test::More tests => 7; use Test::MockModule; use MARC::Record; @@ -32,6 +32,19 @@ my $dbh = C4::Context->dbh; $dbh->{AutoCommit} = 0; $dbh->{RaiseError} = 1; +my $OpacHideMARC = GetOpacHideMARC(); +my $keycount1 = keys %$OpacHideMARC; +ok($keycount1>0,"There are $keycount1 values for the Default framework."); + +$OpacHideMARC = GetOpacHideMARC('YOLO'); +my $keycount2 = keys %$OpacHideMARC; +ok($keycount2==0,'The YOLO framework does not exist.'); + +$OpacHideMARC = GetOpacHideMARC(''); +my $keycount3 = keys %$OpacHideMARC; +ok($keycount1>0,"There are $keycount3 values for the Default framework."); +ok($keycount1==$keycount3,'The no parameter and empty parameter counts match.'); + # Mocking variables my $context = new Test::MockModule('C4::Context'); @@ -153,6 +166,51 @@ sub run_tests { $controlnumber = GetMarcControlnumber( $marc_record, $marcflavour ); is( $controlnumber, '123456789X', 'GetMarcControlnumber handles records with 001' ); + # the frameworkcode for the added biblio was not set, + # so the frameworkcode is '' for Default. + my $title_field = ( $marcflavour eq 'UNIMARC' ) ? '200' : '245'; + my $sth = $dbh->prepare("SELECT hidden FROM marc_subfield_structure WHERE tagfield='$title_field' and tagsubfield='a' and frameworkcode='';"); + $sth->execute(); + my $hidden = $sth->fetchrow; + if (defined($hidden)) { + ok ( defined($hidden), 'There is a title field entry for the ' . $marcflavour . 'Default framework.'); + + $dbh->do("UPDATE marc_subfield_structure SET hidden=4 WHERE tagfield='$title_field' and tagsubfield='a' and frameworkcode='';"); + $sth = $dbh->prepare("SELECT hidden FROM marc_subfield_structure WHERE tagfield='$title_field' and tagsubfield='a' and frameworkcode='';"); + $sth->execute(); + my $new_hidden = $sth->fetchrow; + ok ( defined($new_hidden) && $new_hidden==4, 'Default framework title field hidden value updated.'); + + my $record = GetMarcBiblio($biblionumber); + my $fail_filter = GetFilteredOpacBiblio(); + ok (!defined($fail_filter), 'GetFilteredOpacBiblio properly handles no parameters.'); + + my $filtered_record1 = GetFilteredOpacBiblio($record); + my $filtered_record2 = GetFilteredOpacBiblio($record,''); + my @fields1 = $filtered_record1->fields(); + my @fields2 = $filtered_record2->fields(); + ok ( scalar @fields1 == scalar @fields2, 'No framework code matches Default framework results.'); + + $title = $record->subfield($title_field,'a'); + my $filtered_title = $filtered_record1->subfield($title_field,'a'); + ok ( defined($title) && !defined($filtered_title), 'GetFilteredOpacBiblio filtered title as expected.'); + + $sth = $dbh->prepare("UPDATE marc_subfield_structure SET hidden=? WHERE tagfield='$title_field' and tagsubfield='a' AND frameworkcode='';"); + $sth->execute($hidden); + $sth = $dbh->prepare("SELECT hidden FROM marc_subfield_structure WHERE tagfield='$title_field' and tagsubfield='a' and frameworkcode='';"); + $sth->execute(); + $new_hidden = $sth->fetchrow; + ok ( $new_hidden==$hidden, 'Default framework title field hidden value reset.'); + } + else { + ok ( !defined($hidden), 'Skipping tests for MARC field/subfield hiding under ' . $marcflavour . 'Default framework.'); + ok ( !defined($hidden), 'Skipping tests for MARC field/subfield hiding under ' . $marcflavour . 'Default framework.'); + ok ( !defined($hidden), 'Skipping tests for MARC field/subfield hiding under ' . $marcflavour . 'Default framework.'); + ok ( !defined($hidden), 'Skipping tests for MARC field/subfield hiding under ' . $marcflavour . 'Default framework.'); + ok ( !defined($hidden), 'Skipping tests for MARC field/subfield hiding under ' . $marcflavour . 'Default framework.'); + ok ( !defined($hidden), 'Skipping tests for MARC field/subfield hiding under ' . $marcflavour . 'Default framework.'); + } + ## Testing GetMarcISBN my $record_for_isbn = MARC::Record->new(); my $isbns = GetMarcISBN( $record_for_isbn, $marcflavour ); @@ -241,13 +299,13 @@ sub create_issn_field { } subtest 'MARC21' => sub { - plan tests => 25; + plan tests => 31; run_tests('MARC21'); $dbh->rollback; }; subtest 'UNIMARC' => sub { - plan tests => 25; + plan tests => 31; run_tests('UNIMARC'); $dbh->rollback; }; -- 1.7.9.5