From 48e6da619c45a684534fd214f1514c9b1b676ab2 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. --- C4/Biblio.pm | 108 ++++++++++++++++++++++++++++++++++++++++++++++- C4/XSLT.pm | 17 +++++--- opac/opac-ISBDdetail.pl | 6 ++- opac/opac-MARCdetail.pl | 3 +- opac/opac-detail.pl | 21 +++++++-- opac/opac-export.pl | 7 ++- opac/opac-showmarc.pl | 6 +++ t/db_dependent/Biblio.t | 56 ++++++++++++++++++++++-- 8 files changed, 205 insertions(+), 19 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index dc40d68..c371f62 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') { @@ -1873,7 +1977,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 d85b048..2a43a41 100644 --- a/C4/XSLT.pm +++ b/C4/XSLT.pm @@ -93,12 +93,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 2579a8b..a4ddc40 100755 --- a/opac/opac-ISBDdetail.pl +++ b/opac/opac-ISBDdetail.pl @@ -95,11 +95,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 77de12f..63e4a34 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -91,6 +91,10 @@ if ( ! $record ) { } $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; @@ -377,13 +381,13 @@ if ($session->param('busc')) { my ($previous, $next, $dataBiblioPaging); # Previous biblio if ($paging{'previous'}->{biblionumber}) { - $previous = 'opac-detail.pl?biblionumber=' . $paging{'previous'}->{biblionumber} . '&query_desc=' . $query->param('query_desc'); + $previous = 'opac-detail.pl?biblionumber=' . $paging{'previous'}->{biblionumber} . '&query_desc=' . ($query->param('query_desc') // ''); $dataBiblioPaging = GetBiblioData($paging{'previous'}->{biblionumber}); $template->param('previousTitle' => $dataBiblioPaging->{'title'}) if ($dataBiblioPaging); } # Next biblio if ($paging{'next'}->{biblionumber}) { - $next = 'opac-detail.pl?biblionumber=' . $paging{'next'}->{biblionumber} . '&query_desc=' . $query->param('query_desc'); + $next = 'opac-detail.pl?biblionumber=' . $paging{'next'}->{biblionumber} . '&query_desc=' . ($query->param('query_desc') // ''); $dataBiblioPaging = GetBiblioData($paging{'next'}->{biblionumber}); $template->param('nextTitle' => $dataBiblioPaging->{'title'}) if ($dataBiblioPaging); } @@ -485,6 +489,7 @@ if ( C4::Context->preference('HighlightOwnItemsOnOPAC') ) { } my $dat = &GetBiblioData($biblionumber); +my $OpacHideMARC = GetOpacHideMARC($dat->{'frameworkcode'}); my $itemtypes = GetItemTypes(); # imageurl: @@ -646,7 +651,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, @@ -696,6 +710,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 002c88e..f67f858 100755 --- a/opac/opac-export.pl +++ b/opac/opac-export.pl @@ -35,7 +35,10 @@ my $biblionumber = $query->param("bib")||0; $biblionumber = int($biblionumber); my ($marc, $error)= ('',''); -$marc = GetMarcBiblio($biblionumber, 1) if $biblionumber; +my $unfiltered_marc = GetMarcBiblio($biblionumber, 1) if $biblionumber; +my $frameworkcode = GetFrameworkCode($biblionumber); +$marc = GetFilteredOpacBiblio($unfiltered_marc,$frameworkcode); + if(!$marc) { print $query->redirect("/cgi-bin/koha/errors/404.pl"); exit; @@ -57,7 +60,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 71fb804..380038b 100755 --- a/opac/opac-showmarc.pl +++ b/opac/opac-showmarc.pl @@ -57,6 +57,12 @@ else { 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 4725e53..d50085e 100755 --- a/t/db_dependent/Biblio.t +++ b/t/db_dependent/Biblio.t @@ -3,16 +3,30 @@ # This Koha test module is a stub! # Add more tests here!!! -use strict; -use warnings; -use Test::More tests => 17; +use Modern::Perl; +use Test::More tests => 27; use MARC::Record; use C4::Biblio; +use C4::Context; +use Data::Dumper; BEGIN { use_ok('C4::Biblio'); } +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.'); + my $isbn = '0590353403'; my $title = 'Foundation'; @@ -109,5 +123,41 @@ $field->update('123456789X'); $controlnumber = GetMarcControlnumber( $marc_record, 'MARC21' ); is( $controlnumber, '123456789X', 'GetMarcControlnumber handles records with 001' ); +my $dbh = C4::Context->dbh; + +# the frameworkcode for the added biblio was not set, +# so the frameworkcode is '' for Default. +my $sth = $dbh->prepare("SELECT hidden FROM marc_subfield_structure WHERE tagfield='245' and tagsubfield='a' and frameworkcode='';"); +$sth->execute(); +my $hidden = $sth->fetchrow; +ok ( defined($hidden), 'There is a 245$a entry for the Default framework.'); + +$dbh->do("UPDATE marc_subfield_structure SET hidden=4 WHERE tagfield='245' and tagsubfield='a' and frameworkcode='';"); +$sth = $dbh->prepare("SELECT hidden FROM marc_subfield_structure WHERE tagfield='245' and tagsubfield='a' and frameworkcode='';"); +$sth->execute(); +my $new_hidden = $sth->fetchrow; +ok ( defined($new_hidden) && $new_hidden==4, 'Default framework 245$a 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('245','a'); +my $filtered_title = $filtered_record1->subfield('245','a'); +ok ( defined($title) && !defined($filtered_title), 'GetFilteredOpacBiblio filtered title as expected.'); + +$sth = $dbh->prepare("UPDATE marc_subfield_structure SET hidden=? WHERE tagfield='245' and tagsubfield='a' AND frameworkcode='';"); +$sth->execute($hidden); +$sth = $dbh->prepare("SELECT hidden FROM marc_subfield_structure WHERE tagfield='245' and tagsubfield='a' and frameworkcode='';"); +$sth->execute(); +$new_hidden = $sth->fetchrow; +ok ( $new_hidden==$hidden, 'Default framework 245$a hidden value reset.'); + # clean up after ourselves DelBiblio($biblionumber); -- 1.7.9.5