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

[bugzilla-daemon at bugs.koha-community.org]

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

--- Comment #42 from M. Tompsett <mtompset at hotmail.com> ---
(In reply to David Cook from comment #41)
> ::: C4/Biblio.pm
> > +    my $rv = $sth->execute($frameworkcode);
> 
> Why is there a "my $rv" here? It shouldn't be needed.

It was debugging code that I forgot to strip. :)


> > +            if (!$show || $show==0) {
> 
> $show should always be 1 or undefined, so I wouldn't think $show==0
> would be necessary.

Actually, 1, 0, or undefined. Someone could put => 0 into the hash checked.
I'll confirm if I can simplify the logic.


> @@ +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.

I don't like having undefined values. Those tend to generate annoying floody
errors when some comparison works but was expecting a number, for example.


> @@ +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.

Okay, that makes sense.


> @@ +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.

If you look at the hidden value setting that would be inside an if/else. I
scoped it wider, so there was no need to have two lines of code. Fine, two
lines of code it is.


> > +                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"?

Yes, because otherwise visibility is undef. It should be a valid number (-7 to
+7) reflecting valid values set in the Advanced Constraints screen.


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

This fixes a floody error I had during testing. I suppose it could be another
patch. I'll confirm.


> @@ +2122,4 @@
> >          push @marcsubjects, {
> >              MARCSUBJECT_SUBFIELDS_LOOP => \@subfields_loop,
> >              authoritylink => $authoritylink,
> > +        } if $authoritylink || @subfields_loop;
> 
> Why is this in this patch?

This properly hides the subjects.


> ::: C4/XSLT.pm
> @@ +93,5 @@
> > +                        @new_subfields
> > +                    ) );
> > +                }
> > +                else {
> > +                    $record->delete_fields($field);
> 
> What is this doing here, and why is it in this patch?

If you try to delete the last subfield of a field, you need to delete the
field. If I recall, deleting the last subfield fails. Fields/subfields are
deleted (from memory) to hide them from being displayed.


> ::: 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.

GetRecordValue, which is what was there initially, does not grab values from
marc_subfield_structure, but rather fieldmapping. This means the subtitle is
not properly hidden.


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

The value in the marc_subfield_structure table. Subtitles are a mess in Koha.


> ::: 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?

I think this is related to the save as exports. This is what worked for me.

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