Skip to content
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

[NEW] Add user settings / preferences API endpoint #9457

Merged
merged 30 commits into from
Feb 21, 2018

Conversation

jgtoriginal
Copy link
Contributor

@jgtoriginal jgtoriginal commented Jan 21, 2018

[NEW] Add user settings / preferences API endpoint #8694

Add a user settings / preferences endpoint to api, to make it possible to change user settings via authenticated api call.
@RocketChat/core

Closes #8694

@dennispost raised the concern of not being able to get and set user preferences via API.
@graywolf336 confirmed that such method didn't exist

I'm just providing a possible solution here.

@RocketChat RocketChat deleted a comment Jan 21, 2018
@RocketChat RocketChat deleted a comment Jan 21, 2018
@RocketChat RocketChat deleted a comment Jan 21, 2018
@RocketChat RocketChat deleted a comment Jan 21, 2018
@RocketChat RocketChat deleted a comment Jan 21, 2018
@RocketChat RocketChat deleted a comment Jan 22, 2018
@RocketChat RocketChat deleted a comment Jan 22, 2018
@RocketChat RocketChat deleted a comment Jan 22, 2018
@RocketChat RocketChat deleted a comment Jan 22, 2018
@RocketChat RocketChat deleted a comment Jan 22, 2018
@RocketChat RocketChat deleted a comment Jan 22, 2018
@RocketChat RocketChat deleted a comment Jan 22, 2018
@RocketChat RocketChat deleted a comment Jan 22, 2018
@geekgonecrazy
Copy link
Contributor

@RocketChat/android @rocketchat/ios thoughts? This is probably something useful to you guys isn't it?

@rafaelks
Copy link
Contributor

@geekgonecrazy Yes, this is very useful for us, an API with all user settings would be welcome. 👍

Just one thing... shouldn't users.getPreferences always require authentication?

@jgtoriginal
Copy link
Contributor Author

my bad there @rafaelks
it was just turned off for dev purpose, requiring auth now. =)

@jgtoriginal jgtoriginal changed the title Add user settings / preferences API endpoint #8694 Add user settings / preferences API endpoint Jan 23, 2018
@@ -271,6 +271,77 @@ RocketChat.API.v1.addRoute('users.createToken', { authRequired: true }, {
}
});

RocketChat.API.v1.addRoute('users.getPreferences', { authRequired: true }, {
get() {
const user = this.getUserFromParams();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are still allowing to get data from any user you pass in params, you need to use the this.userId to find the logged user record

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well @rodrigok this is a bit of a chicken and an egg situation, because if you check the file thoroughly, you will find that on 7 other methods we are defining the user in the same way. However, I do agree that we can narrow the scope down and enforce a more secure fashion.
So I'm changing that now and see if you like it. Thx!

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9457 February 17, 2018 15:21 Inactive
@jgtoriginal
Copy link
Contributor Author

jgtoriginal commented Feb 17, 2018

@geekgonecrazy yes, I noticed that. And it's amazing how you guys improved documentation last couple of months. Sorry @MarcosSpessatto you had to pick up some tests writing for me.

I will definitely look into writing tests. But I'm unaware of where to write the docs. If you give me pointers @geekgonecrazy I will definitely look into that too. Thx!!

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9457 February 17, 2018 15:27 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9457 February 17, 2018 16:32 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9457 February 17, 2018 18:52 Inactive
@@ -271,6 +271,77 @@ RocketChat.API.v1.addRoute('users.createToken', { authRequired: true }, {
}
});

RocketChat.API.v1.addRoute('users.getPreferences', { authRequired: true }, {
get() {
const user = this.userId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgtoriginal did you tested this? You will need to execute a find to get the user's object when you have the user's id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodrigok no worries, will get to it at some point today and let you know.
Thanks very much!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has been solved by @MarcosSpessatto on 19a8ad6

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9457 February 20, 2018 02:04 Inactive
@geekgonecrazy
Copy link
Contributor

I will definitely look into writing tests. But I'm unaware of where to write the docs. If you give me pointers @geekgonecrazy I will definitely look into that too. Thx!!

@jgtoriginal https://github.com/RocketChat/docs :)

@@ -273,7 +273,7 @@ RocketChat.API.v1.addRoute('users.createToken', { authRequired: true }, {

RocketChat.API.v1.addRoute('users.getPreferences', { authRequired: true }, {
get() {
const user = this.userId;
const user = RocketChat.models.Users.findOneById(this.userId);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcosSpessatto we are going back to what we previously had here, which @rodrigok asked to change for this.userId Any particular reason?

@jgtoriginal
Copy link
Contributor Author

@MarcosSpessatto you just took over man!! Thank you very much!!
Just to double check, are you also taking over with docs or shall I do that?
Cheers!

@MarcosSpessatto
Copy link
Member

@jgtoriginal i'll create docs now :)

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9457 February 20, 2018 16:38 Inactive
@rodrigok rodrigok merged commit 34340ca into RocketChat:develop Feb 21, 2018
@rodrigok rodrigok mentioned this pull request Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants