Bug 30230

Summary: Search for patrons in checkout should not require edit_borrowers permission
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: CirculationAssignee: Thibaud Guillot <thibaud.guillot>
Status: Pushed to stable --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: david, dcook, emmi.takkinen, fridolin.somers, george, gmcharlt, hebah, jonathan.druart, kyle.m.hall, lisette.scheer, lucas, martin.renvoize, mathsabypro, michaela.sieber, rcoert, thibaud.guillot, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29509
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35381
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03
Bug Depends on: 15812    
Bug Blocks: 29509, 35980    
Attachments: Bug 30230: Add new 'list_borrowers' permission
screenshot of confusing interface
Bug 30230: Add new 'list_borrowers' permission
Bug 30230 : (follow-up) Fix permission on /patrons api route
Link to be removed
Bug 30230: Add new 'list_borrowers' permission
Bug 30230: Add new 'list_borrowers' permission
Bug 30230: Add new 'list_borrowers' permission
Bug 30230: Add new 'list_borrowers' permission
Bug 30230 : (follow-up) Fix display issue on search bar
Bug 30230: (follow-up) Fix display issue on search bar
Bug 30230: Add new 'list_borrowers' permission
Bug 30230: (follow-up) Fix display issue on search bar
Bug 30230: Add new 'list_borrowers' permission
Bug 30230: (follow-up) Fix display issue on search bar
Bug 30230: (follow-up) fix unit test
Bug 30230: (follow-up) Add list permission on home-search include and update atomic update file
Bug 30230: Add new 'list_borrowers' permission
Bug 30230: (follow-up) Fix display issue on search bar
Bug 30230: (follow-up) fix unit test
Bug 30230: (follow-up) Add list permission on home-search include and update atomic update file
Bug 30230: (QA follow-up) Add unit tests for API definition change
Bug 30230: (QA follow-up) Also add list_borrowers to the singular endpoint
Bug 30230: (QA follow-up) Add unit tests for API definition change
Bug 30230: (QA follow-up) Also add list_borrowers to the singular endpoint

Description Andrew Fuerste-Henry 2022-03-04 21:21:47 UTC
Bug 15812 made searching for patrons for checkout use the regular patron search. This resulted in a significant change in behavior for users who have _circulate_ permissions but not _edit_borrowers_.

Previously, a user with these permissions could enter a partial name, be taken to a list of search results, and select the patron they desire.

Following bug 15812, a user with these permissions will hit a "You do not have permissions" block when attempting to search for a partial patron name. One can allow the search by adding _edit_borrowers_ permission, but that also grants the ability to create and edit patrons.
Comment 1 George Williams (NEKLS) 2022-07-27 18:58:13 UTC
This is a problem for our system also.  We have libraries with volunteer staff who do not have "edit_borrowers" permission who can no longer search for borrowers by name.
Comment 2 David Cook 2023-08-16 23:12:03 UTC
We did have some folk struggle due to the change in permissions, although the description for "edit_borrowers" is "Add, modify and view patron information" and the patron search results does show patron information for all patrons. 

I'm not sure requiring "edit_borrowers" was the right way to go, but I can understand wanting to make it more restricted too.
Comment 3 Thibaud Guillot 2023-08-28 12:30:09 UTC
Hello,

