Bug 6488

Summary: opachiddenitems not working in master
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: System AdministrationAssignee: Srdjan Jankovic <srdjan>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P2 CC: chris, fridolin.somers, gmcharlt, jcamins, katrin.fischer, koha.sekjal, lculber, m.de.rooy, paul.poulain, wizzyrea
Version: 3.6   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7690
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=3194
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: patch
detail display is hidden
not hidden on search results
patch
bug_6488: Changed searchResults() interface Take in account opachiddenitems when searching in opac Added trailing \n when parsing OpacHiddenItems to make YAML happy
harry potter marc record
patch
patch
[SIGNED-OFF] bug_6488: Take in account opachiddenitems when searching in opac
patch
[SIGNED-OFF] bug_6488: Take in account opachiddenitems when searching in opac
Bug 6488 - followup - fixes issue with bibs with no items and serials being suppressed
Bug 6488 - followup - fixes issue with bibs with no items and serials being suppressed
Picture of staff results
follow-up patch
[SIGNED_OFF] Followup patch restoring search results numbering

Description Nicole C. Engard 2011-06-09 15:44:18 UTC
Per a discussion on and off the mailing list the opachiddenitems system preference worked in 3.4.1 (and 3.4) but does not appear to be working in master.  I have not been able to test in both versions personally, so this is based on tests by myself and two others in different versions of Koha.

I'm kind of hoping that it's a template issue that came about by fixing one of the TT bugs - but in case it's not I'm putting this in the admin category.
Comment 1 Srdjan Jankovic 2011-09-06 02:12:06 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2011-09-06 13:36:57 UTC
Okay, we're almost there. The holdings are hidden on the detail display, but they show up as available in the search results list.  See my coming attachments.
Comment 3 Nicole C. Engard 2011-09-06 13:37:47 UTC Comment hidden (obsolete)
Comment 4 Nicole C. Engard 2011-09-06 13:38:11 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2011-09-06 23:35:13 UTC
I have no problem placing the filter on the search screen as well, but:
* it was not there to start with
* maybe there's a reason to be able to see the records even without being able to reach the items

Nicole, can you please confirm that opachiddenitems should be applied on the search screen as well
Comment 6 Nicole C. Engard 2011-09-07 16:04:11 UTC
It seems that we're all in agreement that it should be hidden all over if it's marked hidden:


[12:01]  <nengard> bug 6488 can anyone think of a reason that the hidden items should show on the search results and not the bib detail?
[12:01]  <huginn> Bug http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6488 normal, PATCH-Sent, ---, srdjan, ASSIGNED , opachiddenitems not working in master
[12:01]  <nengard> i don't want to make a decision alone
[12:01]  <nengard> i think if it's hidden it should be hidden
[12:01]  <wizzyrea> ^^
[12:01]  <wizzyrea> what's the point of hiding it if it's not hidden
[12:01]  <cait> wizzyrea: here now, but have to be careful to not burn my dinner
[12:01]  <oleonard> Agreed.
[12:01]  <wizzyrea> ooo no, dinner is important
[12:02]  <slef> I think it should be hidden on all OPAC searches, not on intranet
[12:02]  <wizzyrea> I can ask you after you eat :)
[12:02]  <sekjal> yes, if it's hidden in the OPAC, that should be on search results, details, Lists, etc.
[12:02]  <sekjal> consistency
[12:02]  <wizzyrea> make it so, nengard ;)
[12:02]  <cait> nengard: I agree - should be hidden everywhere or it will only be confusing :)
Comment 7 Srdjan Jankovic 2011-09-08 06:28:44 UTC Comment hidden (obsolete)
Comment 8 Srdjan Jankovic 2011-09-08 06:30:30 UTC
Also, since I've changed searchResults() which is used on both interfaces, please check the intranet app as well
Comment 9 Liz Rea 2011-09-20 15:19:45 UTC Comment hidden (obsolete)
Comment 10 Nicole C. Engard 2011-09-26 14:58:33 UTC
I'm thinking this still isn't working quite right - but I could be wrong:

http://screencast.com/t/NN73y6yErzie

Is this what we expect to see?
Comment 11 Srdjan Jankovic 2011-10-06 04:01:22 UTC
Is it possible that cd is marked as lost?
Comment 12 Nicole C. Engard 2011-10-06 12:52:04 UTC
Nope ... doesn't look that way.
Comment 13 Srdjan Jankovic 2011-10-07 05:02:00 UTC
Those two screens are going off different things, (MARC fields vs database). I cannot get it to misbihave in that manner in my environment. Is it possible to get a database dump, please? I'm very keen to sort this out, I suspect some other problems there.

