improve error handling during invoice generation (#2932)

* prevent that items will be marked as exported if invoice can not be generated
* check for existing invoice number to prevent that invalid invoices will be generated
* allow to add a unique id to invoice preview files on batch export
* hide delete invoice action with deactivated permission
* try to automatically fix duplicate invoice id

config/packages/kimai.yaml

@@ -91,6 +91,7 @@ kimai:
             CUSTOMERS_TEAMLEAD: ['view_teamlead_customer','budget_teamlead_customer','comments_teamlead_customer','comments_create_teamlead_customer','details_teamlead_customer']
             INVOICE: ['view_invoice','create_invoice']
             INVOICE_ADMIN: ['manage_invoice_template']
+            INVOICE_ALL: ['delete_invoice']
             TIMESHEET: ['view_own_timesheet','start_own_timesheet','stop_own_timesheet','create_own_timesheet','edit_own_timesheet','export_own_timesheet','delete_own_timesheet','weekly_own_timesheet']
             TIMESHEET_OTHER: ['view_other_timesheet','start_other_timesheet','stop_other_timesheet','create_other_timesheet','edit_other_timesheet','export_other_timesheet','delete_other_timesheet']
             PROFILE: ['view_own_profile','edit_own_profile','password_own_profile','preferences_own_profile','api-token_own_profile']
@@ -120,7 +121,7 @@ kimai:
             ROLE_ADMIN: ['ROLE_ADMIN']
             ROLE_SUPER_ADMIN: ['ROLE_SUPER_ADMIN']
             # only here to register the (partially) unused permissions in the UI
-            ROLE_FAKE: ['CUSTOMERS_ALL_TEAMLEAD','CUSTOMERS_ALL_TEAM','PROJECTS_ALL_TEAMLEAD','PROJECTS_ALL_TEAM','ACTIVITIES_ALL_TEAMLEAD','ACTIVITIES_ALL_TEAM']
+            ROLE_FAKE: ['CUSTOMERS_ALL_TEAMLEAD','CUSTOMERS_ALL_TEAM','PROJECTS_ALL_TEAMLEAD','PROJECTS_ALL_TEAM','ACTIVITIES_ALL_TEAMLEAD','ACTIVITIES_ALL_TEAM','INVOICE_ALL']
         # add or remove single permissions
         roles:
             ROLE_USER: []

src/Command/InvoiceCreateCommand.php

@@ -63,6 +63,7 @@ class InvoiceCreateCommand extends Command
      * @var string|null
      */
     private $previewDirectory;
+    private $previewUniqueFile = false;
 
     public function __construct(
         ServiceInvoice $serviceInvoice,
@@ -104,6 +105,7 @@ class InvoiceCreateCommand extends Command
             ->addOption('search', null, InputOption::VALUE_OPTIONAL, 'Search term to filter invoice entries', null)
             ->addOption('exported', null, InputOption::VALUE_OPTIONAL, 'Exported filter for invoice entries (possible values: exported, all), by default only "not exported" items are fetched', null)
             ->addOption('preview', null, InputOption::VALUE_OPTIONAL, 'Absolute path for a rendered preview of the invoice, which will neither be saved nor the items be marked as exported.', null)
+            ->addOption('preview-unique', null, InputOption::VALUE_NONE, 'Adds a unique part to the filename of the generated invoice preview file, so there is no chance that they get overwritten on same project name.')
         ;
     }
 
@@ -225,6 +227,7 @@ class InvoiceCreateCommand extends Command
 
         $markAsExported = false;
         if ($input->getOption('preview') !== null) {
+            $this->previewUniqueFile = $input->getOption('preview-unique');
             $this->previewDirectory = rtrim($input->getOption('preview'), '/') . '/';
             if (!is_dir($this->previewDirectory) || !is_writable($this->previewDirectory)) {
                 $io->error('Invalid preview directory given');
@@ -350,8 +353,9 @@ class InvoiceCreateCommand extends Command
                     $filename = $filename[1];
                 }
             }
-            // depending on your setup, this might be a good idea
+            if ($this->previewUniqueFile) {
-            // $filename = uniqid() . $filename;
+                $filename = uniqid('invoice_') . $filename;
+            }
         }
 
         if ($response instanceof BinaryFileResponse) {

src/EventSubscriber/Actions/InvoiceSubscriber.php

@@ -36,13 +36,18 @@ class InvoiceSubscriber extends AbstractActionsSubscriber
             $event->addAction('invoice.paid', ['url' => $this->path('admin_invoice_status', ['id' => $invoice->getId(), 'status' => 'paid']), 'class' => 'modal-ajax-form']);
         }
 