This change in patron search has an impact on checkouts, a patron who does not have the permission to edit a borrower cannot perform a checkout, which would be most appropriate for correct this kind of problem ?
Comment 4 David Cook 2023-08-28 23:49:49 UTC
(In reply to Thibaud Guillot from comment #3)
> Hello,
> 
> This change in patron search has an impact on checkouts, a patron who does
> not have the permission to edit a borrower cannot perform a checkout, which
> would be most appropriate for correct this kind of problem ?

They can perform the checkout if they have the cardnumber for the patron. Searching for the patron using the cardnumber will take them directly to the patron where they can do the checkout.
Comment 5 Thibaud Guillot 2023-08-29 09:08:40 UTC
Thanks for your answer, in fact, they can use the cardnumber even if the patron search by name was surely more practical, it changes habits
Comment 6 David Cook 2023-08-29 23:03:19 UTC
(In reply to Thibaud Guillot from comment #5)
> Thanks for your answer, in fact, they can use the cardnumber even if the
> patron search by name was surely more practical, it changes habits

Sorry, my apologies. I agree that it's an issue. I've certainly received complaints about it.
Comment 7 Thibaud Guillot 2023-08-30 06:43:55 UTC
No problem, what would be the best solution to correct this behavior ?(In reply to David Cook from comment #6)
> (In reply to Thibaud Guillot from comment #5)
> > Thanks for your answer, in fact, they can use the cardnumber even if the
> > patron search by name was surely more practical, it changes habits
> 
> Sorry, my apologies. I agree that it's an issue. I've certainly received
> complaints about it.

No problem, what would be the best solution to correct this behavior ?
Comment 8 David Cook 2023-08-30 23:31:55 UTC
(In reply to Thibaud Guillot from comment #7)
> No problem, what would be the best solution to correct this behavior ?

Burn it all down and start again?

I'm just kidding. I suppose we look at who made the change that caused this change, and have a discussion with them about it.

I'm not 100% sure that bug 15812 is the problem. I might take a quick little look and see...
Comment 9 David Cook 2023-08-30 23:55:52 UTC
(In reply to David Cook from comment #8)
> I'm not 100% sure that bug 15812 is the problem. I might take a quick little
> look and see...

Ok my bad I misunderstood how bug 15812 changed things but getting a better picture now.

I think that change has been around too long to just revert, but it's clearly a regression.

Hmm and actually the thing with the cardnumber working... it seems to only retrieve patrons from the same branch as the logged in account...
Comment 10 David Cook 2023-08-31 00:01:51 UTC
Note the autocomplete also gives a 403...
Comment 11 David Cook 2023-08-31 00:29:02 UTC
(In reply to David Cook from comment #10)
> Note the autocomplete also gives a 403...

{"error":"Authorization failure. Missing required permission(s).","required_permissions":[{"borrowers":"edit_borrowers"},{"tools":"label_creator"},{"serials":"routing"},{"acquisition":"order_manage"}]}

--

The authorizations/permissions need a comprehensive review/restructure. I think we've known that for a while.

For instance, the reason why I can't retrieve patrons from other branches for checkout is because I don't have the sub permission "view_borrower_infos_from_any_libraries". But because I don't have "edit_borrower", I can't see borrower info from my current library either anyways. The authorization here makes no sense. 

--

I don't think this issue is really fixable on its own. It would require systemic changes to other functionality to really get it right. 

A short-term fix might be to create a new subpermission called "view_borrowers" and require circ staff to have that but even that's not quite right. 

We've worked ourselves into a corner with the current permissions and the functionality.

Maybe we do just let "circulate_remaining_permissions" view member.pl and have that implicitly have the ability to "view patron information" despite it not really adhering to the explicit goals of the permission system.

That's probably the unfortunate solution here...
Comment 12 David Cook 2023-08-31 00:41:24 UTC
Except that doesn't work either because of the whacky REST API permissions that also require edit_borrowers. 

In theory, the REST API was a good idea, but I think we've approached it a bit naively. 

Rather than search using GET /api/v1/patrons, I reckon we should be POSTing to something like /api/v1/patrons/search, which can perform more nuanced operations. And we are looking into more nuanced operations but that's also challenging in its own way (especially from a performance perspective). 

--

So I think that takes us back to a "view_borrowers" subpermission which is also kind of weird, because then you'd need "view_borrowers" plus "delete_borrowers" or "edit_borrowers" in order to perform those latter operations. That's where role-based access control would be useful. You'd just set up the roles and assign the roles and the weirdness of permissions would be hidden a bit behind the scenes. 

But overhauling permissions is no easy task because we also need to maintain the messy status quo. 

So we end up just adding workaround on top of workaround...

I think it'll either take a big sponsored project or serious collaboration among many developers to really improve access control/authorization.
Comment 13 David Cook 2023-08-31 00:44:38 UTC
(In reply to Thibaud Guillot from comment #7)
> No problem, what would be the best solution to correct this behavior ?

tl;dr

Someone can add "view_borrowers" and add that to member.pl, the Patrons REST API, and probably a bunch of other places. Or I suppose you could add "circulate_remaining_permissions" to member.pl, the Patrons REST API, and a bunch of other places... which would be theoretically silly but it would be practical. 

In any case, that someone isn't going to be me. It's too much work for too little gain. Unfortunately, the most practical workaround is to give "edit_borrowers" permission, and I think that's probably what most people have been doing to avoid this issue over the past couple of years.

I wish whomever takes this on good luck though, because it would be a good one to fix.
Comment 14 Jonathan Druart 2023-08-31 07:28:14 UTC
This is actually a duplicate of bug 29509 I think.
Comment 15 David Cook 2023-08-31 23:15:08 UTC
(In reply to Jonathan Druart from comment #14)
> This is actually a duplicate of bug 29509 I think.

I think you might be right. Or at least there is a lot of overlap. I think bug 29509 is already engaging with the hardest problem.
Comment 16 George Williams (NEKLS) 2023-09-08 03:11:43 UTC
What I'd like to see in the borrower search is, if the staff user doesn't have edit_borrower permission, then 

1. the link to the borrower's details page in the "Name" column is removed from #memberresultst
2. the borrower's address in the "Name" column is removed from #memberresultst
3. the borrower's "Date of birth" column is removed from #memberresultst
4. the "Edit" button is removed from the last column on #memberresultst

This keeps the checkout search and the borrower search the same search (combining what used to be two separate borrower search systems is what got us here in the first place) but it removes the data from the borrower search results that was previously excluded when the checkout search used to be a separate search.

I don't think removing these things from the search resultswould be terribly difficult if the staff user doesn't have edit_borrower permission and it would solve just about all of the problems that have been discussed here.
Comment 17 David Cook 2023-09-08 03:26:46 UTC
(In reply to George Williams (NEKLS) from comment #16)
> I don't think removing these things from the search resultswould be terribly
> difficult if the staff user doesn't have edit_borrower permission and it
> would solve just about all of the problems that have been discussed here.

Unfortunately, in practice it's more complicated than that, because there other permissions like view_borrower_infos_from_any_libraries and preferences that also need to be taken into account for showing borrower information.

It also means updating generic code libraries that are being used for other things where edit_borrower doesn't really apply.

It's a bit of a mess.
Comment 18 David Cook 2023-11-21 23:41:28 UTC
(In reply to David Cook from comment #12)
> In theory, the REST API was a good idea, but I think we've approached it a
> bit naively. 
> 
> Rather than search using GET /api/v1/patrons, I reckon we should be POSTing
> to something like /api/v1/patrons/search, which can perform more nuanced
> operations. And we are looking into more nuanced operations but that's also
> challenging in its own way (especially from a performance perspective). 

Actually, something like /api/v1/circ/search/patrons might be better. (Jonathan's ERM and preservation have shown how it's useful to organise endpoints under a module path.)
Comment 19 David Cook 2023-11-21 23:47:20 UTC
(In reply to David Cook from comment #18)
> Actually, something like /api/v1/circ/search/patrons might be better.
> (Jonathan's ERM and preservation have shown how it's useful to organise
> endpoints under a module path.)

Under the hood, /api/v1/patrons and /api/v1/circ/search/patrons should use the same model code, so it should be fairly trivial to implement in theory. The /api/v1/circ/search/patrons controller would probably just need "circulate_remaining_permissions" permission.

They may need "view_borrower_infos_from_any_libraries" as well although that should hopefully be handled by the underlying model code. (This is what I mean about how our permissions are confusing in terms of providing access to a page vs providing access to an operation. In theory, by extension, we should have a "view_borrowers" permission, except that's not really backwards compatible. Although I suppose we could programmatically give "view_borrowers" to existing users with "edit_borrowers"...)
Comment 20 Thibaud Guillot 2023-11-30 14:29:00 UTC Comment hidden (obsolete)
Comment 21 Thibaud Guillot 2023-11-30 14:31:57 UTC
I've noted that this bug is related to bug 29509 (maybe even the same one, as Jonathan says), but I've been working more on the interface functionality than on what the route could change (apart from adding the 'list_borrowers' permission to get the datatable on search results).
Comment 22 Andrew Fuerste-Henry 2023-11-30 15:19:10 UTC
I feel like this permission should be grouped with the other borrowers permissions, rather than set off as its own top-level permission.

I created a user with only catalogue, circulate_remaining_permissions, and list_borrowers. I was able to check out to a patron using either their cardnumber or name to find them and when I entered a name which found more than one patron I was able to access the patron search screen to select the correct patron. So that all worked as desired.

A couple of minor things seem off, though, both on the member.pl search results page:
 - clicking a patron name in the results list takes one to cgi-bin/koha/members/moremember.pl, a page one would otherwise not be able to access with only list_borrowers

- while on member.pl or moremember.pl as a user with only list_borrowers permission, the Search Patrons feature in the main search bar is only partially disabled -- the Search Patrons option still appears, but when it is selected the search box disappears, thereby preventing the search. This is a visually confusing approach and becomes more confusing if one clicks around between the options here, as the search box will reappear when something other than Search Patrons is selected. See the attached screenshot in which the interface is simultaneously indicating both Check Out and Search Patrons. Can we remove Search Patrons from this bar entirely if the user lacks that permission?
Comment 23 Andrew Fuerste-Henry 2023-11-30 15:19:43 UTC
Created attachment 159412 [details]
screenshot of confusing interface
Comment 24 David Nind 2023-11-30 17:41:46 UTC
Added assignee.
Comment 25 David Cook 2023-11-30 23:01:59 UTC
(In reply to Andrew Fuerste-Henry from comment #22)
> I feel like this permission should be grouped with the other borrowers
> permissions, rather than set off as its own top-level permission.

+1

This needs to be in "permissions" and not "userflags". This is important for the UI and overall permission logic.

> A couple of minor things seem off, though, both on the member.pl search
> results page:
>  - clicking a patron name in the results list takes one to
> cgi-bin/koha/members/moremember.pl, a page one would otherwise not be able
> to access with only list_borrowers

It's a good point.

I think list_borrowers should be able to go to the moremember.pl page, since "list_borrowers" basically means you can list all patrons and their information. But they shouldn't be able to update/delete. 

In general, Koha doesn't have very nuanced authorization, but I suppose in this case maybe we should be defining "flagsrequired" based on HTTP method. If it's a GET, then "list_borrowers" can be included. But if it's a POST or something else, it needs to only be "edit_borrowers". 

> - while on member.pl or moremember.pl as a user with only list_borrowers
> permission, the Search Patrons feature in the main search bar is only
> partially disabled -- the Search Patrons option still appears, but when it
> is selected the search box disappears, thereby preventing the search. This
> is a visually confusing approach and becomes more confusing if one clicks
> around between the options here, as the search box will reappear when
> something other than Search Patrons is selected. See the attached screenshot
> in which the interface is simultaneously indicating both Check Out and
> Search Patrons. Can we remove Search Patrons from this bar entirely if the
> user lacks that permission?

I'm not sure I understand what you're saying here...
Comment 26 Thibaud Guillot 2023-12-01 07:51:42 UTC
Created attachment 159427 [details] [review]
Bug 30230: Add new 'list_borrowers' permission

When a patron search is performed only a user with edit_borrowers
permission can search by name. Search can works only with cardnumber but
it makes searching less intuitive I think.

So, as mentioned in the discussion, I've added a new 'list_borrowers' permission,
completely independent of 'edit_borrowers', so that I can search for a member via the interface
and get the results. In addition to the permission to perform check in and checkouts, this no longer poses an obstacle to simple use.

Test plan:

1) Check with a user without 'edit_borrowers' permission that the patron search can only be performed with cardnumber
2) Apply this patch
3) Make the updatedatabase to add new 'list_borrowers' permissions
4) Set 'list_borrowers' permission on one user and see the difference
Comment 27 Thibaud Guillot 2023-12-01 07:58:37 UTC
(In reply to Andrew Fuerste-Henry from comment #22)
> I feel like this permission should be grouped with the other borrowers
> permissions, rather than set off as its own top-level permission.
> 
> I created a user with only catalogue, circulate_remaining_permissions, and
> list_borrowers. I was able to check out to a patron using either their
> cardnumber or name to find them and when I entered a name which found more
> than one patron I was able to access the patron search screen to select the
> correct patron. So that all worked as desired.
> 
> A couple of minor things seem off, though, both on the member.pl search
> results page:
>  - clicking a patron name in the results list takes one to
> cgi-bin/koha/members/moremember.pl, a page one would otherwise not be able
> to access with only list_borrowers
> 
> - while on member.pl or moremember.pl as a user with only list_borrowers
> permission, the Search Patrons feature in the main search bar is only
> partially disabled -- the Search Patrons option still appears, but when it
> is selected the search box disappears, thereby preventing the search. This
> is a visually confusing approach and becomes more confusing if one clicks
> around between the options here, as the search box will reappear when
> something other than Search Patrons is selected. See the attached screenshot
> in which the interface is simultaneously indicating both Check Out and
> Search Patrons. Can we remove Search Patrons from this bar entirely if the
> user lacks that permission?

Hello ! 

Thanks for your comments Andrew and David, I've updated my patch to take them into account.

I've grouped this permission under those linked to borrowers, and modified access on the moremember.pl page. I'm divided on this point because, as David says, it may be useful to see more detailed information on a member, but at the same time, there's the view_borrower_infos_from_any_libraries permission which allows it, it seems to me. The difference is that it's not limited to one site... so I'm open to suggestions on this point, of course, as I don't know what would be best.

As far as the search bar and the "Search patrons" tab are concerned, I did indeed notice a bug when it appeared, so I modified the code to display this tab only if the user had "circulate" permission... that's what I understood from your example.

The patch can now be tested again :) Thanks to both of you
Comment 28 Andrew Fuerste-Henry 2023-12-01 20:02:57 UTC
With this new patch, I did the following:
 - create a user with catalogue, circulate_remaining_permissions, and list_borrowers permissions
 - log into the staff client as that user
 - in the Checkout field of mainpage.pl, search for "a" (with superlibrarian permissions, this returns all patrons with "a" in their name")
 - Koha takes you to the search results page, but shows no results and gives message "Something went wrong when loading this table. 403: Forbidden"
 - go back to the homepage and enter "Henry" in the Checkout box (in the default sandbox data this name finds only one patron and should take you straight to them)
 - Koha again takes you to the search results page and gives a 403 error
Comment 29 Thibaud Guillot 2023-12-04 08:34:15 UTC
Created attachment 159517 [details] [review]
Bug 30230 : (follow-up) Fix permission on /patrons api route
Comment 30 Thibaud Guillot 2023-12-04 08:35:38 UTC
(In reply to Andrew Fuerste-Henry from comment #28)
> With this new patch, I did the following:
>  - create a user with catalogue, circulate_remaining_permissions, and
> list_borrowers permissions
>  - log into the staff client as that user
>  - in the Checkout field of mainpage.pl, search for "a" (with superlibrarian
> permissions, this returns all patrons with "a" in their name")
>  - Koha takes you to the search results page, but shows no results and gives
> message "Something went wrong when loading this table. 403: Forbidden"
>  - go back to the homepage and enter "Henry" in the Checkout box (in the
> default sandbox data this name finds only one patron and should take you
> straight to them)
>  - Koha again takes you to the search results page and gives a 403 error

Hello Andrew,

Sorry it's my bad I forgot to change permission name on patrons api route. Now it's fixed I think
Comment 31 Andrew Fuerste-Henry 2023-12-04 17:39:10 UTC
Created attachment 159559 [details]
Link to be removed

This latest patch addresses all of my concerns except for the "Search Patrons" link in the main search bar on the patron search results page. See attached screenshot. We should hide that "Search Patrons" link entirely.
Comment 32 Lucas Gass 2023-12-04 20:10:51 UTC
As this stands no borrowers/staff will get the new permission on update. That is a change in behavior that needs to be avoided.

I think we need to give anyone who has the edit_borrowers permission the list_borrowers permission via a DB update.
Comment 33 David Cook 2023-12-04 22:56:48 UTC
*** Bug 35381 has been marked as a duplicate of this bug. ***
Comment 34 Thibaud Guillot 2023-12-05 08:49:04 UTC
(In reply to Andrew Fuerste-Henry from comment #31)
> Created attachment 159559 [details]
> Link to be removed
> 
> This latest patch addresses all of my concerns except for the "Search
> Patrons" link in the main search bar on the patron search results page. See
> attached screenshot. We should hide that "Search Patrons" link entirely.

Hello Andrew, 

Normally I've taken your comments into account and removed the "Search patrons" link from the main search bar... it's only accessible to members with 'catalog' permission. Unless I've misunderstood, but from my point of view, with a member who only has the 'list_borrowers' permission, this seems correct.
Comment 35 Thibaud Guillot 2023-12-05 08:50:55 UTC
(In reply to Lucas Gass from comment #32)
> As this stands no borrowers/staff will get the new permission on update.
> That is a change in behavior that needs to be avoided.
> 
> I think we need to give anyone who has the edit_borrowers permission the
> list_borrowers permission via a DB update.

Hello Lucas,

This permission is only there to allow search and list view of members, no access to the details of each member (moremember.pl) and therefore in my opinion no update... I've missed something else, or perhaps I'm misunderstanding you.
Comment 36 Lucas Gass 2023-12-05 15:05:10 UTC
(In reply to Thibaud Guillot from comment #35)
> (In reply to Lucas Gass from comment #32)
> > As this stands no borrowers/staff will get the new permission on update.
> > That is a change in behavior that needs to be avoided.
> > 
> > I think we need to give anyone who has the edit_borrowers permission the
> > list_borrowers permission via a DB update.
> 
> Hello Lucas,
> 
> This permission is only there to allow search and list view of members, no
> access to the details of each member (moremember.pl) and therefore in my
> opinion no update... I've missed something else, or perhaps I'm
> misunderstanding you.

If staff can see/view borrower information before the patchset they need to have the same behavior after the patchset.
Comment 37 David Cook 2023-12-05 22:15:33 UTC
(In reply to Lucas Gass from comment #36)
> > This permission is only there to allow search and list view of members, no
> > access to the details of each member (moremember.pl) and therefore in my
> > opinion no update... I've missed something else, or perhaps I'm
> > misunderstanding you.
> 
> If staff can see/view borrower information before the patchset they need to
> have the same behavior after the patchset.

I agree with Lucas.

Another way to think of it is as "view" rather than "list". The user could just visit http://localhost:8081/api/v1/patrons?_page=1&_per_page=100 and see all the details, so it doesn't really make sense to stop them from seeing the moremember.pl page. 

(We just need to make sure someone with list_borrowers only can't add/update borrowers. They're fine to view them.)
Comment 38 Thibaud Guillot 2023-12-06 10:11:06 UTC
Created attachment 159613 [details] [review]
Bug 30230: Add new 'list_borrowers' permission

When a patron search is performed only a user with edit_borrowers
permission can search by name. Search can works only with cardnumber but
it makes searching less intuitive I think.

So, as mentioned in the discussion, I've added a new 'list_borrowers' permission,
completely independent of 'edit_borrowers', so that I can search for a member via the interface
and get the results. In addition to the permission to perform check in and checkouts, this no longer poses an obstacle to simple use.

Test plan:

1) Check with a user without 'edit_borrowers' permission that the patron search can only be performed with cardnumber
2) Apply this patch
3) Make the updatedatabase to add new 'list_borrowers' permissions
4) Set 'list_borrowers' permission on one user and see the difference
Comment 39 Thibaud Guillot 2023-12-06 10:18:31 UTC
Hello,

I've added the 'list_borrowers' permission on moremember.pl, as it seemed more logical, I had included it initially but removed it later. 
A member with only the list_borrowers permission won't be able to add or modify an existing pattern, but will be able to view its information.

What I didn't understand was the need to create an atomicupdate to add the list_borrowers permission to members with the 'edit_borrowers' permission (comment 32).

I hope I've understood you correctly this time :) thanks in any case for your feedback.
Comment 40 David Cook 2023-12-06 22:55:11 UTC
Sounds good! I've got something on at the moment, but I'll try to look at this later.

Thanks, Thibaud for your hard work on this one!
Comment 41 David Cook 2023-12-07 05:43:16 UTC
(In reply to Lucas Gass from comment #32)
> As this stands no borrowers/staff will get the new permission on update.
> That is a change in behavior that needs to be avoided.

Originally, I was thinking this too, but now I'm not so sure, since this issue has been around for 2 years now. 
 
> I think we need to give anyone who has the edit_borrowers permission the
> list_borrowers permission via a DB update.

Thibaud has made it so you can have either list_borrowers or edit_borrowers, so I don't think we'd want to give list_borrowers to anyone with edit_borrowers. 

But perhaps we'd want to give list_borrowers to anyone with "circulate_remaining_permissions". 

But then I think maybe we don't need to worry about it, since over the last 2 years people will have probably given "edit_borrowers"... so maybe we're catering more to new installs / new users...
Comment 42 David Cook 2023-12-07 05:45:23 UTC
I would've been happy to sign this off, but looks like there is a little perltidy problem with the database update. 

If you perltidy that file, then you should be good to go.

--

testing 1 commit(s) (applied to 0d483a5 '22 DBRev 23.12.00.000: Start of a new')

Processing files before patches
|========================>| 8 / 8 (100.00%)
Processing files after patches
|========================>| 8 / 8 (100.00%)

 OK     api/v1/swagger/paths/patrons.yaml

 WARN   installer/data/mysql/atomicupdate/bug_29509_add-new-list-borrowers-permission.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 0, now: 1)

 OK     koha-tmpl/intranet-tmpl/prog/en/includes/patron-search-header.inc

 OK     koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc

 OK     koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt

 OK     members/member.pl

 OK     members/members-home.pl

 OK     members/moremember.pl


Processing additional checks OK!
Comment 43 Thibaud Guillot 2023-12-07 08:13:34 UTC
Created attachment 159633 [details] [review]
Bug 30230: Add new 'list_borrowers' permission

When a patron search is performed only a user with edit_borrowers
permission can search by name. Search can works only with cardnumber but
it makes searching less intuitive I think.

So, as mentioned in the discussion, I've added a new 'list_borrowers' permission,
completely independent of 'edit_borrowers', so that I can search for a member via the interface
and get the results. In addition to the permission to perform check in and checkouts, this no longer poses an obstacle to simple use.

Test plan:

1) Check with a user without 'edit_borrowers' permission that the patron search can only be performed with cardnumber
2) Apply this patch
3) Make the updatedatabase to add new 'list_borrowers' permissions
4) Set 'list_borrowers' permission on one user and see the difference
Comment 44 Thibaud Guillot 2023-12-07 08:15:09 UTC
(In reply to David Cook from comment #42)
> I would've been happy to sign this off, but looks like there is a little
> perltidy problem with the database update. 
> 
> If you perltidy that file, then you should be good to go.
> 
> --
> 
> testing 1 commit(s) (applied to 0d483a5 '22 DBRev 23.12.00.000: Start of a
> new')
> 
> Processing files before patches
> |========================>| 8 / 8 (100.00%)
> Processing files after patches
> |========================>| 8 / 8 (100.00%)
> 
>  OK     api/v1/swagger/paths/patrons.yaml
> 
>  WARN  
> installer/data/mysql/atomicupdate/bug_29509_add-new-list-borrowers-
> permission.pl
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 0, now: 1)
> 
>  OK     koha-tmpl/intranet-tmpl/prog/en/includes/patron-search-header.inc
> 
>  OK     koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
> 
>  OK     koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
> 
>  OK     members/member.pl
> 
>  OK     members/members-home.pl
> 
>  OK     members/moremember.pl
> 
> 
> Processing additional checks OK!


Thanks for you feedback, I rebased it cause the bug number was not good in update file. So I hope that will be okay now :) 

Have a nice day
Comment 45 Andrew Fuerste-Henry 2023-12-07 15:34:16 UTC
> Normally I've taken your comments into account and removed the "Search
> patrons" link from the main search bar... it's only accessible to members
> with 'catalog' permission. Unless I've misunderstood, but from my point of
> view, with a member who only has the 'list_borrowers' permission, this seems
> correct.

Hi Thibaud, thanks for your work here! This code will help us a lot.
That "Search Patrons" link still shows up and behaves oddly for someone with just list_borrowers permission, but only in one little spot. To see it:
 - have an account with permissions "Staff access, allows viewing of catalogue in staff interface (catalogue)", "Remaining circulation permissions (circulate_remaining_permissions)", and "Search and list patrons (list_borrowers)"

 - Go to the checkout screen on any patron record (/cgi-bin/koha/circ/circulation.pl?borrowernumber=)

- Click "Routing Lists" in the menu on the left of the screen

- That takes you to /cgi-bin/koha/members/routing-lists.pl?borrowernumber=

- "Search patrons" shows in the search bar, with no search field displayed while "Search patrons" is selected.

I realize this is a really minor display issue on one page few users look at. I'm going to go ahead and sign off on this because it works in every other way. I'm happy to sign off again if you have a chance to change that page, or we can worry about it on a follow-up bug.
Comment 46 ByWater Sandboxes 2023-12-07 15:34:38 UTC
Created attachment 159654 [details] [review]
Bug 30230: Add new 'list_borrowers' permission

When a patron search is performed only a user with edit_borrowers
permission can search by name. Search can works only with cardnumber but
it makes searching less intuitive I think.

So, as mentioned in the discussion, I've added a new 'list_borrowers' permission,
completely independent of 'edit_borrowers', so that I can search for a member via the interface
and get the results. In addition to the permission to perform check in and checkouts, this no longer poses an obstacle to simple use.

Test plan:

1) Check with a user without 'edit_borrowers' permission that the patron search can only be performed with cardnumber
2) Apply this patch
3) Make the updatedatabase to add new 'list_borrowers' permissions
4) Set 'list_borrowers' permission on one user and see the difference

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 47 Emmi Takkinen 2023-12-08 08:03:18 UTC
Created attachment 159667 [details] [review]
Bug 30230: Add new 'list_borrowers' permission

When a patron search is performed only a user with edit_borrowers
permission can search by name. Search can works only with cardnumber but
it makes searching less intuitive I think.

So, as mentioned in the discussion, I've added a new 'list_borrowers' permission,
completely independent of 'edit_borrowers', so that I can search for a member via the interface
and get the results. In addition to the permission to perform check in and checkouts, this no longer poses an obstacle to simple use.

Test plan:

1) Check with a user without 'edit_borrowers' permission that the patron search can only be performed with cardnumber
2) Apply this patch
3) Make the updatedatabase to add new 'list_borrowers' permissions
4) Set 'list_borrowers' permission on one user and see the difference

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 48 Emmi Takkinen 2023-12-08 08:07:21 UTC
Signing off, since this change is much needed and patch works as described.
Comment 49 Thibaud Guillot 2023-12-08 08:47:09 UTC
Created attachment 159668 [details] [review]
Bug 30230 : (follow-up) Fix display issue on search bar
Comment 50 Thibaud Guillot 2023-12-08 08:48:09 UTC
Thank you all for this time of testing and feedback. I think I've managed to fix the display problem Andrew was talking about.
Comment 51 Andrew Fuerste-Henry 2023-12-08 14:11:21 UTC
(In reply to Thibaud Guillot from comment #50)
> Thank you all for this time of testing and feedback. I think I've managed to
> fix the display problem Andrew was talking about.

Thanks!
Comment 52 Victor Grousset/tuxayo 2023-12-17 23:43:34 UTC
Created attachment 159933 [details] [review]
Bug 30230: (follow-up) Fix display issue on search bar
Comment 53 Victor Grousset/tuxayo 2023-12-17 23:45:35 UTC
(Fixed commit message formatting to match coding guidelines)
Comment 54 Victor Grousset/tuxayo 2023-12-19 01:28:54 UTC
Created attachment 159988 [details] [review]
Bug 30230: Add new 'list_borrowers' permission

When a patron search is performed only a user with edit_borrowers
permission can search by name. Search can works only with cardnumber but
it makes searching less intuitive I think.

So, as mentioned in the discussion, I've added a new 'list_borrowers' permission,
completely independent of 'edit_borrowers', so that I can search for a member via the interface
and get the results. In addition to the permission to perform check in and checkouts, this no longer poses an obstacle to simple use.

Test plan:

1) Check with a user without 'edit_borrowers' permission that the patron search can only be performed with cardnumber
2) Apply this patch
3) Make the updatedatabase to add new 'list_borrowers' permissions
4) Set 'list_borrowers' permission on one user and see the difference

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 55 Victor Grousset/tuxayo 2023-12-19 01:28:57 UTC
Created attachment 159989 [details] [review]
Bug 30230: (follow-up) Fix display issue on search bar

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 56 Victor Grousset/tuxayo 2023-12-19 01:29:09 UTC
It works :) including fixing the issue in comment 45

Main test plan done with minimal permissions at the start (catalogue, circulate_remaining_permissions)
And also with maximal permissions (all but superlibrarian and edit_borrowers)
Comment 57 David Cook 2023-12-19 02:49:06 UTC
Created attachment 159990 [details] [review]
Bug 30230: Add new 'list_borrowers' permission

When a patron search is performed only a user with edit_borrowers
permission can search by name. Search can works only with cardnumber but
it makes searching less intuitive I think.

So, as mentioned in the discussion, I've added a new 'list_borrowers' permission,
completely independent of 'edit_borrowers', so that I can search for a member via the interface
and get the results. In addition to the permission to perform check in and checkouts, this no longer poses an obstacle to simple use.

Test plan:

1) Check with a user without 'edit_borrowers' permission that the patron search can only be performed with cardnumber
2) Apply this patch
3) Make the updatedatabase to add new 'list_borrowers' permissions
4) Set 'list_borrowers' permission on one user and see the difference

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 58 David Cook 2023-12-19 02:49:10 UTC
Created attachment 159991 [details] [review]
Bug 30230: (follow-up) Fix display issue on search bar

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 59 David Cook 2023-12-19 02:49:14 UTC
Created attachment 159992 [details] [review]
Bug 30230: (follow-up) fix unit test

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 60 David Cook 2023-12-19 03:04:19 UTC
I'm pretty happy with how this one has turned out. 

Just added a simple unit test fix follow-up. 

It is tempting to add a Selenium test, but I'm not sure what it would prove. Would it prove that it could list borrowers or that it can't edit them? Or that it couldn't access other parts of Koha? 

In any case, marking Passed QA.
Comment 61 Jonathan Druart 2023-12-19 07:30:37 UTC
(In reply to David Cook from comment #60)
> It is tempting to add a Selenium test, but I'm not sure what it would prove.
> Would it prove that it could list borrowers or that it can't edit them? Or
> that it couldn't access other parts of Koha? 

In doubt, cover all use cases? :D
Comment 62 Victor Grousset/tuxayo 2023-12-19 07:37:50 UTC
Thanks David for your help in reviewing :)


> It is tempting to add a Selenium test

Run, you fool! it's a trap! => Bug 29285



(In reply to Jonathan Druart from comment #61)
> In doubt, cover all use cases? :D

To maximize random test failures :D
Comment 63 Katrin Fischer 2023-12-20 13:51:30 UTC Comment hidden (obsolete)
Comment 64 Katrin Fischer 2023-12-20 13:52:23 UTC
This doesn't look like too big a change and I think the permission will be useful for other use cases that we still need to target. So yay!

BUT: I got questions.

1) Should we update existing staff patron accounts to make this change more convenient? Should everyone having edit_borrowers also have list_borrowers?

2) Permission description/scope

And for my understanding: the permission description reads:
Search and list patrons

Do we include "view" in list? 
And should we add it to the description?
How does it work in combination with view_borrower_infos_from_any_libraries? 

I think the problem right now is that our sub permissions in borrowers are not always very clear and I'd like to try and make sure we improve things moving forward! So please bear with my questions :)
Comment 65 David Cook 2023-12-21 03:17:34 UTC
(In reply to Katrin Fischer from comment #64)
> 1) Should we update existing staff patron accounts to make this change more
> convenient? Should everyone having edit_borrowers also have list_borrowers?

I don't think there's be anything functionally to gain from doing that, since edit_borrowers implicitly has list_borrowers permissions. 
 
> And for my understanding: the permission description reads:
> Search and list patrons
> 
> Do we include "view" in list? 
> And should we add it to the description?

Personally, I don't love the description, but I figured it was good enough. "Search, list, and view patrons" would work too though.
Comment 66 Katrin Fischer 2023-12-30 19:29:59 UTC
> How does it work in combination with view_borrower_infos_from_any_libraries? 

Or more: how does it relate to this one? 
I am just trying to get a clear picture on how this works.
Please switch back to PQA after reply, so I don't miss it. Thanks!
Comment 67 David Cook 2024-01-01 22:52:21 UTC
(In reply to Katrin Fischer from comment #66)
> > How does it work in combination with view_borrower_infos_from_any_libraries? 
> 
> Or more: how does it relate to this one? 
> I am just trying to get a clear picture on how this works.
> Please switch back to PQA after reply, so I don't miss it. Thanks!

It works the same way as it does for "edit_borrowers". The "view_borrower_infos_from_any_libraries" is handled by the API via Koha::Patron.
Comment 68 Katrin Fischer 2024-01-05 14:37:55 UTC
I reviewed the patch more closely today and tested. We'll need some more small fixes:

1) +    bug_number  => "BUG_30230",

This is wrong and should be just 30230.

2) Please add view to the permission descriptions in the SQL and template file:

+            "INSERT IGNORE INTO permissions (module_bit, code, description) VALUES (4, 'list_borrowers', 'Search and list patrons')"

3) In the main navigation (not start page, but on top) the Patrons module is missing with only catalogue and list_borrowers permission.


I quite like the way how this lets you limit patron search to your own library in combination with library groups. When view_borrower_infos_from_any_libraries is added, I can search them all. But I am not required to also give edit_borrowers. This is nice. 

Please help to finish this! :)
Comment 69 Victor Grousset/tuxayo 2024-01-08 23:48:41 UTC
Hi :) change needed when bug 35517 (passed QA) is pushed to master (so soon unless a surprise comes up):

Look at the patch: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160671&action=diff

The change should be:
[% ELSIF ( CAN_user_borrowers_edit_borrowers ) %]
↓↓
[% ELSIF ( CAN_user_borrowers_edit_borrowers ) || ( CAN_user_borrowers_list_borrowers ) %]
Comment 70 Katrin Fischer 2024-01-16 14:47:21 UTC
*ping* Still waiting for the follow-up!
Comment 71 Thibaud Guillot 2024-01-16 16:03:49 UTC
Created attachment 161068 [details] [review]
Bug 30230: (follow-up) Add list permission on home-search include and update atomic update file
Comment 72 Martin Renvoize 2024-01-22 13:52:44 UTC
Created attachment 161240 [details] [review]
Bug 30230: Add new 'list_borrowers' permission

When a patron search is performed only a user with edit_borrowers
permission can search by name. Search can works only with cardnumber but
it makes searching less intuitive I think.

So, as mentioned in the discussion, I've added a new 'list_borrowers' permission,
completely independent of 'edit_borrowers', so that I can search for a member via the interface
and get the results. In addition to the permission to perform check in and checkouts, this no longer poses an obstacle to simple use.

Test plan:

1) Check with a user without 'edit_borrowers' permission that the patron search can only be performed with cardnumber
2) Apply this patch
3) Make the updatedatabase to add new 'list_borrowers' permissions
4) Set 'list_borrowers' permission on one user and see the difference

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 73 Martin Renvoize 2024-01-22 13:52:47 UTC
Created attachment 161241 [details] [review]
Bug 30230: (follow-up) Fix display issue on search bar

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 74 Martin Renvoize 2024-01-22 13:52:51 UTC
Created attachment 161242 [details] [review]
Bug 30230: (follow-up) fix unit test

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 75 Martin Renvoize 2024-01-22 13:52:56 UTC
Created attachment 161243 [details] [review]
Bug 30230: (follow-up) Add list permission on home-search include and update atomic update file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Martin Renvoize 2024-01-22 13:58:50 UTC
This is a great start and solves a big problem nicely.

I'm passing QA on it as is, and we should continue to refine other elements on bug 29509, cleaning up old permissions on some of our endpoints and adding this permission retrospectively where needed in a database update.

I'd also love to see some role-based access control work happen.. so the granular permissions that are creeping in like this can be more easily assigned to users for different roles.

QA scripts happy, Tests passing, No regressions.

Passing QA
Comment 77 Martin Renvoize 2024-01-22 14:22:01 UTC
Hold off.. I'm writing API tests for this now.
Comment 78 Martin Renvoize 2024-01-22 14:54:55 UTC
Created attachment 161245 [details] [review]
Bug 30230: (QA follow-up) Add unit tests for API definition change
Comment 79 Martin Renvoize 2024-01-22 14:54:59 UTC
Created attachment 161246 [details] [review]
Bug 30230: (QA follow-up) Also add list_borrowers to the singular endpoint
Comment 80 Martin Renvoize 2024-01-22 14:56:07 UTC
Attempting to write unit tests for the API piece of this appears to highlight some issues.. it seems as soon as you add search terms to the queries the permissions get dropped.. hopefully my brain is just a little slow today.
Comment 81 Martin Renvoize 2024-01-22 15:58:59 UTC
Created attachment 161250 [details] [review]
Bug 30230: (QA follow-up) Add unit tests for API definition change
Comment 82 Martin Renvoize 2024-01-22 15:59:03 UTC
Created attachment 161251 [details] [review]
Bug 30230: (QA follow-up) Also add list_borrowers to the singular endpoint
Comment 83 Martin Renvoize 2024-01-22 16:00:25 UTC
The filtering code was getting in the way of the tests..  the weird 'view_borrower_infos_from_any_libraries' sub-permission that's not entirely a permission in the same way all others are.

Anyway.. tests are fixed and I'm happy to PQA now
Comment 84 Katrin Fischer 2024-01-29 11:17:31 UTC
> 2) Please add view to the permission descriptions in the SQL and template
> file:
> 
> +            "INSERT IGNORE INTO permissions (module_bit, code, description)
> VALUES (4, 'list_borrowers', 'Search and list patrons')"

The .sql file for new installations and the .inc for description in the template were missed. Fixed in a follow-up.

> 3) In the main navigation (not start page, but on top) the Patrons module is
> missing with only catalogue and list_borrowers permission.

This was missed as well, fixed in a follow-up to header.inc.
Comment 85 Katrin Fischer 2024-01-29 11:28:18 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 86 David Cook 2024-01-29 22:40:57 UTC
(In reply to Katrin Fischer from comment #85)
> Pushed for 24.05!
> 
> Well done everyone, thank you!

Great news! Hope that we can get this into 23.11 as well since it's still quite fresh.
Comment 87 Fridolin Somers 2024-02-01 16:10:08 UTC
I've found a minor issue Bug 35980
Comment 88 Fridolin Somers 2024-02-01 16:11:40 UTC
Pushed to 23.11.x for 23.11.03