Bug 20443: Remove CheckUniqueness
authorJonathan Druart <jonathan.druart@bugs.koha-community.org>
Fri, 13 Jul 2018 19:40:14 +0000 (16:40 -0300)
committerMartin Renvoize <martin.renvoize@ptfs-europe.com>
Mon, 23 Mar 2020 13:44:30 +0000 (13:44 +0000)
There is already a method in Koha::Patron::Attribute to check the
uniqueness constraint, let us it to replace CheckUniqueness

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

C4/Auth_with_ldap.pm
C4/Members/Attributes.pm
Koha/Patron/Attribute.pm
members/memberentry.pl
opac/opac-memberentry.pl
t/db_dependent/Members/Attributes.t

index 3cff983..cc05550 100644 (file)
@@ -238,14 +238,15 @@ sub checkpw_ldap {
             unless (exists($borrower{$code}) && $borrower{$code} !~ m/^\s*$/ ) {
                 next;
             }
-            if (C4::Members::Attributes::CheckUniqueness($code, $borrower{$code}, $borrowernumber)) {
-                my $patron = Koha::Patrons->find($borrowernumber);
-                if ( $patron ) { # Should not be needed, but we are in C4::Auth LDAP...
+            my $patron = Koha::Patrons->find($borrowernumber);
+            if ( $patron ) { # Should not be needed, but we are in C4::Auth LDAP...
+                eval {
                     my $attribute = Koha::Patron::Attribute->new({code => $code, attribute => $borrower{$code}});
                     $patron->extended_attributes([$attribute]);
+                };
+                if ($@) { # FIXME Test if Koha::Exceptions::Patron::Attribute::NonRepeatable
+                    warn "ERROR_extended_unique_id_failed $code $borrower{$code}";
                 }
-            } else {
-                warn "ERROR_extended_unique_id_failed $code $borrower{$code}";
             }
         }
     }
index c48828c..c28d59b 100644 (file)
@@ -29,7 +29,7 @@ our ($csv, $AttributeTypes);
 
 BEGIN {
     @ISA = qw(Exporter);
-    @EXPORT_OK = qw(CheckUniqueness
+    @EXPORT_OK = qw(
                     extended_attributes_code_value_arrayref extended_attributes_merge
                     SearchIdMatchingAttribute);
     %EXPORT_TAGS = ( all => \@EXPORT_OK );
@@ -67,50 +67,6 @@ AND (} . join (" OR ", map "attribute like ?", @$filter) .qq{)};
     return [map $_->[0], @{ $sth->fetchall_arrayref }];
 }
 
-=head2 CheckUniqueness
-
-  my $ok = CheckUniqueness($code, $value[, $borrowernumber]);
-
-Given an attribute type and value, verify if would violate
-a unique_id restriction if added to the patron.  The
-optional C<$borrowernumber> is the patron that the attribute
-value would be added to, if known.
-
-Returns false if the C<$code> is not valid or the
-value would violate the uniqueness constraint.
-
-=cut
-
-sub CheckUniqueness {
-    my $code = shift;
-    my $value = shift;
-    my $borrowernumber = @_ ? shift : undef;
-
-    my $attr_type = C4::Members::AttributeTypes->fetch($code);
-
-    return 0 unless defined $attr_type;
-    return 1 unless $attr_type->unique_id();
-
-    my $dbh = C4::Context->dbh;
-    my $sth;
-    if (defined($borrowernumber)) {
-        $sth = $dbh->prepare("SELECT COUNT(*) 
-                              FROM borrower_attributes 
-                              WHERE code = ? 
-                              AND attribute = ?
-                              AND borrowernumber <> ?");
-        $sth->execute($code, $value, $borrowernumber);
-    } else {
-        $sth = $dbh->prepare("SELECT COUNT(*) 
-                              FROM borrower_attributes 
-                              WHERE code = ? 
-                              AND attribute = ?");
-        $sth->execute($code, $value);
-    }
-    my ($count) = $sth->fetchrow_array;
-    return ($count == 0);
-}
-
 =head2 extended_attributes_code_value_arrayref 
 
    my $patron_attributes = "homeroom:1150605,grade:01,extradata:foobar";
index 5dd63e9..8d781b7 100644 (file)
@@ -47,7 +47,7 @@ sub store {
     my $self = shift;
 
     $self->_check_repeatable;
-    $self->_check_unique_id;
+    $self->check_unique_id;
 
     return $self->SUPER::store();
 }
@@ -139,21 +139,26 @@ sub _check_repeatable {
     return $self;
 }
 
-=head3 _check_unique_id
+=head3 check_unique_id
 
-_check_unique_id checks if the attribute type is marked as unique id and throws and exception
+check_unique_id checks if the attribute type is marked as unique id and throws and exception
 if the attribute type is a unique id and there's already an attribute with the same
 code and value on the database.
 
 =cut
 
-sub _check_unique_id {
+sub check_unique_id {
 
     my $self = shift;
 
     if ( $self->type->unique_id ) {
+        my $params = { code => $self->code, attribute => $self->attribute };
+
+        $params->{borrowernumber} = { '!=' => $self->borrowernumber } if $self->borrowernumber;
+        $params->{id}             = { '!=' => $self->id }             if $self->in_storage;
+
         my $unique_count = Koha::Patron::Attributes
-            ->search( { code => $self->code, attribute => $self->attribute } )
+            ->search( $params )
             ->count;
         Koha::Exceptions::Patron::Attribute::UniqueIDConstraint->throw()
             if $unique_count > 0;
index 29219da..ff3c1a6 100755 (executable)
@@ -312,7 +312,7 @@ if ( ( defined $newdata{'userid'} && $newdata{'userid'} eq '' ) || $check_Borrow
 }
   
 $debug and warn join "\t", map {"$_: $newdata{$_}"} qw(dateofbirth dateenrolled dateexpiry);
-my $extended_patron_attributes = ();
+my $extended_patron_attributes;
 if ($op eq 'save' || $op eq 'insert'){
 
     output_and_exit( $input, $cookie, $template,  'wrong_csrf_token' )
@@ -398,19 +398,22 @@ if ($op eq 'save' || $op eq 'insert'){
       push (@errors, "ERROR_bad_email_alternative") if (!Email::Valid->address($emailalt));
   }
 
-  if (C4::Context->preference('ExtendedPatronAttributes')) {
-    $extended_patron_attributes = parse_extended_patron_attributes($input);
-    foreach my $attr (@$extended_patron_attributes) {
-        unless (C4::Members::Attributes::CheckUniqueness($attr->{code}, $attr->{value}, $borrowernumber)) {
-            my $attr_info = C4::Members::AttributeTypes->fetch($attr->{code});
-            push @errors, "ERROR_extended_unique_id_failed";
-            $template->param(
-                ERROR_extended_unique_id_failed_code => $attr->{code},
-                ERROR_extended_unique_id_failed_value => $attr->{value},
-                ERROR_extended_unique_id_failed_description => $attr_info->description()
-            );
-        }
-    }
+  if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) {
+      $extended_patron_attributes = parse_extended_patron_attributes($input);
+      for my $attr ( @$extended_patron_attributes ) {
+          $attr->{borrowernumber} = $borrowernumber if $borrowernumber;
+          my $attribute = Koha::Patron::Attribute->new($attr);
+          eval {$attribute->check_unique_id};
+          if ( $@ ) {
+              push @errors, "ERROR_extended_unique_id_failed";
+              my $attr_info = C4::Members::AttributeTypes->fetch($attr->{code});
+              $template->param(
+                  ERROR_extended_unique_id_failed_code => $attr->{code},
+                  ERROR_extended_unique_id_failed_value => $attr->{attribute},
+                  ERROR_extended_unique_id_failed_description => $attr_info->description()
+              );
+          }
+      }
   }
 }
 
@@ -479,10 +482,6 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){
             }
         }
 
-        if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_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,15 +549,16 @@ 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')) {
-            $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);
         }
        }
 
+    if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) {
+        $patron->extended_attributes->filter_by_branch_limitations->delete;
+        $patron->extended_attributes($extended_patron_attributes);
+    }
+
     if ( $destination eq 'circ' and not C4::Auth::haspermission( C4::Context->userenv->{id}, { circulate => 'circulate_remaining_permissions' } ) ) {
         # If we want to redirect to circulation.pl and need to check if the logged in user has the necessary permission
         $destination = 'not_circ';
@@ -845,7 +845,7 @@ if ( C4::Context->preference('TranslateNotices') ) {
 
 output_html_with_http_headers $input, $cookie, $template->output;
 
-sub  parse_extended_patron_attributes {
+sub parse_extended_patron_attributes {
     my ($input) = @_;
     my @patron_attr = grep { /^patron_attr_\d+$/ } $input->multi_param();
 
@@ -857,7 +857,7 @@ sub  parse_extended_patron_attributes {
         my $code     = $input->param("${key}_code");
         next if exists $dups{$code}->{$value};
         $dups{$code}->{$value} = 1;
-        push @attr, { code => $code, value => $value };
+        push @attr, { code => $code, attribute => $value };
     }
     return \@attr;
 }
@@ -872,15 +872,16 @@ sub patron_attributes_form {
         $template->param(no_patron_attribute_types => 1);
         return;
     }
-    my $patron = Koha::Patrons->find($borrowernumber); # Already fetched but outside of this sub
-    my @attributes = $patron->extended_attributes->as_list; # FIXME Must be improved!
-    my @classes = uniq( map {$_->type->class} @attributes );
-    @classes = sort @classes;
+    my @attributes;
+    if ( $borrowernumber ) {
+        my $patron = Koha::Patrons->find($borrowernumber); # Already fetched but outside of this sub
+        @attributes = $patron->extended_attributes->as_list; # FIXME Must be improved!
+    }
 
     # map patron's attributes into a more convenient structure
     my %attr_hash = ();
     foreach my $attr (@attributes) {
-        push @{ $attr_hash{$attr->{code}} }, $attr;
+        push @{ $attr_hash{$attr->code} }, $attr;
     }
 
     my @attribute_loop = ();
@@ -899,11 +900,11 @@ sub patron_attributes_form {
         if (exists $attr_hash{$attr_type->code()}) {
             foreach my $attr (@{ $attr_hash{$attr_type->code()} }) {
                 my $newentry = { %$entry };
-                $newentry->{value} = $attr->{value};
+                $newentry->{value} = $attr->attribute;
                 $newentry->{use_dropdown} = 0;
                 if ($attr_type->authorised_value_category()) {
                     $newentry->{use_dropdown} = 1;
-                    $newentry->{auth_val_loop} = GetAuthorisedValues($attr_type->authorised_value_category(), $attr->{value});
+                    $newentry->{auth_val_loop} = GetAuthorisedValues($attr_type->authorised_value_category(), $attr->attribute);
                 }
                 $i++;
                 undef $newentry->{value} if ($attr_type->unique_id() && $op eq 'duplicate');
index 6fdc765..f317ac8 100755 (executable)
@@ -98,11 +98,13 @@ my $attributes = ParsePatronAttributes($borrowernumber,$cgi);
 my $conflicting_attribute = 0;
 
 foreach my $attr (@$attributes) {
-    unless ( C4::Members::Attributes::CheckUniqueness($attr->{code}, $attr->{value}, $borrowernumber) ) {
+    my $attribute = Koha::Patron::Attribute->new($attr);
+    eval {$attribute->check_unique_id};
+    if ( $@ ) {
         my $attr_info = C4::Members::AttributeTypes->fetch($attr->{code});
         $template->param(
             extended_unique_id_failed_code => $attr->{code},
-            extended_unique_id_failed_value => $attr->{value},
+            extended_unique_id_failed_value => $attr->{attribute},
             extended_unique_id_failed_description => $attr_info->description()
         );
         $conflicting_attribute = 1;
@@ -665,7 +667,7 @@ sub ParsePatronAttributes {
             }
             else {
                 # we've got a value
-                push @attributes, { code => $code, value => $value };
+                push @attributes, { code => $code, attribute => $value };
 
                 # 'code' is no longer a delete candidate
                 delete $delete_candidates->{$code}
@@ -678,7 +680,7 @@ sub ParsePatronAttributes {
         if ( Koha::Patron::Attributes->search({
                 borrowernumber => $borrowernumber, code => $code })->count > 0 )
         {
-            push @attributes, { code => $code, value => '' }
+            push @attributes, { code => $code, attribute => '' }
                 unless any { $_->{code} eq $code } @attributes;
         }
     }
index b1b001d..593bb50 100644 (file)
@@ -26,7 +26,8 @@ use Koha::Database;
 use t::lib::TestBuilder;
 use t::lib::Mocks;
 
-use Test::More tests => 39;
+use Test::Exception;
+use Test::More tests => 33;
 
 use_ok('C4::Members::Attributes');
 
@@ -150,24 +151,29 @@ is( $attr_0->type->description, $attribute_type1->description(), 'delete then ad
 is( $attr_0->attribute, $attribute->{attribute}, 'delete then add a new attribute updates the field value correctly' );
 
 
-my $check_uniqueness = C4::Members::Attributes::CheckUniqueness();
-is( $check_uniqueness, 0, 'CheckUniqueness without arguments returns false' );
-$check_uniqueness = C4::Members::Attributes::CheckUniqueness($attribute->{code});
-is( $check_uniqueness, 1, 'CheckUniqueness with a valid argument code returns true' );
-$check_uniqueness = C4::Members::Attributes::CheckUniqueness(undef, $attribute->{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' );
-$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');
-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]->{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' );
+lives_ok { # Editing, new value, same patron
+    Koha::Patron::Attribute->new(
+        {
+            code           => $attribute->{code},
+            attribute      => 'new value',
+            borrowernumber => $patron->borrowernumber
+        }
+    )->check_unique_id;
+} 'no exception raised';
+lives_ok { # Editing, same value, same patron
+    Koha::Patron::Attribute->new(
+        {
+            code           => $attributes->[1]->{code},
+            attribute      => $attributes->[1]->{attribute},
+            borrowernumber => $patron->borrowernumber
+        }
+    )->check_unique_id;
+} 'no exception raised';
+throws_ok { # Creating a new one, but already exists!
+    Koha::Patron::Attribute->new(
+        { code => $attribute->{code}, attribute => $attribute->{attribute} } )
+      ->check_unique_id;
+} 'Koha::Exceptions::Patron::Attribute::UniqueIDConstraint';
 
 
 my $borrower_numbers = C4::Members::Attributes::SearchIdMatchingAttribute('attribute1');