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

[No QA] Add rule to recognize user @here mentions #532

Merged
merged 4 commits into from
May 9, 2023

Conversation

getusha
Copy link
Contributor

@getusha getusha commented May 9, 2023

@puneetlath @mananjadhav

Fixed Issues

$ Expensify/App#17882

Tests

  1. Open any chat
  2. Send valid mention e.g. @john, @david
  3. Send @here mention
    The user mention should be replaced as mention and the @here should be @here

QA

Same as tests

@getusha getusha requested a review from a team as a code owner May 9, 2023 17:07
@melvin-bot melvin-bot bot requested review from cristipaval and removed request for a team May 9, 2023 17:07
Copy link
Contributor

@puneetlath puneetlath left a comment

Choose a reason for hiding this comment

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

Approving since the changes are the same as in #524 which I previously approved.

Copy link
Contributor

@puneetlath puneetlath left a comment

Choose a reason for hiding this comment

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

Approving since the changes are the same as in #524 which I previously approved.

@puneetlath puneetlath changed the title Add rule to recognize user @here mentions [No QA] Add rule to recognize user @here mentions May 9, 2023
@mananjadhav
Copy link
Contributor

Verified the code changes, compared to the old PR.

@puneetlath
Copy link
Contributor

Adding No QA since the QA will happen in the PR to incorporate the new expensify-common version in App.

@puneetlath puneetlath merged commit d9c8b08 into Expensify:main May 9, 2023
@puneetlath
Copy link
Contributor

PR for updating the version is here: Expensify/App#18655

@puneetlath
Copy link
Contributor

Thanks @mananjadhav would you be able to do the C+ checklist for the App PR. Since it wasn't needed for this one.


// We don't support mention inside code or codefence styling,
// for example using `@[email protected]` or ```@[email protected]``` will be invalid.
const containsInvalidTag = /(<code>|<pre>|&#x60;)/g.test(mention);
Copy link
Member

Choose a reason for hiding this comment

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

beep boop 🤖 🐧

dropping a friendly note that this PR didn't implement mention inside codeblocks correctly, causing the bug - Expensify/App#18983

If the mention is not at the start of the string it won't be included into the regex match, causing the bug.

*/
{
name: 'userMentions',
regex: new RegExp(`((&#x60;|<code>|<pre>)\\s*?)?[\`.a-zA-Z]?@+${CONST.REG_EXP.EMAIL_PART}`, 'gm'),
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 Dropping a note about this causing a regression in Expensify/App#18857. Essentially just missed a case where mention is inside of a link. More details are here

*/
{
name: 'hereMentions',
regex: /((&#x60;|<code>|<pre>)\s*?)?[`.a-zA-Z]?@here\b/gm,
Copy link
Contributor

Choose a reason for hiding this comment

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

This regex caused the bug here because a word boundary \b does not include _.

* @param {String} mention
* @returns {bool}
*/
isValidMention(mention) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Dropping a note that this has caused Expensify/App#20085
Missed an edge case where [email protected]@gmail.com was rendered as mention
image

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.

6 participants