Node update permissions adjustments

* Create dedicated private function for determining new file name * Adjust and add tests * Update Composer Deps
R0Wi-DEV · Jan 15, 2025 · 231d1cf · 231d1cf
1 parent 74dcd3d
commit 231d1cf
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 110 deletions.
diff --git a/composer.lock b/composer.lock
diff --git a/lib/Service/OcrService.php b/lib/Service/OcrService.php
@@ -245,10 +245,9 @@ private function processTagsAfterSuccessfulOcr(File $file, WorkflowSettings $set
 	/**
 	 * @param string $filePath The filepath of the file to write
 	 * @param string $ocrContent The new filecontent (which was OCR processed)
-	 * @param int $fileId The id of the file to write. Used for locking.
 	 * @param int $fileMtime The mtime of the new file. Can be used to restore the original modification time of the non-OCR file.
 	 */
-	private function createNewFileVersion(string $filePath, string $ocrContent, int $fileId, ?int $fileMtime = null) : void {
+	private function createNewFileVersion(string $filePath, string $ocrContent, ?int $fileMtime = null) : void {
 		$dirPath = dirname($filePath);
 		$filename = basename($filePath);
 
@@ -307,7 +306,6 @@ private function setFileVersionsLabel(File $file, string $uid, string $label): v
 	private function doPostProcessing(Node $file, string $uid, WorkflowSettings $settings, OcrProcessorResult $result, ?int $fileMtime = null): void {
 		$this->processTagsAfterSuccessfulOcr($file, $settings);
 
-		$filePath = $file->getPath();
 		$fileId = $file->getId();
 		$fileContent = $result->getFileContent();
 		$originalFileExtension = $file->getExtension();
@@ -320,20 +318,8 @@ private function doPostProcessing(Node $file, string $uid, WorkflowSettings $set
 				$this->setFileVersionsLabel($file, $uid, self::FILE_VERSION_LABEL_VALUE);
 			}
 
-			if ($originalFileExtension === $newFileExtension) {
-				if (!$file->isUpdateable()) {
-					// Add suffix '_OCR' if original file cannot be updated
-					$fileInfo = pathinfo($filePath);
-					$newFilePath = $fileInfo['dirname'] . '/' . $fileInfo['filename'] . '_OCR.' . $originalFileExtension;
-				} else {
-					$newFilePath = $filePath;
-				}
-			} else {
-				$newFilePath = $filePath . '.pdf';
-			}
-
-
-			$this->createNewFileVersion($newFilePath, $fileContent, $fileId, $fileMtime);
+			$newFilePath = $this->determineNewFilePath($file, $originalFileExtension, $newFileExtension);
+			$this->createNewFileVersion($newFilePath, $fileContent, $fileMtime);
 		}
 
 		$this->eventService->textRecognized($result, $file);
@@ -342,4 +328,28 @@ private function doPostProcessing(Node $file, string $uid, WorkflowSettings $set
 			$this->notificationService->createSuccessNotification($uid, $fileId);
 		}
 	}
+
+	/**
+	 * Determines the new file path for a given file by analyzing the original- and new file extension.
+	 * Also takes into consideration, if the file can be updated by the current user.
+	 *
+	 * @param Node $file The original file node for which the OCR processing has been succeeded.
+	 * @param string $originalFileExtension The original file extension.
+	 * @param string $newFileExtension The new file extension to be applied.
+	 * @return string The new file path with the updated extension.
+	 */
+	private function determineNewFilePath(Node $file, string $originalFileExtension, string $newFileExtension): string {
+		$filePath = $file->getPath();
+		if ($originalFileExtension !== $newFileExtension) {
+			// If the extension changed, will create a new file with the new extension
+			return $filePath . '.' . $newFileExtension;
+		}
+		if (!$file->isUpdateable()) {
+			// Add suffix '_OCR' if original file cannot be updated
+			$fileInfo = pathinfo($filePath);
+			return $fileInfo['dirname'] . '/' . $fileInfo['filename'] . '_OCR.' . $newFileExtension;
+		}
+		// By returning the original file path, we will create a new file version of the original file
+		return $filePath;
+	}
 }
diff --git a/tests/Unit/OperationTest.php b/tests/Unit/OperationTest.php
@@ -26,6 +26,7 @@
 use OCA\WorkflowEngine\Entity\File;
 use OCA\WorkflowOcr\BackgroundJobs\ProcessFileJob;
 use OCA\WorkflowOcr\Helper\IProcessingFileAccessor;
+use OCA\WorkflowOcr\Helper\ProcessingFileAccessor;
 use OCA\WorkflowOcr\Operation;
 use OCP\BackgroundJob\IJobList;
 use OCP\EventDispatcher\Event;
@@ -36,7 +37,6 @@
 use OCP\Files\Node;
 use OCP\IL10N;
 use OCP\IURLGenerator;
-use OCP\IUser;
 use OCP\SystemTag\MapperEvent;
 use OCP\WorkflowEngine\IManager;
 use OCP\WorkflowEngine\IRuleMatcher;
@@ -70,7 +70,7 @@ protected function setUp(): void {
 		$this->l = $this->createMock(IL10N::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
-		$this->processingFileAccessor = $this->createMock(IProcessingFileAccessor::class);
+		$this->processingFileAccessor = ProcessingFileAccessor::getInstance();
 		$this->ruleMatcher = $this->createMock(IRuleMatcher::class);
 		$this->rootFolder = $this->createMock(IRootFolder::class);
 
@@ -79,6 +79,10 @@ protected function setUp(): void {
 			->willReturn($match); // simulate single matching operation
 	}
 
+	protected function tearDown(): void {
+		$this->processingFileAccessor->setCurrentlyProcessedFilePath(null);
+	}
+
 	public function testDoesNothingIfRuleMatcherDoesNotMatch() {
 		$this->jobList->expects($this->never())
 			->method('add')
@@ -120,7 +124,7 @@ public function testDoesNothingOnFolderEvent() {
 	}
 
 	public function testDoesNothingOnPostWriteTriggeredByCurrentOcrProcess() {
-		$fileId = 42;
+		$filePath = '/user/files/somefile.pdf';
 
 		$this->jobList->expects($this->never())
 			->method('add')
@@ -129,26 +133,16 @@ public function testDoesNothingOnPostWriteTriggeredByCurrentOcrProcess() {
 			->method('debug')
 			->withAnyParameters();
 
-		$this->processingFileAccessor->expects($this->once())
-			->method('getCurrentlyProcessedFileId')
-			->willReturn($fileId);
-
+		$this->processingFileAccessor->setCurrentlyProcessedFilePath($filePath);
 		$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder);
 
-		/** @var MockObject|IUser */
-		$userMock = $this->createMock(IUser::class);
-		$userMock->expects($this->never())
-			->method('getUID');
 		/** @var MockObject|Node */
 		$fileMock = $this->createMock(Node::class);
 		$fileMock->method('getType')
 			->willReturn(FileInfo::TYPE_FILE);
-		$fileMock->method('getPath')
-			->willReturn('/someuser/files/somefile.pdf');
-		$fileMock->method('getOwner')
-			->willReturn($userMock);
-		$fileMock->method('getId')
-			->willReturn($fileId);
+		$fileMock->expects($this->never())->method('getOwner');
+		$fileMock->expects($this->atLeastOnce())->method('getPath')
+			->willReturn($filePath);
 		$event = new GenericEvent($fileMock);
 		$eventName = '\OCP\Files::postCreate';
 
@@ -183,31 +177,6 @@ public function testDoesNothingOnInvalidFilePath(string $filePath) {
 		$operation->onEvent($eventName, $event, $this->ruleMatcher);
 	}
 
-	public function testDoesNothingOnFileWithoutOwner() {
-		$this->jobList->expects($this->never())
-			->method('add')
-			->withAnyParameters();
-		$this->logger->expects($this->atLeastOnce())
-			->method('debug')
-			->withAnyParameters();
-
-		$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder);
-
-		/** @var MockObject|Node */
-		$fileMock = $this->createMock(Node::class);
-		$fileMock->method('getType')
-			->willReturn(FileInfo::TYPE_FILE);
-		$fileMock->method('getPath')
-			->willReturn('/admin/files/path/to/file.pdf');
-		$fileMock->method('getOwner')
-			->willReturn(null);
-
-		$event = new GenericEvent($fileMock);
-		$eventName = '\OCP\Files::postCreate';
-
-		$operation->onEvent($eventName, $event, $this->ruleMatcher);
-	}
-
 	public function testAddWithCorrectFilePathAndUser() {
 		$filePath = '/admin/files/path/to/file.pdf';
 		$fileId = 42;
@@ -218,19 +187,13 @@ public function testAddWithCorrectFilePathAndUser() {
 
 		$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder);
 
-		/** @var MockObject|IUser */
-		$userMock = $this->createMock(IUser::class);
-		$userMock->expects($this->never())
-			->method('getUID')
-			->willReturn($uid);
 		/** @var MockObject|Node */
 		$fileMock = $this->createMock(Node::class);
 		$fileMock->method('getType')
 			->willReturn(FileInfo::TYPE_FILE);
 		$fileMock->method('getPath')
 			->willReturn($filePath);
-		$fileMock->method('getOwner')
-			->willReturn($userMock);
+		$fileMock->expects($this->never())->method('getOwner');
 		$fileMock->method('getId')
 			->willReturn($fileId);
 		$event = new GenericEvent($fileMock);
@@ -406,19 +369,13 @@ public function testFileAddedToQueueOnTagAssignedEvent() {
 			->method('add')
 			->with(ProcessFileJob::class, ['fileId' => $fileId, 'uid' => $uid, 'settings' => self::SETTINGS]);
 
-		/** @var MockObject|IUser */
-		$userMock = $this->createMock(IUser::class);
-		$userMock->expects($this->never())
-			->method('getUID')
-			->willReturn($uid);
 		/** @var MockObject|\OCP\Files\File */
 		$fileMock = $this->createMock(\OCP\Files\File::class);
 		$fileMock->method('getType')
 			->willReturn(FileInfo::TYPE_FILE);
 		$fileMock->method('getPath')
 			->willReturn($filePath);
-		$fileMock->method('getOwner')
-			->willReturn($userMock);
+		$fileMock->expects($this->never())->method('getOwner');
 		$fileMock->method('getId')
 			->willReturn($fileId);
 		/** @var MockObject|IRootFolder */
@@ -446,19 +403,13 @@ public function testAddsEventOnEventWhereSubjectIsArray(string $eventName) {
 
 		$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder);
 
-		/** @var MockObject|IUser */
-		$userMock = $this->createMock(IUser::class);
-		$userMock->expects($this->never())
-			->method('getUID')
-			->willReturn($uid);
 		/** @var MockObject|Node */
 		$fileMockSecondFile = $this->createMock(Node::class);
 		$fileMockSecondFile->method('getType')
 			->willReturn(FileInfo::TYPE_FILE);
 		$fileMockSecondFile->method('getPath')
 			->willReturn($filePath);
-		$fileMockSecondFile->method('getOwner')
-			->willReturn($userMock);
+		$fileMockSecondFile->expects($this->never())->method('getOwner');
 		$fileMockSecondFile->method('getId')
 			->willReturn($fileId);
 		/** @var MockObject|Node */