Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-03-22] Сhat - On selecting skin tone emoji symbol disappears #38049

Closed
3 of 6 tasks
lanitochka17 opened this issue Mar 11, 2024 · 21 comments
Closed
3 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Mar 11, 2024

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: 1.4.50
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4417529
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Open a conversation
  3. Enter :wave
  4. Note suggestions shown
  5. Enter colon at end 👋
  6. Note emoji shown
  7. Clear the message
  8. Enter :wave
  9. Tap emoji picker and select different skin tone
  10. Tap outside

Expected Result:

Emoji symbol must not disappear in suggestion list

Actual Result:

Emoji symbol disappears in suggestion list

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

Bug6409714_1710167224561.skin.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Mar 11, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Mar 11, 2024

Triggered auto assignment to @rlinoz (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@lanitochka17
Copy link
Author

@rlinoz 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
Copy link
Author

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

@suneox
Copy link
Contributor

suneox commented Mar 11, 2024

Proposal

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

Сhat - On selecting skin tone emoji symbol disappears

What is the root cause of that problem?

We have a logic at this line return undefined if the skin tone index is 0

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

Instead of check !preferredSkinToneIndex we can check preferredSkinToneIndex === undefined or preferredSkinToneIndex === null

-   if (!preferredSkinToneIndex) {
+   if (preferredSkinToneIndex === undefined || preferredSkinToneIndex === null) {
        return;
    }

POC

Screen.Recording.2024-03-11.at.22.12.31.mov

What alternative solutions did you explore? (Optional)

Simple condition

const getEmojiCodeWithSkinColor = (item: Emoji, preferredSkinToneIndex: OnyxEntry<string | number>): string | undefined => {
    const {code, types} = item;
    if (typeof preferredSkinToneIndex !== 'number') {
        return;
    }

    if (types?.[preferredSkinToneIndex]) {
        return types[preferredSkinToneIndex];
    }

    return code;
};

Solution 2

-    if (!preferredSkinToneIndex) {
-       return;
-   }
+   if (typeof preferredSkinToneIndex === 'number' && types?.[preferredSkinToneIndex]) {
        return types[preferredSkinToneIndex];
    }

@Krishna2323
Copy link
Contributor

Krishna2323 commented Mar 11, 2024

Proposal

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

Сhat - On selecting skin tone emoji symbol disappears

What is the root cause of that problem?

We return from getEmojiCodeWithSkinColor when !preferredSkinToneIndex is true and the last emoji color code has the index of 0 that's why !preferredSkinToneIndex becomes true.
Offending PR: #34487

const getEmojiCodeWithSkinColor = (item: Emoji, preferredSkinToneIndex: OnyxEntry<string | number>): string | undefined => {

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

Instead of !preferredSkinToneIndex we should check for typeof preferredSkinToneIndex !== 'number'.

Result

Alternatively

Create a else statement here and return.

App/src/libs/EmojiUtils.ts

Lines 251 to 253 in 3d4504e

if (typeof preferredSkinToneIndex === 'number' && types?.[preferredSkinToneIndex]) {
return types[preferredSkinToneIndex];
}

Alternative 2

Change the condition to:

 if (typeof preferredSkinToneIndex !== 'number' || !types?.[preferredSkinToneIndex]) {
       return;
   }

@Krishna2323
Copy link
Contributor

@suneox, you updated your proposal after mine, pls comment Proposal Updated.

@suneox
Copy link
Contributor

suneox commented Mar 11, 2024

@Krishna2323 you also updated the proposal so I think you also commented Proposal Updated, I just cleaned up the condition the RCA is still not updated and the original proposal also works without an update

@aimane-chnaif
Copy link
Contributor

For deploy blockers, offending PR should be pointed out in root cause

@Krishna2323
Copy link
Contributor

Offending PR: #34487

@suneox
Copy link
Contributor

suneox commented Mar 11, 2024

PR #34487

@Krishna2323
Copy link
Contributor

Proposal Updated

Added alternative 2

@aimane-chnaif
Copy link
Contributor

Looks like @suneox proposed first with the correct root cause.
This is not yet External so offending PR author (cc: @kubabutkiewicz) has still priority to fix this.
cc: @rlinoz

@suneox
Copy link
Contributor

suneox commented Mar 11, 2024

I also update my proposal alternative 2, the reason change on the PR is preferredSkinToneIndex include null value so the author of PR check !preferredSkinToneIndex to cleanup null value skip typecheck

@rlinoz
Copy link
Contributor

rlinoz commented Mar 11, 2024

I'm removing the deploy blocker label as this is not a crucial path and sending emojis still works.

@rlinoz rlinoz added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Mar 11, 2024
@rlinoz
Copy link
Contributor

rlinoz commented Mar 11, 2024

As @aimane-chnaif pointed out this should be handled by @kubabutkiewicz if possible.

@kubabutkiewicz
Copy link
Contributor

I will take a look on this today @rlinoz

@melvin-bot melvin-bot bot added Reviewing Has a PR in review and removed Daily KSv2 labels Mar 12, 2024
@melvin-bot melvin-bot bot added the Weekly KSv2 label Mar 12, 2024
@kubabutkiewicz
Copy link
Contributor

PR ready @rlinoz

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 15, 2024
@melvin-bot melvin-bot bot changed the title Сhat - On selecting skin tone emoji symbol disappears [HOLD for payment 2024-03-22] Сhat - On selecting skin tone emoji symbol disappears Mar 15, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 15, 2024
Copy link

melvin-bot bot commented Mar 15, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Mar 15, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.52-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-03-22. 🎊

@rlinoz
Copy link
Contributor

rlinoz commented Mar 15, 2024

No payment due, closing.

@rlinoz rlinoz closed this as completed Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants