Conversation
| * @param 'tables'|'views' $nodeCollection Indicates whether to create a row on a table or view | ||
| * @param int $nodeId The identifier of the targeted table or view | ||
| * @param string|array<string, mixed> $data An array containing the column identifiers and their values | ||
| * @return DataResponse<Http::STATUS_OK, TablesRow, array{}>|DataResponse<Http::STATUS_FORBIDDEN|Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND|Http::STATUS_INTERNAL_SERVER_ERROR, array{message: string}, array{}> |
There was a problem hiding this comment.
The added field should be reflected in the response definition of TablesRow, cf. https://github.com/nextcloud/tables/blob/main/lib/ResponseDefinitions.php#L76-L84
lib/Db/Row2.php
Outdated
| /** | ||
| * return merged response-only metadata into the data cells. | ||
| */ |
There was a problem hiding this comment.
The method names says this already, no need to duplicate :)
What we want is however a @return annotation with the array shape. Or @psalm-return as done at jsonSerialize(), which itself would need to be adjusted afterwards.
Did you double check, cannot we only extend jsonSerialize() instead of adding a similar method?
There was a problem hiding this comment.
i was a bit hesitant on using 'jsonSerialize' as i thought i might interfere in another place, but did more testing and it's fine.
lib/Db/Row2.php
Outdated
| private ?string $lastEditBy = null; | ||
| private ?string $lastEditAt = null; | ||
| private ?array $data = []; | ||
| private array $cellMeta = []; |
There was a problem hiding this comment.
Should need to be added to a VIRTUAL_PROPERTIES const, otherwise the Mapper will complain. Have a look at https://github.com/nextcloud/tables/blob/main/lib/Db/Column.php#L161C2-L161C114 for comparison.
Also, the name is too broad and not specific (same problem with data already…), but right now I am also missing a better suggestion.
There was a problem hiding this comment.
ill rename it cellMetadata as the array holds metadata for each cell.
lib/Service/RowService.php
Outdated
| $fullRowData = []; | ||
| $columnNames = []; | ||
|
|
||
| foreach ($columns as $c) { |
There was a problem hiding this comment.
| foreach ($columns as $c) { | |
| foreach ($columns as $column) { |
Please don't save bytes where it is not necessary :)
lib/Service/RowService.php
Outdated
|
|
||
| $rows = $data instanceof RowDataInput ? iterator_to_array($data) : $data; | ||
|
|
||
| foreach ($rows as $r) { |
lib/Service/RowService.php
Outdated
| // attached columnname to returned row for the response only | ||
| foreach ($fullRowData as $meta) { | ||
| if (isset($meta['columnId']) && array_key_exists('columnName', $meta)) { | ||
| $insertedRow->addCellMeta((int)$meta['columnId'], ['columnName' => $meta['columnName']]); |
There was a problem hiding this comment.
can we generate it in Row2 instead? data is populated there. Having all logic in Row2 would capsulate it there and not leak logic to other classes around.
github-project-automation
bot
moved this to 🧭 Planning evaluation (don't pick)
in 📝 Office team
blizzz
moved this from 🧭 Planning evaluation (don't pick)
to 🏗️ In progress
in 📝 Office team
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
2 participants
This PR is a fix for #1840 and #2237.
Description:
Issues fixed: