Page MenuHomePhabricator
Differential D5853

[native] introduced useCanCreateReactionFromMessage function to reaction message utils
ClosedPublic
Actions

Authored by ginsu on Dec 12 2022, 1:01 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:15 PM
Unknown Object (File)
Fri, Nov 8, 4:51 PM
Unknown Object (File)
Fri, Nov 8, 10:00 AM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
atul
tomek
rohan
ashoat
Commits
rCOMM552d15a09663: [native] introduced useCanCreateReactionFromMessage function to reaction…
Summary

introduced useCanCreateReactionFromMessage function to reaction message utils. useCanCreateReactionFromMessage does several checks if the user can send a reaction message and depending on the result of those checks will display the react option to the message tooltip. The function checks if the message has been realized (only realized messages have an id), it also checks if the reaction message creator is currently is not blocked by the other target message creator, and it also checks if the reaction message creator has permission to voice in the thread

Depends on D5780
Linear Task: ENG-2246

Test Plan

Please watch the demo videos to see the changes i made (in the middle of the demo, there is some hangtime where I am waiting for simulator to reconnect, feel free to fast forward through that part):

Demos to show scenario where a blocked/blocker pair of users is in a group chat:

Dan's (blocker) POV:

Ryan's (blocked) POV:

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.Dec 12 2022, 1:01 PM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 12 2022, 1:01 PM
Harbormaster completed remote builds in B14093: Diff 19293.Dec 12 2022, 1:14 PM
ginsu requested review of this revision.Dec 12 2022, 1:14 PM
ginsu edited the summary of this revision. (Show Details)Dec 12 2022, 1:17 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, tomek, rohan.
ginsu added a parent revision: D5780: [native] introduce onPressReact to reaction message utils.
ginsu attached a referenced file: F281389: Screen Recording 2022-12-12 at 4.14.43 PM.mov. (Show Details)
ginsu mentioned this in D5811: [lib] introduced reactions field to chatMessageItems.
ginsu added a child revision: D5783: [native] introduced react option to the message tooltip.Dec 12 2022, 1:39 PM
rohan accepted this revision.Dec 12 2022, 1:55 PM

Seems logical to me after watching the demo

tomek accepted this revision.Dec 13 2022, 7:01 AM
tomek added a reviewer: ashoat.

Accepting because the code looks ok

native/chat/reaction-message-utils.js
94 ↗(On Diff #19293)

We can return a relationship to make this more efficient

102–104 ↗(On Diff #19293)

Are we sure that we want to block reactions depending on the relationship? I guess in personal chats it makes sense, but I'm not sure about the public ones. @ashoat do you have an opinion on this?

Regardless, we have to make sure that this is consistent with server and web logic.

ashoat requested changes to this revision.Dec 13 2022, 4:07 PM

Back to you with questions

native/chat/reaction-message-utils.js
102–104 ↗(On Diff #19293)

Hmm... I'm also curious about how we normally treat a scenario where a blocked/blocker pair of users is in a group chat.

@ginsu, two questions for you about that scenario:

  1. Can the blocked user see the blocker's messages?
  2. Can the blocked user start a thread on the blocker's messages?

Regardless, we have to make sure that this is consistent with server and web logic.

Good reminder!!

This revision now requires changes to proceed.Dec 13 2022, 4:07 PM
ginsu updated this revision to Diff 19345.Dec 14 2022, 8:58 AM
ginsu edited the test plan for this revision. (Show Details)

addressed feedback

ginsu edited the test plan for this revision. (Show Details)Dec 14 2022, 9:01 AM
ginsu attached a referenced file: F284914: Screen Recording 2022-12-14 at 11.50.37 AM.mov. (Show Details)
ginsu attached a referenced file: F284908: Screen Recording 2022-12-14 at 11.51.51 AM.mov. (Show Details)
ginsu added inline comments.Dec 14 2022, 9:09 AM
native/chat/reaction-message-utils.js
102–104 ↗(On Diff #19293)

Updated the test plan to have a demo to show this scenario but the TLDR is this:

  1. The blocked user CAN see the blocker's message
  2. The blocked user CANNOT start a thread on the blocker's method

I tried making the logic for the react tooltip option consistent with what the thread tooltip option has, but let me know if we should be doing something else!

Harbormaster completed remote builds in B14145: Diff 19345.Dec 14 2022, 9:11 AM
ashoat accepted this revision.Dec 14 2022, 10:14 AM

Cool!! Please make sure privacy/blocking logic is consistent across web/native/keyserver

atul accepted this revision.Dec 15 2022, 9:26 AM

Looks good

Please respond to following before landing:

In D5853#175595, @ashoat wrote:

Cool!! Please make sure privacy/blocking logic is consistent across web/native/keyserver

This revision is now accepted and ready to land.Dec 15 2022, 9:26 AM
ginsu updated this revision to Diff 19747.Dec 19 2022, 4:45 PM

changed variable names to be more descriptive

Harbormaster completed remote builds in B14491: Diff 19747.Dec 19 2022, 4:56 PM
ginsu added a comment.Dec 19 2022, 5:03 PM
In D5853#175595, @ashoat wrote:

Cool!! Please make sure privacy/blocking logic is consistent across web/native/keyserver

Found an inconsistency in the privacy/blocking logic in the keyserver and updated D5749 to resolve the inconsistency

ginsu updated this revision to Diff 19900.Dec 20 2022, 3:42 PM

rebase before landing

Harbormaster completed remote builds in B14594: Diff 19900.Dec 20 2022, 3:54 PM
Closed by commit rCOMM552d15a09663: [native] introduced useCanCreateReactionFromMessage function to reaction… (authored by ginsu). · Explain WhyDec 20 2022, 4:11 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMM552d15a09663: [native] introduced useCanCreateReactionFromMessage function to reaction….

Revision Contents
Changeset List

PathSize
native/
chat/
36 lines

Diff 19747

View Options

native/chat/reaction-message-utils.js

Loading...