Bug 20443: Move GetBorrowerAttributes to Koha::Patron->extended_attributes

The GetBorrowerAttributes subroutine return the attributes for a given
patron.

Using get_extended_attributes we can acchieve it easily. The problematic
here is to restore the method's name (value vs attribute,
value_description vs description of the authorised value, as well as
display_checkout that should not be a method of Attribute, but
Attribute::Type instead)

value_description was used when the attribute types were attached to an
authorised value category. To avoid the necessary test in template and
controller there is now a $attribute->description method that will
display either the attribute's value OR the value of the authorised
value when needed. We should certainly use this one from few other
places.

Notes:
* This patch rename Koha::Patron->attributes with Koha::Patron->get_extended_attributes.
It will be renamed with Koha::Patron->extended_attributes in ones of the next
patches when it will become a setter as well.
* GetBorrowerAttributes did not care about the library limits, we still
do not
* The opac_only flag was not used outside of test, we drop it off.
* To maintain the existing behavior we add a default order-by clause to
the search method [code, attribute]
* From C4::Letters::_parseletter we always display the staff description
of the AV, There is now a FIXME to warn about it
* FIXMEs are not regressions, existing behaviors must be kept
* TODO add a new check to bug 21010 to search for inconsistencies in
patron's attributes attached to non-existent authorised values
* One test has been updated in Modifications.t, order_by is now
by default set to ['code', 'attribute']

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

C4/Letters.pm

@@ -28,7 +28,6 @@ use Template;
 use Module::Load::Conditional qw(can_load);
 
 use C4::Members;
-use C4::Members::Attributes qw(GetBorrowerAttributes);
 use C4::Log;
 use C4::SMS;
 use C4::Debug;
