|
Description
Jonathan Druart
2020-03-24 11:24:07 UTC
Created attachment 101668 [details] [review] Bug 24964: Add tests 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. 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. 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.
they should be returned depending on the bug number. Which order do you get? They should be returned depending on their borrowernumber. Which order do you get? (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. Created attachment 101702 [details] [review] Bug 24964: Prevent random failures sorting by borrowernumber Created attachment 101813 [details] [review] Bug 24964: Add tests Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> 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> Created attachment 101815 [details] [review] Bug 24964: Prevent random failures sorting by borrowernumber Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> 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> 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> 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> Created attachment 101823 [details] [review] Bug 24964: Fix perl critic warning Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Nice work everyone! Pushed to master for 20.05 does not apply to 19.11.x branch. not backported please rebase if needed. 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. (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. 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. Nice work everyone! Pushed to master for 20.05 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> 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> 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> Created attachment 102460 [details] [review] Bug 24964: [19.11.x] Fix perl critic warning Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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. (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! Thanks for the backport! Applied and tested it works on 19.11.x Rebase for 19.11 also applies clean to 19.05. Backported to 19.05.x for 19.05.11 |