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

Fix/28946 Mentioning an email with double @ sign results in an invalid email mention #615

Merged

Conversation

Victor-Nyagudi
Copy link
Contributor

@Victor-Nyagudi Victor-Nyagudi commented Dec 12, 2023

Removed the + quantifier from the userMentions regex because this resulted in all the @ signs being matched when a mention had multiple preceding @ signs.

For example, the match looked like this @@user@expensify.com, and clicking it showed a user with @user@expensify.com as their email in the RHP. Trying to message this user then resulted in an error because their email address was invalid.

With this change, the match now looks like this: @@user@expensify.com, which, upon clicking/tapping, shows the valid email user@expensify.com in the RHP, so the user can now be messaged without errors.

Please Note: I've altered the linking below slightly in accordance to this comment because doing it this way assigns the correct people to review.

Fixed Issues

$ Expensify/App#28946
$ Expensify/App#28946 (comment)

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?

The 'Test for user mention with @@username@domain.com' test in ExpensiMark-HTML-test.js.

test('Test for user mention with @@username@domain.com', () => {
    const testString = '@@username@expensify.com';
    const resultString = '@<mention-user>@username@expensify.com</mention-user>';
    expect(parser.replace(testString)).toBe(resultString);
});
  1. What tests did you perform that validates your changed worked?

Running npm run test resulted in all tests passing.

In addition, typing '@@user@expensify.com' and then sending resulted in the correct match: @@user@expensify.com.

QA

  1. What does QA need to do to validate your changes?
  • Log into an account and open a report
  • Type '@' in the composer followed by the valid email of someone you've chatted with before. You can also optionally select one from the mentions suggestions list.
  • Place the cursor at the beginning of the composer, type another '@' sign, and then send the message.
  • Verify that only the first '@' sign before the email and the email is highlighted e.g. @@user@expensify.com.
  1. What areas to they need to test for regressions?

You can repeat the steps above in a workspace chat, group chat, thread chat, and task thread chat (minus the logging into an account step).

This test case is for valid text that should match for user mentions
Copy link

github-actions bot commented Dec 12, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Victor-Nyagudi
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@Victor-Nyagudi Victor-Nyagudi changed the title Fix/28946 invalid email after mention Fix/28946 Mentioning an email with double @ sign results in an invalid email mention Dec 12, 2023
@Victor-Nyagudi
Copy link
Contributor Author

There are a lot of lint errors unrelated to my PR after running npm run grunt watch in ExpensiMark-HTML-test.js.

expensify-errors-after-running-npm-grunt-watch

Some of these are from sections of code that have existed for quite a while, so I'm assuming some errors can be ignored.

There are currently no lint errors present in the code I added.

@Victor-Nyagudi Victor-Nyagudi marked this pull request as ready for review December 12, 2023 09:19
@Victor-Nyagudi Victor-Nyagudi requested a review from a team as a code owner December 12, 2023 09:19
@melvin-bot melvin-bot bot requested review from joelbettner and removed request for a team December 12, 2023 09:19
@joelbettner
Copy link
Contributor

joelbettner commented Dec 12, 2023

Hmm...I don't think I should have been assigned to this PR as I wasn't a part of the issue. Re-assigning.

edit: it appears I'm unable to assign @Santhosh-Sellavel to this.

@joelbettner joelbettner requested review from amyevans and removed request for joelbettner December 12, 2023 20:29
Copy link
Contributor

@amyevans amyevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'll let @Santhosh-Sellavel review too

@amyevans
Copy link
Contributor

Please Note: I've altered the linking below slightly in accordance to this #607 (comment) because doing it this way assigns the correct people to review.

I think the automation just isn't set up for this repo. Either way no biggie

Copy link
Contributor

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@amyevans amyevans merged commit 398bf7c into Expensify:main Dec 13, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants