[$250] Emoji picker - Clicking on same emoji reaction changes skin tone instead of removing

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.10-7
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): bezawitgebremichael+kk@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com
2. Open a chat
3. Send a message
4. Hover over the message and click on the emoji plus icon
5. Click on "Change default skin tone"
6. Click on any skin tones other than the current one
7. Click on thumbs up reaction
8. Hover over the sent message and click on emoji plus icon again
9. Click on "Change default skin tone"
10. Click on the skin tone different than the current selected one
11. Click on the thumbs up reaction added previously with a different skin tone (the one under the sent message)

Expected Result:

When changing the default skin tone at step 9 the thumbs up reaction added changes its skin tone as well and when clicking on the thumbs up reaction again the reaction gets removed

Actual Result:

When changing the default skin tone at step 9 the thumbs up reaction added doesn't change its skin tone and when clicking on the same reaction again with a different skin tone the reaction skin tone changes instead of removing the reaction

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6550237_1721710182409.Button_not_working.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01028ab74a5fe53b78
• Upwork Job ID: 1818859215495322018
• Last Price Increase: 2024-08-01

Issue Owner

Current Issue Owner: @rushatgabhane

[melvin-bot]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@kadiealexander FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[nyomanjyotisa]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Clicking on same emoji reaction changes skin tone instead of removing

What is the root cause of that problem?

While checking if we should remove the emoji reaction or not, we include the skinTone on hasAccountIDEmojiReacted param

App/src/libs/actions/Report.ts

Lines 2569 to 2574 in 98d8a5a

	const skinTone = emoji.types === undefined ? -1 : paramSkinTone;
	if (existingReactionObject && EmojiUtils.hasAccountIDEmojiReacted(currentUserAccountID, existingReactionObject.users, skinTone)) {
	removeEmojiReaction(originalReportID, reportAction.reportActionID, emoji);
	return;
	}

What changes do you think we should make in order to solve the problem?

We should remove skinTone from hasAccountIDEmojiReacted call

    if (existingReactionObject && EmojiUtils.hasAccountIDEmojiReacted(currentUserAccountID, existingReactionObject.users)) {
        removeEmojiReaction(originalReportID, reportAction.reportActionID, emoji);
        return;
    }

RESULT

-1-New-Expensify.14.mp4

What alternative solutions did you explore? (Optional)

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Clicking on added emoji reaction with a different skin tone add the same emoji with the selected skin tone.

What is the root cause of that problem?

When we toggle the emoji reaction, it checks whether the emoji with the selected skin tone already added by the current user or not.

App/src/libs/actions/Report.ts

Lines 2569 to 2576 in 98d8a5a

	const skinTone = emoji.types === undefined ? -1 : paramSkinTone;
	if (existingReactionObject && EmojiUtils.hasAccountIDEmojiReacted(currentUserAccountID, existingReactionObject.users, skinTone)) {
	removeEmojiReaction(originalReportID, reportAction.reportActionID, emoji);
	return;
	}
	addEmojiReaction(originalReportID, reportAction.reportActionID, emoji, skinTone);

Implement to add the same emoji but with different skin tones

If it's not, then it will add the emoji with the skin tone selected. That's how it's implemented from the first in #15125.

What changes do you think we should make in order to solve the problem?

If we want that pressing the added reaction to always remove the emoji,

we can update toggleEmojiReaction to accept a new params ignoreSkinToneOnCompare.

App/src/libs/actions/Report.ts

Lines 2544 to 2550 in 98d8a5a

	function toggleEmojiReaction(
	reportID: string,
	reportAction: ReportAction,
	reactionObject: Emoji,
	existingReactions: OnyxEntry<ReportActionReactions>,
	paramSkinTone: number = preferredSkinTone,
	) {

If it's true, we pass undefined skinTone to hasAccountIDEmojiReacted. It will only be true in the added reaction bubble.

App/src/pages/home/report/ReportActionItem.tsx

Lines 383 to 388 in 98d8a5a

	const toggleReaction = useCallback(
	(emoji: Emoji) => {
	Report.toggleEmojiReaction(report.reportID, action, emoji, emojiReactions);
	},
	[report, action, emojiReactions],
	);

This way, pressing the emoji from emoji picker or context menu will still compare the skin tone too, but pressing the added emoji reaction will ignore the skin tone.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01028ab74a5fe53b78

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

[melvin-bot]

@rushatgabhane, @kadiealexander Huh... This is 4 days overdue. Who can take care of this?

[rushatgabhane]

I like @bernhardoj's proposal 🎀 👀 🎀 It will be cleaner to implement and avoid regressions

[melvin-bot]

Triggered auto assignment to @chiragsalian, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[chiragsalian]

Proposal LGTM, feel free to create the PR @bernhardoj

[bernhardoj]

PR is ready

cc: @rushatgabhane

[rushatgabhane]

@kadiealexander could you please attach a payment summary

[kadiealexander]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@rushatgabhane] The PR that introduced the bug has been identified. Link to the PR:
• [@rushatgabhane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@rushatgabhane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@rushatgabhane] Determine if we should create a regression test for this bug.
• [@rushatgabhane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[kadiealexander]

Payouts due:

• Contributor: $250 @bernhardoj via manual request
• Contributor+: $250 @rushatgabhane via manual request

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj

[melvin-bot]

@chiragsalian, @rushatgabhane, @bernhardoj, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

[kadiealexander]

@rushatgabhane bump on the checklist please!

[garrettmknight]

$250 approved for @rushatgabhane

[melvin-bot]

@chiragsalian, @rushatgabhane, @bernhardoj, @kadiealexander Eep! 4 days overdue now. Issues have feelings too...

[chiragsalian]

Bumped rushat via newDot DM to handle the checklist.

[rushatgabhane]

1. The PR that introduced the bug has been identified. Link to the PR: feat: add/remove reaction #15210

2. The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: feat: add/remove reaction #15210 (review)

3. A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N.A.

4. Determine if we should create a regression test for this bug. Yes

5. If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again -

    1. Open any chat and send any message
    2. Add an emoji reaction with a skin tone to the message from the context menu
    3. Repeat step 2 but with a different skin tone
    4. Verify the emoji is added alongside the previous emoji reaction
    5. Now, change the skin tone again from the add reaction picker to a different one (different from step 2 and 3) but don't select any emoji
    6. Press the added emoji reaction bubble
    7. Verify the emoji reaction is removed
   

[stitesExpensify]

👋 This was not a bug, it was intended functionality. The same functionality happens in slack, and we built it that way to match.

2024-10-02_09-48-28.mp4

For next steps I think that we should revert the PR, and maybe update the regression tests to make sure that it matches? I thought we already had a case for this but I guess not. If that is the case though I'm surprised that it took this long for someone to find as a "bug".

Thoughts @chiragsalian ?

[bernhardoj]

I think what we did here matches the video.

[chiragsalian]

Why revert the PR? i think the code here is an improvement IMO so I'm not following why we want to revert it.
I think when i change skin tone and click on my existing emoji i would want it to be removed, which is the same that happens on slack.

[stitesExpensify]

I believe that @bernhardoj was correct, and I was misunderstanding the change here. My apologies. No need to revert the PR or make any changes!

[melvin-bot]

@chiragsalian, @rushatgabhane, @bernhardoj, @kadiealexander Eep! 4 days overdue now. Issues have feelings too...