SearchSuggestion does filtering on IndependentBranches, etc. We need to implement Koha::Suggestions->search_limited that does that as well.
Created attachment 129508 [details] [review] Bug 29886: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129509 [details] [review] Bug 29886: Add Koha::Suggestions->search_limited Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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!
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?
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
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.
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)
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.
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>
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>
Created attachment 130296 [details] [review] Bug 29886: (QA follow-up)
Oops, didn't mean to upload that last one.. deprecating.
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
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.04
Pushed to 21.05.x for 21.05.13
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.