Bug 20443: Remove DeleteBorrowerAttribute

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

diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm
index d525840..6f5001d 100644 (file)
--- 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 35b666c..d7bea28 100644 (file)
--- 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 c360363..16a1c0f 100755 (executable)
--- 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 5fd03ef..001f2a8 100755 (executable)
--- 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 daee4f9..2a39023 100644 (file)
--- 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 33e8b3d..e5c525d 100644 (file)
--- 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 ecc0e67..e368368 100755 (executable)
--- 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 {