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.

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.

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.

(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.

Still valid.
We have to patch to comment code for some of our libraries.

Why not write a patch to make this a configuration option instead?

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

Created attachment 109657 [details] [review]
Bug 18989: add atomic update

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 ;)

Created attachment 114738 [details] [review]
Bug 18989: add atomic update

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>

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>

Created attachment 116002 [details] [review]
Bug 18989: (QA follow-up) Terminology fixes and linking the system preference in pref description

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.

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.

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?

(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?

Created attachment 119524 [details] [review]
Bug 18989: Fix syspref's value selection

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

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>

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>

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>

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>

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>

Tomas, can you add, or not, your stamp on this one please?

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).

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>

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>

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.

 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.

(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.

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>

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.

Created attachment 120432 [details] [review]
Bug 18989: Restore hidding items on detail

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.

What about opac-shelves?

Items are not hidden there...

(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)?

(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.

Pushed to master for 21.05, thanks to everybody involved!

Enhancement not pushed to 20.11.x

(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

(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...