From 760007272b5b8c5ec875e51131ffa97056818884 Mon Sep 17 00:00:00 2001 From: Jonas Date: Thu, 24 Sep 2026 16:51:36 +0200 Subject: [PATCH 1/4] test(playwright): upload fixture to random folder Should fix errors when the file cannot be overwritten as the Text editor from a previous session still holds a lock on the file to be written. Signed-off-by: Jonas Assisted-by: OpenCode:claude-fable-5.1 --- playwright/e2e/page-links-viewer.spec.ts | 7 ++++--- playwright/support/fixtures/User.ts | 8 ++++++++ playwright/support/helpers/links.ts | 3 ++- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/playwright/e2e/page-links-viewer.spec.ts b/playwright/e2e/page-links-viewer.spec.ts index 9ae42c21b4..b48f7f4af5 100644 --- a/playwright/e2e/page-links-viewer.spec.ts +++ b/playwright/e2e/page-links-viewer.spec.ts @@ -32,14 +32,12 @@ const links: ViewerLinkTestCaseData[] = [ getLinkUrl: ({ fileId }) => `/index.php/f/${fileId}`, fixtureName: 'test.md', mimetype: 'text/markdown', - getPath: () => '', }, { description: 'absolute files app path to image file', getLinkUrl: ({ fileId }) => `/index.php/f/${fileId}`, fixtureName: 'test.png', mimetype: 'image/png', - getPath: () => '', }, { description: 'absolute files app path to PDF file', @@ -57,9 +55,12 @@ test.describe('Links to viewer', () => { test(`Opens link with ${linkData.description} in viewer (${modeLabel} mode)`, async ({ collective, editor, page, user }) => { const sourcePage = collective.getPageByTitle('Link Source') + // Upload into a fresh folder, as the Text editor of a previous test may still hold a lock on a file with the same name. + const path = linkData.getPath?.({ sourcePage }) + ?? await user.createFolder({ path: randomString() }, page) const fileId = await user.uploadFixture({ name: linkData.fixtureName, - path: linkData.getPath({ sourcePage }), + path, mimetype: linkData.mimetype, }, page) diff --git a/playwright/support/fixtures/User.ts b/playwright/support/fixtures/User.ts index d6ff489088..3b52c59036 100644 --- a/playwright/support/fixtures/User.ts +++ b/playwright/support/fixtures/User.ts @@ -23,6 +23,14 @@ export class User { return await trashAndDeleteCollective({ ...options, page }) } + async createFolder(options: { path: string }, page: Page) { + await page.request.fetch( + webdavUrl(this.account.userId, options.path), + { method: 'MKCOL', failOnStatusCode: true }, + ) + return options.path + } + async uploadFixture(options: { name: string, path: string, mimetype: string }, page: Page) { const filepath = resolve(import.meta.dirname, 'files', options.name) await page.request.put( diff --git a/playwright/support/helpers/links.ts b/playwright/support/helpers/links.ts index 59fd314285..cf2b9961ff 100644 --- a/playwright/support/helpers/links.ts +++ b/playwright/support/helpers/links.ts @@ -26,7 +26,8 @@ export type ViewerLinkTestCaseData = { getLinkUrl: (params: { fileId: number }) => string fixtureName: string mimetype: string - getPath: (params: { sourcePage: CollectivePage }) => string + // Defaults to a new folder in the user's home + getPath?: (params: { sourcePage: CollectivePage }) => string } export type SameTabLinkTestCaseData = { From e0e2da87d53937bce1eb09534adf9b32d39872b0 Mon Sep 17 00:00:00 2001 From: Jonas Date: Thu, 24 Sep 2026 17:05:44 +0200 Subject: [PATCH 2/4] test(playwright): close filter menu before typing in page picker Should fix flaky page picker tests. Signed-off-by: Jonas Assisted-by: OpenCode:claude-fable-5.1 --- playwright/e2e/page-picker.spec.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/playwright/e2e/page-picker.spec.ts b/playwright/e2e/page-picker.spec.ts index 72c3ecad30..de370fff68 100644 --- a/playwright/e2e/page-picker.spec.ts +++ b/playwright/e2e/page-picker.spec.ts @@ -90,12 +90,18 @@ test.describe('Custom page picker - cross-collective search', () => { editor.setMode(true) await editor.clickMenu('Insert link', 'Link to page') await page.waitForTimeout(200) - }) - test('link to page from other collective', async ({ page, editor }) => { - await editor.pagePicker.locator('.searchbar [aria-haspopup="menu"]').click() + const filterMenu = editor.pagePicker.locator('.searchbar [aria-haspopup="menu"]') + await filterMenu.click() await page.getByText('Limit to current collective').click() + // The menu stays open with focus on the checkbox. Close it, else typing + // a space into the search field may toggle the checkbox again. + await filterMenu.click() + await expect(page.getByText('Limit to current collective')).toBeHidden() + }) + + test('link to page from other collective', async ({ page, editor }) => { // Should already be listed without searching const otherPageItem = editor.pagePicker.locator('.page-preview-item').filter({ hasText: 'Other collective page' }) await expect(otherPageItem).toBeVisible() @@ -114,10 +120,7 @@ test.describe('Custom page picker - cross-collective search', () => { await expect(pageWidget).toHaveAttribute('href', origin + otherTargetPage.getPageUrl()) }) - test('searching "Landing page" lists landing page', async ({ page, editor }) => { - await editor.pagePicker.locator('.searchbar [aria-haspopup="menu"]').click() - await page.getByText('Limit to current collective').click() - + test('searching "Landing page" lists landing page', async ({ editor }) => { await editor.pagePickerSearch.fill('Landing page') await expect(editor.pagePicker.locator('.page-preview-item').filter({ hasText: 'Source page' })).not.toBeVisible() @@ -125,10 +128,7 @@ test.describe('Custom page picker - cross-collective search', () => { await expect(landingPageItem).toHaveCount(2) }) - test('searching collective title lists landing page', async ({ page, collectives, editor }) => { - await editor.pagePicker.locator('.searchbar [aria-haspopup="menu"]').click() - await page.getByText('Limit to current collective').click() - + test('searching collective title lists landing page', async ({ collectives, editor }) => { await editor.pagePickerSearch.fill(collectives[1].data.name) await expect(editor.pagePicker.locator('.page-preview-item').filter({ hasText: 'Source page' })).not.toBeVisible() From 1570d3796526d02809a1da933b56aff2625a2be0 Mon Sep 17 00:00:00 2001 From: Jonas Date: Thu, 24 Sep 2026 17:12:59 +0200 Subject: [PATCH 3/4] fix(versions): delete version entities when purging a collective deleteVersionsFolder() passed the file ID of each version file to deleteAllVersionsForFile(), which expects the ID of the page file the versions belong to. The lookup never matched, so the collectives_version rows of a purged collective were never deleted. Instead, delete the version entities directly. The version files are removed along with the collective's versions folder anyway. This also skips listing each per-file folder. Signed-off-by: Jonas Assisted-by: OpenCode:claude-fable-5.1 --- lib/Versions/VersionsBackend.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/Versions/VersionsBackend.php b/lib/Versions/VersionsBackend.php index 23e33a8394..67b25d3eec 100644 --- a/lib/Versions/VersionsBackend.php +++ b/lib/Versions/VersionsBackend.php @@ -333,11 +333,8 @@ public function deleteVersionsFolder(int $folderId): void { /** @var Folder $versionsFolder */ $versionsFolder = $this->getVersionsFolder($folderId); foreach ($versionsFolder->getDirectoryListing() as $fileFolder) { - if (!($fileFolder instanceof Folder)) { - continue; - } - foreach ($fileFolder->getDirectoryListing() as $file) { - $this->deleteAllVersionsForFile($folderId, $file->getId()); + if ($fileFolder instanceof Folder) { + $this->collectiveVersionMapper->deleteAllVersionsForFileId((int)$fileFolder->getName()); } } $versionsFolder->delete(); From 25a3c8073a46b0d03a13b4b0c0f36e3f50013b16 Mon Sep 17 00:00:00 2001 From: Jonas Date: Thu, 24 Sep 2026 17:52:02 +0200 Subject: [PATCH 4/4] fix(trash): purge collective before destroying its team If purging the collective fails but the team already got destroyed, the collective no longer can be found in trash (by user). Thus it's safer to first purge the collective and only destroy the team if this succeeded. Signed-off-by: Jonas Assisted-by: OpenCode:claude-fable-5.1 --- lib/Service/CollectiveService.php | 15 ++- tests/Unit/Service/CollectiveServiceTest.php | 107 ++++++++++++++++++- 2 files changed, 116 insertions(+), 6 deletions(-) diff --git a/lib/Service/CollectiveService.php b/lib/Service/CollectiveService.php index 5695e1c257..172a1e5c11 100644 --- a/lib/Service/CollectiveService.php +++ b/lib/Service/CollectiveService.php @@ -337,14 +337,23 @@ public function trashCollective(int $id, string $userId): Collective { */ public function deleteCollective(int $id, string $userId, bool $deleteCircle): Collective { $collective = $this->getCollectiveFromTrash($id, $userId); + $circleId = $collective->getCircleId(); + + // Check permission up front to not purge the collective and then fail to destroy the team + if ($deleteCircle && !$this->circleHelper->isOwner($circleId, $userId)) { + throw new NotPermittedException('Not allowed to destroy team ' . $circleId); + } + + // Purge before touching the team: the collective can only be found in trash while its team exists. + $collective = $this->purgeCollective($collective); if ($deleteCircle) { - $this->circleHelper->destroyCircle($collective->getCircleId(), $userId); + $this->circleHelper->destroyCircle($circleId, $userId); } else { - $this->circleHelper->unflagCircleAsAppManaged($collective->getCircleId()); + $this->circleHelper->unflagCircleAsAppManaged($circleId); } - return $this->purgeCollective($collective); + return $collective; } /** diff --git a/tests/Unit/Service/CollectiveServiceTest.php b/tests/Unit/Service/CollectiveServiceTest.php index 2e4c24ef13..8a5e9d400f 100644 --- a/tests/Unit/Service/CollectiveServiceTest.php +++ b/tests/Unit/Service/CollectiveServiceTest.php @@ -24,11 +24,14 @@ use OCA\Collectives\Service\CollectiveService; use OCA\Collectives\Service\CollectiveShareService; use OCA\Collectives\Service\NotFoundException; +use OCA\Collectives\Service\NotPermittedException; use OCA\Collectives\Service\UnprocessableEntityException; use OCP\App\IAppManager; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\File; use OCP\Files\Folder; +use OCP\Files\InvalidPathException; +use OCP\Files\NotFoundException as FilesNotFoundException; use OCP\IL10N; use Symfony\Component\String\Slugger\SluggerInterface; use Symfony\Component\String\UnicodeString; @@ -38,6 +41,7 @@ class CollectiveServiceTest extends TestCase { private string $userId = 'jane'; private CollectiveMapper $collectiveMapper; private CollectiveHelper $collectiveHelper; + private CollectiveFolderManager $collectiveFolderManager; private CircleHelper $circleHelper; private IL10N $l10n; private CollectiveService $service; @@ -48,13 +52,13 @@ protected function setUp(): void { $this->collectiveMapper = $this->createMock(CollectiveMapper::class); $this->collectiveHelper = $this->createMock(CollectiveHelper::class); - $collectiveFolderManager = $this->createMock(CollectiveFolderManager::class); + $this->collectiveFolderManager = $this->createMock(CollectiveFolderManager::class); $folder = $this->createMock(Folder::class); $file = $this->createMock(File::class); $folder->method('get') ->willReturn($file); - $collectiveFolderManager->method('initializeFolder') + $this->collectiveFolderManager->method('initializeFolder') ->willReturn($folder); $this->circleHelper = $this->createMock(CircleHelper::class); @@ -77,7 +81,7 @@ protected function setUp(): void { $appManager, $this->collectiveMapper, $this->collectiveHelper, - $collectiveFolderManager, + $this->collectiveFolderManager, $this->circleHelper, $shareService, $collectiveUserSettingsMapper, @@ -209,4 +213,101 @@ public function testCreate(): void { 'canLeave' => true, ], $collective->jsonSerialize()); } + + private function mockTrashedCollective(): Collective { + $collective = new Collective(); + $collective->setId(123); + $collective->setCircleId('CircleId'); + $collective->setTrashTimestamp(1700000000); + $this->collectiveMapper->method('findTrashByIdAndUser') + ->with(123, $this->userId) + ->willReturn($collective); + return $collective; + } + + public function testDeleteCollectiveDestroysCircleAfterPurge(): void { + $collective = $this->mockTrashedCollective(); + $this->circleHelper->method('isOwner') + ->willReturn(true); + $this->collectiveFolderManager->method('getFolder') + ->willThrowException(new FilesNotFoundException()); + + $calls = []; + $this->collectiveMapper->expects(self::once()) + ->method('delete') + ->with($collective) + ->willReturnCallback(function (Collective $collective) use (&$calls): Collective { + $calls[] = 'purge'; + return $collective; + }); + $this->circleHelper->expects(self::once()) + ->method('destroyCircle') + ->with('CircleId', $this->userId) + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'destroyCircle'; + }); + + $this->service->deleteCollective(123, $this->userId, true); + self::assertSame(['purge', 'destroyCircle'], $calls); + } + + public function testDeleteCollectiveKeepCircleUnflagsCircleAfterPurge(): void { + $collective = $this->mockTrashedCollective(); + $this->collectiveFolderManager->method('getFolder') + ->willThrowException(new FilesNotFoundException()); + + $calls = []; + $this->collectiveMapper->expects(self::once()) + ->method('delete') + ->with($collective) + ->willReturnCallback(function (Collective $collective) use (&$calls): Collective { + $calls[] = 'purge'; + return $collective; + }); + $this->circleHelper->expects(self::never()) + ->method('destroyCircle'); + $this->circleHelper->expects(self::once()) + ->method('unflagCircleAsAppManaged') + ->with('CircleId') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'unflagCircle'; + }); + + $this->service->deleteCollective(123, $this->userId, false); + self::assertSame(['purge', 'unflagCircle'], $calls); + } + + public function testDeleteCollectiveKeepsCircleIfPurgeFails(): void { + $this->mockTrashedCollective(); + $this->circleHelper->method('isOwner') + ->willReturn(true); + $this->collectiveFolderManager->method('getFolder') + ->willThrowException(new InvalidPathException()); + + $this->collectiveMapper->expects(self::never()) + ->method('delete'); + $this->circleHelper->expects(self::never()) + ->method('destroyCircle'); + $this->circleHelper->expects(self::never()) + ->method('unflagCircleAsAppManaged'); + + $this->expectException(NotFoundException::class); + $this->service->deleteCollective(123, $this->userId, true); + } + + public function testDeleteCollectiveAndCircleAsNonOwnerFailsBeforePurge(): void { + $this->mockTrashedCollective(); + $this->circleHelper->method('isOwner') + ->willReturn(false); + + $this->collectiveFolderManager->expects(self::never()) + ->method('getFolder'); + $this->collectiveMapper->expects(self::never()) + ->method('delete'); + $this->circleHelper->expects(self::never()) + ->method('destroyCircle'); + + $this->expectException(NotPermittedException::class); + $this->service->deleteCollective(123, $this->userId, true); + } }