fix: improve code paths at destroying and cleaning up collectives

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;
 	}
 
 	/**

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();

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)
 

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,21 +120,15 @@ 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()
 
 		const landingPageItem = editor.pagePicker.locator('.page-preview-item').filter({ hasText: 'Landing page' })
 		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()
 

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(

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 = {

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);
+	}
 }