[FIX] Chat Message Reactions REST API End Point #9487
Conversation
|
|
||
| const emoji = this.bodyParams.emoji; | ||
|
|
||
| Meteor.runAsUser(this.userId, () => Meteor.call('setReaction', emoji, msg._id, this.userId)); |
There was a problem hiding this comment.
Why do you need to pass the this.userId if you are already executing the method using runAsUser?
There was a problem hiding this comment.
@rodrigok you are right it is not strictly necessary to do it that way.
However if you look at every single method on that file, they all do it in that very same way. So I just tried to stick to the standards and to the scope.
There was a problem hiding this comment.
@jgtoriginal What method(s) and what files are you talking about?
There was a problem hiding this comment.
@graywolf336 every API method on this file packages/rocketchat-api/server/v1/chat.js does Meteor.runAsUser. Does it make sense?
There was a problem hiding this comment.
Yes, that part makes sense however the part we are talking about is the passed in this.userId to the Meteor.call('setReaction... piece of the code along with the new argument usr added to the definition of the setReaction method. These last two aren't necessary and can be removed.
There was a problem hiding this comment.
@graywolf336 got you! let me get that done. thx!
There was a problem hiding this comment.
I confirm what has been said above, like @graywolf336 said you can remove the usr param, and the verification added over this param. Missing end to end tests in this endpoint.
|
|
||
| const msg = RocketChat.models.Messages.findOneById(this.bodyParams.messageId); | ||
|
|
||
| if (!msg) { |
There was a problem hiding this comment.
You can move this verification to see if have or not message to the setReactions method, cuz already has one call to db for retrieve message, and you can save a call.
There was a problem hiding this comment.
I'm going to have to disagree with this. That is outside of the scope of this pull request. If we change how the setReaction works then we are going to have to change how the other places which calls that method handles the result. The setReaction method doesn't return errors when the message doesn't exist and it only returns false so I think we should keep this error here for other developer's sake.
There was a problem hiding this comment.
Ok @graywolf336, I had not througt about it. You're right.
|
Is this one blocked on anything particular? Would be great to have this land in 0.62.0 |
|
@geekgonecrazy sorry, I'm not blocked on this specific task, I just got jammed among other projects. Will get back to this today so as we can proceed. Thanks for the reminder! |
….reactAPIendPoint
geekgonecrazy
dismissed
MarcosSpessatto’s stale review
please see discussion around review. Seems not needed
|
@jgtoriginal no problem :) Just seems like a great endpoint to make sure lands in 0.62.0. I believe we intend to release the candidate / feature freeze on the 20th |
|
@MarcosSpessatto I can see that you took over on removing the unnecessary parameter mentioned by @graywolf336 further up. |
[FIX] Chat Message Reactions REST API End Point
/api/v1/chat.react method is documented but not present on
packages/rocketchat-api/server/v1/chat.jsafter having a chat with @graywolf336 it seems that he implemented it at some point, but a
git blameon that file shows no result, hence I just provide an implementation for it since it's very useful to have.INSTRUCTION: Just follow the docs
@RocketChat/core
NOTE: not on the
rocketchat-reactionspackage nor on this endpoint, is there any sort of sanitization in place, so if you send a non existing emoji it would work the same. I'm not sure how to go by that, since we could stick to the default emojis for sanitization, but I'm not sure we could do so with custom ones. But probably that would be an other task centralized on therockatchat-reactionspackages that's the one instantiated by this new API method.