From 4de341c2d0fe88c8ac160ba916a86a382be8a6b1 Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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 Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart --- 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 7b7c42b7f6..0d922b6fa6 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 38b1c7f44d..14a47ae879 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 bef31e56f8..b3ba72ee9d 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.39.5