[FIX] Update Message: Does not show edited when message was not edited.#13053
[FIX] Update Message: Does not show edited when message was not edited.#13053sampaiodiego merged 2 commits intoRocketChat:developfrom
Conversation
|
|
|
Thanks for contributing! This looks good in my opinion. @sampaiodiego thoughts? |
| message.urls = urls.map((url) => ({ url })); | ||
|
|
||
| message = RocketChat.callbacks.run('beforeSaveMessage', message); | ||
| if (originalMessage.msg !== message.msg) { |
There was a problem hiding this comment.
should we return before having the message processed by Apps?
There was a problem hiding this comment.
Do you mean message processing by Apps should happen when the messages are not equal ?
There was a problem hiding this comment.
Also, I am not sure what Apps does here.
There was a problem hiding this comment.
Apps can change the message content or prevent a message from being sent.
so my question was: should we skip everything if the edited message didn't change? or should Apps still process messages that were "edited" but still have the same content?
There was a problem hiding this comment.
@rodrigok @d-gubert @graywolf336 what you guys think on above?
There was a problem hiding this comment.
I'd say that we could skip the whole RocketChat.updateMessage method when the message's content doesn't change, not executing Apps, before/after hooks, or message history ->
There was a problem hiding this comment.
cool then sounds like this is good?
There was a problem hiding this comment.
Yeah I'll change it to skipping the whole RocketChat.updateMessage method.
|
Ohh thanks! |
|
@geekgonecrazy I have updated the code according to what we discussed. |
| const prevent = Promise.await(Apps.getBridges().getListenerBridge().messageEvent('IPreMessageUpdatedPrevent', appMessage)); | ||
| if (prevent) { | ||
| throw new Meteor.Error('error-app-prevented-updating', 'A Rocket.Chat App prevented the message updating.'); | ||
| if (originalMessage.msg !== message.msg) { |
There was a problem hiding this comment.
instead of shifting everything to inside the if, can you please change it to something like:
if (originalMessage.msg !== message.msg) {
return;
}
/* rest of the code */this way the code looks nicer and clean as so the diff :)
There was a problem hiding this comment.
Yeah true, I'll do that but shouldn't it be,
if (originalMessage.msg === message.msg) {
return;
}because we want to skip everything and return if both messages are equal ?
client/methods/updateMessage.js
Outdated
| _id: message._id, | ||
| 'u._id': Meteor.userId(), | ||
| }, { $set : messageObject }); | ||
| if (originalMessage.msg !== message.msg) { |
There was a problem hiding this comment.
It looks like you could move this if up to the start of the method and make an "early return", similarly to the other file :)
|
In the latest commit that I pushed, I early returned from |
5 participants
Closes #12282