Details

Reviewers

atul
rohan
tomek

Commits

rCOMMdd21415af96f: [lib] introduced reaction message spec boilerplate

Summary

For this reaction message spec boilerplate, I implemented all of the required methods that a message spec requires. This includes the following functions: messageTitle, rawMessageInfoFromClientDB, createMessageInfo and generatesNotifs. I also added messageContentForClientDB because this goes hand in hand with rawMessageInfoFromClientDB.

For rawMessageInfoFromClientDB, I return the targetMessageID and the reaction and these will be in the content of rawMessageInfoFromClientDB.

For messageTitle I validate the message info and then call and return the results of messagePreviewText which was modified in D5689

For rawMessageInfoFromClientDB I am returning an object of all the necessary info I get from clientDBMessageInfo. This includes getting targetMessageID and reaction which I get from content.

For createMessageInfo I am grabbing the necessary information from rawMessageInfo and creator and returning a ReactionMessageInfo object

As I continue to build out this reaction message spec and implement the other parts of message liking, I might be making some changes to these four functions especially as my understanding improves

Will be adding the other necessary functions for this message spec in subsequent diffs

Depends on D5689
Linear Task: ENG-2245

Test Plan

Ran web and mobile app locally and nothing crashes/will be doing more tests on this message spec in subsequent diffs

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Nov 18 2022, 3:30 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptNov 18 2022, 3:30 PM

ginsu edited the summary of this revision. (Show Details)Nov 18 2022, 3:44 PM

ginsu added reviewers: atul, rohan.

ginsu added a parent revision: D5689: [lib] modified messagePreviewText to handle reaction message types.

Harbormaster completed remote builds in B13587: Diff 18609.Nov 18 2022, 3:44 PM

ginsu requested review of this revision.Nov 18 2022, 3:44 PM

ginsu edited the summary of this revision. (Show Details)Nov 18 2022, 3:49 PM

ginsu added a child revision: D5698: [lib] introduced messageContentForServerDB function in reaction message spec.Nov 21 2022, 6:55 AM

Looks reasonable to me, adding @tomek as blocking to take another look.

lib/shared/messages/reaction-message-spec.js
35–38 ↗	(On Diff #18609)	Looks like we have a function that does this (`lib/shared/message-utils.js:removeCreatorAsViewer(messageInfo: Info)`)... can we use that here?

tomek requested changes to this revision.Nov 22 2022, 5:03 AM

tomek added inline comments.

lib/shared/messages/reaction-message-spec.js
5–11 ↗	(On Diff #18609)	These imports can be merged
28–41 ↗	(On Diff #18609)	What would be the message preview for this type? Have you checked if `messageTitle` function works for reactions?
86 ↗	(On Diff #18609)	Shouldn't we also define the notif text?

This revision now requires changes to proceed.Nov 22 2022, 5:03 AM

ginsu added a child revision: D5746: [lib] introduced shimUnsupportedMessageInfo function to reaction message spec.Nov 28 2022, 4:02 PM

addressed feedback

ginsu added inline comments.Dec 2 2022, 2:19 PM

lib/shared/messages/reaction-message-spec.js
28–41 ↗	(On Diff #18609)	This is what the message preview looks like when a reaction has been sent from my POV and Ashoat's POV. Keeping it generic for now, and eventually will change in a subsequent diff Reaction is sent: My POV: Ashoat's POV:
86 ↗	(On Diff #18609)	These will be introduced in subsequent diffs, I am still testing them out and making sure they behave as expected

ginsu attached a referenced file: F269851: Screenshot 2022-12-02 at 4.56.20 PM.png. (Show Details)Dec 2 2022, 2:20 PM

ginsu attached a referenced file: F269850: Screenshot 2022-12-02 at 4.56.06 PM.png. (Show Details)

ginsu attached a referenced file: F269849: Screenshot 2022-12-02 at 4.56.02 PM.png. (Show Details)

Harbormaster completed remote builds in B13949: Diff 19115.Dec 2 2022, 2:21 PM

ginsu added a child revision: D5803: [lib] introduced rawMessageInfoFromServerDBRow function to reaction message spec.Dec 2 2022, 2:58 PM

ginsu added a child revision: D5804: [lib] introduced rawMessageInfoFromMessageData function to reaction message spec.Dec 2 2022, 3:05 PM

ginsu added a child revision: D5805: [lib] introduce unshimMessageInfo to reaction message spec.Dec 2 2022, 3:22 PM

tomek accepted this revision.Dec 5 2022, 3:11 AM

tomek added inline comments.

lib/shared/messages/reaction-message-spec.js
28–41 ↗	(On Diff #18609)	Makes sense! About text being generic, currently we're supporting only liking, so maybe change `to react` verb with `to like`. In the future, we will probably need to display an emoji because it's impossible to find a verb for each of them.
86 ↗	(On Diff #18609)	In that case, shouldn't we set this flag in a diff that introduces notif text?

This revision is now accepted and ready to land.Dec 5 2022, 3:11 AM

rebased with action field and addressed tomek's comments

Harbormaster completed remote builds in B13976: Diff 19149.Dec 5 2022, 2:28 PM

rebase before landing

Harbormaster completed remote builds in B14041: Diff 19217.Dec 7 2022, 7:32 AM

removed targetMessageID from message content based on some feedback from tomek

forgot to include one change on push

requesting review, because made some bigger changes with getting targetMessageID from clientDBMessageInfo instead of content

Harbormaster completed remote builds in B14049: Diff 19235.Dec 7 2022, 12:42 PM

ginsu added inline comments.Dec 7 2022, 12:45 PM

lib/shared/messages/reaction-message-spec.js
64 ↗	(On Diff #19236)	Unfortunately have to do this, because Flow is not picking up the invariant above and thinks that `target_message_id` could still not be defined... Will circle back to this and try to improve this, but if someone has an idea to fix this please let me know!

Harbormaster completed remote builds in B14050: Diff 19236.Dec 7 2022, 12:45 PM

ClientDBMessageInfo doesn't have a target_message_id field?

lib/shared/messages/reaction-message-spec.js
64 ↗	(On Diff #19236)	ClientDBMessageInfo doesn't have a `target_message_id` field?

This revision now requires changes to proceed.Dec 7 2022, 12:52 PM

addressed feedback and diff is back in state where it was originally approved

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

Harbormaster completed remote builds in B14051: Diff 19238.Dec 7 2022, 1:43 PM

tomek accepted this revision.Dec 8 2022, 4:31 AM

tomek added inline comments.

lib/shared/messages/reaction-message-spec.js
24 ↗	(On Diff #19238)	It might be risky to have `targetMessageID` inside content on client, but as a separate column on server. This might make the code less maintainable, but it is not an urgent thing to fix.