Table scheme import - #2888
Conversation
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
b73b4af to
2c9d442
Compare
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
blizzz
left a comment
blizzz
left a comment
There was a problem hiding this comment.
Early comments, some can be left for later.
UI wise it looks really nice already!
-
I would not display the UUID as title in the check box. It looks technical and table managers will have not deeper knowledge about it. Would completely keeps this identifier internal and instead use the New Title.
-
In my quick test I also see that Views would be dropped instead of being kept
-
Maybe need to double check the sort order as well when nothing is defined
This is very promising already! :)
| $qb->setParameter('uuid', Uuid::v7()->toRfc4122()); | ||
| $qb->executeStatement(); | ||
| } | ||
| $this->connection->commit(); |
There was a problem hiding this comment.
In Version2020Date20260513185340 I commit and start a new transaction every 250 writes for performance reasons. Could be an improvement here as well, beneficiary on updates on instances with many tables. But will work, can be addressed later.
| * 404: Not found | ||
| */ | ||
| #[NoAdminRequired] | ||
| #[RequirePermission(Application::PERMISSION_MANAGE, null, 'context', 'contextId')] |
There was a problem hiding this comment.
nitpick, also can be done in a follow up, just for readability it think this is nicer:
| #[RequirePermission(Application::PERMISSION_MANAGE, null, 'context', 'contextId')] | |
| #[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')] |
applies to the other similar lines as well.
(yes it was used somewhere like this here before, that can also be improved)
Again, not a blocker, by-catch, good enough with follow-up.
| */ | ||
| #[NoAdminRequired] | ||
| #[RequirePermission(Application::PERMISSION_MANAGE, null, 'context', 'contextId')] | ||
| public function importScheme(int $contextId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $tables): DataResponse { |
There was a problem hiding this comment.
Node sure I understand correctly the role of $nodes vs $tables. Maybe have an idea with the TODO against the nodes.
| foreach ($columns as $column) { | ||
| if (isset($column['uuid'])) { | ||
| try { | ||
| $existColumn = $this->mapper->findByUuid($column['uuid']); |
There was a problem hiding this comment.
Is fetching by UUID important here? My notion is, as with the StructureService, to create a diff between the incoming and existing column and then add, modify (or keep), and remove. This requires checking what is there, then we do need to fetch by UUID (which done one-by-one as also rather expensive, and we have intentionally no index there).
| use OCA\Tables\Errors\NotFoundError; | ||
| use OCA\Tables\Errors\PermissionError; | ||
|
|
||
| class StructureService { |
There was a problem hiding this comment.
I know it comes from me, just to note down one thought that circles my mind is whether to add Stateful to the class name for safer usage 🤔 Again, nothing that cannot be improved upon later on.
| $columns = $this->columnService->findAllByTable($id, null, $table); | ||
| $this->enhanceTable($table, $userId); | ||
| return new TableScheme($table->getTitle(), $table->getEmoji(), $columns, $table->getViews() ?: [], $table->getDescription() ?: '', $this->appManager->getAppVersion('tables'), $table->getColumnOrderSettingsArray(), $table->getSortArray()); | ||
| $this->enhanceTable($table, $userId ?? $this->userId); |
There was a problem hiding this comment.
i suppose there is a reason for this line change? ^
| private function handleViewPersistDbException(\OCP\DB\Exception $e, string $context): never { | ||
| if ($e->getReason() === \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { | ||
| throw new BadRequestError('Technical name must be unique in the table.'); | ||
| throw new BadRequestError('Technical name must be unique in the table. ' . $e->getMessage()); |
2 participants
🖼️ Screenshots
🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)