Bug 11592 - opac detail scripts do not respect MARC tag visibility
Summary: opac detail scripts do not respect MARC tag visibility
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P3 major (vote)
Assignee: M. Tompsett
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 7933 (view as bug list)
Depends on: 15382 15777 15870
Blocks: 10589 17527 18371
  Show dependency treegraph
 
Reported: 2014-01-22 05:06 UTC by M. Tompsett
Modified: 2018-12-03 20:04 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11592 - opac scripts do not respect marc tag visibility (7.76 KB, patch)
2014-01-22 05:30 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (11.29 KB, patch)
2014-01-22 05:33 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (11.31 KB, patch)
2014-01-22 16:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (11.30 KB, patch)
2014-01-22 18:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (16.80 KB, patch)
2014-01-23 13:32 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (17.93 KB, patch)
2014-01-23 15:16 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (18.57 KB, patch)
2014-01-24 02:56 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (18.74 KB, patch)
2014-01-24 03:00 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (18.74 KB, patch)
2014-01-24 03:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (18.91 KB, patch)
2014-01-24 03:33 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.59 KB, patch)
2014-01-24 03:34 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.33 KB, patch)
2014-01-24 03:38 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (18.91 KB, patch)
2014-01-24 03:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.33 KB, patch)
2014-01-24 03:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.37 KB, patch)
2014-01-30 04:10 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (18.96 KB, patch)
2014-01-30 04:10 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.44 KB, patch)
2014-01-31 15:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (19.02 KB, patch)
2014-01-31 15:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.44 KB, patch)
2014-05-07 03:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - opac scripts do not respect marc tag visibility (19.42 KB, patch)
2014-05-07 03:48 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (28.63 KB, patch)
2014-05-26 14:31 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (30.92 KB, patch)
2014-05-26 14:55 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.43 KB, patch)
2014-05-26 15:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (31.84 KB, patch)
2014-05-26 15:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (31.48 KB, patch)
2014-06-09 03:09 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (31.76 KB, patch)
2014-09-04 00:31 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.43 KB, patch)
2014-10-16 02:38 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (31.76 KB, patch)
2014-10-16 02:38 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.43 KB, patch)
2014-11-07 03:31 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.43 KB, patch)
2014-11-07 03:32 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (32.17 KB, patch)
2014-11-07 03:32 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (32.20 KB, patch)
2014-11-19 19:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (32.26 KB, patch)
2015-02-12 05:51 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (32.26 KB, patch)
2015-02-12 05:52 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (10.54 KB, patch)
2015-02-12 05:53 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11592 - MARC Visibility settings not respected (32.26 KB, patch)
2015-02-12 05:53 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11592 - Old Filtery stuff (11.88 KB, patch)
2016-02-12 15:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (5.57 KB, patch)
2016-02-12 15:55 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Old Filtery stuff (10.86 KB, patch)
2016-02-12 15:55 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (5.36 KB, patch)
2016-02-12 16:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Add ShouldHideMARC to ViewPolicy filter (2.51 KB, patch)
2016-03-17 23:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Applying filtering to opac interface. (7.45 KB, patch)
2016-03-17 23:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Biblio tweak for MARCSUBJECT (723 bytes, patch)
2016-03-17 23:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Tweak C4/XSLT to delete field when no subfields left. (1.41 KB, patch)
2016-03-17 23:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Add should_hide_marc method to filter (6.86 KB, patch)
2016-03-29 01:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Applying filtering to opac interface. (7.45 KB, patch)
2016-03-29 01:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Biblio tweak for MARCSUBJECT (723 bytes, patch)
2016-03-29 01:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Tweak C4/XSLT to delete field when no subfields left. (1.41 KB, patch)
2016-03-29 01:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Normal view after patch and db update (101.66 KB, image/png)
2016-04-20 23:07 UTC, Nick Clemens
Details
marc virew after db and patch (246.17 KB, image/png)
2016-04-20 23:08 UTC, Nick Clemens
Details
isbd after db and patch (117.13 KB, image/png)
2016-04-20 23:08 UTC, Nick Clemens
Details
Bug 11592: MARCView and ISBD followup (12.22 KB, patch)
2016-04-21 01:52 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592: MARCView and ISBD followup (14.96 KB, patch)
2016-04-21 21:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (5.42 KB, patch)
2016-04-21 23:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11592 - Add should_hide_marc method to filter (6.92 KB, patch)
2016-04-21 23:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Applying filtering to opac interface. (7.51 KB, patch)
2016-04-21 23:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Biblio tweak for MARCSUBJECT (780 bytes, patch)
2016-04-21 23:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11592: MARCView and ISBD followup (15.01 KB, patch)
2016-04-21 23:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (5.06 KB, patch)
2016-05-09 01:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Add should_hide_marc method to filter (6.92 KB, patch)
2016-05-09 01:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Applying filtering to opac interface. (7.51 KB, patch)
2016-05-09 01:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Biblio tweak for MARCSUBJECT (780 bytes, patch)
2016-05-09 01:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592: MARCView and ISBD followup (15.90 KB, patch)
2016-05-09 01:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592: MARCView and ISBD followup (15.24 KB, patch)
2016-05-09 01:57 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592: MARCView and ISBD followup (15.24 KB, patch)
2016-05-09 14:08 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Updated License Text and use Modern::Perl (4.66 KB, patch)
2016-07-15 15:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592 - Add should_hide_marc method to filter (6.92 KB, patch)
2016-07-15 15:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Applying filtering to opac interface. (7.51 KB, patch)
2016-07-15 15:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Biblio tweak for MARCSUBJECT (780 bytes, patch)
2016-07-15 15:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592: MARCView and ISBD followup (15.24 KB, patch)
2016-07-15 15:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592: Updated License Text and use Modern::Perl (4.71 KB, patch)
2016-08-30 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11592: Add should_hide_marc method to filter (6.97 KB, patch)
2016-08-30 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11592: Applying filtering to opac interface. (7.57 KB, patch)
2016-08-30 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11592: Biblio tweak for MARCSUBJECT (846 bytes, patch)
2016-08-30 16:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11592: MARCView and ISBD followup (15.34 KB, patch)
2016-08-30 16:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11592: (QA followup) Simplify code (11.69 KB, patch)
2016-08-30 16:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11592: (QA followup) Add missing framework code to ViewPolicy filter calls (10.40 KB, patch)
2016-08-30 16:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11592: (QA followup) Simplify code (11.74 KB, patch)
2016-09-05 15:38 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11592: (QA followup) Add missing framework code to ViewPolicy filter calls (10.46 KB, patch)
2016-09-05 15:38 UTC, M. Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description M. Tompsett 2014-01-22 05:06:04 UTC
It is possible to change the visibility in OPAC by affecting the value of hidden in marc_subfield_structure (Home->Administration->MARC frameworks->BKS Subfields -> ... -> Advanced Constraints -> Uncheck/Check OPAC).

