Attachment 120887 Details for Bug 28220 – Bug 28220: prevent patron to be created if attributes not stored

[patch] Bug 28220: prevent patron to be created if attributes not stored

Bug-28220-prevent-patron-to-be-created-if-attribut.patch (text/plain), 8.00 KB, created by Martin Renvoize (ashimema) on 2021-05-12 15:57:39 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2021-05-12 15:57:39 UTC

Size: 8.00 KB

patch

obsolete

>From ab7bda347aaee8457abd5c57828bd3a50f51ace1 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>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 <nick@bywatersolutions.com>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> 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.20.1

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