From 52949f88e0110e52b965bedbdcb885bdb5b00eaf Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Fri, 13 Jul 2018 12:10:47 -0300
Subject: [PATCH] Bug 20443: Improve filtering method

Koha::Patron::Attributes->search mimicks what is done in
Koha::AuthorisedValues->search.
But actually it should be more explicit when the caller use it.
For instance filter_by_branch_limitation (see discussion on bug 11983).

This will be useful for the following patches as we will need a way to
replace the $no_branch_limit flag.
When the $no_branch_limit flag is called, a simple ->search call should
be done.
When we want to limit on a specific library we can pass the branchcode
in paramter of filter_by_branch_limitation (this is not used yet).
If not passed the logged-in user library will be used by default.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
---
 Koha/Patron/Attributes.pm     | 67 +++++++++++++++++++++++------------
 t/db_dependent/Koha/Patrons.t | 17 ++++++---
 2 files changed, 58 insertions(+), 26 deletions(-)

diff --git a/Koha/Patron/Attributes.pm b/Koha/Patron/Attributes.pm
index 0cb6875daf..f7289a0be0 100644
--- a/Koha/Patron/Attributes.pm
+++ b/Koha/Patron/Attributes.pm
@@ -18,6 +18,7 @@ package Koha::Patron::Attributes;
 use Modern::Perl;
 
 use Koha::Patron::Attribute;
+use Koha::Patron::Attribute::Types;
 
 use base qw(Koha::Objects);
 
@@ -33,35 +34,57 @@ Koha::Patron::Attributes - Koha Patron Attributes Object set class
 
 =head3 search
 
-my $attributes-> Koha::Patron::Attributes->search( $params );
+my $attributes = Koha::Patron::Attributes->search( $params );
 
 =cut
 
 sub search {
     my ( $self, $params, $attributes ) = @_;
 
-    my $branchcode = $params->{branchcode};
-    delete( $params->{branchcode} );
-
-    my $or =
-        $branchcode
-        ? {
-            '-or' => [
-                'borrower_attribute_types_branches.b_branchcode' => undef,
-                'borrower_attribute_types_branches.b_branchcode' => $branchcode,
-            ]
-        } : {};
-
-    my $join =
-        $branchcode
-        ? {
-            join => {
-                'code' => 'borrower_attribute_types_branches'
-            },
-        } : {};
-    $attributes //= {};
     unless ( exists $attributes->{order_by} ) { $attributes->{order_by} = ['code', 'attribute'] }
-    return $self->SUPER::search( { %$params, %$or }, { %$attributes, %$join } );
+
+    return $self->SUPER::search( $params, $attributes );
+}
+
+=head3 filter_by_branch_limitations
+
+my $attributes = Koha::Patron::Attributes->filter_by_branch_limitations([$branchcode]);
+
+Search patron attributes filtered by a library
+
+If $branchcode exists it will be used to filter the result set.
+
+Otherwise it will be the library of the logged in user.
+
+=cut
+
+sub filter_by_branch_limitations {
+    my ( $self, $branchcode ) = @_;
+
+    # Maybe we should not limit if logged in user is superlibrarian?
+    my $branch_limit =
+        $branchcode          ? $branchcode
+        # Do we raise an exception if no userenv defined?
+      : C4::Context->userenv ? C4::Context->userenv->{"branch"}
+      :                        undef;
+
+    my $or = $branch_limit
+      ? {
+        '-or' => [
+            'borrower_attribute_types_branches.b_branchcode' => undef,
+            'borrower_attribute_types_branches.b_branchcode' => $branch_limit,
+        ]
+      }
+      : {};
+
+    my $join = $branch_limit
+      ? {
+        join => {
+            code => 'borrower_attribute_types_branches'
+        },
+      }
+      : {};
+    return $self->search( $or, $join );
 }
 
 =head3 _type
diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
index 146bdbfb95..daee4f9b89 100644
--- a/t/db_dependent/Koha/Patrons.t
+++ b/t/db_dependent/Koha/Patrons.t
@@ -2020,7 +2020,7 @@ subtest 'anonymize' => sub {
 $schema->storage->txn_rollback;
 
 subtest 'get_extended_attributes' => sub {
-    plan tests => 9;
+    plan tests => 10;
     my $schema = Koha::Database->new->schema;
     $schema->storage->txn_begin;
 
@@ -2094,12 +2094,21 @@ subtest 'get_extended_attributes' => sub {
 
     # Test branch limitations
     set_logged_in_user($patron_2);
-    C4::Members::Attributes::SetBorrowerAttributes($patron_1->borrowernumber, $attributes_for_1);
+    # Return all
     $extended_attributes_for_1 = $patron_1->get_extended_attributes;
-    is( $extended_attributes_for_1->count, 2, 'There should be 2 attributes for patron 1, the limited one should not be returned');
+    is( $extended_attributes_for_1->count, 3, 'There should be 2 attributes for patron 1, the limited one should be returned');
 
+    # Return filtered
+    $extended_attributes_for_1 = $patron_1->get_extended_attributes->filter_by_branch_limitations;
+    is( $extended_attributes_for_1->count, 2, 'There should be 2 attributes for patron 1, the limited one should be returned');
+
+    # Not filtered
     my $limited_value = $patron_1->get_extended_attribute_value( $attribute_type_limited->code );
-    is( $limited_value, undef, );
+    is( $limited_value, 'my attribute limited', );
+
+    ## Do we need a filtered?
+    #$limited_value = $patron_1->get_extended_attribute_value( $attribute_type_limited->code );
+    #is( $limited_value, undef, );
 
     $schema->storage->txn_rollback;
 };
-- 
2.39.5