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.34k stars 2.77k forks source link

[HOLD for payment 2023-07-17] [$1000] Web - App throws 'No content to add' error if we send empty alias for email #21234

Closed kbecciv closed 1 year ago

kbecciv commented 1 year 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!


Action Performed:

  1. Open the app
  2. Open any report
  3. Type in a link with empty alias eg: [](google.com)
  4. Observe that app handles it well
  5. Send email with empty alias eg: [](test@test.com)
  6. Observe that app throws 'No content to add' for step 5 message

Expected Result:

App should handle empty alias for email in similar way like it does for link

Actual Result:

App throws 'No content to add' error for email with empty alias

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.30.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:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/3efe4b24-22e0-47e5-9758-5bae899a4dac

https://github.com/Expensify/App/assets/93399543/b68bed52-6612-4012-9223-1ffaf656b18d

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686755882619169

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012e72288a82e6fae6
  • Upwork Job ID: 1671920286239043584
  • Last Price Increase: 2023-06-29
eh2077 commented 1 year ago

Proposal

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

App throws 'No content to add' error if we send empty alias for email.

What is the root cause of that problem?

The root cause of this issue is that below markdown

[](test@gmail.com)

is translated into html

<a href="mailto:test@gmail.com"></a>

by applying email rule

{
    name: 'email',
    process: (textToProcess, replacement) => {
        const regex = new RegExp(
            `\\[([^[\\]]*)]\\(${CONST.REG_EXP.MARKDOWN_EMAIL}\\)`, 'gim'
        );
        return this.modifyTextForEmailLinks(regex, textToProcess, replacement);
    },
    replacement: (match, g1, g2) => `<a href="mailto:${g2}">${g1.trim()}</a>`,
}

The capturing group g1, ([^[\\]]*) , is possible to be empty string and, in the replacement method, we didn't check if g1.trim()is empty or not. So, we get anchor tag with empty text by apply the email rule.

The backend returns an error when we try to save html <a href="mailto:test@gmail.com"></a> to the backend. The error returned by backend is

image

I think the error from backend makes sense because the backend needs to convert html

<a href="mailto:test@gmail.com"></a>

to text(aka message[0].text) which shouldn't be empty.

So the root cause of this issue is that email rule doesn't avoid creating anchor tag with empty text.

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

To fix this issue, we can skip creating anchor tag with empty text.

To achieve this, we can change the replacement method of email rule from

replacement: (match, g1, g2) => `<a href="mailto:${g2}">${g1.trim()}</a>`,

to

// skip rendering if g1.trim() is empty
replacement: (match, g1, g2) => (!g1.trim() ? match : `<a href="mailto:${g2}">${g1.trim()}</a>`),

Below is the demo video of the solution

https://github.com/Expensify/App/assets/117511920/4fc47889-2aa8-4e10-93eb-4d9eb94d037a

What alternative solutions did you explore? (Optional)

We can also add a negative lookahead group at the beginning of the regex to skip matching email markdown syntax starts with \[\s*\].

To achieve it, we can change the regex

`\\[([^[\\]]*)]\\(${CONST.REG_EXP.MARKDOWN_EMAIL}\\)`, 'gim'

to

`(?!\\[\\s*\\])\\[([^[\\]]*)]\\(${CONST.REG_EXP.MARKDOWN_EMAIL}\\)`, 'gim'

The negative lookahead group (?!\[\s*\]) will be able to skip matching linkText includes only whitespaces(including linebreaks because the regex has multiple line flag m). See below demo video including more test cases.

https://github.com/Expensify/App/assets/117511920/d20de407-1cc7-4990-95e8-fa653b25b9c2

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

laurenreidexpensify commented 1 year ago

Checking something internally here https://expensify.slack.com/archives/C01SKUP7QR0/p1687437975259329 about this

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

parasharrajat commented 1 year ago

@eh2077 Proposal looks good to me. But I am not sure if that is the expected behavior. I am shocked that this issue existed. I am unable to remember how it used to behave a few months back.

@laurenreidexpensify Do you think the video should be expected behavior?

https://user-images.githubusercontent.com/117511920/246675874-4fc47889-2aa8-4e10-93eb-4d9eb94d037a.mov

YacineF0001 commented 1 year ago

Proposal

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

App throws 'No content to add' error if we send empty alias for email.

What is the root cause of that problem?

As @eh2077 's analyzed, when backend parse empty anchor tag, return error.

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

Simple solution is that we would not allow conversion into empty anchor tag. To do that, we can tweak a bit on Regex pattern as following.

Just simply add (?!\[[\s]*\]) at the beginning of email rule's regex pattern. image

Test result is same as @eh2077 's proposal

What alternative solutions did you explore? (Optional)

N/A

eh2077 commented 1 year ago

@YacineF0001 Thanks for tagging me in your proposal. But I think our solutions are different for test case like

[    ](test@gmail.com)

Below is the demo video for two test cases

[](test@gmail.com)
[    ](test@gmail.com)

https://github.com/Expensify/App/assets/117511920/3aaeafeb-34be-4d16-8ba0-83c4cc4c61f3

I think the App prefers to keep the user input as it is. So, in the editing mode, we display the original input.

@laurenreidexpensify Happy to hear your thoughts on the expected behaviour.

cc @parasharrajat

YacineF0001 commented 1 year ago

@eh2077 Thanks for your pointing out. it can be also done with Regex. Just add (?!\[[ ]*\]) at the beginning of pattern. image

@parasharrajat My proposal updated

eh2077 commented 1 year ago

Proposal

Updated to add an alternative regex based method and included a demo video covers more test cases.

cc @parasharrajat

YacineF0001 commented 1 year ago

@eh2077 good catching about linebreak case. but don't add it as your solution with just adding a case on my solution.

YacineF0001 commented 1 year ago

Proposal updated to add linebreak case cc @parasharrajat

eh2077 commented 1 year ago

@eh2077 Thanks for your pointing out. it can be also done with Regex.

Just add (?!\[[ ]*\]) at the beginning of pattern.

image

@parasharrajat My proposal updated

@YacineF0001 I think your method only fixes the effects and not the root cause. The method I suggested to handle all white spaces fixes the root causes. That's the essential.

YacineF0001 commented 1 year ago

@eh2077 you just added a case with '\s' instead of ' ' on my regex. i just missed linebreak case your original solution is also working for this issue, so let's wait for @parasharrajat 's review on your original one and my regex solution

eh2077 commented 1 year ago

@eh2077 you just added a case with '\s' instead of ' ' on my regex.

I'm afraid I can't agree on it. According to timeline, I first came up the regex solution to solve the root cause of all whitespaces, which is essential to fix the root cause of the issue.

so let's wait for @parasharrajat 's review on your original one and my regex solution

I think the C+ team will review it following the contribution guides here

Expensify reviews all solution proposals on a first come first serve basis. If you see other contributors have already proposed a solution, you can still provide a solution proposal and we will review it. We look for the earliest provided, best proposed solution that addresses the job.

YacineF0001 commented 1 year ago

I fixed unit test cases for the change of regex pattern. and added test case with linebreak. Proposal updated cc @parasharrajat

YacineF0001 commented 1 year ago

I think the C+ team will review it following the contribution guides here

From now on, My last updated proposal would be more improved.

laurenreidexpensify commented 1 year ago

@parasharrajat yeah I agree that the expected behaviour should be to display the broken link as normal text rather removing it and throwing an error. So exactly as you have it in the video.

parasharrajat commented 1 year ago

Hey everyone. Thanks for the updates. I will review them as soon as possible.

parasharrajat commented 1 year ago

It's definitely a tough call. I saw the timeline of all the proposal updates. It seems the @eh2077 came first with the better regex. Although @eh2077's main solution is also working, I like the idea of updating the regex.

It seems that @YacineF0001 spark the idea of regex which helped @eh2077 come up with an alternative approach but @eh2077 also shared feedback which helped reach the final proposal of @YacineF0001.

In these situations, we choose a better solution over others. This is part of OSS that a solution sparks ideas in other contributors.

As @eh2077 came up first with a better approach. I think we should go with their proposal's alternative method.

Thanks @YacineF0001 for your proposal. You seem to be a new contributor to the project. Happy to have you. There are many more opportunities waiting. I am sure you will soon land one.

:ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 1 year ago

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

YacineF0001 commented 1 year ago

@parasharrajat Thanks for your review. I hope you take care of my latest proposal. I addressed about auto-test cases to be fixed and added more. Without it, i think it can't be perfect proposal.

YacineF0001 commented 1 year ago

