Page MenuHomePhabricator

[keyserver] modified createMessages function to handle inserting targetMessageID into the DB
ClosedPublic

Authored by ginsu on Nov 28 2022, 4:15 PM.
Tags
None
Referenced Files
F3280528: D5747.id19902.diff
Sat, Nov 16, 9:13 AM
Unknown Object (File)
Fri, Nov 15, 2:25 AM
Unknown Object (File)
Mon, Nov 4, 1:20 PM
Unknown Object (File)
Sat, Nov 2, 6:04 AM
Unknown Object (File)
Tue, Oct 29, 12:36 AM
Unknown Object (File)
Mon, Oct 28, 7:49 PM
Unknown Object (File)
Mon, Oct 28, 7:32 AM
Unknown Object (File)
Sun, Oct 27, 8:33 PM
Subscribers

Details

Summary

modified createdMessages function to handle inserting targetMessageID into the DB. For these changes, I needed to check if a targetMessageID existed on the messageData object, if it did assign it to the targetMessageID variable otherwise assign targetMessageID to null. Then we wanted to modify the messageInsertQuery to handle setting targetMessageID to target_message in the db


Linear Task: ENG-2246

Test Plan

Please see the screenshots below to see what the reaction message looks like in the db. For this example I liked the message "as you inevitably discover bugs, have feature requests..."

Screenshot 2022-11-28 at 7.16.55 PM.png (2×3 px, 1 MB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, rohan, tomek.
ginsu edited the test plan for this revision. (Show Details)
ginsu requested review of this revision.Nov 28 2022, 4:28 PM

Is there any other place where we construct rows like this? Does the new target_message column have a default value?

keyserver/src/creators/message-creator.js
134–136 ↗(On Diff #18902)

I would just do nullish coalescing

174–175 ↗(On Diff #18902)

Please keep lines to 80 chars

addressed ashoat's comments

Is there any other place where we construct rows like this?

No, did a quick search to double check, but this is the only place we insert rows into the messages table

Does the new target_message column have a default value?

Yes, the default value is null, this was handled in this diff: D5658

keyserver/src/creators/message-creator.js
134–136 ↗(On Diff #18961)

With nullish coalescing, flow gets unhappy if one of the fields is not common (creatorID, threadID, time, and type) throughout messageData. If we use a unique field to a specific MessageData type we get this error:

Screenshot 2022-11-29 at 10.27.03 AM.png (864×1 px, 375 KB)

Another example with localID:

Screenshot 2022-11-29 at 10.28.12 AM.png (852×2 px, 390 KB)

This revision is now accepted and ready to land.Nov 30 2022, 2:20 PM