opac-MARCdetail.pl, for the most part, respects it, but if things normally displayed in opac-detail.pl are marked as not having visibility in OPAC (unchecked), they are still displayed.
Comment 1 M. Tompsett 2014-01-22 05:30:44 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2014-01-22 05:33:21 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2014-01-22 05:41:24 UTC
Still to do: Add tests for new functions to C4/Biblio. :)
Comment 4 M. Tompsett 2014-01-22 16:02:44 UTC Comment hidden (obsolete)
Comment 5 M. Tompsett 2014-01-22 18:36:05 UTC Comment hidden (obsolete)
Comment 6 M. Tompsett 2014-01-23 13:32:13 UTC Comment hidden (obsolete)
Comment 7 M. Tompsett 2014-01-23 15:16:24 UTC Comment hidden (obsolete)
Comment 8 Robin Sheat 2014-01-23 22:35:32 UTC
Comment on attachment 24655 [details] [review]
Bug 11592 - opac scripts do not respect marc tag visibility

Review of attachment 24655 [details] [review]:
-----------------------------------------------------------------

Will this also hide appropriate fields in opac-ISBDdetail that should be hidden?

::: C4/Biblio.pm
@@ +1222,5 @@
> +
> +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?

@@ +1230,5 @@
> +
> +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 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. 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.

@@ +1238,5 @@
> +    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) {

!=0 is likely to be marginally faster (unless negatives are a thing you care about.)

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

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

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

::: opac/opac-detail.pl
@@ +489,4 @@
>  }
>  
>  my $dat = &GetBiblioData($biblionumber);
> +my $OpacHideMARC  = &GetOpacHideMARC($dat->{'frameworkcode'});

& is a perl4-ism, not required.

@@ +654,5 @@
> +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 scalar @subtitles;

you don't need to say 'scalar' here.

@@ +656,5 @@
> +if ($st_tag && $st_subtag) {
> +    my @subtitles = $record->subfield($st_tag,$st_subtag);
> +    $subtitle = \@subtitles if scalar @subtitles;
> +}
> +if ($subtitle && scalar @$subtitle) {

nor here
Comment 9 M. Tompsett 2014-01-24 02:45:53 UTC
> 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.
Comment 10 Robin Sheat 2014-01-24 02:52:15 UTC
(In reply to M. Tompsett from comment #9)
> 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.

Ah, interesting. It's also cached in memcache which actually slows things down. This explains why.
Comment 11 M. Tompsett 2014-01-24 02:56:23 UTC Comment hidden (obsolete)
Comment 12 M. Tompsett 2014-01-24 03:00:26 UTC Comment hidden (obsolete)
Comment 13 M. Tompsett 2014-01-24 03:02:41 UTC Comment hidden (obsolete)
Comment 14 M. Tompsett 2014-01-24 03:14:49 UTC
opac-search.pl is out of scope for now.
opac-ISDBdetail.pl could be cleaned up more, but the focus of this was more consistency between opac-MARCdetail and opac-detail pages.
Comment 15 M. Tompsett 2014-01-24 03:33:50 UTC Comment hidden (obsolete)
Comment 16 M. Tompsett 2014-01-24 03:34:02 UTC Comment hidden (obsolete)
Comment 17 M. Tompsett 2014-01-24 03:38:26 UTC Comment hidden (obsolete)
Comment 18 M. Tompsett 2014-01-24 03:43:05 UTC Comment hidden (obsolete)
Comment 19 M. Tompsett 2014-01-24 03:43:15 UTC Comment hidden (obsolete)
Comment 20 Robin Sheat 2014-01-30 04:10:18 UTC Comment hidden (obsolete)
Comment 21 Robin Sheat 2014-01-30 04:10:35 UTC Comment hidden (obsolete)
Comment 22 Robin Sheat 2014-01-30 04:11:34 UTC
(sorry, attached them the wrong way around due to expecting git bz to work like most git commands.)
Comment 23 Kyle M Hall 2014-01-31 15:58:56 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2014-01-31 15:59:17 UTC Comment hidden (obsolete)
Comment 25 David Cook 2014-03-27 03:42:59 UTC
Looking forward to this one going in. It would be a great help...
Comment 26 David Cook 2014-03-27 06:27:30 UTC
A few additional thoughts...

1)
A follow-up should probably be added for a few other OPAC scripts like opac-basket.pl, opac-sendbasket.pl, opac-sendshelf.pl, etc., since these will all show the unfiltered MARC record.


2)
It would be nice to see a future version of this that filters for both the OPAC and the staff client. 

Passing a "context" in and checking the 'hidden' values against a hashref of hashrefs might be a quick way of doing that:

my $display = {
            opac => {
                0 => 1,
                -1 => 1,
                -2 => 1,
                -3 => 1,
                -4 => 1,
                -5 => 1,
                -6 => 1,
                -7 => 1, 
            },
            intranet => {
                -6 => 1,
                -5 => 1,
                -1 => 1,
                0 => 1,
                1 => 1,
                4 => 1,
                6 => 1,
                7 => 1,  
            },
        };

3) I've added a few notes in Splinter Review for ways I think that the filter sub could be improved. Since this has already passed QA, I'll just leave them as notes.

If it's too late to make changes, I'll probably send a patch with some of them after it's pushed in any case.
Comment 27 M. Tompsett 2014-04-22 04:27:32 UTC
ARG! This no longer applies. And because the t/db_dependent/Biblio.t file has gotten more complicated that a straight list of tests, I'm not sure how to rebase.
Comment 28 M. Tompsett 2014-04-22 04:43:10 UTC
Rebase need caused by bug 11912.
Comment 29 M. Tompsett 2014-05-06 04:01:57 UTC
I think I have something working, but I need to get my unimarc VM updated and do a test on it first.
Comment 30 M. Tompsett 2014-05-07 03:47:55 UTC Comment hidden (obsolete)
Comment 31 M. Tompsett 2014-05-07 03:48:13 UTC Comment hidden (obsolete)
Comment 32 M. Tompsett 2014-05-07 04:08:14 UTC
Since bug 11912 forced Biblio.t to UNIMARC and MARC21 marcflavour the tests, I needed to tweak the test cases to properly handle UNIMARC vs. MARC21 testing. It would be good to see if someone that uses UNIMARC could signoff this bug.

The test plan is the same as comment 31, but obviously 200$a is title under UNIMARC. My test cases ran as expected under UNIMARC and MARC21. Test both. :)

Perhaps I should have removed the sign offs from the second patch?
Comment 33 M. Tompsett 2014-05-14 04:02:12 UTC
(In reply to David Cook from comment #26)
> A few additional thoughts...
> 
> 1)
> A follow-up should probably be added for a few other OPAC scripts like
> opac-basket.pl, opac-sendbasket.pl, opac-sendshelf.pl, etc., since these
> will all show the unfiltered MARC record.

Ah, right. GetMarcBiblio.


> 2)
> It would be nice to see a future version of this that filters for both the
> OPAC and the staff client. 
> 
> Passing a "context" in and checking the 'hidden' values against a hashref of
> hashrefs might be a quick way of doing that...

I am reconsidering these suggestions, since tweaking a lot of file is as much of a pain as testing the far reaching effects of modifying GetMarcBiblio.
Comment 34 M. Tompsett 2014-05-14 12:52:44 UTC
And this is where converting the parameters of GetMarcBiblio to a hash reference would be useful, because optional parameters are a pain otherwise. *sigh*
Comment 35 M. Tompsett 2014-05-15 19:04:20 UTC
Moving back to assigned, while I think and work on this a little more.
Comment 36 M. Tompsett 2014-05-26 14:31:56 UTC Comment hidden (obsolete)
Comment 37 M. Tompsett 2014-05-26 14:55:34 UTC Comment hidden (obsolete)
Comment 38 M. Tompsett 2014-05-26 15:39:43 UTC Comment hidden (obsolete)
Comment 39 M. Tompsett 2014-05-26 15:39:56 UTC Comment hidden (obsolete)
Comment 40 M. Tompsett 2014-05-26 15:41:13 UTC
I think this is it.
Comment 41 David Cook 2014-06-06 06:05:52 UTC
Comment on attachment 28480 [details] [review]
Bug 11592 - MARC Visibility settings not respected

Review of attachment 28480 [details] [review]:
-----------------------------------------------------------------

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 .= "&amp;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?
Comment 42 M. Tompsett 2014-06-06 06:36:28 UTC
(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 .= "&amp;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.
Comment 43 M. Tompsett 2014-06-06 07:45:14 UTC
(In reply to David Cook from comment #41)
> @@ +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.
> 

SELECT tagfield,tagsubfield,kohafield FROM marc_subfield_structure WHERE frameworkcode='' AND tagfield IN ('541','952') ORDER BY kohafield;

items.stocknumber is a pain. This is why this code exists. If the first one is hidden (I didn't sort the keys, so it is "random") or visible, the other value doesn't affect it.
Comment 44 M. Tompsett 2014-06-09 03:09:34 UTC Comment hidden (obsolete)
Comment 45 M. Tompsett 2014-06-09 05:54:27 UTC
I changed the title of be 'opac detail scripts', because there are still various OPAC pages this patch doesn't fix. However, these patches provide a way to correct those other problems. I'm hoping others will have time to fix them.
Comment 46 Chris Cormack 2014-09-03 22:27:40 UTC
The second patch has conflicts in the 2 test files, if you fix them up I'll sign off
Comment 47 M. Tompsett 2014-09-04 00:31:07 UTC Comment hidden (obsolete)
Comment 48 M. Tompsett 2014-10-16 02:38:46 UTC Comment hidden (obsolete)
Comment 49 M. Tompsett 2014-10-16 02:38:57 UTC Comment hidden (obsolete)
Comment 50 Fridolin SOMERS 2014-10-29 19:07:02 UTC
*** Bug 7933 has been marked as a duplicate of this bug. ***
Comment 51 Katrin Fischer 2014-11-06 21:28:32 UTC
The patches currently don't apply for me:

Apply? [(y)es, (n)o, (i)nteractive] i
Applying: Bug 11592 - MARC Visibility settings not respected
fatal: sha1 information is lacking or useless (C4/Biblio.pm).
Comment 52 M. Tompsett 2014-11-07 00:49:34 UTC
The removal of the prog theme broke this, specifically bug 13170. Bug 11912 and other related bugs which touched t/db_dependent/Biblio.t affected this as well. I'm working on a rebase.
Comment 53 M. Tompsett 2014-11-07 00:53:24 UTC
(In reply to M. Tompsett from comment #52)
> Bug 11912 and other related bugs which touched
> t/db_dependent/Biblio.t affected this as well.
> I'm working on a rebase.

Oops. Bug 12570, because it was t/Biblio.t :)
Comment 54 M. Tompsett 2014-11-07 03:31:32 UTC Comment hidden (obsolete)
Comment 55 M. Tompsett 2014-11-07 03:32:07 UTC Comment hidden (obsolete)
Comment 56 M. Tompsett 2014-11-07 03:32:15 UTC Comment hidden (obsolete)
Comment 57 M. Tompsett 2014-11-19 19:03:29 UTC Comment hidden (obsolete)
Comment 58 Brendan Gallagher 2015-02-12 05:51:16 UTC Comment hidden (obsolete)
Comment 59 Brendan Gallagher 2015-02-12 05:52:30 UTC Comment hidden (obsolete)
Comment 60 Brendan Gallagher 2015-02-12 05:53:28 UTC Comment hidden (obsolete)
Comment 61 Brendan Gallagher 2015-02-12 05:53:49 UTC Comment hidden (obsolete)
Comment 62 Jonathan Druart 2015-02-12 11:10:32 UTC
Be care, prototype of GetMarcBiblio is modified here, bug 12252 did it too!
Comment 63 Jonathan Druart 2015-02-12 11:11:37 UTC
Comment on attachment 35858 [details] [review]
Bug 11592 - MARC Visibility settings not respected

Review of attachment 35858 [details] [review]:
-----------------------------------------------------------------

::: C4/Biblio.pm
@@ +1369,5 @@
>  =cut
>  
>  sub GetMarcBiblio {
> +    my @catch_parameters = @_;
> +

Passing a hashref would be better.
Comment 64 M. Tompsett 2015-02-12 12:06:13 UTC
(In reply to Jonathan Druart from comment #62)
> Be care, prototype of GetMarcBiblio is modified here, bug 12252 did it too!

This change catches the old way and a hashref. Though, because a third parameter was added in 12252, this would require a more detailed look there. However, that would more likely come anyways if that is pushed to master first anyways, because this would need a rebase. Thanks for the heads up.
Comment 65 Tomás Cohen Arazi 2015-03-10 20:31:50 UTC
(In reply to M. Tompsett from comment #64)
> (In reply to Jonathan Druart from comment #62)
> > Be care, prototype of GetMarcBiblio is modified here, bug 12252 did it too!
> 
> This change catches the old way and a hashref. Though, because a third
> parameter was added in 12252, this would require a more detailed look there.
> However, that would more likely come anyways if that is pushed to master
> first anyways, because this would need a rebase. Thanks for the heads up.

I would consider this bugfix if it was implemented as a Koha::RecordProcessor filter for MARC::Record objects, rather than just another hook to C4::Search. That way we could use it wherever we need it.

BUT I have to say that I've been playing with an implementation that works with MARCMXL instead (using XSLT) and could help us simplicy results rendering and probably performance.

I'm trying to finish the patches (the hackfest had me busy with other stuff) so I can attach them for peer review.
Comment 66 M. Tompsett 2015-03-10 23:10:56 UTC
(In reply to Tomás Cohen Arazi from comment #65)
> I would consider this bugfix if it was implemented as a
> Koha::RecordProcessor filter for MARC::Record objects, rather than just
> another hook to C4::Search. That way we could use it wherever we need it.

This does NOT touch C4::Search at all.

My initial worry is the whole inclusion loop making debugging a pain!
But your idea of effectively moving all the stuff added to C4::Biblio into a filter module is good, if there isn't some magical loop which makes debugging a pain.


> BUT I have to say that I've been playing with an implementation that works
> with MARCMXL instead (using XSLT) and could help us simplicy results
> rendering and probably performance.
> 
> I'm trying to finish the patches (the hackfest had me busy with other stuff)
> so I can attach them for peer review.

I was going to attempt to go that way. My concern is if we have MARCXML filters and non-XML filters, we need to make sure they are identical. I suppose part of the unit tests to add into the continuous integration. :)

This is why I was wondering if perhaps we would go completely MARCXML internally, as I vaguely recall some indexing limit which you hit for a MARC Record, but perhaps that is unrelated.
Comment 67 Marcel de Rooy 2015-04-17 07:31:32 UTC
tcohen: could you please update your comments on bug 11592 as related to bug 12252
Comment 68 Marcel de Rooy 2015-04-17 14:38:20 UTC
Some comments from IRC:

[16:32]	marcelr	so i was curious about your position before starting qa
[16:32]	tcohen	my decision about that kind of stuff is that we should definitely resurrect Koha::RecordProcessor
[16:33]	tcohen	so, Mark should implement (if that's what he wants) a Koha::Filter for MARC records
[16:33]	marcelr	which would mean: rewrite the patch set first
[16:33]	tcohen	I wrote my own for MARCXML (in the form of XML::LibXML::Document objects, which is the result of parsing XML)
[16:33]	tcohen	so, I already have an implementation
[16:34]	tcohen	that "can be used" wherever we want
[16:34]	tcohen	the problem with MArk's is that it is MARC::Record specific, and is hooked in the wrong place (IMHO)
[16:34]	tcohen	it can be discussed
[16:34]	marcelr	where is yours now?
[16:35]	tcohen	https://github.com/tomascohen/koha/tree/xslt_filtering
[16:35]	tcohen	I introduced a Koha::Filter::MARCXML::ViewPolicy filter class

Returning to this later..
Comment 69 M. Tompsett 2015-04-17 15:12:30 UTC
(In reply to M. de Rooy from comment #68)
> Some comments from IRC:
> [16:33]	tcohen	so, Mark should implement (if that's what he wants) a
> Koha::Filter for MARC records

Which is something I will get around to doing.
Comment 70 Mason James 2015-06-17 06:23:45 UTC
I increased the severity of this big should be increased, as it's a big security problem
Comment 71 Jonathan Druart 2015-06-23 14:39:47 UTC
What's next here?
Is it ready to QA?
Looking at the last comments I'd say no.
Comment 72 M. Tompsett 2015-06-23 14:50:01 UTC
I have yet to implement a filter. This would be a huge rewrite.
However, some of the things in this patch could be split off to tinier bugs.
Comment 73 M. Tompsett 2015-12-15 02:46:23 UTC
Progress is being made on this bug. I have a filter. Now to break the mess into parts, and assemble in small testable pieces while tweaking other code to use it.
Comment 74 M. Tompsett 2016-02-12 15:25:40 UTC Comment hidden (obsolete)
Comment 75 M. Tompsett 2016-02-12 15:55:06 UTC Comment hidden (obsolete)
Comment 76 M. Tompsett 2016-02-12 15:55:15 UTC Comment hidden (obsolete)
Comment 77 M. Tompsett 2016-02-12 16:02:03 UTC Comment hidden (obsolete)
Comment 78 M. Tompsett 2016-03-10 00:30:30 UTC
Comment on attachment 47997 [details] [review]
Bug 11592 - Old Filtery stuff

Bug 15870 and bug 15777 contain the equivalent code.
Comment 79 M. Tompsett 2016-03-17 23:26:21 UTC Comment hidden (obsolete)
Comment 80 M. Tompsett 2016-03-17 23:26:33 UTC Comment hidden (obsolete)
Comment 81 M. Tompsett 2016-03-17 23:26:40 UTC Comment hidden (obsolete)
Comment 82 M. Tompsett 2016-03-17 23:26:50 UTC Comment hidden (obsolete)
Comment 83 M. Tompsett 2016-03-17 23:30:49 UTC
Applying bug 15870 and then the last 5 patches above, should give something testable. However, as this is still kind of being worked on, I thought I'd just share this to get feedback on whether the ShouldHideMARC should be on bug 15870 (which has not passed QA), or whether it could be here.

Problem outstanding XML filtering. Any word on a bugzilla report for that, Tomas? I recall you mentioning some sample test code somewhere, but I forget the URL.
Comment 84 M. Tompsett 2016-03-28 03:56:19 UTC
Comment on attachment 35858 [details] [review]
Bug 11592 - MARC Visibility settings not respected

Except for some lack of MARCXML filtering, this should mostly be replaced by the other patches added here.
Comment 85 M. Tompsett 2016-03-28 04:02:05 UTC
See the test plan in comment #61.
Known failings: MARCXML stuff.
I've toggled this out of "In Discussion" as I think it is best to get something into Master (MARC filtering) rather than nothing. I'll fully test later, and set to Needs Signoff after I'm satisfied.
Comment 86 Tomás Cohen Arazi 2016-03-28 14:29:40 UTC
(In reply to M. Tompsett from comment #79)
> Created attachment 49288 [details] [review] [review]
> Add ShouldHideMARC to ViewPolicy filter

I think that function is a mistake! Should be using C4::Biblio::GetMarcStructure instead. C4::Biblio::GetMarcStructure is cached already, and using yours will only make performance worse.
Comment 87 M. Tompsett 2016-03-28 17:19:40 UTC
(In reply to Tomás Cohen Arazi from comment #86)
> I think that function [ShouldHideMARC] is a mistake! Should be using
> C4::Biblio::GetMarcStructure instead. C4::Biblio::GetMarcStructure is cached
> already, and using yours will only make performance worse.

Granted, another SQL query is a performance hit. However, the result is not keyed on kohafield. I'll post a revision that uses GetMarcStructure, as per your suggestion.
Comment 88 M. Tompsett 2016-03-29 01:05:25 UTC Comment hidden (obsolete)
Comment 89 M. Tompsett 2016-03-29 01:05:35 UTC Comment hidden (obsolete)
Comment 90 M. Tompsett 2016-03-29 01:05:45 UTC Comment hidden (obsolete)
Comment 91 M. Tompsett 2016-03-29 01:05:57 UTC Comment hidden (obsolete)
Comment 92 M. Tompsett 2016-03-29 01:08:46 UTC
See the test plan in comment #61.
Known failings: MARCXML stuff.
Known failings: LEADER fields.
Desired extensions beyond the scope of this particular bug: Authorized Values.
I still have to fully test. But this is provided so as to see whether it addresses comment #86 sufficiently.
Comment 93 M. Tompsett 2016-03-30 03:24:45 UTC
This test plan supercedes comment #61. Just one tiny piece missing for 100% code change testing.

Known failings: MARCXML stuff.
Known failings: LEADER fields.

TEST PLAN
---------
 1) Backup your DB
 2) run the following SQL on your DB.
    > UPDATE marc_subfield_structure set hidden=-8;
    -- this should set EVERYTHING to hidden across the board.
 3) In staff client, set OPACXSLTDetailsDisplay to blank
 4) In OPAC, view any detail.
    -- Normal view may mostly leak values still.
    -- MARC view may leak values.
    -- ISBD view may leak values.
 5) In staff client, set OPACXSLTDetailsDisplay to default
 6) In OPAC, view any detail.
    -- same issues as step 4
    -- 'View Plain' may leak too.
 7) 'Save record' -> 'Dublin Core'
 8) Apply this patch
 9) run koha qa test tools
    -- should be fine
10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t
    -- should pass
    -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too
11) In OPAC, view any detail.
    -- Normal view:
       -- Material type comes from the LEADER field.
       -- Lists this is on will still display
       -- 'Tags from this library' will still display
       -- Item information in table will still display
          (THIS IS BEYOND SCOPE)
    -- MARC view:
       -- Record number is leaked
          (THIS IS BEYOND SCOPE)
       -- 'View plain' leaks LEADER field.
    -- ISBD view may leak field headings, but not values.
       (THIS IS BEYOND SCOPE)
12) In staff client, set OPACXSLTDetailsDisplay to blank
13) In OPAC, view any detail.
    -- same kind of output as step 10
