-
Notifications
You must be signed in to change notification settings - Fork 24
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
Feat(Views): Manage data presentation of tables by views #426
Conversation
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.
Some early comments on the first batch of files I went over. Overall this looks super nice already :)
['name' => 'api1#getColumn', 'url' => '/api/1/columns/{columnId}', 'verb' => 'GET'], | ||
['name' => 'api1#deleteColumn', 'url' => '/api/1/columns/{columnId}', 'verb' => 'DELETE'], | ||
['name' => 'api1#updateColumn', 'url' => '/api/1/columns/{columnId}', 'verb' => 'PUT'], | ||
// -> rows -> table | ||
['name' => 'api1#indexTableRowsSimple', 'url' => '/api/1/tables/{tableId}/rows/simple', 'verb' => 'GET'], | ||
['name' => 'api1#indexTableRows', 'url' => '/api/1/tables/{tableId}/rows', 'verb' => 'GET'], |
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.
Not sure if we may need to keep a compatibility endpoint (for this and other dropped ones) to just point to the base view then, as I know @stefan-niedermann is already using this API for an Android client in the works ;)
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.
@juliushaertl if you can give me a list of dropped endpoints I can tell you which one are problematic for the Tables Android 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.
APIs used by the Tables Android App 1.1.2
:
GET /tables
POST /tables
DELETE /tables/{tableId}
PUT /tables/{tableId}
GET /tables/{tableId}/columns
POST /tables/{tableId}/columns
PUT /columns/{columnId}
DELETE /columns/{columnId}
GET /tables/{tableId}/rows <-- Dropped?
POST /tables/{tableId}/rows
PUT /rows/{rowId}
DELETE /rows/{rowId}
If one of them needs to get dropped, it would be awesome to maintain backward compatibility for at least one major version of the Tables server app while announcing deprecations for the APIs that shall be dropped, so 3rd party users have some time to adapt the changes.
Looking forward to a list of APIs that will be dropped and which replacements will be there 🙂
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.
@juliushaertl / @datenangebot ping 🙂
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.
Given the PR has been merged, I assume that there are breaking changes in the API? Are the differences documented or announced somewhere?
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.
Hi, all routes are back and should work as before, no breaking changes.
(Some new routes are not yet documented: #452)
lib/Controller/Api1Controller.php
Outdated
if ($keyword) { | ||
return $this->handleError(function () use ($keyword, $limit, $offset) { | ||
return $this->viewService->search($keyword, $limit, $offset); | ||
}); | ||
} else { | ||
return $this->handleError(function () use ($tableId) { | ||
return $this->viewService->findAll($this->tableService->find($tableId)); | ||
}); | ||
} | ||
|
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.
Might be personal preference but I feel something like this would be easier to read and saves the extra else branch:
if ($keyword) { | |
return $this->handleError(function () use ($keyword, $limit, $offset) { | |
return $this->viewService->search($keyword, $limit, $offset); | |
}); | |
} else { | |
return $this->handleError(function () use ($tableId) { | |
return $this->viewService->findAll($this->tableService->find($tableId)); | |
}); | |
} | |
return $this->handleError(function () use ($keyword, $limit, $offset) use (keyword, $limit, $offset) { | |
if ($keyword !== null) { | |
return $this->viewService->search($keyword, $limit, $offset); | |
} | |
return $this->viewService->findAll($this->tableService->find($tableId)); | |
}); |
Null check would also be safer to do a strict comparison as it allows to use the limit/offset with an empty search term. In gerneral findAll and search could probably be combined in one method.
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.
Actually looking at it again, i find it odd that search ignores the tableId
$table->addColumn('filter', \Doctrine\DBAL\Types\Types::JSON, [ | ||
'notnull' => false, | ||
]); | ||
$table->setPrimaryKey(['id']); |
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 carefully review the queries at some point and see where it would make sense to add indices to the tables
Hello there, We hope that the reviewing 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 reviewing 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! |
Great job, thank you 👍 |
interface IColumnTypeQB { | ||
public const DB_PLATFORM_MYSQL = 0; | ||
public const DB_PLATFORM_PGSQL = 1; | ||
public const DB_PLATFORM_SQLITE = 2; |
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.
Please add another constant for Oracle DB, usually called oci
in our code.
Currently, because MySQL is the else in formatCellValue
, it would use the MySQL dialect, which is not going to work 😞
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.
Oracle is not yet supported. No plans for it yet.
Install shouln't be possible on oci db backends...
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.
Lucky you 😃
46c9378
to
d93d3d5
Compare
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
Signed-off-by: Philipp Hempel <[email protected]>
remove from dashboard view Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
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.
Great changes, need some refactorings later on, but fine for now.
5700900
to
58e0c6e
Compare
Signed-off-by: Florian Steffens <[email protected]>
Signed-off-by: Florian Steffens <[email protected]>
Views are tables that contain and display certain data of the underlying data table. Views allow you to select the columns to display and their order, filter and sort the rows. Combined with functions such as sharing with specific permissions and more, this enables the creation of workflows.
View related
Unrelated but still solved