+        $allowDelete = $this->isGranted('delete_invoice');
         if (!$invoice->isCanceled()) {
-            $event->addAction('invoice.cancel', ['url' => $this->path('admin_invoice_status', ['id' => $invoice->getId(), 'status' => 'canceled'])]);
+            $id = $allowDelete ? 'invoice.cancel' : 'trash';
+            $event->addAction($id, ['url' => $this->path('admin_invoice_status', ['id' => $invoice->getId(), 'status' => 'canceled']), 'title' => 'invoice.cancel', 'translation_domain' => 'actions']);
         }
 
         $event->addDivider();
 
         $event->addAction('download', ['url' => $this->path('admin_invoice_download', ['id' => $invoice->getId()]), 'target' => '_blank']);
-        $event->addDelete($this->path('admin_invoice_delete', ['id' => $invoice->getId(), 'token' => $payload['token']]), false);
+
+        if ($this->isGranted('delete_invoice')) {
+            $event->addDelete($this->path('admin_invoice_delete', ['id' => $invoice->getId(), 'token' => $payload['token']]), false);
+        }
     }
 }

src/Invoice/DuplicateInvoiceNumberException.php

@@ -0,0 +1,18 @@
+<?php
+
+/*
+ * This file is part of the Kimai time-tracking app.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Invoice;
+
+final class DuplicateInvoiceNumberException extends \Exception
+{
+    public function __construct(string $invoiceNumber)
+    {
+        parent::__construct('Invoice number "' . $invoiceNumber . '" already existing');
+    }
+}

src/Invoice/NumberGenerator/ConfigurableNumberGenerator.php

@@ -58,21 +58,29 @@ final class ConfigurableNumberGenerator implements NumberGeneratorInterface
     {
         $format = $this->configuration->find('invoice.number_format');
         $invoiceDate = $this->model->getInvoiceDate();
-        $result = $format;
 
-        preg_match_all('/{[^}]*?}/', $format, $matches);
+        $loops = 0;
-        foreach ($matches[0] as $part) {
+        $increaseBy = 0;
-            $partialResult = $this->parseReplacer($invoiceDate, $part);
+
-            $result = str_replace($part, $partialResult, $result);
+        do {
-        }
+            $result = $format;
+
+            preg_match_all('/{[^}]*?}/', $format, $matches);
+
+            foreach ($matches[0] as $part) {
+                $partialResult = $this->parseReplacer($invoiceDate, $part, $increaseBy);
+                $result = str_replace($part, $partialResult, $result);
+            }
+
+            $increaseBy++;
+        } while ($this->repository->hasInvoice($result) && $loops++ < 99);
 
         return (string) $result;
     }
 
-    private function parseReplacer(\DateTime $invoiceDate, string $originalFormat): string
+    private function parseReplacer(\DateTime $invoiceDate, string $originalFormat, int $increaseBy): string
     {
         $formatterLength = null;
-        $increaseBy = 0;
         $formatPattern = str_replace(['{', '}'], '', $originalFormat);
 
         $parts = preg_split('/([+\-,])+/', $formatPattern, -1, PREG_SPLIT_DELIM_CAPTURE);

src/Invoice/ServiceInvoice.php

@@ -366,12 +366,12 @@ final class ServiceInvoice
             if ($renderer->supports($document)) {
                 $dispatcher->dispatch(new InvoicePreRenderEvent($model, $document, $renderer));
 
-                $response = $renderer->render($document, $model);
+                if ($this->invoiceRepository->hasInvoice($model->getInvoiceNumber())) {
-
+                    throw new DuplicateInvoiceNumberException($model->getInvoiceNumber());
-                if ($model->getQuery()->isMarkAsExported()) {
-                    $this->markEntriesAsExported($model->getEntries());
                 }
 
+                $response = $renderer->render($document, $model);
+
                 $event = new InvoicePostRenderEvent($model, $document, $renderer, $response);
                 $dispatcher->dispatch($event);
 
@@ -382,6 +382,10 @@ final class ServiceInvoice
                 $invoice->setFilename($invoiceFilename);
                 $this->invoiceRepository->saveInvoice($invoice);
 
+                if ($model->getQuery()->isMarkAsExported()) {
+                    $this->markEntriesAsExported($model->getEntries());
+                }
+
                 $dispatcher->dispatch(new InvoiceCreatedEvent($invoice));
 
                 return $invoice;

tests/Controller/PermissionControllerTest.php

@@ -35,7 +35,7 @@ class PermissionControllerTest extends ControllerBaseTest
         $client = $this->getClientForAuthenticatedUser(User::ROLE_SUPER_ADMIN);
         $this->assertAccessIsGranted($client, '/admin/permissions');
         $this->assertHasDataTable($client);
-        $this->assertDataTableRowCount($client, 'datatable_user_admin_permissions', 120);
+        $this->assertDataTableRowCount($client, 'datatable_user_admin_permissions', 121);
         $this->assertPageActions($client, [
             //'back' => $this->createUrl('/admin/user/'),
             'create modal-ajax-form' => $this->createUrl('/admin/permissions/roles/create'),