fix(sharereview): skip context navigation cleanup for non-context shares

Calling contextNavigationMapper::deleteByShareId() for every share
deletion would be misleading since context navigation entries only exist
for context-type shares. Fetching the share entity first lets us guard
the cleanup call behind a node-type check, so it only runs when the
deleted share actually belongs to a context. This removes the confusing
and potentially spammy error log path that could appear for ordinary
table and view share deletions.

Assisted-by: ClaudeCode:claude-sonnet-4-6
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

Author: AndyScherzinger

lib/ShareReview/ShareReviewSource.php

@@ -15,6 +15,8 @@
 use OCA\Tables\Db\ShareMapper;
 use OCA\Tables\Db\TableMapper;
 use OCA\Tables\Db\ViewMapper;
+use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\AppFramework\Db\MultipleObjectsReturnedException;
 use OCP\Constants;
 use OCP\DB\Exception;
 use OCP\IL10N;
@@ -87,22 +89,30 @@ public function getShares(): array {
 
 	public function deleteShare(string $shareId): bool {
 		$this->logger->info('Tables ShareReview: deleting share {id}', ['id' => $shareId]);
-		try {
-			$deleted = $this->shareMapper->deleteById((int)$shareId);
-		} catch (Exception $e) {
-			$this->logger->error('Tables ShareReview: failed to delete share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
-			return false;
-		}
 
-		if (!$deleted) {
+		try {
+			$share = $this->shareMapper->find((int)$shareId);
+		} catch (DoesNotExistException) {
 			$this->logger->warning('Tables ShareReview: share {id} not found', ['id' => $shareId]);
 			return false;
+		} catch (MultipleObjectsReturnedException|Exception $e) {
+			$this->logger->error('Tables ShareReview: failed to find share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
+			return false;
 		}
 
 		try {
-			$this->contextNavigationMapper->deleteByShareId((int)$shareId);
+			$this->shareMapper->delete($share);
 		} catch (Exception $e) {
-			$this->logger->error('Tables ShareReview: failed to clean up context navigation for share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
+			$this->logger->error('Tables ShareReview: failed to delete share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
+			return false;
+		}
+
+		if ($share->getNodeType() === self::NODE_TYPE_CONTEXT) {
+			try {
+				$this->contextNavigationMapper->deleteByShareId((int)$shareId);
+			} catch (Exception $e) {
+				$this->logger->error('Tables ShareReview: failed to clean up context navigation for share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
+			}
 		}
 		return true;
 	}

tests/unit/ShareReview/ShareReviewSourceTest.php

@@ -11,10 +11,12 @@
 
 use OCA\Tables\Db\ContextMapper;
 use OCA\Tables\Db\ContextNavigationMapper;
+use OCA\Tables\Db\Share;
 use OCA\Tables\Db\ShareMapper;
 use OCA\Tables\Db\TableMapper;
 use OCA\Tables\Db\ViewMapper;
 use OCA\Tables\ShareReview\ShareReviewSource;
+use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\Constants;
 use OCP\DB\Exception;
 use OCP\IL10N;
@@ -225,16 +227,36 @@ public function testGetSharesReturnsEmptyOnDbException(): void {
 		$this->assertSame([], $this->source->getShares());
 	}
 
-	public function testDeleteShareSuccess(): void {
-		$this->shareMapper->expects($this->once())->method('deleteById')->with(7)->willReturn(true);
+	private function makeShare(int $id, string $nodeType): Share {
+		$share = new Share();
+		$share->setId($id);
+		$share->setNodeType($nodeType);
+		return $share;
+	}
+
+	public function testDeleteShareTableSuccessSkipsContextNavCleanup(): void {
+		$share = $this->makeShare(7, 'table');
+		$this->shareMapper->expects($this->once())->method('find')->with(7)->willReturn($share);
+		$this->shareMapper->expects($this->once())->method('delete')->with($share);
+		$this->contextNavigationMapper->expects($this->never())->method('deleteByShareId');
+		$this->logger->expects($this->once())->method('info');
+
+		$this->assertTrue($this->source->deleteShare('7'));
+	}
+
+	public function testDeleteShareContextSuccessCleansUpContextNav(): void {
+		$share = $this->makeShare(7, 'context');
+		$this->shareMapper->expects($this->once())->method('find')->with(7)->willReturn($share);
+		$this->shareMapper->expects($this->once())->method('delete')->with($share);
 		$this->contextNavigationMapper->expects($this->once())->method('deleteByShareId')->with(7);
 		$this->logger->expects($this->once())->method('info');
 
 		$this->assertTrue($this->source->deleteShare('7'));
 	}
 
 	public function testDeleteShareNotFoundReturnsFalse(): void {
-		$this->shareMapper->method('deleteById')->willReturn(false);
+		$this->shareMapper->method('find')->willThrowException(new DoesNotExistException('not found'));
+		$this->shareMapper->expects($this->never())->method('delete');
 		$this->contextNavigationMapper->expects($this->never())->method('deleteByShareId');
 		$this->logger->expects($this->once())->method('info');
 		$this->logger->expects($this->once())->method('warning');
@@ -243,7 +265,9 @@ public function testDeleteShareNotFoundReturnsFalse(): void {
 	}
 
 	public function testDeleteShareReturnsFalseOnDeleteException(): void {
-		$this->shareMapper->method('deleteById')->willThrowException($this->createMock(Exception::class));
+		$share = $this->makeShare(7, 'table');
+		$this->shareMapper->method('find')->willReturn($share);
+		$this->shareMapper->method('delete')->willThrowException($this->createMock(Exception::class));
 		$this->contextNavigationMapper->expects($this->never())->method('deleteByShareId');
 		$this->logger->expects($this->once())->method('info');
 		$this->logger->expects($this->once())->method('error');
@@ -252,7 +276,9 @@ public function testDeleteShareReturnsFalseOnDeleteException(): void {
 	}
 
 	public function testDeleteShareReturnsTrueOnContextNavigationException(): void {
-		$this->shareMapper->method('deleteById')->willReturn(true);
+		$share = $this->makeShare(42, 'context');
+		$this->shareMapper->method('find')->willReturn($share);
+		$this->shareMapper->method('delete');
 		$this->contextNavigationMapper->method('deleteByShareId')->willThrowException($this->createMock(Exception::class));
 		$this->logger->expects($this->once())->method('info');
 		$this->logger->expects($this->once())->method('error');