Bug 18989

Summary: Allow displaying biblios with all items hidden by OpacHiddenItems
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: OPACAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: ctodd, dcook, fridolin.somers, jonathan.druart, katrin.fischer, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19064
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24403
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23923
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17787
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28299
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19704
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Currently the bibliographic record will be hidden in the OPAC, if all linked items are hidden by criteria defined in OpacHiddenItems. This patch adds a new system preference OpacHiddenItemsHidesRecord that allows to control this behavior making it possible to show the record, even if all items are hidden.
Version(s) released in:
21.05.00
Bug Depends on: 10584    
Bug Blocks: 28299, 28600    
Attachments: Bug 18989: Allow displaying biblios with all items hidden by OpacHiddenItems
Bug 18989: add atomic update
Bug 18989: add atomic update
Bug 18989: Allow displaying biblios with all items hidden by OpacHiddenItems
Bug 18989: Add atomic update
Bug 18989: (QA follow-up) Terminology fixes and linking the system preference in pref description
Bug 18989: (QA follow-up) Have new preference default to current behaviour
Bug 18989: Fix syspref's value selection
Bug 18989: Allow displaying biblios with all items hidden by OpacHiddenItems
Bug 18989: Add atomic update
Bug 18989: (QA follow-up) Terminology fixes and linking the system preference in pref description
Bug 18989: (QA follow-up) Have new preference default to current behaviour
Bug 18989: Fix syspref's value selection
Bug 18989: (QA follow-up) Make Koha::Biblio->hidden_in_opac aware of OpacHiddenItemsHidesRecord
[OPTIONAL] Bug 18989: (QA follow-up) Make controllers use Koha::Biblio->hidden_in_opac
[OPTIONAL] Bug 18989: (QA follow-up) Make controllers use Koha::Biblio->hidden_in_opac
But 18989: Restore hidding items on detail
Bug 18989: Restore hidding items on detail
Bug 18989: Restore hidding items on detail

Description Tomás Cohen Arazi 2017-07-27 12:48:14 UTC
Bug 10584 made Koha hide biblios for which all items match some criteria (in OpacHiddenItems) so they are hidden.

There should be a syspref controlling this behaviour.
Comment 1 Tomás Cohen Arazi 2017-07-28 21:49:03 UTC
Only for the record,the way removal is implemented makes search results show 19 records instead of 20 (if one record is removed).

We should definitely leverage on the search engine for this kind of stuff.
Comment 2 Katrin Fischer 2017-07-29 08:31:07 UTC
This is why the numbering in the result lists is removed, when you use OpacHiddenItems - to hide the 'holes'. Using the search engine would be better.
Comment 3 Owen Leonard 2017-08-01 17:19:23 UTC
It would be great if this could be done per itemtype. That would allow you to easily define an electronic record itemtype without having to create dummy items.
Comment 4 Tomás Cohen Arazi 2017-08-01 17:25:12 UTC
(In reply to Owen Leonard from comment #3)
> It would be great if this could be done per itemtype. That would allow you
> to easily define an electronic record itemtype without having to create
> dummy items.

I think this is already covered. The problem this bug higlights is that if all items are marked as hidden, then the biblio is hidden.
You can safely create a biblio without items and it will be displayed correctly.
Comment 5 Fridolin Somers 2020-01-21 11:21:02 UTC
Still valid.
We have to patch to comment code for some of our libraries.
Comment 6 Katrin Fischer 2020-01-21 12:45:50 UTC
Why not write a patch to make this a configuration option instead?
Comment 7 Fridolin Somers 2020-09-04 12:50:22 UTC
Created attachment 109656 [details] [review]
Bug 18989: Allow displaying biblios with all items hidden by OpacHiddenItems

Bug 10584 made Koha hide biblios for which all items match some criteria (in OpacHiddenItems) so they are hidden.
Add syspref OpacHiddenItemsHidesRecord controlling this behaviour.

Test plan :
1)
1.1) Create some biblio records with one item having damaged=1
1.2) Define system preference OpacHiddenItems = damaged: 1
2)
2.1) Set system preference OpacHiddenItemsHidesRecord to 'don't hide'
2.2) At OPAC : perform a search showing those records and some more
2.3) Check you see the all the records
2.4) For a record with hidden item check you don't get HTTP 404 for : normal view, ISBD view, MARC view
2.5) Check you can had tags on this record
2.6) Add record to basket, check you see it in basket
3)
3.1) Set system preference OpacHiddenItemsHidesRecord to 'hide'
3.2) At OPAC : perform a search showing those records and some more
3.3) Check you don't see the records with hidden item
3.4) For a record with hidden item check you get HTTP 404 for : normal view, ISBD view, MARC view
3.5) Show basket, check you see the records with hidden item
Comment 8 Fridolin Somers 2020-09-04 12:50:39 UTC
Created attachment 109657 [details] [review]
Bug 18989: add atomic update
Comment 9 Fridolin Somers 2020-09-04 12:52:36 UTC
I propose system preference "OpacHiddenItemsHidesRecord" so that you get it when searching "OpacHiddenItems" :D

