Summary: | Hidden settings in MARC framework don't work correctly on results list | ||
---|---|---|---|
Product: | Koha | Reporter: | Elaine Bradtke <eb> |
Component: | MARC Bibliographic data support | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
Status: | RESOLVED DUPLICATE | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | eb, fridolin.somers, irma, jonathan.druart, m.de.rooy, mtompset, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
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
Created attachment 57014 [details] [review] Bug 17527: Hide hidden fields for XSLT blocks 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? (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? 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 Ah sorry, 16.05! 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 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? (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. 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. (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? (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. (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. 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. (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. (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. (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 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. 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. In short, it works as expected, but there are some strange side effects. (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? (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." 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. (In reply to M. Tompsett from comment #22) > And 'In Discussion' is a wasteland. Sometimes used as an euphemism (nice alias) for Failed QA ? (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. 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)
(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.
> 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.
Marc, could you take another look? 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 ;) I'm taking over this one. |