I think @eh2077 's proposal is imperfect, because he didn't address following issue on unit test. image @parasharrajat Do you think it's can be ignored ?

parasharrajat commented 1 year ago

I will update you tomorrow.

eh2077 commented 1 year ago

@parasharrajat Thanks for reviewing proposals.

I'm open to share the bounty with @YacineF0001 , like 50/50. I'd also suggest to let @YacineF0001 make the PR which should be his first contribution to the App! If speed bonus is qualified, then it will be compensated to him only. Hope team will take this into consideration.

melvin-bot[bot] commented 1 year ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

parasharrajat commented 1 year ago

I am fine with it @eh2077. What do you say @YacineF0001?

Bump @Gonals

YacineF0001 commented 1 year ago

@parasharrajat I agree with this, and appreciate @eh2077 's kindness and professional behavior.

melvin-bot[bot] commented 1 year ago

πŸ“£ @parasharrajat You have been assigned to this job! Please apply to this job in Upwork here and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 year ago

πŸ“£ @eh2077 You have been assigned to this job! Please apply to this job in Upwork here and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 year ago

πŸ“£ @dhanashree! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
melvin-bot[bot] commented 1 year ago

The BZ member will need to manually hire dhanashree for this role Reporter. Please store your Upwork details and apply to our Upwork job so this process is automatic in the future!

YacineF0001 commented 1 year ago

@parasharrajat Thanks for reviewing proposals.

I'm open to share the bounty with @YacineF0001 , like 50/50. I'd also suggest to let @YacineF0001 make the PR which should be his first contribution to the App! If speed bonus is qualified, then it will be compensated to him only. Hope team will take this into consideration.

@laurenreidexpensify could i apply to this job ?

parasharrajat commented 1 year ago

This is still awaiting Engineer review @laurenreidexpensify

laurenreidexpensify commented 1 year ago

@parasharrajat is tehre something specific that requires technical review? we updated the process in the past couple of weeks, so engineering review is only required on the PR or any technical conversation in the issue, so if you're happy with the solution and the team sharing the bounty, then SGTM and we can move to PR?

parasharrajat commented 1 year ago

No, Engineer needs to approve the C+ decision on issue before moving to the PR. @laurenreidexpensify

parasharrajat commented 1 year ago

Anyways, I need approval on https://github.com/Expensify/App/issues/21234#issuecomment-1610433760 before moving forward. And if we agree, let's reassign the issue to the right contributor.

Gonals commented 1 year ago

I am fine with it @eh2077. What do you say @YacineF0001?

Bump @Gonals

I was OOO until today, but I agree with this too πŸ‘

melvin-bot[bot] commented 1 year ago

πŸ“£ @parasharrajat You have been assigned to this job! Please apply to this job in Upwork here and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 year ago

πŸ“£ @YacineF0001 You have been assigned to this job! Please apply to this job in Upwork here and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 year ago

The BZ member will need to manually hire dhanashree for this role Reporter. Please store your Upwork details and apply to our Upwork job so this process is automatic in the future!

YacineF0001 commented 1 year ago

@Gonals @parasharrajat Thank you for assignment. Unfortunately, I can't apply for the job, because my Upwork account was suspended a few days ago. Is there any alternative ? Is it possible to apply there with my friend's account ?

eh2077 commented 1 year ago

@YacineF0001 I think you can start to prepare the PR. Usually you don't have to apply the job to be allowed to submit the PR. You can also apply the job when payment is due, so you'll have time to handle your Upwork account issue during that time.

YacineF0001 commented 1 year ago

Thanks for your advice. But not sure if it can be resolved. Tried something for a few days, but no response. I hope you make PR for this issue. I feel that you are eligible to do that. I will appreciate if you can share me 50% of bounty after payment is proceeded.

parasharrajat commented 1 year ago

@YacineF0001 Can you share your Upwork profile link?

YacineF0001 commented 1 year ago

already registered here. https://www.upwork.com/freelancers/~01cb7dfbaec5a6c8d6 image

parasharrajat commented 1 year ago

Unfortunately, that is a problem. @laurenreidexpensify Can you please look into this?

eh2077 commented 1 year ago

@parasharrajat Should I make the PR to move the issue forward quicker as suggested by @YacineF0001 's https://github.com/Expensify/App/issues/21234#issuecomment-1619379253 ?