Feel free to share your opinion.
I hope we can move on with this old problem ;)
Comment 10 Kelly McElligott 2020-12-29 23:41:15 UTC
Created attachment 114738 [details] [review]
Bug 18989: add atomic update
Comment 11 Katrin Fischer 2021-01-28 23:03:10 UTC
Created attachment 116000 [details] [review]
Bug 18989: Allow displaying biblios with all items hidden by OpacHiddenItems

Bug 10584 made Koha hide biblios for which all items match some criteria (in OpacHiddenItems) so they are hidden.
Add syspref OpacHiddenItemsHidesRecord controlling this behaviour.

Test plan :
1)
1.1) Create some biblio records with one item having damaged=1
1.2) Define system preference OpacHiddenItems = damaged: 1
2)
2.1) Set system preference OpacHiddenItemsHidesRecord to 'don't hide'
2.2) At OPAC : perform a search showing those records and some more
2.3) Check you see the all the records
2.4) For a record with hidden item check you don't get HTTP 404 for : normal view, ISBD view, MARC view
2.5) Check you can had tags on this record
2.6) Add record to basket, check you see it in basket
3)
3.1) Set system preference OpacHiddenItemsHidesRecord to 'hide'
3.2) At OPAC : perform a search showing those records and some more
3.3) Check you don't see the records with hidden item
3.4) For a record with hidden item check you get HTTP 404 for : normal view, ISBD view, MARC view
3.5) Show basket, check you see the records with hidden item

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2021-01-28 23:03:14 UTC
Created attachment 116001 [details] [review]
Bug 18989: Add atomic update

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2021-01-28 23:03:18 UTC
Created attachment 116002 [details] [review]
Bug 18989: (QA follow-up) Terminology fixes and linking the system preference in pref description
Comment 14 Katrin Fischer 2021-01-28 23:03:22 UTC
Created attachment 116003 [details] [review]
Bug 18989: (QA follow-up) Have new preference default to current behaviour

At the moments the records are hidden when all items are hidden, so
we need to make sure that we keep this behaviour when the preference
is added.
Comment 15 Katrin Fischer 2021-01-28 23:09:46 UTC
Hi Tomas, as the current expert on OpacHiddenItems - could you have an additional look here? I did 2 small follow-ups, testing so far everything works.
Comment 16 Victor Grousset/tuxayo 2021-03-27 00:59:41 UTC
Somehow the default value displayed in the UI is "Don't hide". The DB shows that the value is '1'.
When toggling the syspref, the value is 'yes' and 'no'.
So I tried with 'yes' instead of '1' in atomicupdate/bug_18989.sql but the default value is still '1' in the DB and "Don't hide" in the UI.

