From d2cf414f5af6289f207c9cfa3416be6d17bd3489 Mon Sep 17 00:00:00 2001 From: Ian Lindsay Date: Sat, 25 May 2024 01:13:43 +0100 Subject: [PATCH] feat: downloaded files now use the document description where available --- .../Document/AbstractDownload.php | 37 +++++++--- .../Domain/QueryHandler/Document/Download.php | 14 +++- .../QueryHandler/Document/DownloadGuide.php | 6 +- .../src/Service/DocManClient.php | 8 +- .../src/Service/WebDavClient.php | 7 +- .../Document/AbstractDownloadStub.php | 5 +- .../Document/AbstractDownloadTest.php | 73 ++++++++++++++++++- .../Document/DownloadGuideTest.php | 11 ++- .../QueryHandler/Document/DownloadTest.php | 38 ++++++++-- .../QueryHandler/QueryHandlerTestCase.php | 13 ++-- .../File/ContentStoreFileUploaderTest.php | 5 +- 11 files changed, 168 insertions(+), 49 deletions(-) diff --git a/module/Api/src/Domain/QueryHandler/Document/AbstractDownload.php b/module/Api/src/Domain/QueryHandler/Document/AbstractDownload.php index 2259f68fe0..8f053ac6fa 100644 --- a/module/Api/src/Domain/QueryHandler/Document/AbstractDownload.php +++ b/module/Api/src/Domain/QueryHandler/Document/AbstractDownload.php @@ -8,6 +8,8 @@ use Dvsa\Olcs\Api\Domain\UploaderAwareTrait; use Dvsa\Olcs\DocumentShare\Data\Object\File as ContentStoreFile; use Dvsa\Olcs\Utils\Helper\FileHelper; +use Laminas\Http\Response\Stream; +use Olcs\Logging\Log\Logger; use Psr\Container\ContainerInterface; use Laminas\Http\Response; use Psr\Container\ContainerExceptionInterface; @@ -31,24 +33,28 @@ abstract class AbstractDownload extends AbstractQueryHandler implements Uploader /** * Process downloading file * - * @param string $identifier File name - * @param string|null $path Path to file - * - * @return Response\Stream * @throws NotFoundException */ - protected function download($identifier, $path = null) + protected function download(string $identifier, ?string $path = null, ?string $chosenFileName = null): Stream { if ($path === null) { $path = $identifier; } $file = $this->getUploader()->download($path); - if ($file === null) { + + if ($file === false) { + $logInfo = [ + 'identifier' => $identifier, + 'path' => $path, + 'filename' => $chosenFileName, + ]; + + Logger::info('File could not be downloaded', $logInfo); throw new NotFoundException(); } - $response = new \Laminas\Http\Response\Stream(); + $response = new Stream(); $response->setStatusCode(Response::STATUS_CODE_200); $fileName = $file->getResource(); @@ -59,15 +65,26 @@ protected function download($identifier, $path = null) $response->setContentLength($fileSize); $response->setCleanup(true); + $extension = FileHelper::getExtension($identifier); + $isInline = ( $this->isInline === true - || 'html' === FileHelper::getExtension($identifier) + || 'html' === $extension ); - // OLCS-14910 If file doesn't have an extension then add a '.txt' extension $downloadFileName = basename($identifier); - if (empty(FileHelper::getExtension($downloadFileName))) { + + // OLCS-14910 If file doesn't have an extension then add a '.txt' extension + if (empty($extension)) { + //used in case of the original identifier being used $downloadFileName .= '.txt'; + + //used in the case of a user chosen filename + $extension = 'txt'; + } + + if ($chosenFileName !== null) { + $downloadFileName = $chosenFileName . '.' . $extension; } $headers = $response->getHeaders(); diff --git a/module/Api/src/Domain/QueryHandler/Document/Download.php b/module/Api/src/Domain/QueryHandler/Document/Download.php index f4800b396f..7c9ec6926f 100644 --- a/module/Api/src/Domain/QueryHandler/Document/Download.php +++ b/module/Api/src/Domain/QueryHandler/Document/Download.php @@ -30,8 +30,20 @@ public function handleQuery(QueryInterface $query) /* @var \Dvsa\Olcs\Api\Entity\Doc\Document $document */ $document = $this->getRepo()->fetchById($query->getIdentifier()); + $chosenFileName = null; + + if ($this->isInternalUser()) { + $description = $document->getDescription(); + + if (!empty($description)) { + $chosenFileName = $description; + } + } + return $this->download( - $document->getIdentifier() + $document->getIdentifier(), + null, + $chosenFileName ); } } diff --git a/module/Api/src/Domain/QueryHandler/Document/DownloadGuide.php b/module/Api/src/Domain/QueryHandler/Document/DownloadGuide.php index 145a1516fd..85eb703649 100644 --- a/module/Api/src/Domain/QueryHandler/Document/DownloadGuide.php +++ b/module/Api/src/Domain/QueryHandler/Document/DownloadGuide.php @@ -4,6 +4,7 @@ use Dvsa\Olcs\Api\Domain\Exception\NotFoundException; use Dvsa\Olcs\Transfer\Query\QueryInterface; +use Laminas\Http\Response\Stream; /** * Download a guide document, these are located in a "/guides/" directory in the content store @@ -13,14 +14,11 @@ class DownloadGuide extends AbstractDownload protected $extraRepos = ['DocTemplate']; /** - * Handle query - * * @param \Dvsa\Olcs\Transfer\Query\Document\DownloadGuide $query DTO * - * @return array * @throws NotFoundException */ - public function handleQuery(QueryInterface $query) + public function handleQuery(QueryInterface $query): Stream { $this->setIsInline($query->isInline()); diff --git a/module/DocumentShare/src/Service/DocManClient.php b/module/DocumentShare/src/Service/DocManClient.php index dc00419f4f..5c9496fe78 100644 --- a/module/DocumentShare/src/Service/DocManClient.php +++ b/module/DocumentShare/src/Service/DocManClient.php @@ -111,10 +111,8 @@ public function getWorkspace() * Read content from document store * * @param string $path Path - * - * @return File|null */ - public function read($path): ?File + public function read($path): File | false { $tmpFileName = tempnam(sys_get_temp_dir(), self::DS_DOWNLOAD_FILE_PREFIX); @@ -132,7 +130,7 @@ public function read($path): ?File if (!$response->isSuccess()) { $message = json_encode(["error" => self::ERR_RESP_FAIL, "reason" => $response->getStatusCode(), "path" => $path]); Logger::logResponse($response->getStatusCode(), $message); - return null; + return false; } $file = new File(); @@ -158,7 +156,7 @@ public function read($path): ?File Logger::logResponse(Response::STATUS_CODE_404, $errMssg); } - return null; + return false; } /** diff --git a/module/DocumentShare/src/Service/WebDavClient.php b/module/DocumentShare/src/Service/WebDavClient.php index f90eaeb8b7..0e8bda7cd8 100644 --- a/module/DocumentShare/src/Service/WebDavClient.php +++ b/module/DocumentShare/src/Service/WebDavClient.php @@ -7,9 +7,6 @@ use League\Flysystem\FileNotFoundException; use League\Flysystem\FilesystemInterface; -/** - * Class Client - */ class WebDavClient implements DocumentStoreInterface { public const DS_DOWNLOAD_FILE_PREFIX = 'ds_dwnld_'; @@ -43,10 +40,8 @@ public function __construct(FilesystemInterface $filesystem) * Read content from document store * * @param string $path Path - * - * @return File|bool */ - public function read($path) + public function read($path): File | false { $tmpFileName = tempnam(sys_get_temp_dir(), self::DS_DOWNLOAD_FILE_PREFIX); diff --git a/test/module/Api/src/Domain/QueryHandler/Document/AbstractDownloadStub.php b/test/module/Api/src/Domain/QueryHandler/Document/AbstractDownloadStub.php index 5ebf4998e1..9eccaad8ef 100644 --- a/test/module/Api/src/Domain/QueryHandler/Document/AbstractDownloadStub.php +++ b/test/module/Api/src/Domain/QueryHandler/Document/AbstractDownloadStub.php @@ -4,15 +4,16 @@ use Dvsa\Olcs\Api\Domain\QueryHandler\Document\AbstractDownload; use Dvsa\Olcs\Transfer\Query\QueryInterface; +use Laminas\Http\Response\Stream; /** * Stub class of AbstractDownload handler for testing */ class AbstractDownloadStub extends AbstractDownload { - public function download($identifier, $path = null) + public function download(string $identifier, ?string $path = null, ?string $fileName = null): Stream { - return parent::download($identifier, $path); + return parent::download($identifier, $path, $fileName); } public function setIsInline($inline) diff --git a/test/module/Api/src/Domain/QueryHandler/Document/AbstractDownloadTest.php b/test/module/Api/src/Domain/QueryHandler/Document/AbstractDownloadTest.php index 32820c7d77..c511793cc4 100644 --- a/test/module/Api/src/Domain/QueryHandler/Document/AbstractDownloadTest.php +++ b/test/module/Api/src/Domain/QueryHandler/Document/AbstractDownloadTest.php @@ -1,5 +1,7 @@ shouldReceive('download') ->once() ->with($path) - ->andReturn(null); + ->andReturnFalse(); $this->sut->download($path); } @@ -54,7 +56,7 @@ public function testDownloadFailExcNotFound() /** * @dataProvider dpTestDownload */ - public function testDownload($identifier, $path, $isInline, $expect) + public function testDownload($identifier, $path, $isInline, $chosenFileName, $expect) { $this->sut->setIsInline($isInline); @@ -79,7 +81,7 @@ public function testDownload($identifier, $path, $isInline, $expect) ->andReturn($mockFile); // call & check - $actual = $this->sut->download($identifier, $path); + $actual = $this->sut->download($identifier, $path, $chosenFileName); static::assertInstanceOf(\Laminas\Http\Response\Stream::class, $actual); static::assertEquals($tmpFilePath, $actual->getStreamName()); @@ -101,6 +103,7 @@ public function dpTestDownload() 'identifier' => 'unit_file.ext', 'path' => '/unit_dir/unit_file1.pdf', 'isInline' => false, + null, 'expect' => [ 'mime' => self::MIME_TYPE, 'isDownload' => true, @@ -112,6 +115,7 @@ public function dpTestDownload() 'identifier' => 'unit_file.html', 'path' => null, 'isInline' => false, + null, 'expect' => [ 'mime' => self::MIME_TYPE, 'isDownload' => false, @@ -123,6 +127,7 @@ public function dpTestDownload() 'identifier' => 'dir/dir/unit_file.unit_excl_ext', 'path' => null, 'isInline' => false, + null, 'expect' => [ 'mime' => self::MIME_TYPE_EXCLUDE, 'isDownload' => true, @@ -134,6 +139,7 @@ public function dpTestDownload() 'identifier' => 'unit_file.ext', 'path' => 'unti_path', 'isInline' => true, + null, 'expect' => [ 'mime' => self::MIME_TYPE, 'isDownload' => false, @@ -145,6 +151,7 @@ public function dpTestDownload() 'identifier' => '/foo/bar', 'path' => null, 'isInline' => false, + null, 'expect' => [ 'mime' => self::MIME_TYPE, 'isDownload' => true, @@ -152,6 +159,66 @@ public function dpTestDownload() 'filename' => 'bar.txt', ], ], + [ + 'identifier' => 'unit_file.ext', + 'path' => '/unit_dir/unit_file1.pdf', + 'isInline' => false, + 'chosen_filename', + 'expect' => [ + 'mime' => self::MIME_TYPE, + 'isDownload' => true, + 'path' => '/unit_dir/unit_file1.pdf', + 'filename' => 'chosen_filename.ext', + ], + ], + [ + 'identifier' => 'unit_file.html', + 'path' => null, + 'isInline' => false, + 'chosen_filename', + 'expect' => [ + 'mime' => self::MIME_TYPE, + 'isDownload' => false, + 'path' => 'unit_file.html', + 'filename' => 'chosen_filename.html', + ], + ], + [ + 'identifier' => 'dir/dir/unit_file.unit_excl_ext', + 'path' => null, + 'isInline' => false, + 'chosen_filename', + 'expect' => [ + 'mime' => self::MIME_TYPE_EXCLUDE, + 'isDownload' => true, + 'path' => 'dir/dir/unit_file.unit_excl_ext', + 'filename' => 'chosen_filename.unit_excl_ext', + ], + ], + [ + 'identifier' => 'unit_file.ext', + 'path' => 'unti_path', + 'isInline' => true, + 'chosen_filename.txt', + 'expect' => [ + 'mime' => self::MIME_TYPE, + 'isDownload' => false, + 'path' => 'unti_path', + 'filename' => 'chosen_filename.txt.ext', + ], + ], + [ + 'identifier' => '/foo/bar', + 'path' => null, + 'isInline' => false, + 'chosen_filename', + 'expect' => [ + 'mime' => self::MIME_TYPE, + 'isDownload' => true, + 'path' => '/foo/bar', + 'filename' => 'chosen_filename.txt', + ], + ], ]; } } diff --git a/test/module/Api/src/Domain/QueryHandler/Document/DownloadGuideTest.php b/test/module/Api/src/Domain/QueryHandler/Document/DownloadGuideTest.php index 12f857fb89..3043a61402 100644 --- a/test/module/Api/src/Domain/QueryHandler/Document/DownloadGuideTest.php +++ b/test/module/Api/src/Domain/QueryHandler/Document/DownloadGuideTest.php @@ -9,6 +9,7 @@ use Dvsa\Olcs\Api\Service\File\ContentStoreFileUploader; use Dvsa\OlcsTest\Api\Domain\QueryHandler\QueryHandlerTestCase; use Dvsa\Olcs\Api\Domain\Repository\DocTemplate as DocTemplateRepo; +use Laminas\Http\Response\Stream; use Mockery as m; /** @@ -49,13 +50,14 @@ public function testHandleQueryTryingToGetIntoParent() public function testHandleQuery() { $fileName = 'unit_file1.pdf'; + $file = m::mock(Stream::class); $this->sut ->shouldReceive('setIsInline')->once()->with(false) ->shouldReceive('download') ->once() ->with($fileName, '/guides/' . $fileName) - ->andReturn('EXPECTED'); + ->andReturn($file); $query = TransferQry\Document\DownloadGuide::create( [ @@ -65,13 +67,14 @@ public function testHandleQuery() ); $actual = $this->sut->handleQuery($query); - static::assertEquals('EXPECTED', $actual); + static::assertEquals($file, $actual); } public function testHandleQueryIsSlug() { $templateSlug = 'some-template-slug'; $fileName = 'someFile.txt'; + $file = m::mock(Stream::class); $docTemplate = m::mock(DocTemplate::class); @@ -88,7 +91,7 @@ public function testHandleQueryIsSlug() ->shouldReceive('download') ->once() ->with($fileName, '/guides/' . $fileName) - ->andReturn('EXPECTED'); + ->andReturn($file); $query = TransferQry\Document\DownloadGuide::create( [ @@ -99,6 +102,6 @@ public function testHandleQueryIsSlug() ); $actual = $this->sut->handleQuery($query); - static::assertEquals('EXPECTED', $actual); + static::assertEquals($file, $actual); } } diff --git a/test/module/Api/src/Domain/QueryHandler/Document/DownloadTest.php b/test/module/Api/src/Domain/QueryHandler/Document/DownloadTest.php index c274230a4a..e14af3d6c0 100644 --- a/test/module/Api/src/Domain/QueryHandler/Document/DownloadTest.php +++ b/test/module/Api/src/Domain/QueryHandler/Document/DownloadTest.php @@ -1,5 +1,7 @@ sut = m::mock(Download::class . '[download, setIsInline]') ->shouldAllowMockingProtectedMethods(); + $this->mockedSmServices = [ + AuthorizationService::class => m::mock(AuthorizationService::class), + ]; + $this->mockRepo('Document', \Dvsa\Olcs\Api\Domain\Repository\Document::class); $this->mockedSmServices['config'] = []; @@ -30,8 +38,12 @@ public function setUp(): void parent::setUp(); } - public function testHandleQuery() + /** + * @dataProvider dpTestHandleQuery + */ + public function testHandleQuery(bool $isInternalUser, string $documentDescription, ?string $chosenFilename): void { + $this->setupIsInternalUser($isInternalUser); $identifier = 20062016; $query = TransferQry\Document\Download::create( @@ -43,18 +55,32 @@ public function testHandleQuery() $fileName = 'foo/bar/12345.pdf'; + $document = m::mock(Document::class); + $document->expects('getIdentifier')->withNoArgs()->andReturn($fileName); + $document->expects('getDescription')->withNoArgs()->times($isInternalUser ? 1 : 0)->andReturn($documentDescription); + $this->repoMap['Document'] - ->shouldReceive('fetchById') + ->expects('fetchById') ->with($identifier) - ->once() - ->andReturn(new Document($fileName)); + ->andReturn($document); + + $download = m::mock(Stream::class); $this->sut ->shouldReceive('setIsInline')->once()->with(true) - ->shouldReceive('download')->once()->with($fileName)->andReturn('EXPECTED'); + ->shouldReceive('download')->once()->with($fileName, null, $chosenFilename)->andReturn($download); $actual = $this->sut->handleQuery($query); - static::assertEquals('EXPECTED', $actual); + static::assertEquals($download, $actual); + } + + public function dpTestHandleQuery(): array + { + return [ + [false, 'description', null], + [true, 'description', 'description'], + [true, '', null], + ]; } } diff --git a/test/module/Api/src/Domain/QueryHandler/QueryHandlerTestCase.php b/test/module/Api/src/Domain/QueryHandler/QueryHandlerTestCase.php index ffa3402c18..a8c242155d 100644 --- a/test/module/Api/src/Domain/QueryHandler/QueryHandlerTestCase.php +++ b/test/module/Api/src/Domain/QueryHandler/QueryHandlerTestCase.php @@ -8,6 +8,7 @@ use Dvsa\Olcs\Api\Domain\QueryHandler\Result; use Dvsa\Olcs\Api\Domain\QueryHandlerManager; use Dvsa\Olcs\Api\Domain\RepositoryServiceManager; +use Dvsa\Olcs\Api\Entity\User\Permission; use Dvsa\Olcs\Transfer\Query\Cache\ById as CacheByIdQry; use Dvsa\Olcs\Transfer\Query\MyAccount\MyAccount; use Dvsa\Olcs\Transfer\Query\QueryInterface; @@ -309,12 +310,12 @@ public function expectedUserDataCacheCall($result = null, $times = 1) $this->expectedQuery(MyAccount::class, [], $result, $times); } - public function expectedTrafficAreaRbacOverride() + protected function setupIsInternalUser($isInternalUser = true) { - $userData = [ - 'dataAccess' => [ - 'trafficAreas' => ["A", "B"], - ], - ]; + $this->mockedSmServices[AuthorizationService::class] + ->shouldReceive('isGranted') + ->with(Permission::INTERNAL_USER, null) + ->atLeast()->once() + ->andReturn($isInternalUser); } } diff --git a/test/module/Api/src/Service/File/ContentStoreFileUploaderTest.php b/test/module/Api/src/Service/File/ContentStoreFileUploaderTest.php index 1ae02cd0cc..0f01a5d239 100644 --- a/test/module/Api/src/Service/File/ContentStoreFileUploaderTest.php +++ b/test/module/Api/src/Service/File/ContentStoreFileUploaderTest.php @@ -49,12 +49,13 @@ function ($alias, $service) use ($sm) { public function testDownload() { + $returnedFile = m::mock(DsFile::class); $this->mockContentStoreCli->shouldReceive('read') ->once() ->with(self::IDENTIFIER) - ->andReturn('EXPECT'); + ->andReturn($returnedFile); - static::assertEquals('EXPECT', $this->sut->download(self::IDENTIFIER)); + static::assertEquals($returnedFile, $this->sut->download(self::IDENTIFIER)); } public function testRemove()