Page MenuHomePhabricator

[lib] introduced reaction message spec boilerplate
ClosedPublic

Authored by ginsu on Nov 18 2022, 3:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 2, 7:29 AM
Unknown Object (File)
Mar 28 2024, 2:13 AM
Unknown Object (File)
Mar 28 2024, 2:07 AM
Unknown Object (File)
Mar 26 2024, 12:54 AM
Unknown Object (File)
Mar 26 2024, 12:54 AM
Unknown Object (File)
Mar 15 2024, 9:52 AM
Unknown Object (File)
Mar 7 2024, 1:49 PM
Unknown Object (File)
Mar 7 2024, 1:31 PM
Subscribers

Details

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 requested review of this revision.Nov 18 2022, 3:44 PM
atul added a reviewer: tomek.

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
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:

Screenshot 2022-12-02 at 4.56.02 PM.png (1×1 px, 857 KB)

My POV:

Screenshot 2022-12-02 at 4.56.06 PM.png (1×1 px, 790 KB)

Ashoat's POV:

Screenshot 2022-12-02 at 4.56.20 PM.png (1×1 px, 792 KB)

86 ↗(On Diff #18609)

These will be introduced in subsequent diffs, I am still testing them out and making sure they behave as expected

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

removed targetMessageID from message content based on some feedback from tomek

forgot to include one change on push

ginsu requested review of this revision.Dec 7 2022, 12:39 PM
ginsu edited the summary of this revision. (Show Details)

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

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!

atul requested changes to this revision.Dec 7 2022, 12:52 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

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.

This revision is now accepted and ready to land.Dec 15 2022, 8:55 AM
lib/shared/messages/reaction-message-spec.js
24 ↗(On Diff #19238)

Created a task to track this