[Koha-bugs] [Bug 11592] opac scripts do not respect MARC tag visibility

bugzilla-daemon at bugs.koha-community.org bugzilla-daemon at bugs.koha-community.org
Fri Jun 6 08:05:52 CEST 2014


http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11592

--- Comment #41 from David Cook <dcook at prosentient.com.au> ---
Comment on attachment 28480
  --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=28480
Bug 11592 - MARC Visibility settings not respected

Review of attachment 28480:
 --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=11592&attachment=28480)
-----------------------------------------------------------------

Overall, I really want to see this patch get into Koha. However, I think there
is some unnecessary code, some potentially incorrect code, and some code that
probably belongs in a different bug/patch.

::: C4/Biblio.pm
@@ +1322,5 @@
> +    my $display       = ShouldDisplayOnInterface();
> +
> +    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);

Why is there a "my $rv" here? It shouldn't be needed.

@@ +1331,5 @@
> +        my @tmpsplit = split(/\./,$fullfield);
> +        my $field = $tmpsplit[-1];
> +        foreach my $tag (keys %{$data->{$fullfield}}) {
> +            my $show = $display->{$interface}->{ $data->{$fullfield}->{$tag}->{'hidden'} };
> +            if (!$show || $show==0) {

$show should always be 1 or undefined, so I wouldn't think $show==0 would be
necessary

@@ +1334,5 @@
> +            my $show = $display->{$interface}->{ $data->{$fullfield}->{$tag}->{'hidden'} };
> +            if (!$show || $show==0) {
> +                $shouldhidemarc{ $field } = 1;
> +            }
> +            elsif ( !exists($shouldhidemarc{ $field }) ) {

It shouldn't be necessary to add fields that aren't hidden to this hash. Plus,
since the value is 0, you'll most likely get the same end result you would if
you didn't have this block of code.

@@ +1493,5 @@
> +        # LDR field is excluded from $record->fields().
> +        # if we hide it here, the MARCXML->MARC::Record->MARCXML
> +        # transformation blows up.
> +    #}
> +    foreach my $fields ($record->fields()) {

Should probably use $field rather than $fields, as this incorrect pluralization
gets confusing lower down.

@@ +1495,5 @@
> +        # transformation blows up.
> +    #}
> +    foreach my $fields ($record->fields()) {
> +        my $tag = $fields->tag();
> +        my $hidden;

This $hidden variable isn't really necessary. If it should be hidden, you
should just hide/delete it upon detection. The code looks like it would work
either way, but it would be easier to read with less needless code.

@@ +1500,5 @@
> +        if ($tag>=10) {
> +            foreach my $subpairs ($fields->subfields()) {
> +                my ($subtag,$value) = @$subpairs;
> +                my $visibility = $marcsubfieldstructure->{$tag}->{$subtag}->{hidden};
> +                $visibility //= 0;

Is there are a reason to have this extra line instead of "my $visibility =
$marcsubfieldstructure->{$tag}->{$subtag}->{hidden} // 0"?

@@ +1670,4 @@
>                  $oauthors .= "&rft.au=$au";
>              }
>          }
> +        $title = $record->subfield( '245', 'a' ) // '';

Why is this in this patch?

@@ +2122,4 @@
>          push @marcsubjects, {
>              MARCSUBJECT_SUBFIELDS_LOOP => \@subfields_loop,
>              authoritylink => $authoritylink,
> +        } if $authoritylink || @subfields_loop;

Why is this in this patch?

::: C4/XSLT.pm
@@ +93,5 @@
> +                        @new_subfields
> +                    ) );
> +                }
> +                else {
> +                    $record->delete_fields($field);

What is this doing here, and why is it in this patch?

::: opac/opac-detail.pl
@@ +683,4 @@
>  my $marcseriesarray  = GetMarcSeries  ($record,$marcflavour);
>  my $marcurlsarray    = GetMarcUrls    ($record,$marcflavour);
>  my $marchostsarray  = GetMarcHosts($record,$marcflavour);
> +my ($st_tag,$st_subtag) = GetMarcFromKohaField('bibliosubtitle.subtitle',$dat->{'frameworkcode'});

I think that this section is a mistake.

What is bibliosubtitle.subtitle? Also, why would this be in this patch?

::: opac/opac-showmarc.pl
@@ +55,4 @@
>  
>  if ($view eq 'card' || $view eq 'html') {
>      my $xmlrecord= $importid? $record->as_xml(): GetXmlBiblio($biblionumber);
> +    if (!$importid && $view eq 'html') {

I haven't looked at the context, but why not filter for all cases here?

-- 
You are receiving this mail because:
You are watching all bug changes.


More information about the Koha-bugs mailing list