From ad08cdf7b68af365f6450efdd4053ed3319d2961 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 13 Jul 2018 13:07:05 -0300 Subject: [PATCH] Bug 20443: Remove DeleteBorrowerAttribute Signed-off-by: Signed-off-by: Nick Clemens --- C4/Members/Attributes.pm | 24 ++----------------- Koha/Patron.pm | 14 +++++------ opac/opac-user.pl | 4 ++-- t/db_dependent/Auth_with_ldap.t | 2 +- t/db_dependent/Koha/Patrons.t | 16 ++++++------- t/db_dependent/Members/Attributes.t | 36 +++++++++++------------------ tools/modborrowers.pl | 2 +- 7 files changed, 34 insertions(+), 64 deletions(-) diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm index d52584096c..6f5001d12c 100644 --- a/C4/Members/Attributes.pm +++ b/C4/Members/Attributes.pm @@ -30,7 +30,7 @@ our ($csv, $AttributeTypes); BEGIN { @ISA = qw(Exporter); @EXPORT_OK = qw(CheckUniqueness SetBorrowerAttributes - DeleteBorrowerAttribute UpdateBorrowerAttribute + UpdateBorrowerAttribute extended_attributes_code_value_arrayref extended_attributes_merge SearchIdMatchingAttribute); %EXPORT_TAGS = ( all => \@EXPORT_OK ); @@ -147,26 +147,6 @@ sub SetBorrowerAttributes { return 1; # borrower attributes successfully set } -=head2 DeleteBorrowerAttribute - - DeleteBorrowerAttribute($borrowernumber, $attribute); - -Delete a borrower attribute for the patron identified by C<$borrowernumber> and the attribute code of C<$attribute> - -=cut - -sub DeleteBorrowerAttribute { - my ( $borrowernumber, $attribute ) = @_; - - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare(qq{ - DELETE FROM borrower_attributes - WHERE borrowernumber = ? - AND code = ? - } ); - $sth->execute( $borrowernumber, $attribute->{code} ); -} - =head2 UpdateBorrowerAttribute UpdateBorrowerAttribute($borrowernumber, $attribute ); @@ -178,7 +158,7 @@ Update a borrower attribute C<$attribute> for the patron identified by C<$borrow sub UpdateBorrowerAttribute { my ( $borrowernumber, $attribute ) = @_; - DeleteBorrowerAttribute $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 = ?"; diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 35b666cc29..d7bea28b4f 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1556,11 +1556,11 @@ sub add_guarantor { )->store(); } -=head3 get_extended_attribute_value +=head3 get_extended_attribute -my $attribute_value = $patron->get_extended_attribute_value( $code ); +my $attribute_value = $patron->get_extended_attribute( $code ); -Return the attribute's value for the code passed in parameter. +Return the attribute for the code passed in parameter. It not exist it returns undef @@ -1571,13 +1571,13 @@ Maybe you certainly not want to use this method, it is actually only used for SH =cut -sub get_extended_attribute_value { - my ( $self, $code ) = @_; +sub get_extended_attribute { + my ( $self, $code, $value ) = @_; my $rs = $self->_result->borrower_attributes; return unless $rs; - my $attribute = $rs->search( { code => $code } ); + my $attribute = $rs->search({ code => $code, ( $value ? ( attribute => $value ) : () ) }); return unless $attribute->count; - return $attribute->next->attribute; + return $attribute->next; } =head3 to_api diff --git a/opac/opac-user.pl b/opac/opac-user.pl index c360363731..16a1c0f36d 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -289,8 +289,8 @@ $template->param( overdues_count => $overdues_count ); 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 = $patron->get_extended_attribute_value(ATTRIBUTE_SHOW_BARCODE); - undef $show_barcode if defined($patron_show_barcode) && !$patron_show_barcode; + my $patron_show_barcode = $patron->get_extended_attribute(ATTRIBUTE_SHOW_BARCODE); + undef $show_barcode if $patron_show_barcode and not $patron_show_barcode->attribute; } $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 5fd03efd51..001f2a8c27 100755 --- a/t/db_dependent/Auth_with_ldap.t +++ b/t/db_dependent/Auth_with_ldap.t @@ -210,7 +210,7 @@ subtest 'checkpw_ldap tests' => sub { 'Extended attributes are not deleted' ); - is( $patron->get_extended_attribute_value( $attr_type2->{code} ), 'BAR', 'Mapped attribute is BAR' ); + is( $patron->get_extended_attribute( $attr_type2->{code} )->attribute, 'BAR', 'Mapped attribute is BAR' ); $auth->unmock('update_local'); $auth->unmock('ldap_entry_2_hash'); diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index daee4f9b89..2a39023d48 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -2085,12 +2085,12 @@ subtest 'get_extended_attributes' => sub { 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' ); + $attribute_12 = $patron_2->get_extended_attribute( $attribute_type1->code ); + is( $attribute_12->attribute, 'my attribute12', 'Koha::Patron->get_extended_attribute 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' ); + # TODO - What about multiple? POD explains the problem + my $non_existent = $patron_2->get_extended_attribute( 'not_exist' ); + is( $non_existent, undef, 'Koha::Patron->get_extended_attribute must return undef if the attribute does not exist' ); # Test branch limitations set_logged_in_user($patron_2); @@ -2103,11 +2103,11 @@ subtest 'get_extended_attributes' => sub { is( $extended_attributes_for_1->count, 2, 'There should be 2 attributes for patron 1, the limited one should be returned'); # Not filtered - my $limited_value = $patron_1->get_extended_attribute_value( $attribute_type_limited->code ); - is( $limited_value, 'my attribute limited', ); + my $limited_value = $patron_1->get_extended_attribute( $attribute_type_limited->code ); + is( $limited_value->attribute, 'my attribute limited', ); ## Do we need a filtered? - #$limited_value = $patron_1->get_extended_attribute_value( $attribute_type_limited->code ); + #$limited_value = $patron_1->get_extended_attribute( $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 33e8b3d3e9..e5c525dce8 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 => 41; +use Test::More tests => 38; use_ok('C4::Members::Attributes'); @@ -124,13 +124,13 @@ $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'); +$attribute_value = $patron->get_extended_attribute('my invalid code'); is( $attribute_value, undef, 'non existent attribute should undef'); -$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' ); +$attribute_value = $patron->get_extended_attribute($attributes->[0]->{code}); +is( $attribute_value->attribute, $attributes->[0]->{value}, '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' ); my $attribute = { @@ -174,27 +174,17 @@ for my $attr( split(' ', $attributes->[1]->{value}) ) { } -C4::Members::Attributes::DeleteBorrowerAttribute(); +$patron->get_extended_attribute($attribute->{code})->delete; $borrower_attributes = $patron->get_extended_attributes; -is( $borrower_attributes->count, 3, 'DeleteBorrowerAttribute without arguments deletes nothing' ); -C4::Members::Attributes::DeleteBorrowerAttribute($borrowernumber); -$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 = $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 = $patron->get_extended_attributes; -is( $borrower_attributes->count, 2, 'DeleteBorrowerAttribute deletes a borrower attribute' ); +is( $borrower_attributes->count, 2, 'delete attribute by code' ); $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'); +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'); -C4::Members::Attributes::DeleteBorrowerAttribute($borrowernumber, $attributes->[1]); +$patron->get_extended_attribute($attributes->[1]->{code})->delete; $borrower_attributes = $patron->get_extended_attributes; -is( $borrower_attributes->count, 1, 'DeleteBorrowerAttribute deletes a borrower attribute' ); +is( $borrower_attributes->count, 1, 'delete attribute by code' ); # Regression tests for bug 16504 t::lib::Mocks::mock_userenv({ branchcode => $new_library->{branchcode} }); diff --git a/tools/modborrowers.pl b/tools/modborrowers.pl index ecc0e67794..e368368979 100755 --- a/tools/modborrowers.pl +++ b/tools/modborrowers.pl @@ -388,7 +388,7 @@ if ( $op eq 'do' ) { if ( grep { $_ eq $valuename } @disabled ) { # The attribute is disabled, we remove it for this borrower ! eval { - C4::Members::Attributes::DeleteBorrowerAttribute( $borrowernumber, $attribute ); + $patron->get_extended_attribute($attribute->{code})->delete; }; push @errors, { error => $@ } if $@; } else { -- 2.20.1