Bugzilla – Attachment 120613 Details for
Bug 28220
Exception not caught when importing patrons
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28220: Add more tests
Bug-28220-Add-more-tests.patch (text/plain), 15.79 KB, created by
Jonathan Druart
on 2021-05-06 10:49:21 UTC
(
hide
)
Description:
Bug 28220: Add more tests
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2021-05-06 10:49:21 UTC
Size:
15.79 KB
patch
obsolete
>From 246ffb0015246a36bea9c1eefca75c494ee7b512 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Thu, 6 May 2021 12:31:47 +0200 >Subject: [PATCH] Bug 28220: Add more tests > >* Add the transaction when a patron is created >* The changes in merge_and_replace_with prevent the creation of the >patron and so the attributes if several non-repeatable attributes are >passed >--- > Koha/Patron/Attributes.pm | 9 +- > Koha/Patrons/Import.pm | 156 +++++++++++++++------------ > t/db_dependent/Koha/Patrons/Import.t | 71 +++++++++++- > 3 files changed, 158 insertions(+), 78 deletions(-) > >diff --git a/Koha/Patron/Attributes.pm b/Koha/Patron/Attributes.pm >index 7b7c42b7f6c..0d922b6fa6a 100644 >--- a/Koha/Patron/Attributes.pm >+++ b/Koha/Patron/Attributes.pm >@@ -96,8 +96,9 @@ $new_attributes is an arrayref of hashrefs > sub merge_and_replace_with { > my ( $self, $new_attributes ) = @_; > >- my @merged = @{$self->unblessed}; >+ my @existing_attributes = @{$self->unblessed}; > my $attribute_types = { map { $_->code => $_->unblessed } Koha::Patron::Attribute::Types->search }; >+ my @new_attributes; > for my $attr ( @$new_attributes ) { > > my $attribute_type = $attribute_types->{$attr->{code}}; >@@ -107,13 +108,13 @@ sub merge_and_replace_with { > > unless ( $attribute_type->{repeatable} ) { > # filter out any existing attributes of the same code >- @merged = grep {$attr->{code} ne $_->{code}} @merged; >+ @existing_attributes = grep {$attr->{code} ne $_->{code}} @existing_attributes; > } > >- push @merged, $attr; >+ push @new_attributes, $attr; > } > >- @merged = map { { code => $_->{code}, attribute => $_->{attribute} } } @merged; >+ my @merged = map { { code => $_->{code}, attribute => $_->{attribute} } } ( @existing_attributes, @new_attributes ); > > # WARNING - we would like to return a set, but $new_attributes is not in storage yet > # Maybe there is something obvious I (JD) am missing >diff --git a/Koha/Patrons/Import.pm b/Koha/Patrons/Import.pm >index 38b1c7f44d1..14a47ae8796 100644 >--- a/Koha/Patrons/Import.pm >+++ b/Koha/Patrons/Import.pm >@@ -241,6 +241,7 @@ sub import_patrons { > if exists $borrower{$field} and $borrower{$field} eq ""; > } > >+ my $success = 1; > if ($borrowernumber) { > > # borrower exists >@@ -270,9 +271,87 @@ sub import_patrons { > } > > my $patron = Koha::Patrons->find( $borrowernumber ); >- eval { $patron->set(\%borrower)->store }; >- if ( $@ ) { >+ try { >+ $schema->storage->txn_do(sub { >+ $patron->set(\%borrower)->store; >+ # Don't add a new restriction if the existing 'combined' restriction matches this one >+ if ( $borrower{debarred} && ( ( $borrower{debarred} ne $member->{debarred} ) || ( $borrower{debarredcomment} ne $member->{debarredcomment} ) ) ) { >+ >+ # Check to see if this debarment already exists >+ my $debarrments = GetDebarments( >+ { >+ borrowernumber => $borrowernumber, >+ expiration => $borrower{debarred}, >+ comment => $borrower{debarredcomment} >+ } >+ ); >+ >+ # If it doesn't, then add it! >+ unless (@$debarrments) { >+ AddDebarment( >+ { >+ borrowernumber => $borrowernumber, >+ expiration => $borrower{debarred}, >+ comment => $borrower{debarredcomment} >+ } >+ ); >+ } >+ } >+ if ($patron->category->category_type ne 'S' && $overwrite_passwords && defined $borrower{password} && $borrower{password} ne ''){ >+ try { >+ $patron->set_password({ password => $borrower{password} }); >+ } >+ catch { >+ if ( $_->isa('Koha::Exceptions::Password::TooShort') ) { >+ push @errors, { passwd_too_short => 1, borrowernumber => $borrowernumber, length => $_->{length}, min_length => $_->{min_length} }; >+ } >+ elsif ( $_->isa('Koha::Exceptions::Password::WhitespaceCharacters') ) { >+ push @errors, { passwd_whitespace => 1, borrowernumber => $borrowernumber } ; >+ } >+ elsif ( $_->isa('Koha::Exceptions::Password::TooWeak') ) { >+ push @errors, { passwd_too_weak => 1, borrowernumber => $borrowernumber } ; >+ } >+ elsif ( $_->isa('Koha::Exceptions::Password::Plugin') ) { >+ push @errors, { passwd_plugin_err => 1, borrowernumber => $borrowernumber } ; >+ } >+ else { >+ push @errors, { passwd_unknown_err => 1, borrowernumber => $borrowernumber } ; >+ } >+ } >+ } >+ if ($extended) { >+ if ($ext_preserve) { >+ $patron_attributes = $patron->extended_attributes->merge_and_replace_with( $patron_attributes ); >+ } >+ # We do not want to filter by branch, maybe we should? >+ Koha::Patrons->find($borrowernumber)->extended_attributes->delete; >+ $patron->extended_attributes($patron_attributes); >+ } >+ $overwritten++; >+ push( >+ @feedback, >+ { >+ feedback => 1, >+ name => 'lastoverwritten', >+ value => $borrower{'surname'} . ' / ' . $borrowernumber >+ } >+ ); >+ }); >+ } catch { > $invalid++; >+ $success = 0; >+ >+ my $patron_id = defined $matchpoint ? $borrower{$matchpoint} : $matchpoint_attr_type; >+ if ( $_->isa('Koha::Exceptions::Patron::Attribute::UniqueIDConstraint') ) { >+ push @errors, { patron_attribute_unique_id_constraint => 1, patron_id => $patron_id, attribute => $_->attribute }; >+ } elsif ( $_->isa('Koha::Exceptions::Patron::Attribute::InvalidType') ) { >+ push @errors, { patron_attribute_invalid_type => 1, patron_id => $patron_id, attribute_type_code => $_->type }; >+ } elsif ( $_->isa('Koha::Exceptions::Patron::Attribute::NonRepeatable') ) { >+ push @errors, { patron_attribute_non_repeatable => 1, patron_id => $patron_id, attribute => $_->attribute }; >+ } else { >+ use Data::Printer colored => 1; warn p $_; >+ push @errors, { unknown_error => 1 }; >+ } > > push( > @errors, >@@ -282,76 +361,7 @@ sub import_patrons { > value => $borrower{'surname'} . ' / ' . $borrowernumber > } > ); >- next LINE; >- } >- # Don't add a new restriction if the existing 'combined' restriction matches this one >- if ( $borrower{debarred} && ( ( $borrower{debarred} ne $member->{debarred} ) || ( $borrower{debarredcomment} ne $member->{debarredcomment} ) ) ) { >- >- # Check to see if this debarment already exists >- my $debarrments = GetDebarments( >- { >- borrowernumber => $borrowernumber, >- expiration => $borrower{debarred}, >- comment => $borrower{debarredcomment} >- } >- ); >- >- # If it doesn't, then add it! >- unless (@$debarrments) { >- AddDebarment( >- { >- borrowernumber => $borrowernumber, >- expiration => $borrower{debarred}, >- comment => $borrower{debarredcomment} >- } >- ); >- } > } >- if ($patron->category->category_type ne 'S' && $overwrite_passwords && defined $borrower{password} && $borrower{password} ne ''){ >- try { >- $patron->set_password({ password => $borrower{password} }); >- } >- catch { >- if ( $_->isa('Koha::Exceptions::Password::TooShort') ) { >- push @errors, { passwd_too_short => 1, borrowernumber => $borrowernumber, length => $_->{length}, min_length => $_->{min_length} }; >- } >- elsif ( $_->isa('Koha::Exceptions::Password::WhitespaceCharacters') ) { >- push @errors, { passwd_whitespace => 1, borrowernumber => $borrowernumber } ; >- } >- elsif ( $_->isa('Koha::Exceptions::Password::TooWeak') ) { >- push @errors, { passwd_too_weak => 1, borrowernumber => $borrowernumber } ; >- } >- elsif ( $_->isa('Koha::Exceptions::Password::Plugin') ) { >- push @errors, { passwd_plugin_err => 1, borrowernumber => $borrowernumber } ; >- } >- else { >- push @errors, { passwd_unknown_err => 1, borrowernumber => $borrowernumber } ; >- } >- } >- } >- if ($extended) { >- if ($ext_preserve) { >- $patron_attributes = $patron->extended_attributes->merge_and_replace_with( $patron_attributes ); >- } >- eval { >- # We do not want to filter by branch, maybe we should? >- Koha::Patrons->find($borrowernumber)->extended_attributes->delete; >- $patron->extended_attributes($patron_attributes); >- }; >- if ($@) { >- # FIXME This is not an unknown error, we can do better here >- push @errors, { unknown_error => 1 }; >- } >- } >- $overwritten++; >- push( >- @feedback, >- { >- feedback => 1, >- name => 'lastoverwritten', >- value => $borrower{'surname'} . ' / ' . $borrowernumber >- } >- ); > } > else { > try { >@@ -397,6 +407,7 @@ sub import_patrons { > }); > } catch { > $invalid++; >+ $success = 0; > my $patron_id = defined $matchpoint ? $borrower{$matchpoint} : $matchpoint_attr_type; > if ( $_->isa('Koha::Exceptions::Patron::Attribute::UniqueIDConstraint') ) { > push @errors, { patron_attribute_unique_id_constraint => 1, patron_id => $patron_id, attribute => $_->attribute }; >@@ -406,6 +417,7 @@ sub import_patrons { > push @errors, { patron_attribute_non_repeatable => 1, patron_id => $patron_id, attribute => $_->attribute }; > > } else { >+ use Data::Printer colored => 1; warn p $_; > push @errors, { unknown_error => 1 }; > } > push( >@@ -418,6 +430,8 @@ sub import_patrons { > }; > } > >+ next LINE unless $success; >+ > # Add a guarantor if we are given a relationship > if ( $guarantor_id ) { > my $relationship = Koha::Patron::Relationships->find( >diff --git a/t/db_dependent/Koha/Patrons/Import.t b/t/db_dependent/Koha/Patrons/Import.t >index bef31e56f81..b3ba72ee9d8 100755 >--- a/t/db_dependent/Koha/Patrons/Import.t >+++ b/t/db_dependent/Koha/Patrons/Import.t >@@ -969,7 +969,7 @@ subtest 'patron_attributes' => sub { > } > > subtest 'update existing patron' => sub { >- plan tests => 6; >+ plan tests => 19; > > my $patron = $builder->build_object( > { >@@ -1016,8 +1016,7 @@ subtest 'patron_attributes' => sub { > # The normal_attribute_type has been replaced with 'my normal attribute 2' > compare_patron_attributes($patron->extended_attributes->unblessed, { %$attributes, %$new_attributes } ); > >- >- # uniqueness >+ # UniqueIDConstraint > $patron->extended_attributes->delete; # reset > $builder->build_object( > { >@@ -1041,6 +1040,72 @@ subtest 'patron_attributes' => sub { > ); > > is( $result->{overwritten}, 0 ); >+ my $error = $result->{errors}->[0]; >+ is( $error->{patron_attribute_unique_id_constraint}, 1 ); >+ is( $error->{patron_id}, $cardnumber ); >+ is( $error->{attribute}->code, $unique_attribute_type->code ); >+ >+ compare_patron_attributes($patron->extended_attributes->unblessed, {}, ); >+ >+ >+ #InvalidType >+ $attributes = { >+ $non_existent_attribute_type_code => ['my non-existent attribute'], >+ $normal_attribute_type->code => ['my attribute 1'], >+ }; >+ $fh = build_csv({ %$attributes }); >+ >+ $result = $patrons_import->import_patrons( >+ { >+ file => $fh, >+ matchpoint => 'cardnumber', >+ overwrite_cardnumber => 1, >+ preserve_extended_attributes => 1 >+ } >+ ); >+ is( $result->{overwritten}, 0 ); >+ >+ $error = $result->{errors}->[0]; >+ is( $error->{patron_attribute_invalid_type}, 1 ); >+ is( $error->{patron_id}, $cardnumber ); >+ is( $error->{attribute_type_code}, $non_existent_attribute_type_code ); >+ >+ # NonRepeatable >+ $attributes = { >+ $repeatable_attribute_type->code => ['my repeatable attribute 1', 'my repeatable attribute 2'], >+ $normal_attribute_type->code => ['my normal attribute 1', 'my normal attribute 2'], >+ }; >+ $fh = build_csv({ %$attributes }); >+ $result = $patrons_import->import_patrons( >+ { >+ file => $fh, >+ matchpoint => 'cardnumber', >+ overwrite_cardnumber => 1, >+ preserve_extended_attributes => 1 >+ } >+ ); >+ is( $result->{overwritten}, 0 ); >+ >+ $error = $result->{errors}->[0]; >+ is( $error->{patron_attribute_non_repeatable}, 1 ); >+ is( $error->{patron_id}, $cardnumber ); >+ is( $error->{attribute}->code, $normal_attribute_type->code ); >+ >+ # Don't preserve existing attributes >+ $attributes = { >+ $repeatable_attribute_type->code => ['my repeatable attribute 3', 'my repeatable attribute 4'], >+ $normal_attribute_type->code => ['my normal attribute 1'], >+ }; >+ $fh = build_csv({ %$attributes }); >+ $result = $patrons_import->import_patrons( >+ { >+ file => $fh, >+ matchpoint => 'cardnumber', >+ overwrite_cardnumber => 1, >+ preserve_extended_attributes => 1 >+ } >+ ); >+ is( $result->{overwritten}, 1 ); > > compare_patron_attributes($patron->extended_attributes->unblessed, { %$attributes } ); > >-- >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 28220
:
120549
|
120607
|
120608
|
120609
|
120610
|
120611
|
120612
|
120613
|
120614
|
120783
|
120784
|
120785
|
120786
|
120787
|
120788
|
120789
|
120790
|
120885
|
120886
|
120887
|
120888
|
120889
|
120890
|
120891
|
120892