Any hints?
Comment 17 Katrin Fischer 2021-03-27 16:38:32 UTC
(In reply to Victor Grousset/tuxayo from comment #16)
> Somehow the default value displayed in the UI is "Don't hide". The DB shows
> that the value is '1'.
> When toggling the syspref, the value is 'yes' and 'no'.
> So I tried with 'yes' instead of '1' in atomicupdate/bug_18989.sql but the
> default value is still '1' in the DB and "Don't hide" in the UI.
> 
> Any hints?

Setting to failed QA. Wondering if this could have to do with the YAML library changes?
Comment 18 Jonathan Druart 2021-04-13 09:20:10 UTC
Created attachment 119524 [details] [review]
Bug 18989: Fix syspref's value selection

See commit 1fe1b704f9a81172f2a6b86367b314572a854d18
    Bug 22824: Fix yes/no vs 1/0
Comment 19 Katrin Fischer 2021-04-17 12:29:08 UTC
Created attachment 119784 [details] [review]
Bug 18989: Allow displaying biblios with all items hidden by OpacHiddenItems

Bug 10584 made Koha hide biblios for which all items match some criteria (in OpacHiddenItems) so they are hidden.
Add syspref OpacHiddenItemsHidesRecord controlling this behaviour.

Test plan :
1)
1.1) Create some biblio records with one item having damaged=1
1.2) Define system preference OpacHiddenItems = damaged: 1
2)
2.1) Set system preference OpacHiddenItemsHidesRecord to 'don't hide'
2.2) At OPAC : perform a search showing those records and some more
2.3) Check you see the all the records
2.4) For a record with hidden item check you don't get HTTP 404 for : normal view, ISBD view, MARC view
2.5) Check you can had tags on this record
2.6) Add record to basket, check you see it in basket
3)
3.1) Set system preference OpacHiddenItemsHidesRecord to 'hide'
3.2) At OPAC : perform a search showing those records and some more
3.3) Check you don't see the records with hidden item
3.4) For a record with hidden item check you get HTTP 404 for : normal view, ISBD view, MARC view
3.5) Show basket, check you see the records with hidden item

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2021-04-17 12:29:13 UTC
Created attachment 119785 [details] [review]
Bug 18989: Add atomic update

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2021-04-17 12:29:17 UTC
Created attachment 119786 [details] [review]
Bug 18989: (QA follow-up) Terminology fixes and linking the system preference in pref description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2021-04-17 12:29:22 UTC
Created attachment 119787 [details] [review]
Bug 18989: (QA follow-up) Have new preference default to current behaviour

At the moments the records are hidden when all items are hidden, so
we need to make sure that we keep this behaviour when the preference
is added.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2021-04-17 12:29:27 UTC
Created attachment 119788 [details] [review]
Bug 18989: Fix syspref's value selection

See commit 1fe1b704f9a81172f2a6b86367b314572a854d18
    Bug 22824: Fix yes/no vs 1/0

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Jonathan Druart 2021-04-21 13:22:32 UTC
Tomas, can you add, or not, your stamp on this one please?
Comment 25 Tomás Cohen Arazi 2021-04-21 13:44:54 UTC
Hi, I've been following this dev with interest, but failed to add my comments promptly. Sorry for that.

One of the reasons we've been chasing leaks in the OPAC regarding hidden things, is because, too often, we don't use the same logic in all places (in which it is meaningful to do so, of course).

With that in mind, we added things like

Koha::Item->hidden_in_opac
Koha::Biblio->hidden_in_opac

So, to add my stamp, I would like this logic to be added to Koha::Biblio->hidden_in_opac instead, and leave the controllers untouched. Tests should cover all cases and we will be (forever?) sure we don't leak things anymore.

I have doubts about opac-tags.pl, which has a more convoluted logic in it, and probably needs a rewrite (I think it is the missing piece in all this refactoring we've been doing).
Comment 26 Tomás Cohen Arazi 2021-04-23 19:51:30 UTC
Created attachment 120137 [details] [review]
Bug 18989: (QA follow-up) Make Koha::Biblio->hidden_in_opac aware of OpacHiddenItemsHidesRecord

This patch makes the method aware of the new syspref. It will only eval
the rules against items if the OpacHiddenItemsHidesRecord syspref is
set.

Tests are added to reflect this.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2021-04-23 19:55:56 UTC
Created attachment 120138 [details] [review]
[OPTIONAL] Bug 18989: (QA follow-up) Make controllers use Koha::Biblio->hidden_in_opac

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2021-04-23 19:57:23 UTC
I've decided to add a follow-up so we make Koha::Biblio->hidden_in_opac aware of the new syspref. But I decided to defer changing the places in which it would be used to anothe series of bugs.

Some of them are trivial, but others really require big refactorings (good ones, simplifying things, optimizing as well) that are out of the scope of this nice and simple feature addition.

I submit now a partial patch for those trivial cases, for the RM to decide. It will not be complete, and can be skipped to allow this feature to be pushed. And I will certainly add this to 21.11 roadmap.
Comment 29 Jonathan Druart 2021-04-26 10:25:48 UTC
 FAIL   opac/opac-MARCdetail.pl
   FAIL   valid
                Global symbol "$bibio" requires explicit package name (did you forget to declare "my $bibio"?) 
                opac/opac-MARCdetail.pl had compilation errors.
Comment 30 Tomás Cohen Arazi 2021-04-26 11:30:41 UTC
(In reply to Jonathan Druart from comment #29)
>  FAIL   opac/opac-MARCdetail.pl
>    FAIL   valid
>                 Global symbol "$bibio" requires explicit package name (did
> you forget to declare "my $bibio"?) 
>                 opac/opac-MARCdetail.pl had compilation errors.

So you are considering the inclusion of the partial follow-up? I can fix that, just thought you would prefer to handle that on separate bug reports.
Comment 31 Tomás Cohen Arazi 2021-04-26 11:35:06 UTC
Created attachment 120171 [details] [review]
[OPTIONAL] Bug 18989: (QA follow-up) Make controllers use Koha::Biblio->hidden_in_opac

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Jonathan Druart 2021-04-27 13:22:28 UTC
There is something wrong:
1. OpacHiddenItems: "itype: [VM]"
2. OpacHiddenItemsHidesRecord: Hide

3. Biblio 271 has 2 items VM
Hit /cgi-bin/koha/opac-detail.pl?biblionumber=271
=> Redirected to 404, OK

4. Add a new item to 271, itemtype=Music
5. Hit /cgi-bin/koha/opac-detail.pl?biblionumber=271
=> Record is displayed (OK) but all the 3 items appear in the table (KO)

It is ok in master.
Comment 33 Jonathan Druart 2021-05-04 10:19:03 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2021-05-04 10:19:49 UTC
Created attachment 120432 [details] [review]
Bug 18989: Restore hidding items on detail
Comment 35 Tomás Cohen Arazi 2021-05-05 17:19:57 UTC
Created attachment 120558 [details] [review]
Bug 18989: Restore hidding items on detail

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Good catch. I forgot this file didn't get the hiding logic refactoring
yet.
Comment 36 Jonathan Druart 2021-05-07 07:54:27 UTC
What about opac-shelves?

Items are not hidden there...
Comment 37 Jonathan Druart 2021-05-07 09:21:46 UTC
(In reply to Jonathan Druart from comment #36)
> What about opac-shelves?
> 
> Items are not hidden there...

See bug 28299.

Additional question: Does it really make sense to have this one part of 21.05 with all the other OpacHiddenItems bugs we have (see bug 24403)?
Comment 38 Tomás Cohen Arazi 2021-05-07 13:31:00 UTC
(In reply to Jonathan Druart from comment #37)
> (In reply to Jonathan Druart from comment #36)
> > What about opac-shelves?
> > 
> > Items are not hidden there...
> 
> See bug 28299.
> 
> Additional question: Does it really make sense to have this one part of
> 21.05 with all the other OpacHiddenItems bugs we have (see bug 24403)?

I think it makes total sense. Most of those bugs are cosmetic (facets that won't return results, wrong counts). The other ones shouldn't prevent this to get into master. Those need to be rewritten using Koha::Biblio->hidden_in_opac and friends, which takes into account what is introduced on this bug.
Comment 39 Jonathan Druart 2021-05-12 15:46:57 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 40 Fridolin Somers 2021-05-18 09:58:24 UTC
Enhancement not pushed to 20.11.x
Comment 41 David Cook 2022-06-22 05:37:27 UTC
(In reply to Tomás Cohen Arazi from comment #1)
> Only for the record,the way removal is implemented makes search results show
> 19 records instead of 20 (if one record is removed).
> 
> We should definitely leverage on the search engine for this kind of stuff.

+1
Comment 42 David Cook 2022-06-22 05:46:26 UTC
(In reply to Tomás Cohen Arazi from comment #1)
> Only for the record,the way removal is implemented makes search results show
> 19 records instead of 20 (if one record is removed).
> 
> We should definitely leverage on the search engine for this kind of stuff.

Although there are times like bug 19704 where the search engine isn't involved... and sysprefs like "OpacHiddenItemsExceptions" add the patron category to the calculation which might make that search engine idea impossible.

I reckon the whole OpacHidden* needs an overhaul...