Skip to content

Commit c4e3538

Browse files
AndyScherzingerAndyScherzinger
committed
refactor(sharereview): address pre-review feedback on ShareReviewSource
- Remove 'app' key from getShares() return values; the ShareReview consumer already injects the app name from getName() - Add TABLE_NAME constants to AclMapper and BoardMapper so table names have a canonical owner and don't leak into unrelated classes - Move the ACL+board JOIN query to AclMapper::findAllForShareReview() and drop the IDBConnection dependency from ShareReviewSource - Introduce ShareReviewShare DTO; getShares() now builds typed objects via buildShare() and converts them with toArray() - Log a warning for unknown ACL participant types instead of silently defaulting to TYPE_USER - Localize user-facing strings in resolveObjectName() via IL10N Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
1 parent dbdce8e commit c4e3538

8 files changed

Lines changed: 424 additions & 421 deletions

File tree

‎lib/AppInfo/Application.php‎

Lines changed: 230 additions & 230 deletions
Large diffs are not rendered by default.

‎lib/Db/AclMapper.php‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,16 @@
1010
use OCP\AppFramework\Db\DoesNotExistException;
1111
use OCP\AppFramework\Db\Entity;
1212
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
13+
use OCP\DB\Exception;
1314
use OCP\DB\QueryBuilder\IQueryBuilder;
1415
use OCP\IDBConnection;
1516

1617
/** @template-extends DeckMapper<Acl> */
1718
class AclMapper extends DeckMapper implements IPermissionMapper {
19+
public const TABLE_NAME = 'deck_board_acl';
20+
1821
public function __construct(IDBConnection $db) {
19-
parent::__construct($db, 'deck_board_acl', Acl::class);
22+
parent::__construct($db, self::TABLE_NAME, Acl::class);
2023
}
2124

2225
public function findByAccessToken(string $accessToken) {
@@ -129,6 +132,29 @@ public function findByType(int $type): array {
129132
return $this->findEntities($qb);
130133
}
131134

135+
/**
136+
* Fetch all ACL rows with their board title and owner for ShareReview.
137+
*
138+
* @return list<array<string, mixed>>
139+
* @throws Exception
140+
*/
141+
public function findAllForShareReview(): array {
142+
$qb = $this->db->getQueryBuilder();
143+
$qb->select(
144+
'a.id', 'a.board_id', 'a.type', 'a.participant',
145+
'a.permission_edit', 'a.permission_share', 'a.permission_manage', 'a.created_at', 'a.last_modified_at'
146+
)
147+
->selectAlias('b.title', 'board_title')
148+
->selectAlias('b.owner', 'board_owner')
149+
->from(self::TABLE_NAME, 'a')
150+
->leftJoin('a', 'deck_boards', 'b', $qb->expr()->eq('a.board_id', 'b.id'))
151+
->orderBy('a.id', 'ASC');
152+
$result = $qb->executeQuery();
153+
$rows = $result->fetchAll();
154+
$result->closeCursor();
155+
return $rows;
156+
}
157+
132158
public function insert(Entity $entity): Entity {
133159
/** @var Acl $entity */
134160
$now = time();

‎lib/Db/BoardMapper.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
/** @template-extends QBMapper<Board> */
2222
class BoardMapper extends QBMapper implements IPermissionMapper {
23+
public const TABLE_NAME = 'deck_boards';
2324
/** @var CappedMemoryCache<Board[]> */
2425
private CappedMemoryCache $userBoardCache;
2526
/** @var CappedMemoryCache<Board> */
@@ -36,7 +37,7 @@ public function __construct(
3637
private ICloudIdManager $cloudIdManager,
3738
private LoggerInterface $logger,
3839
) {
39-
parent::__construct($db, 'deck_boards', Board::class);
40+
parent::__construct($db, self::TABLE_NAME, Board::class);
4041

4142
$this->userBoardCache = new CappedMemoryCache();
4243
$this->boardCache = new CappedMemoryCache();

‎lib/ShareReview/ShareReviewAccessCheckEvent.php‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,16 @@ public function grantAccess(): void {
2828
public function denyAccess(string $reason): void {
2929
$this->handled = true;
3030
$this->granted = false;
31-
$this->reason = $reason;
31+
$this->reason = $reason;
3232
}
3333

34-
public function isHandled(): bool { return $this->handled; }
35-
public function isGranted(): bool { return $this->granted; }
36-
public function getReason(): ?string { return $this->reason; }
34+
public function isHandled(): bool {
35+
return $this->handled;
36+
}
37+
public function isGranted(): bool {
38+
return $this->granted;
39+
}
40+
public function getReason(): ?string {
41+
return $this->reason;
42+
}
3743
}

‎lib/ShareReview/ShareReviewShare.php‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Deck\ShareReview;
11+
12+
class ShareReviewShare {
13+
public function __construct(
14+
private readonly int $id,
15+
private readonly string $object,
16+
private readonly string $initiator,
17+
private readonly int $type,
18+
private readonly string $recipient,
19+
private readonly int $permissions,
20+
private readonly string $time,
21+
) {
22+
}
23+
24+
/** @return array{id: int, object: string, initiator: string, type: int, recipient: string, permissions: int, password: bool, time: string, action: string} */
25+
public function toArray(): array {
26+
return [
27+
'id' => $this->id,
28+
'object' => $this->object,
29+
'initiator' => $this->initiator,
30+
'type' => $this->type,
31+
'recipient' => $this->recipient,
32+
'permissions' => $this->permissions,
33+
'password' => false,
34+
'time' => $this->time,
35+
'action' => '',
36+
];
37+
}
38+
}

‎lib/ShareReview/ShareReviewSource.php‎

Lines changed: 35 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -10,28 +10,27 @@
1010
namespace OCA\Deck\ShareReview;
1111

1212
use OCA\Deck\Db\Acl;
13+
use OCA\Deck\Db\AclMapper;
1314
use OCA\Deck\Service\BoardService;
1415
use OCA\ShareReview\Sources\ISource;
1516
use OCP\AppFramework\Db\DoesNotExistException;
1617
use OCP\Constants;
1718
use OCP\DB\Exception;
18-
use OCP\DB\QueryBuilder\IQueryBuilder;
1919
use OCP\EventDispatcher\IEventDispatcher;
20-
use OCP\IDBConnection;
20+
use OCP\IL10N;
2121
use OCP\Share\IShare;
2222
use Psr\Log\LoggerInterface;
2323

2424
class ShareReviewSource implements ISource {
2525

26-
private const ACL_TABLE = 'deck_board_acl';
27-
private const BOARDS_TABLE = 'deck_boards';
2826
private const PERMISSION_MANAGE = 32;
2927

3028
public function __construct(
31-
private IDBConnection $db,
32-
private LoggerInterface $logger,
29+
private readonly AclMapper $aclMapper,
30+
private readonly LoggerInterface $logger,
3331
private readonly BoardService $boardService,
3432
private readonly IEventDispatcher $eventDispatcher,
33+
private readonly IL10N $l,
3534
) {
3635
}
3736

@@ -40,27 +39,19 @@ public function getName(): string {
4039
}
4140

4241
/**
43-
* @return list<array{id: int, app: string, object: string, initiator: string, type: int, recipient: string, permissions: int, password: bool, time: string, action: string}>
42+
* @return list<array{id: int, object: string, initiator: string, type: int, recipient: string, permissions: int, password: bool, time: string, action: string}>
4443
*/
4544
public function getShares(): array {
46-
$rawShares = $this->fetchAllShares();
47-
$appName = $this->getName();
48-
$formatted = [];
49-
foreach ($rawShares as $share) {
50-
$formatted[] = [
51-
'id' => (int)$share['id'],
52-
'app' => $appName,
53-
'object' => $this->resolveObjectName($share),
54-
'initiator' => (string)$share['board_owner'],
55-
'type' => $this->mapParticipantType((int)$share['type']),
56-
'recipient' => (string)$share['participant'],
57-
'permissions' => $this->computePermissions($share),
58-
'password' => false,
59-
'time' => date('Y-m-d H:i:s', max((int)$share['created_at'], (int)$share['last_modified_at'])),
60-
'action' => '',
61-
];
45+
try {
46+
$rawShares = $this->aclMapper->findAllForShareReview();
47+
} catch (Exception $e) {
48+
$this->logger->error('Deck ShareReview: failed to fetch shares: {message}', ['message' => $e->getMessage()]);
49+
return [];
6250
}
63-
return $formatted;
51+
return array_map(
52+
fn (array $share) => $this->buildShare($share)->toArray(),
53+
$rawShares,
54+
);
6455
}
6556

6657
public function deleteShare(string $shareId): bool {
@@ -76,41 +67,32 @@ public function deleteShare(string $shareId): bool {
7667
}
7768

7869
try {
79-
$this->boardService->deleteAclForShareReview((int) $shareId);
70+
$this->boardService->deleteAclForShareReview((int)$shareId);
8071
return true;
8172
} catch (DoesNotExistException) {
8273
return false;
8374
}
8475
}
8576

86-
/** @return list<array<string, mixed>> */
87-
private function fetchAllShares(): array {
88-
try {
89-
$qb = $this->db->getQueryBuilder();
90-
$qb->select(
91-
'a.id', 'a.board_id', 'a.type', 'a.participant',
92-
'a.permission_edit', 'a.permission_share', 'a.permission_manage', 'a.created_at', 'a.last_modified_at'
93-
)
94-
->selectAlias('b.title', 'board_title')
95-
->selectAlias('b.owner', 'board_owner')
96-
->from(self::ACL_TABLE, 'a')
97-
->leftJoin('a', self::BOARDS_TABLE, 'b', $qb->expr()->eq('a.board_id', 'b.id'))
98-
->orderBy('a.id', 'ASC');
99-
$result = $qb->executeQuery();
100-
$rows = $result->fetchAll();
101-
$result->closeCursor();
102-
return $rows;
103-
} catch (Exception $e) {
104-
$this->logger->error('Deck ShareReview: failed to fetch shares: {message}', ['message' => $e->getMessage()]);
105-
return [];
106-
}
77+
/** @param array<string, mixed> $share */
78+
private function buildShare(array $share): ShareReviewShare {
79+
return new ShareReviewShare(
80+
id: (int)$share['id'],
81+
object: $this->resolveObjectName($share),
82+
initiator: (string)$share['board_owner'],
83+
type: $this->mapParticipantType((int)$share['type']),
84+
recipient: (string)$share['participant'],
85+
permissions: $this->computePermissions($share),
86+
time: date('Y-m-d H:i:s', max((int)$share['created_at'], (int)$share['last_modified_at'])),
87+
);
10788
}
10889

10990
/** @param array<string, mixed> $share */
11091
private function resolveObjectName(array $share): string {
11192
$title = (string)($share['board_title'] ?? '');
11293
$boardId = (int)($share['board_id'] ?? $share['id']);
113-
return ($title !== '' ? $title : "Board $boardId") . ' (Board)';
94+
$label = $title !== '' ? $title : $this->l->t('Board %d', [$boardId]);
95+
return $this->l->t('%s (Board)', [$label]);
11496
}
11597

11698
private function mapParticipantType(int $type): int {
@@ -119,10 +101,15 @@ private function mapParticipantType(int $type): int {
119101
Acl::PERMISSION_TYPE_GROUP => IShare::TYPE_GROUP,
120102
Acl::PERMISSION_TYPE_REMOTE => IShare::TYPE_REMOTE,
121103
Acl::PERMISSION_TYPE_CIRCLE => IShare::TYPE_CIRCLE,
122-
default => IShare::TYPE_USER,
104+
default => $this->fallbackParticipantType($type),
123105
};
124106
}
125107

108+
private function fallbackParticipantType(int $type): int {
109+
$this->logger->warning('Deck ShareReview: unknown ACL participant type {type}, defaulting to user share', ['type' => $type]);
110+
return IShare::TYPE_USER;
111+
}
112+
126113
/** @param array<string, mixed> $share */
127114
private function computePermissions(array $share): int {
128115
$permissions = Constants::PERMISSION_READ;

‎tests/bootstrap.php‎

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,30 @@
1-
<?php
2-
3-
/**
4-
* @copyright Copyright (c) 2016 Julius Härtl <jus@bitgrid.net>
5-
*
6-
* @author Julius Härtl <jus@bitgrid.net>
7-
*
8-
* @license GNU AGPL version 3 or any later version
9-
*
10-
* This program is free software: you can redistribute it and/or modify
11-
* it under the terms of the GNU Affero General Public License as
12-
* published by the Free Software Foundation, either version 3 of the
13-
* License, or (at your option) any later version.
14-
*
15-
* This program is distributed in the hope that it will be useful,
16-
* but WITHOUT ANY WARRANTY; without even the implied warranty of
17-
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
18-
* GNU Affero General Public License for more details.
19-
*
20-
* You should have received a copy of the GNU Affero General Public License
21-
* along with this program. If not, see <http://www.gnu.org/licenses/>.
22-
*
23-
*/
24-
25-
require_once __DIR__ . '/../../../tests/bootstrap.php';
26-
require_once __DIR__ . '/../appinfo/autoload.php';
27-
28-
if (!interface_exists('OCA\ShareReview\Sources\ISource')) {
29-
require_once __DIR__ . '/unit/ShareReview/Stubs.php';
30-
}
1+
<?php
2+
3+
/**
4+
* @copyright Copyright (c) 2016 Julius Härtl <jus@bitgrid.net>
5+
*
6+
* @author Julius Härtl <jus@bitgrid.net>
7+
*
8+
* @license GNU AGPL version 3 or any later version
9+
*
10+
* This program is free software: you can redistribute it and/or modify
11+
* it under the terms of the GNU Affero General Public License as
12+
* published by the Free Software Foundation, either version 3 of the
13+
* License, or (at your option) any later version.
14+
*
15+
* This program is distributed in the hope that it will be useful,
16+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
17+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
18+
* GNU Affero General Public License for more details.
19+
*
20+
* You should have received a copy of the GNU Affero General Public License
21+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
22+
*
23+
*/
24+
25+
require_once __DIR__ . '/../../../tests/bootstrap.php';
26+
require_once __DIR__ . '/../appinfo/autoload.php';
27+
28+
if (!interface_exists('OCA\ShareReview\Sources\ISource')) {
29+
require_once __DIR__ . '/unit/ShareReview/Stubs.php';
30+
}

0 commit comments

Comments
 (0)