As per a discussion with jcamins, hiding biblios with no items is a bad thing (tm). Only going to touch hidden items case.

1.
+my $sth = $dbh->prepare("SELECT * FROM items WHERE biblionumber=?;");
+$sth->execute($biblionumber);
+
+my @itemsmatchingbiblionumber;
+my $matchingitem = $sth->fetchrow_hashref;
+while ($matchingitem) {
+    push @itemsmatchingbiblionumber,$matchingitem;
+    $matchingitem = $sth->fetchrow_hashref;
+}

is perhaps better written as
my $itemsmatchingbiblionumber = $dbh->selectall_arrayref($sql, { Slice => {}}, $biblionumber)

2. Database queries should all be placed in modules (probably C4/Items.pm for this one, maybe theres even something similar already there) so they can be tested

3. There should be a check
if ($biblionumber) {
  get the items
  etc
}
so we do not make database calls in vain

Oops! Biblioitemnumber != Biblionumber. Back to square one.

Not quite sure how useful this is, if all items are hidden there will be no entry on the search results anyway.

In reply to your inquiry about the usefulness, this prevents data mining on private collections of items.

So much evil in this world...

I have concerns about the implementation of this -- in particular, for a bib that is /not/ going to end up hidden, the acts of retrieving all of the items and checking which ones are hidden occur /twice/.

In my opinion, this is likely to impose too high of a performance penalty for typical OPACs, which don't have high numbers of hidden items.

I suggest revising the patch so that rather than adding a call to GetHiddenItems(), the bailing-out occurs after the original call to GetHiddenItems().  Additional refactoring might be called for (but as the topic of a separate bug) to minimize the processing gets wasted whether or not the bib ends up hidden.

Also, I'm not a fan of the notion of using 0 as a "magic" biblionumber -- to bail out, just issue the redirect.

(In reply to Galen Charlton from comment #12)
> I have concerns about the implementation of this -- in particular, for a bib
> that is /not/ going to end up hidden, the acts of retrieving all of the
> items and checking which ones are hidden occur /twice/.

After looking for GetHiddenItems and GetItemsInfo, I see what you mean by twice.


> In my opinion, this is likely to impose too high of a performance penalty
> for typical OPACs, which don't have high numbers of hidden items.

This is the opac-details.pl page, I have a hard time imagining tens of thousands of items. Daily Newspapers perhaps? 2N is still order N, but I do agree there is room for optimization.


> Also, I'm not a fan of the notion of using 0 as a "magic" biblionumber -- to
> bail out, just issue the redirect.

I'm not a fan of multiple exits, but you have a valid point. Though, the magic bullet is already used in the case that no biblionumber is passed.


> I suggest revising the patch so that rather than adding a call to
> GetHiddenItems(), the bailing-out occurs after the original call to
> GetHiddenItems().

The code is such that I can move and reuse the @hiddenitems in my patched code, and dump @items2hide. This reduces that to a single call.

My concern is more about the GetItemsInfo double call, now that you pointed it out. I'm wondering about the "change back when I've fixed request.pl" comment from 2002. Can I effectively dump @itemsmatchingbiblionumber, and move @all_items into the patched area for use there too, deleting the comment? Or should I stick with a safer pushing the contents of @itemsmatchingbiblionumber to the @all_items, instead of the second call, so as to retain a hint of what the comment might be talking about?


> Additional refactoring might be called for (but as the
> topic of a separate bug) to minimize the processing gets wasted whether or
> not the bib ends up hidden.

I think a single line of code, "return () if (! $yaml =~ /\S/ );" or "return () unless $hidingrules;", in the GetHiddenItems code may be a tiny optimization of note, but agree that is best suited for another bug report.

I believe I addressed Galen's issues. Though, perhaps the "keep the comment entact" philosophy may be flawed. I'll rework if that is a problem.

I don't mean to be a pain, but array copy seems unnecessary. Why not just

@all_items = GetItemsInfo( $biblionumber );
if (scalar @hiddenitems == scalar @all_items )

I set the depends on, because bug 10872 just fixes that one line in C4::Items which is clear enough to eyeball. See GetHiddenItems() call to YAML::Load and ask yourself, where is it required or used? opac-detail.pl and opac-ISBDdetail.pl both load something which uses or requires YAML, but opac-MARCdetail.pl has a smaller use list, which does not.

This fixes the entire block displaying. Bug 10876 is a partial display issue for opac-MARCdetail.pl

Test Before Apply
=================
Somehow part of an earlier version of this made it into master. This means that opac-details.pl will generate a 404.pl page following the "Test After Apply" steps, but the other two URLs will not.

1) Log into staff client and blank OPACHiddenItems
2) Open a browser to the OPAC.
3) Find something that has multiple items (e.g. same book in two branches)
4) Click on the link (e.g. demo.library.kohasystem.ca/cgi-bin/koha/opac-detail.pl?biblionumber=33158)
5) Click on "Marc view", and the same number of items should be displayed.
6) Click on "ISBD view", and an entry should come up.
7) In the staff client hide all the items (e.g. homebranch: [DAV,MNL])
8) Click on the "Normal View", and you should get a 404.pl page.
-- this is due to the push that was already done.
9) Paste in the opac-MARCdetail.pl url (e.g. demo.library.kohasystem.ca/cgi-bin/koha/opac-MARCdetail.pl?biblionumber=33158)
   Items should display as before. :(
10) Paste in the opac-ISBDdetail.pl url (e.g. demo.library.kohasystem.ca/cgi-bin/koha/opac-ISBDdetail.pl?biblionumber=33158)
    Page should be as before.
