-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 2023-09-20][$2000] Can't parse deep link with email param. #16762
Comments
Triggered auto assignment to @sakluger ( |
Bug0 Triage Checklist (Main S/O)
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I was able to reproduce this issue, feels like a bug worth solving! |
Job added to Upwork: https://www.upwork.com/jobs/~013659e025d6ce0e4f |
Current assignee @sakluger is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @0xmiroslav ( |
Triggered auto assignment to @johnmlee101 ( |
I couldn't reproduce this issue. I am not sure how exactly you are copying the url. Can you provide more details, or elaborate on copying step a little bit more |
ProposalPlease re-state the problem that we are trying to solve in this issue.Deep links which contain an email address are incorrectly parsed thereby resulting in broken links in the chat history. What is the root cause of that problem?The ExpensiMark parser is processing the What changes do you think we should make in order to solve the problem?Update the
This will make sure that it only matches an email that starts at the beginning of the line, or if there's a space preceding the email, thereby excluding emails that are already in links. The second capture group contains the matching email, so we use that instead of match, which may include a space. The replacement string is conditionally updated to be prefixed with a space based on the match. This has been tested and works well with the following scenarios:
Please note that the faulty parsed HTML result (with the broken links) occurs at the composer end, before the data is sent to the API, so previous messages that were already malformed will still remain the same way even after updating the ExpensiMark class implementation. A user would have to edit these messages after the changes have been applied in order to fix the broken links. What alternative solutions did you explore? (Optional)None. |
ProposalPlease re-state the problem that we are trying to solve in this issue.App can't parse the deep link detail with email param. The URL prefix (https://) is not part of the link, and the rest is in mailto protocol => we link incorrectly. What is the root cause of that problem?Context: We're using an internal library to parse a message from text to html in this line The issue described in GH issue happens only when users send a plain-text url that contain email as a param. i.e Given input is What changes do you think we should make in order to solve the problem?To fix this issue, we need to:
Code exampleReplace those LOC by {
name: 'autoEmail',
regex: new RegExp(
`(?![^<]*>|[^<>]*<\\/)((?:\\S*=)?)${CONST.REG_EXP.MARKDOWN_EMAIL}(?![^<]*(<\\/pre>|<\\/code>|<\\/a>))`,
'gim'
),
replacement: (match, g1, g2) => {
const urlRegex = new RegExp(`^${URL_REGEX}$`, 'gi');
if (g1 && urlRegex.test(match)) {
return match;
}
return `${g1 || ''}<a href="mailto:${g2}">${g2}</a>`;
}
} |
ProposalPlease re-state the problem that we are trying to solve in this issue.App can't parse the deep link detail with email param. The URL prefix (https://) is not part of the link, and we instead link incorrectly. What is the root cause of that problem?The root cause is here https://github.com/Expensify/expensify-common/blob/3590b7371fe8fa73288a6625daad985e7fcfb057/lib/ExpensiMark.js#L101 where we're parsing the So when we're trying to parse The ordering of Let's consider this email What changes do you think we should make in order to solve the problem?We need to do 3 things:
For this, we can modify the
(note the lookahead Edited: Looks like this has been recently fixed in Expensify/expensify-common#534 In current staging app, we can also easily reproduce it by sending the below
The link To fix this we need to move the What alternative solutions did you explore? (Optional)
to match the email-compatible part between the matched domain and the FYI the approach to avoid replacing the autoEmail if the matched string is an URL will not work if the matched email string is not the full URL but only a part of the URL (For example an URL with the |
ProposalPlease re-state the problem that we are trying to solve in this issue.if we parse link with email param, it parased as email not link. What is the root cause of that problem?the REGEXP of What changes do you think we should make in order to solve the problem?edit REGEXP of {
name: 'autoEmail',
regex: new RegExp(
`(?![^<]*>|[^<>]*<\\/)((?:\\S*[=/])?)${CONST.REG_EXP.MARKDOWN_EMAIL}(?![^<]*(<\\/pre>|<\\/code>|<\\/a>))`,
'gim',
),
replacement: (match, g1, g2) => {
// this condition will return true if g1 is link/auto link which mean this email is part of link/auto link.
// we can also use URL_REGEX and MARKDOWN_URL_REGEX like this
// if(g1 match URL_REGEX or g1 match MARKDOWN_URL_REGEX) return match;
if (Str.isValidURL(Str.sanitizeURL(g1))) {
// if email is part of link or auto link skip parsing.
return match;
}
return g1 + `<a href="mailto:${g2}">${g2}</a>`;
},
}, Explanation
all test passed in expensify-common in this comment #16762 (comment) What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.App can't parse the deep link detail with email param. The URL prefix (https://) is not part of the link, and we instead link incorrectly. What is the root cause of that problem?1 - When the user tries to send a deeplink with email as param, the parser rule that we have defined in the library rule autoEmail should automatically links the email. However our current rule can only work with HTML tags not the raw URL string. 2 - The placement of autoemail rule is also contributing to the problem, as it's been placed before autolink. What changes do you think we should make in order to solve the problem?Step 1 - Update the regex rule to prevent match emails if string contains http or https Step 2 - Reorder autoEmail rule to check if the matching string is a URL, we will use URL_REGEX and don't need to format to an email link What alternative solutions did you explore? (Optional)none |
📣 @anaskhraza! 📣 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.
Format:
|
@Antasel let's make sure to add all automated test cases if possible. |
@0xmiroslav The following test cases are covering all of what you mentioned Test Cases
|
@0xmiroslav @sakluger |
@johnmlee101 |
Expensify/expensify-common#573 was merged but based on https://expensify.slack.com/archives/C02NK2DQWUX/p1694406422306609, we can wait a bit more for app PR (just version bump) to be merged until next deploy. |
@Antasel you'll have to wait for the approval. We typically wait for you to be in these channels before we allow for fully processing payments as a requirement. Note this will not affect any of the payments themselves |
@johnmlee101 |
@Antasel got it, thanks for the context. We still need to wait for a slack admin to approve the invite, it's usually done within a day or so. |
Hi, it should have been approved. Please check your email! |
I sent email again to [email protected] yesterday. but still no updated. waiting more |
expensify-common package version was bumped to the latest in #27169 and our fix was already deployed to staging. |
@0xmiroslav then no need to make App PR ? |
Correct. You can test staging app right now |
@0xmiroslav Is there any guide to run staging app for desktop, android, iOS ? |
iOS staging app is on Testflight https://testflight.apple.com/join/ucuXr4g5 |
thanks. |
I also don't know. Your questions are out of scope for this GH. |
Ok. I've tested on staging web. working as expected. Screencast.from.12-9-23.05.25.49+06.webm |
The PR isn't showing that it was deployed to staging or prod, @0xmiroslav is that specific to Expensfy/expensify-common? I want to make sure the payment date is reflected correctly. |
Correct, in general there should be app PR linked to this issue which just bumps expensify-common version. In our case, we can update issue title manually. |
@Antasel Here's the answer: |
@0xmiroslav thanks for your kind answer |
@0xmiroslav thanks! Copying over the BZ checklist manually since the automation didn't post it in this case: 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:
|
So excited to close out this issue, thanks everyone! Summarizing payouts for this issue: Issue reporter: @hungvu193 $250 (hired and paid via Upwork) Above payments include efficiency bonus 🎉 @0xmiroslav - two things:
|
No regressions. We added lots of automated test cases so I think that should cover regression test enough. |
upwork |
Thanks! I completed the payment through Upwork. |
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:
Expected Result:
App should parse the deep link detail with email param.
Actual Result:
App can't parse the deep link detail with email param. The URL prefix (
https://
) is not part of the link, and we instead link incorrectly.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?
Version Number: 1.2.92-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
Screen.Recording.2023-03-30.at.22.43.51.mov
Expensify/Expensify Issue URL:
Issue reported by: @hungvu193
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680191331185739
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: