Bug 8462

Summary: OpacHiddenItems and hidelostitems hide items, but query result count is off
Product: Koha Reporter: D Ruth Holloway <ruth>
Component: SearchingAssignee: Kyle M Hall <kyle>
Status: RESOLVED MOVED QA Contact: Testopia <testopia>
Severity: major    
Priority: P3 CC: chris, fridolin.somers, katrin.fischer, kyle, magnus, mathsabypro, mirko, mtj, mtompset, nengard, paola.rossi, paul.poulain
Version: master   
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=9778
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11909
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed patch
Bug 8462: OpacHiddenItems and hidelostitems hide items, but query result count is off
Proposed patch (rebased master)
[SIGNED-OFF] Bug 8462: OpacHiddenItems and hidelostitems hide items, but query result count is off
Bug 8462 - OpacHiddenItems and hidelostitems hide items, but query result count is off
Bug 8462 - OpacHiddenItems and hidelostitems hide items, but query result count is off
Bug 8462 - OpacHiddenItems and hidelostitems hide items, but query result count is off
Bug 8462 - OpacHiddenItems and hidelostitems hide items, but query result count is off

Description D Ruth Holloway 2012-07-17 18:29:09 UTC
Both OpacHiddenItems and hidelostitems work, and Dobrica's recent patch to remove the numbering if OpacHiddenItems is used is a swell idea (hidelostitems needs that, too!), but in either event, the hit count at the top of the result list is off.

This is not a problem if you have a screen full of results; no one is gonna count to see that you only have 19 items on that list.  But if the search result says 8 records were returned, and some lower number actually appear, they'll start asking questions.  The example that was shown to me showed 8 results, with only *one* showing.  Definitely question-begging!

If you wrapped the result count in a <div>, maybe, then you could use jquery to change that number in some way?
Comment 1 Fridolin Somers 2012-08-09 14:24:43 UTC
I confirm.
The Bug 6488 introduced to hide a record with only hidden items.
This concerns only records managed in result page (depends on offset and results per page).

But this causes several problems :
- search hints (total number of results) returned by Zebra seems false when there are hidden records. That's what reports D Ruth Bavousett.
- offset and results per page does not correspond to results displayed when there are hidden records. There can be only 2 results displayed in first page with a 20 results per page and clicking on next page will be offset=20.
- when there is only one result and this one is hidden, leads to a 404 error page. (Because test is based on zebra total hints).

Note that hiding records in OPAC can be managed with OpacSuppress syspref. In this case Zebra does not return hidden records, so everything is correct.

So can we keep this feature ?
I seems very difficult to correct.

