Page MenuHomePhabricator

[keyserver, lib] Set target_message for new sidebar_source messages
ClosedPublic

Authored by inka on Apr 19 2023, 3:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 3:31 AM
Unknown Object (File)
Tue, Nov 5, 10:06 PM
Unknown Object (File)
Mon, Nov 4, 11:22 PM
Unknown Object (File)
Fri, Nov 1, 5:34 AM
Unknown Object (File)
Mon, Oct 28, 6:04 AM
Unknown Object (File)
Sep 28 2024, 8:02 AM
Unknown Object (File)
Sep 28 2024, 8:02 AM
Unknown Object (File)
Sep 28 2024, 8:02 AM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-3708/add-a-migration-to-populate-target-message-for-sidebar-source
We need the new sidebar_source messages to have their target_message column filled with the id of the original message.

Test Plan

Created new sidebar, checked that the target_message was correctly filled.
Run flow in keyserver/ and lib/

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Apr 19 2023, 3:48 AM
lib/types/message-types.js
245 ↗(On Diff #25330)
  1. In the other cases (reactions and edit message) we include targetMessageID in the MessageInfo / RawMessageInfo. Can you explain why you aren't including it here?
  2. Should this be a required parameter in the MessageData? It's only used on the keyserver side, so it shouldn't be risky. It's okay to make this change later (after you refactor keyserver code to always include this), but I think it should be a required parameter at the end of your stack
lib/types/message-types.js
245 ↗(On Diff #25330)

I think we don't need to include targetMessageID in *MessageInfo for sidebar source, because we are still keeping it in the content column, and sending it to the client via sourceMessage field. Reactions and edits don't do that, so they need to have targetMessageID in *MessageInfo.

An assumption I am working under: We don't want to rewrite the way we handle sidebar source messages (how we obtain the ID of their source message). We just want to have the target_message column filled.

245 ↗(On Diff #25330)

Alternatively, if having both sourceMessage and targetMessageID in SidebarSourceMessageData is redundant and not a good pattern, I could just extract targetMessageID` from message.sourceMessage inside of messageCreator (here)

like so

let targetMessageID = null;
if (messageData.targetMessageID) {
  targetMessageID = messageData.targetMessageID;
} else if (messageData.sourceMessage) {
  targetMessageID = messageData.sourceMessage.id;
}

This would require no other changes in the code.

ashoat requested changes to this revision.Apr 21 2023, 5:59 AM
ashoat added inline comments.
lib/types/message-types.js
245 ↗(On Diff #25330)

Thanks @inka, I think that's a better idea!

245 ↗(On Diff #25330)

That makes sense – thanks for explaining

This revision now requires changes to proceed.Apr 21 2023, 5:59 AM

Address review - change the approach to extracting the source message id in the message creator, from messageData.sourceMessage.id.
Only messageData for sidebar source message has a sourceMessage field, so this shouldn't break anything. If a messageData has targetMessageID, then it will be set correctly as it used to. If a messageData has a sourceMessage then the message must have been a sidebar source message that doesn't have a targetMessageID, and so targetMessageID will be set to messageData.sourceMessage.id. For all other messages targetMessageID will be null, as it used to.

This revision is now accepted and ready to land.Apr 21 2023, 6:46 AM