Bug 15448 - Placing hold on specific items doesn't enforce OpacHiddenItems
Summary: Placing hold on specific items doesn't enforce OpacHiddenItems
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major with 16 votes (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Victor Grousset/tuxayo
URL:
Keywords: rel_21_05_candidate
: 18167 (view as bug list)
Depends on: 24254
Blocks: 24403
  Show dependency treegraph
 
Reported: 2015-12-30 19:57 UTC by Jessie Zairo
Modified: 2021-02-11 22:09 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.02,20.05.09,19.11.15


Attachments
Suppressed items shown (23.63 KB, image/jpeg)
2015-12-30 19:57 UTC, Jessie Zairo
Details
Bug 15448: Clean existing code in opac-detail (8.29 KB, patch)
2020-11-20 11:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15448: Copy code to opac-reserve.pl (2.61 KB, patch)
2020-11-20 11:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15448: Filter out items hidden in OPAC on placing hold (2.78 KB, patch)
2020-12-04 12:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15448: (follow-up) Fetch the patron category once (1.48 KB, patch)
2020-12-04 12:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15448: (QA follow-up) Move fetching the rules outside the loop (1.91 KB, patch)
2020-12-04 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15448: Filter out items hidden in OPAC on placing hold (2.57 KB, patch)
2020-12-04 14:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15448: (follow-up) Fetch the patron category once (1.46 KB, patch)
2020-12-04 14:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15448: Filter out items hidden in OPAC on placing hold (2.63 KB, patch)
2020-12-04 18:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15448: (follow-up) Fetch the patron category once (1.53 KB, patch)
2020-12-04 18:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15448: Filter out items hidden in OPAC on placing hold (2.67 KB, patch)
2020-12-08 16:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 15448: (follow-up) Fetch the patron category once (1.57 KB, patch)
2020-12-08 16:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 15448: (follow-up) Adapt to changes on bug 24254 (1.77 KB, patch)
2020-12-21 19:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15448: Fix ISE (1.39 KB, patch)
2021-01-14 09:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15448: Remove items fetch (1.60 KB, patch)
2021-01-14 09:46 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 Jessie Zairo 2015-12-30 19:57:05 UTC
Created attachment 46100 [details]
Suppressed items shown

When a patron places a hold on a specific item in the copiesrow table, suppressed items show in the OPAC.

System preference for OPACSuppression is set to Hide. System preference for OPAC Hidden Items is set to hide is suppressed items.
Comment 1 Katrin Fischer 2016-02-29 23:30:34 UTC
Can you share a bit more about your configuration? What's the exact entry in opachiddenitems? Does it work on the detail page?
Comment 2 Owen Leonard 2016-03-01 17:01:54 UTC
I could reproduce this by:

1. Allowing item-level holds for an itemtype in Circulation and Fine rules.
2. Setting the OpacHiddenItems preference to "withdrawn: [1]."
3. Edit an item and set the withdrawn status to 1.
4. Search for the title in the OPAC. The withdrawn item isn't shown.
5. View the detail page for the title. The withdrawn item isn't shown.
6. Place a hold on the item:

  -- Click "Show more options"
  -- Select "A specific item"
  -- Observe that the withdrawn item appears in the list of items.

(along with item which are lost, by the way, even though hidelostitems is set to "don't show")

I don't think the OPACSuppression preference comes into play here.
Comment 3 Jessie Zairo 2017-01-24 14:17:58 UTC
In 16.05.05 when a rule to hide specific items in OPACHiddenItems is set, the item does not show in the holdings table, but does appear when the user attempts to place the hold.

1. Circulation and Fine rules for item-level holds are set to allow.
2. Setting the OpacHiddenItems preference to "ccode: [SUPPRESS]."
3. An item with collection Code Suppress is set.
4. Search for the title in the OPAC. The Suppress ccode item isn't shown.
5. View the detail page for the title. The suppress ccode item isn't shown.
6. Place a hold on the item:

  -- Click "Show more options"
  -- Select "A specific item"
  -- the ccode with the opachiddenitems rule has an item that appear in the list to place a hold.

Jessie
Comment 4 Katrin Fischer 2019-08-18 08:19:35 UTC
Confirmed on current master. Updating to normal.
Comment 5 Fridolin Somers 2019-09-26 13:56:29 UTC
Maybe hidden items should be removed directly in C4::Items::GetItemsInfo, called by opac-reserve.pl
Comment 6 Katrin Fischer 2019-09-30 07:42:22 UTC
As hidden items are hidden for reason... bumping up severity a little bit to get more attention.
Comment 7 Katrin Fischer 2019-09-30 07:43:05 UTC
(In reply to Fridolin SOMERS from comment #5)
> Maybe hidden items should be removed directly in C4::Items::GetItemsInfo,
> called by opac-reserve.pl

Can you explain this a bit more? Not sure how it works right now, did you mean move or remove?
Comment 8 Katrin Fischer 2019-11-02 20:48:58 UTC
Old and mean bug with quite some votes... just giving it another bump.
Comment 9 Fridolin Somers 2019-11-07 10:58:00 UTC
(In reply to Katrin Fischer from comment #7)
> (In reply to Fridolin SOMERS from comment #5)
> > Maybe hidden items should be removed directly in C4::Items::GetItemsInfo,
> > called by opac-reserve.pl
> 
> Can you explain this a bit more? Not sure how it works right now, did you
> mean move or remove?

I mean remove, so they are never given to the OPAC.
One should then add a new arg to the method to say opac or staff interface.
Comment 10 Katrin Fischer 2020-01-11 18:21:20 UTC
*** Bug 18167 has been marked as a duplicate of this bug. ***
Comment 11 Chris Kirby 2020-07-07 19:13:11 UTC
I would like to add another vote to seeing this bug fixed.
Comment 12 Christopher Brannon 2020-07-07 19:50:43 UTC
(In reply to Chris Kirby from comment #11)
> I would like to add another vote to seeing this bug fixed.

+1
Comment 13 Andrew Fuerste-Henry 2020-11-06 12:57:02 UTC
+1 on this!
Comment 14 Jonathan Druart 2020-11-20 11:26:10 UTC
Created attachment 113875 [details] [review]
Bug 15448: Clean existing code in opac-detail

This patch groups the code related to OpacHiddenItems and hidelostitems.
It certainly fixed few issues (we are now filtering after EasyAnalyticalRecords), maybe not...

At least now the code is at the same place.
Comment 15 Jonathan Druart 2020-11-20 11:26:14 UTC
Created attachment 113876 [details] [review]
Bug 15448: Copy code to opac-reserve.pl

So yes, this code must be moved to a module, and use in the different
places when the items are not hidden at the OPAC (See bug 24403).
Call this a first step.
Comment 16 Jonathan Druart 2020-11-20 11:27:27 UTC
Those 2 patches should fix the reported issue. But they are not ready for integration IMO.

However it would be good to have early feedback from testers and QA.
Comment 17 Tomás Cohen Arazi 2020-11-30 16:10:48 UTC
The code should somehow be using Koha::Item->hidden_in_opac.
Comment 18 Jonathan Druart 2020-11-30 16:20:09 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> The code should somehow be using Koha::Item->hidden_in_opac.

That's for hidelostitems, not directly related. There are other bug reports for that.
But yes, everything should end up there at some point.
Comment 19 Tomás Cohen Arazi 2020-12-03 18:30:24 UTC
I want to propose an alternate solution based on bug 24254. Writing the alternate patch. I would love your feedback on that bug and if it makes sense to use here.
Comment 20 Tomás Cohen Arazi 2020-12-03 18:31:28 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> I want to propose an alternate solution based on bug 24254. Writing the
> alternate patch. I would love your feedback on that bug and if it makes
> sense to use here.

^^ this last comment is because the other bug introduces a new method and it could make things harder for bakporting to the stable branches (don't really think so, but just in case)
Comment 21 Tomás Cohen Arazi 2020-12-04 12:21:50 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2020-12-04 12:21:55 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2020-12-04 13:08:05 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2020-12-04 14:02:31 UTC
Created attachment 114193 [details] [review]
Bug 15448: Filter out items hidden in OPAC on placing hold

This patch leverages on bug 24254's
Koha::Items->filter_by_visible_in_opac to filter out items that
shouldn't be presented to the end user in the OPAC interface.

To test:
1. Circulation and Fine rules for item-level holds are set to allow.
2. Setting the OpacHiddenItems preference to "ccode: [SUPPRESS]"
3. An item with collection Code Suppress is set.
4. Search for the title in the OPAC.
=> SUCCESS: The Suppress ccode item isn't shown.
5. View the detail page for the title.
=> SUCCESS: The suppress ccode item isn't shown.
6. Place a hold on the item:
  -- Click "Show more options"
  -- Select "A specific item"
=> FAIL: the ccode with the opachiddenitems rule has an item that appear in the list to place a hold
7. Apply this patch and repeat 6
=> SUCCESS: The item doesn't show!
8. Sign off :-D
Comment 25 Tomás Cohen Arazi 2020-12-04 14:02:36 UTC
Created attachment 114194 [details] [review]
Bug 15448: (follow-up) Fetch the patron category once
Comment 26 Kyle M Hall 2020-12-04 18:27:19 UTC
Created attachment 114208 [details] [review]
Bug 15448: Filter out items hidden in OPAC on placing hold

This patch leverages on bug 24254's
Koha::Items->filter_by_visible_in_opac to filter out items that
shouldn't be presented to the end user in the OPAC interface.

To test:
1. Circulation and Fine rules for item-level holds are set to allow.
2. Setting the OpacHiddenItems preference to "ccode: [SUPPRESS]"
3. An item with collection Code Suppress is set.
4. Search for the title in the OPAC.
=> SUCCESS: The Suppress ccode item isn't shown.
5. View the detail page for the title.
=> SUCCESS: The suppress ccode item isn't shown.
6. Place a hold on the item:
  -- Click "Show more options"
  -- Select "A specific item"
=> FAIL: the ccode with the opachiddenitems rule has an item that appear in the list to place a hold
7. Apply this patch and repeat 6
=> SUCCESS: The item doesn't show!
8. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2020-12-04 18:27:30 UTC
Created attachment 114209 [details] [review]
Bug 15448: (follow-up) Fetch the patron category once

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Victor Grousset/tuxayo 2020-12-08 16:36:07 UTC
Created attachment 114263 [details] [review]
Bug 15448: Filter out items hidden in OPAC on placing hold

This patch leverages on bug 24254's
Koha::Items->filter_by_visible_in_opac to filter out items that
shouldn't be presented to the end user in the OPAC interface.

To test:
1. Circulation and Fine rules for item-level holds are set to allow.
2. Setting the OpacHiddenItems preference to "ccode: [SUPPRESS]"
3. An item with collection Code Suppress is set.
4. Search for the title in the OPAC.
=> SUCCESS: The Suppress ccode item isn't shown.
5. View the detail page for the title.
=> SUCCESS: The suppress ccode item isn't shown.
6. Place a hold on the item:
  -- Click "Show more options"
  -- Select "A specific item"
=> FAIL: the ccode with the opachiddenitems rule has an item that appear in the list to place a hold
7. Apply this patch and repeat 6
=> SUCCESS: The item doesn't show!
8. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 29 Victor Grousset/tuxayo 2020-12-08 16:36:12 UTC
Created attachment 114264 [details] [review]
Bug 15448: (follow-up) Fetch the patron category once

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 30 Victor Grousset/tuxayo 2020-12-08 16:53:13 UTC
Works a expect, QA script ok, code looks ok.
Comment 31 Katrin Fischer 2020-12-09 07:43:38 UTC
Glad to see this one moving!
Comment 32 Jonathan Druart 2020-12-09 10:17:04 UTC
Bug 24254 is not PQA yet.
Comment 33 Victor Grousset/tuxayo 2020-12-09 15:45:47 UTC
Now it is, thanks Martin! :)
Comment 34 Tomás Cohen Arazi 2020-12-21 19:31:30 UTC
Created attachment 114578 [details] [review]
Bug 15448: (follow-up) Adapt to changes on bug 24254

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Jonathan Druart 2021-01-14 09:41:08 UTC
The method Koha::Items->itemnumber is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Objects.pm line 592
Koha::Objects::AUTOLOAD('Koha::Items=HASH(0x55981fd94790)') called at /kohadevbox/koha/opac/opac-reserve.pl line 465


Hum?
Comment 36 Jonathan Druart 2021-01-14 09:46:39 UTC
Created attachment 115133 [details] [review]
Bug 15448: Fix ISE

The method Koha::Items->itemnumber is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Objects.pm line 592
Koha::Objects::AUTOLOAD('Koha::Items=HASH(0x55981fd94790)') called at /kohadevbox/koha/opac/opac-reserve.pl line 465
Comment 37 Jonathan Druart 2021-01-14 09:46:43 UTC
Created attachment 115134 [details] [review]
Bug 15448: Remove items fetch

We fetch them already too many times.
Comment 38 Jonathan Druart 2021-01-14 13:04:25 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 39 Fridolin Somers 2021-01-18 15:12:39 UTC
Depends on Bug 24254 not in 20.11.x
Comment 40 Katrin Fischer 2021-01-18 22:24:41 UTC
(In reply to Fridolin Somers from comment #39)
> Depends on Bug 24254 not in 20.11.x

Hi Frido, could you consider backporting bug 24254? 20.11.x is still young and it's the base for fixing quit evil bugs like this one.
Comment 41 Fridolin Somers 2021-01-19 07:42:38 UTC
(In reply to Katrin Fischer from comment #40)
> (In reply to Fridolin Somers from comment #39)
> > Depends on Bug 24254 not in 20.11.x
> 
> Hi Frido, could you consider backporting bug 24254? 20.11.x is still young
> and it's the base for fixing quit evil bugs like this one.

Oh yes indeed, I had mistaken this one for an enhancement.
I'm on it
Comment 42 Fridolin Somers 2021-01-19 08:19:44 UTC
Pushed to 20.11.x for 20.11.02

Major so I backported Bug 24254 to be able to backport this one.
Comment 43 Katrin Fischer 2021-01-19 09:42:08 UTC
(In reply to Fridolin Somers from comment #42)
> Pushed to 20.11.x for 20.11.02
> 
> Major so I backported Bug 24254 to be able to backport this one.

Thank you :)
Comment 44 Andrew Fuerste-Henry 2021-01-29 17:49:45 UTC
Bug 24254, on which this depends, does not apply cleanly to 20.05. Please clear that if this is needed in 20.05. Thanks!
Comment 45 Tomás Cohen Arazi 2021-01-29 18:36:48 UTC
(In reply to Andrew Fuerste-Henry from comment #44)
> Bug 24254, on which this depends, does not apply cleanly to 20.05. Please
> clear that if this is needed in 20.05. Thanks!

Done!
Comment 46 Andrew Fuerste-Henry 2021-01-29 19:11:31 UTC
This doesn't work as intended and I'm not clear on whether the issue is here or on bug 24254. This blocks holds on any item that doesn't have a value that doesn't match what's in OPACHiddenItems. So, if I set OPACHiddenItems to "copynumber: [2]," then I'm blocked from placing holds on any item with copynubmer=2 AND any item with copynumber is NULL.

This applies to both 20.05 and master.
Comment 47 Tomás Cohen Arazi 2021-01-29 19:14:37 UTC
(In reply to Andrew Fuerste-Henry from comment #46)
> This doesn't work as intended and I'm not clear on whether the issue is here
> or on bug 24254. This blocks holds on any item that doesn't have a value
> that doesn't match what's in OPACHiddenItems. So, if I set OPACHiddenItems
> to "copynumber: [2]," then I'm blocked from placing holds on any item with
> copynubmer=2 AND any item with copynumber is NULL.
> 
> This applies to both 20.05 and master.

Good catch, can you file a separate bug and I'll take a look now?
Comment 48 Andrew Fuerste-Henry 2021-01-29 21:24:08 UTC
Pushed to 20.05.x for 20.05.09

Works as intended so long as it doesn't encounter any NULLs. That issue comes from bug 24254 and is corrected in bug 27580.
Comment 49 Victor Grousset/tuxayo 2021-02-01 23:14:48 UTC
Backported: Pushed to 19.11.x branch for 19.11.15