Bug 24964 - Do not filter patrons AFTER they have been fetched from the DB (when searching with permissions)
Summary: Do not filter patrons AFTER they have been fetched from the DB (when searchin...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 12648
Blocks: 25875
  Show dependency treegraph
 
Reported: 2020-03-24 11:24 UTC by Jonathan Druart
Modified: 2021-12-13 21:08 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00,19.11.06,19.05.11


Attachments
Bug 24964: Add tests (4.37 KB, patch)
2020-03-25 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: Do not filter patrons after they have been fetched (6.95 KB, patch)
2020-03-25 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: Do not filter patrons after they have been fetched (6.95 KB, patch)
2020-03-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: Prevent random failures sorting by borrowernumber (1.04 KB, patch)
2020-03-25 12:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: Add tests (4.43 KB, patch)
2020-03-26 11:52 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 24964: Do not filter patrons after they have been fetched (7.01 KB, patch)
2020-03-26 11:53 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 24964: Prevent random failures sorting by borrowernumber (1.10 KB, patch)
2020-03-26 11:53 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 24964: Add tests (4.51 KB, patch)
2020-03-26 12:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24964: Do not filter patrons after they have been fetched (7.08 KB, patch)
2020-03-26 12:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24964: Prevent random failures sorting by borrowernumber (1.18 KB, patch)
2020-03-26 12:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24964: Fix perl critic warning (929 bytes, patch)
2020-03-26 12:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24964: Do not fetch all patrons for owner/users of funds (2.41 KB, patch)
2020-04-06 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: Do not fetch all patrons for owner/users of funds (1.92 KB, patch)
2020-04-06 09:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: [19.11.x] Add tests (4.50 KB, patch)
2020-04-06 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: [19.11.x] Do not filter patrons after they have been fetched (7.42 KB, patch)
2020-04-06 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: [19.11.x] Prevent random failures sorting by borrowernumber (1.17 KB, patch)
2020-04-06 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: [19.11.x] Fix perl critic warning (921 bytes, patch)
2020-04-06 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24964: [19.11.x] Do not fetch all patrons for owner/users of funds (1.93 KB, patch)
2020-04-06 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-03-24 11:24:07 UTC
There are several places where we are searching for patrons with given permissions, like adding a user to a fund, basket or suggestion (from bug 23590).

From svc/members/search:
 88 # It is not recommanded to use the has_permission param if you use the pagination
 89 # The filter is done AFTER requested the data

See also bug 12648.
Comment 1 Jonathan Druart 2020-03-25 10:31:22 UTC
Created attachment 101668 [details] [review]
Bug 24964: Add tests
Comment 2 Jonathan Druart 2020-03-25 10:31:25 UTC
Created attachment 101669 [details] [review]
Bug 24964: Do not filter patrons after they have been fetched

The svc/members/search script is called in different places.
In some places (Set owner for a fund, add users to a fund, or set a
manager to a suggestion), we need patrons to be filtered depending on
the permissions they have.
For instance you can only set a fund's owner with a patron that has
acquisition.order_manage.

Currently we have fetching X (default 20) patrons, then filter them
depending on their permission.
Says you have 3 patrons that have the correct permissions but are not in
the 20 first patrons, if you do not define a search term, the search
result will be empty.

This is not ideal and we should filter when requesting the DB.

Test plan:
- Have more than 20 patrons, remove them their permissions
- Create 3 more:
1 superlibrarian
1 with the full acq permission
1 with acquisition.order_manage
- Create a fund and set a owner
- Search for patrons, without specifying a search term (to get them all)
=> Without this patch the new patrons you created are not displayed
=> With this patch they are!

Same test plan apply to set a manager to a suggestion (freshly pushed,
see bug 23590), with suggestions and suggestions.suggestions_manage

Note: The code has been written that way to rely on
C4::Auth::haspermission, but the SQL query is quite trivial and the gain
is important.
Comment 3 Jonathan Druart 2020-03-25 11:37:03 UTC
Created attachment 101686 [details] [review]
Bug 24964: Do not filter patrons after they have been fetched

The svc/members/search script is called in different places.
In some places (Set owner for a fund, add users to a fund, or set a
manager to a suggestion), we need patrons to be filtered depending on
the permissions they have.
For instance you can only set a fund's owner with a patron that has
acquisition.order_manage.

Currently we have fetching X (default 20) patrons, then filter them
depending on their permission.
Says you have 3 patrons that have the correct permissions but are not in
the 20 first patrons, if you do not define a search term, the search
result will be empty.

This is not ideal and we should filter when requesting the DB.

Test plan:
- Have more than 20 patrons, remove them their permissions
- Create 3 more:
1 superlibrarian
1 with the full acq permission
1 with acquisition.order_manage
- Create a fund and set a owner
- Search for patrons, without specifying a search term (to get them all)
=> Without this patch the new patrons you created are not displayed
=> With this patch they are!

Same test plan apply to set a manager to a suggestion (freshly pushed,
see bug 23590), with suggestions and suggestions.suggestions_manage

Note: The code has been written that way to rely on
C4::Auth::haspermission, but the SQL query is quite trivial and the gain
is important.
Comment 4 Fridolin Somers 2020-03-25 12:02:16 UTC
In test :
+    is_deeply(
+        [ map { $_->{borrowernumber} } @{ $search_results->{patrons} } ],
+        [
+            $superlibrarian->borrowernumber,
+            $librarian_with_full_permission->borrowernumber,
+            $librarian_with_subpermission->borrowernumber
+        ],
+        'We got the 3 patrons we expected'
+    );

I get an error because $search_results->{patrons} may not be ordered like expected array.
Test is OK when adding sort on both arrays.
Comment 5 Jonathan Druart 2020-03-25 12:30:06 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2020-03-25 12:30:30 UTC
They should be returned depending on their borrowernumber. Which order do you get?
Comment 7 Fridolin Somers 2020-03-25 12:43:15 UTC
(In reply to Jonathan Druart from comment #6)
> They should be returned depending on their borrowernumber. Which order do
> you get?

By adding debug to t/db_dependent/Utils/Datatables_Members.t :
-        'We got the 3 patrons we expected'
+        'We got the 3 patrons we expected'.Data::Dumper::Dumper(map { $_->{borrowernumber} } @{ $search_results->{patrons} })

I get :
# $VAR1 = 7888;
# $VAR2 = 7886;
# $VAR3 = 7887;

I confirm failing is random.
Comment 8 Jonathan Druart 2020-03-25 12:51:19 UTC
Created attachment 101702 [details] [review]
Bug 24964: Prevent random failures sorting by borrowernumber
Comment 9 Fridolin Somers 2020-03-26 11:52:52 UTC
Created attachment 101813 [details] [review]
Bug 24964: Add tests

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 10 Fridolin Somers 2020-03-26 11:53:30 UTC
Created attachment 101814 [details] [review]
Bug 24964: Do not filter patrons after they have been fetched

The svc/members/search script is called in different places.
In some places (Set owner for a fund, add users to a fund, or set a
manager to a suggestion), we need patrons to be filtered depending on
the permissions they have.
For instance you can only set a fund's owner with a patron that has
acquisition.order_manage.

Currently we have fetching X (default 20) patrons, then filter them
depending on their permission.
Says you have 3 patrons that have the correct permissions but are not in
the 20 first patrons, if you do not define a search term, the search
result will be empty.

This is not ideal and we should filter when requesting the DB.

Test plan:
- Have more than 20 patrons, remove them their permissions
- Create 3 more:
1 superlibrarian
1 with the full acq permission
1 with acquisition.order_manage
- Create a fund and set a owner
- Search for patrons, without specifying a search term (to get them all)
=> Without this patch the new patrons you created are not displayed
=> With this patch they are!

Same test plan apply to set a manager to a suggestion (freshly pushed,
see bug 23590), with suggestions and suggestions.suggestions_manage

Note: The code has been written that way to rely on
C4::Auth::haspermission, but the SQL query is quite trivial and the gain
is important.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 11 Fridolin Somers 2020-03-26 11:53:48 UTC
Created attachment 101815 [details] [review]
Bug 24964: Prevent random failures sorting by borrowernumber

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 12 Kyle M Hall 2020-03-26 12:54:22 UTC
Created attachment 101820 [details] [review]
Bug 24964: Add tests

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2020-03-26 12:54:33 UTC
Created attachment 101821 [details] [review]
Bug 24964: Do not filter patrons after they have been fetched

The svc/members/search script is called in different places.
In some places (Set owner for a fund, add users to a fund, or set a
manager to a suggestion), we need patrons to be filtered depending on
the permissions they have.
For instance you can only set a fund's owner with a patron that has
acquisition.order_manage.

Currently we have fetching X (default 20) patrons, then filter them
depending on their permission.
Says you have 3 patrons that have the correct permissions but are not in
the 20 first patrons, if you do not define a search term, the search
result will be empty.

This is not ideal and we should filter when requesting the DB.

Test plan:
- Have more than 20 patrons, remove them their permissions
- Create 3 more:
1 superlibrarian
1 with the full acq permission
1 with acquisition.order_manage
- Create a fund and set a owner
- Search for patrons, without specifying a search term (to get them all)
=> Without this patch the new patrons you created are not displayed
=> With this patch they are!

Same test plan apply to set a manager to a suggestion (freshly pushed,
see bug 23590), with suggestions and suggestions.suggestions_manage

Note: The code has been written that way to rely on
C4::Auth::haspermission, but the SQL query is quite trivial and the gain
is important.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2020-03-26 12:54:36 UTC
Created attachment 101822 [details] [review]
Bug 24964: Prevent random failures sorting by borrowernumber

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2020-03-26 12:54:39 UTC
Created attachment 101823 [details] [review]
Bug 24964: Fix perl critic warning

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Martin Renvoize 2020-03-27 08:38:42 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 17 Joy Nelson 2020-04-03 22:41:53 UTC
does not apply to 19.11.x branch.  not backported
please rebase if needed.
Comment 18 Jonathan Druart 2020-04-06 09:49:00 UTC
Created attachment 102434 [details] [review]
Bug 24964: Do not fetch all patrons for owner/users of funds

admin/add_user_search.pl sets 'patrons_with_acq_perm_only', to only
retrieve patrons with acq (order_manage) permissions.

The original bug was hidden in the existing code, but the behavior was
terrible.
To prevent patrons matching the permissions contrainst to be hidden, we
fetched ALL the patrons from the DB, then filtered them depending on
their permissions. This might have awful performance on DB with lot of
patrons.

So basically with this patch we are adding the pagination back to the
"owner/users for funds" feature, as the permission filtering is now done
at DB level.
Comment 19 Jonathan Druart 2020-04-06 09:50:12 UTC
(In reply to Joy Nelson from comment #17)
> does not apply to 19.11.x branch.  not backported
> please rebase if needed.

I found something when rebasing this for 19.11. I will wait for the last follow-up to be pushed to master before submitting the rebased patches.
Comment 20 Jonathan Druart 2020-04-06 09:54:31 UTC
Created attachment 102435 [details] [review]
Bug 24964: Do not fetch all patrons for owner/users of funds

admin/add_user_search.pl sets 'patrons_with_acq_perm_only', to only
retrieve patrons with acq (order_manage) permissions.

The original bug was hidden in the existing code, but the behavior was
terrible.
To prevent patrons matching the permissions contrainst to be hidden, we
fetched ALL the patrons from the DB, then filtered them depending on
their permissions. This might have awful performance on DB with lot of
patrons.

So basically with this patch we are adding the pagination back to the
"owner/users for funds" feature, as the permission filtering is now done
at DB level.
Comment 21 Martin Renvoize 2020-04-06 09:58:16 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 22 Jonathan Druart 2020-04-06 14:56:06 UTC
Created attachment 102457 [details] [review]
Bug 24964: [19.11.x] Add tests

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Jonathan Druart 2020-04-06 14:56:09 UTC
Created attachment 102458 [details] [review]
Bug 24964: [19.11.x] Do not filter patrons after they have been fetched

The svc/members/search script is called in different places.
In some places (Set owner for a fund, add users to a fund, or set a
manager to a suggestion), we need patrons to be filtered depending on
the permissions they have.
For instance you can only set a fund's owner with a patron that has
acquisition.order_manage.

Currently we have fetching X (default 20) patrons, then filter them
depending on their permission.
Says you have 3 patrons that have the correct permissions but are not in
the 20 first patrons, if you do not define a search term, the search
result will be empty.

This is not ideal and we should filter when requesting the DB.

Test plan:
- Have more than 20 patrons, remove them their permissions
- Create 3 more:
1 superlibrarian
1 with the full acq permission
1 with acquisition.order_manage
- Create a fund and set a owner
- Search for patrons, without specifying a search term (to get them all)
=> Without this patch the new patrons you created are not displayed
=> With this patch they are!

Same test plan apply to set a manager to a suggestion (freshly pushed,
see bug 23590), with suggestions and suggestions.suggestions_manage

Note: The code has been written that way to rely on
C4::Auth::haspermission, but the SQL query is quite trivial and the gain
is important.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Jonathan Druart 2020-04-06 14:56:13 UTC
Created attachment 102459 [details] [review]
Bug 24964: [19.11.x] Prevent random failures sorting by borrowernumber

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Jonathan Druart 2020-04-06 14:56:16 UTC
Created attachment 102460 [details] [review]
Bug 24964: [19.11.x] Fix perl critic warning

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Jonathan Druart 2020-04-06 14:56:20 UTC
Created attachment 102461 [details] [review]
Bug 24964: [19.11.x] Do not fetch all patrons for owner/users of funds

admin/add_user_search.pl sets 'patrons_with_acq_perm_only', to only
retrieve patrons with acq (order_manage) permissions.

The original bug was hidden in the existing code, but the behavior was
terrible.
To prevent patrons matching the permissions contrainst to be hidden, we
fetched ALL the patrons from the DB, then filtered them depending on
their permissions. This might have awful performance on DB with lot of
patrons.

So basically with this patch we are adding the pagination back to the
"owner/users for funds" feature, as the permission filtering is now done
at DB level.
Comment 27 Jonathan Druart 2020-04-06 14:57:58 UTC
(In reply to Joy Nelson from comment #17)
> does not apply to 19.11.x branch.  not backported
> please rebase if needed.

Patches rebased for 19.11.x. Please give them a try before pushing them.
Let me know if you have any questions!
Comment 28 Joy Nelson 2020-04-29 16:02:51 UTC
Thanks for the backport!  Applied and tested it works on 19.11.x
Comment 29 Lucas Gass 2020-05-11 21:04:06 UTC
Rebase for 19.11 also applies clean to 19.05. Backported to 19.05.x for 19.05.11