[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

M. Tompsett <mtompset at hotmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Needs Signoff               |ASSIGNED

--- Comment #9 from M. Tompsett <mtompset at hotmail.com> ---
> Will this also hide appropriate fields in opac-ISBDdetail that should be
> hidden?

I didn't even look at opac-ISBDdetail -- we don't use it. I suppose I should.
Though, it has that syspref related to the output for it. I'll add a filter
call.


> > +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).
> > +
> 
> How can this be used to get information about something that doesn't have a
> kohafield attached? e.g. I want to hide marc field 505 from the OPAC, but it
> doesn't have a corresponding Koha field. Would this still work?

OpacHideMARC is intended as a hack for the loop in opac-detail that creates
template parameters based on koha field names. It was around like 699 in the
master, 713 in the patch. This is how [% title %] and other parameters are
created. This covers the half of the problem that doesn't use the MARC record
directly.

If you are trying to hide something that doesn't have a kohafield, you are
looking at the other function: GetFilteredOpacBiblio. This takes the MARC
record and strips out things marked to be hidden.


> > +    my $sth = $dbh->prepare("SELECT kohafield AS field,tagfield AS tag,hidden FROM marc_subfield_structure WHERE LENGTH(kohafield)>0 AND frameworkcode=? ORDER BY field,tagfield;");
> 
> I would suspect that doing WHERE kohafield <> '' might be a tiny bit faster
> than asking it to do a length calculation.

I didn't do testing, but LENGTH(NULL) = 0, right? Which way handles the NULL
case better? -- Just checked >'' is used elsewhere and returns the same number
on my data.


> Probably negligible though. I'd
> probably also fix the spacing as right now it looks like it groups wrongly
> in the columns that it's selecting, even though it doesn't.

I don't understand what you are trying to say. OH! field,tagfield lacks
spacing. Will fix that to improve readability.


> > +            if ($data->{$fullfield}->{$tag}->{'hidden'}>0) {
> 
> !=0 is likely to be marginally faster (unless negatives are a thing you care
> about.)

valid values for hidden range from -15 to +... anyways... <=0 OPAC visibility
is checked. >0 OPAC visibility is unchecked. So, yes, care about negatives.


> 
> @@ +1270,5 @@
> > +    my $filtered_record = $record->clone;
> > +
> > +    my ( $frameworkcode ) = shift || '';
> > +
> > +    my $marcsubfieldstructure = GetMarcStructure(0,$frameworkcode);
> 
> Maybe allow the marcsubfieldstructure to be passed in instead of the
> framework code. This becomes important if this happens over and over, as
> it'll do a big bunch of database work each time, this makes things very slow
> when it could be cached outside and passed in.
> 
> It should be easy enough to see if you have a scalar or a ref, and so
> whether you have a code or the structure.

Hmm... GetMarcStructure is cached. Look in C4/Biblio.pm for "sub
GetMarcStructure". You will see the $marc_structure_cache line just above that,
and it being used at the top of the function.


> @@ +1274,5 @@
> > +    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.
> > +    }
> 
> This if doesn't actually do anything.

It explains why I didn't hide the LDR record. As this function is called only
once, I don't think it is a big deal, but I will comment it out so people don't
get the idea to fix the remaining LDR field problem this way.


> 
> @@ +1977,4 @@
> >          push @marcsubjects, {
> >              MARCSUBJECT_SUBFIELDS_LOOP => \@subfields_loop,
> >              authoritylink => $authoritylink,
> > +        } if $authoritylink || $#subfields_loop>=0;
> 
> $#subfields_loop>=0 is a bit of an ugly construction. Best to use just
> @subfields_loop, it does the same thing and is easier to read.

Okay, I suppose I can try that.


> >  my $dat = &GetBiblioData($biblionumber);
> > +my $OpacHideMARC  = &GetOpacHideMARC($dat->{'frameworkcode'});
> 
> & is a perl4-ism, not required.

Okay, I was confused a little by it used in some places and not in others. I'll
remove my &.


> > +    $subtitle = \@subtitles if scalar @subtitles;
> 
> you don't need to say 'scalar' here.

> > +if ($subtitle && scalar @$subtitle) {
> 
> nor here

I'll test and remove, once confirmed.

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