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

[$2000] Web - Inconsistently highlighting occurs when multiple mentions are used without spaces. #21753

Closed
1 of 6 tasks
kavimuru opened this issue Jun 27, 2023 · 118 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Jun 27, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Go to any chat.
  2. Type "@ here" multiple times without including spaces.
  3. Observe that all the mentions are highlighted.
  4. Type a user's email ID followed by "@ here" without a space.
  5. Note that the email address is highlighted, but "@ here" is not.

Expected Result:

Consistency is expected, where both cases should either be highlighted or not.

Actual Result:

Multiple use of "@ here" are highlighted, but when using mentions with a user email followed by "@ here," it does not get highlighted.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.33-2
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
Notes/Photos/Videos: Any additional supporting documentation

Recording.920.mp4
Bug.1.mp4

Expensify/Expensify Issue URL:
Issue reported by: @usmantariq96
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687759061501769

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01647f661251c3871e
  • Upwork Job ID: 1674785431378788352
  • Last Price Increase: 2023-07-25
Issue OwnerCurrent Issue Owner: @puneetlath
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

Triggered auto assignment to @flaviadefaria (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@Shivam7-1
Copy link

Shivam7-1 commented Jun 28, 2023

Proposal

Inconsistently highlighting occurs when multiple mentions are used without spaces.

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

What is the root cause of that problem?
The issue occurs in NewChatPage.js File In this The error in the code lies in the getSections method.
In the if condition that checks for hasUnselectedUserToInvite, the check is incorrect. It currently checks if this.state.userToInvite exists and if it is not included in the filterText. However, the filterText is constructed by concatenating the login property of selected options, so it will not include the email part of the userToInvite. T
his causes the userToInvite option to be consistently highlighted, even if it is selected.

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

In Solution of this issue The code can modify as follows:
To fix this issue, We need to modify the if condition in the getSections method as follows:

By checking filterText.includes(this.state.userToInvite.email) instead of filterText.includes(this.state.userToInvite.login), we need to ensure that the email part of userToInvite is included in the check.

This change will ensure that both cases (mentions without spaces and mentions with user email) are consistently highlighted or not highlighted based on their selection status

@flaviadefaria flaviadefaria added the External Added to denote the issue can be worked on by a contributor label Jun 30, 2023
@melvin-bot melvin-bot bot changed the title Web - Inconsistently highlighting occurs when multiple mentions are used without spaces. [$1000] Web - Inconsistently highlighting occurs when multiple mentions are used without spaces. Jun 30, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 30, 2023

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

@melvin-bot melvin-bot bot added Overdue Help Wanted Apply this label when an issue is open to proposals by contributors labels Jun 30, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 30, 2023

Current assignee @flaviadefaria is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jun 30, 2023

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

@flaviadefaria
Copy link
Contributor

Added the external label.

@melvin-bot melvin-bot bot removed the Overdue label Jun 30, 2023
@Shivam7-1
Copy link

Proposal

Inconsistently highlighting occurs when multiple mentions are used without spaces.

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

What is the root cause of that problem?
The issue occurs in NewChatPage.js File In this The error in the code lies in the getSections method.
In the if condition that checks for hasUnselectedUserToInvite, the check is incorrect. It currently checks if this.state.userToInvite exists and if it is not included in the filterText. However, the filterText is constructed by concatenating the login property of selected options, so it will not include the email part of the userToInvite. T
his causes the userToInvite option to be consistently highlighted, even if it is selected.

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

In Solution of this issue The code can modify as follows:
To fix this issue, We need to modify the if condition in the getSections method as follows:

By checking filterText.includes(this.state.userToInvite.email) instead of filterText.includes(this.state.userToInvite.login), we need to ensure that the email part of userToInvite is included in the check.

This change will ensure that both cases (mentions without spaces and mentions with user email) are consistently highlighted or not highlighted based on their selection status

@melvin-bot melvin-bot bot added the Overdue label Jul 3, 2023
@flaviadefaria
Copy link
Contributor

@aimane-chnaif thoughts on the proposal above?

@melvin-bot melvin-bot bot removed the Overdue label Jul 3, 2023
@payal-lathidadiya
Copy link
Contributor

payal-lathidadiya commented Jul 4, 2023

Proposal

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

Inconsistently highlighting occurs when multiple mentions are used without spaces

What is the root cause of that problem?

When @here mention is added along side user mention, ExpensiMark parser doesn't parse @here mention correctly. The reason is below regular expression for here-mention rule code here

/([a-zA-Z0-9.!$%&+/=?^`{|}_-]?)(@here)([a-zA-Z0-9.!$%&+/=?^`{|}_-]?)(?=\b)(?!(@[a-zA-Z0-9-]+?(\.[a-zA-Z]+)+)|((?:(?!<a).)+)?<\/a>|[^<]*(<\/pre>|<\/code>))/gm

This regex doesn't extract correct match @here from regex result test link to replace with our user-mention markdown. it also includes last character from previous word after match.

However, with @here@here comment it extracts @here match correctly test link

Hence, this caused inconsistency in highlight of here mention with multiple mentions.

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

To fix here mention identification when mentioned with other user mention, we can change regular expression with below code here

/([.!$%&+/=?^`{|}_-]?)(@here)([a-zA-Z0-9.!$%&+/=?^`{|}_-]?)(?=\b)(?!(@[a-zA-Z0-9-]+?(\.[a-zA-Z]+)+)|((?:(?!<a).)+)?<\/a>|[^<]*(<\/pre>|<\/code>))/gm

Test link with updated regex

What alternative solutions did you explore? (Optional)

Since, we only consider user mention as valid if it's proceeded by a space. we should apply the same rule for here mention as well to have consistency.

with that @here@here should not be considered as a valid here mention

To apply this logic, we can change hereMentions regular expression as below;

([a-zA-Z0-9.!$%&+=?^`{|}_-]?)(@here)+([a-zA-Z0-9.!$%&+=?^`{|}_-]?)(?=\b)(?!(@[a-zA-Z0-9-]+?(\.[a-zA-Z]+)+)|((?:(?!<a).)+)?<\/a>|[^<]*(<\/pre>|<\/code>))

Test link with this new regex

@melvin-bot melvin-bot bot added the Overdue label Jul 5, 2023
@flaviadefaria
Copy link
Contributor

@aimane-chnaif thoughts on the proposals above?

@melvin-bot melvin-bot bot removed the Overdue label Jul 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 7, 2023

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

@aimane-chnaif
Copy link
Contributor

@Shivam7-1 The root cause is not correct

@Shivam7-1
Copy link

@Shivam7-1 The root cause is not correct

Okay I will check again and make changes in proposal accordingly

@ShogunFire
Copy link
Contributor

ShogunFire commented Jul 9, 2023

Hello @aimane-chnaif What about @payal-lathidadiya proposal ?
I tested it and it looks like it is working

@melvin-bot melvin-bot bot added the Overdue label Jul 9, 2023
@aimane-chnaif
Copy link
Contributor

@payal-lathidadiya can you please explain in detail about your solution (regex)?

@puneetlath
Copy link
Contributor

PR review is ongoing.

@melvin-bot melvin-bot bot removed the Overdue label Nov 15, 2023
@puneetlath
Copy link
Contributor

@agilejune I merged the expensify-common PR. Please open a PR in Expensify/App to update the expensify-common version. Thanks!

agilejune added a commit to agilejune/App that referenced this issue Nov 20, 2023
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 20, 2023
@agilejune
Copy link

@puneetlath @aimane-chnaif
The App PR #31541 is ready

@aimane-chnaif
Copy link
Contributor

Based on #31541 (comment), the fix already deployed to production on Nov 23 in this PR

@puneetlath
Copy link
Contributor

Ok so is there anything left to do here? Or it sounds like we're ready to pay out?

@aimane-chnaif
Copy link
Contributor

Ok so is there anything left to do here? Or it sounds like we're ready to pay out?

yes ready

This was not a regression. As an edge case, the bug always existed after we implement here mentions.
I think automated tests should cover regression test.

@puneetlath
Copy link
Contributor

kk. @aimane-chnaif I sent you an offer.

@agilejune can you please apply here? https://www.upwork.com/jobs/~01f57f05c12d9a825a

@puneetlath
Copy link
Contributor

@agilejune bump. Can you please apply to the Upwork job so that we can close this out? Actually, also @usmantariq96. https://www.upwork.com/jobs/~01f57f05c12d9a825a

@aimane-chnaif has been paid

@usmantariq96
Copy link

usmantariq96 commented Dec 4, 2023

@puneetlath applied

Thanks

@agilejune
Copy link

@puneetlath
I've applied to the upwork job.

@puneetlath
Copy link
Contributor

@agilejune to this one? https://www.upwork.com/jobs/~01f57f05c12d9a825a

I'm not seeing your application.

@agilejune
Copy link

@puneetlath
image

can you check this proposal ?

@agilejune
Copy link

@agilejune to this one? https://www.upwork.com/jobs/~01f57f05c12d9a825a

I'm not seeing your application.

Yes, I applied there

@puneetlath
Copy link
Contributor

Super weird. I don't see it.

image

Can you message me your upwork profile either here or in Slack and I'll try inviting you directly.

Also @usmantariq96 friendly bump to apply.

@agilejune
Copy link

@agilejune
Copy link

I thought I saved my details to contribute here.
But let me try again

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~017f0481af079ea44f

Copy link

melvin-bot bot commented Dec 5, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@usmantariq96
Copy link

usmantariq96 commented Dec 6, 2023

@puneetlath link is not opening at my end.

Can you please send me direct message please. Thanks

@puneetlath applied thanks

@puneetlath
Copy link
Contributor

@usmantariq96
Copy link

@puneetlath offer accepted

Thanks

@puneetlath
Copy link
Contributor

Ok only @agilejune left to pay once they accept.

@agilejune
Copy link

@puneetlath
Thanks for your offer,
I've accepted

@puneetlath
Copy link
Contributor

All paid. Thanks everyone!

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. Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests