From ccfc6572f79eccbf8abae9c4645213ae6d33fa38 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 13 Jul 2018 14:11:44 -0300 Subject: [PATCH] Bug 20443: Remove UpdateBorrowerAttribute and SetBorrowerAttributes 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 Signed-off-by: Martin Renvoize --- C4/Auth_with_ldap.pm | 6 +- C4/ILSDI/Services.pm | 2 +- C4/Letters.pm | 2 +- C4/Members.pm | 2 +- C4/Members/Attributes.pm | 66 +------------ Koha/Patron.pm | 33 ++++++- Koha/Patrons/Import.pm | 19 +++- .../prog/en/includes/circ-menu.inc | 2 +- members/memberentry.pl | 8 +- members/moremember.pl | 2 +- opac/opac-memberentry.pl | 3 +- t/db_dependent/Auth_with_ldap.t | 2 +- t/db_dependent/Koha/Patron/Modifications.t | 2 +- t/db_dependent/Koha/Patrons.t | 20 ++-- t/db_dependent/Koha/Patrons/Import.t | 4 +- t/db_dependent/Members/Attributes.t | 92 ++++++++++--------- t/db_dependent/Utils/Datatables_Members.t | 29 ++++-- tools/modborrowers.pl | 10 +- 18 files changed, 157 insertions(+), 147 deletions(-) diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm index 09b2b3f66b..3cff983bf7 100644 --- a/C4/Auth_with_ldap.pm +++ b/C4/Auth_with_ldap.pm @@ -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}"; } diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 0416f5c82e..29cb58a772 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -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; } diff --git a/C4/Letters.pm b/C4/Letters.pm index a5bb9ead64..9e583cd5ae 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -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; diff --git a/C4/Members.pm b/C4/Members.pm index eb5dbfb4e2..9355436c33 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -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; diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm index 6f5001d12c..c48828c67a 100644 --- a/C4/Members/Attributes.pm +++ b/C4/Members/Attributes.pm @@ -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 diff --git a/Koha/Patron.pm b/Koha/Patron.pm index d7bea28b4f..9a06db38c1 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -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; diff --git a/Koha/Patrons/Import.pm b/Koha/Patrons/Import.pm index 5fe9476d74..e644bcbb8a 100644 --- a/Koha/Patrons/Import.pm +++ b/Koha/Patrons/Import.pm @@ -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 diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc index fbb4a2b24a..7b3494268b 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc @@ -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? %]
  • diff --git a/members/memberentry.pl b/members/memberentry.pl index 6f821a3088..29219dab9d 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -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; diff --git a/members/moremember.pl b/members/moremember.pl index b14148821a..ea00da74f8 100755 --- a/members/moremember.pl +++ b/members/moremember.pl @@ -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; diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index 669687eeed..6fdc765b57 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -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, diff --git a/t/db_dependent/Auth_with_ldap.t b/t/db_dependent/Auth_with_ldap.t index 001f2a8c27..5871b87c52 100755 --- a/t/db_dependent/Auth_with_ldap.t +++ b/t/db_dependent/Auth_with_ldap.t @@ -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' ); diff --git a/t/db_dependent/Koha/Patron/Modifications.t b/t/db_dependent/Koha/Patron/Modifications.t index ce279adc6d..3378ccdc0e 100755 --- a/t/db_dependent/Koha/Patron/Modifications.t +++ b/t/db_dependent/Koha/Patron/Modifications.t @@ -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' ); diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 2a39023d48..69331d9be9 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -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 diff --git a/t/db_dependent/Koha/Patrons/Import.t b/t/db_dependent/Koha/Patrons/Import.t index ea3d1d83bf..fdf90fab00 100644 --- a/t/db_dependent/Koha/Patrons/Import.t +++ b/t/db_dependent/Koha/Patrons/Import.t @@ -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'); diff --git a/t/db_dependent/Members/Attributes.t b/t/db_dependent/Members/Attributes.t index e5c525dce8..b1b001d432 100644 --- 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 => 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' ); diff --git a/t/db_dependent/Utils/Datatables_Members.t b/t/db_dependent/Utils/Datatables_Members.t index feca4f88fc..2e76da5615 100644 --- a/t/db_dependent/Utils/Datatables_Members.t +++ b/t/db_dependent/Utils/Datatables_Members.t @@ -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); diff --git a/tools/modborrowers.pl b/tools/modborrowers.pl index e368368979..dff86263c2 100755 --- a/tools/modborrowers.pl +++ b/tools/modborrowers.pl @@ -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; -- 2.39.5