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

Payment due - 30 May [$250] Typing @useraccount and sending it sends `@useraccount@domain.com #40205

Closed
1 of 6 tasks
m-natarajan opened this issue Apr 13, 2024 · 33 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@m-natarajan
Copy link

m-natarajan commented Apr 13, 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.62-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
Expensify/Expensify Issue URL:
Issue reported by: @cead22
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712874830762349

Action Performed:

  1. Open any chat
  2. Type @applausetester and hit send (Make sure to use "`" to make it a codeblock)

Expected Result:

Appears @applausetester after sending

Actual Result:

Appears as @applausetester@applause.expensifail.com

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

Recording.6.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0115db0ab43f78ece5
  • Upwork Job ID: 1779954988168179712
  • Last Price Increase: 2024-04-22
  • Automatic offers:
    • shubham1206agra | Reviewer | 0
    • tienifr | Contributor | 0
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 13, 2024
Copy link

melvin-bot bot commented Apr 13, 2024

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added the Overdue label Apr 15, 2024
@MitchExpensify
Copy link
Contributor

Reproduced

@MitchExpensify
Copy link
Contributor

Assigning to VSB as low as this is a chat-centric bug that reveals emails

@MitchExpensify MitchExpensify added the External Added to denote the issue can be worked on by a contributor label Apr 15, 2024
@melvin-bot melvin-bot bot changed the title Typing @useraccount and sending it sends `@useraccount@domain.com [$250] Typing @useraccount and sending it sends `@useraccount@domain.com Apr 15, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0115db0ab43f78ece5

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 15, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

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

@sufyan-siddiqui
Copy link

I'm not able to reproduce. Any help would be appreciated.

chrome_TMhEpz03IN.mp4

@shubham1206agra
Copy link
Contributor

@cead22 Can you provide us with a better example, as domains are not the same for everybody?

@kaushiktd
Copy link
Contributor

@cead22 i also can't reproduce it.

image

@tienifr
Copy link
Contributor

tienifr commented Apr 17, 2024

Proposal

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

When we send short hand mention like @ApplauseTester inside backticks, it's appending the private domain into it

Eg. Sending

`@applausetester`

It will appears as @applausetester@applause.expensifail.com

What is the root cause of that problem?

In here, we have a logic where if the current user's private domain is expensify.com, it we tag by short hand, like @tienifr, and there's a tienifr@expensify.com in the personal details list, we'll convert the @tienifr to @tienifr@expensify.com so that it will be treated as a mention.

This is to make users in the same organization able to tag each other by just @shubham, @tienifr, not having to type the full email domain.

However, when we're trying to detect short mention here, we're not taking into account cases like if the mention is in code blocks, code fence, links, ... So even the short-hand mention inside codeblock is still appended with the private domain. And subsequently it will not be considered in the mention detection logic in expensify-common, thus leading to this issue.

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

If we look at the proper logic to detect mentions in expensify-common here and here, we'll see a mention is only valid if:

We should apply the same when detecting short mentions, basically the only difference between short mention detection and full mention detection is: short mention doesn't require an email domain, the rest should be the same.

We'll need to:

  • Modify the Regex for short mentions (similar to for full mention, but do not that for short mention we need to check markdown signs instead of html elements).

We can update this to

SHORT_MENTION: new RegExp(`@[\\w\\-\\+\\'#@]+(?:\\.[\\w\\-\\'\\+]+)*(?![^\`]*\`)`, 'gim'),

So it will not match if the mention is inside backticks ((?![^\`]*\`) added)

  • Add logic to check isValidMention similar to here

We'll add here

if (!Str.isValidMention(match)) {
    return match;
}

So if the matched mention is invalid, we'll not replace anything.

What alternative solutions did you explore? (Optional)

Same solution as above, but we can consider centralize it in expensify-common to be DRY.

@tienifr
Copy link
Contributor

tienifr commented Apr 17, 2024

@cead22 Can you provide us with a better example, as domains are not the same for everybody?

@shubham1206agra To reproduce this, we need:

  • A private domain (if you don't have one, can comment out 'gmail.com', in node_modules/expensify-common/lib/CONST.jsx so that any gmail.com will become a private domain)
  • The short mention we're mentioning should be inside personalDetailsList already

@cead22
Copy link
Contributor

cead22 commented Apr 17, 2024

So even the short-hand mention inside codeblock is still appended with the private domain. And subsequently it will not be considered in the mention detection logic in expensify-common

@tienifr to clarify, it should not be considered a mention if it's inside backticks, right?

We'll need to:

  • Modify the Regex for short mentions (similar to for full mention, but do not that for short mention we need to check markdown signs instead of html elements)

Where/how?

  • Add logic to check isValidMention similar to here

Where/how?

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

This looks incomplete

@shubham1206agra
Copy link
Contributor

@tienifr Can you comment on the above?

@tienifr
Copy link
Contributor

tienifr commented Apr 19, 2024

@cead22 @shubham1206agra

@tienifr to clarify, it should not be considered a mention if it's inside backticks, right?

Yes, same as when we put full email mention in the backticks

This looks incomplete

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

For the rest, I will add some details within a day.

@melvin-bot melvin-bot bot added the Overdue label Apr 22, 2024
@tienifr
Copy link
Contributor

tienifr commented Apr 22, 2024

I'm back from weekend, will take a loot at the above soon.

Copy link

melvin-bot bot commented Apr 22, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Apr 22, 2024

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

@MitchExpensify
Copy link
Contributor

Not overdue, @tienifr is going to have a look soon

@melvin-bot melvin-bot bot removed the Overdue label Apr 22, 2024
Copy link

melvin-bot bot commented Apr 27, 2024

@MitchExpensify @shubham1206agra @laurenreidexpensify 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!

@laurenreidexpensify
Copy link
Contributor

@shubham1206agra bump for review thanks

@shubham1206agra
Copy link
Contributor

Lets go with @tienifr's proposal.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 28, 2024

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

@robertjchen
Copy link
Contributor

👍 to updated proposal

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 29, 2024

📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Apr 29, 2024

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Apr 30, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label May 23, 2024
Copy link

melvin-bot bot commented May 23, 2024

This issue has not been updated in over 15 days. @robertjchen, @MitchExpensify, @shubham1206agra, @laurenreidexpensify, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label May 23, 2024
@laurenreidexpensify laurenreidexpensify added Daily KSv2 and removed Monthly KSv2 labels May 28, 2024
@laurenreidexpensify laurenreidexpensify changed the title [$250] Typing @useraccount and sending it sends `@useraccount@domain.com Payment due - 30 May [$250] Typing @useraccount and sending it sends `@useraccount@domain.com May 28, 2024
@laurenreidexpensify laurenreidexpensify removed their assignment May 28, 2024
@laurenreidexpensify
Copy link
Contributor

@MitchExpensify looks like the payment job on this failed but it's on prod now so should be good to pay on Thurs. Handing back over to you!

@MitchExpensify
Copy link
Contributor

Reminder set to pay tomorrow!

@MitchExpensify
Copy link
Contributor

Payment summary:

@shubham1206agra
Copy link
Contributor

@MitchExpensify Accepted

@MitchExpensify
Copy link
Contributor

Paid and contract ended!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
No open projects
Development

No branches or pull requests

9 participants