From 28393c013c4163c891a635500bc0d3bb3dba64e8 Mon Sep 17 00:00:00 2001 From: samin-z Date: Mon, 7 Sep 2026 14:15:36 +0200 Subject: [PATCH] fix: sync upload folder on rename Signed-off-by: samin-z --- lib/Controller/ApiController.php | 5 ++ lib/Helper/FilePathHelper.php | 50 +++++++++++++++ lib/Service/FormsService.php | 11 ++++ lib/Service/UploadedFilesShareService.php | 22 +++---- .../Controller/ShareApiControllerTest.php | 11 ++-- .../Service/UploadedFilesShareServiceTest.php | 63 ++++++++++++++----- 6 files changed, 126 insertions(+), 36 deletions(-) diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 310c826fc..de50b2fe8 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -361,11 +361,16 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse { } // Create FormEntity with given Params & Id. + $titleChanged = array_key_exists('title', $keyValuePairs); foreach ($keyValuePairs as $key => $value) { $method = 'set' . ucfirst($key); $form->$method($value); } + if ($titleChanged) { + $this->formsService->syncFormUploadFolderName($form); + } + // Update changed Columns in Db. $this->formMapper->update($form); diff --git a/lib/Helper/FilePathHelper.php b/lib/Helper/FilePathHelper.php index 23e876c0e..678d3cb08 100644 --- a/lib/Helper/FilePathHelper.php +++ b/lib/Helper/FilePathHelper.php @@ -75,6 +75,56 @@ public function getAllFormFoldersById(int $formId, string $ownerId): array { return $matchingFolders; } + /** + * Rename the upload folder to the form's current title. + * If that name already contains files, move the other folder's contents into it and delete the other folder. + */ + public function syncFormUploadFolderName(Form $form): void { + $formsFolder = $this->getFormsFolder($form->getOwnerId()); + if ($formsFolder === null) { + return; + } + + $newName = $this->normalizeFileName($form->getId() . ' - ' . $form->getTitle()); + $folders = $this->getAllFormFoldersById($form->getId(), $form->getOwnerId()); + if ($folders === []) { + return; + } + + $source = $folders[0]; + foreach ($folders as $candidate) { + if ($candidate->getDirectoryListing() !== []) { + $source = $candidate; + break; + } + } + + if ($source->getName() !== $newName) { + if ($formsFolder->nodeExists($newName)) { + $existing = $formsFolder->get($newName); + if ($existing instanceof Folder && $existing->getId() !== $source->getId()) { + if ($existing->getDirectoryListing() === []) { + $existing->delete(); + } else { + foreach ($source->getDirectoryListing() as $node) { + $node->move($existing->getPath() . '/' . $node->getName()); + } + $source->delete(); + } + } + } + if (!$formsFolder->nodeExists($newName)) { + $source->move($formsFolder->getPath() . '/' . $newName); + } + } + + foreach ($this->getAllFormFoldersById($form->getId(), $form->getOwnerId()) as $leftover) { + if ($leftover->getName() !== $newName && $leftover->getDirectoryListing() === []) { + $leftover->delete(); + } + } + } + /** * Get the forms folder for a user */ diff --git a/lib/Service/FormsService.php b/lib/Service/FormsService.php index 3d5ee1713..b6534cd67 100644 --- a/lib/Service/FormsService.php +++ b/lib/Service/FormsService.php @@ -1059,6 +1059,17 @@ public function getFormUploadedFilesFolderPath(Form $form): string { return $this->filePathHelper->getFormUploadedFilesFolderPath($form); } + /** + * Keep Forms upload folder name in sync with the form title. + */ + public function syncFormUploadFolderName(Form $form): void { + try { + $this->filePathHelper->syncFormUploadFolderName($form); + } catch (\Throwable $e) { + $this->logger->warning('Failed to sync form upload folder name', ['exception' => $e]); + } + } + public function getUploadedFilePath(Form $form, int $submissionId, int $questionId, ?string $questionName, string $questionText): string { return $this->filePathHelper->getUploadedFilePath($form, $submissionId, $questionId, $questionName, $questionText); } diff --git a/lib/Service/UploadedFilesShareService.php b/lib/Service/UploadedFilesShareService.php index ded293870..2038f9628 100644 --- a/lib/Service/UploadedFilesShareService.php +++ b/lib/Service/UploadedFilesShareService.php @@ -14,15 +14,12 @@ use OCA\Forms\Db\Share; use OCA\Forms\Db\ShareMapper; use OCA\Forms\Helper\FilePathHelper; -use OCP\Files\IRootFolder; -use OCP\Files\NotFoundException; use OCP\Share\IManager; use OCP\Share\IShare; use Psr\Log\LoggerInterface; class UploadedFilesShareService { public function __construct( - private readonly IRootFolder $rootFolder, private readonly FilePathHelper $filePathHelper, private readonly IManager $shareManager, private readonly ShareMapper $shareMapper, @@ -51,18 +48,13 @@ public function removeForCollaborator(Form $form, Share $formShare): void { } try { - $userFolder = $this->rootFolder->getUserFolder($form->getOwnerId()); - $uploadedFilesFolderPath = $this->filePathHelper->getFormUploadedFilesFolderPath($form); - try { - $folder = $userFolder->get($uploadedFilesFolderPath); - } catch (NotFoundException) { - return; - } - - $folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder, false, -1); - foreach ($folderShares as $folderShare) { - if ($folderShare->getSharedWith() === $formShare->getShareWith()) { - $this->shareManager->deleteShare($folderShare); + $folders = $this->filePathHelper->getAllFormFoldersById($form->getId(), $form->getOwnerId()); + foreach ($folders as $folder) { + $folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder, false, -1); + foreach ($folderShares as $folderShare) { + if ($folderShare->getSharedWith() === $formShare->getShareWith()) { + $this->shareManager->deleteShare($folderShare); + } } } } catch (\Throwable $e) { diff --git a/tests/Unit/Controller/ShareApiControllerTest.php b/tests/Unit/Controller/ShareApiControllerTest.php index 42099d651..192ad34ee 100644 --- a/tests/Unit/Controller/ShareApiControllerTest.php +++ b/tests/Unit/Controller/ShareApiControllerTest.php @@ -88,7 +88,6 @@ public function setUp(): void { $this->filePathHelper = new FilePathHelper($this->rootFolder, $this->filenameValidator); $this->shareManager = $this->createMock(IManager::class); $this->uploadedFilesShareService = new UploadedFilesShareService( - $this->rootFolder, $this->filePathHelper, $this->shareManager, $this->shareMapper, @@ -545,14 +544,18 @@ public function testDeleteShare_cleansUpFileShare(): void { ->with('5') ->willReturn($form); - // Mock the uploaded files folder lookup + // Mock the uploaded files folder lookup by form id $folder = $this->createMock(Folder::class); + $folder->method('getName')->willReturn('5 - form'); + $formsFolder = $this->createMock(Folder::class); + $formsFolder->method('getDirectoryListing')->willReturn([$folder]); + $userFolder = $this->createUserFolderMock(); $userFolder->expects($this->once()) ->method('get') - ->willReturn($folder); + ->with(Constants::FILES_FOLDER) + ->willReturn($formsFolder); - // Set expectations on the rootFolder $this->rootFolder->expects($this->once()) ->method('getUserFolder') ->with('currentUser') diff --git a/tests/Unit/Service/UploadedFilesShareServiceTest.php b/tests/Unit/Service/UploadedFilesShareServiceTest.php index 807b51233..c42ed3d48 100644 --- a/tests/Unit/Service/UploadedFilesShareServiceTest.php +++ b/tests/Unit/Service/UploadedFilesShareServiceTest.php @@ -15,11 +15,9 @@ use OCA\Forms\Db\ShareMapper; use OCA\Forms\Helper\FilePathHelper; use OCA\Forms\Service\UploadedFilesShareService; -use OCA\Forms\Tests\Unit\UserFolderMockTrait; use OCP\Files\Folder; use OCP\Files\IFilenameValidator; use OCP\Files\IRootFolder; -use OCP\Files\NotFoundException; use OCP\Share\IManager; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; @@ -27,8 +25,6 @@ use Test\TestCase; class UploadedFilesShareServiceTest extends TestCase { - use UserFolderMockTrait; - private UploadedFilesShareService $service; private ShareMapper|MockObject $shareMapper; private IRootFolder|MockObject $rootFolder; @@ -48,7 +44,6 @@ protected function setUp(): void { ); $this->service = new UploadedFilesShareService( - $this->rootFolder, $filePathHelper, $this->shareManager, $this->shareMapper, @@ -56,6 +51,21 @@ protected function setUp(): void { ); } + /** + * @return array{0: Folder&MockObject, 1: Folder&MockObject} + */ + private function mockFormsFolderWithChildren(string $ownerId, array $children): array { + $formsFolder = $this->createMock(Folder::class); + $formsFolder->method('getDirectoryListing')->willReturn($children); + + $userFolder = $this->createMock(Folder::class); + $userFolder->method('get')->with(Constants::FILES_FOLDER)->willReturn($formsFolder); + + $this->rootFolder->method('getUserFolder')->with($ownerId)->willReturn($userFolder); + + return [$userFolder, $formsFolder]; + } + public function testRemoveAllForFormCleansUpResultsShares(): void { $form = new Form(); $form->setId(2); @@ -78,15 +88,8 @@ public function testRemoveAllForFormCleansUpResultsShares(): void { ->willReturn([$resultsShare, $submitOnlyShare]); $folder = $this->createMock(Folder::class); - $userFolder = $this->createUserFolderMock(); - $userFolder->expects($this->once()) - ->method('get') - ->with('Forms/2 - test') - ->willReturn($folder); - $this->rootFolder->expects($this->once()) - ->method('getUserFolder') - ->with('alice') - ->willReturn($userFolder); + $folder->method('getName')->willReturn('2 - test'); + $this->mockFormsFolderWithChildren('alice', [$folder]); $fileShare = $this->createMock(IShare::class); $fileShare->method('getSharedWith')->willReturn('bob'); @@ -101,6 +104,34 @@ public function testRemoveAllForFormCleansUpResultsShares(): void { $this->service->removeAllForForm($form); } + public function testRemoveForCollaboratorRevokesSharesOnRenamedFormFolder(): void { + $form = new Form(); + $form->setId(19); + $form->setTitle('T2'); + $form->setOwnerId('alice'); + + $share = new Share(); + $share->setShareType(IShare::TYPE_USER); + $share->setShareWith('carol'); + $share->setPermissions([Constants::PERMISSION_RESULTS]); + + $oldTitleFolder = $this->createMock(Folder::class); + $oldTitleFolder->method('getName')->willReturn('19 - T1'); + $this->mockFormsFolderWithChildren('alice', [$oldTitleFolder]); + + $fileShare = $this->createMock(IShare::class); + $fileShare->method('getSharedWith')->willReturn('carol'); + $this->shareManager->expects($this->once()) + ->method('getSharesBy') + ->with('alice', IShare::TYPE_USER, $oldTitleFolder, false, -1) + ->willReturn([$fileShare]); + $this->shareManager->expects($this->once()) + ->method('deleteShare') + ->with($fileShare); + + $this->service->removeForCollaborator($form, $share); + } + public function testRemoveForCollaboratorSkipsShareTypesWithoutFilesShare(): void { $form = new Form(); $form->setId(2); @@ -128,9 +159,7 @@ public function testRemoveForCollaboratorIgnoresMissingUploadedFilesFolder(): vo $share->setShareWith('bob'); $share->setPermissions([Constants::PERMISSION_RESULTS]); - $userFolder = $this->createUserFolderMock(); - $userFolder->method('get')->willThrowException(new NotFoundException()); - $this->rootFolder->method('getUserFolder')->with('alice')->willReturn($userFolder); + $this->mockFormsFolderWithChildren('alice', []); $this->shareManager->expects($this->never())->method('getSharesBy'); $this->service->removeForCollaborator($form, $share);