Invoices: show only custom documents in upload form (#1786)

src/Controller/InvoiceController.php

@@ -284,29 +284,34 @@ final class InvoiceController extends AbstractController
      */
     public function uploadDocumentAction(Request $request, string $projectDirectory, InvoiceDocumentRepository $documentRepository)
     {
-        $dir = $documentRepository->getCustomInvoiceDirectory();
+        $dir = $documentRepository->getUploadDirectory();
+        $invoiceDir = $dir;
+
+        // do not execute realpath, as it will return an empty string if the invoice directory is NOT existing!
+        if ($invoiceDir[0] !== '/') {
             $invoiceDir = $projectDirectory . DIRECTORY_SEPARATOR . $dir;
+        }
+
         $canUpload = true;
-        $form = null;
 
         if (!file_exists($invoiceDir)) {
-            @mkdir($invoiceDir);
+            @mkdir($invoiceDir, 0777);
         }
-        if (!file_exists($invoiceDir)) {
+
-            $this->flashError(sprintf('Invoice directory is not existing and could not be created: %s', $dir));
+        if (!is_dir($invoiceDir)) {
+            $this->flashError(sprintf('Invoice directory "%s" is not existing and could not be created.', $dir));
             $canUpload = false;
-        }
+        } elseif (!is_writable($invoiceDir)) {
-        if (!is_writable($invoiceDir)) {
+            $this->flashError(sprintf('Invoice directory "%s" cannot be written.', $dir));
-            $this->flashError(sprintf('Invoice directory cannot be written: %s', $dir));
             $canUpload = false;
         }
 
-        if ($canUpload) {
         $form = $this->createForm(InvoiceDocumentUploadForm::class, null, [
             'action' => $this->generateUrl('admin_invoice_document_upload', []),
             'method' => 'POST'
         ]);
 
+        if ($canUpload) {
             $form->handleRequest($request);
 
             if ($form->isSubmitted() && $form->isValid()) {
@@ -334,8 +339,8 @@ final class InvoiceController extends AbstractController
         }
 
         return $this->render('invoice/document_upload.html.twig', [
-            'form' => (null !== $form) ? $form->createView() : null,
+            'form' => $form->createView(),
-            'documents' => $this->service->getDocuments(),
+            'documents' => $this->service->getDocuments(true),
             'baseDirectory' => $projectDirectory . DIRECTORY_SEPARATOR,
         ]);
     }

src/DataFixtures/CustomerFixtures.php

@@ -13,7 +13,7 @@ use App\Entity\Activity;
 use App\Entity\Customer;
 use App\Entity\Project;
 use Doctrine\Bundle\FixturesBundle\Fixture;
-use Doctrine\Common\Persistence\ObjectManager;
+use Doctrine\Persistence\ObjectManager;
 use Faker\Factory;
 use Faker\Generator;
 

src/DataFixtures/InvoiceFixtures.php

@@ -11,7 +11,7 @@ namespace App\DataFixtures;
 
 use App\Entity\InvoiceTemplate;
 use Doctrine\Bundle\FixturesBundle\Fixture;
-use Doctrine\Common\Persistence\ObjectManager;
+use Doctrine\Persistence\ObjectManager;
 use Faker\Factory;
 use Faker\Generator;
 
@@ -86,6 +86,7 @@ class InvoiceFixtures extends Fixture
 
         // name, title, renderer, calculator, numberGenerator, company, vat, dueDays, address, paymentTerms
         return [
+            ['Invoice (PDF)',             'Invoice',         'default-pdf',      'default',  'default', $faker->company, 16, 10, $paymentTerms],
             ['Invoice (HTML)',            'Company name',    'default',          'default',  'default', $faker->company, 19, 30, $paymentTerms],
             ['Freelancer (HTML, short)',  'Invoice',         'freelancer',       'short',    'default', $faker->company, 19, 14, $paymentTerms_de],
             ['Timesheet (HTML)',          'Timesheet',       'timesheet',        'default',  'default', $faker->company, 19, 7,  $paymentTerms_alt],

src/DataFixtures/TagFixtures.php

@@ -11,7 +11,7 @@ namespace App\DataFixtures;
 
 use App\Entity\Tag;
 use Doctrine\Bundle\FixturesBundle\Fixture;
-use Doctrine\Common\Persistence\ObjectManager;
+use Doctrine\Persistence\ObjectManager;
 use Faker\Factory;
 
 /**

src/DataFixtures/TeamFixtures.php

@@ -14,7 +14,7 @@ use App\Entity\Team;
 use App\Entity\User;
 use Doctrine\Bundle\FixturesBundle\Fixture;
 use Doctrine\Common\DataFixtures\DependentFixtureInterface;
-use Doctrine\Common\Persistence\ObjectManager;
+use Doctrine\Persistence\ObjectManager;
 use Faker\Factory;
 
 /**

src/DataFixtures/TimesheetFixtures.php

@@ -18,7 +18,7 @@ use App\Entity\UserPreference;
 use App\Timesheet\Util;
 use Doctrine\Bundle\FixturesBundle\Fixture;
 use Doctrine\Common\DataFixtures\DependentFixtureInterface;
-use Doctrine\Common\Persistence\ObjectManager;
+use Doctrine\Persistence\ObjectManager;
 use Faker\Factory;
 
 /**

src/DataFixtures/UserFixtures.php

@@ -12,7 +12,7 @@ namespace App\DataFixtures;
 use App\Entity\User;
 use App\Entity\UserPreference;
 use Doctrine\Bundle\FixturesBundle\Fixture;
-use Doctrine\Common\Persistence\ObjectManager;
+use Doctrine\Persistence\ObjectManager;
 use Faker\Factory;
 use Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface;
 

src/DependencyInjection/Configuration.php

@@ -11,6 +11,7 @@ namespace App\DependencyInjection;
 
 use App\Entity\Customer;
 use App\Entity\User;
+use App\Repository\InvoiceDocumentRepository;
 use App\Timesheet\Rounding\RoundingInterface;
 use App\Widget\Type\CompoundRow;
 use App\Widget\Type\Counter;
@@ -215,7 +216,7 @@ class Configuration implements ConfigurationInterface
                     ->scalarPrototype()->end()
                     ->defaultValue([
                         'var/invoices/',
-                        'templates/invoice/renderer/'
+                        InvoiceDocumentRepository::DEFAULT_DIRECTORY
                     ])
                 ->end()
                 ->arrayNode('documents')

src/Form/InvoiceDocumentUploadForm.php

@@ -9,14 +9,28 @@
 
 namespace App\Form;
 
+use App\Repository\InvoiceDocumentRepository;
 use Symfony\Component\Form\AbstractType;
 use Symfony\Component\Form\Extension\Core\Type\FileType;
 use Symfony\Component\Form\FormBuilderInterface;
+use Symfony\Component\HttpFoundation\File\UploadedFile;
 use Symfony\Component\OptionsResolver\OptionsResolver;
+use Symfony\Component\Validator\Constraints\Callback;
 use Symfony\Component\Validator\Constraints\File;
+use Symfony\Component\Validator\Context\ExecutionContextInterface;
 
 class InvoiceDocumentUploadForm extends AbstractType
 {
+    /**
+     * @var InvoiceDocumentRepository
+     */
+    private $repository;
+
+    public function __construct(InvoiceDocumentRepository $repository)
+    {
+        $this->repository = $repository;
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -37,12 +51,33 @@ class InvoiceDocumentUploadForm extends AbstractType
                             'application/vnd.oasis.opendocument.spreadsheet',
                         ],
                         'mimeTypesMessage' => 'This file type is not allowed',
-                    ])
+                    ]),
+                    new Callback([$this, 'validateDocument'])
                 ],
             ])
         ;
     }
 
+    public function validateDocument($value, ExecutionContextInterface $context)
+    {
+        if (!($value instanceof UploadedFile)) {
+            return;
+        }
+
+        $name = $value->getClientOriginalName();
+
+        foreach ($this->repository->findBuiltIn() as $document) {
+            if ($document->getName() !== $name) {
+                continue;
+            }
+
+            $context->buildViolation('This invoice document cannot be used, please rename the file and upload it again.')
+                ->setTranslationDomain('validators')
+                ->setCode('kimai-invoice-document-upload-01')
+                ->addViolation();
+        }
+    }
+
     /**
      * {@inheritdoc}
      */

src/Invoice/ServiceInvoice.php

@@ -134,10 +134,15 @@ final class ServiceInvoice
     /**
      * Returns an array of invoice renderer, which will consist of a unique name and a controller action.
      *
+     * @param bool $customOnly
      * @return InvoiceDocument[]
      */
-    public function getDocuments(): array
+    public function getDocuments(bool $customOnly = false): array
     {
+        if ($customOnly) {
+            return $this->documents->findCustom();
+        }
+
         return $this->documents->findAll();
     }
 

src/Repository/InvoiceDocumentRepository.php

@@ -14,6 +14,8 @@ use Symfony\Component\Finder\Finder;
 
 final class InvoiceDocumentRepository
 {
+    public const DEFAULT_DIRECTORY = 'templates/invoice/renderer/';
+
     /**
      * @var array
      */
@@ -21,12 +23,48 @@ final class InvoiceDocumentRepository
 
     public function __construct(array $directories)
     {
-        $this->documentDirs = $directories;
+        foreach ($directories as $directory) {
+            $this->addDirectory($directory);
+        }
     }
 
+    public function addDirectory(string $directory)
+    {
+        $this->documentDirs[] = $directory;
+
+        return $this;
+    }
+
+    public function removeDirectory(string $directory)
+    {
+        if (($key = array_search($directory, $this->documentDirs)) !== false) {
+            unset($this->documentDirs[$key]);
+        }
+
+        return $this;
+    }
+
+    /**
+     * @deprecated since 1.10 - will be removed with 2.0 - use getCustomInvoiceDirectory() instead
+     */
     public function getCustomInvoiceDirectory(): string
     {
-        return $this->documentDirs[0];
+        return $this->getUploadDirectory();
+    }
+
+    public function getUploadDirectory(): string
+    {
+        // reverse the array, as bundles can register invoice directories a well (as prepend extensions)
+        // and then the first entries are the directories from the bundles and not the default ones registered in Kimai
+        foreach (array_reverse($this->documentDirs) as $dir) {
+            if ($dir === self::DEFAULT_DIRECTORY) {
+                continue;
+            }
+
+            return $dir;
+        }
+
+        throw new \Exception('Unknown upload directory');
     }
 
     public function findByName(string $name): ?InvoiceDocument
@@ -41,21 +79,71 @@ final class InvoiceDocumentRepository
     }
 
     /**
-     * Returns an array of invoice renderer, which will consist of a unique name and a controller action.
+     * Returns an array of all custom invoice documents.
+     *
+     * @return InvoiceDocument[]
+     */
+    public function findCustom()
+    {
+        $paths = [];
+        foreach ($this->documentDirs as $dir) {
+            if ($dir === self::DEFAULT_DIRECTORY) {
+                continue;
+            }
+            $paths[] = $dir;
+        }
+
+        return $this->findByPaths($paths);
+    }
+
+    /**
+     * Returns an array of all original Kimai documents.
+     *
+     * @return InvoiceDocument[]
+     */
+    public function findBuiltIn()
+    {
+        foreach ($this->documentDirs as $dir) {
+            if ($dir === self::DEFAULT_DIRECTORY) {
+                return $this->findByPaths([$dir]);
+            }
+        }
+
+        return [];
+    }
+
+    /**
+     * Returns an array of invoice documents.
      *
      * @return InvoiceDocument[]
      */
     public function findAll()
