From 717814e83fe3bcc0a7abd5904c963dd1e06132c8 Mon Sep 17 00:00:00 2001 From: Sean Hagstrom Date: Tue, 3 Sep 2024 10:12:44 +0100 Subject: [PATCH] fix!: ensure deleting message does not accidentally delete unrelated notifications (#5789) This change attempts to resolve an issue with notifications being deleted for users after another user deletes the last message inside a community. The cause of the issue seemed to be related to how notifications could reference a message with the `LastMessage` field. And if the last message was deleted from a community, then the notifications that reference the last message via `LastMessage` field would also removed/deleted. Moving forward, we will no longer use the `LastMessage` field as a way to detect notifications that need to be deleted, and we now only use the `Message` field of a notification, and we have adapted the creation of notifications to avoid using the `LastMessage` field. --- protocol/activity_center_persistence.go | 4 - protocol/activity_center_persistence_test.go | 125 +++++++++++++++++-- protocol/messenger_handler.go | 2 +- 3 files changed, 118 insertions(+), 13 deletions(-) diff --git a/protocol/activity_center_persistence.go b/protocol/activity_center_persistence.go index 02b1efafe..456c8b4a2 100644 --- a/protocol/activity_center_persistence.go +++ b/protocol/activity_center_persistence.go @@ -60,10 +60,6 @@ func (db sqlitePersistence) DeleteActivityCenterNotificationForMessage(chatID st } for _, notification := range notifications { - if notification.LastMessage != nil && notification.LastMessage.ID == messageID { - withNotification(notification) - } - if notification.Message != nil && notification.Message.ID == messageID { withNotification(notification) } diff --git a/protocol/activity_center_persistence_test.go b/protocol/activity_center_persistence_test.go index c7ef6db80..db8426f46 100644 --- a/protocol/activity_center_persistence_test.go +++ b/protocol/activity_center_persistence_test.go @@ -191,6 +191,7 @@ func (s *ActivityCenterPersistenceTestSuite) Test_DeleteActivityCenterNotificati notif, err := p.GetActivityCenterNotificationByID(nID1) s.Require().NoError(err) + s.Require().NotNil(notif) s.Require().True(notif.Deleted) s.Require().True(notif.Dismissed) s.Require().True(notif.Read) @@ -199,26 +200,34 @@ func (s *ActivityCenterPersistenceTestSuite) Test_DeleteActivityCenterNotificati for _, id := range []types.HexBytes{nID2, nID3, nID4} { notif, err = p.GetActivityCenterNotificationByID(id) s.Require().NoError(err) + s.Require().NotNil(notif) s.Require().False(notif.Deleted, notif.ID) s.Require().False(notif.Dismissed, notif.ID) s.Require().False(notif.Read, notif.ID) } // Test: soft delete the notifications that have Message.ID == messages[1].ID - // or LastMessage.ID == chat.LastMessage. _, err = p.DeleteActivityCenterNotificationForMessage(chat.ID, messages[1].ID, currentMilliseconds()) s.Require().NoError(err) - for _, id := range []types.HexBytes{nID2, nID3} { - notif, err = p.GetActivityCenterNotificationByID(id) - s.Require().NoError(err, notif.ID) - s.Require().True(notif.Deleted, notif.ID) - s.Require().True(notif.Dismissed, notif.ID) - s.Require().True(notif.Read, notif.ID) - } + notif, err = p.GetActivityCenterNotificationByID(nID2) + s.Require().NoError(err) + s.Require().NotNil(notif) + s.Require().True(notif.Deleted, notif.ID) + s.Require().True(notif.Dismissed, notif.ID) + s.Require().True(notif.Read, notif.ID) + + // Check that notifications with LastMessage.ID == messages[1].ID will remain. + notif, err = p.GetActivityCenterNotificationByID(nID3) + s.Require().NoError(err) + s.Require().NotNil(notif) + s.Require().False(notif.Deleted, notif.ID) + s.Require().False(notif.Dismissed, notif.ID) + s.Require().False(notif.Read, notif.ID) notif, err = p.GetActivityCenterNotificationByID(nID4) s.Require().NoError(err) + s.Require().NotNil(notif) s.Require().False(notif.Deleted) s.Require().False(notif.Dismissed) s.Require().False(notif.Read) @@ -228,6 +237,106 @@ func (s *ActivityCenterPersistenceTestSuite) Test_DeleteActivityCenterNotificati s.Require().NoError(err) } +func (s *ActivityCenterPersistenceTestSuite) Test_DeleteActivityCenterNotificationsForMessage_LastMessage() { + // Create the temporary test-database that will be used to store chats, messages, and notifications + db, err := openTestDB() + s.Require().NoError(err) + p := newSQLitePersistence(db) + + // Create and save the public chat that will be used to group our test messages + chat := CreatePublicChat("test-chat", &testTimeSource{}) + err = p.SaveChat(*chat) + s.Require().NoError(err) + + // Define multiple test messages for our chat so we can emulate a chat with a latest message. + messages := []*common.Message{ + { + ID: "0x1", + ChatMessage: &protobuf.ChatMessage{}, + LocalChatID: chat.ID, + }, + { + ID: "0x2", + ChatMessage: &protobuf.ChatMessage{}, + LocalChatID: chat.ID, + }, + } + err = p.SaveMessages(messages) + s.Require().NoError(err) + + chat.LastMessage = messages[1] + err = p.SaveChat(*chat) + s.Require().NoError(err) + + chatMessages, _, err := p.MessageByChatID(chat.ID, "", 2) + s.Require().NoError(err) + s.Require().Len(chatMessages, 2) + + // Define multiple notifications of different types to emulate the removal of notifications when deleting a message + nID1 := types.HexBytes("1") + nID2 := types.HexBytes("2") + nID3 := types.HexBytes("3") + nID4 := types.HexBytes("4") + + s.createNotifications(p, []*ActivityCenterNotification{ + { + ID: nID1, + ChatID: chat.ID, + Type: ActivityCenterNotificationTypeMention, + Message: messages[0], + LastMessage: messages[1], + }, + { + ID: nID2, + ChatID: chat.ID, + Type: ActivityCenterNotificationTypeMention, + Message: messages[1], + }, + { + ID: nID3, + ChatID: chat.ID, + Type: ActivityCenterNotificationTypeNewOneToOne, + LastMessage: messages[1], + }, + { + ID: nID4, + ChatID: chat.ID, + Type: ActivityCenterNotificationTypeNewPrivateGroupChat, + LastMessage: messages[1], + }, + }) + + // Action: soft delete notifications related to a chat and message + _, err = p.DeleteActivityCenterNotificationForMessage(chat.ID, messages[1].ID, currentMilliseconds()) + s.Require().NoError(err) + + // Test: check that notifications unrelated to the message are not affected. + notif, err := p.GetActivityCenterNotificationByID(nID1) + s.Require().NoError(err) + s.Require().NotNil(notif) + s.Require().False(notif.Deleted, notif.ID) + s.Require().False(notif.Dismissed, notif.ID) + s.Require().False(notif.Read, notif.ID) + + // Test: check notifications directly related to the message are soft deleted + notif, err = p.GetActivityCenterNotificationByID(nID2) + s.Require().NoError(err) + s.Require().NotNil(notif) + s.Require().True(notif.Deleted) + s.Require().True(notif.Dismissed) + s.Require().True(notif.Read) + + // Test: check NewOneToOne or NewPrivateGroupChat notifications that are indirectly related to the message are soft deleted + for _, id := range []types.HexBytes{nID3, nID4} { + notif, err = p.GetActivityCenterNotificationByID(id) + s.Require().NoError(err) + s.Require().NotNil(notif) + s.Require().False(notif.Deleted) + s.Require().False(notif.Dismissed) + s.Require().False(notif.Read) + } +} + func (s *ActivityCenterPersistenceTestSuite) Test_AcceptActivityCenterNotificationsForInvitesFromUser() { db, err := openTestDB() s.Require().NoError(err) diff --git a/protocol/messenger_handler.go b/protocol/messenger_handler.go index d8d2fd783..c830978a5 100644 --- a/protocol/messenger_handler.go +++ b/protocol/messenger_handler.go @@ -295,7 +295,7 @@ func (m *Messenger) createMessageNotification(chat *Chat, messageState *Received notification := &ActivityCenterNotification{ ID: types.FromHex(chat.ID), Name: chat.Name, - LastMessage: message, + Message: message, Type: notificationType, Author: messageState.CurrentMessageState.Contact.ID, Timestamp: messageState.CurrentMessageState.WhisperTimestamp,