-
Notifications
You must be signed in to change notification settings - Fork 248
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
14044 handle edit messages in bridge #4983
Conversation
Jenkins Builds
|
editMessage := EditMessage{ | ||
EditMessage: &protobuf.EditMessage{ | ||
Clock: messageToEdit.Clock + 1, | ||
Text: editedText, | ||
MessageType: protobuf.MessageType_ONE_TO_ONE, | ||
MessageId: messageToEdit.ID, | ||
ChatId: theirChat.ID, | ||
ContentType: protobuf.ChatMessage_BRIDGE_MESSAGE, | ||
}, | ||
From: common.PubkeyToHex(&theirMessenger.identity.PublicKey), | ||
} | ||
state := &ReceivedMessageState{ | ||
Response: &MessengerResponse{}, | ||
} | ||
|
||
// Handle edit first | ||
err = s.m.handleEditMessage(state, editMessage) | ||
s.Require().NoError(err) | ||
|
||
// It save the edit | ||
s.Require().Len(state.Response.Messages(), 1) | ||
s.Require().Len(state.Response.Chats(), 1) | ||
s.Require().NotEmpty(state.Response.Chats()[0].LastMessage.EditedAt) | ||
s.Require().Equal(state.Response.Messages()[0].GetBridgeMessage().Content, "edited text") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't you call WaitOnMessengerResponse
instead of a direct call of s.m.handleEditMessage(state, editMessage)
in order to check that s.m
received the edited message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a comment related the test
var hasMessage bool | ||
hasMessage, err = db.bridgeMessageExists(tx, msg.GetBridgeMessage().MessageID) | ||
if err != nil { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of err, maybe we should just ignore and save the message as before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if it returned the error, it would mean that there is something wrong with db connection and another db operation would fail also. Am I right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean return the error. I mean just not return
. Because since you return, you will miss that message
// Finds status messages id which are replies for bridgeMessageID | ||
func (db sqlitePersistence) findStatusMessageIdsRepliedTo(tx *sql.Tx, parentMessageID string) (string, error) { | ||
rows, err := tx.Query(`SELECT user_messages_id FROM bridge_messages WHERE message_id = ?`, parentMessageID) | ||
func (db sqlitePersistence) FindStatusMessageIdForBridgeMessageId(messageID string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean it could get slower over time? Excuse my ignorance, are the messages split per chats or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we query from bridge messages.
Bridge messages have a separated table. They are joined with messages via a messageId.
I did not investigate how much time does it take.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, quite clear changes.
Issue #14044
222089a
to
1d8099e
Compare
Cherry-picked to master. |
Adjust EditMessage() to handle bridge messages (will be used by the bridge).
Adjust applyEditMessage() to handle bridge messages.
Closes #14044