Bug 20443: Remove UpdateBorrowerAttribute and SetBorrowerAttributes
authorJonathan Druart <jonathan.druart@bugs.koha-community.org>
Fri, 13 Jul 2018 17:11:44 +0000 (14:11 -0300)
committerMartin Renvoize <martin.renvoize@ptfs-europe.com>
Mon, 23 Mar 2020 13:44:16 +0000 (13:44 +0000)
This patch replace Koha::Patron->get_extended_attributes with
->extended_attributes
It's now a getter a setter method.

It permits to replace UpdateBorrowerAttribute and use
create_related from DBIx::Class

Notes:
* We face the same variable names difference than in a previous patch
(value vs attribute)

Bug 20443: Remove SetBorrowerAttributes

squash  + RM get_extended_attributes

 RM get_extended_attributes

SQUASH Bug 20443: Remove UpdateBorrowerAttribute and SetBorrowerAttribute

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

18 files changed:
C4/Auth_with_ldap.pm
C4/ILSDI/Services.pm
C4/Letters.pm
C4/Members.pm
C4/Members/Attributes.pm
Koha/Patron.pm
Koha/Patrons/Import.pm
koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc
members/memberentry.pl
members/moremember.pl
opac/opac-memberentry.pl
t/db_dependent/Auth_with_ldap.t
t/db_dependent/Koha/Patron/Modifications.t
t/db_dependent/Koha/Patrons.t
t/db_dependent/Koha/Patrons/Import.t
t/db_dependent/Members/Attributes.t
t/db_dependent/Utils/Datatables_Members.t
tools/modborrowers.pl

index 09b2b3f..3cff983 100644 (file)
@@ -239,7 +239,11 @@ sub checkpw_ldap {
                 next;
             }
             if (C4::Members::Attributes::CheckUniqueness($code, $borrower{$code}, $borrowernumber)) {
-                C4::Members::Attributes::UpdateBorrowerAttribute($borrowernumber, {code => $code, attribute => $borrower{$code}});
+                my $patron = Koha::Patrons->find($borrowernumber);
+                if ( $patron ) { # Should not be needed, but we are in C4::Auth LDAP...
+                    my $attribute = Koha::Patron::Attribute->new({code => $code, attribute => $borrower{$code}});
+                    $patron->extended_attributes([$attribute]);
+                }
             } else {
                 warn "ERROR_extended_unique_id_failed $code $borrower{$code}";
             }
