Bugzilla – Attachment 101131 Details for
Bug 20443
Move C4::Members::Attributes to Koha namespace
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 20443: Remove CheckUniqueness
Bug-20443-Remove-CheckUniqueness.patch (text/plain), 17.18 KB, created by
Jonathan Druart
on 2020-03-20 14:47:30 UTC
(
hide
)
Description:
Bug 20443: Remove CheckUniqueness
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2020-03-20 14:47:30 UTC
Size:
17.18 KB
patch
obsolete
>From 8ea2f1db86d78527e0cd47be6f8d4b18b5ec660a Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >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 <nick@bywatersolutions.com> >--- > 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 20443
:
101122
|
101123
|
101124
|
101125
|
101126
|
101127
|
101128
|
101129
|
101130
|
101131
|
101132
|
101133
|
101134
|
101135
|
101136
|
101137
|
101138
|
101139
|
101140
|
101141
|
101142
|
101143
|
101144
|
101145
|
101146
|
101147
|
101148
|
101149
|
101150
|
101209
|
101210
|
101211
|
101212
|
101213
|
101214
|
101215
|
101216
|
101217
|
101218
|
101219
|
101220
|
101221
|
101222
|
101223
|
101224
|
101225
|
101226
|
101227
|
101228
|
101229
|
101230
|
101231
|
101232
|
101233
|
101234
|
101235
|
101236
|
101237
|
101472
|
104519