[HOLD for payment 2023-06-13] [$1000] Insert emoji before a colon text after is replaced by "1" string

[kavimuru]

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

---

Action Performed:

1. Open any chat
2. Type :em
3. Move cursor before colon
4. Type :em and select any emoji

Expected Result:

The text after emoji is replace by a space

Actual Result:

The text after emoji is replace by "1" string

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.16-5
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screencast from 17-05-2023 12_25_02.webm

Recording.654.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dukenv0307
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684301189498089

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f4ab1937aacf2d66
• Upwork Job ID: 1664302246693322752
• Last Price Increase: 2023-06-01

[melvin-bot]

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[kavimuru]

Proposal by @dukenv0307

Proposal

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

Insert emoji before a colon text after is replaced by "1" string

What is the root cause of that problem?

commentAfterColonWithEmojiNameRemoved is replaced by CONST.SPACE if the text after the cursor starting with a colon and ending with a space or newline character.

App/src/pages/home/report/ReportActionCompose.js

Line 594 in 811c0f3

const commentAfterColonWithEmojiNameRemoved = this.state.value.slice(this.state.selection.end).replace(CONST.REGEX.EMOJI_REPLACER, CONST.SPACE);

But actually CONST.SPACE is "1" instead of space.

App/src/CONST.js

Line 1255 in 811c0f3

SPACE: 1,

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

We shoud change SPACE constant in here to space.

App/src/CONST.js

Line 1255 in 811c0f3

SPACE: 1,

What alternative solutions did you explore? (Optional)

We could remove replace function to remain the text after selection to same as Slack now.

[hungvu193]

Proposal

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

Insert emoji before a colon text after is replaced by "1" string

What is the root cause of that problem?

We're using logic in here to calculate the logic when inserting an emoji:

App/src/pages/home/report/ReportActionCompose.js

Lines 591 to 594 in 811c0f3

	const commentBeforeColon = this.state.value.slice(0, this.state.colonIndex);
	const emojiObject = this.state.suggestedEmojis[highlightedEmojiIndex];
	const emojiCode = emojiObject.types && emojiObject.types[this.props.preferredSkinTone] ? emojiObject.types[this.props.preferredSkinTone] : emojiObject.code;
	const commentAfterColonWithEmojiNameRemoved = this.state.value.slice(this.state.selection.end).replace(CONST.REGEX.EMOJI_REPLACER, CONST.SPACE);

At line 594, we're using EMOJI_REPLACERregex to remove all the matched words with CONST.SPACE which equal 1.

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

Here's the evidence how slack handle the emoji in this case:

Screen.Recording.2023-05-21.at.23.28.24.mov

As we can see, slack doesn't remove the text after the selected emoji, and I think we can also do it in our case, instead of remove entire string, we should only remove unnecessary character after the selected emoji, in this case, that will be the semi colon, so we need to update the condition of commentAfterColonWithEmojiNameRemoved:

       let commentAfterColonWithEmojiNameRemoved;
        if (this.state.value.slice(this.state.selection.end).match(CONST.REGEX.EMOJI_REPLACER)) {
          // we can remove the semi colon right after the selection end, in case it matched our regex
            commentAfterColonWithEmojiNameRemoved = this.state.value.slice(this.state.selection.end + 1);
        } else {
            commentAfterColonWithEmojiNameRemoved = this.state.value.slice(this.state.selection.end)
        };

What alternative solutions did you explore? (Optional)

Do it like slack, we can also keep the semi colon:

commentAfterColonWithEmojiNameRemoved = this.state.value.slice(this.state.selection.end)

Result:

Screen.Recording.2023-05-21.at.23.36.40.mov

[melvin-bot]

@muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!

[muttmuure]

Out of office this week, picking it up when I'm back.

[melvin-bot]

@muttmuure Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[dukenv0307]

I can't update the above proposal so I create a new comment to make clear my alternative solution

What alternative solutions did you explore? (Optional)
We could remove replace function to remain the text after selection to same as Slack now.

This is the current behavior
Expensify:

Screen.Recording.2023-05-27.at.22.42.36.mov

Slack:

Screen.Recording.2023-05-27.at.22.43.27.mov

The difference is when inserting an emoji before semicolon, Slack will remain semicolon with the text after that

If we come up with this approach, We need to remove replace function here

App/src/pages/home/report/ReportActionCompose.js

Line 625 in 12d2f3c

const commentAfterColonWithEmojiNameRemoved = this.state.value.slice(this.state.selection.end).replace(CONST.REGEX.EMOJI_REPLACER, CONST.SPACE);

[melvin-bot]

@muttmuure Still overdue 6 days?! Let's take care of this!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f4ab1937aacf2d66

[muttmuure]

Reproduced

[melvin-bot]

Current assignee @muttmuure is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

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

[s77rt]

@dukenv0307 Thanks for the proposal. Your RCA is correct. The alternative solution looks good to me i.e not to replace the text after the colon. It seems to work fine but not sure if it may break something else so I asked here #14686 (comment). In case this didn't work we will go with the first solution.

🎀 👀 🎀 C+ reviewed

cc @chiragsalian

[s77rt]

@hungvu193 Thanks for the proposal but I think it's the same as @dukenv0307's proposal.

[melvin-bot]

@chiragsalian @s77rt @muttmuure this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

📣 @dukenv0307 You have been assigned to this job by @chiragsalian!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[dukenv0307]

@s77rt The PR is ready for review. But we still need to wait #14686 (comment) is resolved

[melvin-bot]

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

[melvin-bot]

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

• Fix: remove replace function when inserting emoji #20149

If no regressions arise, payment will be issued on 2023-06-13. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] 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:
• [@s77rt] 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:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] 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.
• [@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: Static emoji autosuggestion  #14686
• The offending PR has been commented on: Static emoji autosuggestion  #14686 (comment)
• A discussion in #expensify-bugs has been started: I'm pretty sure we have nothing to discuss it was an honest mistake where the SPACE const was 1 instead of .
• Determine if we should create a regression test for this bug: No

[muttmuure]

I've invited @s77rt to apply but my message had some old text in it about bug reporting by mistake. Sorry for the confusion!

[s77rt]

@muttmuure No problem! I have accepted the offer. By the way @dukenv0307 should also be invited for both contributor role and bug reporter

[muttmuure]

@dukenv0307 not sure what name you go by in Upwork, please could you apply for the job? https://www.upwork.com/ab/applicants/1664302246693322752/job-details

[muttmuure]

@s77rt paid

[dukenv0307]

@muttmuure I've applied, my Upwork profile: https://www.upwork.com/freelancers/~01f5cbe690701118a2. Thanks.

[muttmuure]

We have a support ticket open with Upwork

[muttmuure]

@dukenv0307 invited

[muttmuure]

Everyone has been paid

@dukenv0307 - $1750 for reporting and 3 day merge
@s77rt - $1500 for 3 day merge