From 5896ae26c674bc5f78f0d2de1f51851af27e94eb Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Mon, 15 Nov 2021 19:21:22 +0100 Subject: [PATCH] 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 | 3 ++- src/Command/InvoiceCreateCommand.php | 8 +++++-- .../Actions/InvoiceSubscriber.php | 9 +++++-- .../DuplicateInvoiceNumberException.php | 18 ++++++++++++++ .../ConfigurableNumberGenerator.php | 24 ++++++++++++------- src/Invoice/ServiceInvoice.php | 12 ++++++---- tests/Controller/PermissionControllerTest.php | 2 +- 7 files changed, 58 insertions(+), 18 deletions(-) create mode 100644 src/Invoice/DuplicateInvoiceNumberException.php diff --git a/config/packages/kimai.yaml b/config/packages/kimai.yaml index a147624a..61c9d847 100644 --- a/config/packages/kimai.yaml +++ b/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: [] diff --git a/src/Command/InvoiceCreateCommand.php b/src/Command/InvoiceCreateCommand.php index a19442cd..f55e8dac 100644 --- a/src/Command/InvoiceCreateCommand.php +++ b/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 - // $filename = uniqid() . $filename; + if ($this->previewUniqueFile) { + $filename = uniqid('invoice_') . $filename; + } } if ($response instanceof BinaryFileResponse) { diff --git a/src/EventSubscriber/Actions/InvoiceSubscriber.php b/src/EventSubscriber/Actions/InvoiceSubscriber.php index 10cb991e..d277ce0d 100644 --- a/src/EventSubscriber/Actions/InvoiceSubscriber.php +++ b/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); + } } } diff --git a/src/Invoice/DuplicateInvoiceNumberException.php b/src/Invoice/DuplicateInvoiceNumberException.php new file mode 100644 index 00000000..c1252060 --- /dev/null +++ b/src/Invoice/DuplicateInvoiceNumberException.php @@ -0,0 +1,18 @@ +configuration->find('invoice.number_format'); $invoiceDate = $this->model->getInvoiceDate(); - $result = $format; - preg_match_all('/{[^}]*?}/', $format, $matches); - foreach ($matches[0] as $part) { - $partialResult = $this->parseReplacer($invoiceDate, $part); - $result = str_replace($part, $partialResult, $result); - } + $loops = 0; + $increaseBy = 0; + + 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); diff --git a/src/Invoice/ServiceInvoice.php b/src/Invoice/ServiceInvoice.php index 8155a8a1..8dbd7e07 100644 --- a/src/Invoice/ServiceInvoice.php +++ b/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 ($model->getQuery()->isMarkAsExported()) { - $this->markEntriesAsExported($model->getEntries()); + if ($this->invoiceRepository->hasInvoice($model->getInvoiceNumber())) { + throw new DuplicateInvoiceNumberException($model->getInvoiceNumber()); } + $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; diff --git a/tests/Controller/PermissionControllerTest.php b/tests/Controller/PermissionControllerTest.php index 59eeb487..3a41ae71 100644 --- a/tests/Controller/PermissionControllerTest.php +++ b/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'),