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?
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?
Created attachment 120756 [details] [review] Bug 28299: Make buildKohaItemsNamespace accept Koha::Items
Something like that, Tomas?
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>
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>
Created attachment 120795 [details] [review] Bug 28299: Unit tests for buildKohaItemsNamespace Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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
(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
Created attachment 121018 [details] [review] Bug 28299: Unit tests for buildKohaItemsNamespace Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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.
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>
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>
Created attachment 121023 [details] [review] Bug 28299: Unit tests for buildKohaItemsNamespace Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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.
Created attachment 122050 [details] [review] Bug 28299: Unit tests for buildKohaItemsNamespace Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
(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!
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>
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>
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>
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>
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>
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>
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!
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.02
Depends on Bug 18989 not in 20.11.x
*** Bug 17921 has been marked as a duplicate of this bug. ***