11) Click the logo, and redo the initial search
12) Nothing related to the biblionumber used should come up.

Test After Apply
================
1) Log into staff client and blank OPACHiddenItems
2) Open a browser to the OPAC.
3) Find something that has multiple items (e.g. same book in two branches)
4) Click on the link (e.g. demo.library.kohasystem.ca/cgi-bin/koha/opac-detail.pl?biblionumber=33158)
5) Click on "Marc view", and the same number of items should be displayed.
6) Click on "ISBD view", and an entry should come up.
7) In the staff client hide all the items (e.g. homebranch: [DAV,MNL])
8) Click on the "Normal View", and you should get a 404.pl page.
9) Paste in the opac-MARCdetail.pl url (e.g. demo.library.kohasystem.ca/cgi-bin/koha/opac-MARCdetail.pl?biblionumber=33158)
   A 404.pl page should be given. IF NOT, YOU FORGOT TO TEST AND APPLY BUG 10872.
10) Paste in the opac-ISBDdetail.pl url (e.g. demo.library.kohasystem.ca/cgi-bin/koha/opac-ISBDdetail.pl?biblionumber=33158)
    A 404.pl page should be given.
11) Click the logo, and redo the initial search
12) Nothing related to the biblionumber used should come up.

This bug fixes the full hidden case.

patch works as advertised, and looks good :)

Created attachment 21076 [details] [review]
Bug 10584 - Hide OPAC biblio details if all items are hidden

If there are items for a given biblio number, and they are all
hidden, then biblio needs to be hidden. If the biblio needs to
be hidden, it immediately redirects to a 404.pl page, just as
if the biblionumber does not exist.

Arrays used to represent all the items were relocated and used,
added if they didn't exist. Arrays representing the hidden
items were relocated and used if they existed, added if they
didn't exist.

Upon debugging the opac-MARCdetail.pl modification, it was
discovered the reason getHiddenItems was failing was because
'use YAML qw/Load/;' was not mentioned in C4::Items, and other
libraries were triggering the loading of YAML to compensate
for opac-detail.pl and opac-ISBDdetail.pl files.

Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Not sure a redirect to 404 is the better thing to do but it is the existing behavior.

Marked as Passed QA.

I believe Jonathan is referring to the optimization of if there is no biblionumber then redirect to 404.pl that has been added. No biblionumber means it will fail to get a record, which means it will redirect to 404.pl anyways. The immediate redirect to 404.pl is an optimization, because it makes no sense to run a bunch of function calls just to end up going to 404.pl anyways.

If it should give a meaningful error, the optimization makes it possible to do it sooner rather than later -- in another bug.

Marking this as an enhancement -- it falls on the bug/enhancement boundary, but on balance, this change in behavior leans a bit more towards the latter IMO.

Pushed to master.  Thanks, Mark!

What is a bit strange is that when you make a search without results you've got a "No results found!" answer. But if the search finds only one biblio matching, but all items are hidden, then a 404 error page is displaid. 
OPAC users don't understand the difference and for them 404 error page means the system fails, not that there are no results to their search.

Oooo... sounds like a test case missed. Because the hidden item wouldn't be listed in a search, so there would be no way to go to the details page. And looking for the page of a hidden item is like looking for a biblio that is non-existant (404.pl). However, Koha when there is only one result automatically goes to the detail page. I'll look into this.