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