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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

diff --git a/C4/Utils/DataTables/Members.pm b/C4/Utils/DataTables/Members.pm
index b7887a8..d2b4089 100644 (file)
--- a/C4/Utils/DataTables/Members.pm
+++ b/C4/Utils/DataTables/Members.pm
@@ -13,6 +13,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 ) {
@@ -27,12 +28,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 ) {
@@ -57,6 +77,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,
@@ -65,8 +86,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 2d73986..76c5749 100755 (executable)
--- 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,22 +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} } ) {
-        push @patrons_with_permission, $patron
-            if haspermission( $patron->{userid}, { $permission => $subpermission } );
-    }
-    $results->{patrons} = \@patrons_with_permission;
-    $results->{iTotalDisplayRecords} = scalar( @patrons_with_permission );
-}
-
 $template->param(
     sEcho => $sEcho,
     iTotalRecords => $results->{iTotalRecords},