Fix/18554: Update reaction dynamically in the reaction popup #18701

dukenv0307 · 2023-05-10T05:09:41Z

Detail

We will move the logic to get information of the reaction to PopoverReactionList.js itself. So, this component could be dynamically updated

Fixed Issues

$ #18554
PROPOSAL: #18554 (comment)

Tests

On device 1 and login with User A and go to chat with user B
On device 2 and login with User B
On device 1 send a message to user B and react to that message. Long press on that emoji to see the emoji reaction number and the user who reacted it (Do not close the popup )
Now on device 2, from user B, react to the above message
Verify that the emoji reaction number and the users who reacted to the message are dynamically updated on the popup on device 1

Verify that no errors appear in the JS console

Offline tests

None

QA Steps

On device 1 and login with User A and go to chat with user B
On device 2 and login with User B
On device 1 send a message to user B and react to that message. Long press on that emoji to see the emoji reaction number and the user who reacted it (Do not close the popup )
Now on device 2, from user B, react to the above message
Verify that the emoji reaction number and the users who reacted to the message are dynamically updated on the popup on device 1

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Record_2023-05-10-16-22-58.mp4

Mobile Web - Safari && IOS

Screen-Recording-2023-05-10-at-16.42.53.mp4

Desktop

iOS

In the safari section

Android

18554.mp4

melvin-bot · 2023-05-10T09:43:46Z

@cristipaval @rushatgabhane One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

rushatgabhane · 2023-05-10T15:23:55Z

@dukenv0307 kindly fix the merge conflicts

dukenv0307 · 2023-05-10T15:38:15Z

@rushatgabhane everything is updated

rushatgabhane · 2023-05-10T17:46:15Z

@dukenv0307 can you sign the CLA please

rushatgabhane · 2023-05-10T17:46:28Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-05-16.at.04.30.18.mov

Mobile Web - Chrome

Screen.Recording.2023-05-16.at.04.41.53.mov

Mobile Web - Safari

Screen.Recording.2023-05-16.at.04.42.17.mov

Desktop

Screen.Recording.2023-05-16.at.04.35.08.mov

iOS

Android

Screen.Recording.2023-05-16.at.04.49.25.mov

dukenv0307 · 2023-05-11T03:53:54Z

@rushatgabhane Sorry about my late. I just updated, help to check again

dukenv0307 · 2023-05-12T13:00:41Z

@rushatgabhane Bump, please help to review

strepanier03 · 2023-05-13T22:47:30Z

@rushatgabhane - Little bump for the PR review here the next time you're online and available. Thanks!

rushatgabhane · 2023-05-15T08:52:28Z

Reviewing today!

src/components/Reactions/ReportActionItemReactions.js

rushatgabhane · 2023-05-15T23:07:50Z

@dukenv0307 everything else looks good to me, nice work on the PR!

rushatgabhane

@cristipaval LGTM!

rushatgabhane · 2023-05-16T03:57:06Z

@dukenv0307 can you please merge with main and fix the lint errors

rushatgabhane · 2023-05-16T06:42:41Z

@dukenv0307 we still need to fix the lint errors - there's a prettier diff. Please push it!

rushatgabhane

LGTM! @cristipaval

dukenv0307 · 2023-05-17T03:59:47Z

@cristipaval Friendly bump

OSBotify · 2023-05-17T15:25:35Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

tgolen · 2023-05-17T21:05:33Z

src/pages/home/report/ReactionList/PopoverReactionList.js

+import lodashMap from 'lodash/map';
+import lodashFilter from 'lodash/filter';
+import lodashFind from 'lodash/find';
+import lodashEach from 'lodash/each';
+import lodashIsEqual from 'lodash/isEqual';


Using underscore is our standard to keep everything consistent: https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#object--array-methods

Please remove these and use underscore instead.

lodashGet is the only one we allow since underscore doesn't have an equivalent.

@tgolen @rushatgabhane I just created PR to fix: #19183

tgolen · 2023-05-17T21:07:32Z

src/pages/home/report/ReactionList/PopoverReactionList.js

+ * @param {Array} users
+ * @return {string[]}
+ * */
+const getUniqueEmojiCodes = (emoji, users) => {


This method is now defined twice and needs to be DRYed up https://github.com/Expensify/App/blob/main/src/components/Reactions/ReportActionItemReactions.js#L24

tgolen · 2023-05-17T21:08:13Z

src/pages/home/report/ReactionList/PopoverReactionList.js

+    withOnyx({
+        reportActions: {
+            key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
+            canEvict: false,


Why is this here?

@rushatgabhane The PR is merged, so do we create a new PR to fix it or revert this PR?

what is wrong with it tho? @dukenv0307

and about other comments, you can create a new PR to address them and tag me for a review

This is an explanation about canEvict field
https://github.com/Expensify/react-native-onyx/blob/1d496b001f327c509131597faf260a81c002be77/lib/withOnyx.js#L113-L116
@tgolen @rushatgabhane I see that canEvict: false is added when we get reportAction everywhere in App repo. So I think in here we also add it for consistency

Oh. But that's not a good reason.

When I initially reviewed this, this is how I reasoned it - When storage is full, don't remove reactions from storage. Which seems reasonable to me.

I'm pretty sure the eviction logic is completely broken, and I think this should just be removed, but maybe it should be there for report actions... I do see it used everywhere. @vitHoracek @marcaaron do you have any guidance on the canEvict setting for report actions?

cc @mountiny some guidance on above please

we tagged the fake @vitHoracek

How did we deduce that the eviction logic was completely broken? Is there more context about that? I'm not aware of any issues with it, but perhaps it is broken.

The guidance on canEvict is that you should pass false if it's used for something that the UI depends on e.g. you don't want to evict report actions that are currently in view, but it is fine to remove them for a report that is not in view.

In reality, only report actions are evicted and when we set this up there weren't many places where we would disallow eviction other than the one place where we subscribed to a reportActions_<reportID> key that was needed for the active screen's UI (i.e. the chat screen).

Ok, I did some poking and all the places where it is being used look kosher to me. There is documented guidance here: https://github.com/expensify/app#storage-eviction

Open to improvements or suggestions. I think there's probably a better version of storage eviction waiting to be built at some point in all of our imaginations - but this one has gonna us pretty far. Let me know if there's anything else I can help with 🙇‍♂️

@marcaaron The reason I believe that the eviction logic is broken is from this line here: https://github.com/Expensify/react-native-onyx/blob/main/lib/withOnyx.js#L129

When I was working with Thaibault (during ExpensiconX) and looking at migrating Onxy to TypeScript, we found that mapping.connectionID is undefined. This is because:

The connectionID gets saved to the mapping here in Onyx.connect()

We assume that the mapping there is a reference to the same mapping object that is in withOnyx but actually, it's a copy as we can see by the spread operator being used on mapping here when it is passed to Onyx.connect()

checkEvictableKeys() then looks through all the mappings in mapOnyxToState here and references mapping.connectionID. This is where it breaks 💥 because that object never had connectionID saved to it, the connectionID was only saved to a copy of it.

OSBotify · 2023-05-18T07:38:45Z

🚀 Deployed to staging by https://github.com/cristipaval in version: 1.3.16-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-05-22T23:40:12Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.16-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-05-23T00:16:07Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.16-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-05-23T19:54:01Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.16-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-05-23T21:52:45Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.16-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

abdulrahuman5196 · 2023-06-21T13:46:34Z

src/pages/home/report/ReactionList/PopoverReactionList.js

+        const emoji = lodashFind(emojis, (e) => e.name === selectedReaction.emoji);
+        const emojiCodes = getUniqueEmojiCodes(emoji, selectedReaction.users);
+        const hasUserReacted = Report.hasAccountIDReacted(this.props.currentUserPersonalDetails.accountID, reactionUsers);
+        const users = PersonalDetailsUtils.getPersonalDetailsByIDs(reactionUsers);


We missed to pass the accountID as the second param during this refractor causing this issue - #20267

dukenv0307 added 3 commits May 10, 2023 00:37

fix: add reportID reportActionID to reaction list

c70b7a7

fix: merge main

a76ac8e

fix: move logic to Poopver reaction list

fdddac6

dukenv0307 force-pushed the fix/18554 branch from fda3120 to fdddac6 Compare May 10, 2023 08:54

fix: merge main

2368ccf

dukenv0307 marked this pull request as ready for review May 10, 2023 09:43

dukenv0307 requested a review from a team as a code owner May 10, 2023 09:43

melvin-bot bot requested review from cristipaval and rushatgabhane and removed request for a team May 10, 2023 09:43

dukenv0307 mentioned this pull request May 10, 2023

[HOLD for payment 2023-06-02] [$1000] The emoji reaction number and the user who reached the emoji doesn't dynamically update on mweb chrome #18554

Closed

6 tasks

dukenv0307 changed the title ~~Fix/18554~~ Fix/18554: Update reaction dynamically in the reaction popup May 10, 2023

fix: remove redundant

b47754c

fix: merge main

8611ccd

dukenv0307 force-pushed the fix/18554 branch from a26ca33 to 8611ccd Compare May 10, 2023 17:28

fix: merge main

156b2fd

rushatgabhane suggested changes May 15, 2023

View reviewed changes

src/components/Reactions/ReportActionItemReactions.js Show resolved Hide resolved

rushatgabhane previously approved these changes May 15, 2023

View reviewed changes

fix: add comment

1c45f51

dukenv0307 dismissed rushatgabhane’s stale review via 1c45f51 May 16, 2023 01:11

rushatgabhane previously approved these changes May 16, 2023

View reviewed changes

fix: merge main

8c02276

fix: lint

92adcb6

dukenv0307 dismissed rushatgabhane’s stale review via 92adcb6 May 16, 2023 06:47

rushatgabhane approved these changes May 16, 2023

View reviewed changes

cristipaval approved these changes May 17, 2023

View reviewed changes

cristipaval merged commit a108f42 into Expensify:main May 17, 2023

tgolen reviewed May 17, 2023

View reviewed changes

OSBotify mentioned this pull request May 18, 2023

Deploy Checklist: New Expensify 2023-05-18 #19175

Closed

39 tasks

dukenv0307 mentioned this pull request May 18, 2023

[NO QA] Fix comment in PR 18701 #19183

Merged

55 tasks

abdulrahuman5196 reviewed Jun 21, 2023

View reviewed changes

abdulrahuman5196 mentioned this pull request Jun 21, 2023

[HOLD for payment 2023-06-28] [$1000] Chat - Email address is displayed instead of 'You' when pulling up the reaction detail #20267

Closed

6 tasks

fedirjh mentioned this pull request Jun 23, 2023

[HOLD for payment 2023-06-19] [$1000] The name/picture is not dynamically updated if the emoji popup is open but works well with emoji numbers #19788

Closed

6 tasks

Fix/18554: Update reaction dynamically in the reaction popup #18701

Fix/18554: Update reaction dynamically in the reaction popup #18701

Conversation

dukenv0307 commented May 10, 2023 • edited Loading

Detail

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented May 10, 2023

rushatgabhane commented May 10, 2023

dukenv0307 commented May 10, 2023

rushatgabhane commented May 10, 2023

rushatgabhane commented May 10, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

dukenv0307 commented May 11, 2023

dukenv0307 commented May 12, 2023

strepanier03 commented May 13, 2023

rushatgabhane commented May 15, 2023

rushatgabhane commented May 15, 2023 • edited Loading

rushatgabhane left a comment

Choose a reason for hiding this comment

rushatgabhane commented May 16, 2023

rushatgabhane commented May 16, 2023 • edited Loading

rushatgabhane left a comment

Choose a reason for hiding this comment

dukenv0307 commented May 17, 2023

OSBotify commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented May 18, 2023

OSBotify commented May 22, 2023

OSBotify commented May 23, 2023

OSBotify commented May 23, 2023

OSBotify commented May 23, 2023

Choose a reason for hiding this comment

dukenv0307 commented May 10, 2023 •

edited

Loading

rushatgabhane commented May 10, 2023 •

edited

Loading

rushatgabhane commented May 15, 2023 •

edited

Loading

rushatgabhane commented May 16, 2023 •

edited

Loading

rushatgabhane May 22, 2023 •

edited

Loading