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

[koha-ffzg.git] / Koha / Patrons / Import.pm

diff --git a/Koha/Patrons/Import.pm b/Koha/Patrons/Import.pm
index 6f5646a..13597c3 100644 (file)
--- a/Koha/Patrons/Import.pm
+++ b/Koha/Patrons/Import.pm
@@ -227,9 +227,9 @@ sub import_patrons {
             next LINE;
         }
 
-        my $relationship        = $borrower{relationship};
-        my $guarantor_id        = $borrower{guarantor_id};
-        delete $borrower{relationship};
+        my $guarantor_relationship = $borrower{guarantor_relationship};
+        delete $borrower{guarantor_relationship};
+        my $guarantor_id = $borrower{guarantor_id};
         delete $borrower{guarantor_id};
 
         # Remove warning for int datatype that cannot be null
@@ -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,50 +354,55 @@ sub import_patrons {
             );
         }
         else {
-            my $patron = eval {
-                Koha::Patron->new(\%borrower)->store;
-            };
-            unless ( $@ ) {
+            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(
+                            {
+                                borrowernumber => $patron->borrowernumber,
+                                categorycode   => $patron->categorycode,
+                            }
+                        );
+                    }
 
-                if ($set_messaging_prefs) {
-                    C4::Members::Messaging::SetMessagingPreferencesFromDefaults(
+                    $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,
                     {
@@ -405,21 +410,36 @@ sub import_patrons {
                         value => $borrower{'surname'} . ' / Create patron',
                     }
                 );
-            }
+            };
         }
 
         # Add a guarantor if we are given a relationship
         if ( $guarantor_id ) {
-            Koha::Patron::Relationship->new(
+            my $relationship = Koha::Patron::Relationships->find(
                 {
                     guarantee_id => $borrowernumber,
-                    relationship => $relationship,
                     guarantor_id => $guarantor_id,
                 }
-            )->store();
+            );
+
+            if ( $relationship ) {
+                $relationship->relationship( $guarantor_relationship );
+                $relationship->store();
+            }
+            else {
+                Koha::Patron::Relationship->new(
+                    {
+                        guarantee_id => $borrowernumber,
+                        relationship => $guarantor_relationship,
+                        guarantor_id => $guarantor_id,
+                    }
+                )->store();
+            }
         }
     }
 
+    $schema->storage->txn_rollback if $dry_run;
+
     return {
         feedback      => \@feedback,
         errors        => \@errors,
@@ -429,8 +449,6 @@ sub import_patrons {
         invalid       => $invalid,
         imported_borrowers => \@imported_borrowers,
     };
-
-    $schema->storage->txn_rollback if $dry_run;
 }
 
 =head2 prepare_columns
@@ -494,6 +512,7 @@ sub set_column_keys {
 
     my @columnkeys = map { $_ ne 'borrowernumber' ? $_ : () } Koha::Patrons->columns();
     push( @columnkeys, 'patron_attributes' ) if $extended;
+    push( @columnkeys, qw( guarantor_relationship guarantor_id ) );
 
     return @columnkeys;
 }
@@ -523,7 +542,7 @@ sub generate_patron_attributes {
     my $ok   = $csv->parse($string);  # parse field again to get subfields!
     my @list = $csv->fields();
     my @patron_attributes =
-      sort { $a->{code} cmp $b->{code} || $a->{value} cmp $b->{value} }
+      sort { $a->{code} cmp $b->{code} || $a->{attribute} cmp $b->{attribute} }
       map {
         my @arr = split /:/, $_, 2;
         { code => $arr[0], attribute => $arr[1] }