Also Nicole, in that screencast, you said that you wanted to hide books from a branch. The way it works now is that it will hide all items that are books as well as all items that are homed in that branch. Can you please comment.
Comment 14 Nicole C. Engard 2011-10-07 12:17:30 UTC
Srdjan,

Because my BK item in that video was currently at the NIC branch but not normally owned by that branch it should show in the holdings in the OPAC. The second item on that record was CD not a BK but was also being hidden for some reason.  Basically, neither of those two items should have been hidden because they didn't meet the criteria - unless the criteria is being ORed instead of ANDed.  Does that make sense?  

Nicole
Comment 15 Nicole C. Engard 2011-10-07 12:18:09 UTC
Created attachment 5767 [details]
harry potter marc record

this is the marc record i was using in the video - or did you need my entire database dumped?
Comment 16 Srdjan Jankovic 2011-10-10 04:10:29 UTC
Entire database please, some config option may be important.
Comment 17 Srdjan Jankovic 2011-10-10 04:13:35 UTC
Yes, criteria for hiding are ORed, not ANDed. If you think it is a bug, we need to go differently about it.
Comment 18 Nicole C. Engard 2011-10-10 14:26:12 UTC
Okay if it's ORed then we need to ask the community what they want/expect.  I would think that it should be ANDed - but that's just me.

Community we need feedback.

Nicole
Comment 19 Nicole C. Engard 2011-10-10 14:34:33 UTC
I'm not sure how to give you a full DB dump since it's pretty darn large and won't attach here.
Comment 20 lculber 2011-10-10 15:03:43 UTC
I think that we want it to be "OR"  because in our case we have OpacHiddenItems set to
itype: [07, 10]
location: [STAFF, ISO]

meaning that if itype ='07' OR  itype='10' OR location='STAFF' OR location='ISO'  then we want it hidden
Comment 21 Nicole C. Engard 2011-10-11 11:56:44 UTC
I have tested again with the one patch on here - are there supposed to be two?

Anyway, there are all kinds of things going wrong and I doubt it's a data issue. Here's a new video: http://screencast.com/t/06N5phPIarm
Comment 22 Srdjan Jankovic 2011-11-15 07:07:36 UTC Comment hidden (obsolete)
Comment 23 Srdjan Jankovic 2011-12-09 04:14:25 UTC Comment hidden (obsolete)
Comment 24 Srdjan Jankovic 2011-12-09 04:15:48 UTC
Please retest. Again, if changing item records zebra needs reindexing
Comment 25 Nicole C. Engard 2011-12-14 14:58:54 UTC Comment hidden (obsolete)
Comment 26 Paul Poulain 2012-01-03 21:42:27 UTC
QA comment:
 1 this patch adds a lot of reindentation, he's quite hard to read. But those indentations are needed, so it's OK
 2 prove t/00-perlcritic.t is OK
 3 prove xt/author/valid-templates.t is OK
 4 the patch works if XSLT is ON. With XSLT OFF there is no difference before and after applying the patch.
 5 about the removed numbering, which one is it. On my setup & test I still see a numbering after each item, for example:
Availability: Copies available for loan: Médiathèque Cannet des Maures[800] (43). Copies available for reference: Médiathèque Cannet des Maures[800] (1). Checked out (2). 

Marking failed QA until I've some feedback for points 4 and 5
Comment 27 Srdjan Jankovic 2012-01-06 00:29:54 UTC
4. I tried both XSLT and "normal", and got the same result. Can we ask someone else to check this?

5. The number that was removed is one that was next to the Save tickbox, the ordinal number of the result in the result set. It was removed because it was awkward to show eg 6 results numbered 3,5,8,13,16,19
Comment 28 Paul Poulain 2012-01-09 19:36:45 UTC
I switch back to "signed-off". I'll try to send a screenshot.
Could it be a MARC21 / UNIMARC difference ? it's highly possible !
Comment 29 Ian Walls 2012-01-20 17:36:36 UTC
Patch no longer applies to master.  Please rebase (looks to be pretty straightforward, just the one change to cataloguing/addbooks.pl)
Comment 30 Srdjan Jankovic 2012-01-23 04:07:47 UTC Comment hidden (obsolete)
Comment 31 Nicole C. Engard 2012-01-30 14:50:09 UTC Comment hidden (obsolete)
Comment 32 Paul Poulain 2012-02-01 15:31:36 UTC
Testing this patch again, to see if my previous question:
" 4 the patch works if XSLT is ON. With XSLT OFF there is no difference before
and after applying the patch."
still applies.
It still applies, but I think/feel it's related to my test database.
I think it's not related to UNIMARC, but to the fact some of my items don't have any callnumber at all. In this case, the counter is not exact, but I don't think it's related to this patch.

