Bi-directional reactions to messages#5448
Bi-directional reactions to messages#5448engelgabriel merged 2 commits intoRocketChat:developfrom kable-wilmoth:ReactToMsgs
Conversation
|
|
||
| return @findOne query | ||
|
|
||
| findOneBySlackTs: (slackTs) -> |
There was a problem hiding this comment.
Needed the ability to look up a slack message w/ just the timestamp because the react message coming from slack didn't include the bot ID
| Meteor.methods({ | ||
| setReaction(reaction, messageId) { | ||
| if (!Meteor.userId()) { | ||
| setReaction(reaction, messageId, user) { |
There was a problem hiding this comment.
It was previously assumed that a reaction was coming from the current user. This is true most of the time, but in my case when Slack sends a reaction, I needed to call this method on behalf of the user. So added a user parameter, but if not present, it behaves as before.
There was a problem hiding this comment.
Please revert this change and instead run this method as the user who is doing the reacting like such:
Meteor.runAsUser(user._id, () => {
Meteor.call('setReaction', reaction, messageId);
});Otherwise this is a breaking change for all of the clients out there who call this method as the user and don't pass in the user object.
There was a problem hiding this comment.
Will do.
Can you help me understand why it would have been a breaking change. I am new to JavaScript but I understood parameters to be optional. I had done some testing calling this method w/ out the user parameter and then the user was just resolved inside the method as previously coded. Is the potential breakage more to do w/ Meteor and the Rocket framework <meteor.call()>?
| @@ -41,8 +44,10 @@ Meteor.methods({ | |||
| if (_.isEmpty(message.reactions)) { | |||
| delete message.reactions; | |||
| RocketChat.models.Messages.unsetReactions(messageId); | |||
There was a problem hiding this comment.
Added two new callbacks: setReaction and unsetReaction
There was a problem hiding this comment.
Can you add the callbacks to run on the client side as well? This way people can take advantage of it for statistics, such as Piwiki or future Google Analytics
There was a problem hiding this comment.
Haven't done any client side changes to Rocket yet, but will look into this.
| Meteor.methods({ | ||
| setReaction(reaction, messageId) { | ||
| if (!Meteor.userId()) { | ||
| setReaction(reaction, messageId, user) { |
There was a problem hiding this comment.
Please revert this change and instead run this method as the user who is doing the reacting like such:
Meteor.runAsUser(user._id, () => {
Meteor.call('setReaction', reaction, messageId);
});Otherwise this is a breaking change for all of the clients out there who call this method as the user and don't pass in the user object.
| @@ -41,8 +44,10 @@ Meteor.methods({ | |||
| if (_.isEmpty(message.reactions)) { | |||
| delete message.reactions; | |||
| RocketChat.models.Messages.unsetReactions(messageId); | |||
There was a problem hiding this comment.
Can you add the callbacks to run on the client side as well? This way people can take advantage of it for statistics, such as Piwiki or future Google Analytics
Responded to pull request comments * Instead of passing in the user object, just used Meteor.runAs() * Added callback events to the client side
|
@graywolf336 I have submitted the requested fixes. Not sure how to trigger the next 'steps' in the pull request process. Maybe this comment is it. |
graywolf336
left a comment
graywolf336
left a comment
There was a problem hiding this comment.
Thanks for making those changes, looks good to me.
3 participants
@RocketChat/core
Added the ability to react to slack and rocket messages. Doesn't quite work all the way because Slack sees the reactions as coming from the 'bot'. I am working w/ the Slack team to figure out some work arounds based on some alternative bot authentication mechanisms.