Bug 34147 - Patron search displays "processing" when category has library limitations that exclude the logged in library name
Summary: Patron search displays "processing" when category has library limitations tha...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Nick Clemens (kidclamp)
QA Contact: Emily Lamancusa (emlam)
URL:
Keywords:
: 37379 (view as bug list)
Depends on:
Blocks: 38444
  Show dependency treegraph
 
Reported: 2023-06-28 16:00 UTC by John Yorio
Modified: 2024-11-18 09:25 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.04
Circulation function:


Attachments
Bug 34147: Prevent crashing of patron search (1.49 KB, patch)
2024-07-17 08:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34147: Load all categories for displaying patron results (2.02 KB, patch)
2024-07-23 12:55 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34147: Load all categories for displaying patron results (2.07 KB, patch)
2024-07-23 13:36 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 34147: Load all categories for displaying patron results (2.14 KB, patch)
2024-07-25 14:30 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description John Yorio 2023-06-28 16:00:16 UTC
I'll start by saying that I don't know if this relates to https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24132 or not.

To replicate, you'll need more than one library in Administration | Libraries.

Setup a patron category with a Library Limitation, so that one library is excluded (by not selecting it).

Create a patron with a common surname in your borrowers table and assign to that category.

While the logged in branch is set to one of the libraries that are not excluded for that category you created above, search for patrons with the surname of your test patron.

You should get results as expected.

Change the logged in branch name to the excluded library. Conduct the same search. A processing message appears and just spins.

I'm not sure whether the desired result should be to exclude the patron in question from the search results or whether it should display but with some restriction on the Patron Category.
Comment 1 Jonathan Druart 2023-07-05 10:50:20 UTC
Still valid in master? I don't recreate.

If you do can you provide what you have in the console and eventually the Koha log if you got a 500?
Comment 2 Jonathan Druart 2023-07-05 10:51:32 UTC
Is this a duplicate of bug 34145?
Comment 3 Jason Robb 2024-07-16 14:17:12 UTC
I was able to replicate this in a sandbox with these steps:

1. Make sure you are logged in as Centerville
2. Do a patron search for 'Ac' -- there are two patrons: Henry (Staff) and Edna (Patron)
2. Go to Administration > Patron Categories
3. Edit the Patron (PT) category, set the Library limitations to Fairfield only
4. Do the patron search for 'Ac' again - it hangs up with a Processing box

This happens when logged in with the superlibrarian account, and when logged in with an account with view_borrower_infos_from_any_library enabled.

The console shows this error:

Uncaught TypeError: Cannot read properties of undefined (reading 'description')
    at render (member.pl?quicksearch=1&searchmember=Ac&searchfieldstype=standard&branchcode_filter=&categorycode_filter=:3141:90)
    at datatables.min_24.0600014.js:41:12136
    at e.fnGetData (datatables.min_24.0600014.js:41:15516)
    at S (datatables.min_24.0600014.js:41:18049)
    at St (datatables.min_24.0600014.js:41:20531)
    at y (datatables.min_24.0600014.js:41:22994)
    at It (datatables.min_24.0600014.js:41:28350)
    at datatables.min_24.0600014.js:41:26897
    at Object.e [as success] (datatables.min_24.0600014.js:41:25877)
    at c (jquery-3.6.0.min_24.0600014.js:2:28327)
Comment 4 Lucas Gass (lukeg) 2024-07-16 22:04:30 UTC
*** Bug 37379 has been marked as a duplicate of this bug. ***
Comment 5 Pedro Amorim 2024-07-17 08:41:10 UTC
Created attachment 169073 [details] [review]
Bug 34147: Prevent crashing of patron search

This is just working around the problem, I think.
It fixes the crashing issue but the limited patron's category is no longer listed on the table.
I dont fully understand the 'library limitation' functionality, what is the expected functionality here when following the test plan?
Is 'Acosta, Edna' expected to not show up? Since its PT category?
Comment 6 Lucas Gass (lukeg) 2024-07-17 16:17:02 UTC
> Is 'Acosta, Edna' expected to not show up? Since its PT category?

I think yes. If there is a library limitation and your are logged into library not included in that limitation you should see no search results for the given patron category.
Comment 7 Jason Robb 2024-07-17 16:18:31 UTC
Historically patron category library limitations only restrict the creation of patrons to libraries for which the limits are set and do not affect patron search results.

In the example, I definitely do still want to see Edna show up. As a consortium with a shared patron database that limits different categories for different branches, we need to be able to see all possible patrons when searching to avoid duplicates, and so patrons from one library can be found when they walk into another library.

The patch seems to work well other than what Pedro mentioned. When following the steps to replicate, the table cell in the "Category" column for Edna Acosta is blank where it should say "Patron".
Comment 8 AspenCat Team 2024-07-18 16:27:48 UTC
Our consortia does not share a patron database.  We use the patron category branch limitation to force libraries to only select patron categories that they use regularly.  This isn't the tool we use to prevent one branch from seeing another branches patrons.  For that we use a permission called "view_borrower_infos_from_any_libraries" and when that is not selected it, it prevents one branch from seeing another branches patrons, even if the share a patron category.  This being set is also causing a similar failing in searching as described in bug 37378 - Bob Bennhoff
Comment 9 Katrin Fischer 2024-07-18 16:33:53 UTC
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.
Comment 10 Nick Clemens (kidclamp) 2024-07-23 12:55:35 UTC
Created attachment 169417 [details] [review]
Bug 34147: Load all categories for displaying patron results

Currently we are only loading unlimited categories into the patron search, so when we return
patrons from a category limited to another library we crash because we don't have the descriptions
loaded.

This patch removes the limit from 'categories_map' which is only used for displaying patrons. As patron
visibility is a feature of permissions and groups, I don't think we should enforce if based on category
limitations.

NOTE: the categories for searching are still limited in the left hand filters - but not in the dropdown if you click the options button in the top search form. This is beyond the scope of this bug.

To test:
1. Make sure you are logged in as Centerville
2. Do a patron search for 'Ac' -- there are two patrons: Henry (Staff) and Edna (Patron)
2. Go to Administration > Patron Categories
3. Edit the Patron (PT) category, set the Library limitations to Fairfield only
4. Do the patron search for 'Ac' again - it hangs up with a Processing box
5. Apply patch
6. Reload the page (clear cache) and confirm the results now load
Comment 11 Nick Clemens (kidclamp) 2024-07-23 12:57:27 UTC
(In reply to Katrin Fischer from comment #9)
> 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.

Unfortunately no, this bug is related to the fact that we are not returning the descriptions of limited categories, but returning those patrons. I think allowing all categories to be viewed is correct. As Jason points out, the limits are to restrict the categories used when adding patrons, not to enforce visibility of patrons
Comment 12 ByWater Sandboxes 2024-07-23 13:36:49 UTC
Created attachment 169421 [details] [review]
Bug 34147: Load all categories for displaying patron results

Currently we are only loading unlimited categories into the patron search, so when we return
patrons from a category limited to another library we crash because we don't have the descriptions
loaded.

This patch removes the limit from 'categories_map' which is only used for displaying patrons. As patron
visibility is a feature of permissions and groups, I don't think we should enforce if based on category
limitations.

NOTE: the categories for searching are still limited in the left hand filters - but not in the dropdown if you click the options button in the top search form. This is beyond the scope of this bug.

To test:
1. Make sure you are logged in as Centerville
2. Do a patron search for 'Ac' -- there are two patrons: Henry (Staff) and Edna (Patron)
2. Go to Administration > Patron Categories
3. Edit the Patron (PT) category, set the Library limitations to Fairfield only
4. Do the patron search for 'Ac' again - it hangs up with a Processing box
5. Apply patch
6. Reload the page (clear cache) and confirm the results now load

Signed-off-by: Jason Robb <jrobb@sekls.org>
Comment 13 Emily Lamancusa (emlam) 2024-07-25 14:30:14 UTC
Created attachment 169559 [details] [review]
Bug 34147: Load all categories for displaying patron results

Currently we are only loading unlimited categories into the patron search, so when we return
patrons from a category limited to another library we crash because we don't have the descriptions
loaded.

This patch removes the limit from 'categories_map' which is only used for displaying patrons. As patron
visibility is a feature of permissions and groups, I don't think we should enforce if based on category
limitations.

NOTE: the categories for searching are still limited in the left hand filters - but not in the dropdown if you click the options button in the top search form. This is beyond the scope of this bug.

To test:
1. Make sure you are logged in as Centerville
2. Do a patron search for 'Ac' -- there are two patrons: Henry (Staff) and Edna (Patron)
2. Go to Administration > Patron Categories
3. Edit the Patron (PT) category, set the Library limitations to Fairfield only
4. Do the patron search for 'Ac' again - it hangs up with a Processing box
5. Apply patch
6. Reload the page (clear cache) and confirm the results now load

Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 14 Martin Renvoize (ashimema) 2024-07-26 12:55:47 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant
Comment 15 Lucas Gass (lukeg) 2024-08-29 14:30:03 UTC
Backported to 24.05.x for upcoming 24,05.04
Comment 16 Jonathan Druart 2024-09-03 09:33:30 UTC
Please provide tests when you are modifying the patrons search code. We have a selenium tests for that (t/db_dependent/selenium/patrons_search.t).
Comment 17 Jonathan Druart 2024-11-18 09:12:13 UTC
> As patron visibility is a feature of permissions and groups, I don't think we should enforce if based on category limitations.

> NOTE: the categories for searching are still limited in the left hand filters - but not in the dropdown if you click the options button in the top search form. This is beyond the scope of this bug.

Then what? We need to follow-up here.

Is there a consensus on the desired behaviour? IIRC the original goal of this "library limitation" at category level was to completely restrict the access. Since we now have the library groups, does it mean we should get rid of it?
Comment 18 Katrin Fischer 2024-11-18 09:25:09 UTC
(In reply to Jonathan Druart from comment #17)
> > As patron visibility is a feature of permissions and groups, I don't think we should enforce if based on category limitations.
> 
> > NOTE: the categories for searching are still limited in the left hand filters - but not in the dropdown if you click the options button in the top search form. This is beyond the scope of this bug.
> 
> Then what? We need to follow-up here.
> 
> Is there a consensus on the desired behaviour? IIRC the original goal of
> this "library limitation" at category level was to completely restrict the
> access. Since we now have the library groups, does it mean we should get rid
> of it?

I am not sure if the assumption is correct about the original goal. I think it's more: you cannot "use" the category as a restricted library. This means you cannot add patrons with this category for your library. The whole purpose of the feature is not always clear in all areas.

But we already had the issue where libraries can see and edit patrons of other libraries and had to work around that to avoid unintentional data loss on editing these patrons. 

So for the "Add" buttons we should definitely enforce library limitations.
For the pull down in search I think it's also helpful, but more for usability - the library restrictions can be used to "shorten" the pull downs to what you actually use.
But if a datatable has patrons I can see with library categories I can't use, I think having them in the filter would not hurt?