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 2023-09-20][$2000] Can't parse deep link with email param. #16762

Closed
1 of 6 tasks
kavimuru opened this issue Mar 30, 2023 · 207 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Mar 30, 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. Try to open user detail from deep link on browser (for example: https://staging.new.expensify.com/[email protected]).
  2. Now go to any report, paste that link and click send.
  3. Notice that it can't parse the whole link, and when user try to click on it, it opens the email app with the wrong email filled.

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?

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

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

Untitled

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
  • Upwork Job URL: https://www.upwork.com/jobs/~013659e025d6ce0e4f
  • Upwork Job ID: 1644482548992663552
  • Last Price Increase: 2023-06-26
  • Automatic offers:
    • 0xmiroslav | Reviewer | 26524537
    • Antasel | Contributor | 26524539
    • hungvu193 | Reporter | 26524541
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 30, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

MelvinBot commented Mar 30, 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

@melvin-bot melvin-bot bot added the Overdue label Apr 3, 2023
@MelvinBot

This comment was marked as outdated.

@MelvinBot

This comment was marked as outdated.

@MelvinBot

This comment was marked as outdated.

@sakluger
Copy link
Contributor

sakluger commented Apr 7, 2023

I was able to reproduce this issue, feels like a bug worth solving!

@melvin-bot melvin-bot bot removed the Overdue label Apr 7, 2023
@sakluger sakluger added External Added to denote the issue can be worked on by a contributor Overdue labels Apr 7, 2023
@melvin-bot melvin-bot bot changed the title Can't parse deep link with email param. [$1000] Can't parse deep link with email param. Apr 7, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 7, 2023
@MelvinBot
Copy link

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

@sakluger sakluger removed the Overdue label Apr 7, 2023
@alitoshmatov
Copy link
Contributor

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

@akinwale
Copy link
Contributor

akinwale commented Apr 8, 2023

Proposal

Please 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 autoEmail rule before the autoLink rule, which results in a broken link, because the email part of the deep link is parsed first, hence the full link will never be recognised.
The currently defined order is like so: https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L95-L130

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

Update the autoEmail rule like so:

{
    name: 'autoEmail',
    regex: new RegExp(
        `(?![^<]*>|[^<>]*<\\/)(^| |(?<!http(s?):\\S*))${CONST.REG_EXP.MARKDOWN_EMAIL}(?![^<]*(<\\/pre>|<\\/code>|<\\/a>))`,
        'gim',
    ),
    replacement: (match, g1, g2, g3) => {
        let repl = `<a href="mailto:${g3}">${g3}</a>`;
        if (match.startsWith(' ')) {
            repl = ' ' + repl;
        }
        return repl;
    }
}

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:

[email protected] https://www.expensify.com
bruh [email protected] https://www.expensify.com
https://www.expensify.com [email protected]
https://staging.new.expensify.com/details?login= [email protected] bacon [email protected]
multiple spaces https://www.expensify.com    [email protected]
[test ]]([email protected])
another scenario [[email protected]]
checking boundaries \[email protected]
https://staging.new.expensify.com/details?name=test&email= [email protected]
[email protected] last check
[email protected] [email protected] tests fine
https://www.expensify.com?name=test&[email protected]
[email protected] https://www.expensify.com?name=test&[email protected] [email protected]

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.

@hoangzinh
Copy link
Contributor

hoangzinh commented Apr 8, 2023

Proposal

Please 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.
For more details, with a plain-text url string, when it's passed into our parser, it will reach this parser's rule autoEmail. As described in description, this rule tries to "automatically links emails that are not in a link". But it only works with HTML tag but not raw url string.

i.e Given input is https://staging.new.expensify.com/[email protected]
=> matching text of this rule will be //staging.new.expensify.com/[email protected]
=> output after this rule is <a href="mailto://staging.new.expensify.com/[email protected]">//staging.new.expensify.com/[email protected]</a>
=> it leads to the out of whole parser is https:<a href="mailto://staging.new.expensify.com/[email protected]">//staging.new.expensify.com/[email protected]</a>

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

To fix this issue, we need to:

  1. Update the regex rule of autoEmail so that it includes optional substring regex ((?:\\S*=)?)
  2. Update replacement of autoEmail rule so that, if the matching string is an URL, we don't need to format to an email link.
Code example

