Bug 28299 - OpacHiddenItems not working on opac-shelves
Summary: OpacHiddenItems not working on opac-shelves
Status: Failed QA
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: 2021-05-16 22:54 UTC (History)
6 users (show)

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


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

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.