Bug 19704 - OPACs most popular feature includes titles hidden by OpacHiddenItems
Summary: OPACs most popular feature includes titles hidden by OpacHiddenItems
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P3 normal
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
: 11690 20662 (view as bug list)
Depends on:
Blocks: 24403
  Show dependency tree / graph
 
Reported: 2017-11-27 21:39 UTC by Alex Buckley
Modified: 2023-11-01 11:24 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the "Most popular" list in the OPAC (enabled using the OpacTopissue system preference) so that items are correctly excluded when the OpacSuppression and OpacHiddenItems system preferences are configured. Before this, settings for these two system preferences had no effect on the "Most popular" page.
Version(s) released in:
Circulation function:

Attachments
Bug 17904: Make OPAC "Most popular" respect hidden record sysprefs (5.21 KB, patch)
2022-06-23 00:11 UTC, David Cook 		Details | Diff | Splinter Review
Bug 19704: Make OPAC "Most popular" respect hidden record sysprefs (5.16 KB, patch)
2022-06-23 00:29 UTC, David Cook 		Details | Diff | Splinter Review
Bug 19704: Make OPAC "Most popular" respect hidden record sysprefs (5.21 KB, patch)
2022-07-07 12:42 UTC, David Nind 		Details | Diff | Splinter Review
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2017-11-27 21:39:28 UTC 
When a library item has multiple bibliographic records and associated items with differing item types (e.g. fiction and express fiction and non-fiction) the Most popular feature is displaying links to items which are set to be hidden in the opachiddenitems syspref.

When the user clicks on the link to these items a 404 error is displayed to the user. 

Therefore the most popular feature is incorrectly displaying links to hidden items.
Comment 1 David Cook 2021-06-02 05:50:56 UTC 
*** Bug 20662 has been marked as a duplicate of this bug. ***
Comment 2 David Cook 2022-01-04 03:01:28 UTC 
Still an issue in master
Comment 3 David Cook 2022-01-04 03:03:44 UTC 
It should also take into account OpacSuppression
Comment 4 David Cook 2022-06-22 02:37:19 UTC 
"OpacHiddenItems" is such a pain...

I was thinking that it's too bad that we don't just save a "hidden" status when items are saved, but then I remembered that you can update "OpacHiddenItems" at any time, and that can dramatically change which records are hidden, and it would be expensive to re-calculate a stored state for every record as a result...

But with C4::Circulation::GetTopIssues, we need to process every record fetched to see if it's hidden... 

I suppose we could fetch $count and if X records are hidden then we need to fetch at least X new records (based off the $count offset) to try to fill in the gaps. 

It wouldn't be very efficient but it might be the only way of working with the current "hidden" model...
Comment 5 David Cook 2022-06-22 02:39:33 UTC 
Technically, using multiple queries could also mean we could run into a race condition but I can't think of any other way of doing it.
Comment 6 David Cook 2022-06-22 02:40:14 UTC 
(In reply to David Cook from comment #5)
> Technically, using multiple queries could also mean we could run into a race
> condition but I can't think of any other way of doing it.

Actually, one safeguard against that would be to initially pre-fetch X records more than we need I suppose...
Comment 7 David Cook 2022-06-22 02:46:14 UTC 
Actually... I have another idea...

https://wiki.koha-community.org/wiki/OpacHiddenItems

We could read that YAML into a Perl hash and then create a DBIC query or use SQL::Abstract...
Comment 8 David Cook 2022-06-22 02:47:56 UTC 
Of course, that wouldn't take into account "OpacSuppression" which isn't stored anywhere except the MARCXML. 

Maybe a database column would make sense for "OpacSuppression"...
Comment 9 David Cook 2022-06-22 05:31:46 UTC 
(In reply to David Cook from comment #7)
> Actually... I have another idea...
> 
> https://wiki.koha-community.org/wiki/OpacHiddenItems
> 
> We could read that YAML into a Perl hash and then create a DBIC query or use
> SQL::Abstract...

Ah the syspref "OpacHiddenItemsExceptions" exists... that makes it even more fun.
Comment 10 David Cook 2022-06-23 00:11:53 UTC Comment hidden (obsolete)
Comment 11 David Cook 2022-06-23 00:29:00 UTC 
Created attachment 136434 [details] [review]
Bug 19704: Make OPAC "Most popular" respect hidden record sysprefs

At the moment, OpacSuppression and OpacHiddenItems don't affect the output
of the OPAC's "Most popular" page.

This patch adds support for the OpacSuppression and OpacHiddenItems system preferences, so
that items are excluded from the "Most popular" calculation based on the hidden rules.

Test plan:
0a. Apply patches
0b. koha-plack --restart kohadev
0c. Log into Koha as "koha" user
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=OpacTopissue
2. Enable "OpacTopissue"
3. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
4. Checkout 39999000001310
5. Checkout 39999000005578
6. Checkout 39999000004571
7. Checkout 39999000005592
8. Go to http://localhost:8080/cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1
9. See 3 titles
10. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=OpacHiddenItems
11. Change "OpacHiddenItems" to the following (within the single quotes):
barcode: [39999000001310,39999000005578]
location: ['RANDOM','THAT','THIS']
12. Go to http://localhost:8080/cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1
13. See 2 titles ("Gairm" is hidden, as is one of the items for "Continuous delivery /")
14. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=OpacHiddenItemsExceptions
15. Update the syspref to 'S'
16. Go to http://localhost:8080/cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1
17. See 3 titles again

18. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=OpacSuppression
19. Change syspref to "Hide"
20. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=114
21. Change 942$n to "Yes" and save record
22. Go to http://localhost:8080/cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1
23. See 2 titles ("Clean code : a handbook of agile software craftsmanship / Robert C. Martin ... [et al.]" is suppressed)
Comment 12 David Nind 2022-07-07 12:42:31 UTC 
Created attachment 137273 [details] [review]
Bug 19704: Make OPAC "Most popular" respect hidden record sysprefs

At the moment, OpacSuppression and OpacHiddenItems don't affect the output
of the OPAC's "Most popular" page.

This patch adds support for the OpacSuppression and OpacHiddenItems system preferences, so
that items are excluded from the "Most popular" calculation based on the hidden rules.

Test plan:
0a. Apply patches
0b. koha-plack --restart kohadev
0c. Log into Koha as "koha" user
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=OpacTopissue
2. Enable "OpacTopissue"
3. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
4. Checkout 39999000001310
5. Checkout 39999000005578
6. Checkout 39999000004571
7. Checkout 39999000005592
8. Go to http://localhost:8080/cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1
9. See 3 titles
10. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=OpacHiddenItems
11. Change "OpacHiddenItems" to the following (within the single quotes):
barcode: [39999000001310,39999000005578]
location: ['RANDOM','THAT','THIS']
12. Go to http://localhost:8080/cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1
13. See 2 titles ("Gairm" is hidden, as is one of the items for "Continuous delivery /")
14. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=OpacHiddenItemsExceptions
15. Update the syspref to 'S'
16. Go to http://localhost:8080/cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1
17. See 3 titles again

18. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=OpacSuppression
19. Change syspref to "Hide"
20. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=114
21. Change 942$n to "Yes" and save record
22. Go to http://localhost:8080/cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1
23. See 2 titles ("Clean code : a handbook of agile software craftsmanship / Robert C. Martin ... [et al.]" is suppressed)

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 Katrin Fischer 2022-07-08 09:14:50 UTC 
Quite keen to see this one fixed, but we have 2 complaints in the QA test tools. Can you please fix?

 FAIL	C4/Circulation.pm
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 2 violation(s).
Comment 14 David Cook 2022-07-24 23:31:17 UTC 
(In reply to Katrin Fischer from comment #13)
> Quite keen to see this one fixed, but we have 2 complaints in the QA test
> tools. Can you please fix?
> 
>  FAIL	C4/Circulation.pm
>    FAIL	  critic
> 		# Variables::ProhibitConditionalDeclarations: Got 2 violation(s).

I saw your message, and it's on my TODO list. I'll be coming back to this!
Comment 15 David Cook 2022-09-26 06:13:15 UTC 
(In reply to David Cook from comment #14)
> (In reply to Katrin Fischer from comment #13)
> > Quite keen to see this one fixed, but we have 2 complaints in the QA test
> > tools. Can you please fix?
> > 
> >  FAIL	C4/Circulation.pm
> >    FAIL	  critic
> > 		# Variables::ProhibitConditionalDeclarations: Got 2 violation(s).
> 
> I saw your message, and it's on my TODO list. I'll be coming back to this!

My ever growing TODO list, unfortunately...
Comment 16 Katrin Fischer 2022-11-15 00:01:48 UTC 
Ping? :)
Comment 17 David Cook 2023-04-04 03:47:30 UTC 
I think I need to refactor this one.

Need to move the code into a separate module and add unit tests for it. 

The Hidden and Suppression fields need more methods built up around them...
Comment 18 David Cook 2023-04-04 03:52:31 UTC 
(In reply to David Cook from comment #17)
> I think I need to refactor this one.
> 
> Need to move the code into a separate module and add unit tests for it. 
 
Although that's hard to do because it needs to produce SQL specific to the GetTopIssues function in C4::Circulation...
Comment 19 David Cook 2023-07-10 00:38:04 UTC 
*** Bug 11690 has been marked as a duplicate of this bug. ***
Comment 20 Katrin Fischer 2023-10-01 14:18:26 UTC 
Would be great to see this one revived.
Comment 21 David Cook 2023-10-09 02:09:06 UTC 
(In reply to Katrin Fischer from comment #20)
> Would be great to see this one revived.

It's pretty low on my list at the moment, so I probably won't be looking at it until early to mid 2024 I think.