Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.79k forks source link

[$250] Task – `.com` appears after assignee avatar when create a task manualy #49454

Open IuliiaHerets opened 1 week ago

IuliiaHerets commented 1 week ago

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: v9.0.38-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: https://expensify.testrail.io/index.php?/tests/view/4983838 Email or phone of affected tester (no customers): ponikarchuks+118924@gmail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in as a Gmail account
  3. Open any chat
  4. Enter [] @ and select Expensify user, wright task title and send Example: [] @applausetester+jpcategory_2@applause.expensifail.com New task

Expected Result:

.com not appears after assignee avatar

Actual Result:

.com appears after assignee avatar

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/cde0eeb8-e402-499c-8ba9-76b577211b18

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836890784466753823
  • Upwork Job ID: 1836890784466753823
  • Last Price Increase: 2024-09-19
Issue OwnerCurrent Issue Owner: @Ollyws
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @mallenexpensify (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.

IuliiaHerets commented 1 week ago

@mallenexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

CyberAndrii commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-09-19 12:17:38 UTC.

Proposal

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

Task – .com appears after assignee avatar when create a task manualy

What is the root cause of that problem?

This regex does not handle subdomains in emails correctly

https://github.com/Expensify/App/blob/37bf6b2fef128786dfcd83257a56f56c0cd1eb3d/src/pages/home/report/ReportFooter.tsx#L126-L132

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

Replace it with this regex:

-const taskRegex = /^\[\]\s+(?:@([^\s@]+(?:@\w+\.\w+)?))?\s*([\s\S]*)/;
+const taskRegex = /^\[\]\s+@([^\s]+)\s*([\s\S]*)/;

Result

https://github.com/user-attachments/assets/e290328a-967d-4b1b-a8e3-8bae0b0305d6

ChavdaSachin commented 1 week ago

Proposal

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

What is the root cause of that problem?

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

bernhardoj commented 1 week ago

Proposal

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

.com appears as the task title when create task from comment.

What is the root cause of that problem?

This happens because the logic can't handle multiple level of domain of an email, for example, @applause.expensifail.com. https://github.com/Expensify/App/blob/8d645bfc76bfa1676e0b7730bf6019b4c4161442/src/pages/home/report/ReportFooter.tsx#L132

The regex above, specifically (?:@\w+\.\w+) only allows (xx.xx) pattern of the domain. This happens after https://github.com/Expensify/App/pull/39475 to allow create task with short domain by making the domain optional, but somehow also changes the rule of the domain which is not needed which also allows the user to mention an email with the last domain with only 1 character, for example, @example.c. If we use the prev regex, it requires at least 2 character, @example.co.

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

Let's use the old regex and make the domain optional.

const taskRegex = /^\[\]\s+(?:@([^\s@]+(?:@[\w.-]+\.[a-zA-Z]{2,})?))?\s*([\s\S]*)/;

I think we can remove - from the domain regex too.

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

mallenexpensify commented 1 week ago

Unable to repro on staging, Desktop. @Ollyws can you attempt reproduction plz? Some users have default avatars and some have custom)

image
MelvinBot commented 1 week ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

CyberAndrii commented 1 week ago

@mallenexpensify you need to also enter the task title after an email. For example New task

Screenshot 2024-09-20 at 01 42 08
Ollyws commented 1 week ago

@CyberAndrii and @ChavdaSachin both your proposals will break adding a task without a defined user ([] @ taskname should create a task)

@bernhardoj's proposal LGTM.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

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

iwiznia commented 1 week ago

Is the regex supposed to only match [] user@email.com and [] @someuser types of texts? Asking because it also matches things like [] some@thing which I think is invalid and would break things?

bernhardoj commented 1 week ago

The email regex from MDN matches some@thing. Maybe we can use that regex and make the domain optional.

^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+(@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*)?$
// simplified
^[\w.!#$%&'*+/=?^`{|}~-]+(@[a-zA-Z\d](?:[a-zA-Z\d-]{0,61}[a-zA-Z\d])?(?:\.[a-zA-Z\d](?:[a-zA-Z\d-]{0,61}[a-zA-Z\d])?)*)?$
melvin-bot[bot] commented 1 week ago

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

iwiznia commented 6 days ago

Maybe we can use that regex and make the domain optional.

That sounds good to me

bernhardoj commented 5 days ago

PR is ready

cc: @Ollyws