@@ -852,11 +851,13 @@ sub _parseletter {
     }
 
     if ($table eq 'borrowers' && $letter->{content}) {
-        if ( my $attributes = GetBorrowerAttributes($values->{borrowernumber}) ) {
+        my $patron = Koha::Patrons->find( $values->{borrowernumber} );
+        if ( $patron ) {
+            my $attributes = $patron->get_extended_attributes;
             my %attr;
-            foreach (@$attributes) {
-                my $code = $_->{code};
-                my $val  = $_->{value_description} || $_->{value};
+            while ( my $attribute = $attributes->next ) {
+                my $code = $attribute->code;
+                my $val  = $attribute->description; # FIXME - we always display intranet description here!
                 $val =~ s/\p{P}(?=$)//g if $val;
                 next unless $val gt '';
                 $attr{$code} ||= [];

C4/Members/Attributes.pm

@@ -29,7 +29,7 @@ our ($csv, $AttributeTypes);
 
 BEGIN {
     @ISA = qw(Exporter);
-    @EXPORT_OK = qw(GetBorrowerAttributes CheckUniqueness SetBorrowerAttributes
+    @EXPORT_OK = qw(CheckUniqueness SetBorrowerAttributes
                     DeleteBorrowerAttribute UpdateBorrowerAttribute
                     extended_attributes_code_value_arrayref extended_attributes_merge
                     SearchIdMatchingAttribute);
@@ -43,58 +43,9 @@ C4::Members::Attributes - manage extend patron attributes
 =head1 SYNOPSIS
 
   use C4::Members::Attributes;
-  my $attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
 
 =head1 FUNCTIONS
 
-=head2 GetBorrowerAttributes
-
-  my $attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber[, $opac_only]);
-
-Retrieve an arrayref of extended attributes associated with the
-patron specified by C<$borrowernumber>.  Each entry in the arrayref
-is a hashref containing the following keys:
-
-code (attribute type code)
-description (attribute type description)
-value (attribute value)
-value_description (attribute value description (if associated with an authorised value))
-
-If the C<$opac_only> parameter is present and has a true value, only the attributes
-marked for OPAC display are returned.
-
-=cut
-
-sub GetBorrowerAttributes {
-    my $borrowernumber = shift;
-    my $opac_only = @_ ? shift : 0;
-
-    my $dbh = C4::Context->dbh();
-    my $query = "SELECT code, description, attribute, lib, display_checkout, category_code, class
-                 FROM borrower_attributes
-                 JOIN borrower_attribute_types USING (code)
-                 LEFT JOIN authorised_values ON (category = authorised_value_category AND attribute = authorised_value)
-                 WHERE borrowernumber = ?";
-    $query .= "\nAND opac_display = 1" if $opac_only;
-    $query .= "\nORDER BY code, attribute";
-    my $sth = $dbh->prepare_cached($query);
-    $sth->execute($borrowernumber);
-    my @results = ();
-    while (my $row = $sth->fetchrow_hashref()) {
-        push @results, {
-            code              => $row->{'code'},
-            description       => $row->{'description'},
-            value             => $row->{'attribute'},
-            value_description => $row->{'lib'},
-            display_checkout  => $row->{'display_checkout'},
-            category_code     => $row->{'category_code'},
-            class             => $row->{'class'},
-        }
-    }
-    $sth->finish;
-    return \@results;
-}
-
 =head2 SearchIdMatchingAttribute
 
   my $matching_borrowernumbers = C4::Members::Attributes::SearchIdMatchingAttribute($filter);

Koha/Patron.pm

@@ -1438,20 +1438,19 @@ sub generate_userid {
      return $self;
 }
 
-=head3 attributes
+=head3 get_extended_attributes
 
-my $attributes = $patron->attributes
+my $attributes = $patron->get_extended_attributes
 
 Return object of Koha::Patron::Attributes type with all attributes set for this patron
 
 =cut
 
-sub attributes {
+sub get_extended_attributes {
     my ( $self ) = @_;
-    return Koha::Patron::Attributes->search({
-        borrowernumber => $self->borrowernumber,
-        branchcode     => $self->branchcode,
-    });
+    my $rs = $self->_result->borrower_attributes;
+    # We call search to use the filters in Koha::Patron::Attributes->search
+    return Koha::Patron::Attributes->_new_from_dbic($rs)->search;
 }
 
 =head3 lock

Koha/Patron/Attribute.pm

@@ -64,9 +64,54 @@ sub type {
 
     my $self = shift;
 
-    return Koha::Patron::Attribute::Types->find( $self->code );
+    return scalar Koha::Patron::Attribute::Types->find( $self->code );
 }
 
+=head3
+
+my $authorised_value = $attribute->authorised_value;
+
+Return the Koha::AuthorisedValue object of this attribute when one is attached.
+
+Return undef if this attribute is not attached to an authorised value
+
+=cut
+
+sub authorised_value {
+    my ($self) = @_;
+
+    return unless $self->type->authorised_value_category;
+
+    my $av = Koha::AuthorisedValues->search(
+        {
+            category         => $self->type->authorised_value_category,
+            authorised_value => $self->attribute,
+        }
+    );
+    return unless $av->count; # Data inconsistency
+    return $av->next;
+}
+
+=head3
+
+my $description = $patron_attribute->description;
+
+Return the value of this attribute or the description of the authorised value (when attached).
+
+This method must be called when the authorised value's description must be
+displayed instead of the code.
+
+=cut
+
+sub description {
+    my ( $self) = @_;
+    if ( $self->type->authorised_value_category ) {
+        return $self->authorised_value->lib;
+    }
+    return $self->attribute;
+}
+
+
 =head2 Internal methods
 
 =head3 _check_repeatable

Koha/Patron/Attributes.pm

@@ -60,6 +60,7 @@ sub search {
             },
         } : {};
     $attributes //= {};
+    unless ( exists $attributes->{order_by} ) { $attributes->{order_by} = ['code', 'attribute'] }
     return $self->SUPER::search( { %$params, %$or }, { %$attributes, %$join } );
 }
 

Koha/Patrons/Import.pm

@@ -299,7 +299,7 @@ sub import_patrons {
             }
             if ($extended) {
                 if ($ext_preserve) {
-                    my $old_attributes = GetBorrowerAttributes($borrowernumber);
+                    my $old_attributes = $patron->get_extended_attributes->as_list;
                     $patron_attributes = extended_attributes_merge( $old_attributes, $patron_attributes );
                 }
                 push @errors, { unknown_error => 1 }

koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc

@@ -71,9 +71,13 @@
     [% END %]
 
     [% IF Koha.Preference('ExtendedPatronAttributes') %]
-        [% FOREACH extendedattribute IN patron.attributes %]
-            [% IF ( extendedattribute.display_checkout ) %]
-                <li class="patronattribute"><span class="patronattributelabel">[% extendedattribute.type_description | html %]</span> : [% IF ( extendedattribute.value_description ) %][% extendedattribute.value_description | html %][% ELSE %][% extendedattribute.attribute | html %][% END %]</li>
+        [% FOREACH extendedattribute IN patron.get_extended_attributes %]
+            [% IF ( extendedattribute.type.display_checkout ) %] [%# FIXME We should filter in the line above %]
+                [% IF ( extendedattribute.attribute ) %] [%# FIXME Why that? why not if == 0? %]
+                    <li class="patronattribute">
+                        <span class="patronattributelabel">[% extendedattribute.type.description | html %]</span>: [% extendedattribute.description | html %]
+                    </li>
+                [% END %]
             [% END %]
         [% END %]
     [% END %]

koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt

@@ -381,12 +381,8 @@
                                                     <ol>
                                                         [% FOREACH item IN attribute.items %]
                                                             <li>
-                                                                <span class="label">[% item.description | html %]: </span>
-                                                                [% IF ( item.value_description ) %]
-                                                                    [% item.value_description | html %]
-                                                                [% ELSE %]
-                                                                    [% item.value| html | html_line_break %]
-                                                                [% END %]
+                                                                <span class="label">[% item.type.description | html %]: </span>
+                                                                [% item.description | html_line_break %]
                                                             </li>
                                                         [% END %]
                                                     </ol>

koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt

@@ -191,7 +191,8 @@
                                                     <td>[% borrower.debarredcomment | html %]</td>
                                                     [% FOREACH pa IN borrower.patron_attributes %]
                                                         [% IF ( pa.code ) %]
-                                                            <td>[% pa.code | html %]=[% pa.value | html %]</td>
+                                                        [%# Replace pa.attribute with pa.description if we prefer to display the description %]
+                                                            <td>[% pa.code | html %]=[% pa.attribute | html %]</td>
                                                         [% ELSE %]
                                                             <td></td>
                                                         [% END %]

members/memberentry.pl

@@ -870,13 +870,14 @@ sub patron_attributes_form {
         $template->param(no_patron_attribute_types => 1);
         return;
     }
-    my $attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-    my @classes = uniq( map {$_->{class}} @$attributes );
+    my $patron = Koha::Patrons->find($borrowernumber); # Already fetched but outside of this sub
+    my @attributes = $patron->get_extended_attributes->as_list; # FIXME Must be improved!
+    my @classes = uniq( map {$_->type->class} @attributes );
     @classes = sort @classes;
 
     # map patron's attributes into a more convenient structure
     my %attr_hash = ();
-    foreach my $attr (@$attributes) {
+    foreach my $attr (@attributes) {
         push @{ $attr_hash{$attr->{code}} }, $attr;
     }
 

members/moremember.pl

@@ -35,7 +35,6 @@ use C4::Output;
 use C4::Members::AttributeTypes;
 use C4::Form::MessagingPreferences;
 use List::MoreUtils qw/uniq/;
-use C4::Members::Attributes qw(GetBorrowerAttributes);
 use Koha::Patron::Debarments qw(GetDebarments);
 use Koha::Patron::Messages;
 use Koha::DateUtils;
@@ -131,15 +130,15 @@ $template->param(
 );
 
 if (C4::Context->preference('ExtendedPatronAttributes')) {
-    my $attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-    my @classes = uniq( map {$_->{class}} @$attributes );
+    my @attributes = $patron->get_extended_attributes->as_list; # FIXME Must be improved!
+    my @classes = uniq( map {$_->type->class} @attributes );
     @classes = sort @classes;
 
     my @attributes_loop;
     for my $class (@classes) {
         my @items;
-        for my $attr (@$attributes) {
-            push @items, $attr if $attr->{class} eq $class
+        for my $attr (@attributes) {
+            push @items, $attr if $attr->type->class eq $class
         }
         my $av = Koha::AuthorisedValues->search({ category => 'PA_CLASS', authorised_value => $class });
         my $lib = $av->count ? $av->next->lib : $class;

opac/opac-memberentry.pl

@@ -26,7 +26,6 @@ use String::Random qw( random_string );
 use C4::Auth;
 use C4::Output;
 use C4::Members;
-use C4::Members::Attributes qw( GetBorrowerAttributes );
 use C4::Form::MessagingPreferences;
 use Koha::AuthUtils;
 use Koha::Patrons;

t/db_dependent/Auth_with_ldap.t

@@ -206,11 +206,7 @@ subtest 'checkpw_ldap tests' => sub {
 
         C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola', password => 'hey' );
         ok(
-            @{
-                C4::Members::Attributes::GetBorrowerAttributes(
-                    $borrower->{borrowernumber}
-                )
-            },
+            Koha::Patrons->find($borrower->{borrowernumber})->get_extended_attributes->count,
             'Extended attributes are not deleted'
         );
 

t/db_dependent/Koha/Patron/Modifications.t

@@ -30,7 +30,6 @@ use Try::Tiny;
 
 use C4::Context;
 use C4::Members;
-use C4::Members::Attributes qw( GetBorrowerAttributes );
 use Koha::Patrons;
 use Koha::Patron::Attribute;
 
@@ -175,14 +174,16 @@ subtest 'approve tests' => sub {
     );
     is( $patron->firstname, 'Kyle',
         'Patron modification set the right firstname' );
-    my @patron_attributes = GetBorrowerAttributes( $patron->borrowernumber );
-    is( $patron_attributes[0][0]->{code},
+    my $patron_attributes = $patron->get_extended_attributes;
+    my $attribute_1 = $patron_attributes->next;
+    is( $attribute_1->code,
         'CODE_1', 'Patron modification correctly saved attribute code' );
-    is( $patron_attributes[0][0]->{value},
+    is( $attribute_1->attribute,
         'VALUE_1', 'Patron modification correctly saved attribute value' );
-    is( $patron_attributes[0][1]->{code},
+    my $attribute_2 = $patron_attributes->next;
+    is( $attribute_2->code,
         'CODE_2', 'Patron modification correctly saved attribute code' );
-    is( $patron_attributes[0][1]->{value},
+    is( $attribute_2->attribute,
         0, 'Patron modification correctly saved attribute with value 0, not confused with delete' );
 
     # Create a new Koha::Patron::Modification, skip extended_attributes to
@@ -219,7 +220,7 @@ subtest 'approve tests' => sub {
     )->store();
     ok( $patron_modification->approve,
         'Patron modification correctly approved' );
-    @patron_attributes
+    my @patron_attributes
         = map { $_->unblessed }
         Koha::Patron::Attributes->search(
         { borrowernumber => $patron->borrowernumber } );
@@ -232,12 +233,12 @@ subtest 'approve tests' => sub {
     is( $patron_attributes[1]->{code},
         'CODE_2', 'Attribute updated correctly (code)' );
     is( $patron_attributes[1]->{attribute},
-        'Tomasito', 'Attribute updated correctly (attribute)' );
+        'None', 'Attribute updated correctly (attribute)' );
 
     is( $patron_attributes[2]->{code},
         'CODE_2', 'Attribute updated correctly (code)' );
     is( $patron_attributes[2]->{attribute},
-        'None', 'Attribute updated correctly (attribute)' );
+        'Tomasito', 'Attribute updated correctly (attribute)' );
 
     my $empty_code_json = '[{"code":"CODE_2","value":""}]';
     $verification_token = md5_hex( time() . {} . rand() . {} . $$ );
@@ -251,7 +252,7 @@ subtest 'approve tests' => sub {
     )->store();
     ok( $patron_modification->approve,
         'Patron modification correctly approved' );
-    @patron_attributes
+    $patron_attributes
         = map { $_->unblessed }
         Koha::Patron::Attributes->search(
         { borrowernumber => $patron->borrowernumber } );

t/db_dependent/Koha/Patrons.t

@@ -2020,7 +2020,7 @@ subtest 'anonymize' => sub {
 $schema->storage->txn_rollback;
 
 subtest 'get_extended_attributes' => sub {
-    plan tests => 7;
+    plan tests => 9;
     my $schema = Koha::Database->new->schema;
     $schema->storage->txn_begin;
 
@@ -2092,5 +2092,14 @@ subtest 'get_extended_attributes' => sub {
     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' );
 
+    # Test branch limitations
+    set_logged_in_user($patron_2);
+    C4::Members::Attributes::SetBorrowerAttributes($patron_1->borrowernumber, $attributes_for_1);
+    $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');
+
+    my $limited_value = $patron_1->get_extended_attribute_value( $attribute_type_limited->code );
+    is( $limited_value, undef, );
+
     $schema->storage->txn_rollback;
 };

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 => 46;
+use Test::More tests => 41;
 
 use_ok('C4::Members::Attributes');
 
@@ -70,12 +70,9 @@ my $attribute_type_limited = C4::Members::AttributeTypes->new('my code3', 'my de
 $attribute_type_limited->branches([ $new_library->{branchcode} ]);
 $attribute_type_limited->store;
 
-my $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes();
-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 $borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 0, 'get_extended_attributes returns the correct number of borrower attributes' );
 
 my $attributes = [
     {
@@ -92,28 +89,18 @@ my $attributes = [
     }
 ];
 
-my $set_borrower_attributes = C4::Members::Attributes::SetBorrowerAttributes();
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes();
-is( @$borrower_attributes, 0, 'SetBorrowerAttributes without arguments does not add borrower attributes' );
-
-$set_borrower_attributes = C4::Members::Attributes::SetBorrowerAttributes($borrowernumber);
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes();
-is( @$borrower_attributes, 0, 'SetBorrowerAttributes without the attributes does not add borrower attributes' );
-
-$set_borrower_attributes = C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $attributes);
+my $set_borrower_attributes = C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $attributes);
 is( $set_borrower_attributes, 1, 'SetBorrowerAttributes returns the success code' );
-$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, 3, 'GetBorrowerAttributes returns the correct number of borrower attributes' );
-is( $borrower_attributes->[0]->{code}, $attributes->[0]->{code}, 'SetBorrowerAttributes stores the correct code correctly' );
-is( $borrower_attributes->[0]->{description}, $attribute_type1->description(), 'SetBorrowerAttributes stores the field description correctly' );
-is( $borrower_attributes->[0]->{value}, $attributes->[0]->{value}, 'SetBorrowerAttributes stores the field value correctly' );
-is( $borrower_attributes->[1]->{code}, $attributes->[1]->{code}, 'SetBorrowerAttributes stores the field code correctly' );
-is( $borrower_attributes->[1]->{description}, $attribute_type2->description(), 'SetBorrowerAttributes stores the field description correctly' );
-is( $borrower_attributes->[1]->{value}, $attributes->[1]->{value}, 'SetBorrowerAttributes stores the field value correctly' );
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 3, 'GetBorrowerAttributes returns the correct number of borrower attributes' );
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 3, 'get_extended_attributes returns the correct number of borrower attributes' );
+my $attr_0 = $borrower_attributes->next;
+is( $attr_0->code, $attributes->[0]->{code}, 'SetBorrowerAttributes stores the correct code correctly' );
+is( $attr_0->type->description, $attribute_type1->description(), 'SetBorrowerAttributes stores the field description correctly' );
+is( $attr_0->attribute, $attributes->[0]->{value}, 'SetBorrowerAttributes stores the field value correctly' );
+my $attr_1 = $borrower_attributes->next;
+is( $attr_1->code, $attributes->[1]->{code}, 'SetBorrowerAttributes stores the field code correctly' );
+is( $attr_1->type->description, $attribute_type2->description(), 'SetBorrowerAttributes stores the field description correctly' );
+is( $attr_1->attribute, $attributes->[1]->{value}, 'SetBorrowerAttributes stores the field value correctly' );
 
 $attributes = [
     {
@@ -126,8 +113,8 @@ $attributes = [
     }
 ];
 C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $attributes);
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 3, 'SetBorrowerAttributes should not have removed the attributes limited to another branch' );
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 3, 'SetBorrowerAttributes should not have removed the attributes limited to another branch' );
 
 # TODO This is not implemented yet
 #$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber, undef, 'branch_limited');
@@ -151,11 +138,12 @@ my $attribute = {
     code => $attribute_type1->code(),
 };
 C4::Members::Attributes::UpdateBorrowerAttribute($borrowernumber, $attribute);
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 3, 'UpdateBorrowerAttribute does not change the number of borrower attributes' );
-is( $borrower_attributes->[0]->{code}, $attribute->{code}, 'UpdateBorrowerAttribute updates the field code correctly' );
-is( $borrower_attributes->[0]->{description}, $attribute_type1->description(), 'UpdateBorrowerAttribute updates the field description correctly' );
-is( $borrower_attributes->[0]->{value}, $attribute->{attribute}, 'UpdateBorrowerAttribute updates the field value correctly' );
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 3, 'UpdateBorrowerAttribute does not change the number of borrower attributes' );
+$attr_0 = $borrower_attributes->next;
+is( $attr_0->code, $attribute->{code}, 'UpdateBorrowerAttribute updates the field code correctly' );
+is( $attr_0->type->description, $attribute_type1->description(), 'UpdateBorrowerAttribute updates the field description correctly' );
+is( $attr_0->attribute, $attribute->{attribute}, 'UpdateBorrowerAttribute updates the field value correctly' );
 
 
 my $check_uniqueness = C4::Members::Attributes::CheckUniqueness();
@@ -187,30 +175,31 @@ for my $attr( split(' ', $attributes->[1]->{value}) ) {
 
 
 C4::Members::Attributes::DeleteBorrowerAttribute();
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 3, 'DeleteBorrowerAttribute without arguments deletes nothing' );
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 3, 'DeleteBorrowerAttribute without arguments deletes nothing' );
 C4::Members::Attributes::DeleteBorrowerAttribute($borrowernumber);
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 3, 'DeleteBorrowerAttribute without the attribute deletes nothing' );
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 3, 'DeleteBorrowerAttribute without the attribute deletes nothing' );
 C4::Members::Attributes::DeleteBorrowerAttribute(undef, $attribute);
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 3, 'DeleteBorrowerAttribute with a undef borrower number deletes nothing' );
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 3, 'DeleteBorrowerAttribute with a undef borrower number deletes nothing' );
 
 C4::Members::Attributes::DeleteBorrowerAttribute($borrowernumber, $attribute);
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 2, 'DeleteBorrowerAttribute deletes a borrower attribute' );
-is( $borrower_attributes->[0]->{code}, $attributes->[1]->{code}, 'DeleteBorrowerAttribute deletes the correct entry');
-is( $borrower_attributes->[0]->{description}, $attribute_type2->description(), 'DeleteBorrowerAttribute deletes the correct entry');
-is( $borrower_attributes->[0]->{value}, $attributes->[1]->{value}, 'DeleteBorrowerAttribute deletes the correct entry');
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 2, 'DeleteBorrowerAttribute deletes a borrower attribute' );
+$attr_0 = $borrower_attributes->next;
+is( $attr_0->code, $attributes->[1]->{code}, 'DeleteBorrowerAttribute deletes the correct entry');
+is( $attr_0->type->description, $attribute_type2->description(), 'DeleteBorrowerAttribute deletes the correct entry');
+is( $attr_0->attribute, $attributes->[1]->{value}, 'DeleteBorrowerAttribute deletes the correct entry');
 
 C4::Members::Attributes::DeleteBorrowerAttribute($borrowernumber, $attributes->[1]);
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 1, 'DeleteBorrowerAttribute deletes a borrower attribute' );
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 1, 'DeleteBorrowerAttribute deletes a borrower attribute' );
 
 # Regression tests for bug 16504
 t::lib::Mocks::mock_userenv({ branchcode => $new_library->{branchcode} });
-my $another_patron = $builder->build(
-    {   source => 'Borrower',
+my $another_patron = $builder->build_object(
+    {   class  => 'Koha::Patrons',
         value  => {
             firstname    => 'my another firstname',
             surname      => 'my another surname',
@@ -232,8 +221,8 @@ $attributes = [
         code => $attribute_type_limited->code(),
     }
 ];
-C4::Members::Attributes::SetBorrowerAttributes($another_patron->{borrowernumber}, $attributes);
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($another_patron->{borrowernumber});
-is( @$borrower_attributes, 3, 'SetBorrowerAttributes should have added the 3 attributes for another patron');
-$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber);
-is( @$borrower_attributes, 1, 'SetBorrowerAttributes should not have removed the attributes of other patrons' );
+C4::Members::Attributes::SetBorrowerAttributes($another_patron->borrowernumber, $attributes);
+$borrower_attributes = $another_patron->get_extended_attributes;
+is( $borrower_attributes->count, 3, 'SetBorrowerAttributes should have added the 3 attributes for another patron');
+$borrower_attributes = $patron->get_extended_attributes;
+is( $borrower_attributes->count, 1, 'SetBorrowerAttributes should not have removed the attributes of other patrons' );

tools/modborrowers.pl

@@ -92,11 +92,12 @@ if ( $op eq 'show' ) {
         my $patron = Koha::Patrons->find( { cardnumber => $cardnumber } );
         if ( $patron ) {
             if ( $logged_in_user->can_see_patron_infos( $patron ) ) {
-                $patron = $patron->unblessed;
-                $patron->{patron_attributes} = C4::Members::Attributes::GetBorrowerAttributes( $patron->{borrowernumber} );
-                $max_nb_attr = scalar( @{ $patron->{patron_attributes} } )
-                    if scalar( @{ $patron->{patron_attributes} } ) > $max_nb_attr;
-                push @borrowers, $patron;
+                my $borrower = $patron->unblessed;
+                my $attributes = $patron->get_extended_attributes;
+                $borrower->{patron_attributes} = $attributes->as_list;
+                $borrower->{patron_attributes_count} = $attributes->count;
+                $max_nb_attr = $borrower->{patron_attributes_count} if $borrower->{patron_attributes_count} > $max_nb_attr;
+                push @borrowers, $borrower;
             } else {
                 push @notfoundcardnumbers, $cardnumber;
             }
@@ -107,7 +108,7 @@ if ( $op eq 'show' ) {
 
     # Just for a correct display
     for my $borrower ( @borrowers ) {
-        my $length = scalar( @{ $borrower->{patron_attributes} } );
+        my $length = $borrower->{patron_attributes_count};
         push @{ $borrower->{patron_attributes} }, {} for ( $length .. $max_nb_attr - 1);
     }
 
@@ -373,7 +374,7 @@ if ( $op eq 'do' ) {
             }
         }
 
-        my $borrower_categorycode = Koha::Patrons->find( $borrowernumber )->categorycode;
+        my $patron = Koha::Patrons->find( $borrowernumber );
         my $i=0;
         for ( @attributes ) {
             next unless $_;
@@ -382,7 +383,7 @@ if ( $op eq 'do' ) {
             $attribute->{attribute} = $attr_values[$i];
             my $attr_type = C4::Members::AttributeTypes->fetch( $_ );
             # If this borrower is not in the category of this attribute, we don't want to modify this attribute
-            ++$i and next if $attr_type->{category_code} and $attr_type->{category_code} ne $borrower_categorycode;
+            ++$i and next if $attr_type->{category_code} and $attr_type->{category_code} ne $patron->category_code;
             my $valuename = "attr" . $i . "_value";
             if ( grep { $_ eq $valuename } @disabled ) {
                 # The attribute is disabled, we remove it for this borrower !
@@ -407,11 +408,13 @@ if ( $op eq 'do' ) {
     for my $borrowernumber ( @borrowernumbers ) {
         my $patron = Koha::Patrons->find( $borrowernumber );
         if ( $patron ) {
-            $patron = $patron->unblessed;
-            $patron->{patron_attributes} = C4::Members::Attributes::GetBorrowerAttributes( $patron->{borrowernumber} );
-            $max_nb_attr = scalar( @{ $patron->{patron_attributes} } )
-                if scalar( @{ $patron->{patron_attributes} } ) > $max_nb_attr;
-            push @borrowers, $patron;
+            my $category_description = $patron->category->description;
+            my $borrower = $patron->unblessed;
+            $borrower->{category_description} = $category_description;
+            my $attributes = $patron->get_extended_attributes;
+            $borrower->{patron_attributes} = $attributes->as_list;
+            $max_nb_attr = $attributes->count if $attributes->count > $max_nb_attr;
+            push @borrowers, $borrower;
         }
     }
     my @patron_attributes_option;