From 66d468b093e7c4510722f22b4a0c069cb95e6117 Mon Sep 17 00:00:00 2001
From: Moritz Bunkus <m.bunkus@linet-services.de>
Date: Mon, 15 Aug 2016 17:17:36 +0200
Subject: [PATCH] Bankauszug verbuchen: Warnungen/Fehler anzeigen; pro Zeile
 eine DB-Transaktion
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

Das Verbuchen von Bankauszügen wird nun in Datenbanktransaktionen
gekapselt. Damit die BenutzerIn bei einem Fehler nicht alles erneut
einstellen muss, wird eine Datenbanktransaktion pro
Banktransaktion benutzt.

Treten dabei Warnungen oder Fehler auf, so werden diese nun in einer
Tabelle übersichtlich dargestellt. Die Tabelle enthält sowohl die Daten
der Banktransaktion, bei der das Problem auftrat (entfernte
Kontonummer/BIC, entferne KontobesitzerIn, Betrag, Transaktionsdatum),
als auch Links zu den mit dieser Transaktion verknüpften
Rechnungen. Damit wird die BearbeiterIn in die Lage versetzt, die Fehler
genauer zu analysieren.
---
 SL/Controller/BankTransaction.pm              | 112 +++++++++++++++---
 locale/de/all                                 |   4 +
 .../webpages/bank_transactions/_problems.html |  46 +++++++
 .../webpages/bank_transactions/list.html      |   4 +
 4 files changed, 147 insertions(+), 19 deletions(-)
 create mode 100644 templates/webpages/bank_transactions/_problems.html

diff --git a/SL/Controller/BankTransaction.pm b/SL/Controller/BankTransaction.pm
index aba5fdb50..c16173537 100644
--- a/SL/Controller/BankTransaction.pm
+++ b/SL/Controller/BankTransaction.pm
@@ -29,7 +29,7 @@ use List::Util qw(max);
 
 use Rose::Object::MakeMethods::Generic
 (
- 'scalar --get_set_init' => [ qw(models) ],
+  'scalar --get_set_init' => [ qw(models problems) ],
 );
 
 __PACKAGE__->run_before('check_auth');
@@ -394,20 +394,64 @@ sub action_save_invoices {
   #           '44' => [ '50', '51', 52' ]
   #         };
 
-  my $skonto_hash  = delete $::form->{invoice_skontos} || {}; # array containing the payment type, could be empty
+  $::form->{invoice_skontos} ||= {}; # hash of arrays containing the payment types, could be empty
 
   # a bank_transaction may be assigned to several invoices, i.e. a customer
   # might pay several open invoices with one transaction
 
-  while ( my ($bt_id, $invoice_ids) = each(%$invoice_hash) ) {
-    my $bank_transaction      = SL::DB::Manager::BankTransaction->find_by(id => $bt_id);
+  $self->problems([]);
+
+  while ( my ($bank_transaction_id, $invoice_ids) = each(%$invoice_hash) ) {
+    push @{ $self->problems }, $self->save_single_bank_transaction(
+      bank_transaction_id => $bank_transaction_id,
+      invoice_ids         => $invoice_ids,
+    );
+  }
+
+  $self->action_list();
+}
+
+sub save_single_bank_transaction {
+  my ($self, %params) = @_;
+
+  my %data = (
+    %params,
+    bank_transaction => SL::DB::Manager::BankTransaction->find_by(id => $params{bank_transaction_id}),
+    invoices         => [],
+  );
+
+  if (!$data{bank_transaction}) {
+    return {
+      %data,
+      result => 'error',
+      message => $::locale->text('The ID #1 is not a valid database ID.', $data{bank_transaction_id}),
+    };
+  }
+
+  my (@warnings);
+
+  my $worker = sub {
+    my $bt_id                 = $data{bank_transaction_id};
+    my $bank_transaction      = $data{bank_transaction};
     my $sign                  = $bank_transaction->amount < 0 ? -1 : 1;
     my $amount_of_transaction = $sign * $bank_transaction->amount;
 
     my @invoices;
-    foreach my $invoice_id (@{ $invoice_ids }) {
-      push @invoices, (SL::DB::Manager::Invoice->find_by(id => $invoice_id) || SL::DB::Manager::PurchaseInvoice->find_by(id => $invoice_id));
+    foreach my $invoice_id (@{ $params{invoice_ids} }) {
+      my $invoice = SL::DB::Manager::Invoice->find_by(id => $invoice_id) || SL::DB::Manager::PurchaseInvoice->find_by(id => $invoice_id);
+      if (!$invoice) {
+        return {
+          %data,
+          result  => 'error',
+          message => $::locale->text("The ID #1 is not a valid database ID.", $invoice_id),
+        };
+      }
+
+      push @invoices, $invoice;
     }
+
+    $data{invoices} = \@invoices;
+
     @invoices = sort { return 1 if ( $a->is_sales and $a->amount > 0);
                        return 1 if (!$a->is_sales and $a->amount < 0);
                        return -1;
@@ -425,22 +469,31 @@ sub action_save_invoices {
       $n_invoices++ ;
       # Check if bank_transaction already has a link to the invoice, may only be linked once per invoice
       # This might be caused by the user reloading a page and resending the form
-      die t8("Bank transaction with id #1 has already been linked to #2.", $bank_transaction->id, $invoice->displayable_name)
-        if _existing_record_link($bank_transaction, $invoice);
+      if (_existing_record_link($bank_transaction, $invoice)) {
+        return {
+          %data,
+          result  => 'error',
+          message => $::locale->text("Bank transaction with id #1 has already been linked to #2.", $bank_transaction->id, $invoice->displayable_name),
+        };
+      }
+
+      if ($amount_of_transaction == 0) {
+        push @warnings, {
+          %data,
+          result  => 'warning',
+          message => $::locale->text('There are invoices which could not be paid by bank transaction #1 (Account number: #2, bank code: #3)!',
+                                     $bank_transaction->purpose, $bank_transaction->remote_account_number, $bank_transaction->remote_bank_code),
+        };
+        last;
+      }
 
       my $payment_type;
-      if ( defined $skonto_hash->{"$bt_id"} ) {
-        $payment_type = shift(@{ $skonto_hash->{"$bt_id"} });
+      if ( defined $::form->{invoice_skontos}->{"$bt_id"} ) {
+        $payment_type = shift(@{ $::form->{invoice_skontos}->{"$bt_id"} });
       } else {
         $payment_type = 'without_skonto';
       };
-      if ($amount_of_transaction == 0) {
-        flash('warning',  $::locale->text('There are invoices which could not be paid by bank transaction #1 (Account number: #2, bank code: #3)!',
-                                          $bank_transaction->purpose,
-                                          $bank_transaction->remote_account_number,
-                                          $bank_transaction->remote_bank_code));
-        last;
-      }
+
       # pay invoice or go to the next bank transaction if the amount is not sufficiently high
       if ($invoice->open_amount <= $amount_of_transaction && $n_invoices < $max_invoices) {
         # first calculate new bank transaction amount ...
@@ -490,9 +543,29 @@ sub action_save_invoices {
 
     }
     $bank_transaction->save;
-  }
 
-  $self->action_list();
+    # 'undef' means 'no error' here.
+    return undef;
+  };
+
+  my $error;
+  my $rez = $data{bank_transaction}->db->with_transaction(sub {
+    eval {
+      $error = $worker->();
+      1;
+
+    } or do {
+      $error = {
+        %data,
+        result  => 'error',
+        message => $@,
+      };
+    };
+
+    die if $error;
+  });
+
+  return grep { $_ } ($error, @warnings);
 }
 
 sub action_save_proposals {
@@ -651,6 +724,7 @@ sub _existing_record_link {
   return @$linked_records ? 1 : 0;
 };
 
+sub init_problems { [] }
 
 sub init_models {
   my ($self) = @_;
diff --git a/locale/de/all b/locale/de/all
index ccf33400e..5fb3ed5e5 100755
--- a/locale/de/all
+++ b/locale/de/all
@@ -369,6 +369,8 @@ $self->{texts} = {
   'Bank transaction with id #1 has already been linked to #2.' => 'Bankbuchung mit id #1 wurde schon mit #2 verlinkt.',
   'Bank transactions'           => 'Bankbewegungen',
   'Bank transactions MT940'     => 'Kontoauszug verbuchen',
+  'Bank transactions that either only have warnings or no message at all have been posted.' => 'Banktransaktionen, die entweder nur Warnungen oder gar keine Nachricht haben, wurden hingegen verbucht.',
+  'Bank transactions with errors have not been posted.' => 'Banktransaktionen mit Fehlern wurden nicht verbucht.',
   'Bank transfer amount'        => 'Überweisungssumme',
   'Bank transfer payment list for export #1' => 'Überweisungszahlungsliste für SEPA-Export #1',
   'Bank transfer via SEPA'      => 'Überweisung via SEPA',
@@ -2730,6 +2732,7 @@ $self->{texts} = {
   'The GL transaction #1 has been deleted.' => 'Die Dialogbuchung #1 wurde gelöscht.',
   'The IBAN \'#1\' is not valid as IBANs in #2 must be exactly #3 characters long.' => 'Die IBAN \'#1\' ist ungültig, da IBANs in #2 genau #3 Zeichen lang sein müssen.',
   'The IBAN is missing.'        => 'Die IBAN fehlt.',
+  'The ID #1 is not a valid database ID.' => 'Die ID #1 ist keine gültige Datenbank-ID.',
   'The LDAP server "#1:#2" is unreachable. Please check config/kivitendo.conf.' => 'Der LDAP-Server "#1:#2" ist nicht erreichbar. Bitte &uuml;berpr&uuml;fen Sie die Angaben in config/kivitendo.conf.',
   'The MT940 import needs an import profile called MT940' => 'Der MT940 Import benötigt ein Importprofil mit dem Namen "MT940"',
   'The PDF has been created'    => 'Die PDF-Datei wurde erstellt.',
@@ -3327,6 +3330,7 @@ $self->{texts} = {
   'Warn before saving orders with duplicate parts (new controller only)' => 'Beim Speichern warnen, wenn doppelte Artikel in einem Auftrag sind',
   'Warning'                     => 'Warnung',
   'Warning! Loading a draft will discard unsaved data!' => 'Achtung! Beim Laden eines Entwurfs werden ungespeicherte Daten verworfen!',
+  'Warnings and errors'         => 'Warnungen und Fehler',
   'WebDAV'                      => 'WebDAV',
   'WebDAV link'                 => 'WebDAV-Link',
   'WebDAV save documents'       => 'Belege in WebDAV-Ablage speichern',
diff --git a/templates/webpages/bank_transactions/_problems.html b/templates/webpages/bank_transactions/_problems.html
new file mode 100644
index 000000000..9a10fca92
--- /dev/null
+++ b/templates/webpages/bank_transactions/_problems.html
@@ -0,0 +1,46 @@
+[%- USE LxERP -%][%- USE T8 -%][%- USE HTML -%][%- USE P -%]
+<h3>
+ [% LxERP.t8("Warnings and errors") %]
+</h3>
+
+<p>
+ [% LxERP.t8("Bank transactions with errors have not been posted.") %]
+ [% LxERP.t8("Bank transactions that either only have warnings or no message at all have been posted.") %]
+</p>
+
+<table>
+ <thead>
+  <tr class="listheading">
+   <th>[% LxERP.t8("Type") %]</th>
+   <th>[% LxERP.t8("Invoices") %]</th>
+   <th>[% LxERP.t8("Transdate") %]</th>
+   <th>[% LxERP.t8("Amount") %]</th>
+   <th>[% LxERP.t8("Remote name") %]</th>
+   <th>[% LxERP.t8("Purpose") %]</th>
+   <th>[% LxERP.t8("Remote account number") %]</th>
+   <th>[% LxERP.t8("Remote bank code") %]</th>
+   <th>[% LxERP.t8("Message") %]</th>
+  </tr>
+ </thead>
+
+ <tbody>
+  [% FOREACH problem = SELF.problems %]
+   <tr class="listrow[% IF problem.result == 'error' %]_error[% END %]">
+    <td>[% IF problem.result == 'error' %][% LxERP.t8("Error") %][% ELSE %][% LxERP.t8("Warning") %][% END %]</td>
+    <td>
+     [% FOREACH invoice = problem.invoices %]
+      [% P.invoice(invoice) %]
+      [% UNLESS loop.last %]<br>[% END %]
+     [% END %]
+    </td>
+    <td>[% HTML.escape(problem.bank_transaction.transdate.to_kivitendo) %]</td>
+    <td>[% HTML.escape(LxERP.format_amount(problem.bank_transaction.amount, 2)) %]</td>
+    <td>[% HTML.escape(problem.bank_transaction.remote_name) %]</td>
+    <td>[% HTML.escape(problem.bank_transaction.purpose) %]</td>
+    <td>[% HTML.escape(problem.bank_transaction.remote_account_number) %]</td>
+    <td>[% HTML.escape(problem.bank_transaction.remote_bank_code) %]</td>
+    <td>[% HTML.escape(problem.message) %]</td>
+   </tr>
+  [% END %]
+ </tbody>
+</table>
diff --git a/templates/webpages/bank_transactions/list.html b/templates/webpages/bank_transactions/list.html
index af6ef2275..c810c8f9c 100644
--- a/templates/webpages/bank_transactions/list.html
+++ b/templates/webpages/bank_transactions/list.html
@@ -4,6 +4,10 @@
 
 [%- INCLUDE 'common/flash.html' %]
 
+[% IF SELF.problems.size %]
+ [% INCLUDE 'bank_transactions/_problems.html' %]
+[% END %]
+
 <p>[% HTML.escape(bank_account.name) %] [% HTML.escape(bank_account.iban) %], [% 'Bank code' | $T8 %] [% HTML.escape(bank_account.bank_code) %], [% 'Bank' | $T8 %] [% HTML.escape(bank_account.bank) %]</p>
 <p>
 [% IF FORM.filter.fromdate %] [% 'From' | $T8 %] [% FORM.filter.fromdate %] [% END %]
-- 
2.20.1