From 8ea2f1db86d78527e0cd47be6f8d4b18b5ec660a Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 13 Jul 2018 16:40:14 -0300 Subject: [PATCH] Bug 20443: Remove CheckUniqueness There is already a method in Koha::Patron::Attribute to check the uniqueness constraint, let us it to replace CheckUniqueness Signed-off-by: Signed-off-by: Nick Clemens --- C4/Auth_with_ldap.pm | 11 ++--- C4/Members/Attributes.pm | 46 +-------------------- Koha/Patron/Attribute.pm | 15 ++++--- members/memberentry.pl | 63 +++++++++++++++-------------- opac/opac-memberentry.pl | 10 +++-- t/db_dependent/Members/Attributes.t | 44 +++++++++++--------- 6 files changed, 80 insertions(+), 109 deletions(-) diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm index 3cff983bf7..cc05550ca3 100644 --- a/C4/Auth_with_ldap.pm +++ b/C4/Auth_with_ldap.pm @@ -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}"; } } } diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm index c48828c67a..c28d59bac8 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(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"; diff --git a/Koha/Patron/Attribute.pm b/Koha/Patron/Attribute.pm index 5dd63e952f..8d781b77b1 100644 --- a/Koha/Patron/Attribute.pm +++ b/Koha/Patron/Attribute.pm @@ -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; diff --git a/members/memberentry.pl b/members/memberentry.pl index 4b3a0c0f13..88f21e1903 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -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'; @@ -832,7 +832,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(); @@ -844,7 +844,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; } @@ -859,15 +859,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 = (); @@ -886,11 +887,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'); diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index 4f3c1de98b..5899380454 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -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; @@ -664,7 +666,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} @@ -677,7 +679,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; } } diff --git a/t/db_dependent/Members/Attributes.t b/t/db_dependent/Members/Attributes.t index b1b001d432..593bb50a1a 100644 --- a/t/db_dependent/Members/Attributes.t +++ b/t/db_dependent/Members/Attributes.t @@ -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'); -- 2.20.1