From 9707167a482894f270549ae9633eaef980f1ed35 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 12 Jul 2018 18:26:58 -0300 Subject: [PATCH] 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 Signed-off-by: Martin Renvoize --- C4/Letters.pm | 11 ++- C4/Members/Attributes.pm | 51 +--------- Koha/Patron.pm | 13 ++- Koha/Patron/Attribute.pm | 47 ++++++++- Koha/Patron/Attributes.pm | 1 + Koha/Patrons/Import.pm | 2 +- .../prog/en/includes/circ-menu.inc | 10 +- .../prog/en/modules/members/moremember.tt | 8 +- .../prog/en/modules/tools/modborrowers.tt | 3 +- members/memberentry.pl | 7 +- members/moremember.pl | 9 +- opac/opac-memberentry.pl | 1 - t/db_dependent/Auth_with_ldap.t | 6 +- t/db_dependent/Koha/Patron/Modifications.t | 21 ++-- t/db_dependent/Koha/Patrons.t | 11 ++- t/db_dependent/Members/Attributes.t | 97 ++++++++----------- tools/modborrowers.pl | 29 +++--- 17 files changed, 161 insertions(+), 166 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index 7c5d470bb0..a5bb9ead64 100644 --- a/C4/Letters.pm +++ b/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} ||= []; diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm index f2e78c96a4..96fb139410 100644 --- a/C4/Members/Attributes.pm +++ b/C4/Members/Attributes.pm @@ -29,7 +29,7 @@ our ($csv, $AttributeTypes); BEGIN { @ISA = qw(Exporter); - @EXPORT_OK = qw(GetBorrowerAttributes 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); diff --git a/Koha/Patron.pm b/Koha/Patron.pm index c8d0981773..35b666cc29 100644 --- a/Koha/Patron.pm +++ b/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 diff --git a/Koha/Patron/Attribute.pm b/Koha/Patron/Attribute.pm index 0a44cbf49b..5dd63e952f 100644 --- a/Koha/Patron/Attribute.pm +++ b/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 diff --git a/Koha/Patron/Attributes.pm b/Koha/Patron/Attributes.pm index 587189ff4d..0cb6875daf 100644 --- a/Koha/Patron/Attributes.pm +++ b/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 } ); } diff --git a/Koha/Patrons/Import.pm b/Koha/Patrons/Import.pm index 3b3d5865a8..5fe9476d74 100644 --- a/Koha/Patrons/Import.pm +++ b/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 } 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 95ace5e248..fbb4a2b24a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc +++ b/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 ) %] -
  • [% extendedattribute.type_description | html %] : [% IF ( extendedattribute.value_description ) %][% extendedattribute.value_description | html %][% ELSE %][% extendedattribute.attribute | html %][% END %]
    • + [% 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? %] +
  • + [% extendedattribute.type.description | html %]: [% extendedattribute.description | html %] +
    • + [% END %] [% END %] [% END %] [% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt index ab3e89abf7..4953249861 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt @@ -381,12 +381,8 @@
      [% FOREACH item IN attribute.items %]
    1. - [% item.description | html %]: - [% IF ( item.value_description ) %] - [% item.value_description | html %] - [% ELSE %] - [% item.value| html | html_line_break %] - [% END %] + [% item.type.description | html %]: + [% item.description | html_line_break %]
      2. [% END %]
    diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt index adcb860fb3..9147d522f9 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt @@ -191,7 +191,8 @@ [% borrower.debarredcomment | html %] [% FOREACH pa IN borrower.patron_attributes %] [% IF ( pa.code ) %] - [% pa.code | html %]=[% pa.value | html %] + [%# Replace pa.attribute with pa.description if we prefer to display the description %] + [% pa.code | html %]=[% pa.attribute | html %] [% ELSE %] [% END %] diff --git a/members/memberentry.pl b/members/memberentry.pl index ed482b9b81..6f821a3088 100755 --- a/members/memberentry.pl +++ b/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; } diff --git a/members/moremember.pl b/members/moremember.pl index f083203d4a..b14148821a 100755 --- a/members/moremember.pl +++ b/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; diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index 8985c5e18e..669687eeed 100755 --- a/opac/opac-memberentry.pl +++ b/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; diff --git a/t/db_dependent/Auth_with_ldap.t b/t/db_dependent/Auth_with_ldap.t index f267a072cc..5fd03efd51 100755 --- a/t/db_dependent/Auth_with_ldap.t +++ b/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' ); diff --git a/t/db_dependent/Koha/Patron/Modifications.t b/t/db_dependent/Koha/Patron/Modifications.t index 65edba477f..ce279adc6d 100755 --- a/t/db_dependent/Koha/Patron/Modifications.t +++ b/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 } ); diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index f02611d98d..146bdbfb95 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -2020,7 +2020,7 @@ subtest 'anonymize' => sub { $schema->storage->txn_rollback; subtest 'get_extended_attributes' => sub { - plan tests => 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; }; diff --git a/t/db_dependent/Members/Attributes.t b/t/db_dependent/Members/Attributes.t index 775a28d71d..33e8b3d3e9 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 => 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' ); diff --git a/tools/modborrowers.pl b/tools/modborrowers.pl index 1a08a8ffbd..ecc0e67794 100755 --- a/tools/modborrowers.pl +++ b/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; -- 2.39.5