From a3058da5defd20eb34c65a799e03367e005c0bcb Mon Sep 17 00:00:00 2001 From: Ernesto Serrano Date: Tue, 22 Sep 2026 05:14:37 +0100 Subject: [PATCH 1/4] test: extend file stubs for legacy migration --- tests/bootstrap-standalone.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/bootstrap-standalone.php b/tests/bootstrap-standalone.php index a5deb62..f5f5077 100644 --- a/tests/bootstrap-standalone.php +++ b/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; }'); } From 32ebd5211ebb3272e3fb8beb53201ee62142fd1e Mon Sep 17 00:00:00 2001 From: Ernesto Serrano Date: Tue, 22 Sep 2026 05:14:48 +0100 Subject: [PATCH 2/4] refactor: extract legacy extension migration service --- lib/Service/LegacyFileMigrationService.php | 54 ++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 lib/Service/LegacyFileMigrationService.php diff --git a/lib/Service/LegacyFileMigrationService.php b/lib/Service/LegacyFileMigrationService.php new file mode 100644 index 0000000..12c239c --- /dev/null +++ b/lib/Service/LegacyFileMigrationService.php @@ -0,0 +1,54 @@ +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; + } + } +} From d7a68f76ebe97bbc1442b481974d4c4468f49f59 Mon Sep 17 00:00:00 2001 From: Ernesto Serrano Date: Tue, 22 Sep 2026 05:14:51 +0100 Subject: [PATCH 3/4] refactor: delegate legacy file migration --- lib/Controller/EditorController.php | 46 ++--------------------------- 1 file changed, 3 insertions(+), 43 deletions(-) diff --git a/lib/Controller/EditorController.php b/lib/Controller/EditorController.php index 30e4879..87dadbd 100644 --- a/lib/Controller/EditorController.php +++ b/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 - * `.elpx`, falling back to ` (2).elpx`, ` (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: * From 6422b8bf45f5b1a165c8e97e3bd0f338e562811c Mon Sep 17 00:00:00 2001 From: Ernesto Serrano Date: Tue, 22 Sep 2026 05:15:14 +0100 Subject: [PATCH 4/4] test: cover legacy extension migration --- .../LegacyFileMigrationServiceTest.php | 138 ++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 tests/Unit/Service/LegacyFileMigrationServiceTest.php diff --git a/tests/Unit/Service/LegacyFileMigrationServiceTest.php b/tests/Unit/Service/LegacyFileMigrationServiceTest.php new file mode 100644 index 0000000..a1eefda --- /dev/null +++ b/tests/Unit/Service/LegacyFileMigrationServiceTest.php @@ -0,0 +1,138 @@ +packages = $this->createMock(ElpxPackageService::class); + $this->service = new LegacyFileMigrationService($this->packages); + } + + /** + * @return iterable + */ + 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 + */ + 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 + */ + 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; + } +}