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

[HOLD for payment 2024-10-08][$250] Task – .com appears after assignee avatar when create a task manualy #49454

Closed
3 of 6 tasks
IuliiaHerets opened this issue Sep 19, 2024 · 27 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Needs Reproduction Reproducible steps needed

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 19, 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: 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:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6608501_1726727831321.Task_assignee_com.mp4

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
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

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
Copy link
Author

@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
Copy link
Contributor

CyberAndrii commented Sep 19, 2024

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

/**
* Matching task rule by group
* Group 1: Start task rule with []
* Group 2: Optional email group between \s+....\s* start rule with @+valid email or short mention
* Group 3: Title is remaining characters
*/
const taskRegex = /^\[\]\s+(?:@([^\s@]+(?:@\w+\.\w+)?))?\s*([\s\S]*)/;

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

49454.mov

@ChavdaSachin
Copy link
Contributor

Proposal

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

  • When task is created from composer and assignee's email contains 2 or more (.) in domain name - part for domain name after second (.) moves into task title.

What is the root cause of that problem?

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

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

and

            const title = match[3] ? match[3].trim().replace(/\n/g, ' ') : undefined;

const title = match[2] ? match[2].trim().replace(/\n/g, ' ') : undefined;

What alternative solutions did you explore? (Optional)

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
Copy link
Contributor

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.

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

The regex above, specifically (?:@\w+\.\w+) only allows (xx.xx) pattern of the domain. This happens after #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.

@mallenexpensify mallenexpensify added the External Added to denote the issue can be worked on by a contributor label Sep 19, 2024
@melvin-bot melvin-bot bot changed the title Task – .com appears after assignee avatar when create a task manualy [$250] Task – .com appears after assignee avatar when create a task manualy Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

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

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

melvin-bot bot commented Sep 19, 2024

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

@mallenexpensify mallenexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 19, 2024
@mallenexpensify
Copy link
Contributor

mallenexpensify commented Sep 19, 2024

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

@mallenexpensify mallenexpensify added the Needs Reproduction Reproducible steps needed label Sep 19, 2024
@MelvinBot
Copy link

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

@CyberAndrii
Copy link
Contributor

@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
Copy link
Contributor

Ollyws commented Sep 20, 2024

@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

Copy link

melvin-bot bot commented Sep 20, 2024

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

@iwiznia
Copy link
Contributor

iwiznia commented Sep 20, 2024

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
Copy link
Contributor

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])?)*)?$

Copy link

melvin-bot bot commented Sep 23, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
@iwiznia
Copy link
Contributor

iwiznia commented Sep 24, 2024

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

That sounds good to me

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 25, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @Ollyws

@mallenexpensify mallenexpensify changed the title [$250] Task – .com appears after assignee avatar when create a task manualy [HOLD for payment 2024-10-08][$250] Task – .com appears after assignee avatar when create a task manualy Oct 7, 2024
@mallenexpensify mallenexpensify added Daily KSv2 Waiting for copy User facing verbiage needs polishing and removed Reviewing Has a PR in review Weekly KSv2 labels Oct 7, 2024
Copy link

melvin-bot bot commented Oct 7, 2024

Triggered auto assignment to @NickTooker (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

@mallenexpensify mallenexpensify added Awaiting Payment Auto-added when associated PR is deployed to production and removed Waiting for copy User facing verbiage needs polishing labels Oct 7, 2024
@mallenexpensify
Copy link
Contributor

Sorry @NickTooker , wrong label :ohnothing:

Copy link

melvin-bot bot commented Oct 8, 2024

Payment Summary

Upwork Job

BugZero Checklist (@mallenexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1836890784466753823/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@mallenexpensify
Copy link
Contributor

Contributor: @bernhardoj due $250 via NewDot
Contributor+: @Ollyws due $250 via NewDot

@Ollyws plz complete the BZ checklist.

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:

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

@bernhardoj
Copy link
Contributor

Requested in ND.

@Ollyws
Copy link
Contributor

Ollyws commented Oct 9, 2024

BugZero Checklist:

  • The PR that introduced the bug has been identified. Link to the PR:

#39475

  • 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:

https://github.com/Expensify/App/pull/39475/files#r1792852417

  • 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:

N/A

  • Determine if we should create a regression test for this bug.

Yes.

  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. Open any chat
2. Enter a task markdown with an assignee with a multi-level domain, for example: [] @example@example.co.id test
3. Verify the task title shown is test, not .id test

Do we agree 👍 or 👎

@Ollyws
Copy link
Contributor

Ollyws commented Oct 9, 2024

Requested in ND.

@garrettmknight
Copy link
Contributor

$250 approved for @Ollyws

@mallenexpensify
Copy link
Contributor

Test case created

Thanks all!

@JmillsExpensify
Copy link

$250 approved for @bernhardoj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Needs Reproduction Reproducible steps needed
Projects
None yet
Development

No branches or pull requests