Replace 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>`;
              }
          }

@tienifr
Copy link
Contributor

tienifr commented Apr 8, 2023

Proposal

Please 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 autoEmail before the autoLink.

So when we're trying to parse https://staging.new.expensify.com/[email protected], it will parse //staging.new.expensify.com/[email protected] as an email. Then when we try to apply the autolink, it will not work because the <a> tags are already inserted by the autoEmail rule.

The ordering of autoEmail and autolink are like that for a reason, to avoid the auto link rule to apply to a part of the email address.

Let's consider this email [email protected], if the autolink rule is before the autoEmail rule, the google.com will become an auto link and it will not be parsed as an email.

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

We need to do 3 things:

  1. Put the auto link rule before the autoEmail rule, this can be done simply by reodering this rule https://github.com/Expensify/expensify-common/blob/3590b7371fe8fa73288a6625daad985e7fcfb057/lib/ExpensiMark.js#L101 and this rule https://github.com/Expensify/expensify-common/blob/3590b7371fe8fa73288a6625daad985e7fcfb057/lib/ExpensiMark.js#L114

  2. Address this issue.

The ordering of autoEmail and autolink are like that for a reason, to avoid the auto link rule to apply to a part of the email address.

For this, we can modify the URL_WEBSITE_REGEX here https://github.com/Expensify/expensify-common/blob/f37b3bc00d1275f232c7ddb8bbcb5b6f69310918/lib/Url.js#L3 such that it will not match if the domain is followed by an email signature (a.k.a @domain.tld). This can be done by updating this rule https://github.com/Expensify/expensify-common/blob/f37b3bc00d1275f232c7ddb8bbcb5b6f69310918/lib/Url.js#L3
to

const URL_WEBSITE_REGEX = `${URL_PROTOCOL_REGEX}?((?:www\\.)?[a-z0-9](?:[-a-z0-9]*[a-z0-9])?\\.)+(?:${TLD_REGEX})(?!@[a-zA-Z0-9-]+?(\.[a-zA-Z]+)+)(?:\\:${ALLOWED_PORTS}|\\b|(?=_))`;

(note the lookahead (?!@[a-zA-Z0-9-]+?(\.[a-zA-Z]+)+) to match the @domain.tld signature, it's the same regex we're using to match that part of an autoEmail)

Edited: Looks like this has been recently fixed in Expensify/expensify-common#534
There's a hidden bug in here that makes the code abort all instances of matching links if the first match has @. This is because if abort is set to true once, it will persist on the next iteration of the matching links.

In current staging app, we can also easily reproduce it by sending the below

⁦@expensify.com 
https://www.expensify.com

The link https://www.expensify.com will not be detected.

To fix this we need to move the let abort = false; inside the while loop here https://github.com/Expensify/expensify-common/blob/3cdaa947fe77016206c15e523017cd50678f2359/lib/ExpensiMark.js#L372 so it can be reset to false when processing the next match.

What alternative solutions did you explore? (Optional)

  • We can improve the regex in 2 if there're special edge cases that need to be addressed (special characters, ...), for example by adding
[a-zA-Z0-9.!$%&'*+=^_`{|}~-]

to match the email-compatible part between the matched domain and the @domain.tld signature, but the main point is the same. If an accidental URL that belongs to an email address is not parsed as an auto link, it will be safe to reorder the rules.

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 & character)

@ahmedGaber93
Copy link
Contributor

ahmedGaber93 commented Apr 9, 2023

Proposal

Please 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 autoEmail allow to parse email in link.

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

edit REGEXP of autoEmail to be match optional ((?:\\S*[=/])?) and use it in replacement case like this.

{
    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

  1. RegExp will match two groups ((?:\\S*[=/])?) and ${CONST.REG_EXP.MARKDOWN_EMAIL}.
  2. we will use group1 to determined if the matched email (group2) is part of link or not.
    2.1 if group1 is found, we need to confirm it is a link and not a normal text like chars "-" or ":"
    2.2 we will use Str.isValidURL or URL_REGEX to confirm the group1 is a link.
    2.3 in case group1 is auto link, we will use Str.sanitizeURL to convert it to link, then check if it is a link or not Str.isValidURL(Str.sanitizeURL(g1)).
  3. if we confirm group1 is a link or auto link, skip parse auto email return match;
  4. if group1 is not a link, return group1 + parsed email return g1 + `<a href="mailto:${g2}">${g2}</a>`;

all test passed in expensify-common in this comment #16762 (comment)

What alternative solutions did you explore? (Optional)

N/A

@anaskhraza
Copy link

Proposal

Please 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

@MelvinBot
Copy link

📣 @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.
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>

@0xmiros
Copy link
Contributor

0xmiros commented Sep 6, 2023

@Antasel let's make sure to add all automated test cases if possible.
Combine all of these:
#16762 (comment)
#16762 (comment)
#16762 (comment)
And more whatever you find is edge case including yours - #16762 (comment)

@Antasel
Copy link
Contributor

Antasel commented Sep 6, 2023

@0xmiroslav The following test cases are covering all of what you mentioned

Test Cases
test('Test a url with email autolinks correctly', () => {
    let testString = 'https://staging.new.expensify.com/[email protected]';
    let resultString = '<a href="https://staging.new.expensify.com/[email protected]" target="_blank" rel="noreferrer noopener">https://staging.new.expensify.com/[email protected]</a>';
    testString = '[email protected]/https://www.expensify.com';
    resultString = '<a href="mailto:[email protected]">[email protected]</a>/<a href="https://www.expensify.com" target="_blank" rel="noreferrer noopener">https://www.expensify.com</a>';
    expect(parser.replace(testString)).toBe(resultString);
});
test('Test a url autolinks correctly', () => {
    let testString = '[email protected] https://www.expensify.com\n' +
    '[email protected]://www.expensify.com\n' +
    '[email protected]/https://www.expensify.com\n' +
    '[email protected]?https://www.expensify.com\n' +
    '[email protected]>https://www.expensify.com\n' +
    'https://staging.new.expensify.com/details/[email protected]\n' +
    'staging.new.expensify.com/details\n\n' +
    'https://www.expensify.com?name=test&[email protected]\n' +
    'https://staging.new.expensify.com/[email protected]\n' +
    'staging.new.expensify.com/[email protected]\n' +
    'http://necolas.github.io/react-native-web/docs/?path=/docs/components-pressable--disabled\n' +
    '-https://www.expensify.com /https://www.expensify.com @https://www.expensify.com\n' +
    'expensify.com -expensify.com @expensify.com\n' +
    'https//www.expensify.com\n' +
    '//www.expensify.com?name=test&[email protected]\n' +
    '//staging.new.expensify.com/[email protected]\n' +
    '/[email protected]\n' +
    '?name=test&[email protected]\n\n' +
    'example.com/https://www.expensify.com\n' +
    '[email protected] staging.new.expensify.com/[email protected]&redirectUrl=https://google.com\n' +
    '[email protected] //staging.new.expensify.com/[email protected]&redirectUrl=https://google.com\n' +
    '[email protected]://staging.new.expensify.com/[email protected]&redirectUrl=https://google.com';
    let resultString = '<a href="mailto:[email protected]">[email protected]</a> <a href="https://www.expensify.com" target="_blank" rel="noreferrer noopener">https://www.expensify.com</a><br />' + 
    '<a href="mailto:[email protected]">[email protected]</a>-<a href="https://www.expensify.com" target="_blank" rel="noreferrer noopener">https://www.expensify.com</a><br />' +
    '<a href="mailto:[email protected]">[email protected]</a>/<a href="https://www.expensify.com" target="_blank" rel="noreferrer noopener">https://www.expensify.com</a><br />' +
    '<a href="mailto:[email protected]">[email protected]</a>?<a href="https://www.expensify.com" target="_blank" rel="noreferrer noopener">https://www.expensify.com</a><br />' + 
    '<a href="mailto:[email protected]">[email protected]</a>&gt;<a href="https://www.expensify.com" target="_blank" rel="noreferrer noopener">https://www.expensify.com</a><br />' +
    '<a href="https://staging.new.expensify.com/details/[email protected]" target="_blank" rel="noreferrer noopener">https://staging.new.expensify.com/details/[email protected]</a><br />' +
    '<a href="https://staging.new.expensify.com/details" target="_blank" rel="noreferrer noopener">staging.new.expensify.com/details</a><br /><br />'+ 
    '<a href="https://www.expensify.com?name=test&amp;[email protected]" target="_blank" rel="noreferrer noopener">https://www.expensify.com?name=test&amp;[email protected]</a><br />' +
    '<a href="https://staging.new.expensify.com/[email protected]" target="_blank" rel="noreferrer noopener">https://staging.new.expensify.com/[email protected]</a><br />' +
    '<a href="https://staging.new.expensify.com/[email protected]" target="_blank" rel="noreferrer noopener">staging.new.expensify.com/[email protected]</a><br />' +
    '<a href="http://necolas.github.io/react-native-web/docs/?path=/docs/components-pressable--disabled" target="_blank" rel="noreferrer noopener">http://necolas.github.io/react-native-web/docs/?path=/docs/components-pressable--disabled</a><br />' +
    '-<a href="https://www.expensify.com" target="_blank" rel="noreferrer noopener">https://www.expensify.com</a> /<a href="https://www.expensify.com" target="_blank" rel="noreferrer noopener">https://www.expensify.com</a> @https://www.expensify.com<br />' +
    '<a href="https://expensify.com" target="_blank" rel="noreferrer noopener">expensify.com</a> -<a href="https://expensify.com" target="_blank" rel="noreferrer noopener">expensify.com</a> @expensify.com<br />' +
    'https//<a href="https://www.expensify.com" target="_blank" rel="noreferrer noopener">www.expensify.com</a><br />' +
    '//<a href="https://www.expensify.com?name=test&amp;[email protected]" target="_blank" rel="noreferrer noopener">www.expensify.com?name=test&amp;[email protected]</a><br />' +
    '//<a href="https://staging.new.expensify.com/[email protected]" target="_blank" rel="noreferrer noopener">staging.new.expensify.com/[email protected]</a><br />' +
    '/details?login=<a href="mailto:[email protected]">[email protected]</a><br />' +
    '?name=test&amp;email=<a href="mailto:[email protected]">[email protected]</a><br /><br />' +
    '<a href="https://example.com/https://www.expensify.com" target="_blank" rel="noreferrer noopener">example.com/https://www.expensify.com</a><br />' +
    '<a href="mailto:[email protected]">[email protected]</a> <a href="https://staging.new.expensify.com/[email protected]&amp;redirectUrl=https://google.com" target="_blank" rel="noreferrer noopener">staging.new.expensify.com/[email protected]&amp;redirectUrl=https://google.com</a><br />' +
    '<a href="mailto:[email protected]">[email protected]</a> //<a href="https://staging.new.expensify.com/[email protected]&amp;redirectUrl=https://google.com" target="_blank" rel="noreferrer noopener">staging.new.expensify.com/[email protected]&amp;redirectUrl=https://google.com</a><br />' +
    '<a href="mailto:[email protected]">[email protected]</a>-<a href="https://staging.new.expensify.com/[email protected]&amp;redirectUrl=https://google.com" target="_blank" rel="noreferrer noopener">https://staging.new.expensify.com/[email protected]&amp;redirectUrl=https://google.com</a>';
    expect(parser.replace(testString)).toBe(resultString);

    testString = '[email protected]/https://example.com/[email protected][email protected]';
    resultString = '<a href="mailto:[email protected]\">[email protected]</a>/<a href="https://example.com/[email protected][email protected]" target="_blank" rel="noreferrer noopener">https://example.com/[email protected][email protected]</a>';
    expect(parser.replace(testString)).toBe(resultString);

    testString = '[email protected]/[email protected]/https://example.com/[email protected][email protected]';
    resultString = '<a href="mailto:[email protected]">[email protected]</a>/<a href="mailto:[email protected]">[email protected]</a>/<a href="https://example.com/[email protected][email protected]" target="_blank" rel="noreferrer noopener">https://example.com/[email protected][email protected]</a>';
    expect(parser.replace(testString)).toBe(resultString);
});

@Antasel
Copy link
Contributor

Antasel commented Sep 6, 2023

@0xmiroslav @sakluger
The PR Expensify/expensify-common#573 for expensify-common is ready for review

@Antasel
Copy link
Contributor

Antasel commented Sep 11, 2023

@johnmlee101
Could you help me to get expensify open-source channel invitation on Slack ?
I've sent email to [email protected], but still no on my [email protected] .

@0xmiros
Copy link
Contributor

0xmiros commented Sep 11, 2023

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.

Screenshot 2023-09-11 at 6 43 21 AM

@johnmlee101
Copy link
Contributor

@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

@Antasel
Copy link
Contributor

Antasel commented Sep 11, 2023

@johnmlee101
I am not worrying about payment, just would like to live in expensify community and get instant help from other devs if needed.
I am getting some new issues on building android/ios

@sakluger
Copy link
Contributor

@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.

@johnmlee101
Copy link
Contributor

Hi, it should have been approved. Please check your email!

@Antasel
Copy link
Contributor

Antasel commented Sep 12, 2023

Hi, it should have been approved. Please check your email!

I sent email again to [email protected] yesterday. but still no updated. waiting more

@0xmiros
Copy link
Contributor

0xmiros commented Sep 12, 2023

expensify-common package version was bumped to the latest in #27169 and our fix was already deployed to staging.
So all good now.

@Antasel
Copy link
Contributor

Antasel commented Sep 12, 2023

@0xmiroslav then no need to make App PR ?

@0xmiros
Copy link
Contributor

0xmiros commented Sep 12, 2023

@0xmiroslav then no need to make App PR ?

Correct. You can test staging app right now

@Antasel
Copy link
Contributor

Antasel commented Sep 12, 2023

@0xmiroslav Is there any guide to run staging app for desktop, android, iOS ?

@0xmiros
Copy link
Contributor

0xmiros commented Sep 12, 2023

@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

@Antasel
Copy link
Contributor

Antasel commented Sep 12, 2023

thanks.
Where can i download desktop app image ? and for android ?

@0xmiros
Copy link
Contributor

0xmiros commented Sep 12, 2023

thanks. Where can i download desktop app image ? and for android ?

I also don't know. Your questions are out of scope for this GH.
You can just verify staging web as this issue doesn't depend on platforms.

@Antasel
Copy link
Contributor

Antasel commented Sep 12, 2023

Ok. I've tested on staging web. working as expected.

Screencast.from.12-9-23.05.25.49+06.webm

@sakluger
Copy link
Contributor

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.

@0xmiros
Copy link
Contributor

0xmiros commented Sep 14, 2023

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.
But this is edge case. Version was bumped in other PR which fixes another issue.

In our case, we can update issue title manually.
Deployed to production on Sep 13 - #27169 (comment)

@0xmiros
Copy link
Contributor

0xmiros commented Sep 14, 2023

thanks. Where can i download desktop app image ? and for android ?

@Antasel Here's the answer:
desktop: https://staging.new.expensify.com/NewExpensify.dmg
android: I believe there you would have to be accepted on some internal list so its not very scalable

@Antasel
Copy link
Contributor

Antasel commented Sep 18, 2023

@0xmiroslav thanks for your kind answer

@sakluger sakluger changed the title [$2000] Can't parse deep link with email param. [HOLD for Payment 2023-09-20][$2000] Can't parse deep link with email param. Sep 18, 2023
@sakluger
Copy link
Contributor

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

  • [@0xmiroslav] The PR that introduced the bug has been identified. Link to the PR:
  • [@0xmiroslav] 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:
  • [@0xmiroslav] 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:
  • [@0xmiroslav] Determine if we should create a regression test for this bug.
  • [@0xmiroslav] 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.
  • [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@sakluger
Copy link
Contributor

So excited to close out this issue, thanks everyone!

Summarizing payouts for this issue:

Issue reporter: @hungvu193 $250 (hired and paid via Upwork)
Contributor: @Antasel $3000 (hired and paid via Upwork)
Contributor+: @0xmiroslav $3000

Above payments include efficiency bonus 🎉
Upwork job: https://www.upwork.com/jobs/~013659e025d6ce0e4f

@0xmiroslav - two things:

  1. Could you please complete the BZ checklist?
  2. Would you like to be paid via NewDot manual request or via Upwork? I see that we sent you an offer here, but it's probably because this issue is so old.

@0xmiros
Copy link
Contributor

0xmiros commented Sep 20, 2023

No regressions. We added lots of automated test cases so I think that should cover regression test enough.

@0xmiros
Copy link
Contributor

0xmiros commented Sep 20, 2023

Would you like to be paid via NewDot manual request or via Upwork?

upwork

@sakluger
Copy link
Contributor

Thanks! I completed the payment through Upwork.

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests