Bug 20443: Move GetBorrowerAttributeValue to Koha::Patron->get_extended_attribute_value

We want to retrieve a specific patron's attribute for a given patron.
We then add a new method to Koha::Patron.

This patch add a getter method ->get_extended_attribute_value
to use the DBIx::Class relation

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm
index cf9051d..f2e78c9 100644 (file)
--- a/C4/Members/Attributes.pm
+++ b/C4/Members/Attributes.pm
@@ -29,7 +29,7 @@ our ($csv, $AttributeTypes);
 
 BEGIN {
     @ISA = qw(Exporter);
-    @EXPORT_OK = qw(GetBorrowerAttributes GetBorrowerAttributeValue CheckUniqueness SetBorrowerAttributes
+    @EXPORT_OK = qw(GetBorrowerAttributes CheckUniqueness SetBorrowerAttributes
                     DeleteBorrowerAttribute UpdateBorrowerAttribute
                     extended_attributes_code_value_arrayref extended_attributes_merge
                     SearchIdMatchingAttribute);
@@ -95,28 +95,6 @@ sub GetBorrowerAttributes {
     return \@results;
 }
 
-=head2 GetBorrowerAttributeValue
-
-  my $value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, $attribute_code);
-
-Retrieve the value of an extended attribute C<$attribute_code> associated with the
-patron specified by C<$borrowernumber>.
-
-=cut
-
-sub GetBorrowerAttributeValue {
-    my $borrowernumber = shift;
-    my $code = shift;
-
-    my $dbh = C4::Context->dbh();
-    my $query = "SELECT attribute
-                 FROM borrower_attributes
-                 WHERE borrowernumber = ?
-                 AND code = ?";
-    my $value = $dbh->selectrow_array($query, undef, $borrowernumber, $code);
-    return $value;
-}
-
 =head2 SearchIdMatchingAttribute
 
   my $matching_borrowernumbers = C4::Members::Attributes::SearchIdMatchingAttribute($filter);

diff --git a/Koha/Patron.pm b/Koha/Patron.pm
index 8b8b6d5..c8d0981 100644 (file)
--- a/Koha/Patron.pm
+++ b/Koha/Patron.pm
@@ -1557,6 +1557,30 @@ sub add_guarantor {
     )->store();
 }
 
+=head3 get_extended_attribute_value
+
+my $attribute_value = $patron->get_extended_attribute_value( $code );
+
+Return the attribute's value for the code passed in parameter.
+
+It not exist it returns undef
+
+Note that this will not work for repeatable attribute types.
+
+Maybe you certainly not want to use this method, it is actually only used for SHOW_BARCODE
+(which should be a real patron's attribute (not extended)
+
+=cut
+
+sub get_extended_attribute_value {
+    my ( $self, $code ) = @_;
+    my $rs = $self->_result->borrower_attributes;
+    return unless $rs;
+    my $attribute = $rs->search( { code => $code } );
+    return unless $attribute->count;
+    return $attribute->next->attribute;
+}
+
 =head3 to_api
 
     my $json = $patron->to_api;

diff --git a/opac/opac-user.pl b/opac/opac-user.pl
index 96e9bf4..c360363 100755 (executable)
--- a/opac/opac-user.pl
+++ b/opac/opac-user.pl
@@ -28,7 +28,6 @@ use C4::External::BakerTaylor qw( image_url link_url );
 use C4::Reserves;
 use C4::Members;
 use C4::Members::AttributeTypes;
-use C4::Members::Attributes qw/GetBorrowerAttributeValue/;
 use C4::Output;
 use C4::Biblio;
 use C4::Items;
@@ -287,10 +286,10 @@ $template->param( canrenew     => $canrenew );
 $template->param( OVERDUES       => \@overdues );
 $template->param( overdues_count => $overdues_count );
 
-my $show_barcode = Koha::Patron::Attribute::Types->search(
+my $show_barcode = Koha::Patron::Attribute::Types->search( # FIXME we should not need this search
     { code => ATTRIBUTE_SHOW_BARCODE } )->count;
 if ($show_barcode) {
-    my $patron_show_barcode = GetBorrowerAttributeValue($borrowernumber, ATTRIBUTE_SHOW_BARCODE);
+    my $patron_show_barcode = $patron->get_extended_attribute_value(ATTRIBUTE_SHOW_BARCODE);
     undef $show_barcode if defined($patron_show_barcode) && !$patron_show_barcode;
 }
 $template->param( show_barcode => 1 ) if $show_barcode;

diff --git a/t/db_dependent/Auth_with_ldap.t b/t/db_dependent/Auth_with_ldap.t
index e0c3c45..f267a07 100755 (executable)
--- a/t/db_dependent/Auth_with_ldap.t
+++ b/t/db_dependent/Auth_with_ldap.t
@@ -117,6 +117,8 @@ $builder->build(
     }
 );
 
+my $patron = Koha::Patrons->find($borrower->{borrowernumber});
+
 # C4::Auth_with_ldap needs several stuff set first ^^^
 use_ok('C4::Auth_with_ldap');
 can_ok(
@@ -212,13 +214,13 @@ subtest 'checkpw_ldap tests' => sub {
             'Extended attributes are not deleted'
         );
 
-        is( C4::Members::Attributes::GetBorrowerAttributeValue($borrower->{borrowernumber}, $attr_type2->{code}), 'BAR', 'Mapped attribute is BAR' );
+        is( $patron->get_extended_attribute_value( $attr_type2->{code} ), 'BAR', 'Mapped attribute is BAR' );
         $auth->unmock('update_local');
         $auth->unmock('ldap_entry_2_hash');
 
         $update               = 0;
         $desired_count_result = 0;    # user auth problem
-        Koha::Patrons->find( $borrower->{borrowernumber} )->delete;
+        $patron->delete;
         reload_ldap_module();
         is(
             C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola', password => 'hey' ),

diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
index 662b852..f02611d 100644 (file)
--- a/t/db_dependent/Koha/Patrons.t
+++ b/t/db_dependent/Koha/Patrons.t
@@ -2018,3 +2018,79 @@ subtest 'anonymize' => sub {
     is( $patron2->firstname, undef, 'First name patron2 cleared' );
 };
 $schema->storage->txn_rollback;
+
+subtest 'get_extended_attributes' => sub {
+    plan tests => 7;
+    my $schema = Koha::Database->new->schema;
+    $schema->storage->txn_begin;
+
+    my $patron_1 = $builder->build_object({class=> 'Koha::Patrons'});
+    my $patron_2 = $builder->build_object({class=> 'Koha::Patrons'});
+
+    set_logged_in_user($patron_1);
+
+    my $attribute_type1 = C4::Members::AttributeTypes->new('my code1', 'my description1');
+    $attribute_type1->unique_id(1);
+    $attribute_type1->store();
+
+    my $attribute_type2 = C4::Members::AttributeTypes->new('my code2', 'my description2');
+    $attribute_type2->opac_display(1);
+    $attribute_type2->staff_searchable(1);
+    $attribute_type2->store();
+
+    my $new_library = $builder->build( { source => 'Branch' } );
+    my $attribute_type_limited = C4::Members::AttributeTypes->new('my code3', 'my description3');
+    $attribute_type_limited->branches([ $new_library->{branchcode} ]);
+    $attribute_type_limited->store;
+
+    my $attributes_for_1 = [
+        {
+            value => 'my attribute1',
+            code => $attribute_type1->code(),
+        },
+        {
+            value => 'my attribute2',
+            code => $attribute_type2->code(),
+        },
+        {
+            value => 'my attribute limited',
+            code => $attribute_type_limited->code(),
+        }
+    ];
+
+    my $attributes_for_2 = [
+        {
+            value => 'my attribute12',
+            code => $attribute_type1->code(),
+        },
+        {
+            value => 'my attribute limited 2',
+            code => $attribute_type_limited->code(),
+        }
+    ];
+
+    my $extended_attributes = $patron_1->get_extended_attributes;
+    is( ref($extended_attributes), 'Koha::Patron::Attributes', 'Koha::Patron->get_extended_attributes must return a Koha::Patron::Attribute set' );
+    is( $extended_attributes->count, 0, 'There should not be attribute yet');
+
+    C4::Members::Attributes::SetBorrowerAttributes($patron_1->borrowernumber, $attributes_for_1);
+    C4::Members::Attributes::SetBorrowerAttributes($patron_2->borrowernumber, $attributes_for_2);
+
+    my $extended_attributes_for_1 = $patron_1->get_extended_attributes;
+    is( $extended_attributes_for_1->count, 3, 'There should be 3 attributes now for patron 1');
+
+    my $extended_attributes_for_2 = $patron_2->get_extended_attributes;
+    is( $extended_attributes_for_2->count, 2, 'There should be 2 attributes now for patron 2');
+
+    my $attribute_12 = $extended_attributes_for_2->search({ code => $attribute_type1->code });
+    is( $attribute_12->next->attribute, 'my attribute12', 'search by code should return the correct attribute' );
+
+    $attribute_12 = $patron_2->get_extended_attribute_value( $attribute_type1->code );
+    is( $attribute_12, 'my attribute12', 'Koha::Patron->get_extended_attribute_value should return the correct attribute value' );
+
+    # TODO - What about multiple?
+    my $non_existent = $patron_2->get_extended_attribute_value( 'not_exist' );
+    is( $non_existent, undef, 'Koha::Patron->get_extended_attribute_value must return undef if the attribute does not exist' );
+
+    $schema->storage->txn_rollback;
+};

diff --git a/t/db_dependent/Members/Attributes.t b/t/db_dependent/Members/Attributes.t
index b30291d..775a28d 100644 (file)
--- a/t/db_dependent/Members/Attributes.t
+++ b/t/db_dependent/Members/Attributes.t
@@ -26,7 +26,7 @@ use Koha::Database;
 use t::lib::TestBuilder;
 use t::lib::Mocks;
 
-use Test::More tests => 48;
+use Test::More tests => 46;
 
 use_ok('C4::Members::Attributes');
 
@@ -71,9 +71,11 @@ $attribute_type_limited->branches([ $new_library->{branchcode} ]);
 $attribute_type_limited->store;
 
 my $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes();
-is( @$borrower_attributes, 0, 'GetBorrowerAttributes without the borrower number returns an empty array' );
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 0, 'GetBorrowerAttributes returns the correct number of borrower attributes' );
+ok(1);
+#is( @$borrower_attributes, 0, 'GetBorrowerAttributes without the borrower number returns an empty array' );
+$patron = Koha::Patrons->find($borrowernumber);
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 0, 'GetBorrowerAttributes returns the correct number of borrower attributes' );
 
 my $attributes = [
     {
@@ -131,19 +133,17 @@ is( @$borrower_attributes, 3, 'SetBorrowerAttributes should not have removed the
 #$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber, undef, 'branch_limited');
 #is( @$borrower_attributes, 2, 'GetBorrowerAttributes returns the correct number of borrower attributes filtered on library' );
 
-my $attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue();
-is( $attribute_value, undef, 'GetBorrowerAttributeValue without arguments returns undef' );
-$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber);
-is( $attribute_value, undef, 'GetBorrowerAttributeValue without the attribute code returns undef' );
-$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue(undef, $attributes->[0]->{code});
-is( $attribute_value, undef, 'GetBorrowerAttributeValue with a undef borrower number returns undef' );
-$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, 'my invalid code');
-is( $attribute_value, undef, 'GetBorrowerAttributeValue with an invalid code retuns undef' );
+$patron = Koha::Patrons->find($borrowernumber);
+my $extended_attributes = $patron->get_extended_attributes;
+my $attribute_value = $extended_attributes->search({ code => 'my invalid code' });
+is( $attribute_value->count, 0, 'non existent attribute should return empty result set');
+$attribute_value = $patron->get_extended_attribute_value('my invalid code');
+is( $attribute_value, undef, 'non existent attribute should undef');
 
-$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, $attributes->[0]->{code});
-is( $attribute_value, $attributes->[0]->{value}, 'GetBorrowerAttributeValue returns the correct attribute value' );
-$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, $attributes->[1]->{code});
-is( $attribute_value, $attributes->[1]->{value}, 'GetBorrowerAttributeValue returns the correct attribute value' );
+$attribute_value = $patron->get_extended_attribute_value($attributes->[0]->{code});
+is( $attribute_value, $attributes->[0]->{value}, 'get_extended_attribute_value returns the correct attribute value' );
+$attribute_value = $patron->get_extended_attribute_value($attributes->[1]->{code});
+is( $attribute_value, $attributes->[1]->{value}, 'get_extended_attribute_value returns the correct attribute value' );
 
 
 my $attribute = {
@@ -166,7 +166,6 @@ $check_uniqueness = C4::Members::Attributes::CheckUniqueness(undef, $attribute->
 is( $check_uniqueness, 0, 'CheckUniqueness without the argument code returns false' );
 $check_uniqueness = C4::Members::Attributes::CheckUniqueness('my invalid code');
 is( $check_uniqueness, 0, 'CheckUniqueness with an invalid argument code returns false' );
-$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, $attributes->[1]->{code});
 $check_uniqueness = C4::Members::Attributes::CheckUniqueness('my invalid code', $attribute->{attribute});
 is( $check_uniqueness, 0, 'CheckUniqueness with an invalid argument code returns fale' );
 $check_uniqueness = C4::Members::Attributes::CheckUniqueness($attribute->{code}, 'new value');