From 65a7b27fc90574e95d3724377509b39f9fe4a1e0 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 7 Jun 2026 14:41:39 +0200 Subject: [PATCH 01/19] feat(sharereview): add listener registering Tables as a share source Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewListener.php | 27 +++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 lib/ShareReview/ShareReviewListener.php diff --git a/lib/ShareReview/ShareReviewListener.php b/lib/ShareReview/ShareReviewListener.php new file mode 100644 index 0000000000..f8f8c8c0d3 --- /dev/null +++ b/lib/ShareReview/ShareReviewListener.php @@ -0,0 +1,27 @@ + */ +class ShareReviewListener implements IEventListener { + public function __construct() { + } + + public function handle(Event $event): void { + if (!$event instanceof SourceEvent) { + return; + } + $event->registerSource(ShareReviewSource::class); + } +} From 7e23687aae76ae9b9ee6b6ecb0f7658e460115cd Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 7 Jun 2026 14:42:51 +0200 Subject: [PATCH 02/19] feat(sharereview): add ShareReviewSource with constructor and getName() Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewSource.php | 46 +++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 lib/ShareReview/ShareReviewSource.php diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php new file mode 100644 index 0000000000..b42dc6f7e1 --- /dev/null +++ b/lib/ShareReview/ShareReviewSource.php @@ -0,0 +1,46 @@ + Date: Sun, 7 Jun 2026 14:44:09 +0200 Subject: [PATCH 03/19] feat(sharereview): implement getShares() with batched name lookups Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewSource.php | 126 +++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-) diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index b42dc6f7e1..1f469f802f 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -10,7 +10,10 @@ namespace OCA\Tables\ShareReview; use OCA\ShareReview\Sources\ISource; +use OCP\Constants; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; +use OCP\Share\IShare; use Psr\Log\LoggerInterface; class ShareReviewSource implements ISource { @@ -36,11 +39,132 @@ public function getName(): string { return 'Tables'; } + /** + * @return list + */ public function getShares(): array { - return []; + $rawShares = $this->fetchAllShares(); + + $tableNames = $this->fetchNames(self::TABLES_TABLE, 'title', $rawShares, self::NODE_TYPE_TABLE); + $viewNames = $this->fetchNames(self::VIEWS_TABLE, 'title', $rawShares, self::NODE_TYPE_VIEW); + $contextNames = $this->fetchNames(self::CONTEXTS_TABLE, 'name', $rawShares, self::NODE_TYPE_CONTEXT); + + $formatted = []; + foreach ($rawShares as $share) { + $formatted[] = [ + 'id' => (int)$share['id'], + 'app' => $this->getName(), + 'object' => $this->resolveObjectName($share, $tableNames, $viewNames, $contextNames), + 'initiator' => (string)$share['sender'], + 'type' => $this->mapReceiverType((string)$share['receiver_type']), + 'recipient' => $share['receiver_type'] === self::RECEIVER_TYPE_LINK + ? (string)$share['token'] + : (string)$share['receiver'], + 'permissions' => $this->computePermissions($share), + 'password' => $share['password'] !== null, + 'time' => (string)($share['created_at'] ?? '1970-01-01 01:00:00'), + 'action' => '', + ]; + } + return $formatted; } public function deleteShare($shareId): bool { return false; } + + /** @return list> */ + private function fetchAllShares(): array { + $qb = $this->db->getQueryBuilder(); + $qb->select( + 'id', 'sender', 'receiver', 'receiver_type', 'node_id', 'node_type', + 'token', 'password', + 'permission_read', 'permission_create', 'permission_update', + 'permission_delete', 'permission_manage', + 'created_at' + )->from(self::SHARE_TABLE) + ->orderBy('id', 'ASC'); + $result = $qb->executeQuery(); + $rows = $result->fetchAll(); + $result->closeCursor(); + return $rows; + } + + /** + * Batch-fetch node IDs → names for one entity type. + * + * @param list> $shares + * @return array + */ + private function fetchNames(string $table, string $nameColumn, array $shares, string $nodeType): array { + $ids = array_values(array_unique(array_map( + static fn (array $share): int => (int)$share['node_id'], + array_filter($shares, static fn (array $share): bool => $share['node_type'] === $nodeType) + ))); + + if ($ids === []) { + return []; + } + + $qb = $this->db->getQueryBuilder(); + $qb->select('id', $nameColumn) + ->from($table) + ->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); + $result = $qb->executeQuery(); + $rows = $result->fetchAll(); + $result->closeCursor(); + + $map = []; + foreach ($rows as $row) { + $map[(int)$row['id']] = (string)$row[$nameColumn]; + } + return $map; + } + + /** + * @param array $share + * @param array $tableNames + * @param array $viewNames + * @param array $contextNames + */ + private function resolveObjectName(array $share, array $tableNames, array $viewNames, array $contextNames): string { + $nodeId = (int)$share['node_id']; + return match($share['node_type']) { + self::NODE_TYPE_TABLE => ($tableNames[$nodeId] ?? "Table $nodeId") . ' (Table)', + self::NODE_TYPE_VIEW => ($viewNames[$nodeId] ?? "View $nodeId") . ' (View)', + self::NODE_TYPE_CONTEXT => ($contextNames[$nodeId] ?? "Context $nodeId") . ' (Context)', + default => "Unknown $nodeId", + }; + } + + private function mapReceiverType(string $receiverType): int { + return match($receiverType) { + 'user' => IShare::TYPE_USER, + 'group' => IShare::TYPE_GROUP, + 'link' => IShare::TYPE_LINK, + 'circle' => IShare::TYPE_CIRCLE, + default => IShare::TYPE_USER, + }; + } + + /** @param array $share */ + private function computePermissions(array $share): int { + $permissions = 0; + if ($share['permission_read']) { + $permissions |= Constants::PERMISSION_READ; + } + if ($share['permission_update']) { + $permissions |= Constants::PERMISSION_UPDATE; + } + if ($share['permission_create']) { + $permissions |= Constants::PERMISSION_CREATE; + } + if ($share['permission_delete']) { + $permissions |= Constants::PERMISSION_DELETE; + } + if ($share['permission_manage']) { + $permissions |= Constants::PERMISSION_SHARE; + } + return $permissions > 0 ? $permissions : Constants::PERMISSION_READ; + } } From 55ca8a73e8ef5e56542cf32fbcd26fbf30da8a40 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 7 Jun 2026 14:45:09 +0200 Subject: [PATCH 04/19] feat(sharereview): implement deleteShare() via direct SQL with logging Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewSource.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index 1f469f802f..2860f1bbc4 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -70,7 +70,11 @@ public function getShares(): array { } public function deleteShare($shareId): bool { - return false; + $this->logger->info('Tables ShareReview: deleting share {id}', ['id' => $shareId]); + $qb = $this->db->getQueryBuilder(); + $qb->delete(self::SHARE_TABLE) + ->where($qb->expr()->eq('id', $qb->createNamedParameter((int)$shareId, IQueryBuilder::PARAM_INT))); + return $qb->executeStatement() > 0; } /** @return list> */ From e80072cd4f163e0d0d4c9eb9b506d25e6ad7415d Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 7 Jun 2026 14:45:57 +0200 Subject: [PATCH 05/19] feat(sharereview): register ShareReview listener on SourceEvent Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/AppInfo/Application.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 4c00c491df..9bb3333d62 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -9,12 +9,14 @@ use Exception; use OCA\Analytics\Datasource\DatasourceEvent; +use OCA\ShareReview\Sources\SourceEvent; use OCA\Tables\Capabilities; use OCA\Tables\Event\RowDeletedEvent; use OCA\Tables\Event\TableDeletedEvent; use OCA\Tables\Event\TableOwnershipTransferredEvent; use OCA\Tables\Event\ViewDeletedEvent; use OCA\Tables\Listener\AddMissingIndicesListener; +use OCA\Tables\ShareReview\ShareReviewListener; use OCA\Tables\Listener\AnalyticsDatasourceListener; use OCA\Tables\Listener\BeforeTemplateRenderedListener; use OCA\Tables\Listener\LoadAdditionalListener; @@ -79,6 +81,7 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(BeforeUserDeletedEvent::class, UserDeletedListener::class); $context->registerEventListener(DatasourceEvent::class, AnalyticsDatasourceListener::class); + $context->registerEventListener(SourceEvent::class, ShareReviewListener::class); $context->registerEventListener(RenderReferenceEvent::class, TablesReferenceListener::class); $context->registerEventListener(BeforeTemplateRenderedEvent::class, BeforeTemplateRenderedListener::class); $context->registerEventListener(LoadAdditionalScriptsEvent::class, LoadAdditionalListener::class); From 41bb345f42bfd16cf58547ccc51c1ab0c5afb9da Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 7 Jun 2026 14:51:56 +0200 Subject: [PATCH 06/19] style(sharereview): apply coding standards and Psalm fixes Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/AppInfo/Application.php | 2 +- lib/ShareReview/ShareReviewSource.php | 8 ++++---- tests/stub.phpstub | 12 ++++++++++++ 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 9bb3333d62..9b17681dd4 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -16,7 +16,6 @@ use OCA\Tables\Event\TableOwnershipTransferredEvent; use OCA\Tables\Event\ViewDeletedEvent; use OCA\Tables\Listener\AddMissingIndicesListener; -use OCA\Tables\ShareReview\ShareReviewListener; use OCA\Tables\Listener\AnalyticsDatasourceListener; use OCA\Tables\Listener\BeforeTemplateRenderedListener; use OCA\Tables\Listener\LoadAdditionalListener; @@ -33,6 +32,7 @@ use OCA\Tables\Search\SearchTablesProvider; use OCA\Tables\Service\Support\AuditLogServiceInterface; use OCA\Tables\Service\Support\DefaultAuditLogService; +use OCA\Tables\ShareReview\ShareReviewListener; use OCA\Tables\UserMigration\TablesMigrator; use OCP\AppFramework\App; use OCP\AppFramework\Bootstrap\IBootContext; diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index 2860f1bbc4..c7f40d649c 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -73,7 +73,7 @@ public function deleteShare($shareId): bool { $this->logger->info('Tables ShareReview: deleting share {id}', ['id' => $shareId]); $qb = $this->db->getQueryBuilder(); $qb->delete(self::SHARE_TABLE) - ->where($qb->expr()->eq('id', $qb->createNamedParameter((int)$shareId, IQueryBuilder::PARAM_INT))); + ->where($qb->expr()->eq('id', $qb->createNamedParameter((int)$shareId, IQueryBuilder::PARAM_INT))); return $qb->executeStatement() > 0; } @@ -87,7 +87,7 @@ private function fetchAllShares(): array { 'permission_delete', 'permission_manage', 'created_at' )->from(self::SHARE_TABLE) - ->orderBy('id', 'ASC'); + ->orderBy('id', 'ASC'); $result = $qb->executeQuery(); $rows = $result->fetchAll(); $result->closeCursor(); @@ -112,8 +112,8 @@ private function fetchNames(string $table, string $nameColumn, array $shares, st $qb = $this->db->getQueryBuilder(); $qb->select('id', $nameColumn) - ->from($table) - ->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); + ->from($table) + ->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); $result = $qb->executeQuery(); $rows = $result->fetchAll(); $result->closeCursor(); diff --git a/tests/stub.phpstub b/tests/stub.phpstub index 800bb6518f..c71cac53ed 100644 --- a/tests/stub.phpstub +++ b/tests/stub.phpstub @@ -15,6 +15,18 @@ namespace OCA\Analytics\Datasource { } } +namespace OCA\ShareReview\Sources { + class SourceEvent extends \OCP\EventDispatcher\Event { + abstract public function registerSource(string $source): void {} + } + + interface ISource { + public function getName(): string; + public function getShares(): array; + public function deleteShare($shareId): bool; + } +} + namespace OCA\Circles\Model { class Circle { abstract public function getSingleId(): string {} From d21d6f26321b0373fc9166d02d26693f911e002a Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 7 Jun 2026 14:57:08 +0200 Subject: [PATCH 07/19] test(sharereview): add unit tests for ShareReviewSource Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- .../ShareReview/ShareReviewSourceTest.php | 215 ++++++++++++++++++ 1 file changed, 215 insertions(+) create mode 100644 tests/unit/ShareReview/ShareReviewSourceTest.php diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php new file mode 100644 index 0000000000..383602df2d --- /dev/null +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -0,0 +1,215 @@ +db = $this->createMock(IDBConnection::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->source = new ShareReviewSource($this->db, $this->logger); + } + + private function makeResult(array $rows): MockObject { + $result = $this->createMock(IResult::class); + $result->method('fetchAll')->willReturn($rows); + $result->method('closeCursor')->willReturn(true); + return $result; + } + + private function makeQb(array $fetchRows = [], int $statementRows = 0): MockObject { + $expr = $this->createMock(IExpressionBuilder::class); + $expr->method('eq')->willReturn('1=1'); + $expr->method('in')->willReturn('1=1'); + + $qb = $this->createMock(IQueryBuilder::class); + $qb->method('select')->willReturnSelf(); + $qb->method('from')->willReturnSelf(); + $qb->method('where')->willReturnSelf(); + $qb->method('orderBy')->willReturnSelf(); + $qb->method('delete')->willReturnSelf(); + $qb->method('createNamedParameter')->willReturn('?'); + $qb->method('expr')->willReturn($expr); + $qb->method('executeQuery')->willReturn($this->makeResult($fetchRows)); + $qb->method('executeStatement')->willReturn($statementRows); + + return $qb; + } + + /** @param array $overrides */ + private function makeShareRow(array $overrides = []): array { + return array_merge([ + 'id' => 1, + 'sender' => 'alice', + 'receiver' => 'bob', + 'receiver_type' => 'user', + 'node_id' => 10, + 'node_type' => 'table', + 'token' => null, + 'password' => null, + 'permission_read' => 1, + 'permission_create' => 0, + 'permission_update' => 0, + 'permission_delete' => 0, + 'permission_manage' => 0, + 'created_at' => '2026-01-15 12:00:00', + ], $overrides); + } + + public function testGetSharesEmpty(): void { + $this->db->method('getQueryBuilder')->willReturn($this->makeQb()); + + $this->assertSame([], $this->source->getShares()); + } + + public function testGetSharesUserShare(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow()]), + $this->makeQb([['id' => 10, 'title' => 'My Table']]), + ); + + $shares = $this->source->getShares(); + + $this->assertCount(1, $shares); + $share = $shares[0]; + $this->assertSame(1, $share['id']); + $this->assertSame('Tables', $share['app']); + $this->assertSame('My Table (Table)', $share['object']); + $this->assertSame('alice', $share['initiator']); + $this->assertSame(IShare::TYPE_USER, $share['type']); + $this->assertSame('bob', $share['recipient']); + $this->assertSame(Constants::PERMISSION_READ, $share['permissions']); + $this->assertFalse($share['password']); + $this->assertSame('2026-01-15 12:00:00', $share['time']); + $this->assertSame('', $share['action']); + } + + public function testGetSharesLinkShare(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow(['receiver_type' => 'link', 'token' => 'abc123token', 'password' => 'hashed_pw'])]), + $this->makeQb([['id' => 10, 'title' => 'Shared Table']]), + ); + + $shares = $this->source->getShares(); + + $this->assertCount(1, $shares); + $this->assertSame(IShare::TYPE_LINK, $shares[0]['type']); + $this->assertSame('abc123token', $shares[0]['recipient']); + $this->assertTrue($shares[0]['password']); + } + + public function testGetSharesContextShare(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow(['node_type' => 'context', 'node_id' => 99])]), + $this->makeQb([['id' => 99, 'name' => 'My Context']]), + ); + + $shares = $this->source->getShares(); + + $this->assertCount(1, $shares); + $this->assertSame('My Context (Context)', $shares[0]['object']); + } + + public function testGetSharesMixedNodeTypes(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([ + $this->makeShareRow(['id' => 1, 'node_type' => 'table', 'node_id' => 10]), + $this->makeShareRow(['id' => 2, 'node_type' => 'view', 'node_id' => 20]), + $this->makeShareRow(['id' => 3, 'node_type' => 'context', 'node_id' => 30]), + ]), + $this->makeQb([['id' => 10, 'title' => 'Table One']]), + $this->makeQb([['id' => 20, 'title' => 'View One']]), + $this->makeQb([['id' => 30, 'name' => 'Context One']]), + ); + + $shares = $this->source->getShares(); + + $this->assertCount(3, $shares); + $this->assertSame('Table One (Table)', $shares[0]['object']); + $this->assertSame('View One (View)', $shares[1]['object']); + $this->assertSame('Context One (Context)', $shares[2]['object']); + } + + public function testGetSharesDeletedNode(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow(['node_type' => 'table', 'node_id' => 42])]), + $this->makeQb([]), + ); + + $shares = $this->source->getShares(); + + $this->assertCount(1, $shares); + $this->assertSame('Table 42 (Table)', $shares[0]['object']); + } + + public function testDeleteShareSuccess(): void { + $this->db->method('getQueryBuilder')->willReturn($this->makeQb([], 1)); + $this->logger->expects($this->once())->method('info'); + + $this->assertTrue($this->source->deleteShare(7)); + } + + public function testDeleteShareNotFound(): void { + $this->db->method('getQueryBuilder')->willReturn($this->makeQb([], 0)); + $this->logger->expects($this->once())->method('info'); + + $this->assertFalse($this->source->deleteShare(99)); + } + + public function testComputePermissionsAllFalse(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow([ + 'permission_read' => 0, + 'permission_create' => 0, + 'permission_update' => 0, + 'permission_delete' => 0, + 'permission_manage' => 0, + ])]), + $this->makeQb([['id' => 10, 'title' => 'T']]), + ); + + $shares = $this->source->getShares(); + + $this->assertSame(Constants::PERMISSION_READ, $shares[0]['permissions']); + } + + public function testComputePermissionsAllTrue(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow([ + 'permission_read' => 1, + 'permission_create' => 1, + 'permission_update' => 1, + 'permission_delete' => 1, + 'permission_manage' => 1, + ])]), + $this->makeQb([['id' => 10, 'title' => 'T']]), + ); + + $shares = $this->source->getShares(); + + $expected = Constants::PERMISSION_READ | Constants::PERMISSION_UPDATE | Constants::PERMISSION_CREATE | Constants::PERMISSION_DELETE | Constants::PERMISSION_SHARE; + $this->assertSame($expected, $shares[0]['permissions']); + } +} From 1032d1e49e86f0551862fddf2b92a0e049ffbf50 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 7 Jun 2026 15:14:45 +0200 Subject: [PATCH 08/19] fix(sharereview): harden and optimize implementation and testing Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewSource.php | 139 +++++++++++++----- tests/stub.phpstub | 2 +- .../ShareReview/ShareReviewSourceTest.php | 56 ++++++- tests/unit/ShareReview/Stubs.php | 22 +++ tests/unit/bootstrap.php | 4 + 5 files changed, 178 insertions(+), 45 deletions(-) create mode 100644 tests/unit/ShareReview/Stubs.php diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index c7f40d649c..11c7e2949c 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -11,6 +11,7 @@ use OCA\ShareReview\Sources\ISource; use OCP\Constants; +use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; use OCP\Share\IShare; @@ -22,6 +23,7 @@ class ShareReviewSource implements ISource { private const TABLES_TABLE = 'tables_tables'; private const VIEWS_TABLE = 'tables_views'; private const CONTEXTS_TABLE = 'tables_contexts_context'; + private const CONTEXTS_NAVIGATION_TABLE = 'tables_contexts_navigation'; private const NODE_TYPE_TABLE = 'table'; private const NODE_TYPE_VIEW = 'view'; @@ -29,6 +31,8 @@ class ShareReviewSource implements ISource { private const RECEIVER_TYPE_LINK = 'link'; + private const BATCH_SIZE = 997; + public function __construct( private IDBConnection $db, private LoggerInterface $logger, @@ -45,15 +49,17 @@ public function getName(): string { public function getShares(): array { $rawShares = $this->fetchAllShares(); - $tableNames = $this->fetchNames(self::TABLES_TABLE, 'title', $rawShares, self::NODE_TYPE_TABLE); - $viewNames = $this->fetchNames(self::VIEWS_TABLE, 'title', $rawShares, self::NODE_TYPE_VIEW); - $contextNames = $this->fetchNames(self::CONTEXTS_TABLE, 'name', $rawShares, self::NODE_TYPE_CONTEXT); + $idsByType = $this->groupIdsByNodeType($rawShares); + $tableNames = $this->fetchNames(self::TABLES_TABLE, 'title', $idsByType[self::NODE_TYPE_TABLE]); + $viewNames = $this->fetchNames(self::VIEWS_TABLE, 'title', $idsByType[self::NODE_TYPE_VIEW]); + $contextNames = $this->fetchNames(self::CONTEXTS_TABLE, 'name', $idsByType[self::NODE_TYPE_CONTEXT]); + $appName = $this->getName(); $formatted = []; foreach ($rawShares as $share) { $formatted[] = [ 'id' => (int)$share['id'], - 'app' => $this->getName(), + 'app' => $appName, 'object' => $this->resolveObjectName($share, $tableNames, $viewNames, $contextNames), 'initiator' => (string)$share['sender'], 'type' => $this->mapReceiverType((string)$share['receiver_type']), @@ -69,62 +75,117 @@ public function getShares(): array { return $formatted; } - public function deleteShare($shareId): bool { + public function deleteShare(string $shareId): bool { $this->logger->info('Tables ShareReview: deleting share {id}', ['id' => $shareId]); - $qb = $this->db->getQueryBuilder(); - $qb->delete(self::SHARE_TABLE) - ->where($qb->expr()->eq('id', $qb->createNamedParameter((int)$shareId, IQueryBuilder::PARAM_INT))); - return $qb->executeStatement() > 0; + try { + $qb = $this->db->getQueryBuilder(); + $qb->delete(self::SHARE_TABLE) + ->where($qb->expr()->eq('id', $qb->createNamedParameter((int)$shareId, IQueryBuilder::PARAM_INT))); + $deleted = $qb->executeStatement() > 0; + } catch (Exception $e) { + $this->logger->error('Tables ShareReview: failed to delete share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]); + return false; + } + + if ($deleted) { + $this->deleteContextNavigation((int)$shareId); + } + + return $deleted; } /** @return list> */ private function fetchAllShares(): array { - $qb = $this->db->getQueryBuilder(); - $qb->select( - 'id', 'sender', 'receiver', 'receiver_type', 'node_id', 'node_type', - 'token', 'password', - 'permission_read', 'permission_create', 'permission_update', - 'permission_delete', 'permission_manage', - 'created_at' - )->from(self::SHARE_TABLE) - ->orderBy('id', 'ASC'); - $result = $qb->executeQuery(); - $rows = $result->fetchAll(); - $result->closeCursor(); - return $rows; + try { + $qb = $this->db->getQueryBuilder(); + $qb->select( + 'id', 'sender', 'receiver', 'receiver_type', 'node_id', 'node_type', + 'token', 'password', + 'permission_read', 'permission_create', 'permission_update', + 'permission_delete', 'permission_manage', + 'created_at' + )->from(self::SHARE_TABLE) + ->orderBy('id', 'ASC'); + $result = $qb->executeQuery(); + $rows = $result->fetchAll(); + $result->closeCursor(); + return $rows; + } catch (Exception $e) { + $this->logger->error('Tables ShareReview: failed to fetch shares: {message}', ['message' => $e->getMessage()]); + return []; + } } /** - * Batch-fetch node IDs → names for one entity type. + * Group distinct node IDs by node type in a single pass over the share list. * * @param list> $shares - * @return array + * @return array{table: list, view: list, context: list} */ - private function fetchNames(string $table, string $nameColumn, array $shares, string $nodeType): array { - $ids = array_values(array_unique(array_map( - static fn (array $share): int => (int)$share['node_id'], - array_filter($shares, static fn (array $share): bool => $share['node_type'] === $nodeType) - ))); + private function groupIdsByNodeType(array $shares): array { + $tableIds = []; + $viewIds = []; + $contextIds = []; + foreach ($shares as $share) { + $nodeId = (int)$share['node_id']; + $type = (string)$share['node_type']; + if ($type === self::NODE_TYPE_TABLE) { + $tableIds[$nodeId] = true; + } elseif ($type === self::NODE_TYPE_VIEW) { + $viewIds[$nodeId] = true; + } elseif ($type === self::NODE_TYPE_CONTEXT) { + $contextIds[$nodeId] = true; + } + } + return [ + self::NODE_TYPE_TABLE => array_keys($tableIds), + self::NODE_TYPE_VIEW => array_keys($viewIds), + self::NODE_TYPE_CONTEXT => array_keys($contextIds), + ]; + } + /** + * Batch-fetch node IDs → names, chunked to stay within database IN-clause limits. + * + * @param list $ids + * @return array + */ + private function fetchNames(string $table, string $nameColumn, array $ids): array { if ($ids === []) { return []; } - $qb = $this->db->getQueryBuilder(); - $qb->select('id', $nameColumn) - ->from($table) - ->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); - $result = $qb->executeQuery(); - $rows = $result->fetchAll(); - $result->closeCursor(); - $map = []; - foreach ($rows as $row) { - $map[(int)$row['id']] = (string)$row[$nameColumn]; + foreach (array_chunk($ids, self::BATCH_SIZE) as $chunk) { + try { + $qb = $this->db->getQueryBuilder(); + $qb->select('id', $nameColumn) + ->from($table) + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); + $result = $qb->executeQuery(); + $rows = $result->fetchAll(); + $result->closeCursor(); + foreach ($rows as $row) { + $map[(int)$row['id']] = (string)$row[$nameColumn]; + } + } catch (Exception $e) { + $this->logger->error('Tables ShareReview: failed to fetch names from {table}: {message}', ['table' => $table, 'message' => $e->getMessage()]); + } } return $map; } + private function deleteContextNavigation(int $shareId): void { + try { + $qb = $this->db->getQueryBuilder(); + $qb->delete(self::CONTEXTS_NAVIGATION_TABLE) + ->where($qb->expr()->eq('share_id', $qb->createNamedParameter($shareId, IQueryBuilder::PARAM_INT))); + $qb->executeStatement(); + } catch (Exception $e) { + $this->logger->error('Tables ShareReview: failed to clean up context navigation for share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]); + } + } + /** * @param array $share * @param array $tableNames diff --git a/tests/stub.phpstub b/tests/stub.phpstub index c71cac53ed..a445f1fbf4 100644 --- a/tests/stub.phpstub +++ b/tests/stub.phpstub @@ -23,7 +23,7 @@ namespace OCA\ShareReview\Sources { interface ISource { public function getName(): string; public function getShares(): array; - public function deleteShare($shareId): bool; + public function deleteShare(string $shareId): bool; } } diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index 383602df2d..a8f3c9bc68 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -11,6 +11,7 @@ use OCA\Tables\ShareReview\ShareReviewSource; use OCP\Constants; +use OCP\DB\Exception; use OCP\DB\IResult; use OCP\DB\QueryBuilder\IExpressionBuilder; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -58,6 +59,20 @@ private function makeQb(array $fetchRows = [], int $statementRows = 0): MockObje return $qb; } + private function makeThrowingQb(): MockObject { + $qb = $this->createMock(IQueryBuilder::class); + $qb->method('select')->willReturnSelf(); + $qb->method('from')->willReturnSelf(); + $qb->method('where')->willReturnSelf(); + $qb->method('orderBy')->willReturnSelf(); + $qb->method('delete')->willReturnSelf(); + $qb->method('createNamedParameter')->willReturn('?'); + $qb->method('expr')->willReturn($this->createMock(IExpressionBuilder::class)); + $qb->method('executeQuery')->willThrowException($this->createMock(Exception::class)); + $qb->method('executeStatement')->willThrowException($this->createMock(Exception::class)); + return $qb; + } + /** @param array $overrides */ private function makeShareRow(array $overrides = []): array { return array_merge([ @@ -164,18 +179,49 @@ public function testGetSharesDeletedNode(): void { $this->assertSame('Table 42 (Table)', $shares[0]['object']); } + public function testGetSharesReturnsEmptyOnDbException(): void { + $this->db->method('getQueryBuilder')->willReturn($this->makeThrowingQb()); + $this->logger->expects($this->once())->method('error'); + + $this->assertSame([], $this->source->getShares()); + } + public function testDeleteShareSuccess(): void { - $this->db->method('getQueryBuilder')->willReturn($this->makeQb([], 1)); + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([], 1), + $this->makeQb([], 0), + ); + $this->logger->expects($this->once())->method('info'); + + $this->assertTrue($this->source->deleteShare('7')); + } + + public function testDeleteShareCleansContextNavigation(): void { + $shareDeleteQb = $this->makeQb([], 1); + $navDeleteQb = $this->makeQb([], 1); + + $this->db->expects($this->exactly(2)) + ->method('getQueryBuilder') + ->willReturnOnConsecutiveCalls($shareDeleteQb, $navDeleteQb); + + $this->source->deleteShare('42'); + } + + public function testDeleteShareNotFoundSkipsContextNavigation(): void { + $this->db->expects($this->once()) + ->method('getQueryBuilder') + ->willReturn($this->makeQb([], 0)); $this->logger->expects($this->once())->method('info'); - $this->assertTrue($this->source->deleteShare(7)); + $this->assertFalse($this->source->deleteShare('99')); } - public function testDeleteShareNotFound(): void { - $this->db->method('getQueryBuilder')->willReturn($this->makeQb([], 0)); + public function testDeleteShareReturnsFalseOnDbException(): void { + $this->db->method('getQueryBuilder')->willReturn($this->makeThrowingQb()); $this->logger->expects($this->once())->method('info'); + $this->logger->expects($this->once())->method('error'); - $this->assertFalse($this->source->deleteShare(99)); + $this->assertFalse($this->source->deleteShare('7')); } public function testComputePermissionsAllFalse(): void { diff --git a/tests/unit/ShareReview/Stubs.php b/tests/unit/ShareReview/Stubs.php new file mode 100644 index 0000000000..80ec8a5171 --- /dev/null +++ b/tests/unit/ShareReview/Stubs.php @@ -0,0 +1,22 @@ + Date: Mon, 15 Jun 2026 12:14:50 +0200 Subject: [PATCH 09/19] refactor(sharereview): replace raw SQL in deleteShare() with mapper layer Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/Db/ShareMapper.php | 10 ++++ lib/ShareReview/ShareReviewSource.php | 37 ++++++------ .../ShareReview/ShareReviewSourceTest.php | 56 ++++++++++--------- 3 files changed, 56 insertions(+), 47 deletions(-) diff --git a/lib/Db/ShareMapper.php b/lib/Db/ShareMapper.php index 72f05c7d8e..1b8bbddb6b 100644 --- a/lib/Db/ShareMapper.php +++ b/lib/Db/ShareMapper.php @@ -191,6 +191,16 @@ public function findAllSharesForNodeTo(string $nodeType, int $nodeId, string $re return $this->findEntities($qb); } + /** + * @throws Exception + */ + public function deleteById(int $id): bool { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->table) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT))); + return $qb->executeStatement() > 0; + } + /** * @param int $nodeId * @param string $nodeType diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index 11c7e2949c..b51ece1d44 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -10,6 +10,8 @@ namespace OCA\Tables\ShareReview; use OCA\ShareReview\Sources\ISource; +use OCA\Tables\Db\ContextNavigationMapper; +use OCA\Tables\Db\ShareMapper; use OCP\Constants; use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -23,7 +25,6 @@ class ShareReviewSource implements ISource { private const TABLES_TABLE = 'tables_tables'; private const VIEWS_TABLE = 'tables_views'; private const CONTEXTS_TABLE = 'tables_contexts_context'; - private const CONTEXTS_NAVIGATION_TABLE = 'tables_contexts_navigation'; private const NODE_TYPE_TABLE = 'table'; private const NODE_TYPE_VIEW = 'view'; @@ -31,10 +32,12 @@ class ShareReviewSource implements ISource { private const RECEIVER_TYPE_LINK = 'link'; - private const BATCH_SIZE = 997; + private const DB_CHUNK_SIZE = 1_000; public function __construct( private IDBConnection $db, + private ShareMapper $shareMapper, + private ContextNavigationMapper $contextNavigationMapper, private LoggerInterface $logger, ) { } @@ -78,20 +81,23 @@ public function getShares(): array { public function deleteShare(string $shareId): bool { $this->logger->info('Tables ShareReview: deleting share {id}', ['id' => $shareId]); try { - $qb = $this->db->getQueryBuilder(); - $qb->delete(self::SHARE_TABLE) - ->where($qb->expr()->eq('id', $qb->createNamedParameter((int)$shareId, IQueryBuilder::PARAM_INT))); - $deleted = $qb->executeStatement() > 0; + $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) { - $this->deleteContextNavigation((int)$shareId); + if (!$deleted) { + $this->logger->warning('Tables ShareReview: share {id} not found', ['id' => $shareId]); + return false; } - return $deleted; + 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; } /** @return list> */ @@ -156,7 +162,7 @@ private function fetchNames(string $table, string $nameColumn, array $ids): arra } $map = []; - foreach (array_chunk($ids, self::BATCH_SIZE) as $chunk) { + foreach (array_chunk($ids, self::DB_CHUNK_SIZE) as $chunk) { try { $qb = $this->db->getQueryBuilder(); $qb->select('id', $nameColumn) @@ -175,17 +181,6 @@ private function fetchNames(string $table, string $nameColumn, array $ids): arra return $map; } - private function deleteContextNavigation(int $shareId): void { - try { - $qb = $this->db->getQueryBuilder(); - $qb->delete(self::CONTEXTS_NAVIGATION_TABLE) - ->where($qb->expr()->eq('share_id', $qb->createNamedParameter($shareId, IQueryBuilder::PARAM_INT))); - $qb->executeStatement(); - } catch (Exception $e) { - $this->logger->error('Tables ShareReview: failed to clean up context navigation for share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]); - } - } - /** * @param array $share * @param array $tableNames diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index a8f3c9bc68..9fbbe17502 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -9,6 +9,8 @@ namespace OCA\Tables\Tests\Unit\ShareReview; +use OCA\Tables\Db\ContextNavigationMapper; +use OCA\Tables\Db\ShareMapper; use OCA\Tables\ShareReview\ShareReviewSource; use OCP\Constants; use OCP\DB\Exception; @@ -23,14 +25,23 @@ final class ShareReviewSourceTest extends TestCase { private MockObject $db; + private MockObject $shareMapper; + private MockObject $contextNavigationMapper; private MockObject $logger; private ShareReviewSource $source; protected function setUp(): void { parent::setUp(); $this->db = $this->createMock(IDBConnection::class); + $this->shareMapper = $this->createMock(ShareMapper::class); + $this->contextNavigationMapper = $this->createMock(ContextNavigationMapper::class); $this->logger = $this->createMock(LoggerInterface::class); - $this->source = new ShareReviewSource($this->db, $this->logger); + $this->source = new ShareReviewSource( + $this->db, + $this->shareMapper, + $this->contextNavigationMapper, + $this->logger, + ); } private function makeResult(array $rows): MockObject { @@ -40,7 +51,7 @@ private function makeResult(array $rows): MockObject { return $result; } - private function makeQb(array $fetchRows = [], int $statementRows = 0): MockObject { + private function makeQb(array $fetchRows = []): MockObject { $expr = $this->createMock(IExpressionBuilder::class); $expr->method('eq')->willReturn('1=1'); $expr->method('in')->willReturn('1=1'); @@ -50,11 +61,9 @@ private function makeQb(array $fetchRows = [], int $statementRows = 0): MockObje $qb->method('from')->willReturnSelf(); $qb->method('where')->willReturnSelf(); $qb->method('orderBy')->willReturnSelf(); - $qb->method('delete')->willReturnSelf(); $qb->method('createNamedParameter')->willReturn('?'); $qb->method('expr')->willReturn($expr); $qb->method('executeQuery')->willReturn($this->makeResult($fetchRows)); - $qb->method('executeStatement')->willReturn($statementRows); return $qb; } @@ -65,11 +74,9 @@ private function makeThrowingQb(): MockObject { $qb->method('from')->willReturnSelf(); $qb->method('where')->willReturnSelf(); $qb->method('orderBy')->willReturnSelf(); - $qb->method('delete')->willReturnSelf(); $qb->method('createNamedParameter')->willReturn('?'); $qb->method('expr')->willReturn($this->createMock(IExpressionBuilder::class)); $qb->method('executeQuery')->willThrowException($this->createMock(Exception::class)); - $qb->method('executeStatement')->willThrowException($this->createMock(Exception::class)); return $qb; } @@ -187,41 +194,38 @@ public function testGetSharesReturnsEmptyOnDbException(): void { } public function testDeleteShareSuccess(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([], 1), - $this->makeQb([], 0), - ); + $this->shareMapper->expects($this->once())->method('deleteById')->with(7)->willReturn(true); + $this->contextNavigationMapper->expects($this->once())->method('deleteByShareId')->with(7); $this->logger->expects($this->once())->method('info'); $this->assertTrue($this->source->deleteShare('7')); } - public function testDeleteShareCleansContextNavigation(): void { - $shareDeleteQb = $this->makeQb([], 1); - $navDeleteQb = $this->makeQb([], 1); - - $this->db->expects($this->exactly(2)) - ->method('getQueryBuilder') - ->willReturnOnConsecutiveCalls($shareDeleteQb, $navDeleteQb); + public function testDeleteShareNotFoundReturnsFalse(): void { + $this->shareMapper->method('deleteById')->willReturn(false); + $this->contextNavigationMapper->expects($this->never())->method('deleteByShareId'); + $this->logger->expects($this->once())->method('info'); + $this->logger->expects($this->once())->method('warning'); - $this->source->deleteShare('42'); + $this->assertFalse($this->source->deleteShare('99')); } - public function testDeleteShareNotFoundSkipsContextNavigation(): void { - $this->db->expects($this->once()) - ->method('getQueryBuilder') - ->willReturn($this->makeQb([], 0)); + public function testDeleteShareReturnsFalseOnDeleteException(): void { + $this->shareMapper->method('deleteById')->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'); - $this->assertFalse($this->source->deleteShare('99')); + $this->assertFalse($this->source->deleteShare('7')); } - public function testDeleteShareReturnsFalseOnDbException(): void { - $this->db->method('getQueryBuilder')->willReturn($this->makeThrowingQb()); + public function testDeleteShareReturnsTrueOnContextNavigationException(): void { + $this->shareMapper->method('deleteById')->willReturn(true); + $this->contextNavigationMapper->method('deleteByShareId')->willThrowException($this->createMock(Exception::class)); $this->logger->expects($this->once())->method('info'); $this->logger->expects($this->once())->method('error'); - $this->assertFalse($this->source->deleteShare('7')); + $this->assertTrue($this->source->deleteShare('42')); } public function testComputePermissionsAllFalse(): void { From 4d80200c4f788535cd9eb634924a18b7257ff88f Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 15 Jun 2026 12:17:06 +0200 Subject: [PATCH 10/19] fix(sharereview): remove redundant app key from getShares() output Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewSource.php | 4 +--- tests/unit/ShareReview/ShareReviewSourceTest.php | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index b51ece1d44..d73047103f 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -47,7 +47,7 @@ public function getName(): string { } /** - * @return list + * @return list */ public function getShares(): array { $rawShares = $this->fetchAllShares(); @@ -57,12 +57,10 @@ public function getShares(): array { $viewNames = $this->fetchNames(self::VIEWS_TABLE, 'title', $idsByType[self::NODE_TYPE_VIEW]); $contextNames = $this->fetchNames(self::CONTEXTS_TABLE, 'name', $idsByType[self::NODE_TYPE_CONTEXT]); - $appName = $this->getName(); $formatted = []; foreach ($rawShares as $share) { $formatted[] = [ 'id' => (int)$share['id'], - 'app' => $appName, 'object' => $this->resolveObjectName($share, $tableNames, $viewNames, $contextNames), 'initiator' => (string)$share['sender'], 'type' => $this->mapReceiverType((string)$share['receiver_type']), diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index 9fbbe17502..2bbbc8386c 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -117,7 +117,6 @@ public function testGetSharesUserShare(): void { $this->assertCount(1, $shares); $share = $shares[0]; $this->assertSame(1, $share['id']); - $this->assertSame('Tables', $share['app']); $this->assertSame('My Table (Table)', $share['object']); $this->assertSame('alice', $share['initiator']); $this->assertSame(IShare::TYPE_USER, $share['type']); From 03397bed75121c158037f3ee640117753aee58b0 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 15 Jun 2026 12:21:30 +0200 Subject: [PATCH 11/19] fix(sharereview): localize user-facing strings in ShareReviewSource Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewSource.php | 12 +++++++----- tests/unit/ShareReview/ShareReviewSourceTest.php | 5 +++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index d73047103f..601946edbf 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -16,6 +16,7 @@ use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; +use OCP\IL10N; use OCP\Share\IShare; use Psr\Log\LoggerInterface; @@ -38,12 +39,13 @@ public function __construct( private IDBConnection $db, private ShareMapper $shareMapper, private ContextNavigationMapper $contextNavigationMapper, + private IL10N $l10n, private LoggerInterface $logger, ) { } public function getName(): string { - return 'Tables'; + return $this->l10n->t('Tables'); } /** @@ -188,10 +190,10 @@ private function fetchNames(string $table, string $nameColumn, array $ids): arra private function resolveObjectName(array $share, array $tableNames, array $viewNames, array $contextNames): string { $nodeId = (int)$share['node_id']; return match($share['node_type']) { - self::NODE_TYPE_TABLE => ($tableNames[$nodeId] ?? "Table $nodeId") . ' (Table)', - self::NODE_TYPE_VIEW => ($viewNames[$nodeId] ?? "View $nodeId") . ' (View)', - self::NODE_TYPE_CONTEXT => ($contextNames[$nodeId] ?? "Context $nodeId") . ' (Context)', - default => "Unknown $nodeId", + self::NODE_TYPE_TABLE => $this->l10n->t('%s (Table)', [$tableNames[$nodeId] ?? $this->l10n->t('Table %s', [$nodeId])]), + self::NODE_TYPE_VIEW => $this->l10n->t('%s (View)', [$viewNames[$nodeId] ?? $this->l10n->t('View %s', [$nodeId])]), + self::NODE_TYPE_CONTEXT => $this->l10n->t('%s (Context)', [$contextNames[$nodeId] ?? $this->l10n->t('Context %s', [$nodeId])]), + default => $this->l10n->t('Unknown %s', [$nodeId]), }; } diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index 2bbbc8386c..9b8b54264a 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -18,6 +18,7 @@ use OCP\DB\QueryBuilder\IExpressionBuilder; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; +use OCP\IL10N; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -27,6 +28,7 @@ final class ShareReviewSourceTest extends TestCase { private MockObject $db; private MockObject $shareMapper; private MockObject $contextNavigationMapper; + private MockObject $l10n; private MockObject $logger; private ShareReviewSource $source; @@ -35,11 +37,14 @@ protected function setUp(): void { $this->db = $this->createMock(IDBConnection::class); $this->shareMapper = $this->createMock(ShareMapper::class); $this->contextNavigationMapper = $this->createMock(ContextNavigationMapper::class); + $this->l10n = $this->createMock(IL10N::class); + $this->l10n->method('t')->willReturnCallback(fn (string $text, array $params = []) => vsprintf($text, $params)); $this->logger = $this->createMock(LoggerInterface::class); $this->source = new ShareReviewSource( $this->db, $this->shareMapper, $this->contextNavigationMapper, + $this->l10n, $this->logger, ); } From 844e4feade8c26e87aa0eba3328426f49e26ea77 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 15 Jun 2026 12:24:04 +0200 Subject: [PATCH 12/19] fix(sharereview): log warnings for unknown node and receiver types of shares Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewSource.php | 45 +++++++++++++------ .../ShareReview/ShareReviewSourceTest.php | 25 +++++++++++ 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index 601946edbf..aa1f527b38 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -189,22 +189,41 @@ private function fetchNames(string $table, string $nameColumn, array $ids): arra */ private function resolveObjectName(array $share, array $tableNames, array $viewNames, array $contextNames): string { $nodeId = (int)$share['node_id']; - return match($share['node_type']) { - self::NODE_TYPE_TABLE => $this->l10n->t('%s (Table)', [$tableNames[$nodeId] ?? $this->l10n->t('Table %s', [$nodeId])]), - self::NODE_TYPE_VIEW => $this->l10n->t('%s (View)', [$viewNames[$nodeId] ?? $this->l10n->t('View %s', [$nodeId])]), - self::NODE_TYPE_CONTEXT => $this->l10n->t('%s (Context)', [$contextNames[$nodeId] ?? $this->l10n->t('Context %s', [$nodeId])]), - default => $this->l10n->t('Unknown %s', [$nodeId]), - }; + $nodeType = (string)$share['node_type']; + if ($nodeType === self::NODE_TYPE_TABLE) { + return $this->l10n->t('%s (Table)', [$tableNames[$nodeId] ?? $this->l10n->t('Table %s', [$nodeId])]); + } + if ($nodeType === self::NODE_TYPE_VIEW) { + return $this->l10n->t('%s (View)', [$viewNames[$nodeId] ?? $this->l10n->t('View %s', [$nodeId])]); + } + if ($nodeType === self::NODE_TYPE_CONTEXT) { + return $this->l10n->t('%s (Context)', [$contextNames[$nodeId] ?? $this->l10n->t('Context %s', [$nodeId])]); + } + $this->logger->warning( + 'Tables ShareReview: unknown node type {type} for share node {id}', + ['type' => $nodeType, 'id' => $nodeId] + ); + return $this->l10n->t('Unknown %s', [$nodeId]); } private function mapReceiverType(string $receiverType): int { - return match($receiverType) { - 'user' => IShare::TYPE_USER, - 'group' => IShare::TYPE_GROUP, - 'link' => IShare::TYPE_LINK, - 'circle' => IShare::TYPE_CIRCLE, - default => IShare::TYPE_USER, - }; + if ($receiverType === 'user') { + return IShare::TYPE_USER; + } + if ($receiverType === 'group') { + return IShare::TYPE_GROUP; + } + if ($receiverType === 'link') { + return IShare::TYPE_LINK; + } + if ($receiverType === 'circle') { + return IShare::TYPE_CIRCLE; + } + $this->logger->warning( + 'Tables ShareReview: unknown receiver type {type}, falling back to user share type', + ['type' => $receiverType] + ); + return IShare::TYPE_USER; } /** @param array $share */ diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index 9b8b54264a..d8506002c1 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -190,6 +190,31 @@ public function testGetSharesDeletedNode(): void { $this->assertSame('Table 42 (Table)', $shares[0]['object']); } + public function testGetSharesUnknownNodeTypeLogsWarning(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow(['node_type' => 'dashboard', 'node_id' => 5])]), + ); + $this->logger->expects($this->once())->method('warning'); + + $shares = $this->source->getShares(); + + $this->assertCount(1, $shares); + $this->assertSame('Unknown 5', $shares[0]['object']); + } + + public function testGetSharesUnknownReceiverTypeLogsWarning(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow(['receiver_type' => 'email', 'receiver' => 'bob@example.com'])]), + $this->makeQb([['id' => 10, 'title' => 'My Table']]), + ); + $this->logger->expects($this->once())->method('warning'); + + $shares = $this->source->getShares(); + + $this->assertCount(1, $shares); + $this->assertSame(IShare::TYPE_USER, $shares[0]['type']); + } + public function testGetSharesReturnsEmptyOnDbException(): void { $this->db->method('getQueryBuilder')->willReturn($this->makeThrowingQb()); $this->logger->expects($this->once())->method('error'); From 6004cbfb6171ef0323a499af52b320c28ab6a398 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Tue, 16 Jun 2026 09:11:12 +0200 Subject: [PATCH 13/19] feat(sharereview): expose last-edit time instead of creation time to share review Use last_edit_at as the share time when it is more recent than created_at, falling back to created_at otherwise. This ensures that shares whose permissions were modified after creation show the more recent timestamp, allowing the ShareReview app to surface recently-changed shares accurately. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewSource.php | 14 ++++++-- .../ShareReview/ShareReviewSourceTest.php | 34 +++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index aa1f527b38..72a576d595 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -71,7 +71,7 @@ public function getShares(): array { : (string)$share['receiver'], 'permissions' => $this->computePermissions($share), 'password' => $share['password'] !== null, - 'time' => (string)($share['created_at'] ?? '1970-01-01 01:00:00'), + 'time' => $this->resolveShareTime($share), 'action' => '', ]; } @@ -109,7 +109,7 @@ private function fetchAllShares(): array { 'token', 'password', 'permission_read', 'permission_create', 'permission_update', 'permission_delete', 'permission_manage', - 'created_at' + 'created_at', 'last_edit_at' )->from(self::SHARE_TABLE) ->orderBy('id', 'ASC'); $result = $qb->executeQuery(); @@ -226,6 +226,16 @@ private function mapReceiverType(string $receiverType): int { return IShare::TYPE_USER; } + /** @param array $share */ + private function resolveShareTime(array $share): string { + $createdAt = (string)($share['created_at'] ?? '1970-01-01 01:00:00'); + $lastEditAt = isset($share['last_edit_at']) ? (string)$share['last_edit_at'] : null; + if ($lastEditAt !== null && $lastEditAt > $createdAt) { + return $lastEditAt; + } + return $createdAt; + } + /** @param array $share */ private function computePermissions(array $share): int { $permissions = 0; diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index d8506002c1..6766192659 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -102,6 +102,7 @@ private function makeShareRow(array $overrides = []): array { 'permission_delete' => 0, 'permission_manage' => 0, 'created_at' => '2026-01-15 12:00:00', + 'last_edit_at' => '2026-01-15 12:00:00', ], $overrides); } @@ -132,6 +133,39 @@ public function testGetSharesUserShare(): void { $this->assertSame('', $share['action']); } + public function testShareTimeUsesLastEditWhenNewer(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow(['created_at' => '2026-01-15 12:00:00', 'last_edit_at' => '2026-06-01 08:00:00'])]), + $this->makeQb([['id' => 10, 'title' => 'T']]), + ); + + $shares = $this->source->getShares(); + + $this->assertSame('2026-06-01 08:00:00', $shares[0]['time']); + } + + public function testShareTimeUsesCreatedAtWhenLastEditEquals(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow(['created_at' => '2026-01-15 12:00:00', 'last_edit_at' => '2026-01-15 12:00:00'])]), + $this->makeQb([['id' => 10, 'title' => 'T']]), + ); + + $shares = $this->source->getShares(); + + $this->assertSame('2026-01-15 12:00:00', $shares[0]['time']); + } + + public function testShareTimeUsesCreatedAtWhenLastEditIsNull(): void { + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( + $this->makeQb([$this->makeShareRow(['created_at' => '2026-01-15 12:00:00', 'last_edit_at' => null])]), + $this->makeQb([['id' => 10, 'title' => 'T']]), + ); + + $shares = $this->source->getShares(); + + $this->assertSame('2026-01-15 12:00:00', $shares[0]['time']); + } + public function testGetSharesLinkShare(): void { $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( $this->makeQb([$this->makeShareRow(['receiver_type' => 'link', 'token' => 'abc123token', 'password' => 'hashed_pw'])]), From ed816f1c5c7edc6119f1dd5cdd7950d37736c327 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 22 Jun 2026 15:21:24 +0200 Subject: [PATCH 14/19] refactor(sharereview): move DB queries to mapper layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Database resource names and raw query logic no longer leak from ShareReviewSource into the application layer. ShareMapper gains findAllRaw() to fetch all shares without exposing the table name. TableMapper and ViewMapper gain findIdToTitleMap(), and ContextMapper gains findIdToNameMap() — each mapper owns its own SQL and table name. ShareReviewSource now depends on the three node-type mappers instead of IDBConnection directly. The tests are updated to mock at the mapper level, eliminating all raw IQueryBuilder/IDBConnection stub setup. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/Db/ContextMapper.php | 27 +++ lib/Db/ShareMapper.php | 22 ++ lib/Db/TableMapper.php | 27 +++ lib/Db/ViewMapper.php | 27 +++ lib/ShareReview/ShareReviewSource.php | 100 +++------ .../ShareReview/ShareReviewSourceTest.php | 209 +++++++----------- 6 files changed, 221 insertions(+), 191 deletions(-) diff --git a/lib/Db/ContextMapper.php b/lib/Db/ContextMapper.php index 3acbbe9633..880c6fc61a 100644 --- a/lib/Db/ContextMapper.php +++ b/lib/Db/ContextMapper.php @@ -279,6 +279,33 @@ public function findAllContainingNode(int $nodeType, int $nodeId, string $userId return $resultEntities; } + /** + * Fetch a map of id → name for the given context IDs. + * + * @param int[] $ids + * @return array + * @throws Exception + */ + public function findIdToNameMap(array $ids): array { + if ($ids === []) { + return []; + } + $qb = $this->db->getQueryBuilder(); + $qb->select('id', 'name') + ->from($this->table) + ->where($qb->expr()->in('id', $qb->createParameter('ids'))); + $map = []; + foreach (array_chunk($ids, 1_000) as $chunk) { + $qb->setParameter('ids', $chunk, IQueryBuilder::PARAM_INT_ARRAY); + $result = $qb->executeQuery(); + foreach ($result->fetchAll() as $row) { + $map[(int)$row['id']] = (string)$row['name']; + } + $result->closeCursor(); + } + return $map; + } + protected function applyOwnedOrSharedQuery(IQueryBuilder $qb, string $userId): void { $sharedToConditions = $qb->expr()->orX(); diff --git a/lib/Db/ShareMapper.php b/lib/Db/ShareMapper.php index 1b8bbddb6b..a5825cf9d9 100644 --- a/lib/Db/ShareMapper.php +++ b/lib/Db/ShareMapper.php @@ -246,6 +246,28 @@ public function findAllSharesForTablesAndContexts(array $tableIds, array $contex return $this->findEntities($qb); } + /** + * Return all shares as raw associative arrays, ordered by id. + * + * @return list> + * @throws Exception + */ + public function findAllRaw(): array { + $qb = $this->db->getQueryBuilder(); + $qb->select( + 'id', 'sender', 'receiver', 'receiver_type', 'node_id', 'node_type', + 'token', 'password', + 'permission_read', 'permission_create', 'permission_update', + 'permission_delete', 'permission_manage', + 'created_at', 'last_edit_at' + )->from($this->table) + ->orderBy('id', 'ASC'); + $result = $qb->executeQuery(); + $rows = $result->fetchAll(); + $result->closeCursor(); + return $rows; + } + /** * @throws Exception */ diff --git a/lib/Db/TableMapper.php b/lib/Db/TableMapper.php index e6785f79a1..0e3fcdd1f5 100644 --- a/lib/Db/TableMapper.php +++ b/lib/Db/TableMapper.php @@ -176,4 +176,31 @@ public function insert(Entity $entity): Table { public function getDbConnection() { return $this->db; } + + /** + * Fetch a map of id → title for the given table IDs. + * + * @param int[] $ids + * @return array + * @throws Exception + */ + public function findIdToTitleMap(array $ids): array { + if ($ids === []) { + return []; + } + $qb = $this->db->getQueryBuilder(); + $qb->select('id', 'title') + ->from($this->table) + ->where($qb->expr()->in('id', $qb->createParameter('ids'))); + $map = []; + foreach (array_chunk($ids, 1_000) as $chunk) { + $qb->setParameter('ids', $chunk, IQueryBuilder::PARAM_INT_ARRAY); + $result = $qb->executeQuery(); + foreach ($result->fetchAll() as $row) { + $map[(int)$row['id']] = (string)$row['title']; + } + $result->closeCursor(); + } + return $map; + } } diff --git a/lib/Db/ViewMapper.php b/lib/Db/ViewMapper.php index 531803be5f..0c286fc3af 100644 --- a/lib/Db/ViewMapper.php +++ b/lib/Db/ViewMapper.php @@ -178,4 +178,31 @@ public function search(?string $term = null, ?string $userId = null, ?int $limit return $this->findEntities($qb); } + + /** + * Fetch a map of id → title for the given view IDs. + * + * @param int[] $ids + * @return array + * @throws Exception + */ + public function findIdToTitleMap(array $ids): array { + if ($ids === []) { + return []; + } + $qb = $this->db->getQueryBuilder(); + $qb->select('id', 'title') + ->from($this->table) + ->where($qb->expr()->in('id', $qb->createParameter('ids'))); + $map = []; + foreach (array_chunk($ids, 1_000) as $chunk) { + $qb->setParameter('ids', $chunk, IQueryBuilder::PARAM_INT_ARRAY); + $result = $qb->executeQuery(); + foreach ($result->fetchAll() as $row) { + $map[(int)$row['id']] = (string)$row['title']; + } + $result->closeCursor(); + } + return $map; + } } diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index 72a576d595..305c40b1f9 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -10,35 +10,31 @@ namespace OCA\Tables\ShareReview; use OCA\ShareReview\Sources\ISource; +use OCA\Tables\Db\ContextMapper; use OCA\Tables\Db\ContextNavigationMapper; use OCA\Tables\Db\ShareMapper; +use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\ViewMapper; use OCP\Constants; use OCP\DB\Exception; -use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\IDBConnection; use OCP\IL10N; use OCP\Share\IShare; use Psr\Log\LoggerInterface; class ShareReviewSource implements ISource { - private const SHARE_TABLE = 'tables_shares'; - private const TABLES_TABLE = 'tables_tables'; - private const VIEWS_TABLE = 'tables_views'; - private const CONTEXTS_TABLE = 'tables_contexts_context'; - private const NODE_TYPE_TABLE = 'table'; private const NODE_TYPE_VIEW = 'view'; private const NODE_TYPE_CONTEXT = 'context'; private const RECEIVER_TYPE_LINK = 'link'; - private const DB_CHUNK_SIZE = 1_000; - public function __construct( - private IDBConnection $db, private ShareMapper $shareMapper, private ContextNavigationMapper $contextNavigationMapper, + private TableMapper $tableMapper, + private ViewMapper $viewMapper, + private ContextMapper $contextMapper, private IL10N $l10n, private LoggerInterface $logger, ) { @@ -52,12 +48,35 @@ public function getName(): string { * @return list */ public function getShares(): array { - $rawShares = $this->fetchAllShares(); + try { + $rawShares = $this->shareMapper->findAllRaw(); + } catch (Exception $e) { + $this->logger->error('Tables ShareReview: failed to fetch shares: {message}', ['message' => $e->getMessage()]); + return []; + } $idsByType = $this->groupIdsByNodeType($rawShares); - $tableNames = $this->fetchNames(self::TABLES_TABLE, 'title', $idsByType[self::NODE_TYPE_TABLE]); - $viewNames = $this->fetchNames(self::VIEWS_TABLE, 'title', $idsByType[self::NODE_TYPE_VIEW]); - $contextNames = $this->fetchNames(self::CONTEXTS_TABLE, 'name', $idsByType[self::NODE_TYPE_CONTEXT]); + + try { + $tableNames = $this->tableMapper->findIdToTitleMap($idsByType[self::NODE_TYPE_TABLE]); + } catch (Exception $e) { + $this->logger->error('Tables ShareReview: failed to fetch table names: {message}', ['message' => $e->getMessage()]); + $tableNames = []; + } + + try { + $viewNames = $this->viewMapper->findIdToTitleMap($idsByType[self::NODE_TYPE_VIEW]); + } catch (Exception $e) { + $this->logger->error('Tables ShareReview: failed to fetch view names: {message}', ['message' => $e->getMessage()]); + $viewNames = []; + } + + try { + $contextNames = $this->contextMapper->findIdToNameMap($idsByType[self::NODE_TYPE_CONTEXT]); + } catch (Exception $e) { + $this->logger->error('Tables ShareReview: failed to fetch application names: {message}', ['message' => $e->getMessage()]); + $contextNames = []; + } $formatted = []; foreach ($rawShares as $share) { @@ -100,28 +119,6 @@ public function deleteShare(string $shareId): bool { return true; } - /** @return list> */ - private function fetchAllShares(): array { - try { - $qb = $this->db->getQueryBuilder(); - $qb->select( - 'id', 'sender', 'receiver', 'receiver_type', 'node_id', 'node_type', - 'token', 'password', - 'permission_read', 'permission_create', 'permission_update', - 'permission_delete', 'permission_manage', - 'created_at', 'last_edit_at' - )->from(self::SHARE_TABLE) - ->orderBy('id', 'ASC'); - $result = $qb->executeQuery(); - $rows = $result->fetchAll(); - $result->closeCursor(); - return $rows; - } catch (Exception $e) { - $this->logger->error('Tables ShareReview: failed to fetch shares: {message}', ['message' => $e->getMessage()]); - return []; - } - } - /** * Group distinct node IDs by node type in a single pass over the share list. * @@ -150,37 +147,6 @@ private function groupIdsByNodeType(array $shares): array { ]; } - /** - * Batch-fetch node IDs → names, chunked to stay within database IN-clause limits. - * - * @param list $ids - * @return array - */ - private function fetchNames(string $table, string $nameColumn, array $ids): array { - if ($ids === []) { - return []; - } - - $map = []; - foreach (array_chunk($ids, self::DB_CHUNK_SIZE) as $chunk) { - try { - $qb = $this->db->getQueryBuilder(); - $qb->select('id', $nameColumn) - ->from($table) - ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); - $result = $qb->executeQuery(); - $rows = $result->fetchAll(); - $result->closeCursor(); - foreach ($rows as $row) { - $map[(int)$row['id']] = (string)$row[$nameColumn]; - } - } catch (Exception $e) { - $this->logger->error('Tables ShareReview: failed to fetch names from {table}: {message}', ['table' => $table, 'message' => $e->getMessage()]); - } - } - return $map; - } - /** * @param array $share * @param array $tableNames diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index 6766192659..4350050f7a 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -9,15 +9,14 @@ namespace OCA\Tables\Tests\Unit\ShareReview; +use OCA\Tables\Db\ContextMapper; use OCA\Tables\Db\ContextNavigationMapper; use OCA\Tables\Db\ShareMapper; +use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\ViewMapper; use OCA\Tables\ShareReview\ShareReviewSource; use OCP\Constants; use OCP\DB\Exception; -use OCP\DB\IResult; -use OCP\DB\QueryBuilder\IExpressionBuilder; -use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\IDBConnection; use OCP\IL10N; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; @@ -25,66 +24,36 @@ use Psr\Log\LoggerInterface; final class ShareReviewSourceTest extends TestCase { - private MockObject $db; private MockObject $shareMapper; private MockObject $contextNavigationMapper; + private MockObject $tableMapper; + private MockObject $viewMapper; + private MockObject $contextMapper; private MockObject $l10n; private MockObject $logger; private ShareReviewSource $source; protected function setUp(): void { parent::setUp(); - $this->db = $this->createMock(IDBConnection::class); $this->shareMapper = $this->createMock(ShareMapper::class); $this->contextNavigationMapper = $this->createMock(ContextNavigationMapper::class); + $this->tableMapper = $this->createMock(TableMapper::class); + $this->viewMapper = $this->createMock(ViewMapper::class); + $this->contextMapper = $this->createMock(ContextMapper::class); $this->l10n = $this->createMock(IL10N::class); $this->l10n->method('t')->willReturnCallback(fn (string $text, array $params = []) => vsprintf($text, $params)); $this->logger = $this->createMock(LoggerInterface::class); $this->source = new ShareReviewSource( - $this->db, $this->shareMapper, $this->contextNavigationMapper, + $this->tableMapper, + $this->viewMapper, + $this->contextMapper, $this->l10n, $this->logger, ); } - private function makeResult(array $rows): MockObject { - $result = $this->createMock(IResult::class); - $result->method('fetchAll')->willReturn($rows); - $result->method('closeCursor')->willReturn(true); - return $result; - } - - private function makeQb(array $fetchRows = []): MockObject { - $expr = $this->createMock(IExpressionBuilder::class); - $expr->method('eq')->willReturn('1=1'); - $expr->method('in')->willReturn('1=1'); - - $qb = $this->createMock(IQueryBuilder::class); - $qb->method('select')->willReturnSelf(); - $qb->method('from')->willReturnSelf(); - $qb->method('where')->willReturnSelf(); - $qb->method('orderBy')->willReturnSelf(); - $qb->method('createNamedParameter')->willReturn('?'); - $qb->method('expr')->willReturn($expr); - $qb->method('executeQuery')->willReturn($this->makeResult($fetchRows)); - - return $qb; - } - - private function makeThrowingQb(): MockObject { - $qb = $this->createMock(IQueryBuilder::class); - $qb->method('select')->willReturnSelf(); - $qb->method('from')->willReturnSelf(); - $qb->method('where')->willReturnSelf(); - $qb->method('orderBy')->willReturnSelf(); - $qb->method('createNamedParameter')->willReturn('?'); - $qb->method('expr')->willReturn($this->createMock(IExpressionBuilder::class)); - $qb->method('executeQuery')->willThrowException($this->createMock(Exception::class)); - return $qb; - } - /** @param array $overrides */ private function makeShareRow(array $overrides = []): array { return array_merge([ @@ -106,17 +75,22 @@ private function makeShareRow(array $overrides = []): array { ], $overrides); } + private function stubNameMappers(array $tableNames = [], array $viewNames = [], array $contextNames = []): void { + $this->tableMapper->method('findIdToTitleMap')->willReturn($tableNames); + $this->viewMapper->method('findIdToTitleMap')->willReturn($viewNames); + $this->contextMapper->method('findIdToNameMap')->willReturn($contextNames); + } + public function testGetSharesEmpty(): void { - $this->db->method('getQueryBuilder')->willReturn($this->makeQb()); + $this->shareMapper->method('findAllRaw')->willReturn([]); + $this->stubNameMappers(); $this->assertSame([], $this->source->getShares()); } public function testGetSharesUserShare(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow()]), - $this->makeQb([['id' => 10, 'title' => 'My Table']]), - ); + $this->shareMapper->method('findAllRaw')->willReturn([$this->makeShareRow()]); + $this->stubNameMappers(tableNames: [10 => 'My Table']); $shares = $this->source->getShares(); @@ -134,43 +108,37 @@ public function testGetSharesUserShare(): void { } public function testShareTimeUsesLastEditWhenNewer(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow(['created_at' => '2026-01-15 12:00:00', 'last_edit_at' => '2026-06-01 08:00:00'])]), - $this->makeQb([['id' => 10, 'title' => 'T']]), - ); - - $shares = $this->source->getShares(); + $this->shareMapper->method('findAllRaw')->willReturn([ + $this->makeShareRow(['created_at' => '2026-01-15 12:00:00', 'last_edit_at' => '2026-06-01 08:00:00']), + ]); + $this->stubNameMappers(tableNames: [10 => 'T']); - $this->assertSame('2026-06-01 08:00:00', $shares[0]['time']); + $this->assertSame('2026-06-01 08:00:00', $this->source->getShares()[0]['time']); } public function testShareTimeUsesCreatedAtWhenLastEditEquals(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow(['created_at' => '2026-01-15 12:00:00', 'last_edit_at' => '2026-01-15 12:00:00'])]), - $this->makeQb([['id' => 10, 'title' => 'T']]), - ); + $this->shareMapper->method('findAllRaw')->willReturn([ + $this->makeShareRow(['created_at' => '2026-01-15 12:00:00', 'last_edit_at' => '2026-01-15 12:00:00']), + ]); + $this->stubNameMappers(tableNames: [10 => 'T']); - $shares = $this->source->getShares(); - - $this->assertSame('2026-01-15 12:00:00', $shares[0]['time']); + $this->assertSame('2026-01-15 12:00:00', $this->source->getShares()[0]['time']); } public function testShareTimeUsesCreatedAtWhenLastEditIsNull(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow(['created_at' => '2026-01-15 12:00:00', 'last_edit_at' => null])]), - $this->makeQb([['id' => 10, 'title' => 'T']]), - ); - - $shares = $this->source->getShares(); + $this->shareMapper->method('findAllRaw')->willReturn([ + $this->makeShareRow(['created_at' => '2026-01-15 12:00:00', 'last_edit_at' => null]), + ]); + $this->stubNameMappers(tableNames: [10 => 'T']); - $this->assertSame('2026-01-15 12:00:00', $shares[0]['time']); + $this->assertSame('2026-01-15 12:00:00', $this->source->getShares()[0]['time']); } public function testGetSharesLinkShare(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow(['receiver_type' => 'link', 'token' => 'abc123token', 'password' => 'hashed_pw'])]), - $this->makeQb([['id' => 10, 'title' => 'Shared Table']]), - ); + $this->shareMapper->method('findAllRaw')->willReturn([ + $this->makeShareRow(['receiver_type' => 'link', 'token' => 'abc123token', 'password' => 'hashed_pw']), + ]); + $this->stubNameMappers(tableNames: [10 => 'Shared Table']); $shares = $this->source->getShares(); @@ -181,10 +149,10 @@ public function testGetSharesLinkShare(): void { } public function testGetSharesContextShare(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow(['node_type' => 'context', 'node_id' => 99])]), - $this->makeQb([['id' => 99, 'name' => 'My Context']]), - ); + $this->shareMapper->method('findAllRaw')->willReturn([ + $this->makeShareRow(['node_type' => 'context', 'node_id' => 99]), + ]); + $this->stubNameMappers(contextNames: [99 => 'My Context']); $shares = $this->source->getShares(); @@ -193,15 +161,15 @@ public function testGetSharesContextShare(): void { } public function testGetSharesMixedNodeTypes(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([ - $this->makeShareRow(['id' => 1, 'node_type' => 'table', 'node_id' => 10]), - $this->makeShareRow(['id' => 2, 'node_type' => 'view', 'node_id' => 20]), - $this->makeShareRow(['id' => 3, 'node_type' => 'context', 'node_id' => 30]), - ]), - $this->makeQb([['id' => 10, 'title' => 'Table One']]), - $this->makeQb([['id' => 20, 'title' => 'View One']]), - $this->makeQb([['id' => 30, 'name' => 'Context One']]), + $this->shareMapper->method('findAllRaw')->willReturn([ + $this->makeShareRow(['id' => 1, 'node_type' => 'table', 'node_id' => 10]), + $this->makeShareRow(['id' => 2, 'node_type' => 'view', 'node_id' => 20]), + $this->makeShareRow(['id' => 3, 'node_type' => 'context', 'node_id' => 30]), + ]); + $this->stubNameMappers( + tableNames: [10 => 'Table One'], + viewNames: [20 => 'View One'], + contextNames: [30 => 'Context One'], ); $shares = $this->source->getShares(); @@ -213,10 +181,10 @@ public function testGetSharesMixedNodeTypes(): void { } public function testGetSharesDeletedNode(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow(['node_type' => 'table', 'node_id' => 42])]), - $this->makeQb([]), - ); + $this->shareMapper->method('findAllRaw')->willReturn([ + $this->makeShareRow(['node_type' => 'table', 'node_id' => 42]), + ]); + $this->stubNameMappers(); $shares = $this->source->getShares(); @@ -225,9 +193,10 @@ public function testGetSharesDeletedNode(): void { } public function testGetSharesUnknownNodeTypeLogsWarning(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow(['node_type' => 'dashboard', 'node_id' => 5])]), - ); + $this->shareMapper->method('findAllRaw')->willReturn([ + $this->makeShareRow(['node_type' => 'dashboard', 'node_id' => 5]), + ]); + $this->stubNameMappers(); $this->logger->expects($this->once())->method('warning'); $shares = $this->source->getShares(); @@ -237,10 +206,10 @@ public function testGetSharesUnknownNodeTypeLogsWarning(): void { } public function testGetSharesUnknownReceiverTypeLogsWarning(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow(['receiver_type' => 'email', 'receiver' => 'bob@example.com'])]), - $this->makeQb([['id' => 10, 'title' => 'My Table']]), - ); + $this->shareMapper->method('findAllRaw')->willReturn([ + $this->makeShareRow(['receiver_type' => 'email', 'receiver' => 'bob@example.com']), + ]); + $this->stubNameMappers(tableNames: [10 => 'My Table']); $this->logger->expects($this->once())->method('warning'); $shares = $this->source->getShares(); @@ -250,7 +219,7 @@ public function testGetSharesUnknownReceiverTypeLogsWarning(): void { } public function testGetSharesReturnsEmptyOnDbException(): void { - $this->db->method('getQueryBuilder')->willReturn($this->makeThrowingQb()); + $this->shareMapper->method('findAllRaw')->willThrowException($this->createMock(Exception::class)); $this->logger->expects($this->once())->method('error'); $this->assertSame([], $this->source->getShares()); @@ -292,37 +261,29 @@ public function testDeleteShareReturnsTrueOnContextNavigationException(): void { } public function testComputePermissionsAllFalse(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow([ - 'permission_read' => 0, - 'permission_create' => 0, - 'permission_update' => 0, - 'permission_delete' => 0, - 'permission_manage' => 0, - ])]), - $this->makeQb([['id' => 10, 'title' => 'T']]), - ); - - $shares = $this->source->getShares(); + $this->shareMapper->method('findAllRaw')->willReturn([$this->makeShareRow([ + 'permission_read' => 0, + 'permission_create' => 0, + 'permission_update' => 0, + 'permission_delete' => 0, + 'permission_manage' => 0, + ])]); + $this->stubNameMappers(tableNames: [10 => 'T']); - $this->assertSame(Constants::PERMISSION_READ, $shares[0]['permissions']); + $this->assertSame(Constants::PERMISSION_READ, $this->source->getShares()[0]['permissions']); } public function testComputePermissionsAllTrue(): void { - $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls( - $this->makeQb([$this->makeShareRow([ - 'permission_read' => 1, - 'permission_create' => 1, - 'permission_update' => 1, - 'permission_delete' => 1, - 'permission_manage' => 1, - ])]), - $this->makeQb([['id' => 10, 'title' => 'T']]), - ); - - $shares = $this->source->getShares(); + $this->shareMapper->method('findAllRaw')->willReturn([$this->makeShareRow([ + 'permission_read' => 1, + 'permission_create' => 1, + 'permission_update' => 1, + 'permission_delete' => 1, + 'permission_manage' => 1, + ])]); + $this->stubNameMappers(tableNames: [10 => 'T']); $expected = Constants::PERMISSION_READ | Constants::PERMISSION_UPDATE | Constants::PERMISSION_CREATE | Constants::PERMISSION_DELETE | Constants::PERMISSION_SHARE; - $this->assertSame($expected, $shares[0]['permissions']); + $this->assertSame($expected, $this->source->getShares()[0]['permissions']); } } From dea5e08d794e4c67d1bc686c6bb4d788f51194c3 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 22 Jun 2026 15:22:54 +0200 Subject: [PATCH 15/19] refactor(sharereview): introduce ShareInfo DTO for typed share entries Replace the anonymous associative array built inline in getShares() with a proper ShareInfo value object. ShareReviewSource now constructs ShareInfo instances via buildShareInfo() and converts them to the array format required by ISource::getShares() through ShareInfo::toArray(). This improves maintainability and gives static analysis a concrete type to reason about instead of an ad-hoc array shape. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareInfo.php | 56 +++++++++++++++++++++++++++ lib/ShareReview/ShareReviewSource.php | 35 ++++++++++------- 2 files changed, 78 insertions(+), 13 deletions(-) create mode 100644 lib/ShareReview/ShareInfo.php diff --git a/lib/ShareReview/ShareInfo.php b/lib/ShareReview/ShareInfo.php new file mode 100644 index 0000000000..370b0b4c53 --- /dev/null +++ b/lib/ShareReview/ShareInfo.php @@ -0,0 +1,56 @@ + $this->id, + 'object' => $this->object, + 'initiator' => $this->initiator, + 'type' => $this->type, + 'recipient' => $this->recipient, + 'permissions' => $this->permissions, + 'password' => $this->password, + 'time' => $this->time, + 'action' => '', + ]; + } +} diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index 305c40b1f9..16a5145bdb 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -80,19 +80,7 @@ public function getShares(): array { $formatted = []; foreach ($rawShares as $share) { - $formatted[] = [ - 'id' => (int)$share['id'], - 'object' => $this->resolveObjectName($share, $tableNames, $viewNames, $contextNames), - 'initiator' => (string)$share['sender'], - 'type' => $this->mapReceiverType((string)$share['receiver_type']), - 'recipient' => $share['receiver_type'] === self::RECEIVER_TYPE_LINK - ? (string)$share['token'] - : (string)$share['receiver'], - 'permissions' => $this->computePermissions($share), - 'password' => $share['password'] !== null, - 'time' => $this->resolveShareTime($share), - 'action' => '', - ]; + $formatted[] = $this->buildShareInfo($share, $tableNames, $viewNames, $contextNames)->toArray(); } return $formatted; } @@ -119,6 +107,27 @@ public function deleteShare(string $shareId): bool { return true; } + /** + * @param array $share + * @param array $tableNames + * @param array $viewNames + * @param array $contextNames + */ + private function buildShareInfo(array $share, array $tableNames, array $viewNames, array $contextNames): ShareInfo { + return new ShareInfo( + id: (int)$share['id'], + object: $this->resolveObjectName($share, $tableNames, $viewNames, $contextNames), + initiator: (string)$share['sender'], + type: $this->mapReceiverType((string)$share['receiver_type']), + recipient: $share['receiver_type'] === self::RECEIVER_TYPE_LINK + ? (string)$share['token'] + : (string)$share['receiver'], + permissions: $this->computePermissions($share), + password: $share['password'] !== null, + time: $this->resolveShareTime($share), + ); + } + /** * Group distinct node IDs by node type in a single pass over the share list. * From 3cc9ce3e870d8d8eb2f40327288991890df4f694 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 22 Jun 2026 15:23:17 +0200 Subject: [PATCH 16/19] fix(sharereview): use 'Application' as user-facing label for contexts 'Context' is an internal technical term. The user-facing name for the same concept is 'Application'. Update the share review object label so users see e.g. 'My App (Application)' instead of 'My App (Context)'. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/ShareReview/ShareReviewSource.php | 2 +- tests/unit/ShareReview/ShareReviewSourceTest.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index 16a5145bdb..beb5c2f0f8 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -172,7 +172,7 @@ private function resolveObjectName(array $share, array $tableNames, array $viewN return $this->l10n->t('%s (View)', [$viewNames[$nodeId] ?? $this->l10n->t('View %s', [$nodeId])]); } if ($nodeType === self::NODE_TYPE_CONTEXT) { - return $this->l10n->t('%s (Context)', [$contextNames[$nodeId] ?? $this->l10n->t('Context %s', [$nodeId])]); + return $this->l10n->t('%s (Application)', [$contextNames[$nodeId] ?? $this->l10n->t('Application %s', [$nodeId])]); } $this->logger->warning( 'Tables ShareReview: unknown node type {type} for share node {id}', diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index 4350050f7a..e7ec37f95b 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -157,7 +157,7 @@ public function testGetSharesContextShare(): void { $shares = $this->source->getShares(); $this->assertCount(1, $shares); - $this->assertSame('My Context (Context)', $shares[0]['object']); + $this->assertSame('My Context (Application)', $shares[0]['object']); } public function testGetSharesMixedNodeTypes(): void { @@ -177,7 +177,7 @@ public function testGetSharesMixedNodeTypes(): void { $this->assertCount(3, $shares); $this->assertSame('Table One (Table)', $shares[0]['object']); $this->assertSame('View One (View)', $shares[1]['object']); - $this->assertSame('Context One (Context)', $shares[2]['object']); + $this->assertSame('Context One (Application)', $shares[2]['object']); } public function testGetSharesDeletedNode(): void { From 8f0518c6902977b6663901f039f09089ae7affa1 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 22 Jun 2026 15:24:38 +0200 Subject: [PATCH 17/19] 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 --- lib/ShareReview/ShareReviewSource.php | 28 ++++++++++----- .../ShareReview/ShareReviewSourceTest.php | 36 ++++++++++++++++--- 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index beb5c2f0f8..e9327d0959 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/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; } diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index e7ec37f95b..e29af9d2bb 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/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,8 +227,27 @@ 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'); @@ -234,7 +255,8 @@ public function testDeleteShareSuccess(): void { } 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'); From b0444a462554190cac22d85cee575b965ac303d0 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 22 Jun 2026 16:40:08 +0200 Subject: [PATCH 18/19] feat(sharereview): gate ACL deletion behind event-based access check Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger --- lib/Service/ShareService.php | 18 ++++ .../ShareReviewAccessCheckEvent.php | 43 ++++++++ lib/ShareReview/ShareReviewSource.php | 36 +++---- tests/unit/Service/ShareServiceTest.php | 57 ++++++++++ .../ShareReviewAccessCheckEventTest.php | 58 ++++++++++ .../ShareReview/ShareReviewSourceTest.php | 101 ++++++++++-------- 6 files changed, 247 insertions(+), 66 deletions(-) create mode 100644 lib/ShareReview/ShareReviewAccessCheckEvent.php create mode 100644 tests/unit/ShareReview/ShareReviewAccessCheckEventTest.php diff --git a/lib/Service/ShareService.php b/lib/Service/ShareService.php index 72f8a09524..9947d74a64 100644 --- a/lib/Service/ShareService.php +++ b/lib/Service/ShareService.php @@ -696,6 +696,24 @@ private function addReceiverDisplayNames(array $shares): array { return $shares; } + /** + * Delete a share on behalf of a trusted share-review operation. + * + * PERMISSION_MANAGE is intentionally not checked. The caller must verify + * operator access via ShareReviewAccessCheckEvent before invoking this + * method. All other side effects are preserved so the deletion is auditable. + * + * @throws \OCP\AppFramework\Db\DoesNotExistException if $id does not exist + * @throws Exception on database failure + */ + public function deleteForShareReview(int $id): void { + $share = $this->mapper->find($id); + $this->mapper->delete($share); + if ($share->getNodeType() === 'context') { + $this->contextNavigationMapper->deleteByShareId($share->getId()); + } + } + public function deleteAllForTable(Table $table):void { try { $this->mapper->deleteByNode($table->getId(), 'table'); diff --git a/lib/ShareReview/ShareReviewAccessCheckEvent.php b/lib/ShareReview/ShareReviewAccessCheckEvent.php new file mode 100644 index 0000000000..b9c152404b --- /dev/null +++ b/lib/ShareReview/ShareReviewAccessCheckEvent.php @@ -0,0 +1,43 @@ +handled && !$this->granted) { + return; // a prior denyAccess() cannot be escalated to a grant — deny wins + } + $this->handled = true; + $this->granted = true; + } + + public function denyAccess(string $reason): void { + $this->handled = true; + $this->granted = false; + $this->reason = $reason; + } + + public function isHandled(): bool { + return $this->handled; + } + public function isGranted(): bool { + return $this->granted; + } + public function getReason(): ?string { + return $this->reason; + } +} diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index e9327d0959..1e8db4a0ad 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -11,14 +11,14 @@ use OCA\ShareReview\Sources\ISource; use OCA\Tables\Db\ContextMapper; -use OCA\Tables\Db\ContextNavigationMapper; use OCA\Tables\Db\ShareMapper; use OCA\Tables\Db\TableMapper; use OCA\Tables\Db\ViewMapper; +use OCA\Tables\Service\ShareService; use OCP\AppFramework\Db\DoesNotExistException; -use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\Constants; use OCP\DB\Exception; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IL10N; use OCP\Share\IShare; use Psr\Log\LoggerInterface; @@ -33,12 +33,13 @@ class ShareReviewSource implements ISource { public function __construct( private ShareMapper $shareMapper, - private ContextNavigationMapper $contextNavigationMapper, private TableMapper $tableMapper, private ViewMapper $viewMapper, private ContextMapper $contextMapper, private IL10N $l10n, private LoggerInterface $logger, + private readonly ShareService $shareService, + private readonly IEventDispatcher $eventDispatcher, ) { } @@ -88,33 +89,26 @@ public function getShares(): array { } public function deleteShare(string $shareId): bool { - $this->logger->info('Tables ShareReview: deleting share {id}', ['id' => $shareId]); - - try { - $share = $this->shareMapper->find((int)$shareId); - } catch (DoesNotExistException) { - $this->logger->warning('Tables ShareReview: share {id} not found', ['id' => $shareId]); + if (!is_numeric($shareId)) { return false; - } catch (MultipleObjectsReturnedException|Exception $e) { - $this->logger->error('Tables ShareReview: failed to find share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]); + } + + $event = new ShareReviewAccessCheckEvent(); + $this->eventDispatcher->dispatchTyped($event); + + if (!$event->isHandled() || !$event->isGranted()) { return false; } try { - $this->shareMapper->delete($share); + $this->shareService->deleteForShareReview((int)$shareId); + return true; + } catch (DoesNotExistException) { + return false; } catch (Exception $e) { $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; } /** diff --git a/tests/unit/Service/ShareServiceTest.php b/tests/unit/Service/ShareServiceTest.php index 2e1e46a905..c2127e03fc 100644 --- a/tests/unit/Service/ShareServiceTest.php +++ b/tests/unit/Service/ShareServiceTest.php @@ -21,6 +21,8 @@ use OCA\Tables\Service\PermissionsService; use OCA\Tables\Service\ShareService; use OCA\Tables\Service\ValueObject\ShareCreate; +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\DB\Exception; use OCP\IDBConnection; use OCP\IUserManager; use OCP\Security\IHasher; @@ -114,4 +116,59 @@ public function testUpdatePermissionThrowsOnContextShare(): void { $this->expectException(PermissionError::class); $this->shareService->updatePermission(1, ['manage' => true]); } + + public function testDeleteForShareReviewCallsSideEffects(): void { + $share = new Share(); + $share->setId(42); + $share->setNodeType('table'); + $this->mapper->expects($this->once()) + ->method('find') + ->with(42) + ->willReturn($share); + $this->mapper->expects($this->once()) + ->method('delete') + ->with($share); + $this->contextNavigationMapper->expects($this->never())->method('deleteByShareId'); + + $this->shareService->deleteForShareReview(42); + } + + public function testDeleteForShareReviewCleansUpContextNavigation(): void { + $share = new Share(); + $share->setId(7); + $share->setNodeType('context'); + $this->mapper->expects($this->once()) + ->method('find') + ->with(7) + ->willReturn($share); + $this->mapper->expects($this->once()) + ->method('delete') + ->with($share); + $this->contextNavigationMapper->expects($this->once()) + ->method('deleteByShareId') + ->with(7); + + $this->shareService->deleteForShareReview(7); + } + + public function testDeleteForShareReviewPropagatesDoesNotExist(): void { + $this->mapper->expects($this->once()) + ->method('find') + ->with(99) + ->willThrowException(new DoesNotExistException('')); + + $this->expectException(DoesNotExistException::class); + $this->shareService->deleteForShareReview(99); + } + + public function testDeleteForShareReviewPropagatesDbException(): void { + $share = new Share(); + $share->setId(5); + $share->setNodeType('table'); + $this->mapper->method('find')->willReturn($share); + $this->mapper->method('delete')->willThrowException($this->createMock(Exception::class)); + + $this->expectException(Exception::class); + $this->shareService->deleteForShareReview(5); + } } diff --git a/tests/unit/ShareReview/ShareReviewAccessCheckEventTest.php b/tests/unit/ShareReview/ShareReviewAccessCheckEventTest.php new file mode 100644 index 0000000000..aa9b6a9613 --- /dev/null +++ b/tests/unit/ShareReview/ShareReviewAccessCheckEventTest.php @@ -0,0 +1,58 @@ +assertFalse($event->isHandled()); + $this->assertFalse($event->isGranted()); + $this->assertNull($event->getReason()); + } + + public function testGrantAccess(): void { + $event = new ShareReviewAccessCheckEvent(); + $event->grantAccess(); + + $this->assertTrue($event->isHandled()); + $this->assertTrue($event->isGranted()); + } + + public function testDenyAccess(): void { + $event = new ShareReviewAccessCheckEvent(); + $event->denyAccess('not in group'); + + $this->assertTrue($event->isHandled()); + $this->assertFalse($event->isGranted()); + $this->assertSame('not in group', $event->getReason()); + } + + public function testGrantThenDenyIsGrantedFalse(): void { + $event = new ShareReviewAccessCheckEvent(); + $event->grantAccess(); + $event->denyAccess('revoked'); + + $this->assertFalse($event->isGranted()); + $this->assertSame('revoked', $event->getReason()); + } + + public function testDenyThenGrantIsGrantedFalse(): void { + $event = new ShareReviewAccessCheckEvent(); + $event->denyAccess('not allowed'); + $event->grantAccess(); + + $this->assertFalse($event->isGranted()); + } +} diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index e29af9d2bb..d37c29b311 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -10,15 +10,16 @@ namespace OCA\Tables\Tests\Unit\ShareReview; 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\Service\ShareService; +use OCA\Tables\ShareReview\ShareReviewAccessCheckEvent; use OCA\Tables\ShareReview\ShareReviewSource; use OCP\AppFramework\Db\DoesNotExistException; use OCP\Constants; use OCP\DB\Exception; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IL10N; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; @@ -27,32 +28,35 @@ final class ShareReviewSourceTest extends TestCase { private MockObject $shareMapper; - private MockObject $contextNavigationMapper; private MockObject $tableMapper; private MockObject $viewMapper; private MockObject $contextMapper; private MockObject $l10n; private MockObject $logger; + private MockObject $shareService; + private MockObject $eventDispatcher; private ShareReviewSource $source; protected function setUp(): void { parent::setUp(); $this->shareMapper = $this->createMock(ShareMapper::class); - $this->contextNavigationMapper = $this->createMock(ContextNavigationMapper::class); $this->tableMapper = $this->createMock(TableMapper::class); $this->viewMapper = $this->createMock(ViewMapper::class); $this->contextMapper = $this->createMock(ContextMapper::class); $this->l10n = $this->createMock(IL10N::class); $this->l10n->method('t')->willReturnCallback(fn (string $text, array $params = []) => vsprintf($text, $params)); $this->logger = $this->createMock(LoggerInterface::class); + $this->shareService = $this->createMock(ShareService::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->source = new ShareReviewSource( $this->shareMapper, - $this->contextNavigationMapper, $this->tableMapper, $this->viewMapper, $this->contextMapper, $this->l10n, $this->logger, + $this->shareService, + $this->eventDispatcher, ); } @@ -227,63 +231,70 @@ public function testGetSharesReturnsEmptyOnDbException(): void { $this->assertSame([], $this->source->getShares()); } - private function makeShare(int $id, string $nodeType): Share { - $share = new Share(); - $share->setId($id); - $share->setNodeType($nodeType); - return $share; + public function testDeleteShareNonNumericReturnsFalse(): void { + $this->eventDispatcher->expects($this->never())->method('dispatchTyped'); + + $this->assertFalse($this->source->deleteShare('abc')); } - 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'); + public function testDeleteShareEventNotHandledReturnsFalse(): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->isInstanceOf(ShareReviewAccessCheckEvent::class)); + $this->shareService->expects($this->never())->method('deleteForShareReview'); - $this->assertTrue($this->source->deleteShare('7')); + $this->assertFalse($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'); + public function testDeleteShareEventDeniedReturnsFalse(): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->isInstanceOf(ShareReviewAccessCheckEvent::class)) + ->willReturnCallback(function (ShareReviewAccessCheckEvent $event): void { + $event->denyAccess('not a share-review operator'); + }); + $this->shareService->expects($this->never())->method('deleteForShareReview'); - $this->assertTrue($this->source->deleteShare('7')); + $this->assertFalse($this->source->deleteShare('7')); } - public function testDeleteShareNotFoundReturnsFalse(): void { - $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'); + public function testDeleteShareEventGrantedReturnsTrue(): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->isInstanceOf(ShareReviewAccessCheckEvent::class)) + ->willReturnCallback(function (ShareReviewAccessCheckEvent $event): void { + $event->grantAccess(); + }); + $this->shareService->expects($this->once())->method('deleteForShareReview')->with(7); - $this->assertFalse($this->source->deleteShare('99')); + $this->assertTrue($this->source->deleteShare('7')); } - public function testDeleteShareReturnsFalseOnDeleteException(): void { - $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'); + public function testDeleteShareDoesNotExistReturnsFalse(): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->willReturnCallback(function (ShareReviewAccessCheckEvent $event): void { + $event->grantAccess(); + }); + $this->shareService->expects($this->once()) + ->method('deleteForShareReview') + ->willThrowException($this->createMock(DoesNotExistException::class)); $this->assertFalse($this->source->deleteShare('7')); } - public function testDeleteShareReturnsTrueOnContextNavigationException(): void { - $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'); + public function testDeleteShareDbExceptionReturnsFalse(): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->willReturnCallback(function (ShareReviewAccessCheckEvent $event): void { + $event->grantAccess(); + }); + $this->shareService->expects($this->once()) + ->method('deleteForShareReview') + ->willThrowException($this->createMock(Exception::class)); $this->logger->expects($this->once())->method('error'); - $this->assertTrue($this->source->deleteShare('42')); + $this->assertFalse($this->source->deleteShare('7')); } public function testComputePermissionsAllFalse(): void { From ee92159e9e1270243df36d576c4cf18c41f32c29 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Thu, 2 Jul 2026 09:41:03 +0200 Subject: [PATCH 19/19] refactor(sharereview): adopt OCP\Share\ShareReview interface and event namespaces Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger --- lib/AppInfo/Application.php | 4 +- .../ShareReviewAccessCheckEvent.php | 43 ---------- lib/ShareReview/ShareReviewListener.php | 6 +- lib/ShareReview/ShareReviewSource.php | 7 +- tests/stub.phpstub | 22 ++++- .../ShareReviewAccessCheckEventTest.php | 58 ------------- .../ShareReview/ShareReviewSourceTest.php | 2 +- tests/unit/ShareReview/Stubs.php | 83 +++++++++++++++++-- tests/unit/bootstrap.php | 2 +- 9 files changed, 103 insertions(+), 124 deletions(-) delete mode 100644 lib/ShareReview/ShareReviewAccessCheckEvent.php delete mode 100644 tests/unit/ShareReview/ShareReviewAccessCheckEventTest.php diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 9b17681dd4..186c7b84c7 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -9,7 +9,6 @@ use Exception; use OCA\Analytics\Datasource\DatasourceEvent; -use OCA\ShareReview\Sources\SourceEvent; use OCA\Tables\Capabilities; use OCA\Tables\Event\RowDeletedEvent; use OCA\Tables\Event\TableDeletedEvent; @@ -42,6 +41,7 @@ use OCP\Collaboration\Reference\RenderReferenceEvent; use OCP\Collaboration\Resources\LoadAdditionalScriptsEvent; use OCP\DB\Events\AddMissingIndicesEvent; +use OCP\Share\ShareReview\RegisterShareReviewSourceEvent; use OCP\User\Events\BeforeUserDeletedEvent; use Psr\Container\ContainerInterface; @@ -81,7 +81,7 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(BeforeUserDeletedEvent::class, UserDeletedListener::class); $context->registerEventListener(DatasourceEvent::class, AnalyticsDatasourceListener::class); - $context->registerEventListener(SourceEvent::class, ShareReviewListener::class); + $context->registerEventListener(RegisterShareReviewSourceEvent::class, ShareReviewListener::class); $context->registerEventListener(RenderReferenceEvent::class, TablesReferenceListener::class); $context->registerEventListener(BeforeTemplateRenderedEvent::class, BeforeTemplateRenderedListener::class); $context->registerEventListener(LoadAdditionalScriptsEvent::class, LoadAdditionalListener::class); diff --git a/lib/ShareReview/ShareReviewAccessCheckEvent.php b/lib/ShareReview/ShareReviewAccessCheckEvent.php deleted file mode 100644 index b9c152404b..0000000000 --- a/lib/ShareReview/ShareReviewAccessCheckEvent.php +++ /dev/null @@ -1,43 +0,0 @@ -handled && !$this->granted) { - return; // a prior denyAccess() cannot be escalated to a grant — deny wins - } - $this->handled = true; - $this->granted = true; - } - - public function denyAccess(string $reason): void { - $this->handled = true; - $this->granted = false; - $this->reason = $reason; - } - - public function isHandled(): bool { - return $this->handled; - } - public function isGranted(): bool { - return $this->granted; - } - public function getReason(): ?string { - return $this->reason; - } -} diff --git a/lib/ShareReview/ShareReviewListener.php b/lib/ShareReview/ShareReviewListener.php index f8f8c8c0d3..3253eed05a 100644 --- a/lib/ShareReview/ShareReviewListener.php +++ b/lib/ShareReview/ShareReviewListener.php @@ -9,17 +9,17 @@ namespace OCA\Tables\ShareReview; -use OCA\ShareReview\Sources\SourceEvent; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; +use OCP\Share\ShareReview\RegisterShareReviewSourceEvent; -/** @template-implements IEventListener */ +/** @template-implements IEventListener */ class ShareReviewListener implements IEventListener { public function __construct() { } public function handle(Event $event): void { - if (!$event instanceof SourceEvent) { + if (!$event instanceof RegisterShareReviewSourceEvent) { return; } $event->registerSource(ShareReviewSource::class); diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index 1e8db4a0ad..8d873e42d2 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -9,7 +9,6 @@ namespace OCA\Tables\ShareReview; -use OCA\ShareReview\Sources\ISource; use OCA\Tables\Db\ContextMapper; use OCA\Tables\Db\ShareMapper; use OCA\Tables\Db\TableMapper; @@ -21,9 +20,11 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\IL10N; use OCP\Share\IShare; +use OCP\Share\ShareReview\Events\ShareReviewAccessCheckEvent; +use OCP\Share\ShareReview\IShareReviewSource; use Psr\Log\LoggerInterface; -class ShareReviewSource implements ISource { +class ShareReviewSource implements IShareReviewSource { private const NODE_TYPE_TABLE = 'table'; private const NODE_TYPE_VIEW = 'view'; @@ -93,7 +94,7 @@ public function deleteShare(string $shareId): bool { return false; } - $event = new ShareReviewAccessCheckEvent(); + $event = new ShareReviewAccessCheckEvent('Tables', $shareId); $this->eventDispatcher->dispatchTyped($event); if (!$event->isHandled() || !$event->isGranted()) { diff --git a/tests/stub.phpstub b/tests/stub.phpstub index a445f1fbf4..c5f28fdb82 100644 --- a/tests/stub.phpstub +++ b/tests/stub.phpstub @@ -15,18 +15,32 @@ namespace OCA\Analytics\Datasource { } } -namespace OCA\ShareReview\Sources { - class SourceEvent extends \OCP\EventDispatcher\Event { - abstract public function registerSource(string $source): void {} +namespace OCP\Share\ShareReview { + class RegisterShareReviewSourceEvent extends \OCP\EventDispatcher\Event { + public function registerSource(string $source): void {} + public function getSources(): array { return []; } } - interface ISource { + interface IShareReviewSource { public function getName(): string; public function getShares(): array; public function deleteShare(string $shareId): bool; } } +namespace OCP\Share\ShareReview\Events { + class ShareReviewAccessCheckEvent extends \OCP\EventDispatcher\Event { + public function __construct(string $sourceName, string $shareId) {} + public function getSourceName(): string { return ''; } + public function getShareId(): string { return ''; } + public function grantAccess(): void {} + public function denyAccess(string $reason): void {} + public function isHandled(): bool { return false; } + public function isGranted(): bool { return false; } + public function getReason(): ?string { return null; } + } +} + namespace OCA\Circles\Model { class Circle { abstract public function getSingleId(): string {} diff --git a/tests/unit/ShareReview/ShareReviewAccessCheckEventTest.php b/tests/unit/ShareReview/ShareReviewAccessCheckEventTest.php deleted file mode 100644 index aa9b6a9613..0000000000 --- a/tests/unit/ShareReview/ShareReviewAccessCheckEventTest.php +++ /dev/null @@ -1,58 +0,0 @@ -assertFalse($event->isHandled()); - $this->assertFalse($event->isGranted()); - $this->assertNull($event->getReason()); - } - - public function testGrantAccess(): void { - $event = new ShareReviewAccessCheckEvent(); - $event->grantAccess(); - - $this->assertTrue($event->isHandled()); - $this->assertTrue($event->isGranted()); - } - - public function testDenyAccess(): void { - $event = new ShareReviewAccessCheckEvent(); - $event->denyAccess('not in group'); - - $this->assertTrue($event->isHandled()); - $this->assertFalse($event->isGranted()); - $this->assertSame('not in group', $event->getReason()); - } - - public function testGrantThenDenyIsGrantedFalse(): void { - $event = new ShareReviewAccessCheckEvent(); - $event->grantAccess(); - $event->denyAccess('revoked'); - - $this->assertFalse($event->isGranted()); - $this->assertSame('revoked', $event->getReason()); - } - - public function testDenyThenGrantIsGrantedFalse(): void { - $event = new ShareReviewAccessCheckEvent(); - $event->denyAccess('not allowed'); - $event->grantAccess(); - - $this->assertFalse($event->isGranted()); - } -} diff --git a/tests/unit/ShareReview/ShareReviewSourceTest.php b/tests/unit/ShareReview/ShareReviewSourceTest.php index d37c29b311..e06759be73 100644 --- a/tests/unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/unit/ShareReview/ShareReviewSourceTest.php @@ -14,7 +14,6 @@ use OCA\Tables\Db\TableMapper; use OCA\Tables\Db\ViewMapper; use OCA\Tables\Service\ShareService; -use OCA\Tables\ShareReview\ShareReviewAccessCheckEvent; use OCA\Tables\ShareReview\ShareReviewSource; use OCP\AppFramework\Db\DoesNotExistException; use OCP\Constants; @@ -22,6 +21,7 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\IL10N; use OCP\Share\IShare; +use OCP\Share\ShareReview\Events\ShareReviewAccessCheckEvent; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; diff --git a/tests/unit/ShareReview/Stubs.php b/tests/unit/ShareReview/Stubs.php index 80ec8a5171..9c8c69e039 100644 --- a/tests/unit/ShareReview/Stubs.php +++ b/tests/unit/ShareReview/Stubs.php @@ -7,16 +7,81 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -namespace OCA\ShareReview\Sources; +namespace OCP\Share\ShareReview { -/** - * Runtime stub for the optional grc_sharereview app. - * Only loaded when the real app is not installed. - */ -interface ISource { - public function getName(): string; + /** + * Runtime stub for servers that do not ship the ShareReview OCP classes yet. + * Only loaded when the real classes are not available. + */ + interface IShareReviewSource { + public function getName(): string; + + public function getShares(): array; + + public function deleteShare(string $shareId): bool; + } + + class RegisterShareReviewSourceEvent extends \OCP\EventDispatcher\Event { + /** @var array> */ + private array $sources = []; + + public function registerSource(string $source): void { + $this->sources[] = $source; + } + + public function getSources(): array { + return $this->sources; + } + } +} + +namespace OCP\Share\ShareReview\Events { + + class ShareReviewAccessCheckEvent extends \OCP\EventDispatcher\Event { + private bool $handled = false; + private bool $granted = false; + private ?string $reason = null; + + public function __construct( + private readonly string $sourceName, + private readonly string $shareId, + ) { + parent::__construct(); + } + + public function getSourceName(): string { + return $this->sourceName; + } + + public function getShareId(): string { + return $this->shareId; + } + + public function grantAccess(): void { + if ($this->handled && !$this->granted) { + return; + } + $this->handled = true; + $this->granted = true; + } + + public function denyAccess(string $reason): void { + $this->handled = true; + $this->granted = false; + $this->reason = $reason; + $this->stopPropagation(); + } + + public function isHandled(): bool { + return $this->handled; + } - public function getShares(): array; + public function isGranted(): bool { + return $this->granted; + } - public function deleteShare(string $shareId): bool; + public function getReason(): ?string { + return $this->reason; + } + } } diff --git a/tests/unit/bootstrap.php b/tests/unit/bootstrap.php index c266666d22..139f6f45dd 100644 --- a/tests/unit/bootstrap.php +++ b/tests/unit/bootstrap.php @@ -23,7 +23,7 @@ require_once __DIR__ . '/Db/Row2MapperTestDependencies.php'; -if (!interface_exists('OCA\ShareReview\Sources\ISource')) { +if (!interface_exists('OCP\Share\ShareReview\IShareReviewSource')) { require_once __DIR__ . '/ShareReview/Stubs.php'; }