+    {
+        return $this->findByPaths($this->documentDirs);
+    }
+
+    /**
+     * Returns an array of invoice documents.
+     *
+     * @return InvoiceDocument[]
+     */
+    private function findByPaths(array $paths)
     {
         $base = \dirname(\dirname(__DIR__)) . DIRECTORY_SEPARATOR;
 
         $documents = [];
 
-        foreach ($this->documentDirs as $searchPath) {
+        foreach ($paths as $searchPath) {
-            if (!is_dir($base . $searchPath)) {
+            $searchDir = $searchPath;
+            if ($searchDir[0] !== '/') {
+                $searchDir = $base . $searchPath;
+            }
+
+            if (!is_dir($searchDir)) {
                 continue;
             }
-            $finder = Finder::create()->ignoreDotFiles(true)->files()->in($base . $searchPath)->name('*.*');
+
+            $finder = Finder::create()->ignoreDotFiles(true)->files()->in($searchDir)->name('*.*');
             foreach ($finder->getIterator() as $file) {
                 $doc = new InvoiceDocument($file);
                 // the first found invoice document wins

templates/invoice/document_upload.html.twig

@@ -36,9 +36,9 @@
                     <tbody>
                     {% for document in documents %}
                         <tr>
-                            <td>{{ document.id }}</td>
+                            <td style="width: 33%">{{ document.id }}</td>
-                            <td>{{ document.lastChange|date }}</td>
+                            <td style="width: 33%">{{ document.lastChange|date }}</td>
-                            <td>{{ document.filename|replace({(baseDirectory): ''}) }}</td>
+                            <td>{{ document.name }}</td>
                         </tr>
                     {% endfor %}
                     </tbody>

tests/Controller/InvoiceControllerTest.php

@@ -412,8 +412,8 @@ class InvoiceControllerTest extends ControllerBaseTest
         $this->request($client, '/invoice/document_upload');
         $this->assertTrue($client->getResponse()->isSuccessful());
 
-        $node = $client->getCrawler()->filter('div.box#invoice_document_list');
+        $node = $client->getCrawler()->filter('form[name=invoice_document_upload_form]');
-        self::assertEquals(1, $node->count());
+        self::assertEquals(1, $node->count(), 'Could not find upload form');
         // we do not test the upload here, just make sure that the action can be rendered properly
     }
 }

tests/Repository/InvoiceDocumentRepositoryTest.php

@@ -22,6 +22,12 @@ class InvoiceDocumentRepositoryTest extends TestCase
         'templates/invoice/renderer'
     ];
 
+    protected static $testDocuments = [
+        'spreadsheet.xsls',
+        'open-spreadsheet.ods',
+        'default.pdf.twig',
+    ];
+
     protected static $defaultDocuments = [
         'company.docx',
         'default.html.twig',
@@ -33,19 +39,50 @@ class InvoiceDocumentRepositoryTest extends TestCase
         'xml.xml.twig',
     ];
 
-    public function testWithEmptyDirectory()
+    public function testDirectories()
     {
         $sut = new InvoiceDocumentRepository([]);
-        $this->assertEmpty($sut->findAll());
+        self::assertEmpty($sut->findAll());
-        $this->assertIsArray($sut->findAll());
+        self::assertIsArray($sut->findAll());
-        $this->assertNull($sut->findByName('default'));
+        self::assertNull($sut->findByName('default'));
+
+        try {
+            $sut->getUploadDirectory();
+            $this->fail('Expected exception was not raised');
+        } catch (\Exception $ex) {
+            $this->assertEquals('Unknown upload directory', $ex->getMessage());
+        }
+
+        $path = realpath(__DIR__ . '/../Invoice/templates/');
+        $sut->addDirectory($path);
+        $sut->addDirectory(InvoiceDocumentRepository::DEFAULT_DIRECTORY);
+        $sut->addDirectory(__DIR__);
+        self::assertEquals(__DIR__, $sut->getUploadDirectory());
+
+        $sut->removeDirectory(__DIR__);
+
+        self::assertCount(\count(self::$defaultDocuments), $sut->findBuiltIn());
+        self::assertCount(\count(self::$testDocuments), $sut->findCustom());
+
+        // template "default" exists twice, so its 10 instead of 11
+        $all = [];
+        foreach (self::$defaultDocuments as $document) {
+            $all[] = substr($document, 0, strpos($document, '.'));
+        }
+        foreach (self::$testDocuments as $document) {
+            $all[] = substr($document, 0, strpos($document, '.'));
+        }
+        $all = array_unique(array_values($all));
+        self::assertCount(\count($all), $sut->findAll());
+
+        self::assertEquals($path, $sut->getUploadDirectory());
     }
 
     public function testDefaultTemplatesExists()
     {
         $sut = new InvoiceDocumentRepository(self::$defaultDirectories);
         $all = $sut->findAll();
-        $this->assertEquals(\count(self::$defaultDocuments), \count($all));
+        $this->assertCount(\count(self::$defaultDocuments), $all);
 
         foreach ($all as $document) {
             $this->assertTrue(\in_array($document->getName(), self::$defaultDocuments));

translations/validators.de.xlf

@@ -18,6 +18,10 @@
                 <source>You must select at least one user or team.</source>
                 <target>Sie müssen mindestens einen Benutzer oder ein Team auswählen.</target>
             </trans-unit>
+            <trans-unit id="This invoice document cannot be used, please rename the file and upload it again.">
+                <source>This invoice document cannot be used, please rename the file and upload it again.</source>
+                <target>Dieses Rechnungsdokument kann nicht verwendet werden, bitte benennen Sie die Datei um und laden Sie diese erneut hoch.</target>
+            </trans-unit>
         </body>
     </file>
 </xliff>

translations/validators.en.xlf

@@ -18,6 +18,10 @@
                 <source>You must select at least one user or team.</source>
                 <target>You must select at least one user or team.</target>
             </trans-unit>
+            <trans-unit id="This invoice document cannot be used, please rename the file and upload it again.">
+                <source>This invoice document cannot be used, please rename the file and upload it again.</source>
+                <target>This invoice document cannot be used, please rename the file and upload it again.</target>
+            </trans-unit>
         </body>
     </file>
 </xliff>