Bug 10584 - Hide OPAC biblio details if all items are hidden.
Summary: Hide OPAC biblio details if all items are hidden.
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: M. Tompsett
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 10872
Blocks: 18989 10876
  Show dependency treegraph
 
Reported: 2013-07-13 00:00 UTC by M. Tompsett
Modified: 2017-12-19 10:19 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
If there is at least one item and every item is hidden, hide the biblio entry. (2.32 KB, patch)
2013-07-13 00:36 UTC, M. Tompsett
Details | Diff | Splinter Review
If there is at least one item and every item is hidden, hide the biblio entry. (1.56 KB, patch)
2013-07-17 04:44 UTC, M. Tompsett
Details | Diff | Splinter Review
If there is at least one item and every item is hidden, hide the biblio entry. (1.54 KB, patch)
2013-07-17 13:08 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10584 - Hide OPAC biblio details if all items are hidden (1.58 KB, patch)
2013-07-18 00:53 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 10584 - Hide OPAC biblio details if all items are hidden (1.67 KB, patch)
2013-09-06 12:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10584 - Hide OPAC biblio details if all items are hidden (2.35 KB, patch)
2013-09-09 00:44 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10584 - Hide OPAC biblio details if all items are hidden (5.15 KB, patch)
2013-09-09 03:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10584 - Hide OPAC biblio details if all items are hidden (4.79 KB, patch)
2013-09-13 03:11 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10584 - Hide OPAC biblio details if all items are hidden (4.88 KB, patch)
2013-09-13 09:01 UTC, Mason James
Details | Diff | Splinter Review
Bug 10584 - Hide OPAC biblio details if all items are hidden (4.90 KB, patch)
2013-09-13 09:35 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description M. Tompsett 2013-07-13 00:00:20 UTC
Using the OpacHiddenItems systempreference, items can be hidden. However, it looks strange to have a biblio entry in the OPAC if all the items are hidden or there are no items. In this case, hide the biblio too.

Test
====
1) Find a biblio with multiple item records across multiple branches.
2) View it in OPAC.
3) In the staff client hide one of the branches using OpacHiddenItems.
4) Refresh the OPAC details. There should be less, because of the hidden branch.
5) In the staff client hide all of the branches (e.g. homebranch: [BR01,BR02])
6) Refresh the OPAC details. A biblio entry with no items will be displayed.
7) In the staff client enter a dummy biblio record, with no items.
8) View the OPAC details for that biblio. A biblio entry with no items will be displayed.

NOTE: There is a tiny scope creep, in that a silent warning hazard for the biblionumber was also corrected. (forgive me)

9) Go to .../cgi-bin/koha/opac-detail.pl?biblionumber= with no number entered.  There should be a 404 output page.
10) Check the opac error log file. There should be a warning related to the int(undef)
11) Apply the patch.
12) Go to .../cgi-bin/koha/opac-detail.pl?biblionumber= with no number entered.  There should be a 404 output page.
13) There should be no new warning.
14) Go to the dummy biblio that you entered (.../cgi-bin/koha/opac-detail.pl?biblionumber=#####). There should be a 404 output page.
15) Go back to the initial biblionumber (.../cgi-bin/koha/opac-detail.pl?biblionumber=#####).  There should be a 404 output page.

Displaying 404.pl is not the best or correct solution (see bug 2318's comment discussion), but it is sufficient, until that gets corrected.
Comment 1 M. Tompsett 2013-07-13 00:11:34 UTC
As per a discussion with jcamins, hiding biblios with no items is a bad thing (tm). Only going to touch hidden items case.
Comment 2 M. Tompsett 2013-07-13 00:36:29 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2013-07-17 03:39:10 UTC
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
Comment 4 M. Tompsett 2013-07-17 04:44:44 UTC Comment hidden (obsolete)
Comment 5 M. Tompsett 2013-07-17 04:48:28 UTC
Oops! Biblioitemnumber != Biblionumber. Back to square one.
Comment 6 M. Tompsett 2013-07-17 13:08:31 UTC Comment hidden (obsolete)
Comment 7 Srdjan Jankovic 2013-07-18 00:53:58 UTC Comment hidden (obsolete)
Comment 8 Srdjan Jankovic 2013-07-18 00:55:43 UTC
Not quite sure how useful this is, if all items are hidden there will be no entry on the search results anyway.
Comment 9 M. Tompsett 2013-07-18 01:03:08 UTC
In reply to your inquiry about the usefulness, this prevents data mining on private collections of items.
Comment 10 Srdjan Jankovic 2013-07-18 01:09:37 UTC
So much evil in this world...
Comment 11 Kyle M Hall 2013-09-06 12:37:24 UTC Comment hidden (obsolete)
Comment 12 Galen Charlton 2013-09-07 21:24:32 UTC
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.
Comment 13 M. Tompsett 2013-09-08 04:50:53 UTC
(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.
Comment 14 M. Tompsett 2013-09-09 00:44:57 UTC Comment hidden (obsolete)
Comment 15 M. Tompsett 2013-09-09 00:48:21 UTC
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.
Comment 16 Srdjan Jankovic 2013-09-09 01:23:04 UTC
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 )
Comment 17 M. Tompsett 2013-09-09 03:47:57 UTC Comment hidden (obsolete)
Comment 18 M. Tompsett 2013-09-13 03:11:38 UTC Comment hidden (obsolete)
Comment 19 M. Tompsett 2013-09-13 04:04:11 UTC
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.
Comment 20 Mason James 2013-09-13 09:01:35 UTC Comment hidden (obsolete)
Comment 21 Mason James 2013-09-13 09:02:43 UTC
patch works as advertised, and looks good :)
Comment 22 Jonathan Druart 2013-09-13 09:35:56 UTC
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>
Comment 23 Jonathan Druart 2013-09-13 09:38:44 UTC
Not sure a redirect to 404 is the better thing to do but it is the existing behavior.

Marked as Passed QA.
Comment 24 M. Tompsett 2013-09-13 10:19:42 UTC
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.
Comment 25 Galen Charlton 2013-09-18 15:23:13 UTC
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.
Comment 26 Galen Charlton 2013-09-18 16:07:40 UTC
Pushed to master.  Thanks, Mark!
Comment 27 Sophie MEYNIEUX 2013-12-20 13:26:15 UTC
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.
Comment 28 M. Tompsett 2013-12-20 18:55:43 UTC
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.