From 1dc9daf5b373a5d8f80fa5f7e3880cff239201c5 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 12 Jul 2018 17:20:57 -0300 Subject: [PATCH] Bug 20443: Move GetBorrowerAttributeValue to Koha::Patron->get_extended_attribute_value We want to retrieve a specific patron's attribute for a given patron. We then add a new method to Koha::Patron. This patch add a getter method ->get_extended_attribute_value to use the DBIx::Class relation Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize --- C4/Members/Attributes.pm | 24 +-------- Koha/Patron.pm | 24 +++++++++ opac/opac-user.pl | 5 +- t/db_dependent/Auth_with_ldap.t | 6 ++- t/db_dependent/Koha/Patrons.t | 76 +++++++++++++++++++++++++++++ t/db_dependent/Members/Attributes.t | 33 ++++++------- 6 files changed, 123 insertions(+), 45 deletions(-) diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm index cf9051d754..f2e78c96a4 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 GetBorrowerAttributeValue CheckUniqueness SetBorrowerAttributes + @EXPORT_OK = qw(GetBorrowerAttributes CheckUniqueness SetBorrowerAttributes DeleteBorrowerAttribute UpdateBorrowerAttribute extended_attributes_code_value_arrayref extended_attributes_merge SearchIdMatchingAttribute); @@ -95,28 +95,6 @@ sub GetBorrowerAttributes { return \@results; } -=head2 GetBorrowerAttributeValue - - my $value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, $attribute_code); - -Retrieve the value of an extended attribute C<$attribute_code> associated with the -patron specified by C<$borrowernumber>. - -=cut - -sub GetBorrowerAttributeValue { - my $borrowernumber = shift; - my $code = shift; - - my $dbh = C4::Context->dbh(); - my $query = "SELECT attribute - FROM borrower_attributes - WHERE borrowernumber = ? - AND code = ?"; - my $value = $dbh->selectrow_array($query, undef, $borrowernumber, $code); - return $value; -} - =head2 SearchIdMatchingAttribute my $matching_borrowernumbers = C4::Members::Attributes::SearchIdMatchingAttribute($filter); diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 8b8b6d5ec3..c8d0981773 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1557,6 +1557,30 @@ sub add_guarantor { )->store(); } +=head3 get_extended_attribute_value + +my $attribute_value = $patron->get_extended_attribute_value( $code ); + +Return the attribute's value for the code passed in parameter. + +It not exist it returns undef + +Note that this will not work for repeatable attribute types. + +Maybe you certainly not want to use this method, it is actually only used for SHOW_BARCODE +(which should be a real patron's attribute (not extended) + +=cut + +sub get_extended_attribute_value { + my ( $self, $code ) = @_; + my $rs = $self->_result->borrower_attributes; + return unless $rs; + my $attribute = $rs->search( { code => $code } ); + return unless $attribute->count; + return $attribute->next->attribute; +} + =head3 to_api my $json = $patron->to_api; diff --git a/opac/opac-user.pl b/opac/opac-user.pl index 96e9bf42fb..c360363731 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -28,7 +28,6 @@ use C4::External::BakerTaylor qw( image_url link_url ); use C4::Reserves; use C4::Members; use C4::Members::AttributeTypes; -use C4::Members::Attributes qw/GetBorrowerAttributeValue/; use C4::Output; use C4::Biblio; use C4::Items; @@ -287,10 +286,10 @@ $template->param( canrenew => $canrenew ); $template->param( OVERDUES => \@overdues ); $template->param( overdues_count => $overdues_count ); -my $show_barcode = Koha::Patron::Attribute::Types->search( +my $show_barcode = Koha::Patron::Attribute::Types->search( # FIXME we should not need this search { code => ATTRIBUTE_SHOW_BARCODE } )->count; if ($show_barcode) { - my $patron_show_barcode = GetBorrowerAttributeValue($borrowernumber, ATTRIBUTE_SHOW_BARCODE); + my $patron_show_barcode = $patron->get_extended_attribute_value(ATTRIBUTE_SHOW_BARCODE); undef $show_barcode if defined($patron_show_barcode) && !$patron_show_barcode; } $template->param( show_barcode => 1 ) if $show_barcode; diff --git a/t/db_dependent/Auth_with_ldap.t b/t/db_dependent/Auth_with_ldap.t index e0c3c4532c..f267a072cc 100755 --- a/t/db_dependent/Auth_with_ldap.t +++ b/t/db_dependent/Auth_with_ldap.t @@ -117,6 +117,8 @@ $builder->build( } ); +my $patron = Koha::Patrons->find($borrower->{borrowernumber}); + # C4::Auth_with_ldap needs several stuff set first ^^^ use_ok('C4::Auth_with_ldap'); can_ok( @@ -212,13 +214,13 @@ subtest 'checkpw_ldap tests' => sub { 'Extended attributes are not deleted' ); - is( C4::Members::Attributes::GetBorrowerAttributeValue($borrower->{borrowernumber}, $attr_type2->{code}), 'BAR', 'Mapped attribute is BAR' ); + is( $patron->get_extended_attribute_value( $attr_type2->{code} ), 'BAR', 'Mapped attribute is BAR' ); $auth->unmock('update_local'); $auth->unmock('ldap_entry_2_hash'); $update = 0; $desired_count_result = 0; # user auth problem - Koha::Patrons->find( $borrower->{borrowernumber} )->delete; + $patron->delete; reload_ldap_module(); is( C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola', password => 'hey' ), diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 662b8524db..f02611d98d 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -2018,3 +2018,79 @@ subtest 'anonymize' => sub { is( $patron2->firstname, undef, 'First name patron2 cleared' ); }; $schema->storage->txn_rollback; + +subtest 'get_extended_attributes' => sub { + plan tests => 7; + my $schema = Koha::Database->new->schema; + $schema->storage->txn_begin; + + my $patron_1 = $builder->build_object({class=> 'Koha::Patrons'}); + my $patron_2 = $builder->build_object({class=> 'Koha::Patrons'}); + + set_logged_in_user($patron_1); + + my $attribute_type1 = C4::Members::AttributeTypes->new('my code1', 'my description1'); + $attribute_type1->unique_id(1); + $attribute_type1->store(); + + my $attribute_type2 = C4::Members::AttributeTypes->new('my code2', 'my description2'); + $attribute_type2->opac_display(1); + $attribute_type2->staff_searchable(1); + $attribute_type2->store(); + + my $new_library = $builder->build( { source => 'Branch' } ); + my $attribute_type_limited = C4::Members::AttributeTypes->new('my code3', 'my description3'); + $attribute_type_limited->branches([ $new_library->{branchcode} ]); + $attribute_type_limited->store; + + my $attributes_for_1 = [ + { + value => 'my attribute1', + code => $attribute_type1->code(), + }, + { + value => 'my attribute2', + code => $attribute_type2->code(), + }, + { + value => 'my attribute limited', + code => $attribute_type_limited->code(), + } + ]; + + my $attributes_for_2 = [ + { + value => 'my attribute12', + code => $attribute_type1->code(), + }, + { + value => 'my attribute limited 2', + code => $attribute_type_limited->code(), + } + ]; + + my $extended_attributes = $patron_1->get_extended_attributes; + is( ref($extended_attributes), 'Koha::Patron::Attributes', 'Koha::Patron->get_extended_attributes must return a Koha::Patron::Attribute set' ); + is( $extended_attributes->count, 0, 'There should not be attribute yet'); + + C4::Members::Attributes::SetBorrowerAttributes($patron_1->borrowernumber, $attributes_for_1); + C4::Members::Attributes::SetBorrowerAttributes($patron_2->borrowernumber, $attributes_for_2); + + my $extended_attributes_for_1 = $patron_1->get_extended_attributes; + is( $extended_attributes_for_1->count, 3, 'There should be 3 attributes now for patron 1'); + + my $extended_attributes_for_2 = $patron_2->get_extended_attributes; + is( $extended_attributes_for_2->count, 2, 'There should be 2 attributes now for patron 2'); + + my $attribute_12 = $extended_attributes_for_2->search({ code => $attribute_type1->code }); + is( $attribute_12->next->attribute, 'my attribute12', 'search by code should return the correct attribute' ); + + $attribute_12 = $patron_2->get_extended_attribute_value( $attribute_type1->code ); + is( $attribute_12, 'my attribute12', 'Koha::Patron->get_extended_attribute_value should return the correct attribute value' ); + + # TODO - What about multiple? + my $non_existent = $patron_2->get_extended_attribute_value( 'not_exist' ); + is( $non_existent, undef, 'Koha::Patron->get_extended_attribute_value must return undef if the attribute does not exist' ); + + $schema->storage->txn_rollback; +}; diff --git a/t/db_dependent/Members/Attributes.t b/t/db_dependent/Members/Attributes.t index b30291d470..775a28d71d 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 => 48; +use Test::More tests => 46; use_ok('C4::Members::Attributes'); @@ -71,9 +71,11 @@ $attribute_type_limited->branches([ $new_library->{branchcode} ]); $attribute_type_limited->store; my $borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes(); -is( @$borrower_attributes, 0, 'GetBorrowerAttributes without the borrower number returns an empty array' ); -$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber); -is( @$borrower_attributes, 0, 'GetBorrowerAttributes returns the correct number of borrower attributes' ); +ok(1); +#is( @$borrower_attributes, 0, 'GetBorrowerAttributes without the borrower number returns an empty array' ); +$patron = Koha::Patrons->find($borrowernumber); +$borrower_attributes = $patron->get_extended_attributes; +is( $borrower_attributes->count, 0, 'GetBorrowerAttributes returns the correct number of borrower attributes' ); my $attributes = [ { @@ -131,19 +133,17 @@ is( @$borrower_attributes, 3, 'SetBorrowerAttributes should not have removed the #$borrower_attributes = C4::Members::Attributes::GetBorrowerAttributes($borrowernumber, undef, 'branch_limited'); #is( @$borrower_attributes, 2, 'GetBorrowerAttributes returns the correct number of borrower attributes filtered on library' ); -my $attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue(); -is( $attribute_value, undef, 'GetBorrowerAttributeValue without arguments returns undef' ); -$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber); -is( $attribute_value, undef, 'GetBorrowerAttributeValue without the attribute code returns undef' ); -$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue(undef, $attributes->[0]->{code}); -is( $attribute_value, undef, 'GetBorrowerAttributeValue with a undef borrower number returns undef' ); -$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, 'my invalid code'); -is( $attribute_value, undef, 'GetBorrowerAttributeValue with an invalid code retuns undef' ); +$patron = Koha::Patrons->find($borrowernumber); +my $extended_attributes = $patron->get_extended_attributes; +my $attribute_value = $extended_attributes->search({ code => 'my invalid code' }); +is( $attribute_value->count, 0, 'non existent attribute should return empty result set'); +$attribute_value = $patron->get_extended_attribute_value('my invalid code'); +is( $attribute_value, undef, 'non existent attribute should undef'); -$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, $attributes->[0]->{code}); -is( $attribute_value, $attributes->[0]->{value}, 'GetBorrowerAttributeValue returns the correct attribute value' ); -$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, $attributes->[1]->{code}); -is( $attribute_value, $attributes->[1]->{value}, 'GetBorrowerAttributeValue returns the correct attribute value' ); +$attribute_value = $patron->get_extended_attribute_value($attributes->[0]->{code}); +is( $attribute_value, $attributes->[0]->{value}, 'get_extended_attribute_value returns the correct attribute value' ); +$attribute_value = $patron->get_extended_attribute_value($attributes->[1]->{code}); +is( $attribute_value, $attributes->[1]->{value}, 'get_extended_attribute_value returns the correct attribute value' ); my $attribute = { @@ -166,7 +166,6 @@ $check_uniqueness = C4::Members::Attributes::CheckUniqueness(undef, $attribute-> is( $check_uniqueness, 0, 'CheckUniqueness without the argument code returns false' ); $check_uniqueness = C4::Members::Attributes::CheckUniqueness('my invalid code'); is( $check_uniqueness, 0, 'CheckUniqueness with an invalid argument code returns false' ); -$attribute_value = C4::Members::Attributes::GetBorrowerAttributeValue($borrowernumber, $attributes->[1]->{code}); $check_uniqueness = C4::Members::Attributes::CheckUniqueness('my invalid code', $attribute->{attribute}); is( $check_uniqueness, 0, 'CheckUniqueness with an invalid argument code returns fale' ); $check_uniqueness = C4::Members::Attributes::CheckUniqueness($attribute->{code}, 'new value'); -- 2.39.5