[HOLD for payment 2023-03-22] [$1000] Emails inside link and normal text behave differently for specific characters

[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 a chat
2. Send a specific text message with email address ‘*abdul@gmail.com’ or ‘~test@gmail.com’ or
3. Send a same inside a link like ’maillink

Expected Result:

Both should behave similar way and ignore *~ characters.

Actual Result:

The specific characters not considered in normal text while inside link its considered as email.

Workaround:

unknown

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.2.78-0
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:

Screen.Recording.2023-03-02.at.2.56.14.AM.mov

Recording.1615.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b3b7bbe257a58a4e
• Upwork Job ID: 1631691231022665728
• Last Price Increase: 2023-03-03

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[garrettmknight]

@abdulrahuman5196 Looks like you had a proposal in thread, want to put that here?

[abdulrahuman5196]

@garrettmknight Thank you tagging

Proposal

I was the reporter of this bug and proposed a solution as part of the bug report here - https://expensify.slack.com/archives/C049HHMV9SM/p1677859222866439?thread_ts=1677706040.537379&cid=C049HHMV9SM

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

Emails inside link and normal text are not behaving same way on click/tooltip/email address link formation when emails have specific character.

What is the root cause of that problem?

We are using different regex to validate email in 'email' and 'autoEmail' rule in ExpensiMark.js
autoEmail rule for 'abdul@gmail.com' - https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L100
email rule for [text]-(abdul@gmail.com) - https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L60 (Just adding hypon as separator as github is not showing exact string)

AutoEmail rule has the latest regex change made sometime back but the email rule has outdated regex.

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

This is a straight-forward fix.

We should make the email and autoEmail rule consistent.
https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L100
https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L60

As autoEmail has the latest regex we should propagate it to email rule as well. We should create a common place for email regex and use it for email and autoEmail so that this issue doesn't occur in future.

We should form the new 'email' rule regex by just having to append the existing part of regex to find '[text]' link to exiting autoEmail rule regex link(which we can move to common place), so that autoEmail and email rules behaves consistently.

Optional: We can also update '[text]' regex to ignore specific characters but it might be out of scope of this issue.

What alternative solutions did you explore? (Optional)

None

[cristipaval]

I don't think it's overdue for now. @Santhosh-Sellavel are we still waiting for other proposals?

[Santhosh-Sellavel]

Missed this one, reviewing soon!

[Santhosh-Sellavel]

Thanks for the proposal @abdulrahuman5196 what will the new behavior can you list some examples

[abdulrahuman5196]

The expected behaviour is text and 'emailAddress' should recognise valid emailIds in the same way.

But it doesn't do the same way now.

For example

Eg 1: textMessage =

'[link](~test@gmail.com) and ~test@gmail.com' 

Here in the link email segment ~ is considered as valid email and in the second plain email section ~ is not considered valid email character
Eg 1: textMessage =

'[link](*test@gmail.com) and *test@gmail.com'

Here in the link email segment * is considered as valid email and in the second plain email section * is not considered valid email character

This was due to recently changes for valid email regex was done only at https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L100 but the same was not propogated to link email https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L60

After our change in the above example *,~ won't be recognised as valid email character.

In both the example only 'test@gmail.com' will be recognised as valid email Id.

@Santhosh-Sellavel Let me know if I was clear or more information is required.

[Santhosh-Sellavel]

@abdulrahuman5196 Screenshot or recording of input & result would be clear!

[abdulrahuman5196]

Before change:

Untitled.6.mp4

After change:

Untitled.7.mp4

[abdulrahuman5196]

@Santhosh-Sellavel Kindly take a look into the above videos and let me know if we need more information

[Santhosh-Sellavel]

@cristipaval

@abdulrahuman5196 proposal LGTM!

C+ Reviewed
🎀👀🎀

[cristipaval]

Thanks @Santhosh-Sellavel !
@abdulrahuman5196 , happy to see you are going to create a regex in a common place for both usages, to avoid this in the future. Go ahead with the PR 🚀

[Santhosh-Sellavel]

@cristipaval or @garrettmknight assign @abdulrahuman5196 this one, thanks!

[abdulrahuman5196]

Thank you for assigning the issue. @Santhosh-Sellavel @cristipaval

I have created a PR for the fix
Expensify/expensify-common#508

[abdulrahuman5196]

@cristipaval Thank you merging the expensify-common PR.

@Santhosh-Sellavel I have created a related ExpensifyApp PR as well - #15821
Kindly review and let me know if anything else is required from my end

[MelvinBot]

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

[MelvinBot]

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

• Updating expensify-common to bring the fix of maintaining consistency between emails inside link and normal text emails #15821

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

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

[MelvinBot]

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:

• [@Santhosh-Sellavel / @cristipaval] The PR that introduced the bug has been identified. Link to the PR:
• [@Santhosh-Sellavel / @cristipaval] 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:
• [@Santhosh-Sellavel / @cristipaval] 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:
• [@garrettmknight] Determine if we should create a regression test for the bug.
• [@Santhosh-Sellavel] If we decide to create a regression test for the bug, please propose the regression test steps to the appropriate location to ensure the same bug will not reach production again.
• [@garrettmknight] Review the proposed regression test steps and location.
• [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MelvinBot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[garrettmknight]

This was due to recently changes for valid email regex was done only at https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L100 but the same was not propogated to link email https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L60

I'm not sure if this was introduced in a PR or whether it's always been this way inline with the note above.

Determine if we should create a regression test for the bug.

This seems like a pretty niche edge case since the unexpected behavior only happens with special characters at the beginning of the email address so I don't think an RT is necessary.

@Santhosh-Sellavel @cristipaval thoughts?

[abdulrahuman5196]

@garrettmknight

I'm not sure if this was introduced in a PR or whether it's always been this way inline with the note above.
I don't we can point a single PR as cause of this issue as it seems always we had 2 regex declarations which caused the regex to diverge. Correct me if wrong @Santhosh-Sellavel @cristipaval

[cristipaval]

I don't we can point a single PR as cause of this issue as it seems always we had 2 regex declarations which caused the regex to diverge. Correct me if wrong @Santhosh-Sellavel @cristipaval

You're right @abdulrahuman5196. After looking at git history, it seems like there have always been 2 markdowns for emails which didn't reuse the same regex.

[Santhosh-Sellavel]

I agree RT is unnecessary! @garrettmknight

[Santhosh-Sellavel]

@cristipaval We can check off the checklist as we don't need a discussion here!

[Santhosh-Sellavel]

@garrettmknight I believe only payment is due here and the PR qualifies for a speed bonus also, thanks!

[garrettmknight]

Everyone's paid! Closing