14) 'Save record' -> BIBTEXT
    -- Should be next to nothing leaked.
15) 'Save record' -> Dublin Core
    -- Should be the same or less leaked between the two versions.
    -- (XML FILTERING IS BEYOND SCOPE)
16) Still trying to remember how I triggered the XSLT change.
Comment 94 M. Tompsett 2016-04-08 13:49:11 UTC
Okay, that last patch requires bad data to trigger it as far as I can tell. I did, however, notice another problem in C4/XSLT, that I'll make a separate bug for that.
Comment 95 M. Tompsett 2016-04-08 13:49:52 UTC
Comment on attachment 49615 [details] [review]
Tweak C4/XSLT to delete field when no subfields left.

Can only be triggered by bad data, as far as I can tell. This change is not required.
Comment 96 M. Tompsett 2016-04-08 13:50:28 UTC
See the test plan in comment #93.
Comment 97 Nick Clemens 2016-04-20 22:50:41 UTC
Testing, I see a huge change in the 'Normal view' but little to no change in either the MARC or ISBD view

Export bibtex or DublinCore are both almost empty after patch.

Tests pass.

If I understood the test plan I think I should see nothing/next to nothing in the MARC/ISBD views but I still see most of the record.
Comment 98 M. Tompsett 2016-04-20 23:02:51 UTC
(In reply to Nick Clemens from comment #97)
> Testing, I see a huge change in the 'Normal view' but little to no change in
> either the MARC or ISBD view

How did you test?
Did you do:
UPDATE marc_subfield_structure set hidden=-8;
Because if you only do one field at a time, obviously, only that field/subfield will disappear.


> If I understood the test plan I think I should see nothing/next to nothing
> in the MARC/ISBD views but I still see most of the record.

Yes, exactly.
Screen shots. Because it works fine for me.
I'll post mine shortly.
Comment 99 Nick Clemens 2016-04-20 23:07:38 UTC Comment hidden (obsolete)
Comment 100 Nick Clemens 2016-04-20 23:08:01 UTC Comment hidden (obsolete)
Comment 101 Nick Clemens 2016-04-20 23:08:18 UTC Comment hidden (obsolete)
Comment 102 M. Tompsett 2016-04-21 00:52:16 UTC
I think some ISBD bug which may have gotten pushed recently caused a bit of fun here. The GetISBDView function needs massive reworking.
I believe I have the MARCView working.
Comment 103 M. Tompsett 2016-04-21 01:52:03 UTC Comment hidden (obsolete)
Comment 104 M. Tompsett 2016-04-21 01:59:29 UTC
Comment on attachment 50484 [details]
Normal view after patch and db update

confirmed.
Comment 105 M. Tompsett 2016-04-21 01:59:55 UTC
Comment on attachment 50485 [details]
marc virew after db and patch

Confirmed and patched.
Comment 106 M. Tompsett 2016-04-21 02:02:19 UTC
Comment on attachment 50486 [details]
isbd after db and patch

Confirmed and patched.
This led to a GetISBDView refactor.
It also led to the discovery of leakage in the carts and lists.
Lists are mostly dealt with. Carts partially, but may not be noticeable.
Comment 107 M. Tompsett 2016-04-21 20:48:00 UTC
Comment on attachment 50489 [details] [review]
Bug 11592: MARCView and ISBD followup

Review of attachment 50489 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-ISBDdetail.pl
@@ +92,4 @@
>      }
>  }
>  
> +my $record_unfiltered = GetMarcBiblio($biblionumber,1);

This is because GetMarcBiblio was previously called in C4::Biblio::GetISBDView with ,1 which includes embedded items.

::: opac/opac-MARCdetail.pl
@@ +108,5 @@
>  my ($bt_tag,$bt_subtag) = GetMarcFromKohaField('biblio.title',$itemtype);
>  $template->param(
>      bibliotitle => $biblio->{title},
> +) if $tagslib->{$bt_tag}->{$bt_subtag}->{hidden} <= 0 && # <=0 OPAC visible.
> +     $tagslib->{$bt_tag}->{$bt_subtag}->{hidden} > -8;   # except -8;

Forgot the -8 (flagged) case.
Comment 108 M. Tompsett 2016-04-21 21:21:40 UTC Comment hidden (obsolete)
Comment 109 Nick Clemens 2016-04-21 23:44:22 UTC Comment hidden (obsolete)
Comment 110 Nick Clemens 2016-04-21 23:44:29 UTC Comment hidden (obsolete)
Comment 111 Nick Clemens 2016-04-21 23:44:36 UTC Comment hidden (obsolete)
Comment 112 Nick Clemens 2016-04-21 23:44:41 UTC Comment hidden (obsolete)
Comment 113 Nick Clemens 2016-04-21 23:44:48 UTC Comment hidden (obsolete)
Comment 114 Katrin Fischer 2016-05-08 13:42:09 UTC
I am sorry, this patch no longer applies:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 11592 - Updated License Text and use Modern::Perl
Using index info to reconstruct a base tree...
M	C4/Biblio.pm
M	C4/XSLT.pm
M	opac/opac-detail.pl
Falling back to patching base and 3-way merge...
Auto-merging C4/XSLT.pm
Auto-merging C4/Biblio.pm
Applying: Bug 11592 - Add should_hide_marc method to filter
Applying: Applying filtering to opac interface.
Applying: Biblio tweak for MARCSUBJECT
Applying: Bug 11592: MARCView and ISBD followup
fatal: sha1 information is lacking or useless (C4/Biblio.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 115 M. Tompsett 2016-05-09 00:27:25 UTC
The OPACISBD variable bug caused this. I'll work on a rebase.
Comment 116 M. Tompsett 2016-05-09 01:43:01 UTC Comment hidden (obsolete)
Comment 117 M. Tompsett 2016-05-09 01:43:10 UTC Comment hidden (obsolete)
Comment 118 M. Tompsett 2016-05-09 01:43:18 UTC Comment hidden (obsolete)
Comment 119 M. Tompsett 2016-05-09 01:43:24 UTC Comment hidden (obsolete)
Comment 120 M. Tompsett 2016-05-09 01:43:32 UTC Comment hidden (obsolete)
Comment 121 M. Tompsett 2016-05-09 01:57:38 UTC Comment hidden (obsolete)
Comment 122 M. Tompsett 2016-05-09 14:08:32 UTC Comment hidden (obsolete)
Comment 123 Tomás Cohen Arazi 2016-07-15 14:06:10 UTC
Mark, I'm sorry but could you please rebase this?
Comment 124 M. Tompsett 2016-07-15 15:22:57 UTC Comment hidden (obsolete)
Comment 125 M. Tompsett 2016-07-15 15:23:05 UTC Comment hidden (obsolete)
Comment 126 M. Tompsett 2016-07-15 15:23:13 UTC Comment hidden (obsolete)
Comment 127 M. Tompsett 2016-07-15 15:23:21 UTC Comment hidden (obsolete)
Comment 128 M. Tompsett 2016-07-15 15:23:34 UTC Comment hidden (obsolete)
Comment 129 Katrin Fischer 2016-08-15 21:40:43 UTC
Hi Mark,
looking at https://wiki.koha-community.org/wiki/Hidden_values:
-8 is described as "flag" - which noone knows exactly what it means I think. For "Everything hidden everywhere" I would have picked 8 or 5. 
Can you explain your choice of -8?
Comment 130 M. Tompsett 2016-08-15 23:02:44 UTC
(In reply to Katrin Fischer from comment #129)
> Hi Mark,
> looking at https://wiki.koha-community.org/wiki/Hidden_values:
> -8 is described as "flag" - which noone knows exactly what it means I think.
> For "Everything hidden everywhere" I would have picked 8 or 5. 
> Can you explain your choice of -8?

Because even though it is <0 which is typically visible in OPAC, -8 is NOT visible in OPAC. There is logic which uses <0 in a lot of places and a -8 is bound to catch it if you are looking around and notice it isn't there.
Comment 131 M. Tompsett 2016-08-15 23:05:22 UTC
(In reply to Katrin Fischer from comment #129)
> Can you explain your choice of -8?

opac-MARCDetail.pl on the last patch.
Comment 132 Katrin Fischer 2016-08-19 09:00:23 UTC
Will continue with this in the next days.
Comment 133 Tomás Cohen Arazi 2016-08-30 16:00:24 UTC
Created attachment 55040 [details] [review]
Bug 11592: Updated License Text and use Modern::Perl

Why not clean up the License Agreement stuff while the files
are being changed? Used the current one found at:
http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence

Changed the strict and warning lines into just a Modern::Perl.

Signed-off-by: Robin Sheat <robin@catalyst.net.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 134 Tomás Cohen Arazi 2016-08-30 16:00:43 UTC
Created attachment 55041 [details] [review]
Bug 11592: Add should_hide_marc method to filter

Add should_hide_marc to ViewPolicy filter
Add should_hide_marc tests to t/db_dependent/Filter_MARC_ViewPolicy.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 135 Tomás Cohen Arazi 2016-08-30 16:00:59 UTC
Created attachment 55042 [details] [review]
Bug 11592: Applying filtering to opac interface.

Applying the filtering and then...
Debugging opac/opac-detail.pl filtering
Debugging opac/opac-ISBDdetail.pl more
Debugging opac/opac-export.pl
Tweak opac/opac-export.pl fix variable declarations, conditional assignments
Debugging opac/opac-showmarc.pl

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 136 Tomás Cohen Arazi 2016-08-30 16:01:21 UTC
Created attachment 55043 [details] [review]
Bug 11592: Biblio tweak for MARCSUBJECT

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 137 Tomás Cohen Arazi 2016-08-30 16:01:33 UTC
Created attachment 55044 [details] [review]
Bug 11592: MARCView and ISBD followup

There are still some leaks, but it is not as a result
of the filter, but rather a result of poorly written
template files.

Bug fixing template files is beyond the scope of this
set of patches.

TEST PLAN
---------
 1) Backup your DB
 2) run the following SQL on your DB.
    > UPDATE marc_subfield_structure set hidden=-8;
    -- this should set EVERYTHING to hidden across the board.
 3) In staff client, set OPACXSLTDetailsDisplay to blank
 4) In OPAC, view any detail.
    -- Normal view may mostly leak values still.
    -- MARC view may leak values.
    -- ISBD view may leak values.
 5) In staff client, set OPACXSLTDetailsDisplay to default
 6) In OPAC, view any detail.
    -- same issues as step 4
    -- 'View Plain' may leak too.
 7) 'Save record' -> 'Dublin Core'
 8) Apply this patch
 9) run koha qa test tools
    -- should be fine
10) prove -v t/db_dependent/Filter_MARC_ViewPolicy.t
    -- should pass
    -- this proves Koha/Filter/MARC/ViewPolicy.pm tweaks too
11) In OPAC, view any detail.
    -- Normal view:
       -- Material type comes from the LEADER field.
       -- Lists this is on will still display
       -- 'Tags from this library' will still display
       -- Item information in table will still display
          (THIS IS BEYOND SCOPE)
    -- MARC view:
       -- Record number is leaked
          (THIS IS BEYOND SCOPE)
       -- 'View plain' leaks LEADER field.
    -- ISBD view may leak field headings, but not values.
       (THIS IS BEYOND SCOPE)
12) In staff client, set OPACXSLTDetailsDisplay to blank
13) In OPAC, view any detail.
    -- same kind of output as step 10
14) 'Save record' -> BIBTEXT
    -- Should be next to nothing leaked.
15) 'Save record' -> Dublin Core
    -- Should be the same or less leaked between the two versions.
    -- (XML FILTERING IS BEYOND SCOPE)
16) In the staff client, go view the same record.
    -- it should be mostly hidden in ISBD View.
17) run the following SQL on your DB.
    > UPDATE marc_subfield_structure set hidden=1;
    -- this should set EVERYTHING to hidden in OPAC, but not
       the STAFF across the board.
18) Refresh the staff ISBD page
    -- values should reappear.
19) View the ISBD details in the OPAC
    -- values should still be hidden.
20) Check out the OPAC Cart and List
    -- while the intermediate pages may still leak
       the download links should leak very minimally.
    -- (CARTS AND LISTS ARE BEYOND SCOPE, THOUGH
        THE INTRANET ISBD AND SOME CART/LIST STUFF
        WERE FIXED BECAUSE OF THE GetISBDView REFACTOR)

Expectations:
Before Patch - all the OPAC Detail pages will display things
After Patch - all the OPAC Detail pages will display much less,
              and hopefully nothing (though there are known limits).
              the ISBD detail page in the Staff client will be
              filtered as well based on STAFF settings.
              The saving/exporting should generate nearly empty
              files.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 138 Tomás Cohen Arazi 2016-08-30 16:01:47 UTC Comment hidden (obsolete)
Comment 139 Tomás Cohen Arazi 2016-08-30 16:02:01 UTC Comment hidden (obsolete)
Comment 140 M. Tompsett 2016-08-30 17:48:08 UTC
Comment on attachment 55045 [details] [review]
Bug 11592: (QA followup) Simplify code

Review of attachment 55045 [details] [review]:
-----------------------------------------------------------------

Just an eyeball.

::: catalogue/ISBDdetail.pl
@@ -33,5 @@
>  
>  =cut
>  
> -use strict;
> -#use warnings; FIXME - Bug 2505

I wouldn't change to Modern::Perl, unless all the Bug 2505 references are fixed like this.

::: opac/opac-export.pl
@@ +35,5 @@
>  $biblionumber = int($biblionumber);
>  my $error = q{};
>  
> +my $include_items = ($format =~ /bibtex/) ? 0 : 1;
> +my $marc = GetMarcBiblio($biblionumber, $include_items)