I had the idea to perform hidden test during Zebra indexing to set OPAC suppress flag but this will cause a longer indexing (due to database queries).
Comment 2 Fridolin Somers 2012-09-06 12:15:01 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-09-07 13:23:43 UTC Comment hidden (obsolete)
Comment 4 Mason James 2012-09-14 07:07:16 UTC
(In reply to comment #3)
> Created attachment 12042 [details] [review]
> Bug 8462: OpacHiddenItems and hidelostitems hide items, but query result
> count is off
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

patch looks good, passing QA...

$ koha-qa.pl 

testing 1 commit(s) (applied to commit 60790d0)
 * 82c74b0 Bug 8462: OpacHiddenItems and hidelostitems hide items, but query
      C4/Search.pm

* C4/Search.pm                                                             OK
Comment 5 Paul Poulain 2012-09-14 14:03:22 UTC
i'm not sure i'm happy with this fix. It means you will get results without any items, right ?

I'm asking for another signoff or someone opinion by switching back to "need signoff"
Comment 6 Fridolin Somers 2012-09-14 15:37:35 UTC
(In reply to comment #5)
> i'm not sure i'm happy with this fix. It means you will get results without
> any items, right ?
Right.
I'm not against this feature.
It is just that the way it works by now is very tricky for OPAC users, like explained Ruth Bavousett ; and causes several bugs.
Comment 7 Nicole C. Engard 2012-09-28 13:13:58 UTC
I would like to sign off on this because this is confusing.  But the patch doesn't apply:

Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 8462: OpacHiddenItems and hidelostitems hide items, but query result count is off
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Search.pm
CONFLICT (content): Merge conflict in C4/Search.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8462: OpacHiddenItems and hidelostitems hide items, but query result count is off
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
nengard@kohavm:~/kohaclone$
Comment 8 Fridolin Somers 2012-09-28 13:35:53 UTC Comment hidden (obsolete)
Comment 9 Nicole C. Engard 2012-09-28 13:59:30 UTC Comment hidden (obsolete)
Comment 10 Nicole C. Engard 2012-09-28 13:59:59 UTC
I think this works to avoid confusion for now and we should come up with a way to hide bib records later on.
Comment 11 Mason James 2012-10-03 00:50:56 UTC
(In reply to comment #10)
> I think this works to avoid confusion for now and we should come up with a
> way to hide bib records later on.

hmm, its a tricky one - do we want...

1/ hidden-bibs, but incorrect search-results count, or...
2/ hidden-bibs feature removed, and correct search-results count

i think some people will be unhappy about this change, either way
but now 2 sign-offs, so passing QA...

$ koha-qa.pl -c 1

testing 1 commit(s) (applied to commit 0a35b26)
 * b864e85 Bug 8462: OpacHiddenItems and hidelostitems hide items, but query result count is off
      C4/Search.pm

* C4/Search.pm                                                             OK
Comment 12 Katrin Fischer 2012-10-04 07:16:01 UTC
I think hiding the record with the last item being hidden has been in Koha for a while now. While it's not good that the result count is off, I don't think we can just revert this behaviour that has been advertised and that library have been using in their collections. More so as the records being hidden now will just quietly reappear after the update (if I understand the change made here correctly). I think we need a way to control this - yes, maybe a new system preference.
Comment 13 Chris Cormack 2012-10-04 07:42:20 UTC
I agree, this suddenly changes behaviour with utterly no warning and suddenly biblio records that used to be hidden are now visible.
Comment 14 Magnus Enger 2012-10-04 07:43:12 UTC
(In reply to comment #12)
>  I don't
> think we can just revert this behaviour that has been advertised and that
> library have been using in their collections. 

+1
Comment 15 Mirko Tietgen 2012-10-04 07:48:30 UTC
I agree with Katrin, I am opposed to taking away functionality that libraries are using. I would be less than happy to have that suddenly stop working if I relied on it.
Comment 16 Fridolin Somers 2012-10-04 08:55:18 UTC
But what about the several bugs and strange behaviors due to this feature ?
Comment 17 Chris Cormack 2012-10-04 09:04:13 UTC
(In reply to comment #16)
> But what about the several bugs and strange behaviors due to this feature ?

We can't fix bugs by introducing other strange behaviors (biblios suddenly becoming unhidden). This removes a feature that is 7 months old without any warning, that libraries are using.

If you want this in, I would suggest adding a systempreference to turn the hiding biblio feature off.
Comment 18 Fridolin Somers 2012-10-04 09:41:40 UTC
(In reply to comment #17)
> If you want this in, I would suggest adding a systempreference to turn the
> hiding biblio feature off.
I agree, a syspref is a good idea.
Maybe 'OpacHideRecordHiddenItems'.
A suggestion on the name ?
Comment 19 Katrin Fischer 2012-10-04 10:05:40 UTC
Hm, maybe it can be grouped together for display with the OpacHidenItems pref. So that people find the new option easily.

OpacHiddenItemsRecord maybe?
Comment 20 Fridolin Somers 2012-10-04 10:35:41 UTC
(In reply to comment #19)
> Hm, maybe it can be grouped together for display with the OpacHidenItems
> pref. So that people find the new option easily.
> 
> OpacHiddenItemsRecord maybe?

+1
OpacHiddenItemsHidesRecord ?
Comment 21 Mason James 2012-10-09 06:00:37 UTC
(In reply to comment #1)
> I confirm.
> The Bug 6488 introduced to hide a record with only hidden items.
> This concerns only records managed in result page (depends on offset and
> results per page).
> 
> But this causes several problems :
> - search hints (total number of results) returned by Zebra seems false when
> there are hidden records. That's what reports D Ruth Bavousett.
> - offset and results per page does not correspond to results displayed when
> there are hidden records. There can be only 2 results displayed in first
> page with a 20 results per page and clicking on next page will be offset=20.
> - when there is only one result and this one is hidden, leads to a 404 error
> page. (Because test is based on zebra total hints).
> 
> Note that hiding records in OPAC can be managed with OpacSuppress syspref.
> In this case Zebra does not return hidden records, so everything is correct.
> 
> So can we keep this feature ?
> I seems very difficult to correct.
> 


i did a patch for this issue 1.5 years ago, but i never committed it

the method i used, was to fetch more records than needed from zebra
(like 40 bibs, not 20)

its obviously not a perfect solution, there is a chance that all 40 bibs fetched might be hidden, and you would still end up with 0 bibs displayed

^^despite that, in real life this patch worked well

the only other solution, afaik, is to keep doing zebra queries until one page of non-hidden bibs is accumulated

i'll upload my patch soon...
Comment 22 Mathieu Saby 2012-11-21 22:16:46 UTC
What about using zebra "Suppress" index to allow librarians to hide propely and automatically records when all items are hidden by OpacHiddenItems or Hidelostitems, without generating 404 error?

When all items are hidden by OpacHiddenItems or Hidelostitems, Koha will put a 
"1" in a field of the record mapped with Suppress index.
I see 2 possibilties :

1. We can use the field indexed in "Suppress" in the current record.abs of the library
Drawback : if one item is added again to this record, or if the library change the value of OpacHiddenItems (to show again all items), librarians will have to change manually the value of this field => not good for me.

2. We can use an other field, so that 2 fields in zebra will be index in "Suppress". For example, lets say 099$f and 099$k. 099$f will be used to change manually the visibiliy of record, 099$k to change it automatically if and only if all items are hidden by OpacHiddenItems or Hidelostitems. So a record could have 0-1, 1-1, 1-0, 0-0. I suppose we will need to store the value of this new field in a syspref (as it could be different in each library). 
Drawback : better, but need to rebuild zebra.

Your opinion?
(I had the begining of an other idea, with a new field in biblio table, but to complicated, and it is to fuzzy in my mind...)

M. Saby
Rennes 2 University
Comment 23 Fridolin Somers 2012-11-22 14:28:15 UTC
(In reply to comment #22)
+1 for solution 2.

I think indeed the best solution is to use Suppress index.
The automatic suppress field could be updated in rebuild_zebra.pl.
An idea for syspref : OpacAutoSuppressField.
Comment 24 Katrin Fischer 2012-11-22 14:33:10 UTC
Hi,
I am not sure I understand how the suppress index would work here, so I have a question. :)

We regularly import records from our union catalog. It's an automatic process, so everytime a record is updated in the union catalog by any participating library the record will be overlayed in Koha during the next night. If a library chooses to suppress records by certain criteria (opachiddenitems) - would that be overwritten too or can it be preserved by setting the flag automatically pre-indexing?
Comment 25 Fridolin Somers 2012-11-23 10:29:31 UTC
> would that be overwritten too or can it be preserved by setting the flag automatically pre-indexing?

I'd say yes.
I propose to set suppress info in record during indexing (rebuild_zebra.pl already sets some infos in record before exporting to indexed files), so the record in database will not have the information, it can change like you want.
Comment 26 Fridolin Somers 2013-03-07 16:47:32 UTC
(In reply to comment #23)
> (In reply to comment #22)
> +1 for solution 2.
> 
> I think indeed the best solution is to use Suppress index.
> The automatic suppress field could be updated in rebuild_zebra.pl.
> An idea for syspref : OpacAutoSuppressField.

A better solution is to add a new column to biblio or bilioitems table, mapped with opacsuppress subfield.
This can be used to forbid access to a biblio via URL direct edition.
Comment 27 Mathieu Saby 2013-03-10 10:26:28 UTC
I am considering improvements for managing opac visibility (bz9778), but I will probably wait if somebody wants to map opacsuppress with biblio or biblioitems.

M. Saby
Rennes 2 university
Comment 28 Kyle M Hall 2014-04-11 17:23:05 UTC
This patch has been in discussion for over a year now with no movement. I'm going to QA this and let the RM make the final decision. It looks like nobody is interested in developing an alternative solutions.
Comment 29 Kyle M Hall 2014-04-11 17:43:51 UTC
Well that didn't work out ; )
Patch no longer solves the problem.
Comment 30 Kyle M Hall 2014-04-11 17:47:35 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2014-04-11 17:48:44 UTC Comment hidden (obsolete)
Comment 32 Paola Rossi 2014-05-12 14:29:18 UTC
I test against master 3.15.00.051

Applying: Bug 8462 - OpacHiddenItems and hidelostitems hide items, but query result count is off
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-search.pl
CONFLICT (content): Merge conflict in opac/opac-search.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 8462 - OpacHiddenItems and hidelostitems hide items, but query result count is off
Comment 33 Mark Tompsett 2014-06-11 22:23:46 UTC Comment hidden (obsolete)
Comment 34 Mark Tompsett 2014-06-11 22:26:21 UTC
All I did was rebase. The patch was obvious enough to fix conflicts.
Comment 35 Paola Rossi 2014-06-12 14:54:15 UTC
I've applied against master 3.17.00.007

I set pref OpacHiddenItems to 
itemlost: [1]

On OPAC the results count is 4 instead of 5, correct as required.

So I pass the patch to "Signed Off" status.
Comment 36 Paola Rossi 2014-06-12 14:55:03 UTC
Created attachment 28823 [details] [review]
Bug 8462 - OpacHiddenItems and hidelostitems hide items, but query result count is off
Comment 37 Mark Tompsett 2014-07-07 01:39:03 UTC
Please re-test on something with lots of results. Your result set size is 40 by default, I think. So your results are always going to be less than 40, even if there are thousands of matches. This is the case I am worried about, but don't have time to test. This is what the first tweak worries me about, the second line change looks fine to me.
Comment 38 Katrin Fischer 2014-07-07 04:31:05 UTC
Mark, should this be reset to 'needs signoff' then?
Comment 39 Mark Tompsett 2014-07-07 14:20:27 UTC
I haven't tested, so I can't say whether or not I am right or wrong. I'm just providing a test case in which I think it will fail.
Comment 40 Katrin Fischer 2014-07-12 19:11:35 UTC
Mark was right, with the patch, every search with more than OPACnumSearchResults results will show a counter with OPACnumSearchResults search results.

In my case 22.
Comment 41 Paola Rossi 2014-12-02 14:30:50 UTC
I pass this bug to a "Resolved - Moved" status because this solution is a dead end. 
See bug 11909.