Bug 28299 - OpacHiddenItems not working in OPAC lists
Summary: OpacHiddenItems not working in OPAC lists
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 18989
Blocks: 24403
  Show dependency treegraph
 
Reported: 2021-05-07 09:15 UTC by Jonathan Druart
Modified: 2022-08-01 14:05 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes an issue where items that should be hidden from display in the OPAC (using the rules in OpacHiddenItems, for example: damaged) were displayed under availability in OPAC lists.
Version(s) released in:
21.11.00,21.05.02


Attachments
Bug 28299: Take OpacHiddenItems into account on opac-shelves (7.84 KB, patch)
2021-05-07 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items (3.73 KB, patch)
2021-05-10 08:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28299: Make buildKohaItemsNamespace accept a Koha::Items resultset (4.12 KB, patch)
2021-05-10 12:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items (5.56 KB, patch)
2021-05-10 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28299: Unit tests for buildKohaItemsNamespace (3.86 KB, patch)
2021-05-10 14:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28299: Unit tests for buildKohaItemsNamespace (3.87 KB, patch)
2021-05-16 03:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28299: Take OpacHiddenItems into account on opac-shelves (8.40 KB, patch)
2021-05-16 22:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items (5.61 KB, patch)
2021-05-16 22:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28299: Unit tests for buildKohaItemsNamespace (3.87 KB, patch)
2021-05-16 22:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28299: Unit tests for buildKohaItemsNamespace (4.82 KB, patch)
2021-06-16 20:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28299: Take OpacHiddenItems into account on opac-shelves (8.40 KB, patch)
2021-06-16 20:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items (5.61 KB, patch)
2021-06-16 20:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28299: Unit tests for buildKohaItemsNamespace (4.87 KB, patch)
2021-06-20 19:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 28299: Take OpacHiddenItems into account on opac-shelves (8.45 KB, patch)
2021-06-20 19:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items (5.66 KB, patch)
2021-06-20 19:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 28299: Unit tests for buildKohaItemsNamespace (4.93 KB, patch)
2021-07-05 20:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28299: Take OpacHiddenItems into account on opac-shelves (8.52 KB, patch)
2021-07-05 20:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items (5.72 KB, patch)
2021-07-05 20:57 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-05-07 09:15:13 UTC

    
Comment 1 Jonathan Druart 2021-05-07 09:18:35 UTC
Created attachment 120662 [details] [review]
Bug 28299: Take OpacHiddenItems into account on opac-shelves

hidden_items was not passed to XSLTParse4Display

2 things:
* Should we hide the biblio record if OpacHiddenItemsHidesRecord is set?
* allow_onshelf_holds is not working like in other scripts, what's the
expected behaviour? If hidden should we completely ignore the item?
Comment 2 Tomás Cohen Arazi 2021-05-07 13:42:23 UTC
I expect to see something like:

    my $items = $biblio
                  ->items
                  ->filter_by_visible_in_opac({ patron => $patron });

in the controller.

I see why it is written like this (buildKohaItemsNamespace makes a new query on items filtering the hidden ones out). Should we (temporarily) add an extra parameters $only_this_items and pass this calculated value instead?
Comment 3 Jonathan Druart 2021-05-10 08:01:23 UTC
Created attachment 120756 [details] [review]
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items
Comment 4 Jonathan Druart 2021-05-10 08:01:45 UTC
Something like that, Tomas?
Comment 5 Tomás Cohen Arazi 2021-05-10 12:11:51 UTC
Created attachment 120769 [details] [review]
Bug 28299: Make buildKohaItemsNamespace accept a Koha::Items resultset

This patch makes buildKohaItemsNamespace (and its only caller,
XSLTParse4Display) accept a an optional Koha::Items resultset.

This way we don't need to calculate (from the DB) the list of hidden
itemnumbers, but solve it in one query.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2021-05-10 13:35:56 UTC
Created attachment 120792 [details] [review]
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items

This patch makes buildKohaItemsNamespace (and its only caller,
XSLTParse4Display) accept a an optional Koha::Items resultset.

This way we don't need to calculate (from the DB) the list of hidden
itemnumbers, but solve it in one query.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2021-05-10 14:18:23 UTC
Created attachment 120795 [details] [review]
Bug 28299: Unit tests for buildKohaItemsNamespace

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2021-05-10 14:40:33 UTC
To everyone looking at this patches:

(In reply to Jonathan Druart from comment #1)
> 2 things:
> * Should we hide the biblio record if OpacHiddenItemsHidesRecord is set?
> * allow_onshelf_holds is not working like in other scripts, what's the
> expected behaviour? If hidden should we completely ignore the item?

^^ this questions are still valid
Comment 9 Katrin Fischer 2021-05-10 15:14:26 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> To everyone looking at this patches:
> 
> (In reply to Jonathan Druart from comment #1)
> > 2 things:
> > * Should we hide the biblio record if OpacHiddenItemsHidesRecord is set?

Yes, definitely. The records should not show up anywhere in the OPAC, if they are hidden by OpacHiddenItemsHidesRecord.

> > * allow_onshelf_holds is not working like in other scripts, what's the
> > expected behaviour? If hidden should we completely ignore the item?

Not sure where the holds come from here :) Is this the use case?

* One or more hidden items could fill a hold
* Should we be able to place the hold, although all visible items cannot fill a hold.

For item level holds this is easy: We don't want to see the items, so no item level holds can be placed.

For the record level holds... it's harder. My feeling is that if you don't want your users to see something in the catalog, it's unlikely that you would want to count them towards holds. But maybe in that case the library should just take extra configuration steps to prevent this (like using a not-for-loan status on the items?
 
> ^^ this questions are still valid
Comment 10 Victor Grousset/tuxayo 2021-05-16 03:49:11 UTC
Created attachment 121018 [details] [review]
Bug 28299: Unit tests for buildKohaItemsNamespace

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Victor Grousset/tuxayo 2021-05-16 05:48:29 UTC
Solved minor conflict on the test patch.

However tests pass even with the patch «Make buildKohaItemsNamespace accept Koha::Items»
reverted and even with the other one reverted.

So that means they don't test the change.
Comment 12 Victor Grousset/tuxayo 2021-05-16 22:53:21 UTC
Created attachment 121021 [details] [review]
Bug 28299: Take OpacHiddenItems into account on opac-shelves

hidden_items was not passed to XSLTParse4Display

2 things:
* Should we hide the biblio record if OpacHiddenItemsHidesRecord is set?
* allow_onshelf_holds is not working like in other scripts, what's the
expected behaviour? If hidden should we completely ignore the item?

Test plan:
1. Without the patch
2. fill OpacHiddenItems with «damaged: [1]»
3. find a record with several item
4. mark one of them as damaged
5. OPAC: go to the record, you shouldn't see the damaged item
6. add the record to a list
7. see the list
8. in Availability you see the damaged item mentioned, this is the issue
9. apply the patches and restart the services
10. the list should not mention the damaged item :D

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 13 Victor Grousset/tuxayo 2021-05-16 22:53:25 UTC
Created attachment 121022 [details] [review]
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items

This patch makes buildKohaItemsNamespace (and its only caller,
XSLTParse4Display) accept a an optional Koha::Items resultset.

This way we don't need to calculate (from the DB) the list of hidden
itemnumbers, but solve it in one query.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 14 Victor Grousset/tuxayo 2021-05-16 22:53:29 UTC
Created attachment 121023 [details] [review]
Bug 28299: Unit tests for buildKohaItemsNamespace

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Victor Grousset/tuxayo 2021-05-16 22:54:54 UTC
It work :D

Didn't signed-off the test patch due to comment 11

Added test plan. Is that enough? I didn't check if the second patch doesn't something else.
Comment 16 Tomás Cohen Arazi 2021-06-16 20:00:19 UTC
Created attachment 122050 [details] [review]
Bug 28299: Unit tests for buildKohaItemsNamespace

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi 2021-06-16 20:00:24 UTC
Created attachment 122051 [details] [review]
Bug 28299: Take OpacHiddenItems into account on opac-shelves

hidden_items was not passed to XSLTParse4Display

2 things:
* Should we hide the biblio record if OpacHiddenItemsHidesRecord is set?
* allow_onshelf_holds is not working like in other scripts, what's the
expected behaviour? If hidden should we completely ignore the item?

Test plan:
1. Without the patch
2. fill OpacHiddenItems with «damaged: [1]»
3. find a record with several item
4. mark one of them as damaged
5. OPAC: go to the record, you shouldn't see the damaged item
6. add the record to a list
7. see the list
8. in Availability you see the damaged item mentioned, this is the issue
9. apply the patches and restart the services
10. the list should not mention the damaged item :D

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 18 Tomás Cohen Arazi 2021-06-16 20:00:28 UTC
Created attachment 122052 [details] [review]
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items

This patch makes buildKohaItemsNamespace (and its only caller,
XSLTParse4Display) accept a an optional Koha::Items resultset.

This way we don't need to calculate (from the DB) the list of hidden
itemnumbers, but solve it in one query.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Tomás Cohen Arazi 2021-06-16 20:03:15 UTC
(In reply to Victor Grousset/tuxayo from comment #11)
> Solved minor conflict on the test patch.
> 
> However tests pass even with the patch «Make buildKohaItemsNamespace accept
> Koha::Items»
> reverted and even with the other one reverted.
> 
> So that means they don't test the change.

Sorry for that. The tests only tested the trivial case in which all items for the biblio are passed in the resultset. I now added one more item, that is excluded from the resultset, to better highlight the interaction between both params.

Thanks!
Comment 20 David Nind 2021-06-20 19:32:45 UTC
Created attachment 122196 [details] [review]
Bug 28299: Unit tests for buildKohaItemsNamespace

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 David Nind 2021-06-20 19:32:50 UTC
Created attachment 122197 [details] [review]
Bug 28299: Take OpacHiddenItems into account on opac-shelves

hidden_items was not passed to XSLTParse4Display

2 things:
* Should we hide the biblio record if OpacHiddenItemsHidesRecord is set?
* allow_onshelf_holds is not working like in other scripts, what's the
expected behaviour? If hidden should we completely ignore the item?

Test plan:
1. Without the patch
2. fill OpacHiddenItems with «damaged: [1]»
3. find a record with several item
4. mark one of them as damaged
5. OPAC: go to the record, you shouldn't see the damaged item
6. add the record to a list
7. see the list
8. in Availability you see the damaged item mentioned, this is the issue
9. apply the patches and restart the services
10. the list should not mention the damaged item :D

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2021-06-20 19:32:55 UTC
Created attachment 122198 [details] [review]
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items

This patch makes buildKohaItemsNamespace (and its only caller,
XSLTParse4Display) accept a an optional Koha::Items resultset.

This way we don't need to calculate (from the DB) the list of hidden
itemnumbers, but solve it in one query.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: David Nind <david@davidnind.com>
Comment 23 Katrin Fischer 2021-07-05 20:57:40 UTC
Created attachment 122593 [details] [review]
Bug 28299: Unit tests for buildKohaItemsNamespace

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2021-07-05 20:57:45 UTC
Created attachment 122594 [details] [review]
Bug 28299: Take OpacHiddenItems into account on opac-shelves

hidden_items was not passed to XSLTParse4Display

2 things:
* Should we hide the biblio record if OpacHiddenItemsHidesRecord is set?
* allow_onshelf_holds is not working like in other scripts, what's the
expected behaviour? If hidden should we completely ignore the item?

Test plan:
1. Without the patch
2. fill OpacHiddenItems with «damaged: [1]»
3. find a record with several item
4. mark one of them as damaged
5. OPAC: go to the record, you shouldn't see the damaged item
6. add the record to a list
7. see the list
8. in Availability you see the damaged item mentioned, this is the issue
9. apply the patches and restart the services
10. the list should not mention the damaged item :D

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2021-07-05 20:57:50 UTC
Created attachment 122595 [details] [review]
Bug 28299: Make buildKohaItemsNamespace accept Koha::Items

This patch makes buildKohaItemsNamespace (and its only caller,
XSLTParse4Display) accept a an optional Koha::Items resultset.

This way we don't need to calculate (from the DB) the list of hidden
itemnumbers, but solve it in one query.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Jonathan Druart 2021-07-06 09:03:08 UTC
Too bad I hadn't look at this one more closely before I wrote bug 12561. Most of the controllers changes will need to be rewritten.

Nice work, Tomas!
Comment 27 Jonathan Druart 2021-07-06 09:05:42 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 28 Kyle M Hall 2021-07-09 17:49:59 UTC
Pushed to 21.05.x for 21.05.02
Comment 29 Fridolin Somers 2021-08-02 20:39:32 UTC
Depends on Bug 18989 not in 20.11.x
Comment 30 Tomás Cohen Arazi 2021-10-13 15:06:04 UTC
*** Bug 17921 has been marked as a duplicate of this bug. ***