-
Notifications
You must be signed in to change notification settings - Fork 36
#1424: Add Tables to user_migration #2292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| { | ||
| $item = new Column(); | ||
| $item->setTableId($table->getId()); | ||
| $item->setTitle($column['title']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure with the abstraction layers in the tables app but you could probably shorten this by using https://github.com/nextcloud/server/blob/master/lib/public/AppFramework/Db/Entity.php#L51-L62
$item = Column::fromRow($column);
Please verify though, setTableId may need separate handling still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliusknorr will do.
lib/Service/ContextService.php
Outdated
| $newContext->setName($context['name'] ?? ''); | ||
| $newContext->setIcon($context['iconName'] ?? ''); | ||
| $newContext->setDescription($context['description'] ?? ''); | ||
| $newContext->setOwnerId($context['owner'] ?? ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we import, do we set the old owner id? Or would be be needed to change this to the current user id of the importer instead? Same probably applies to other importers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliusknorr this was actually one of the final fixes im making, im going to actually check if the user exist and if not then use current logged in user, what do you think about that? or should we go straight with the current user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should always use the user that is importing as the new owner (as I assume only your own tables/context are exported). The current logged in user may not work as I assume it is importing in a background job?
But I would guess there is a way to get the "target" user as part of the import process of the user_migration app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
understandable, i'm currently working on that.
lib/TablesMigrator.php
Outdated
| public function __construct( | ||
| IL10N $l10n, | ||
| TableMapper $tableMapper, | ||
| ColumnMapper $columnMapper, | ||
| RowSleeveMapper $rowSleeveMapper, | ||
| ViewMapper $viewMapper, | ||
| ContextMapper $contextMapper, | ||
| ShareMapper $shareMapper, | ||
| ContextNodeRelationMapper $contextNodeRelationMapper, | ||
| FavoritesService $favoritesService, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ability to export/import user tables.