From 9151aa7f042ca0fe3bf9f0c3201a122ee452926d Mon Sep 17 00:00:00 2001 From: Jonathan Rainville Date: Tue, 16 May 2023 12:11:52 -0400 Subject: [PATCH] fix(unviewed): fix edge case where unviewed count would be -1 (#3491) (#3496) --- VERSION | 2 +- protocol/message_persistence.go | 10 +--- protocol/messenger.go | 15 ++---- protocol/messenger_chats.go | 1 + protocol/messenger_delete_message_test.go | 66 ++++++++++++++++++++++- protocol/messenger_handler.go | 6 ++- protocol/persistence.go | 19 ++++++- 7 files changed, 92 insertions(+), 27 deletions(-) diff --git a/VERSION b/VERSION index fd53ae84e..d7fb48457 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.151.6 +0.151.7 diff --git a/protocol/message_persistence.go b/protocol/message_persistence.go index 4b3284dc9..6c6c76d88 100644 --- a/protocol/message_persistence.go +++ b/protocol/message_persistence.go @@ -1726,14 +1726,8 @@ func (db sqlitePersistence) MarkAllRead(chatID string, clock uint64) (int64, int _, err = tx.Exec( `UPDATE chats - SET unviewed_message_count = - (SELECT COUNT(1) - FROM user_messages - WHERE local_chat_id = ? AND seen = 0), - unviewed_mentions_count = - (SELECT COUNT(1) - FROM user_messages - WHERE local_chat_id = ? AND seen = 0 AND (mentioned or replied)), + SET unviewed_message_count = 0, + unviewed_mentions_count = 0, highlight = 0 WHERE id = ?`, chatID, chatID, chatID) diff --git a/protocol/messenger.go b/protocol/messenger.go index ae2c9140c..c9efbae7f 100644 --- a/protocol/messenger.go +++ b/protocol/messenger.go @@ -4857,7 +4857,7 @@ func (m *Messenger) markAllRead(chatID string, clock uint64, shouldBeSynced bool return errors.New("chat not found") } - seen, mentioned, err := m.persistence.MarkAllRead(chatID, clock) + _, _, err := m.persistence.MarkAllRead(chatID, clock) if err != nil { return err } @@ -4872,17 +4872,8 @@ func (m *Messenger) markAllRead(chatID string, clock uint64, shouldBeSynced bool chat.ReadMessagesAtClockValue = clock chat.Highlight = false - if chat.UnviewedMessagesCount >= uint(seen) { - chat.UnviewedMessagesCount -= uint(seen) - } else { - chat.UnviewedMessagesCount = 0 - } - - if chat.UnviewedMentionsCount >= uint(mentioned) { - chat.UnviewedMentionsCount -= uint(mentioned) - } else { - chat.UnviewedMentionsCount = 0 - } + chat.UnviewedMessagesCount = 0 + chat.UnviewedMentionsCount = 0 // TODO(samyoul) remove storing of an updated reference pointer? m.allChats.Store(chat.ID, chat) diff --git a/protocol/messenger_chats.go b/protocol/messenger_chats.go index 083c299f1..ae25258cc 100644 --- a/protocol/messenger_chats.go +++ b/protocol/messenger_chats.go @@ -477,6 +477,7 @@ func (m *Messenger) saveChat(chat *Chat) error { chat.Alias = name chat.Identicon = identicon } + // Sync chat if it's a new active public chat, but not a timeline chat if !ok && chat.Active && chat.Public() && !chat.ProfileUpdates() && !chat.Timeline() { diff --git a/protocol/messenger_delete_message_test.go b/protocol/messenger_delete_message_test.go index b0b4fed55..c6c260315 100644 --- a/protocol/messenger_delete_message_test.go +++ b/protocol/messenger_delete_message_test.go @@ -453,6 +453,68 @@ func (s *MessengerDeleteMessageSuite) TestDeleteMessageWithAMention() { s.Require().Len(response.Chats(), 1) s.Require().Len(response.Messages(), 1) // Receiver (us) is no longer mentioned - s.Require().Equal(int(response.Chats()[0].UnviewedMessagesCount), 0) - s.Require().Equal(int(response.Chats()[0].UnviewedMentionsCount), 0) + s.Require().Equal(int(state.Response.Chats()[0].UnviewedMessagesCount), 0) + s.Require().Equal(int(state.Response.Chats()[0].UnviewedMentionsCount), 0) +} + +// This test makes sure the UnviewMessageCount doesn't go below 0 in a very rare case where the Chat could be marked +// as read but the message still unseen (Seen == false) +func (s *MessengerDeleteMessageSuite) TestDeleteMessageAndChatIsAlreadyRead() { + theirMessenger := s.newMessenger() + _, err := theirMessenger.Start() + s.Require().NoError(err) + + theirChat := CreateOneToOneChat("Their 1TO1", &s.privateKey.PublicKey, s.m.transport) + err = theirMessenger.SaveChat(theirChat) + s.Require().NoError(err) + + ourChat := CreateOneToOneChat("Our 1TO1", &theirMessenger.identity.PublicKey, s.m.transport) + err = s.m.SaveChat(ourChat) + s.Require().NoError(err) + + inputMessage := buildTestMessage(*theirChat) + sendResponse, err := theirMessenger.SendChatMessage(context.Background(), inputMessage) + s.NoError(err) + s.Require().Len(sendResponse.Messages(), 1) + + response, err := WaitOnMessengerResponse( + s.m, + func(r *MessengerResponse) bool { return len(r.messages) == 1 }, + "no messages", + ) + s.Require().NoError(err) + s.Require().Len(response.Chats(), 1) + s.Require().Equal(response.Chats()[0].UnviewedMessagesCount, uint(1)) + s.Require().Len(response.Messages(), 1) + + // Force UnviewedMessagesCount to 0 to test if the uint validation is done correctly + ourChat.UnviewedMessagesCount = 0 + err = s.m.saveChat(ourChat) + + s.Require().NoError(err) + + ogMessage := sendResponse.Messages()[0] + + deleteMessage := DeleteMessage{ + DeleteMessage: protobuf.DeleteMessage{ + Clock: 2, + MessageType: protobuf.MessageType_ONE_TO_ONE, + MessageId: ogMessage.ID, + ChatId: theirChat.ID, + }, + From: common.PubkeyToHex(&theirMessenger.identity.PublicKey), + } + + state := &ReceivedMessageState{ + Response: &MessengerResponse{}, + } + + // Handle Delete first + err = s.m.HandleDeleteMessage(state, deleteMessage) + + s.Require().NoError(err) + s.Require().Len(response.Chats(), 1) + s.Require().Len(response.Messages(), 1) + // Receiver (us) no longer has unread messages and it's not negative + s.Require().Equal(0, int(state.Response.Chats()[0].UnviewedMessagesCount)) } diff --git a/protocol/messenger_handler.go b/protocol/messenger_handler.go index 4c5dffbec..25d3e14b3 100644 --- a/protocol/messenger_handler.go +++ b/protocol/messenger_handler.go @@ -1632,8 +1632,10 @@ func (m *Messenger) HandleDeleteMessage(state *ReceivedMessageState, deleteMessa // Reduce chat mention count and unread count if unread if !messageToDelete.Seen && !unreadCountDecreased { unreadCountDecreased = true - chat.UnviewedMessagesCount-- - if messageToDelete.Mentioned || messageToDelete.Replied { + if chat.UnviewedMessagesCount > 0 { + chat.UnviewedMessagesCount-- + } + if chat.UnviewedMentionsCount > 0 && (messageToDelete.Mentioned || messageToDelete.Replied) { chat.UnviewedMentionsCount-- } err := m.saveChat(chat) diff --git a/protocol/persistence.go b/protocol/persistence.go index 153e105a0..28e535dc6 100644 --- a/protocol/persistence.go +++ b/protocol/persistence.go @@ -418,6 +418,9 @@ func (db sqlitePersistence) Chat(chatID string) (*Chat, error) { imagePayload []byte ) + var unviewedMessagesCount int + var unviewedMentionsCount int + err := db.db.QueryRow(` SELECT id, @@ -459,8 +462,8 @@ func (db sqlitePersistence) Chat(chatID string) (*Chat, error) { &chat.Timestamp, &chat.ReadMessagesAtClockValue, &chat.DeletedAtClockValue, - &chat.UnviewedMessagesCount, - &chat.UnviewedMentionsCount, + &unviewedMessagesCount, + &unviewedMentionsCount, &chat.LastClockValue, &lastMessageBytes, &encodedMembers, @@ -498,6 +501,18 @@ func (db sqlitePersistence) Chat(chatID string) (*Chat, error) { if profile.Valid { chat.Profile = profile.String } + + // Set UnviewedCounts and make sure they are above 0 + // Since Chat's UnviewedMessagesCount is uint and the SQL column is INT, it can create a discrepancy + if unviewedMessagesCount < 0 { + unviewedMessagesCount = 0 + } + if unviewedMentionsCount < 0 { + unviewedMentionsCount = 0 + } + chat.UnviewedMessagesCount = uint(unviewedMessagesCount) + chat.UnviewedMentionsCount = uint(unviewedMentionsCount) + // Restore members membersDecoder := gob.NewDecoder(bytes.NewBuffer(encodedMembers)) err = membersDecoder.Decode(&chat.Members)