[lib/keyserver] make localID optional in the sendReactionMessageRequestInputValidator
ClosedPublic
Actions

Authored by ginsu on Jan 11 2023, 3:45 PM.

Details

Reviewers

atul
ashoat
tomek

Commits

rCOMMf9887b6792f3: [lib/keyserver] make localID optional in the…

Summary

make localID optional in the sendReactionMessageRequestInputValidator. We need to do this because old native clients ie. build 172 does not have a localID field and by previously making localID requried it would cause the sendReactionMessageRequestInputValidator to fail

Depends on D6244

Test Plan

Tested both older and newer native clients.

To test newer clients, I sent a reaction message and everything worked as expected, I checked the DB and the reaction message had the creation column filled.

To test older clients, I checked out the commit with the release tag using git checkout v1.0.172. This is to test an older native client. Then I copied and pasted the necessary code in the message-responders file to demonstrate an updated keyserver, and when I tried sending a reaction message, everything worked as expected and when I checked the DB, the reaction message did not have the creation column filled which makes sense since no localID was passed

Screenshot 2023-01-11 at 6.46.50 PM.png (1×3 px, 2 MB)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Jan 11 2023, 3:45 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptJan 11 2023, 3:45 PM

ginsu edited the test plan for this revision. (Show Details)Jan 11 2023, 3:50 PM

ginsu added reviewers: atul, ashoat, tomek.

ginsu edited the test plan for this revision. (Show Details)

ginsu attached a referenced file: F326293: Screenshot 2023-01-11 at 6.46.50 PM.png. (Show Details)

ginsu edited the summary of this revision. (Show Details)

ginsu added a parent revision: D6244: Revert "[keyserver/lib] introuce localID field to sendReactionMessage action and reactionMessageCreationResponder".

Harbormaster completed remote builds in B15273: Diff 20829.Jan 11 2023, 3:56 PM

ginsu requested review of this revision.Jan 11 2023, 3:56 PM

ashoat accepted this revision.Jan 11 2023, 4:04 PM

ashoat added inline comments.

keyserver/src/responders/message-responders.js
250 ↗	(On Diff #20829)	It's a bit more messy in terms of code, but can you make it so we don't include this field (rather than including it set to `undefined`) if it doesn't exist? Cleanest way to do that might be to do this sort of construction: let messageData: ReactionMessageData; if (localID) { messageData = { ... // define with localID }; } else { messageData = { ... // define without localID }; }

This revision is now accepted and ready to land.Jan 11 2023, 4:04 PM

ashoat added inline comments.Jan 11 2023, 4:06 PM

keyserver/src/responders/message-responders.js
250 ↗	(On Diff #20829)	Or maybe this would be better: let messageData = { ... // define without localID }; if (localID) { messageData = { ...messageData, localID }; }

ginsu added inline comments.Jan 11 2023, 4:09 PM

keyserver/src/responders/message-responders.js
250 ↗	(On Diff #20829)	Also found that with text messages you guys do this: const messageData: TextMessageData = { type: messageTypes.TEXT, threadID, creatorID: viewer.id, time: Date.now(), text, }; if (localID) { messageData.localID = localID; } The only issue with this variation is that we need to make `localID` in `ReactionMessageData` type be writeable. I personally think your 2nd option is the best way, but just wanted to bring this to your attention