-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Livechat extended REST APIs #4117
Conversation
I'm not sure if this is the best way to create this APIs though 😞 |
Change from `/livechat-api/sms-incoming/:service` to `/api/v1/livechat/sms-incoming/:service`
7b6904e
to
7d1b96d
Compare
Ready for merge! Should we start create some tests for REST APIs? |
return RocketChat.API.v1.unauthorized(); | ||
} | ||
|
||
return RocketChat.API.v1.success(_.pick(RocketChat.models.Settings.findOneNotHiddenById(this.urlParams._id), '_id', 'value')); |
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.
Why not use RocketChat.settings.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.
Because I don't want let users see settings hidden from the admin panel.
// REST endpoints | ||
api.addFiles('server/rest/departments.js', 'server'); | ||
api.addFiles('server/rest/sms.js', 'server'); | ||
api.addFiles('server/rest/users.js', 'server'); |
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.
Maybe we can start using imports here. What do you think?
We can keep the server/api.js
and just add the imports for each api file.
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.
👍
@RocketChat/core
Closes #3957
New endpoints examples: