Attachment 102458 Details for Bug 24964 – Bug 24964: [19.11.x] Do not filter patrons after they have been fetched

[patch] Bug 24964: [19.11.x] Do not filter patrons after they have been fetched

Bug-24964-1911x-Do-not-filter-patrons-after-they-h.patch (text/plain), 7.42 KB, created by Jonathan Druart on 2020-04-06 14:56:09 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2020-04-06 14:56:09 UTC

Size: 7.42 KB

patch

obsolete

>From 6cba8aea88644a7b3bc3c4fe9ed81787b293744d Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Wed, 25 Mar 2020 11:21:15 +0100
>Subject: [PATCH] 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>
>---
> C4/Utils/DataTables/Members.pm | 43 ++++++++++++++++++++++++++++------
> svc/members/search             | 30 +++++-------------------
> 2 files changed, 42 insertions(+), 31 deletions(-)
>
>diff --git a/C4/Utils/DataTables/Members.pm b/C4/Utils/DataTables/Members.pm
>index 9fb539e4f1..7f298d8fa0 100644
>--- a/C4/Utils/DataTables/Members.pm
>+++ b/C4/Utils/DataTables/Members.pm
>@@ -14,6 +14,7 @@ sub search {
>     my $branchcode = $params->{branchcode};
>     my $searchtype = $params->{searchtype} || 'contain';
>     my $searchfieldstype = $params->{searchfieldstype} || 'standard';
>+    my $has_permission = $params->{has_permission};
>     my $dt_params = $params->{dt_params};
> 
>     unless ( $searchmember ) {
>@@ -28,12 +29,31 @@ sub search {
> 
>     my ($sth, $query, $iTotalQuery, $iTotalRecords, $iTotalDisplayRecords);
>     my $dbh = C4::Context->dbh;
>+
>+    # Get the module_bit from a given permission code
>+    if ( $has_permission ) {
>+        ($has_permission->{module_bit}) = $dbh->selectrow_array(q|
>+            SELECT bit FROM userflags WHERE flag=?
>+        |, undef, $has_permission->{permission});
>+    }
>+
>     # Get the iTotalRecords DataTable variable
>-    $query = $iTotalQuery = "SELECT COUNT(borrowers.borrowernumber) FROM borrowers";
>+    $iTotalQuery = "SELECT COUNT(borrowers.borrowernumber) FROM borrowers";
>+    if ( $has_permission ) {
>+        $iTotalQuery .= ' LEFT JOIN user_permissions on borrowers.borrowernumber=user_permissions.borrowernumber';
>+    }
>+
>+    my (@where, @conditions);
>     if ( @restricted_branchcodes ) {
>-        $iTotalQuery .= " WHERE borrowers.branchcode IN (" . join( ',', ('?') x @restricted_branchcodes ) . ")";
>+        push @where, "borrowers.branchcode IN (" . join( ',', ('?') x @restricted_branchcodes ) . ")";
>+        push @conditions, @restricted_branchcodes;
>     }
>-    ($iTotalRecords) = $dbh->selectrow_array( $iTotalQuery, undef, @restricted_branchcodes );
>+    if ( $has_permission ) {
>+        push @where, '( borrowers.flags = 1 OR borrowers.flags & (1 << ?) OR module_bit=? AND code=? )';
>+        push @conditions, ($has_permission->{module_bit}) x 2, $has_permission->{subpermission};
>+    }
>+    $iTotalQuery .= ' WHERE ' . join ' AND ', @where if @where;
>+    ($iTotalRecords) = $dbh->selectrow_array( $iTotalQuery, undef, @conditions );
> 
>     # Do that after iTotalQuery!
>     if ( defined $branchcode and $branchcode ) {
>@@ -58,6 +78,7 @@ sub search {
> 
>     my $select = "SELECT
>         borrowers.borrowernumber, borrowers.surname, borrowers.firstname,
>+        borrowers.flags,
>         borrowers.streetnumber, borrowers.streettype, borrowers.address,
>         borrowers.address2, borrowers.city, borrowers.state, borrowers.zipcode,
>         borrowers.country, cardnumber, borrowers.dateexpiry,
>@@ -66,8 +87,12 @@ sub search {
>         categories.description AS category_description, categories.category_type,
>         branches.branchname, borrowers.phone";
>     my $from = "FROM borrowers
>-        LEFT JOIN branches ON borrowers.branchcode = branches.branchcode
>-        LEFT JOIN categories ON borrowers.categorycode = categories.categorycode";
>+                LEFT JOIN branches ON borrowers.branchcode = branches.branchcode
>+                LEFT JOIN categories ON borrowers.categorycode = categories.categorycode";
>+    if ( $has_permission ) {
>+        $from .= '
>+                LEFT JOIN user_permissions on borrowers.borrowernumber=user_permissions.borrowernumber';
>+    }
>     my @where_args;
>     my @where_strs;
>     if(defined $firstletter and $firstletter ne '') {
>@@ -142,8 +167,12 @@ sub search {
>             if @where_strs_or;
>     }
> 
>-    my $where;
>-    $where = " WHERE " . join (" AND ", @where_strs) if @where_strs;
>+    if ( $has_permission ) {
>+        push @where_strs, '( borrowers.flags = 1 OR borrowers.flags & (1 << ?) OR module_bit=? AND code=? )';
>+        push @where_args, ($has_permission->{module_bit}) x 2, $has_permission->{subpermission};
>+    }
>+
>+    my $where = " WHERE " . join (" AND ", @where_strs) if @where_strs;
>     my $orderby = dt_build_orderby($dt_params);
> 
>     my $limit;
>diff --git a/svc/members/search b/svc/members/search
>index abb57164ce..27c0f675ad 100755
>--- a/svc/members/search
>+++ b/svc/members/search
>@@ -72,6 +72,11 @@ if ( $searchmember
>     } if $member;
> }
> 
>+if ($has_permission) {
>+    my ( $permission, $subpermission ) = split /\./, $has_permission;
>+    $has_permission = {permission => $permission, subpermission => $subpermission};
>+}
>+
> # Perform the patrons search
> $results = C4::Utils::DataTables::Members::search(
>     {
>@@ -82,33 +87,10 @@ $results = C4::Utils::DataTables::Members::search(
>         searchtype => $searchtype,
>         searchfieldstype => $searchfieldstype,
>         dt_params => \%dt_params,
>+        ( $has_permission ? ( has_permission => $has_permission ) : () ),
>     }
> ) unless $results;
> 
>-# It is not recommanded to use the has_permission param if you use the pagination
>-# The filter is done AFTER requested the data
>-if ($has_permission) {
>-    my ( $permission, $subpermission ) = split /\./, $has_permission;
>-    my @patrons_with_permission;
>-    for my $patron ( @{ $results->{patrons} } ) {
>-        my $perms = haspermission( $patron->{userid}, undef );
>-        if (   $perms->{superlibrarian} == 1
>-            or $perms->{$permission} == 1 )
>-        {
>-            push @patrons_with_permission, $patron;
>-            next;
>-        }
>-
>-        if ($subpermission) {
>-            my $subperms = get_user_subpermissions( $patron->{userid} );
>-            push @patrons_with_permission, $patron
>-              if $subperms->{$permission}->{$subpermission};
>-        }
>-    }
>-    $results->{patrons} = \@patrons_with_permission;
>-    $results->{iTotalDisplayRecords} = scalar( @patrons_with_permission );
>-}
>-
> $template->param(
>     sEcho => $sEcho,
>     iTotalRecords => $results->{iTotalRecords},
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 24964: 101668 | 101669 | 101686 | 101702 | 101813 | 101814 | 101815 | 101820 | 101821 | 101822 | 101823 | 102434 | 102435 | 102457 | 102458 | 102459 | 102460 | 102461