QAind the code now
Comment 33 Paul Poulain 2012-02-01 16:01:51 UTC
QA comment:
* changes the signature of searchResults by adding a new parameter at the end and changing the type of variable for the 7th ($ instead of @) All instances of the use of this sub have been updated
* in addbooks.pl, searchResults is called without opachiddenitems, that's OK, as it's the last argument. i've updated coding guidelines (PERL8 rule)

passed QA
Comment 34 Marcel de Rooy 2012-02-02 13:24:39 UTC
Warning like below apparently come from the latest commit on Search.pm:

[Thu Feb 02 13:33:42 2012] [error] [client 192.87.126.61] [Thu Feb  2 13:33:42 2012] opac-search.pl: Unable to parse OpacHiddenItems syspref : YAML Error: Stream does not end with newline character, referer: http://libdevelop.rijksmuseum.nl:8008/
[Thu Feb 02 13:33:42 2012] [error] [client 192.87.126.61] [Thu Feb  2 13:33:42 2012] opac-search.pl:    Code: YAML_PARSE_ERR_NO_FINAL_NEWLINE, referer: http://libdevelop.rijksmuseum.nl:8008/
[Thu Feb 02 13:33:42 2012] [error] [client 192.87.126.61] [Thu Feb  2 13:33:42 2012] opac-search.pl:    Line: 0, referer: http://libdevelop.rijksmuseum.nl:8008/
[Thu Feb 02 13:33:42 2012] [error] [client 192.87.126.61] [Thu Feb  2 13:33:42 2012] opac-search.pl:    Document: 0, referer: http://libdevelop.rijksmuseum.nl:8008/
[Thu Feb 02 13:33:42 2012] [error] [client 192.87.126.61] [Thu Feb  2 13:33:42 2012] opac-search.pl:  at /usr/local/lib/perl5/site_perl/5.10.0/YAML.pm line 36, referer: http://libdevelop.rijksmuseum.nl:8008/
[Thu Feb 02 13:33:42 2012] [error] [client 192.87.126.61] [Thu Feb  2 13:33:42 2012] opac-search.pl: Unable to parse OpacHiddenItems syspref : YAML Error: Stream does not end with newline character, referer: http://libdevelop.rijksmuseum.nl:8008/
Comment 35 Paul Poulain 2012-02-02 14:22:28 UTC
(In reply to comment #34)
> Warning like below apparently come from the latest commit on Search.pm:
> 
> [Thu Feb 02 13:33:42 2012] [error] [client 192.87.126.61] [Thu Feb  2 13:33:42
> 2012] opac-search.pl: Unable to parse OpacHiddenItems syspref : YAML Error:
OK, I get the error, but only when the syspref is empty.

That's strange, because in the code there is:
+    $yaml = "$yaml\n"; # YAML is anal on ending \n. Surplus does not hurt

so yaml should never be empty...

Adding 2 \n solve the problem.

follow-up with a 2nd \n pushed, thanks to confirm the log is silenced now.
Comment 36 Marcel de Rooy 2012-02-02 14:28:48 UTC
Yep, works fine now! Thx.
Comment 37 Liz Rea 2012-02-17 18:07:56 UTC
it seems that adding this has broken viewing of bibs with no items in Master?

Also, a followup will be necessary that removes the documentation from the filesystem (or duplicates it in a more accessible place) and putting it somewhere more accessible to users. Asking people to consult a file on the filesystem will confuse librarians.
Comment 38 Liz Rea 2012-02-17 18:09:08 UTC
Additionally, search.pm reports that there are x results, and paginates them, but doesn't show them. It's a decent sized display weirdness.
Comment 39 Paul Poulain 2012-02-17 18:21:26 UTC
(In reply to comment #37)
> it seems that adding this has broken viewing of bibs with no items in Master?

Could you give more details about your configuration, because I don't see this kind of problem on mine
Comment 40 Liz Rea 2012-02-17 19:07:04 UTC
MARC21 db
nothing in the syspref or with itype: [BK] - no difference
independantbranches ON and OFF (same result either way)
Many bibs with no items - none can be found.

Default framework on all.

Any other relevant preferences you want me to check?
Comment 41 Liz Rea 2012-02-17 19:22:39 UTC
XSLT on or off - no change.
Comment 42 Chris Cormack 2012-02-17 19:24:30 UTC
Its these lines here

+        next if $is_opac       && $hideatopac_count >= $items_count;
+        next if $hidelostitems && $itemlost_count   >= $items_count;
+

That are the problem. Means that if its opac, and the counts are both 0 (ie record with no items) its skipped.

At least I think that is what the problem is, it is saturday morning and I havent had coffee yet so someone else looking would be good
Comment 43 Jared Camins-Esakov 2012-02-17 19:32:29 UTC
(In reply to comment #42)
> Its these lines here
> 
> +        next if $is_opac       && $hideatopac_count >= $items_count;
> +        next if $hidelostitems && $itemlost_count   >= $items_count;
> +
> 
> That are the problem. Means that if its opac, and the counts are both 0 (ie
> record with no items) its skipped.
> 
> At least I think that is what the problem is, it is saturday morning and I
> havent had coffee yet so someone else looking would be good

I can confirm that those two lines are the problem. What I'm not sure about is why there isn't an:
if ($items_count > 0)  { ... } around it.
Comment 44 Liz Rea 2012-02-17 19:53:44 UTC
confirm that adding the check for >0 items fixes the issue. Also discovered that the patch was nuking serials as well, as they don't have items.
Comment 45 Liz Rea 2012-02-17 20:10:23 UTC Comment hidden (obsolete)
Comment 46 Jared Camins-Esakov 2012-02-18 16:31:38 UTC Comment hidden (obsolete)
Comment 47 Jared Camins-Esakov 2012-02-18 16:32:55 UTC
Note that serial and ebook records will not show up in the OPAC without the follow up. Raising importance of bug to reflect seriousness of issue.
Comment 48 Marcel de Rooy 2012-02-20 12:51:05 UTC
QA Comment: Small fix for serious problem. Marked as Passed QA.
Comment 49 Paul Poulain 2012-02-20 15:28:21 UTC
follow-up pushed to master
Comment 50 Marcel de Rooy 2012-03-01 09:25:53 UTC Comment hidden (obsolete)
Comment 51 Marcel de Rooy 2012-03-01 09:26:22 UTC
Comment on attachment 7957 [details]
Picture of staff results

Wrong report. *Sorry*
Comment 52 Marcel de Rooy 2012-03-01 10:57:42 UTC
Also mailed the dev list:

I personally do not favor that adding an optional feature of hiding items removes search results numbering in general. The numbering is very handy. Now everybody loses that while not even using the hidden items feature!

I would suggest that possibly Srdjan (as the author) adds the search results numbering back and only removes it when opachiddenitems is active. This could be provisional while resolving the numbering issue in full.
Comment 53 Katrin Fischer 2012-03-01 13:08:09 UTC
I agree that making it dependent on opachiddenitems would be good. I also have the impression that people use the numbering for orientation.
Comment 54 Srdjan Jankovic 2012-03-02 00:06:26 UTC
Right, so can I assume that we all agree that I put the numbering back conditionally? And since this has been pushed to master, I take that we need a new
bug report.

BTW, hiding items is not the only way to make numbers skip, not showing non-available items is an example.

And I'm not the author, just a willing executor :)
Comment 55 Marcel de Rooy 2012-03-02 08:29:47 UTC
Great that you want to do do. Sorry for confusing you with the author, but I read your name on the commit..
You do no need a new report. Just set the status back here. If you have it there, I will be happy to sign off on it..
Comment 56 Srdjan Jankovic 2012-03-07 04:04:24 UTC Comment hidden (obsolete)
Comment 57 Marcel de Rooy 2012-03-07 12:39:44 UTC
Created attachment 8054 [details] [review]
[SIGNED_OFF] Followup patch restoring search results numbering
Comment 58 Marcel de Rooy 2012-03-07 12:42:03 UTC
QA Comment: Small patch restoring search results numbering when opachiddenitems is not in effect.
Thanks Srdjan for submitting this change on short notice.
Marking as Passed QA.