Bug 29886 - Add Koha::Suggestions->search_limited
Summary: Add Koha::Suggestions->search_limited
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 23991
  Show dependency treegraph
 
Reported: 2022-01-14 15:59 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.04,21.05.13


Attachments
Bug 29886: Unit tests (3.40 KB, patch)
2022-01-14 17:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29886: Add Koha::Suggestions->search_limited (1.69 KB, patch)
2022-01-14 17:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29886: Unit tests (3.45 KB, patch)
2022-01-21 18:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 29886: Add Koha::Suggestions->search_limited (1.74 KB, patch)
2022-01-21 18:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 29886: Unit tests (3.51 KB, patch)
2022-02-08 15:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29886: Add Koha::Suggestions->search_limited (1.80 KB, patch)
2022-02-08 15:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29886: (QA follow-up) (1.71 KB, patch)
2022-02-08 15:24 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-01-14 15:59:55 UTC
SearchSuggestion does filtering on IndependentBranches, etc. We need to implement Koha::Suggestions->search_limited that does that as well.
Comment 1 Tomás Cohen Arazi 2022-01-14 17:52:26 UTC
Created attachment 129508 [details] [review]
Bug 29886: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2022-01-14 17:52:51 UTC
Created attachment 129509 [details] [review]
Bug 29886: Add Koha::Suggestions->search_limited

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Nind 2022-01-21 18:47:48 UTC
Created attachment 129709 [details] [review]
Bug 29886: Unit tests

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2022-01-21 18:47:53 UTC
Created attachment 129710 [details] [review]
Bug 29886: Add Koha::Suggestions->search_limited

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2022-01-21 18:49:32 UTC
Test plan used:

1. Create some suggestions for different libraries using the OPAC.
2. Log in as a staff user without super librarian privileges and view the suggestions - you should see suggestions for all libraries.
3. Enable the IndependentBranches system preference.
4. Repeat step 2 - you should only see suggestions for the staff user's library.
5. Run the tests - these should all pass: prove t/db_dependent/Koha/Suggestions.t
6. Apply the patch.
7. Repeat step 2-5 - there should be no change in behaviour, and things should work as expected.
8. Sign-off!
Comment 6 Martin Renvoize 2022-02-03 15:34:35 UTC
This is the third Koha::PluralClass::search_limited method we've introduced recently.. and they're all nearly the same.

Should we not DRY this code out and adopt something similar to Koha::Objects::Limit::Library here?
Comment 7 Martin Renvoize 2022-02-03 16:06:33 UTC
Hmm.. this is very different to our other search_limited methods..

Instead of using the Koha::Patron->libraries_where_can_see_patrons we're doing some special cases IndependantBranches stuff..   I'm wondering if we ought to be using the existing limitation method
Comment 8 Martin Renvoize 2022-02-03 16:08:49 UTC
The 'C4::Context::only_my_library' call in the other search_limited form should catch the same IndependantBranches stuff as this does..

Given we have good tests written here, I'll try to adapt it and use the tests to prove the route.
Comment 9 Martin Renvoize 2022-02-03 16:23:39 UTC
Ack, more confusion here..

We are limiting by Suggestion branch.. which I think makes sense.. but could expose more than we're expecting (we could perhaps limit on suggester.branchcode to limit to only suggestions made by users from the branches you are allowed to see).

ArticleRequests takes the above approach and limits by the requester.branchcode.. (but this could also be seen as a possible bug.. as you may want to see all article requests at the current branch.. regardless of who requested them and where they're based)
Comment 10 Tomás Cohen Arazi 2022-02-07 17:26:49 UTC
Those limits are inherited from C4::Suggestions::SearchSuggestion, which is to be removed on bug 23991. I stripped this from that bug report in order to backport it to 21.11, which implements /suggestions routes that really need this filter.

If the logic is not correct, then it is not correct in Koha already. And we should probably discuss it in a follow-up bug.
Comment 11 Martin Renvoize 2022-02-08 15:24:35 UTC
Created attachment 130294 [details] [review]
Bug 29886: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2022-02-08 15:24:40 UTC
Created attachment 130295 [details] [review]
Bug 29886: Add Koha::Suggestions->search_limited

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2022-02-08 15:24:44 UTC
Created attachment 130296 [details] [review]
Bug 29886: (QA follow-up)
Comment 14 Martin Renvoize 2022-02-08 15:25:17 UTC
Oops, didn't mean to upload that last one.. deprecating.
Comment 15 Martin Renvoize 2022-02-08 15:27:41 UTC
Right.. I decided to PQA this as it stands.

It makes sense that we limit only by IndependantBranches, though I'd love to see it moved to utilise the library groups feature in the future.

As for the different style of search_limited, it also makes sense here.. we're limiting the object by it's branch and the current users permission to see that branch.. it has nothing to do with the patrons attached to the suggestion (that should be getting handled at another level)

I will open a bug to discuss how I believe the same method in ArticleRequests is wrong on this matter.

So.. Passing QA
Comment 16 Fridolin Somers 2022-03-04 02:12:15 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 17 Kyle M Hall 2022-03-04 14:24:50 UTC
Pushed to 21.11.x for 21.11.04
Comment 18 Andrew Fuerste-Henry 2022-03-20 15:36:25 UTC
Pushed to 21.05.x for 21.05.13
Comment 19 Victor Grousset/tuxayo 2022-03-21 00:18:09 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.