Bug 17527

Summary: Hidden settings in MARC framework don't work correctly on results list
Product: Koha Reporter: Elaine Bradtke <eb>
Component: MARC Bibliographic data supportAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED DUPLICATE QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: eb, fridolin.somers, irma, jonathan.druart, katrin.fischer, m.de.rooy, mtompset, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 11592    
Bug Blocks:    
Attachments: Bug 17527: Hide hidden fields for XSLT blocks
Bug 17527: Avoid double filtering and floody log clean up

Description Elaine Bradtke 2016-10-31 22:25:07 UTC
245 $h will not be hidden.
​Even when this subfield is set to "ignore", it still shows up in the staff interface and the OPAC.  However, the 245$h does not appear in the MARC view.
Just checked and setting other subfields in other fields to "ignore" seems to work as expected. I haven't tested them all, but it consistently doesn't work in 245 
This applies to 3.22 and 16.05​ (MARC 21)
Comment 1 Jonathan Druart 2016-11-01 10:26:12 UTC
Created attachment 57014 [details] [review]
Bug 17527: Hide hidden fields for XSLT blocks
Comment 2 Jonathan Druart 2016-11-01 10:27:40 UTC
Here is a patch for discussion.
Tomas, could you confirm this is the way to go? Why don't we have a lower level subroutine/method to retrieve a filtered record?
Comment 3 Jonathan Druart 2016-11-01 10:28:08 UTC
(In reply to Elaine Bradtke from comment #0)
> This applies to 3.22 and 16.05​ (MARC 21)

Do you mean it worked before 3.22?
Comment 4 Katrin Fischer 2016-11-01 13:42:01 UTC
To my knowledge it never worked for the XSLT/normal views, the hidden values only take effect on the MARC views. There is a patch from Mark that might add this feature in 16.11:
Bug 15870 - Add Filter for MARC to respect visibility settings
Comment 5 Katrin Fischer 2016-11-01 13:42:37 UTC
Ah sorry, 16.05!
Comment 6 Katrin Fischer 2016-11-01 13:52:13 UTC
Hm got curious about this one, that's what happens for me:

If I uncheck the visibility for 245$h for the OPAC it works for me immediately on the OPAC detail view, but not in the result list. It's still visible there after reindexing too.
Comment 7 Mark Tompsett 2016-11-01 16:03:54 UTC
Comment on attachment 57014 [details] [review]
Bug 17527: Hide hidden fields for XSLT blocks

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

::: C4/Search.pm
@@ +2222,4 @@
>          # XSLT processing of some stuff
>          # we fetched the sysprefs already before the loop through all retrieved record!
>          if (!$scan && $xslfile) {
> +            $oldbiblio->{XSLTResultsRecord} = XSLTParse4Display($oldbiblio->{biblionumber}, $marcrecord, $xslsyspref, 1, \@hiddenitems, $sysxml, $xslfile, $lang, $interface);

It's the simplest change, but I really dislike that many parameters.

::: C4/XSLT.pm
@@ +255,1 @@
>      my $itemsxml  = buildKohaItemsNamespace($biblionumber, $hidden_items);

Technically, 952 visibility could be checked/unchecked accordingly, so there might be something necessary here.

::: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
@@ +347,4 @@
>    [% IF ( unknownbiblionumber ) %]
>      Unknown record
>    [% ELSE %]
> +/    Details for <i>[% title |html %]  [% FOREACH subtitl IN subtitle %] [% subtitl.subfield | html %][% END %]</i>

What reason is there for adding this slash?
Comment 8 Mark Tompsett 2016-11-01 16:07:02 UTC
(In reply to Katrin Fischer from comment #4)
> To my knowledge it never worked for the XSLT/normal views, the hidden values
> only take effect on the MARC views.

That is my understanding too. And it looks like Jonathan has adapted the filter for use against the XSLT views. I'm thinking this patch might be the way forward for XSLT currently. Though, see my comment about items in comment #7.
Comment 9 Mark Tompsett 2016-11-01 16:09:21 UTC
Also, templates. The template files are using parameters passed directly which may have grabbed the unfiltered values. I'd like to see a refactor of templates, but that is not necessary to solve this problem.
Comment 10 Jonathan Druart 2016-11-01 16:31:17 UTC
(In reply to M. Tompsett from comment #7)
> Comment on attachment 57014 [details] [review] [review]
> Bug 17527: Hide hidden fields for XSLT blocks
> 
> Review of attachment 57014 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Search.pm
> @@ +2222,4 @@
> >          # XSLT processing of some stuff
> >          # we fetched the sysprefs already before the loop through all retrieved record!
> >          if (!$scan && $xslfile) {
> > +            $oldbiblio->{XSLTResultsRecord} = XSLTParse4Display($oldbiblio->{biblionumber}, $marcrecord, $xslsyspref, 1, \@hiddenitems, $sysxml, $xslfile, $lang, $interface);
> 
> It's the simplest change, but I really dislike that many parameters.

Me too, that's why I spend half of my time refactoring legacy code.

> ::: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
> @@ +347,4 @@
> >    [% IF ( unknownbiblionumber ) %]
> >      Unknown record
> >    [% ELSE %]
> > +/    Details for <i>[% title |html %]  [% FOREACH subtitl IN subtitle %] [% subtitl.subfield | html %][% END %]</i>
> 
> What reason is there for adding this slash?

Just a typo maybe?
Comment 11 Elaine Bradtke 2016-11-03 22:58:45 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Elaine Bradtke from comment #0)
> > This applies to 3.22 and 16.05​ (MARC 21)
> 
> Do you mean it worked before 3.22?

I don't know if it worked before 3.22. I have two different systems, one running 3.22 and the other running 16.05 and it doesn't work in either of them.
Comment 12 Mark Tompsett 2016-11-03 23:16:09 UTC
(In reply to Elaine Bradtke from comment #11)
> (In reply to Jonathan Druart from comment #3)
> > (In reply to Elaine Bradtke from comment #0)
> > > This applies to 3.22 and 16.05​ (MARC 21)
> > 
> > Do you mean it worked before 3.22?
> 
> I don't know if it worked before 3.22. I have two different systems, one
> running 3.22 and the other running 16.05 and it doesn't work in either of
> them.

Proper application of the MARC framework visibility settings has been broken for quite a while.
Comment 13 Mark Tompsett 2016-11-03 23:18:21 UTC
If there is no objection, I would like to test this for the reported problem 245$h and sign off. Granted, there will be further visibility issues as raised by comment #7, with respect to item information, but this should solve a large group of visibility issues with XSLT.
Comment 14 Tomás Cohen Arazi 2016-11-04 12:54:15 UTC
(In reply to M. Tompsett from comment #13)
> If there is no objection, I would like to test this for the reported problem
> 245$h and sign off. Granted, there will be further visibility issues as
> raised by comment #7, with respect to item information, but this should
> solve a large group of visibility issues with XSLT.

Go for it, but check for possible double-filtering on the controller scripts.
Comment 15 Mark Tompsett 2016-11-04 15:11:36 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> Go for it, but check for possible double-filtering on the controller scripts.

Because of the templates, I am guessing there may be. And yep, double filtering. Why? Because of the way the template works. An ugly if can fix that up... *cringe* Patch incoming.
Comment 16 Tomás Cohen Arazi 2016-11-04 15:36:15 UTC
(In reply to M. Tompsett from comment #15)
> (In reply to Tomás Cohen Arazi from comment #14)
> > Go for it, but check for possible double-filtering on the controller scripts.
> 
> Because of the templates, I am guessing there may be. And yep, double
> filtering. Why? Because of the way the template works. An ugly if can fix
> that up... *cringe* Patch incoming.

no, I mean because the records get filtered in the controller scripts by calling Koha::RecordProcessor, and then filtered again for XSLTParse4Display
Comment 17 Mark Tompsett 2016-11-04 15:54:26 UTC
Created attachment 57217 [details] [review]
Bug 17527: Avoid double filtering and floody log clean up

Because the filtering filters parameter values, C4/Output was
triggering floody output.

During testing, everything was set to hidden=-8. While this is
not realistic, it did uncover some floodiness which may have
already existed with the non-XSLT output.

OPAC did have a double filtering call, because of the poor design
of the template. An if condition around the processor call
prevents that, while still allowing the template to work.

TEST PLAN
---------
0) Back up your DB
1) in marc_subfield_structure, set all hidden=-8
   -- this makes everything hidden everywhere.
   -- it is completely arbitrary, any value which hides in
      OPAC would suffice for this patch.
2) in the staff client, make sure the OPACXSLTDetailsDisplay
   system preference is 'default' or some custome template
   -- default was used for testing.
3) make sure at least one book is entered in the system
   -- my testing assumes 1
   -- testing data set only had book with 'carbs' in title.
4) search for 'carbs' in the OPAC
   -- because I only had 1 match it redirected to results.
   -- material type 'book' comes from LDR, which is not
      filtered.
   -- the items section is built and not part of the filter.
   -- should be painfully sparse
5) select the three views
   -- all should be painfully sparse
6) select 'view plain' in the MARC view tab.
   -- painfully sparse
7) run koha qa test tools

NOTE: This patch only needs testing in OPAC.
Comment 18 Mark Tompsett 2016-11-04 18:07:34 UTC
The problem with XSLT is the processing a record vs. displaying it. The staff client does some strange things with this when you attempt to add a biblio from Z39.50 and then an item when all the frameworks settings have hidden=-8. Strangely, the data makes it in. I would have preferred that one could not even add anything.
Comment 19 Mark Tompsett 2016-11-04 18:08:09 UTC
In short, it works as expected, but there are some strange side effects.
Comment 20 Katrin Fischer 2016-11-06 10:42:52 UTC
(In reply to M. Tompsett from comment #19)
> In short, it works as expected, but there are some strange side effects.

What are the strange side effects?
Comment 21 Mark Tompsett 2016-11-07 11:50:24 UTC
(In reply to Katrin Fischer from comment #20)
> What are the strange side effects?

See comment #18 -- describes steps into strangeness
"The staff client does some strange things with this when you attempt to add a biblio from Z39.50 and then an item when all the frameworks settings have hidden=-8."
Comment 22 Mark Tompsett 2017-01-24 00:54:20 UTC
Changed to needs sign off, because I'm pretty sure it solved the issues mentioned on bug 11592 after it was pushed to master. And 'In Discussion' is a wasteland.
Comment 23 Marcel de Rooy 2017-04-03 13:23:08 UTC
(In reply to M. Tompsett from comment #22)
> And 'In Discussion' is a wasteland.

Sometimes used as an euphemism (nice alias) for Failed QA ?
Comment 24 Mark Tompsett 2017-04-08 14:20:29 UTC
(In reply to Marcel de Rooy from comment #23)
> (In reply to M. Tompsett from comment #22)
> > And 'In Discussion' is a wasteland.
> 
> Sometimes used as an euphemism (nice alias) for Failed QA ?

Sometimes, yes. Which explains what the Failed QA (255) and In Discussion (219) queues are roughly the same.
Comment 25 Katrin Fischer 2017-08-18 13:16:51 UTC
Hi, I tested this, but can't get it to work on the results lists:

- Applied patches
- Removed accidental /

::: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
@@ +347,4 @@
>    [% IF ( unknownbiblionumber ) %]
>      Unknown record
>    [% ELSE %]
> +/    Details for <i>[% title |html %]  [% FOREACH subtitl IN subtitle %] [% subtitl.subfield | html %][% END %]</i>

- restart_all in kohadevbox
- Set 245$b and 245$p to 'hidden in OPAC' in the frameworks
- Checked the result list - subfields still display
- Checked the detail page - subfields are hidden (same without patches)
Comment 26 Mark Tompsett 2017-08-18 22:40:39 UTC
(In reply to Katrin Fischer from comment #25)
> - restart_all in kohadevbox
> - Set 245$b and 245$p to 'hidden in OPAC' in the frameworks
> - Checked the result list - subfields still display
> - Checked the detail page - subfields are hidden (same without patches)

There are lots of places in OPAC related code where it grabs the subtitle, etc. directly from a bibliorecord, rather than the filtered MARC record.

BTW, this *might* improve with bugs 19040 and 14385. I make no promises.
Comment 27 Katrin Fischer 2017-08-19 06:00:21 UTC
> There are lots of places in OPAC related code where it grabs the subtitle,
> etc. directly from a bibliorecord, rather than the filtered MARC record.
> 
> BTW, this *might* improve with bugs 19040 and 14385. I make no promises.

I don't think this applies to the XSLT built result list.
Comment 28 Katrin Fischer 2018-01-02 07:14:42 UTC
Marc, could you take another look?
Comment 29 Fridolin Somers 2018-07-23 10:08:33 UTC
This bug is quite important I think.

In my opinion, it should focus on XSLT generated HTML, in order to continue integration.

The case of subtitle :
  [% FOREACH subtitl IN subtitle %] [% subtitl.subfield | html %][% END %]
It should be another bug.
Or we consider that setting a subtitle means it is to show it at intranet and OPAC.
Its like trying to hide author ;)
Comment 30 Tomás Cohen Arazi 2020-05-11 12:11:07 UTC
I'm taking over this one.
Comment 31 Tomás Cohen Arazi 2020-05-11 12:12:36 UTC
Starting fresh on bug 24458, as this one covered things that are already fixed in master.

*** This bug has been marked as a duplicate of bug 24458 ***