[keyserver] Handle deleted messages when searching
ClosedPublic
Actions

Authored by tomek on Tue, Apr 8, 3:58 AM.

Details

Reviewers

kamil
ashoat

Commits

rCOMM8702a758cf22: [keyserver] Handle deleted messages when searching

Summary

When we're creating a new message, we're updating a search index. This means that when we delete a message, we can delete all the index entries associated with the deleted message.

https://linear.app/comm/issue/ENG-10517/consider-deleted-message-in-message-search

Depends on D14552

Test Plan

Created a message, deleted it, sent 20 more messages, changed active thread, refreshed the app, opened the previous active thread, and searched for the message. After this diff - mo messages are shown. Before this diff - the original message content was shown (if less than 20 messages were sent, the deleted message was shown in the results).

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Tue, Apr 8, 3:58 AM

tomek added a child revision: D14557: [native][web] Handle deleted messages when searching.Tue, Apr 8, 4:01 AM

Harbormaster completed remote builds in B33926: Diff 47705.Tue, Apr 8, 4:17 AM

tomek requested review of this revision.Tue, Apr 8, 4:17 AM

ashoat accepted this revision.Tue, Apr 8, 6:36 AM

ashoat added inline comments.

keyserver/src/fetchers/message-fetchers.js
859 ↗	(On Diff #47705)	How does this change relate to the other one? It doesn't seem to have been explained anywhere, and I don't think it's necessary for the other changes

This revision is now accepted and ready to land.Tue, Apr 8, 6:36 AM

Remove unnecessary condition

keyserver/src/fetchers/message-fetchers.js
859 ↗	(On Diff #47705)	Hmm... it is unrelated. I think that included this type was making the function more correct, but I haven't observed any difference after introducing it. We can remove it.

Harbormaster completed remote builds in B33951: Diff 47735.Thu, Apr 10, 5:48 AM

ashoat requested changes to this revision.Thu, Apr 10, 6:14 AM

ashoat added inline comments.

keyserver/src/fetchers/message-fetchers.js
859 ↗	(On Diff #47705)	I wasn't asking to remove it, and I'm confused why you removed it. This function is used in two places, `fetchPinnedMessageInfos` and `searchMessagesInSingleChat`. Is it necessary to include deleted messages in those cases? By not including deleted messages, are you risking that deleted pinned messages and searched messages can be displayed to the user? My request what to explain the change, not to remove it. We should have a clear understanding of the changes we're making, both when adding a line of code and when removing the line of code. Given the lack of explanation, it feels like you didn't have a good understanding when the line was added, and you didn't have a good understanding when it was removed.

This revision now requires changes to proceed.Thu, Apr 10, 6:14 AM

Return deleted message when fetching related messages

keyserver/src/fetchers/message-fetchers.js
859 ↗	(On Diff #47705)	This function is used in two places, fetchPinnedMessageInfos and searchMessagesInSingleChat. Is it necessary to include deleted messages in those cases? By not including deleted messages, are you risking that deleted pinned messages and searched messages can be displayed to the user? No, in both cases, it isn't necessary to include the removed messages because messages that got removed won't be included in the results. While deleting a message, we're unpinning it and we're updating a message search index. My request what to explain the change, not to remove it. We should have a clear understanding of the changes we're making, both when adding a line of code and when removing the line of code. Given the lack of explanation, it feels like you didn't have a good understanding when the line was added, and you didn't have a good understanding when it was removed. As I said, including this check makes the function more correct, but it doesn't change anything in the current state. It is possible that in the future we introduce some logic that uses this util and for which having a deleted message would make a difference.