Oooo... nice simplification.

::: opac/opac-showmarc.pl
@@ -58,4 @@
>  
>  if ($view eq 'card' || $view eq 'html') {
> -    # FIXME: GetXmlBiblio needs filtering later.
> -    my $xml = $importid ? $record->as_xml(): GetXmlBiblio($biblionumber);

GetXmlBiblio includes fields which aren't necessarily in GetMarcBiblio, if I recall correctly. I'm not sure about this.
Comment 141 M. Tompsett 2016-08-30 17:59:02 UTC
Comment on attachment 55046 [details] [review]
Bug 11592: (QA followup) Add missing framework code to ViewPolicy filter calls

Review of attachment 55046 [details] [review]:
-----------------------------------------------------------------

Just a quick eyeball.

::: opac/opac-MARCdetail.pl
@@ +73,5 @@
> +if ( ! $record ) {
> +    print $query->redirect("/cgi-bin/koha/errors/404.pl");
> +    exit;
> +}
> +

Okay. More logical location.
Comment 142 Tomás Cohen Arazi 2016-08-30 18:19:42 UTC
(In reply to M. Tompsett from comment #140)
> Comment on attachment 55045 [details] [review] [review]
> Bug 11592: (QA followup) Simplify code
> 
> Review of attachment 55045 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Just an eyeball.
> 
> ::: catalogue/ISBDdetail.pl
> @@ -33,5 @@
> >  
> >  =cut
> >  
> > -use strict;
> > -#use warnings; FIXME - Bug 2505
> 
> I wouldn't change to Modern::Perl, unless all the Bug 2505 references are
> fixed like this.

I prefer to do it, and take responsability if it introduces a warning. Haven't seen one so far.

> ::: opac/opac-showmarc.pl
> @@ -58,4 @@
> >  
> >  if ($view eq 'card' || $view eq 'html') {
> > -    # FIXME: GetXmlBiblio needs filtering later.
> > -    my $xml = $importid ? $record->as_xml(): GetXmlBiblio($biblionumber);
> 
> GetXmlBiblio includes fields which aren't necessarily in GetMarcBiblio, if I
> recall correctly. I'm not sure about this.

The main difference is that GetMarcBiblio does some sanitization. I think it is safe. This is probably some reminder from the days when everything was done against biblioitems.marc instead of biblioitems.marcxml.
Comment 143 Katrin Fischer 2016-09-05 06:24:56 UTC
Mark, are you going to sign off on Tomas' follow-ups so this can move on?
Comment 144 M. Tompsett 2016-09-05 13:53:24 UTC
(In reply to Katrin Fischer from comment #143)
> Mark, are you going to sign off on Tomas' follow-ups so this can move on?

Thanks for the reminder. So many distractions.
Comment 145 M. Tompsett 2016-09-05 15:38:48 UTC
Created attachment 55184 [details] [review]
Bug 11592: (QA followup) Simplify code

Koha::RecordProcessor and the defined filters are supposed to bring us
joy and happiness. Let's keep the code compact, simple and clean.

This patch removes record cloning all over the place.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 146 M. Tompsett 2016-09-05 15:38:57 UTC
Created attachment 55185 [details] [review]
Bug 11592: (QA followup) Add missing framework code to ViewPolicy filter calls

This patch adds the frameworkcode option param, using each record's frameworkcode
as expected by the filter. Otherwise the ViewPolicy filter falls back to the
default framework.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 147 Kyle M Hall 2016-09-08 12:56:20 UTC
Pushed to master for 16.11, thanks Mark, Tomas!
Comment 148 Oleg Vasilenko 2017-01-23 09:38:35 UTC
In UNIMARC XSLT trasformation on page opac-detail.pl fails with software error

Unsupported UNIMARC character encoding [] for XML output for UNIMARC; 100$a ->  at /usr/share/perl5/MARC/File/XML.pm line 568.

Error occurs if subfield 100$a is made invisible in OPAC. 
Yet there is no problem with MARC-view and ISBD view

Should this be a new bug?
Comment 149 Jonathan Druart 2017-01-23 10:02:11 UTC
(In reply to Oleg Vasilenko from comment #148)
> Should this be a new bug?

Hi Oleg,
This bug report is marked as pushed to master and the error you got does not seem related. So yes, please open a new bug report or email the list if you have a specific problem.
Your problem is usually caused by the fact to have the "marcflavour" system preference set to UNIMARC but you have MARC21 records in your DB.
Comment 150 Oleg Vasilenko 2017-01-23 10:50:18 UTC
Hi Jonathan,
This the problem with UNIMARC 100$a is connected with this patch, because this problem started after this bug 11592 was pushed to master. Removing this patch solved problem until I found the reason why it happens.

It appears, that after filtering (rows 92-100), record is passed to XSLT processing (rows 152-163), and it already lacks field 100$a (it was filtered before). SetUTF8Flag (row 147) does not add filtered 100$a.

So right now turning 100$a OPAC visibility off leads to error when user accesses opac-detail.pl. There is no problems with opac-MARCdetail.pl and opac-ISBDdetail.pl — subfield is hidden correctly.

So should I create a new bug?

(In reply to Jonathan Druart from comment #149)
> (In reply to Oleg Vasilenko from comment #148)
> > Should this be a new bug?
> 
> Hi Oleg,
> This bug report is marked as pushed to master and the error you got does not
> seem related. So yes, please open a new bug report or email the list if you
> have a specific problem.
> Your problem is usually caused by the fact to have the "marcflavour" system
> preference set to UNIMARC but you have MARC21 records in your DB.
Comment 151 Jonathan Druart 2017-01-23 11:08:36 UTC
(In reply to Oleg Vasilenko from comment #150)
> So should I create a new bug?

Yes please, and link it ("Depends on") to this one.
Comment 152 M. Tompsett 2017-01-24 00:53:18 UTC
(In reply to Oleg Vasilenko from comment #148)
> Should this be a new bug?

Yes, but I think it may be covered by 17527, which is currently in discussion, because people didn't test and go, "Oh, strangeness when we do something bad like everything set to hidden=-8 is perfectly okay." I'm going to tweak it to Needs Sign off, so we can move forward on this.