Details

Reviewers

atul
tomek
rohan

Commits

rCOMM8a3e94c97c73: [lib] introduced rawMessageInfoFromServerDBRow function to reaction message spec

Summary

introduced rawMessageInfoFromServerDBRow function to reaction message spec. rawMessageInfoFromServerDBRow does some basic parsing of the message content from the db and returns the information necessary for a RawReactionMessageInfo object

UPDATE

Got some feedback to grab targetMessageID directly from the messages table instead of from messageContent. To do this successfully, I had to modify some of the message-fetchers queries to include targetMessageID in the query selection.

Depends on D5690
Linear Task: ENG-2245

Test Plan

Ran web and mobile app locally and nothing crashes/there are no regressions. More testing of this function will come in subsequent diffs.

UPDATE

To test that the queries were returning the correct information, I logged out row.targetMessageID.toString() and got this result, which matched what I found in the database:

Screenshot 2022-12-07 at 5.16.14 PM.png (166×424 px, 78 KB)

Screenshot 2022-12-07 at 5.34.55 PM.png (1×3 px, 1 MB)

(also note that the message content no longer has targetMessageID)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Dec 2 2022, 2:57 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 2 2022, 2:57 PM

ginsu edited the summary of this revision. (Show Details)Dec 2 2022, 2:58 PM

ginsu added reviewers: atul, tomek, rohan.

ginsu added a parent revision: D5690: [lib] introduced reaction message spec boilerplate.

Harbormaster completed remote builds in B13954: Diff 19120.Dec 2 2022, 3:09 PM

ginsu requested review of this revision.Dec 2 2022, 3:09 PM

tomek requested changes to this revision.Dec 6 2022, 6:00 AM

tomek added inline comments.

lib/shared/messages/reaction-message-spec.js
60 ↗	(On Diff #19120)	Is it intentional to not convert this id to string? What is its type in the db?
62 ↗	(On Diff #19120)	Are we still using this name?

This revision now requires changes to proceed.Dec 6 2022, 6:00 AM

rebase with action field

ginsu added inline comments.Dec 6 2022, 10:05 AM

lib/shared/messages/reaction-message-spec.js
60 ↗	(On Diff #19120)	Yea `content.targetMessageID` is already a string where the other IDs like `threadID` are numbers so we need to convert them to strings
62 ↗	(On Diff #19120)	Good catch! forgot to push the rebase for this diff yesterday

ginsu attached a referenced file: F274633: Screenshot 2022-12-06 at 1.01.17 PM.png. (Show Details)Dec 6 2022, 10:05 AM

Harbormaster completed remote builds in B14017: Diff 19192.Dec 6 2022, 10:15 AM

tomek requested changes to this revision.Dec 7 2022, 3:02 AM

tomek added inline comments.

lib/shared/messages/reaction-message-spec.js
60 ↗	(On Diff #19120)	Something is wrong here. `targer_message` is defined separately in `message` table `target_message bigint(20) DEFAULT NULL` and is not a part of `content` json field. (at least on master). We have to make sure that type of `id` matches `target message id` and that the proper value source is used.

This revision now requires changes to proceed.Dec 7 2022, 3:02 AM

ginsu added inline comments.Dec 7 2022, 6:52 AM

lib/shared/messages/reaction-message-spec.js
60 ↗	(On Diff #19120)	In D5698, I introduced `messageContentForServerDB` which handles putting the `content` into the DB. In this method, I did include `targetMessageID` in the content; however, after your comments and rethinking it, this might be unnecessary if we are putting `targetMessageID` separately in the `message` table` as `target_message`. Let me know what you think, and hopefully, this clears up the confusion.

addressed tomek's feedback

Harbormaster completed remote builds in B14053: Diff 19240.Dec 7 2022, 2:31 PM

ginsu edited the summary of this revision. (Show Details)Dec 7 2022, 2:35 PM

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

ginsu attached a referenced file: F277032: Screenshot 2022-12-07 at 5.34.55 PM.png. (Show Details)

ginsu attached a referenced file: F277031: Screenshot 2022-12-07 at 5.16.14 PM.png. (Show Details)

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

tomek requested changes to this revision.Dec 8 2022, 4:10 AM

tomek added inline comments.

keyserver/src/fetchers/message-fetchers.js
590–594 ↗	(On Diff #19240)	It might be a good idea to also update this select.
lib/shared/messages/reaction-message-spec.js
63 ↗	(On Diff #19240)	What happens when `target_message` isn't set. Is it safe to call `toString` on the result in such a case?
60 ↗	(On Diff #19120)	I think there's no need to write target message twice into the db. We introduced a separate column for that so that we can e.g. query by this and use an index.

This revision now requires changes to proceed.Dec 8 2022, 4:10 AM

ginsu added inline comments.Dec 8 2022, 7:50 AM

keyserver/src/fetchers/message-fetchers.js
590–594 ↗	(On Diff #19240)	I didn't include this select because local messages won't have reactions at the moment, but I can update this select since we will probably one day want local reaction messages too
lib/shared/messages/reaction-message-spec.js
63 ↗	(On Diff #19240)	all reaction messages will have a `target_message` set but I will definitely add an invariant to make it extra safe

tomek added inline comments.Dec 8 2022, 7:59 AM

keyserver/src/fetchers/message-fetchers.js
590–594 ↗	(On Diff #19240)	While introducing optimistic liking we will probably make it possible for a reaction to be a local message. But regardless, I think that the code between db and js should be as general as possible and should have a minimal amount of business logic involved. So if we try to write this code only as a translation between one model and the other, we would be less likely to introduce bugs in the future.