refactor: extract legacy editor file migration

lib/Controller/EditorController.php

@@ -6,6 +6,7 @@
 
 use OCA\ExeLearning\AppInfo\Application;
 use OCA\ExeLearning\Service\ElpxPackageService;
+use OCA\ExeLearning\Service\LegacyFileMigrationService;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Attribute\NoAdminRequired;
@@ -37,6 +38,7 @@ public function __construct(
 		IRequest $request,
 		private readonly IUserSession $userSession,
 		private readonly ElpxPackageService $packageService,
+		private readonly LegacyFileMigrationService $legacyFileMigration,
 		private readonly IInitialState $initialState,
 		private readonly IURLGenerator $urlGenerator,
 	) {
@@ -146,7 +148,7 @@ public function save(int $fileId): DataResponse {
 		// #20). Best-effort: if the rename fails (no permission, name
 		// collision we can't resolve, …) we keep the original extension
 		// rather than failing the save.
-		$file = $this->migrateLegacyExtensionIfNeeded($file, $user->getUID(), $fileId);
+		$file = $this->legacyFileMigration->migrate($file, $user->getUID(), $fileId);
 
 		return new DataResponse([
 			'id' => $file->getId(),
@@ -156,48 +158,6 @@ public function save(int $fileId): DataResponse {
 		]);
 	}
 
-	/**
-	 * Renames a `.elp` file to `.elpx` after a successful save so the
-	 * Files-app shows the modern extension going forward. Picks
-	 * `<base>.elpx`, falling back to `<base> (2).elpx`, `<base> (3).elpx`,
-	 * … if a sibling already exists. Returns the (possibly renamed)
-	 * file, re-fetched by id since `Node::move` invalidates the cached
-	 * handle.
-	 */
-	private function migrateLegacyExtensionIfNeeded(
-		\OCP\Files\File $file,
-		string $userId,
-		int $fileId,
-	): \OCP\Files\File {
-		$name = $file->getName();
-		if (!str_ends_with(strtolower($name), '.elp') || str_ends_with(strtolower($name), '.elpx')) {
-			return $file;
-		}
-		$base = substr($name, 0, -4); // strip '.elp'
-		try {
-			$parent = $file->getParent();
-		} catch (NotFoundException|NotPermittedException) {
-			return $file;
-		}
-		$candidate = $base . '.elpx';
-		for ($i = 2; $i < 100 && $parent->nodeExists($candidate); $i++) {
-			$candidate = sprintf('%s (%d).elpx', $base, $i);
-		}
-		if ($parent->nodeExists($candidate)) {
-			return $file;
-		}
-		try {
-			$file->move($parent->getPath() . '/' . $candidate);
-		} catch (NotPermittedException|\OCP\Files\InvalidPathException) {
-			return $file;
-		}
-		try {
-			return $this->packageService->getForUserById($userId, $fileId);
-		} catch (NotFoundException|NotPermittedException) {
-			return $file;
-		}
-	}
-
 	/**
 	 * Serves the static eXeLearning editor HTML inside an iframe, with:
 	 *

lib/Service/LegacyFileMigrationService.php

@@ -0,0 +1,54 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OCA\ExeLearning\Service;
+
+use OCP\Files\File;
+use OCP\Files\InvalidPathException;
+use OCP\Files\NotFoundException;
+use OCP\Files\NotPermittedException;
+
+/**
+ * Best-effort migration from the legacy .elp extension to .elpx after save.
+ */
+class LegacyFileMigrationService {
+	public function __construct(
+		private readonly ElpxPackageService $packageService,
+	) {
+	}
+
+	public function migrate(File $file, string $userId, int $fileId): File {
+		$name = $file->getName();
+		if (!str_ends_with(strtolower($name), '.elp') || str_ends_with(strtolower($name), '.elpx')) {
+			return $file;
+		}
+
+		$base = substr($name, 0, -4);
+		try {
+			$parent = $file->getParent();
+		} catch (NotFoundException|NotPermittedException) {
+			return $file;
+		}
+
+		$candidate = $base . '.elpx';
+		for ($i = 2; $i < 100 && $parent->nodeExists($candidate); $i++) {
+			$candidate = sprintf('%s (%d).elpx', $base, $i);
+		}
+		if ($parent->nodeExists($candidate)) {
+			return $file;
+		}
+
+		try {
+			$file->move($parent->getPath() . '/' . $candidate);
+		} catch (NotPermittedException|InvalidPathException) {
+			return $file;
+		}
+
+		try {
+			return $this->packageService->getForUserById($userId, $fileId);
+		} catch (NotFoundException|NotPermittedException) {
+			return $file;
+		}
+	}
+}

tests/Unit/Service/LegacyFileMigrationServiceTest.php

@@ -0,0 +1,138 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OCA\ExeLearning\Tests\Unit\Service;
+
+use OCA\ExeLearning\Service\ElpxPackageService;
+use OCA\ExeLearning\Service\LegacyFileMigrationService;
+use OCP\Files\File;
+use OCP\Files\Folder;
+use OCP\Files\InvalidPathException;
+use OCP\Files\NotFoundException;
+use OCP\Files\NotPermittedException;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+final class LegacyFileMigrationServiceTest extends TestCase {
+	private ElpxPackageService $packages;
+	private LegacyFileMigrationService $service;
+
+	protected function setUp(): void {
+		$this->packages = $this->createMock(ElpxPackageService::class);
+		$this->service = new LegacyFileMigrationService($this->packages);
+	}
+
+	/**
+	 * @return iterable<string, array{string}>
+	 */
+	public static function nonLegacyNames(): iterable {
+		yield 'modern extension' => ['lesson.elpx'];
+		yield 'modern uppercase extension' => ['lesson.ELPX'];
+		yield 'unrelated archive' => ['lesson.zip'];
+	}
+
+	#[DataProvider('nonLegacyNames')]
+	public function testLeavesNonLegacyNamesUntouched(string $name): void {
+		$file = $this->file($name);
+		$file->expects(self::never())->method('getParent');
+
+		self::assertSame($file, $this->service->migrate($file, 'alice', 42));
+	}
+
+	public function testLeavesFileUntouchedWhenParentCannotBeResolved(): void {
+		$file = $this->file('Lesson.elp');
+		$file->method('getParent')->willThrowException(new NotFoundException('gone'));
+
+		self::assertSame($file, $this->service->migrate($file, 'alice', 42));
+	}
+
+	public function testMigratesToModernExtensionAndRefetchesFile(): void {
+		$parent = $this->parent('/Lessons');
+		$parent->method('nodeExists')->with('Lesson.elpx')->willReturn(false);
+		$file = $this->file('Lesson.elp');
+		$file->method('getParent')->willReturn($parent);
+		$file->expects(self::once())->method('move')->with('/Lessons/Lesson.elpx');
+		$renamed = $this->file('Lesson.elpx');
+		$this->packages->expects(self::once())
+			->method('getForUserById')
+			->with('alice', 42)
+			->willReturn($renamed);
+
+		self::assertSame($renamed, $this->service->migrate($file, 'alice', 42));
+	}
+
+	public function testChoosesFirstAvailableCollisionSuffix(): void {
+		$parent = $this->parent('/Lessons');
+		$parent->method('nodeExists')->willReturnCallback(
+			static fn (string $name): bool => in_array($name, ['Lesson.elpx', 'Lesson (2).elpx'], true),
+		);
+		$file = $this->file('Lesson.elp');
+		$file->method('getParent')->willReturn($parent);
+		$file->expects(self::once())->method('move')->with('/Lessons/Lesson (3).elpx');
+		$this->packages->method('getForUserById')->willReturn($file);
+
+		self::assertSame($file, $this->service->migrate($file, 'alice', 42));
+	}
+
+	public function testStopsWhenEveryCandidateIsOccupied(): void {
+		$parent = $this->parent('/Lessons');
+		$parent->method('nodeExists')->willReturn(true);
+		$file = $this->file('Lesson.elp');
+		$file->method('getParent')->willReturn($parent);
+		$file->expects(self::never())->method('move');
+
+		self::assertSame($file, $this->service->migrate($file, 'alice', 42));
+	}
+
+	/**
+	 * @return iterable<string, array{\Throwable}>
+	 */
+	public static function moveFailures(): iterable {
+		yield 'permission denied' => [new NotPermittedException('denied')];
+		yield 'invalid path' => [new InvalidPathException('invalid')];
+	}
+
+	#[DataProvider('moveFailures')]
+	public function testMoveFailuresAreBestEffort(\Throwable $error): void {
+		$parent = $this->parent('/Lessons');
+		$parent->method('nodeExists')->willReturn(false);
+		$file = $this->file('Lesson.elp');
+		$file->method('getParent')->willReturn($parent);
+		$file->method('move')->willThrowException($error);
+		$this->packages->expects(self::never())->method('getForUserById');
+
+		self::assertSame($file, $this->service->migrate($file, 'alice', 42));
+	}
+
+	/**
+	 * @return iterable<string, array{\Throwable}>
+	 */
+	public static function refetchFailures(): iterable {
+		yield 'not found after move' => [new NotFoundException('gone')];
+		yield 'not permitted after move' => [new NotPermittedException('denied')];
+	}
+
+	#[DataProvider('refetchFailures')]
+	public function testRefetchFailuresReturnOriginalHandle(\Throwable $error): void {
+		$parent = $this->parent('/Lessons');
+		$parent->method('nodeExists')->willReturn(false);
+		$file = $this->file('Lesson.elp');
+		$file->method('getParent')->willReturn($parent);
+		$this->packages->method('getForUserById')->willThrowException($error);
+
+		self::assertSame($file, $this->service->migrate($file, 'alice', 42));
+	}
+
+	private function file(string $name): File {
+		$file = $this->createMock(File::class);
+		$file->method('getName')->willReturn($name);
+		return $file;
+	}
+
+	private function parent(string $path): Folder {
+		$parent = $this->createMock(Folder::class);
+		$parent->method('getPath')->willReturn($path);
+		return $parent;
+	}
+}

tests/bootstrap-standalone.php

@@ -179,6 +179,9 @@ public function getPath();
 			public function getMTime();
 			public function getEtag();
 			public function isUpdateable();
+			public function getParent();
+			public function move($target);
+			public function putContent($data);
 		}
 	');
 }
@@ -188,6 +191,8 @@ public function isUpdateable();
 		interface Folder extends Node {
 			public function getById($fileId);
 			public function get($path);
+			public function nodeExists($path);
+			public function getPath();
 		}
 	');
 }
@@ -205,6 +210,9 @@ public function getUserFolder($userId);
 if (!class_exists('OCP\\Files\\NotPermittedException', false)) {
 	eval('namespace OCP\\Files; class NotPermittedException extends \\Exception {}');
 }
+if (!class_exists('OCP\\Files\\InvalidPathException', false)) {
+	eval('namespace OCP\\Files; class InvalidPathException extends \\Exception {}');
+}
 if (!class_exists('OCP\\Constants', false)) {
 	eval('namespace OCP; class Constants { public const PERMISSION_READ = 1; }');
 }