Summary: | Patron searches can fail when library groups are set to 'Limit patron data access by group' | ||
---|---|---|---|
Product: | Koha | Reporter: | Lucas Gass (lukeg) <lucas> |
Component: | Patrons | Assignee: | Nick Clemens (kidclamp) <nick> |
Status: | Pushed to oldstable --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | critical | ||
Priority: | P5 - low | CC: | aspencatteam, fridolin.somers, gmcharlt, holly, kyle.m.hall, martin.renvoize, nick, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: |
24.11.00,24.05.04,23.11.09
|
Circulation function: | |
Bug Depends on: | 29523 | ||
Bug Blocks: | 37392 | ||
Attachments: |
Bug 37378: Unit test
Bug 37378: libraries_where_can_see_things should always return an array Bug 37378: Tidy Bug 37378: Unit test Bug 37378: libraries_where_can_see_things should always return an array Bug 37378: Tidy Bug 37378: Unit test Bug 37378: libraries_where_can_see_things should always return an array Bug 37378: Tidy |
Description
Lucas Gass (lukeg)
2024-07-16 18:08:46 UTC
We can't operate in our consortial setting the way we need to until this fixed. We need the ability to not view patrons from other branches. - Bob Bennhoff Bug 37368 has a note that makes me think it might be related - even a fix maybe? This patch adds the missing library embed from the patron search forms. This embed is required for the case where a patron is missing both first and last name or is from a different library group and thus falls into the 'Patron from library X' block. Created attachment 169411 [details] [review] Bug 37378: Unit test Created attachment 169412 [details] [review] Bug 37378: libraries_where_can_see_things should always return an array The subroutine libraries_where_can_see_things stores the list of libraries that things can be viewed from in an internal variable, so we can return this directly if we have already calculated. When returning if not cached, we dereference the list and return an array. If cached, we are returning an arrayref. This patch simply ensures we dereference the array even if already cached. Before this patch, we were fetching the patrons, then redacting all info as their branches didn't match against an arrayref, rather than checking against each branch we are allowed to view. To test: 1. Setup a library group and check the "Limit patron data access by group ." option. 2. Add some libraries to the group. ( IN k-t-d I added CPL and MPL ) 3. Create a staff account who has staff access permissions and all of the borrower permissions except "view_borrower_infos_from_any_libraries" 4. Set the home library of that staff member to one of the branches in step 2. ( In my test I choose MPL ) 5. Log in as that patron and attempt a patron search that would include users from either library in step 2. 6. See the error: Something went wrong when loading the table. 500: Internal Server Error. Expected boolean - got null. Expected boolean - got null. Expected string - got null. Expected string - got null. Expected string - got null. Expected integer - got null. Expected integer - got null. Expected integer - got null. Expected boolean - got null. Expected boolean - got null. Expected string - got null. 7. Apply patch, restart all 8. Search again, you can see the expected patrons Created attachment 169413 [details] [review] Bug 37378: Tidy (In reply to Katrin Fischer from comment #2) > Bug 37368 has a note that makes me think it might be related - even a fix > maybe? > > This patch adds the missing library embed from the patron search forms. > This embed is required for the case where a patron is missing both first and > last name or is from a different library group and thus falls into the > 'Patron from library X' block. Nope, this is unrelated. We limit the search by libraries that can be seen, then redact any patrons we can't see - that redaction breaks the api response as it blanks fields that should not be null We don't need to redact in this case, because we already limited to visible libraries, we just didn't get the list correctly Created attachment 169430 [details] [review] Bug 37378: Unit test Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Created attachment 169431 [details] [review] Bug 37378: libraries_where_can_see_things should always return an array The subroutine libraries_where_can_see_things stores the list of libraries that things can be viewed from in an internal variable, so we can return this directly if we have already calculated. When returning if not cached, we dereference the list and return an array. If cached, we are returning an arrayref. This patch simply ensures we dereference the array even if already cached. Before this patch, we were fetching the patrons, then redacting all info as their branches didn't match against an arrayref, rather than checking against each branch we are allowed to view. To test: 1. Setup a library group and check the "Limit patron data access by group ." option. 2. Add some libraries to the group. ( IN k-t-d I added CPL and MPL ) 3. Create a staff account who has staff access permissions and all of the borrower permissions except "view_borrower_infos_from_any_libraries" 4. Set the home library of that staff member to one of the branches in step 2. ( In my test I choose MPL ) 5. Log in as that patron and attempt a patron search that would include users from either library in step 2. 6. See the error: Something went wrong when loading the table. 500: Internal Server Error. Expected boolean - got null. Expected boolean - got null. Expected string - got null. Expected string - got null. Expected string - got null. Expected integer - got null. Expected integer - got null. Expected integer - got null. Expected boolean - got null. Expected boolean - got null. Expected string - got null. 7. Apply patch, restart all 8. Search again, you can see the expected patrons Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Created attachment 169432 [details] [review] Bug 37378: Tidy Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Created attachment 169711 [details] [review] Bug 37378: Unit test Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 169713 [details] [review] Bug 37378: libraries_where_can_see_things should always return an array The subroutine libraries_where_can_see_things stores the list of libraries that things can be viewed from in an internal variable, so we can return this directly if we have already calculated. When returning if not cached, we dereference the list and return an array. If cached, we are returning an arrayref. This patch simply ensures we dereference the array even if already cached. Before this patch, we were fetching the patrons, then redacting all info as their branches didn't match against an arrayref, rather than checking against each branch we are allowed to view. To test: 1. Setup a library group and check the "Limit patron data access by group ." option. 2. Add some libraries to the group. ( IN k-t-d I added CPL and MPL ) 3. Create a staff account who has staff access permissions and all of the borrower permissions except "view_borrower_infos_from_any_libraries" 4. Set the home library of that staff member to one of the branches in step 2. ( In my test I choose MPL ) 5. Log in as that patron and attempt a patron search that would include users from either library in step 2. 6. See the error: Something went wrong when loading the table. 500: Internal Server Error. Expected boolean - got null. Expected boolean - got null. Expected string - got null. Expected string - got null. Expected string - got null. Expected integer - got null. Expected integer - got null. Expected integer - got null. Expected boolean - got null. Expected boolean - got null. Expected string - got null. 7. Apply patch, restart all 8. Search again, you can see the expected patrons Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 169714 [details] [review] Bug 37378: Tidy Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks Nick.. Passing QA Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant Backported to 24.05.x for upcoming 24.05.04 Pushed to 23.11.x for 23.11.09 Not backporting to 23.05.x unless requested |