Skip to content

Commit 2983cf5

Browse files
AndyScherzingerAndyScherzinger
committed
fix(tables): address review comments
AI-assistant: Claude Code v2.1.117 (Claude Sonnet 4.6) Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
1 parent 5732e49 commit 2983cf5

13 files changed

Lines changed: 222 additions & 101 deletions

‎lib/Controller/ApiTablesController.php‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -187,18 +187,18 @@ public function createFromScheme(string $title, string $emoji, string $descripti
187187
$colMap[$column['id']] = $col->getId();
188188
}
189189
if (!empty($columnOrder) || !empty($sort)) {
190-
$remappedColumnOrder = !empty($columnOrder) ? array_map(static function (array $entry) use ($colMap): array {
190+
$remappedColumnOrder = !empty($columnOrder) ? ColumnSettings::createFromInputArray(array_map(static function (array $entry) use ($colMap): array {
191191
if (isset($entry['columnId']) && $entry['columnId'] > 0) {
192192
$entry['columnId'] = $colMap[$entry['columnId']] ?? $entry['columnId'];
193193
}
194194
return $entry;
195-
}, $columnOrder) : null;
196-
$remappedSort = !empty($sort) ? array_map(static function (array $entry) use ($colMap): array {
195+
}, $columnOrder)) : null;
196+
$remappedSort = !empty($sort) ? SortRuleSet::createFromInputArray(array_map(static function (array $entry) use ($colMap): array {
197197
if (isset($entry['columnId']) && $entry['columnId'] > 0) {
198198
$entry['columnId'] = $colMap[$entry['columnId']] ?? $entry['columnId'];
199199
}
200200
return $entry;
201-
}, $sort) : null;
201+
}, $sort)) : null;
202202
$table = $this->service->update($table->getId(), null, null, null, null, $this->userId, $remappedColumnOrder, $remappedSort);
203203
}
204204
foreach ($views as $view) {
@@ -315,23 +315,19 @@ public function update(int $id, ?string $title = null, ?string $emoji = null, ?s
315315
$sort = json_decode($sort, true) ?? null;
316316
}
317317
try {
318-
if ($columnSettings !== null) {
319-
if (!is_array($columnSettings)) {
320-
throw new \InvalidArgumentException('Invalid columnSettings: must be a JSON array');
321-
}
322-
ColumnSettings::createFromInputArray($columnSettings);
318+
if ($columnSettings !== null && !is_array($columnSettings)) {
319+
throw new \InvalidArgumentException('Invalid columnSettings: must be a JSON array');
323320
}
324-
if ($sort !== null) {
325-
if (!is_array($sort)) {
326-
throw new \InvalidArgumentException('Invalid sort: must be a JSON array');
327-
}
328-
SortRuleSet::createFromInputArray($sort);
321+
if ($sort !== null && !is_array($sort)) {
322+
throw new \InvalidArgumentException('Invalid sort: must be a JSON array');
329323
}
324+
$columnSettingsObj = $columnSettings !== null ? ColumnSettings::createFromInputArray($columnSettings) : null;
325+
$sortObj = $sort !== null ? SortRuleSet::createFromInputArray($sort) : null;
330326
} catch (\InvalidArgumentException $e) {
331327
return new DataResponse(['message' => $e->getMessage()], Http::STATUS_BAD_REQUEST);
332328
}
333329
try {
334-
return new DataResponse($this->service->update($id, $title, $emoji, $description, $archived, $this->userId, $columnSettings, $sort)->jsonSerialize());
330+
return new DataResponse($this->service->update($id, $title, $emoji, $description, $archived, $this->userId, $columnSettingsObj, $sortObj)->jsonSerialize());
335331
} catch (PermissionError $e) {
336332
return $this->handlePermissionError($e);
337333
} catch (\InvalidArgumentException $e) {

‎lib/Controller/TableController.php‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -80,19 +80,17 @@ public function update(int $id, ?string $title = null, ?string $emoji = null, ?b
8080
$sort = json_decode($sort, true) ?? null;
8181
}
8282
return $this->handleError(function () use ($id, $title, $emoji, $archived, $columnSettings, $sort) {
83-
if ($columnSettings !== null) {
84-
if (!is_array($columnSettings)) {
85-
throw new \InvalidArgumentException('Invalid columnSettings: must be a JSON array');
86-
}
87-
ColumnSettings::createFromInputArray($columnSettings);
83+
if ($columnSettings !== null && !is_array($columnSettings)) {
84+
throw new \InvalidArgumentException('Invalid columnSettings: must be a JSON array');
8885
}
89-
if ($sort !== null) {
90-
if (!is_array($sort)) {
91-
throw new \InvalidArgumentException('Invalid sort: must be a JSON array');
92-
}
93-
SortRuleSet::createFromInputArray($sort);
86+
if ($sort !== null && !is_array($sort)) {
87+
throw new \InvalidArgumentException('Invalid sort: must be a JSON array');
9488
}
95-
return $this->service->update($id, $title, $emoji, null, $archived, $this->userId, $columnSettings, $sort);
89+
return $this->service->update(
90+
$id, $title, $emoji, null, $archived, $this->userId,
91+
$columnSettings !== null ? ColumnSettings::createFromInputArray($columnSettings) : null,
92+
$sort !== null ? SortRuleSet::createFromInputArray($sort) : null,
93+
);
9694
});
9795
}
9896
}

‎lib/Db/Table.php‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
use OCA\Tables\Model\Permissions;
1212
use OCA\Tables\Model\SortRuleSet;
1313
use OCA\Tables\ResponseDefinitions;
14-
use OCA\Tables\Service\ValueObject\ViewColumnInformation;
14+
use OCA\Tables\Service\ValueObject\ColumnOrderInformation;
1515

1616
/**
1717
* @psalm-suppress PropertyNotSetInConstructor
@@ -44,7 +44,7 @@
4444
* @method setRowsCount(int $rowsCount)
4545
* @method getColumnsCount(): int
4646
* @method setColumnsCount(int $columnsCount)
47-
* @method getViews(): ?array
47+
* @method getViews(): ?array<TablesView>
4848
* @method setViews(array $views)
4949
* @method getColumns(): array
5050
* @method setColumns(array $columns)
@@ -109,14 +109,17 @@ public function jsonSerialize(): array {
109109
'lastEditAt' => $this->lastEditAt ?: '',
110110
'archived' => $this->archived,
111111
'isShared' => (bool)$this->isShared,
112-
'favorite' => $this->favorite,
112+
'favorite' => (bool)$this->favorite,
113113
'onSharePermissions' => $this->getSharePermissions()?->jsonSerialize(),
114114
'hasShares' => (bool)$this->hasShares,
115115
'rowsCount' => $this->rowsCount ?: 0,
116116
'columnsCount' => $this->columnsCount ?: 0,
117-
'views' => $this->getViewsArray(),
118-
'description' => $this->description ?:'',
119-
'columnOrder' => $this->getColumnOrderSettingsArray(),
117+
'description' => $this->description ?: '',
118+
'views' => array_values($this->getViewsArray()),
119+
'columnOrder' => array_map(
120+
static fn (ColumnOrderInformation $c) => ['columnId' => $c->getId(), 'order' => $c->getOrder()],
121+
$this->getColumnOrderSettingsArray()
122+
),
120123
'sort' => $this->getSortArray(),
121124
];
122125
}
@@ -126,27 +129,26 @@ private function getSharePermissions(): ?Permissions {
126129
}
127130

128131
/**
129-
* @psalm-suppress MismatchingDocblockReturnType
130132
* @return TablesView[]
131133
*/
132134
private function getViewsArray(): array {
133135
return $this->getViews() ?: [];
134136
}
135137

136138
/**
137-
* @psalm-suppress MismatchingDocblockReturnType
138139
* @return int[]
139140
*/
140141
public function getColumnOrderArray(): array {
141142
$columnSettings = $this->getColumnOrderSettingsArray();
142-
usort($columnSettings, static function (ViewColumnInformation $a, ViewColumnInformation $b) {
143+
usort($columnSettings, static function (ColumnOrderInformation $a, ColumnOrderInformation $b) {
143144
return $a->getOrder() - $b->getOrder();
144145
});
145-
return array_map(static fn (ViewColumnInformation $vci): int => $vci->getId(), $columnSettings);
146+
/** @var list<ColumnOrderInformation> $columnSettings */
147+
return array_map(static fn (ColumnOrderInformation $vci): int => $vci->getId(), $columnSettings);
146148
}
147149

148150
/**
149-
* @return array<ViewColumnInformation>
151+
* @return list<ColumnOrderInformation>
150152
*/
151153
public function getColumnOrderSettingsArray(): array {
152154
$columns = $this->getArray($this->getColumnOrder());
@@ -155,18 +157,17 @@ public function getColumnOrderSettingsArray(): array {
155157
}
156158

157159
if (is_array($columns[array_key_first($columns)] ?? null)) {
158-
return array_values(array_map(static fn (array $a): ViewColumnInformation => ViewColumnInformation::fromArray($a), $columns));
160+
return array_values(array_map(static fn (array $a): ColumnOrderInformation => ColumnOrderInformation::fromArray($a), $columns));
159161
}
160162

161163
$result = [];
162164
foreach ($columns as $index => $columnId) {
163-
$result[] = new ViewColumnInformation($columnId, order: (int)$index + 1);
165+
$result[] = new ColumnOrderInformation((int)$columnId, order: (int)$index + 1);
164166
}
165167
return $result;
166168
}
167169

168170
/**
169-
* @psalm-suppress MismatchingDocblockReturnType
170171
* @return list<array{columnId: int, mode: 'ASC'|'DESC'}>
171172
*/
172173
public function getSortArray(): array {
@@ -176,9 +177,8 @@ public function getSortArray(): array {
176177

177178
private function getArray(?string $json): array {
178179
if ($json !== '' && $json !== null && $json !== 'null') {
179-
return \json_decode($json, true);
180-
} else {
181-
return [];
180+
return \json_decode($json, true) ?? [];
182181
}
182+
return [];
183183
}
184184
}

‎lib/Model/ColumnSettings.php‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,19 @@
1111

1212
use Generator;
1313
use JsonSerializable;
14+
use OCA\Tables\Service\ValueObject\ColumnOrderInformation;
1415
use OCA\Tables\Service\ValueObject\ViewColumnInformation;
1516

1617
class ColumnSettings implements JsonSerializable {
1718
/**
18-
* @param ViewColumnInformation[] $columnSettings
19+
* @param ColumnOrderInformation[] $columnSettings
1920
*/
2021
public function __construct(
2122
protected array $columnSettings,
2223
) {
2324
foreach ($this->columnSettings as $columnSetting) {
24-
if (!$columnSetting instanceof ViewColumnInformation) {
25-
throw new \InvalidArgumentException('Provided column settings must be of type ViewColumnInformation');
25+
if (!$columnSetting instanceof ColumnOrderInformation) {
26+
throw new \InvalidArgumentException('Provided column settings must be of type ColumnOrderInformation');
2627
}
2728
}
2829
}
@@ -33,7 +34,24 @@ public function columnInformation(): Generator {
3334
}
3435
}
3536

37+
/**
38+
* Creates column settings with only columnId and order (for table use).
39+
*/
3640
public static function createFromInputArray(array $inputColumnSettings): self {
41+
$columnSettings = [];
42+
foreach ($inputColumnSettings as $inputColumnSetting) {
43+
if (!is_array($inputColumnSetting)) {
44+
throw new \InvalidArgumentException('Each column settings entry must be an array');
45+
}
46+
$columnSettings[] = ColumnOrderInformation::fromArray($inputColumnSetting);
47+
}
48+
return new self($columnSettings);
49+
}
50+
51+
/**
52+
* Creates column settings with view-specific fields (columnId, order, readonly, mandatory).
53+
*/
54+
public static function createViewSettingsFromInputArray(array $inputColumnSettings): self {
3755
$columnSettings = [];
3856
foreach ($inputColumnSettings as $inputColumnSetting) {
3957
if (!is_array($inputColumnSetting)) {
@@ -45,6 +63,6 @@ public static function createFromInputArray(array $inputColumnSettings): self {
4563
}
4664

4765
public function jsonSerialize(): array {
48-
return array_map(static fn (ViewColumnInformation $vci) => $vci->jsonSerialize(), $this->columnSettings);
66+
return array_map(static fn (ColumnOrderInformation $vci) => $vci->jsonSerialize(), $this->columnSettings);
4967
}
5068
}

‎lib/Model/SortRuleSet.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ public static function createFromInputArray(array $data): self {
5050
return new self($sortRules);
5151
}
5252

53+
/**
54+
* @return list<array{columnId: int, mode: 'ASC'|'DESC'}>
55+
*/
5356
public function jsonSerialize(): array {
5457
return array_map(static fn (SortRule $s) => $s->jsonSerialize(), $this->sortRules);
5558
}

‎lib/Model/ViewUpdateInput.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ public static function fromInputArray(array $data): self {
8484
title: ($data['title'] ?? null) ? new Title($data['title']) : null,
8585
description: $data['description'] ?? null,
8686
emoji: ($data['emoji'] ?? null) ? new Emoji($data['emoji']) : null,
87-
columnSettings: ($data['columnSettings'] ?? null) ? ColumnSettings::createFromInputArray($data['columnSettings']) : null,
87+
columnSettings: ($data['columnSettings'] ?? null) ? ColumnSettings::createViewSettingsFromInputArray($data['columnSettings']) : null,
8888
filterSet: ($data['filter'] ?? null) ? FilterSet::createFromInputArray($data['filter']) : null,
8989
sortRuleSet: ($data['sort'] ?? null) ? SortRuleSet::createFromInputArray($data['sort']) : null,
9090
);

‎lib/ResponseDefinitions.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,10 @@
6464
* },
6565
* hasShares: bool,
6666
* rowsCount: int,
67+
* description: string,
6768
* views: list<TablesView>,
6869
* columnsCount: int,
69-
* columnOrder: list<array{columnId: int, order: int, readonly: bool}>,
70+
* columnOrder: list<array{columnId: int, order: int}>,
7071
* sort: list<array{columnId: int, mode: 'ASC'|'DESC'}>,
7172
* }
7273
*

‎lib/Service/TableService.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ public function delete(int $id, ?string $userId = null): Table {
476476
* @throws NotFoundError
477477
* @throws PermissionError
478478
*/
479-
public function update(int $id, ?string $title, ?string $emoji, ?string $description, ?bool $archived = null, ?string $userId = null, ?array $columnSettings = null, ?array $sort = null): Table {
479+
public function update(int $id, ?string $title, ?string $emoji, ?string $description, ?bool $archived = null, ?string $userId = null, ?ColumnSettings $columnSettings = null, ?SortRuleSet $sort = null): Table {
480480
$userId = $this->permissionsService->preCheckUserId($userId);
481481

482482
try {
@@ -509,10 +509,10 @@ public function update(int $id, ?string $title, ?string $emoji, ?string $descrip
509509
$table->setDescription($description);
510510
}
511511
if ($columnSettings !== null) {
512-
$table->setColumnOrder(\json_encode(ColumnSettings::createFromInputArray($columnSettings)->jsonSerialize()));
512+
$table->setColumnOrder(\json_encode($columnSettings->jsonSerialize()));
513513
}
514514
if ($sort !== null) {
515-
$table->setSort(\json_encode(SortRuleSet::createFromInputArray($sort)->jsonSerialize()));
515+
$table->setSort(\json_encode($sort->jsonSerialize()));
516516
}
517517
$table->setLastEditBy($userId);
518518
$table->setLastEditAt($time->format('Y-m-d H:i:s'));

‎lib/Service/ValueObject/ColumnOrderInformation.php‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
9+
namespace OCA\Tables\Service\ValueObject;
10+
11+
use ArrayAccess;
12+
use JsonSerializable;
13+
14+
/**
15+
* Value object representing the column id and display order for a table.
16+
* This is the base type for table column ordering; ViewColumnInformation
17+
* extends it with view-specific fields (readonly, mandatory).
18+
*
19+
* @template-implements ArrayAccess<string, bool|int>
20+
*/
21+
class ColumnOrderInformation implements ArrayAccess, JsonSerializable {
22+
public const KEY_ID = 'columnId';
23+
public const KEY_ORDER = 'order';
24+
25+
protected array $data = [];
26+
protected const KEYS = [
27+
self::KEY_ID,
28+
self::KEY_ORDER,
29+
];
30+
31+
public function __construct(int $columnId, int $order) {
32+
$this->offsetSet(self::KEY_ID, $columnId);
33+
$this->offsetSet(self::KEY_ORDER, $order);
34+
}
35+
36+
public function getId(): int {
37+
return (int)$this->offsetGet(self::KEY_ID);
38+
}
39+
40+
public function getOrder(): int {
41+
return (int)$this->offsetGet(self::KEY_ORDER);
42+
}
43+
44+
public static function fromArray(array $data): self {
45+
if (!isset($data[self::KEY_ID], $data[self::KEY_ORDER])) {
46+
throw new \InvalidArgumentException('Column settings entry is missing required fields: columnId and order are required');
47+
}
48+
return new self((int)$data[self::KEY_ID], (int)$data[self::KEY_ORDER]);
49+
}
50+
51+
public function offsetExists(mixed $offset): bool {
52+
return in_array((string)$offset, static::KEYS);
53+
}
54+
55+
public function offsetGet(mixed $offset): bool|int {
56+
return $this->data[$offset];
57+
}
58+
59+
public function offsetSet(mixed $offset, mixed $value): void {
60+
if (!$this->offsetExists($offset)) {
61+
return;
62+
}
63+
$this->data[$offset] = $this->ensureType((string)$offset, $value);
64+
}
65+
66+
public function offsetUnset(mixed $offset): void {
67+
if (!$this->offsetExists($offset)) {
68+
return;
69+
}
70+
unset($this->data[(string)$offset]);
71+
}
72+
73+
/**
74+
* @return array{columnId: int, order: int, ...}
75+
*/
76+
public function jsonSerialize(): array {
77+
return [
78+
self::KEY_ID => $this->getId(),
79+
self::KEY_ORDER => $this->getOrder(),
80+
];
81+
}
82+
83+
protected function ensureType(string $offset, mixed $value): int|bool {
84+
return match ($offset) {
85+
self::KEY_ID,
86+
self::KEY_ORDER => (int)$value,
87+
default => throw new \InvalidArgumentException("Invalid offset: $offset"),
88+
};
89+
}
90+
}

‎lib/Service/ValueObject/SortRule.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ public function __construct(
2222
}
2323
}
2424

25+
/**
26+
* @return array{columnId: int, mode: 'ASC'|'DESC'}
27+
*/
2528
public function jsonSerialize(): array {
2629
return [
2730
'columnId' => $this->columnId,

0 commit comments

Comments
 (0)