-
Notifications
You must be signed in to change notification settings - Fork 281
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
Add JSON API #271
Add JSON API #271
Conversation
Codecov Report
@@ Coverage Diff @@
## master #271 +/- ##
==========================================
- Coverage 77.21% 73.87% -3.35%
==========================================
Files 38 35 -3
Lines 1163 1018 -145
==========================================
- Hits 898 752 -146
- Misses 265 266 +1 |
@srbaker Sorry for the long time without a reply. As you may have recognized, it has been silent around deck in the last weeks. I'll make sure to review this during the weekend so we can go on with that. |
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.
Looks good in general. 👍 I've added some comments to the the code.
In general please make sure to indent the files by tab not spaces.
* Create a stack with the specified title and order. | ||
*/ | ||
public function create($boardId, $title, $order) { | ||
// this throws a StatusException that needs to be caught and handled |
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 catch StatusExceptions in the "SharingMiddleware" (although the Sharing is not part of that anymore, so it might need some rename).
deck/lib/Middleware/SharingMiddleware.php
Line 61 in 15e5a43
public function afterException($controller, $methodName, \Exception $exception) { |
You can either add some logic to return proper error responses there, or exclude any exceptions from ApiControllers there and add the handling to the controller.
public function get($id) { | ||
$board = $this->service->find($id); | ||
|
||
// FIXME: this should probably 404 if the board has been deleted |
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.
It should be a 200 if it got deleted and a 404 if the entity for $id is not found. Otherwise you cannot separate between a successful deletion and a failing one, because the entity does not exist.
return new DataResponse($board); | ||
} | ||
|
||
// this is taken from BoardController, but it's not ideal |
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.
You can move the method to the BoardService class, then we can use it from both controllers.
See:
deck/lib/Controller/BoardController.php
Line 53 in 2c63bfb
* TODO: move to boardservice |
appinfo/routes.php
Outdated
@@ -62,5 +62,18 @@ | |||
['name' => 'label#update', 'url' => '/labels/{labelId}', 'verb' => 'PUT'], | |||
['name' => 'label#delete', 'url' => '/labels/{labelId}', 'verb' => 'DELETE'], | |||
|
|||
// api |
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.
indentation seems wrong here 😉
Thanks! I've also been a bit distracted by other things; I'll plan to address these this week, and finish up the last bits. I'm hoping to use this to get a mobile app running before the end of the year. Deck is starting to become a rather important part of my personal workflows. :) |
@srbaker Sure no need to hurry. I plan to do a 0.3.0 release in like 1 or 2 months, so we have some time if we want to get this in there. |
Are you guys still working on this? Would be really great to sync Deck with external apps! |
Heya, I haven't had the time I was hoping to. But I've got some time schedule to get this through to completion soon! Thanks for your interest! |
I'm also waiting for implementation of JSON API. I plan to develop a native application for elementary OS based on deck |
@srbaker That is awesome to hear. I think the JSON API will be a huge benefit for the deck app. Thank you already for digging into it 👍 |
@juliushaertl It is encouraging that I can drag out a contribution this long, and still have it be valued. I really want to see this, but sometimes real life gets in the way, you know? I've got some holidays coming up and a good set of tasks to work through. Cheers! |
Okay! I am back on this, after some delay. :) Do you know if there is a linter for PHP that can tell me when I've violated the nextcloud coding style? I'd really like to just set up my editor (emacs) to do the right thing for me, rather than thinking about it. Thanks! |
I think this solves the open issues. Please, comment on the latest. I'm going to be giving this more attention this week and next, so please comment if you have time. |
public function __construct($appName, IRequest $request, IUserManager $userManager, IGroupManager $groupManager, BoardService $service, $userId) { | ||
parent::__construct($appName, $request); | ||
$this->service = $service; | ||
$this->userId = $userId; |
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.
No need for the user/group manager anymore.
$this->userId = $userId; | ||
$this->userManager = $userManager; | ||
$this->groupManager = $groupManager; | ||
$this->userInfo = $this->service->getBoardPrerequisites(); |
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 can actually move that service to the BoardService now. 😉
* Return all of the boards that the current user has access to. | ||
*/ | ||
public function index() { | ||
$boards = $this->service->findAll($this->userInfo); |
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.
As mentioned above, let's move the user info fetching to the board service and get rid of the parameter here.
I'm not aware of any linter with a config available for the Nextcloud coding style. https://docs.nextcloud.com/server/10/developer_manual/general/codingguidelines.html#php Since many developers use PHPStorm, the code style settings are available here: https://github.com/nextcloud/server/blob/master/.idea/codeStyleSettings.xml I'm sorry, I cannot tell anything about emacs, but in general the coding style looks fine on this PR. |
Sorry, I missed that, it should have been obvious. Clearly I need to go back through and do a refactor. |
I haven't done the commit signing: do I have to go back and amend all of my commits with a signature? Or is there something else I can do? |
We can also squash those commits when merging, if you are fine with that. Btw. feel free to join #nextcloud-deck on freenode IRC if you have some quick questions. 😉 |
Looks like we can expect an API for Deck soon, very excited! |
This is the first run at an API, as discussed in #83. It is also required for #30.