-
Notifications
You must be signed in to change notification settings - Fork 280
Feat: add OCS routes for mailbox and mail listing #11425
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
Conversation
Thanks for opening your first pull request in this repository! ✌️ |
f459cda
to
ba32a24
Compare
ba32a24
to
04063bb
Compare
cf675c2
to
4d368a7
Compare
$userId, | ||
$view | ||
); | ||
return new DataResponse(json_encode($messages), Http::STATUS_OK); |
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.
json_encode should not be necessary when using DataResponse, I think 🤔
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're right, removed :)
89f3d55
to
bb06df5
Compare
* 200: Mailbox list | ||
* 404: User was not logged in or account doesn't exist | ||
*/ | ||
#[ApiRoute(verb: 'GET', url: '/mailbox/list')] |
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.
Nitpick: not happy about the URL design. /api/mailboxes
is already taken by the internal API. How about /api/ocs/mailboxes
or similar to have a new, dedicated sub path for public ocs APIs?
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.
The api/
part is left out for the OCS routes for now, but I'm open to anything here. Could put ocs instead of api, so /ocs/mailboxes/list
?
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 fine with leaving out the api part for ocs. But please make it ocs/mailboxes
without the list
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.
Sure, done!
Signed-off-by: Jana Peper <[email protected]>
Signed-off-by: Jana Peper <[email protected]>
bb06df5
to
a76eb7a
Compare
Signed-off-by: Jana Peper <[email protected]>
a76eb7a
to
f6b793d
Compare
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.
👍 otherwise :)
Co-authored-by: Christoph Wurst <[email protected]> Signed-off-by: janepie <[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.
Looks good! Thank you!
adds OCS routes to