index 0416f5c..29cb58a 100644 (file)
@@ -506,7 +506,7 @@ sub GetPatronInfo {
     if ( $show_attributes && $show_attributes eq "1" ) {
         # FIXME Regression expected here, we do not retrieve the same field as previously
         # Waiting for answer on bug 14257 comment 15
-        my $attrs = $patron->get_extended_attributes->search({ opac_display => 1 })->unblessed;
+        my $attrs = $patron->extended_attributes->search({ opac_display => 1 })->unblessed;
         $borrower->{'attributes'} = $attrs;
     }
 
index a5bb9ea..9e583cd 100644 (file)
@@ -853,7 +853,7 @@ sub _parseletter {
     if ($table eq 'borrowers' && $letter->{content}) {
         my $patron = Koha::Patrons->find( $values->{borrowernumber} );
         if ( $patron ) {
-            my $attributes = $patron->get_extended_attributes;
+            my $attributes = $patron->extended_attributes;
             my %attr;
             while ( my $attribute = $attributes->next ) {
                 my $code = $attribute->code;
index eb5dbfb..9355436 100644 (file)
@@ -33,7 +33,7 @@ use C4::Reserves;
 use C4::Accounts;
 use C4::Biblio;
 use C4::Letters;
-use C4::Members::Attributes qw(SearchIdMatchingAttribute UpdateBorrowerAttribute);
+use C4::Members::Attributes qw(SearchIdMatchingAttribute);
 use C4::NewsChannels; #get slip news
 use DateTime;
 use Koha::Database;
index 6f5001d..c48828c 100644 (file)
@@ -29,8 +29,7 @@ our ($csv, $AttributeTypes);
 
 BEGIN {
     @ISA = qw(Exporter);
-    @EXPORT_OK = qw(CheckUniqueness SetBorrowerAttributes
-                    UpdateBorrowerAttribute
+    @EXPORT_OK = qw(CheckUniqueness
                     extended_attributes_code_value_arrayref extended_attributes_merge
                     SearchIdMatchingAttribute);
     %EXPORT_TAGS = ( all => \@EXPORT_OK );
@@ -112,71 +111,14 @@ sub CheckUniqueness {
     return ($count == 0);
 }
 
-=head2 SetBorrowerAttributes 
-
-  SetBorrowerAttributes($borrowernumber, [ { code => 'CODE', value => 'value' }, ... ] );
-
-Set patron attributes for the patron identified by C<$borrowernumber>,
-replacing any that existed previously.
-
-=cut
-
-sub SetBorrowerAttributes {
-    my $borrowernumber = shift;
-    my $attr_list = shift;
-    my $no_branch_limit = shift // 0;
-
-    my $dbh = C4::Context->dbh;
-
-    my $attributes = Koha::Patrons->find($borrowernumber)->get_extended_attributes;
-
-    unless ( $no_branch_limit ) {
-        $attributes = $attributes->filter_by_branch_limitations;
-    }
-    $attributes->delete;
-
-    my $sth = $dbh->prepare("INSERT INTO borrower_attributes (borrowernumber, code, attribute)
-                             VALUES (?, ?, ?)");
-    foreach my $attr (@$attr_list) {
-        $sth->execute($borrowernumber, $attr->{code}, $attr->{value});
-        if ($sth->err) {
-            warn sprintf('Database returned the following error: %s', $sth->errstr);
-            return; # bail immediately on errors
-        }
-    }
-    return 1; # borrower attributes successfully set
-}
-
-=head2 UpdateBorrowerAttribute
-
-  UpdateBorrowerAttribute($borrowernumber, $attribute );
-
-Update a borrower attribute C<$attribute> for the patron identified by C<$borrowernumber>,
-
-=cut
-
-sub UpdateBorrowerAttribute {
-    my ( $borrowernumber, $attribute ) = @_;
-
-    Koha::Patrons->find($borrowernumber)->get_extended_attributes->search({ 'me.code' => $attribute->{code} })->filter_by_branch_limitations->delete;
-
-    my $dbh = C4::Context->dbh;
-    my $query = "INSERT INTO borrower_attributes SET attribute = ?, code = ?, borrowernumber = ?";
-    my @params = ( $attribute->{attribute}, $attribute->{code}, $borrowernumber );
-    my $sth = $dbh->prepare( $query );
-
-    $sth->execute( @params );
-}
-
-
 =head2 extended_attributes_code_value_arrayref 
 
    my $patron_attributes = "homeroom:1150605,grade:01,extradata:foobar";
    my $aref = extended_attributes_code_value_arrayref($patron_attributes);
 
-Takes a comma-delimited CSV-style string argument and returns the kind of data structure that SetBorrowerAttributes wants, 
+Takes a comma-delimited CSV-style string argument and returns the kind of data structure that Koha::Patron->extended_attributes wants,
 namely a reference to array of hashrefs like:
- [ { code => 'CODE', value => 'value' }, { code => 'CODE2', value => 'othervalue' } ... ]
+ [ { code => 'CODE', attribute => 'value' }, { code => 'CODE2', attribute => 'othervalue' } ... ]
 
 Caches Text::CSV parser object for efficiency.
 
@@ -190,7 +132,7 @@ sub extended_attributes_code_value_arrayref {
     # TODO: error handling (check $ok)
     return [
         sort {&_sort_by_code($a,$b)}
-        map { map { my @arr = split /:/, $_, 2; { code => $arr[0], value => $arr[1] } } $_ }
+        map { map { my @arr = split /:/, $_, 2; { code => $arr[0], attribute => $arr[1] } } $_ }
         @list
     ];
     # nested map because of split
index d7bea28..9a06db3 100644 (file)
@@ -1438,16 +1438,41 @@ sub generate_userid {
      return $self;
 }
 
-=head3 get_extended_attributes
+=head3 add_extended_attribute
 
-my $attributes = $patron->get_extended_attributes
+=cut
+
+sub add_extended_attribute {
+    my ($self, $attribute) = @_;
+    $attribute->{borrowernumber} = $self->borrowernumber;
+    return Koha::Patron::Attribute->new($attribute)->store;
+}
+
+=head3 extended_attributes
 
 Return object of Koha::Patron::Attributes type with all attributes set for this patron
 
+Or setter FIXME
+
 =cut
 
-sub get_extended_attributes {
-    my ( $self ) = @_;
+sub extended_attributes {
+    my ( $self, $attributes ) = @_;
+    if ($attributes) {    # setter
+        my $schema = $self->_result->result_source->schema;
+        $schema->txn_do(
+            sub {
+                # Remove the existing one
+                $self->extended_attributes->filter_by_branch_limitations->delete;
+
+                # Insert the new ones
+                for my $attribute (@$attributes) {
+                    $self->_result->create_related('borrower_attributes', $attribute);
+                }
+            }
+        );
+    }
+
     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;
index 5fe9476..e644bcb 100644 (file)
@@ -299,11 +299,18 @@ sub import_patrons {
             }
             if ($extended) {
                 if ($ext_preserve) {
-                    my $old_attributes = $patron->get_extended_attributes->as_list;
+                    my $old_attributes = $patron->extended_attributes->as_list;
                     $patron_attributes = extended_attributes_merge( $old_attributes, $patron_attributes );
                 }
-                push @errors, { unknown_error => 1 }
-                  unless SetBorrowerAttributes( $borrower{'borrowernumber'}, $patron_attributes, 'no_branch_limit' );
+                eval {
+                    # We do not want to filter by branch, maybe we should?
+                    Koha::Patrons->find($borrowernumber)->extended_attributes->delete;
+                    $patron->extended_attributes($patron_attributes);
+                };
+                if ($@) {
+                    # FIXME This is not an unknown error, we can do better here
+                    push @errors, { unknown_error => 1 };
+                }
             }
             $overwritten++;
             push(
@@ -332,7 +339,9 @@ sub import_patrons {
                 }
 
                 if ($extended) {
-                    SetBorrowerAttributes( $patron->borrowernumber, $patron_attributes );
+                    # FIXME Hum, we did not filter earlier and now we do?
+                    $patron->extended_attributes->filter_by_branch_limitations->delete;
+                    $patron->extended_attributes($patron_attributes);
                 }
 
                 if ($set_messaging_prefs) {
@@ -460,7 +469,7 @@ sub set_column_keys {
 
  my $patron_attributes = set_patron_attributes($extended, $borrower{patron_attributes}, $feedback);
 
-Returns a reference to array of hashrefs data structure as expected by SetBorrowerAttributes.
+Returns a reference to array of hashrefs data structure as expected by Koha::Patron->extended_attributes
 
 =cut
 
index fbb4a2b..7b34942 100644 (file)
@@ -71,7 +71,7 @@
     [% END %]
 
     [% IF Koha.Preference('ExtendedPatronAttributes') %]
-        [% FOREACH extendedattribute IN patron.get_extended_attributes %]
+        [% FOREACH extendedattribute IN patron.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">
index 6f821a3..29219da 100755 (executable)
@@ -480,7 +480,8 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){
         }
 
         if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) {
-            C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $extended_patron_attributes);
+            $patron->extended_attributes->filter_by_branch_limitations->delete;
+            $patron->extended_attributes($extended_patron_attributes);
         }
         if (C4::Context->preference('EnhancedMessagingPreferences') and $input->param('setting_messaging_prefs')) {
             C4::Form::MessagingPreferences::handle_form_action($input, { borrowernumber => $borrowernumber }, $template, 1, $newdata{'categorycode'});
@@ -550,7 +551,8 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){
 
         add_guarantors( $patron, $input );
         if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) {
-            C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $extended_patron_attributes);
+            $patron->extended_attributes->filter_by_branch_limitations->delete;
+            $patron->extended_attributes($extended_patron_attributes);
         }
         if (C4::Context->preference('EnhancedMessagingPreferences') and $input->param('setting_messaging_prefs')) {
             C4::Form::MessagingPreferences::handle_form_action($input, { borrowernumber => $borrowernumber }, $template);
@@ -871,7 +873,7 @@ sub patron_attributes_form {
         return;
     }
     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 @attributes = $patron->extended_attributes->as_list; # FIXME Must be improved!
     my @classes = uniq( map {$_->type->class} @attributes );
     @classes = sort @classes;
 
index b141488..ea00da7 100755 (executable)
@@ -130,7 +130,7 @@ $template->param(
 );
 
 if (C4::Context->preference('ExtendedPatronAttributes')) {
-    my @attributes = $patron->get_extended_attributes->as_list; # FIXME Must be improved!
+    my @attributes = $patron->extended_attributes->as_list; # FIXME Must be improved!
     my @classes = uniq( map {$_->type->class} @attributes );
     @classes = sort @classes;
 
index 669687e..6fdc765 100755 (executable)
@@ -213,7 +213,8 @@ if ( $action eq 'create' ) {
             my $patron = Koha::Patron->new( \%borrower )->store;
             Koha::Patron::Consent->new({ borrowernumber => $patron->borrowernumber, type => 'GDPR_PROCESSING', given_on => $consent_dt })->store if $consent_dt;
             if ( $patron ) {
-                C4::Members::Attributes::SetBorrowerAttributes( $patron->borrowernumber, $attributes );
+                $patron->extended_attributes->filter_by_branch_limitations->delete;
+                $patron->extended_attributes($attributes);
                 if ( C4::Context->preference('EnhancedMessagingPreferences') ) {
                     C4::Form::MessagingPreferences::handle_form_action(
                         $cgi,
index 001f2a8..5871b87 100755 (executable)
@@ -206,7 +206,7 @@ subtest 'checkpw_ldap tests' => sub {
 
         C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola', password => 'hey' );
         ok(
-            Koha::Patrons->find($borrower->{borrowernumber})->get_extended_attributes->count,
+            Koha::Patrons->find($borrower->{borrowernumber})->extended_attributes->count,
             'Extended attributes are not deleted'
         );
 
index ce279ad..3378ccd 100755 (executable)
@@ -174,7 +174,7 @@ subtest 'approve tests' => sub {
     );
     is( $patron->firstname, 'Kyle',
         'Patron modification set the right firstname' );
-    my $patron_attributes = $patron->get_extended_attributes;
+    my $patron_attributes = $patron->extended_attributes;
     my $attribute_1 = $patron_attributes->next;
     is( $attribute_1->code,
         'CODE_1', 'Patron modification correctly saved attribute code' );
index 2a39023..69331d9 100644 (file)
@@ -2019,7 +2019,7 @@ subtest 'anonymize' => sub {
 };
 $schema->storage->txn_rollback;
 
-subtest 'get_extended_attributes' => sub {
+subtest 'extended_attributes' => sub {
     plan tests => 10;
     my $schema = Koha::Database->new->schema;
     $schema->storage->txn_begin;
@@ -2069,17 +2069,19 @@ subtest 'get_extended_attributes' => sub {
         }
     ];
 
-    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' );
+    my $extended_attributes = $patron_1->extended_attributes;
+    is( ref($extended_attributes), 'Koha::Patron::Attributes', 'Koha::Patron->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);
+    $patron_1->extended_attributes->filter_by_branch_limitations->delete;
+    $patron_2->extended_attributes->filter_by_branch_limitations->delete;
+    $patron_1->extended_attributes($attributes_for_1);
+    $patron_2->extended_attributes($attributes_for_2);
 
-    my $extended_attributes_for_1 = $patron_1->get_extended_attributes;
+    my $extended_attributes_for_1 = $patron_1->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;
+    my $extended_attributes_for_2 = $patron_2->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 });
@@ -2095,11 +2097,11 @@ subtest 'get_extended_attributes' => sub {
     # Test branch limitations
     set_logged_in_user($patron_2);
     # Return all
-    $extended_attributes_for_1 = $patron_1->get_extended_attributes;
+    $extended_attributes_for_1 = $patron_1->extended_attributes;
     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;
+    $extended_attributes_for_1 = $patron_1->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
index ea3d1d8..fdf90fa 100644 (file)
@@ -565,10 +565,10 @@ subtest 'test_set_patron_attributes' => sub {
     # Then ...
     ok($result_3, 'Got some data back from set patron attributes');
     is($result_3->[0]->{code}, 'grade', 'Got the expected first code from set patron attributes');
-    is($result_3->[0]->{value}, '01', 'Got the expected first value from set patron attributes');
+    is($result_3->[0]->{attribute}, '01', 'Got the expected first value from set patron attributes');
 
     is($result_3->[1]->{code}, 'homeroom', 'Got the expected second code from set patron attributes');
-    is($result_3->[1]->{value}, 1150605, 'Got the expected second value from set patron attributes');
+    is($result_3->[1]->{attribute}, 1150605, 'Got the expected second value from set patron attributes');
 
     is(scalar @feedback_3, 1, 'Got the expected 1 array size from set patron attributes with extended user');
     is($feedback_3[0]->{feedback}, 1, 'Got the expected second feedback from set patron attributes with extended user');
index e5c525d..b1b001d 100644 (file)
@@ -26,7 +26,7 @@ use Koha::Database;
 use t::lib::TestBuilder;
 use t::lib::Mocks;
 
-use Test::More tests => 38;
+use Test::More tests => 39;
 
 use_ok('C4::Members::Attributes');
 
@@ -71,79 +71,83 @@ $attribute_type_limited->branches([ $new_library->{branchcode} ]);
 $attribute_type_limited->store;
 
 $patron = Koha::Patrons->find($borrowernumber);
-my $borrower_attributes = $patron->get_extended_attributes;
-is( $borrower_attributes->count, 0, 'get_extended_attributes returns the correct number of borrower attributes' );
+my $borrower_attributes = $patron->extended_attributes;
+is( $borrower_attributes->count, 0, 'extended_attributes returns the correct number of borrower attributes' );
 
 my $attributes = [
     {
-        value => 'my attribute1',
+        attribute => 'my attribute1',
         code => $attribute_type1->code(),
     },
     {
-        value => 'my attribute2',
+        attribute => 'my attribute2',
         code => $attribute_type2->code(),
     },
     {
-        value => 'my attribute limited',
+        attribute => 'my attribute limited',
         code => $attribute_type_limited->code(),
     }
 ];
 
-my $set_borrower_attributes = C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $attributes);
-is( $set_borrower_attributes, 1, 'SetBorrowerAttributes returns the success code' );
-$borrower_attributes = $patron->get_extended_attributes;
-is( $borrower_attributes->count, 3, 'get_extended_attributes returns the correct number of borrower attributes' );
+$patron->extended_attributes->filter_by_branch_limitations->delete;
+my $set_borrower_attributes = $patron->extended_attributes($attributes);
+is( ref($set_borrower_attributes), 'Koha::Patron::Attributes', '');
+is( $set_borrower_attributes->count, 3, '');
+$borrower_attributes = $patron->extended_attributes;
+is( $borrower_attributes->count, 3, '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' );
+is( $attr_0->code, $attributes->[0]->{code}, 'setter for extended_attributes sestores the correct code correctly' );
+is( $attr_0->type->description, $attribute_type1->description(), 'setter for extended_attributes stores the field description correctly' );
+is( $attr_0->attribute, $attributes->[0]->{attribute}, 'setter for extended_attributes 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' );
+is( $attr_1->code, $attributes->[1]->{code}, 'setter for extended_attributes stores the field code correctly' );
+is( $attr_1->type->description, $attribute_type2->description(), 'setter for extended_attributes stores the field description correctly' );
+is( $attr_1->attribute, $attributes->[1]->{attribute}, 'setter for extended_attributes stores the field value correctly' );
 
 $attributes = [
     {
-        value => 'my attribute1',
+        attribute => 'my attribute1',
         code => $attribute_type1->code(),
     },
     {
-        value => 'my attribute2',
+        attribute => 'my attribute2',
         code => $attribute_type2->code(),
     }
 ];
-C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $attributes);
-$borrower_attributes = $patron->get_extended_attributes;
-is( $borrower_attributes->count, 3, 'SetBorrowerAttributes should not have removed the attributes limited to another branch' );
+$patron->extended_attributes->filter_by_branch_limitations->delete;
+$patron->extended_attributes($attributes);
+$borrower_attributes = $patron->extended_attributes;
+is( $borrower_attributes->count, 3, 'setter for extended_attributes 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');
 #is( @$borrower_attributes, 2, 'GetBorrowerAttributes returns the correct number of borrower attributes filtered on library' );
 
 $patron = Koha::Patrons->find($borrowernumber);
-my $extended_attributes = $patron->get_extended_attributes;
+my $extended_attributes = $patron->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('my invalid code');
 is( $attribute_value, undef, 'non existent attribute should undef');
 
 $attribute_value = $patron->get_extended_attribute($attributes->[0]->{code});
-is( $attribute_value->attribute, $attributes->[0]->{value}, 'get_extended_attribute returns the correct attribute value' );
+is( $attribute_value->attribute, $attributes->[0]->{attribute}, 'get_extended_attribute returns the correct attribute value' );
 $attribute_value = $patron->get_extended_attribute($attributes->[1]->{code});
-is( $attribute_value->attribute, $attributes->[1]->{value}, 'get_extended_attribute returns the correct attribute value' );
+is( $attribute_value->attribute, $attributes->[1]->{attribute}, 'get_extended_attribute returns the correct attribute value' );
 
 
 my $attribute = {
     attribute => 'my attribute3',
     code => $attribute_type1->code(),
 };
-C4::Members::Attributes::UpdateBorrowerAttribute($borrowernumber, $attribute);
-$borrower_attributes = $patron->get_extended_attributes;
-is( $borrower_attributes->count, 3, 'UpdateBorrowerAttribute does not change the number of borrower attributes' );
+$patron->extended_attributes->search({'me.code' => $attribute->{code}})->filter_by_branch_limitations->delete;
+$patron->add_extended_attribute($attribute);
+$borrower_attributes = $patron->extended_attributes;
+is( $borrower_attributes->count, 3, 'delete then add a new attribute 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' );
+is( $attr_0->code, $attribute->{code}, 'delete then add a new attribute updates the field code correctly' );
+is( $attr_0->type->description, $attribute_type1->description(), 'delete then add a new attribute updates the field description correctly' );
+is( $attr_0->attribute, $attribute->{attribute}, 'delete then add a new attribute updates the field value correctly' );
 
 
 my $check_uniqueness = C4::Members::Attributes::CheckUniqueness();
@@ -160,7 +164,7 @@ $check_uniqueness = C4::Members::Attributes::CheckUniqueness($attribute->{code},
 is( $check_uniqueness, 1, 'CheckUniqueness with a new value returns true' );
 $check_uniqueness = C4::Members::Attributes::CheckUniqueness('my invalid code', 'new value');
 is( $check_uniqueness, 0, 'CheckUniqueness with an invalid argument code and a new value returns false' );
-$check_uniqueness = C4::Members::Attributes::CheckUniqueness($attributes->[1]->{code}, $attributes->[1]->{value});
+$check_uniqueness = C4::Members::Attributes::CheckUniqueness($attributes->[1]->{code}, $attributes->[1]->{attribute});
 is( $check_uniqueness, 1, 'CheckUniqueness with an attribute unique_id=0 returns true' );
 $check_uniqueness = C4::Members::Attributes::CheckUniqueness($attribute->{code}, $attribute->{attribute});
 is( $check_uniqueness, '', 'CheckUniqueness returns false' );
@@ -168,22 +172,22 @@ is( $check_uniqueness, '', 'CheckUniqueness returns false' );
 
 my $borrower_numbers = C4::Members::Attributes::SearchIdMatchingAttribute('attribute1');
 is( @$borrower_numbers, 0, 'SearchIdMatchingAttribute searchs only in attributes with staff_searchable=1' );
-for my $attr( split(' ', $attributes->[1]->{value}) ) {
+for my $attr( split(' ', $attributes->[1]->{attribute}) ) {
     $borrower_numbers = C4::Members::Attributes::SearchIdMatchingAttribute($attr);
     is( $borrower_numbers->[0], $borrowernumber, 'SearchIdMatchingAttribute returns the borrower numbers matching' );
 }
 
 
 $patron->get_extended_attribute($attribute->{code})->delete;
-$borrower_attributes = $patron->get_extended_attributes;
+$borrower_attributes = $patron->extended_attributes;
 is( $borrower_attributes->count, 2, 'delete attribute by code' );
 $attr_0 = $borrower_attributes->next;
 is( $attr_0->code, $attributes->[1]->{code}, 'delete attribute by code');
 is( $attr_0->type->description, $attribute_type2->description(), 'delete attribute by code');
-is( $attr_0->attribute, $attributes->[1]->{value}, 'delete attribute by code');
+is( $attr_0->attribute, $attributes->[1]->{attribute}, 'delete attribute by code');
 
 $patron->get_extended_attribute($attributes->[1]->{code})->delete;
-$borrower_attributes = $patron->get_extended_attributes;
+$borrower_attributes = $patron->extended_attributes;
 is( $borrower_attributes->count, 1, 'delete attribute by code' );
 
 # Regression tests for bug 16504
@@ -199,20 +203,22 @@ my $another_patron = $builder->build_object(
 );
 $attributes = [
     {
-        value => 'my attribute1',
+        attribute => 'my attribute1',
         code => $attribute_type1->code(),
     },
     {
-        value => 'my attribute2',
+        attribute => 'my attribute2',
         code => $attribute_type2->code(),
     },
     {
-        value => 'my attribute limited',
+        attribute => 'my attribute limited',
         code => $attribute_type_limited->code(),
     }
 ];
-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' );
+
+$another_patron->extended_attributes->filter_by_branch_limitations->delete;
+$another_patron->extended_attributes($attributes);
+$borrower_attributes = $another_patron->extended_attributes;
+is( $borrower_attributes->count, 3, 'setter for extended_attributes should have added the 3 attributes for another patron');
+$borrower_attributes = $patron->extended_attributes;
+is( $borrower_attributes->count, 1, 'setter for extended_attributes should not have removed the attributes of other patrons' );
index feca4f8..2e76da5 100644 (file)
@@ -26,6 +26,7 @@ use C4::Members::Attributes;
 use C4::Members::AttributeTypes;
 
 use Koha::Library;
+use Koha::Patrons;
 use Koha::Patron::Categories;
 
 use t::lib::Mocks;
@@ -279,15 +280,29 @@ my $attribute_type = C4::Members::AttributeTypes->new( 'ATM_1', 'my attribute ty
 $attribute_type->{staff_searchable} = 1;
 $attribute_type->store;
 
-
-C4::Members::Attributes::SetBorrowerAttributes(
-    $john_doe->{borrowernumber}, [ { code => $attribute_type->{code}, value => 'the default value for a common user' } ]
+Koha::Patrons->find( $john_doe->{borrowernumber} )->extended_attributes(
+    [
+        {
+            code      => $attribute_type->{code},
+            attribute => 'the default value for a common user'
+        }
+    ]
 );
-C4::Members::Attributes::SetBorrowerAttributes(
-    $jane_doe->{borrowernumber}, [ { code => $attribute_type->{code}, value => 'the default value for another common user' } ]
+Koha::Patrons->find( $jane_doe->{borrowernumber} )->extended_attributes(
+    [
+        {
+            code      => $attribute_type->{code},
+            attribute => 'the default value for another common user'
+        }
+    ]
 );
-C4::Members::Attributes::SetBorrowerAttributes(
-    $john_smith->{borrowernumber}, [ { code => $attribute_type->{code}, value => 'Attribute which not appears even if contains "Dupont"' } ]
+Koha::Patrons->find( $john_smith->{borrowernumber} )->extended_attributes(
+    [
+        {
+            code      => $attribute_type->{code},
+            attribute => 'Attribute which not appears even if contains "Dupont"'
+        }
+    ]
 );
 
 t::lib::Mocks::mock_preference('ExtendedPatronAttributes', 1);
index e368368..dff8626 100755 (executable)
@@ -93,7 +93,7 @@ if ( $op eq 'show' ) {
         if ( $patron ) {
             if ( $logged_in_user->can_see_patron_infos( $patron ) ) {
                 my $borrower = $patron->unblessed;
-                my $attributes = $patron->get_extended_attributes;
+                my $attributes = $patron->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;
@@ -393,7 +393,11 @@ if ( $op eq 'do' ) {
                 push @errors, { error => $@ } if $@;
             } else {
                 eval {
-                    C4::Members::Attributes::UpdateBorrowerAttribute( $borrowernumber, $attribute );
+                    # Note:
+                    # We should not need to filter by branch, but stay on the safe side
+                    # Repeatable are not supported so we can do that - TODO
+                    $patron->extended_attributes->search({'me.code' => $attribute->{code}})->filter_by_branch_limitations->delete;
+                    $patron->add_extended_attribute($attribute);
                 };
                 push @errors, { error => $@ } if $@;
             }
@@ -411,7 +415,7 @@ if ( $op eq 'do' ) {
             my $category_description = $patron->category->description;
             my $borrower = $patron->unblessed;
             $borrower->{category_description} = $category_description;
-            my $attributes = $patron->get_extended_attributes;
+            my $attributes = $patron->extended_attributes;
             $borrower->{patron_attributes} = $attributes->as_list;
             $max_nb_attr = $attributes->count if $attributes->count > $max_nb_attr;
             push @borrowers, $borrower;