[FIX] Add parameter to REST chat.react endpoint, to make it work like a setter #10447
Conversation
MarcosSpessatto
changed the title
| if (message.reactions[reaction].usernames.length === 0) { | ||
| delete message.reactions[reaction]; | ||
| } | ||
| }; | ||
| if (userAlreadyReacted) { |
There was a problem hiding this comment.
why not just do something like?
if(userAlreadyReacted === shouldReact) {
return;
}There was a problem hiding this comment.
I think we should not simply return if they are the same in this case, because if we do not throw an error when we try to react in a message already reacted, the REST API will end up returning success, without actually having performed the react action. What do you think?
There was a problem hiding this comment.
I'm not sure... but this change aims allow set the reaction instead toggle. if you want react but you already reacted, I think you got it, its not a ERROR...
There was a problem hiding this comment.
I think there's no need to throw an error if the end result is going to be the same as when the method is successful.
There was a problem hiding this comment.
We'd need to change quite a few REST things, then. We have more methods that error with "The current status is the same as it would be set to"
There was a problem hiding this comment.
It's something that is worth looking into, then. We should get everybody's opinion on which approach is better and then standardize every method.
|
@MarcosSpessatto could you update the docs? |
|
@ggazzo here it is https://github.com/RocketChat/docs/pull/804 ;) |
Add parameter to REST chat.react endpoint, to make it work like a setter
Closes #10370.