From fa100d8e5e66aa94cd7b41e314514807a56c3178 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 6 May 2021 10:35:16 +0200 Subject: [PATCH] Bug 28220: prevent patron to be created if attributes not stored Using a transaction Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart --- Koha/Patrons/Import.pm | 84 +++++++++++++++------------- t/db_dependent/Koha/Patrons/Import.t | 19 ++++--- 2 files changed, 55 insertions(+), 48 deletions(-) diff --git a/Koha/Patrons/Import.pm b/Koha/Patrons/Import.pm index c0269106eb..13597c3423 100644 --- a/Koha/Patrons/Import.pm +++ b/Koha/Patrons/Import.pm @@ -331,7 +331,7 @@ sub import_patrons { } if ($extended) { if ($ext_preserve) { - $patron_attributes = $patron->extended_attributes->merge_with( $patron_attributes ); + $patron_attributes = $patron->extended_attributes->merge_and_replace_with( $patron_attributes ); } eval { # We do not want to filter by branch, maybe we should? @@ -354,51 +354,55 @@ sub import_patrons { ); } else { - my $patron = eval { - Koha::Patron->new(\%borrower)->store; - }; - unless ( $@ ) { - $borrowernumber = $patron->id; + try { + $schema->storage->txn_do(sub { + my $patron = Koha::Patron->new(\%borrower)->store; + $borrowernumber = $patron->id; - if ( $patron->is_debarred ) { - AddDebarment( - { - borrowernumber => $patron->borrowernumber, - expiration => $patron->debarred, - comment => $patron->debarredcomment, - } - ); - } + if ( $patron->is_debarred ) { + AddDebarment( + { + borrowernumber => $patron->borrowernumber, + expiration => $patron->debarred, + comment => $patron->debarredcomment, + } + ); + } - if ($extended) { - # FIXME Hum, we did not filter earlier and now we do? - $patron->extended_attributes->filter_by_branch_limitations->delete; - $patron->extended_attributes($patron_attributes); - } + if ($extended) { + # FIXME Hum, we did not filter earlier and now we do? + $patron->extended_attributes->filter_by_branch_limitations->delete; + $patron->extended_attributes($patron_attributes); + } - if ($set_messaging_prefs) { - C4::Members::Messaging::SetMessagingPreferencesFromDefaults( + if ($set_messaging_prefs) { + C4::Members::Messaging::SetMessagingPreferencesFromDefaults( + { + borrowernumber => $patron->borrowernumber, + categorycode => $patron->categorycode, + } + ); + } + + $imported++; + push @imported_borrowers, $patron->borrowernumber; #for patronlist + push( + @feedback, { - borrowernumber => $patron->borrowernumber, - categorycode => $patron->categorycode, + feedback => 1, + name => 'lastimported', + value => $patron->surname . ' / ' . $patron->borrowernumber, } ); - } - - $imported++; - push @imported_borrowers, $patron->borrowernumber; #for patronlist - push( - @feedback, - { - feedback => 1, - name => 'lastimported', - value => $patron->surname . ' / ' . $patron->borrowernumber, - } - ); - } - else { + }); + } catch { $invalid++; - push @errors, { unknown_error => 1 }; + if ( $_->isa('Koha::Exceptions::Patron::Attribute::UniqueIDConstraint') ) { + my $patron_id = defined $matchpoint ? $borrower{$matchpoint} : $matchpoint_attr_type; + push @errors, { patron_attribute_unique_id_constraint => 1, patron_id => $patron_id, attribute => $_->attribute }; + } else { + push @errors, { unknown_error => 1 }; + } push( @errors, { @@ -406,7 +410,7 @@ sub import_patrons { value => $borrower{'surname'} . ' / Create patron', } ); - } + }; } # Add a guarantor if we are given a relationship diff --git a/t/db_dependent/Koha/Patrons/Import.t b/t/db_dependent/Koha/Patrons/Import.t index 884268baae..b343b60f86 100755 --- a/t/db_dependent/Koha/Patrons/Import.t +++ b/t/db_dependent/Koha/Patrons/Import.t @@ -845,7 +845,7 @@ subtest 'test_format_dates' => sub { subtest 'patron_attributes' => sub { - plan tests => 4; + plan tests => 6; t::lib::Mocks::mock_preference('ExtendedPatronAttributes', 1); @@ -875,6 +875,8 @@ subtest 'patron_attributes' => sub { my $non_existent_attribute_type_code = $non_existent_attribute_type->code; $non_existent_attribute_type->delete; + our $cardnumber = "1042"; + # attributes is { code => \@attributes } sub build_csv { my ($attributes) = @_; @@ -882,7 +884,7 @@ subtest 'patron_attributes' => sub { my $csv_headers = 'cardnumber,surname,firstname,branchcode,categorycode,patron_attributes'; my @attributes_str = map { my $code = $_; map { sprintf "%s:%s", $code, $_ } @{ $attributes->{$code} } } keys %$attributes; my $attributes_str = join ',', @attributes_str; - my $csv_line = sprintf '1042,John,D,MPL,PT,"%s"', $attributes_str; + my $csv_line = sprintf '%s,John,D,MPL,PT,"%s"', $cardnumber, $attributes_str; my $filename = make_csv( $temp_dir, $csv_headers, $csv_line ); open( my $fh, "<:encoding(utf8)", $filename ) or die "cannot open $filename: $!"; return $fh; @@ -899,7 +901,7 @@ subtest 'patron_attributes' => sub { is( $result->{imported}, 1 ); - my $patron = Koha::Patrons->find({cardnumber => "1042"}); + my $patron = Koha::Patrons->find({cardnumber => $cardnumber}); compare_patron_attributes($patron->extended_attributes->unblessed, { %$attributes } ); $patron->delete; } @@ -918,12 +920,13 @@ subtest 'patron_attributes' => sub { }; my $fh = build_csv({ %$attributes }); - throws_ok { - $patrons_import->import_patrons({file => $fh}); - } - 'Koha::Exceptions::Patron::Attribute::UniqueIDConstraint'; + my $result = $patrons_import->import_patrons({file => $fh, matchpoint => 'cardnumber'}); + 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 ); - my $patron = Koha::Patrons->find({cardnumber => "1042"}); + my $patron = Koha::Patrons->find({cardnumber => $cardnumber}); is( $patron, undef ); } -- 2.39.5