[$1000] App doesn't open a new tab when clicking on a URL of staging on production #22523

kavimuru · 2023-07-09T05:13:56Z

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:

Go to production
Open any report
Copy a deep link of staging i.e https://staging.new.expensify.com/settings/profile/display-name
Send the link as a message in report
Click on the link

Expected Result:

App should open a new tab to redirect to the page in staging

Actual Result:

App doesn't open a new tab and the link is production link

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

Screencast.from.06-07-2023.00_53_10.webm

Expensify/Expensify Issue URL:
Issue reported by: @dukenv0307
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688619360838289?thread_ts=1688135098.594039&cid=C049HHMV9SM

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e923e4ff221ffae9
Upwork Job ID: 1678503120618921984
Last Price Increase: 2023-08-07
Automatic offers:

situchan | Contributor | 25863058
dukenv0307 | Reporter | 25863059
Talha345 | Contributor | 26077438

melvin-bot · 2023-07-09T05:14:01Z

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

melvin-bot · 2023-07-09T05:14:04Z

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

tienifr · 2023-07-10T09:06:20Z

Proposal

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

App doesn't open a new tab when clicking on a URL of staging on production

What is the root cause of that problem?

In

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Lines 29 to 33 in b9f1d47

    
           const internalNewExpensifyPath = 
        
               (Url.hasSameExpensifyOrigin(attrHref, CONST.NEW_EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONST.STAGING_NEW_EXPENSIFY_URL)) && 
        
               !CONST.PATHS_TO_TREAT_AS_EXTERNAL.includes(attrPath) 
        
                   ? attrPath 
        
                   : '';

We're opening the link internally if the url has the same origin with staging link (https://staging.new.expensify.com) or production link (https://new.expensify.com)

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Lines 51 to 54 in b9f1d47

    
           if (internalNewExpensifyPath) { 
        
               Navigation.navigate(internalNewExpensifyPath); 
        
               return; 
        
           }

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

We should create a new function to check internal origin

On website platforms

    hasNewExpensifyOrigin(attrHref){
 return Url.hasSameExpensifyOrigin(attrHref, window.location.origin);
}

On others platforms, we don't have window.location.origin and I think we shouldn't open link internally on native/desktop platform if the hostname is localhost. That why I want to preserve the current main logic here

    hasNewExpensifyOrigin(attrHref){
 return Url.hasSameExpensifyOrigin(attrHref, CONST.NEW_EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONST.STAGING_NEW_EXPENSIFY_URL);
}

and change this line

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Line 30 in b105aa3

    
           (Url.hasSameExpensifyOrigin(attrHref, CONST.NEW_EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONST.STAGING_NEW_EXPENSIFY_URL)) &&

to

            hasNewExpensifyOrigin(attrHref) &&

Unfortunately, the current hasSameExpensifyOrigin function doesn't support http://localhost:8080 so I want to change the getURLObject to support all the kinds of link

Change URL_WEBSITE_REGEX to MARKDOWN_URL_REGEX, baseUrl=match[3], protocol = match[4];

Result

Screen.Recording.2023-07-20.at.11.58.38.mov

adelekennedy · 2023-07-10T20:34:56Z

Reproduced and up to date on the Slack thread

melvin-bot · 2023-07-10T20:35:05Z

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

melvin-bot · 2023-07-10T20:35:09Z

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

melvin-bot · 2023-07-10T20:35:15Z

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

Talha345 · 2023-07-11T01:01:16Z

Proposal

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

If a staging link (staging.new.expensify.com) is sent on production app via a chat message ,the app treats it like an internal link and tries to open it internally via react-navigation instead of opening it in a new tab as an external link.

What is the root cause of that problem?

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Lines 29 to 33 in b105aa3

    
           const internalNewExpensifyPath = 
        
               (Url.hasSameExpensifyOrigin(attrHref, CONST.NEW_EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONST.STAGING_NEW_EXPENSIFY_URL)) && 
        
               !CONST.PATHS_TO_TREAT_AS_EXTERNAL.includes(attrPath) 
        
                   ? attrPath 
        
                   : '';

The internalNewExpensifyPath is only checking if the given link is an internal new expensify link i.e it is either a production or a staging link and not verifying the link with the current URL host.

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Lines 49 to 54 in b105aa3

    
           // If we are handling a New Expensify link then we will assume this should be opened by the app internally. This ensures that the links are opened internally via react-navigation 
        
           // instead of in a new tab or with a page refresh (which is the default behavior of an anchor tag) 
        
           if (internalNewExpensifyPath) { 
        
               Navigation.navigate(internalNewExpensifyPath); 
        
               return; 
        
           }

Since, the conditions are true here as the given link is a staging link, so it is opened internally via react-navigation.

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

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Lines 23 to 33 in b8aeec0

    
           const isAttachment = Boolean(htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]); 
        
           const displayName = lodashGet(props.tnode, 'domNode.children[0].data', ''); 
        
           const parentStyle = lodashGet(props.tnode, 'parent.styles.nativeTextRet', {}); 
        
           const attrHref = htmlAttribs.href || ''; 
        
           const attrPath = lodashGet(Url.getURLObject(attrHref), 'path', '').replace('/', ''); 
        
           const hasExpensifyOrigin = Url.hasSameExpensifyOrigin(attrHref, CONFIG.EXPENSIFY.EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONFIG.EXPENSIFY.STAGING_API_ROOT); 
        
           const internalNewExpensifyPath = 
        
               (Url.hasSameExpensifyOrigin(attrHref, CONST.NEW_EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONST.STAGING_NEW_EXPENSIFY_URL)) && 
        
               !CONST.PATHS_TO_TREAT_AS_EXTERNAL.includes(attrPath) 
        
                   ? attrPath 
        
                   : '';

In the above code, calculation of attrPath is changed like this to accomodate DEV urls since attrPath is calculated from getURLObject method which utilizes a regex from here to match the provided URL and the regex does not take into account URLs such as localhost:8080 for DEV envs.:

const isDevUrl = attrHref.startsWith(CONST.DEV_NEW_EXPENSIFY_URL);
    const attrPath = isDevUrl ? Url.getPathFromDevURL(attrHref) : lodashGet(Url.getURLObject(attrHref), 'path', '').replace('/', '');

Making changes to the mentioned Regex just to accomodate localhost is not feasible because of the way it has been structured and used.

where getPathFromDevURL

function getPathFromDevURL(url) {
    const path = new URL(url).pathname;
    return path.substring(1); // Remove the leading '/'
}

A new simpler method to compare the origin of the provided URL with the current URL. (KISS principle)

function hasSameOrigin(url1, url2) {
    const host1 = new URL(url1).host;
    const host2 = new URL(url2).host;
    return host1 === host2;
}

3.Then using const {environmentURL} = useEnvironment(); from here like this: const hasSameOrigin = Url.hasSameOrigin(attrHref, environmentURL);

The if clause for internalNewExpensifyPath is updated as follows:

const internalNewExpensifyPath =
        (Url.hasSameExpensifyOrigin(attrHref, CONST.NEW_EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONST.STAGING_NEW_EXPENSIFY_URL) || isDevUrl) &&
        !CONST.PATHS_TO_TREAT_AS_EXTERNAL.includes(attrPath)
            ? attrPath
            : '';

And finally:

if (internalNewExpensifyPath && hasSameOrigin) {
            Navigation.navigate(internalNewExpensifyPath);
            return;
        }

I would also like to use https://www.npmjs.com/package/react-native-url-polyfill package to make the new URL(url) object work across all platforms since RN URL polyfill has known issues ,see https://github.com/charpeni/react-native-url-polyfill#why-do-we-need-this.

The methods getPathFromDevURL and hasSameOrigin in my proposal do not have guard clauses for invalid URLs which I will obviously add in the PR.

What alternative solutions did you explore? (Optional)

Result

For Result after this implementation, please see #22523 (comment)

melvin-bot · 2023-07-11T01:01:19Z

📣 @Talha345! 📣
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:

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.
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.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

Talha345 · 2023-07-11T01:04:59Z

Contributor details
Your Expensify account email: talha.97.mahmood@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~0135b581d6099afe8a

melvin-bot · 2023-07-11T01:05:01Z

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

alitoshmatov · 2023-07-11T07:17:18Z

Proposal

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

App doesn't open a new tab when clicking on a URL of staging on production

What is the root cause of that problem?

The root cause is here when we are checking for the same origin we are also checking for staging url and if url matches staging url we are assuming it is the same origin

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Line 28 in b8aeec0

    
           const hasExpensifyOrigin = Url.hasSameExpensifyOrigin(attrHref, CONFIG.EXPENSIFY.EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONFIG.EXPENSIFY.STAGING_API_ROOT);

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

We should remove Url.hasSameExpensifyOrigin(attrHref, CONFIG.EXPENSIFY.STAGING_API_ROOT) from this condition, because staging url is not the same origin as production.

We don't need to worry about opening internal urls in staging because when app is in staging environment CONFIG.EXPENSIFY.EXPENSIFY_URL in the condition becomes staging url

App/.env.staging

Line 1 in 8bc6f52

NEW_EXPENSIFY_URL=https://staging.new.expensify.com/

What alternative solutions did you explore? (Optional)

adelekennedy · 2023-07-14T19:21:15Z

@0xmiroslav pending proposal review

Talha345 · 2023-07-17T14:43:07Z

@adelekennedy @0xmiroslav any update on this?

adelekennedy · 2023-07-17T15:36:03Z

@Talha345 we're pending @0xmiroslav review on the proposals above!

Talha345 · 2023-07-17T15:47:40Z

@adelekennedy Yes i know but since @0xmiroslav has been MIA, I was wondering if there was a procedure in place to assign some other C+ member instead.

adelekennedy · 2023-07-17T15:51:05Z

I've reached out in Slack - this isn't a pressing bug at the moment so I'd rather give them time to review this week before assigning another member of the C+ team

melvin-bot · 2023-07-17T16:00:04Z

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

0xmiros · 2023-07-17T17:03:03Z

http://localhost:8080/settings/profile/display-name
https://staging.new.expensify.com/settings/profile/display-name
https://new.expensify.com/settings/profile/display-name

I think we should confirm the expected behavior per each env.

env	localhost:8080	staging.new.expensify.com	new.expensify.com
dev	internal 🔴	internal	internal
staging	external	internal	external 🔴
prod	external	external 🔴	internal

Here, internal means deep link, external means new tab, 🔴 is to fix

@adelekennedy do you agree with this table?

Talha345 · 2023-08-11T08:51:34Z

Hi @francoisl , those links should work as expected as we are not changing any code that affects which URLs are to be rendered as links or not.

The code we are changing is only being used to create a URL object from a string and then compare those URL objects origins.I also tested it with the proposed changes and everything seems to be working fine.

francoisl · 2023-08-11T22:30:12Z

Ah ok makes sense, thanks for clarifying. Let's go with your proposal then.

melvin-bot · 2023-08-11T22:30:34Z

📣 @Talha345 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

Talha345 · 2023-08-11T22:45:51Z

Thanks @francoisl!

PR will be up tomorrow.

Talha345 · 2023-08-11T22:47:46Z

Ah ok makes sense, thanks for clarifying. Let's go with your proposal then.

@francoisl We have 2 options for the import of the polyfill, either import it in the root App.js so it overrides the default RN implementation completely or just use it where required i.e in the Url.js file.What do you suggest?

Talha345 · 2023-08-12T14:16:55Z

@situchan PR is ready for review

melvin-bot · 2023-08-17T03:59:45Z

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @Talha345 got assigned: 2023-08-11 22:30:34 Z
when the PR got merged: 2023-08-17 03:59:45 UTC
days elapsed: 3

On to the next one 🚀

melvin-bot · 2023-08-18T15:34:37Z

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @Talha345 got assigned: 2023-08-11 22:30:34 Z
when the PR got merged: 2023-08-18 15:34:37 UTC
days elapsed: 4

On to the next one 🚀

Talha345 · 2023-08-30T19:07:53Z

Hi @francoisl ! This PR was merged 12 days ago, any update on the payment?

situchan · 2023-08-30T19:12:10Z

It seems automation didn't work here. PR deployed to production on Aug 24 so payment is due tomorrow

francoisl · 2023-08-31T18:42:22Z

Yeah not sure what happened with the automation here, but it's due for payment now. @adelekennedy can you assist with that please?

adelekennedy · 2023-08-31T18:46:15Z

yikes - thanks for the heads up!

Payouts due:

Issue Reporter: $250 @dukenv0307
Contributor: $1000 @Talha345 (Upwork)
Contributor+: $1000 @situchan

Eligible for 50% #urgency bonus? N

Upwork job is here.

Talha345 · 2023-08-31T18:53:11Z

yikes - thanks for the heads up!

Payouts due:

Issue Reporter: $250 @dukenv0307
Contributor: $1000 @Talha345 (Upwork)
Contributor+: $1000 @situchan

Eligible for 50% #urgency bonus? N

Upwork job is here.

@adelekennedy Payment received! Thanks.

Can you also leave a positive feedback comment on my profile for the Upwork job?

ayazhussain79 · 2023-08-31T20:59:23Z

@adelekennedy @francoisl I reported regression from this bug PR please check this one #24483 (comment)

Talha345 · 2023-08-31T21:03:58Z

@adelekennedy @francoisl I reported regression from this bug PR please check this one #24483 (comment)

@ayazhussain79 it was fixed already!

situchan · 2023-08-31T21:04:57Z

yeah, @ayazhussain79 is eligible for reporting bonus. Thanks to their report, we were able to fix quickly before PR hit staging.

adelekennedy · 2023-08-31T21:19:17Z

Ah thank you! @ayazhussain79 will you apply here? If that link doesn't work I may have set it to invite only

ayazhussain79 · 2023-08-31T21:23:17Z

@adelekennedy Applied on Upwork, Thank you

ayazhussain79 · 2023-08-31T21:28:08Z

Offer accepted, Thank you

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 9, 2023

melvin-bot bot assigned adelekennedy Jul 9, 2023

adelekennedy added the External Added to denote the issue can be worked on by a contributor label Jul 10, 2023

melvin-bot bot changed the title ~~App doesn't open a new tab when clicking on a URL of staging on production~~ [$1000] App doesn't open a new tab when clicking on a URL of staging on production Jul 10, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 10, 2023

melvin-bot bot assigned 0xmiros Jul 10, 2023

melvin-bot bot added the Overdue label Jul 13, 2023

melvin-bot bot added Overdue and removed Overdue labels Jul 14, 2023

melvin-bot bot removed the Overdue label Jul 17, 2023

francoisl assigned Talha345 Aug 11, 2023

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 11, 2023

Talha345 mentioned this issue Aug 12, 2023

Fix Link Navigation relative to current Environment #24483

Merged

59 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 12, 2023

Talha345 mentioned this issue Aug 18, 2023

[22523] Fix path calculation #25479

Merged

59 tasks

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Aug 18, 2023

adelekennedy closed this as completed Aug 31, 2023

situchan mentioned this issue Oct 21, 2024

[$50] Onboarding - Deeplink in concierge chat doesn't navigate to the correct screen within the app #50648

Closed

6 tasks

[$1000] App doesn't open a new tab when clicking on a URL of staging on production #22523

[$1000] App doesn't open a new tab when clicking on a URL of staging on production #22523

Comments

kavimuru commented Jul 9, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Jul 9, 2023

melvin-bot bot commented Jul 9, 2023 • edited by adelekennedy Loading

Bug0 Triage Checklist (Main S/O)

tienifr commented Jul 10, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

Result

adelekennedy commented Jul 10, 2023

melvin-bot bot commented Jul 10, 2023

melvin-bot bot commented Jul 10, 2023

melvin-bot bot commented Jul 10, 2023

Talha345 commented Jul 11, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Result

melvin-bot bot commented Jul 11, 2023

Talha345 commented Jul 11, 2023

melvin-bot bot commented Jul 11, 2023

alitoshmatov commented Jul 11, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

adelekennedy commented Jul 14, 2023

Talha345 commented Jul 17, 2023

adelekennedy commented Jul 17, 2023

Talha345 commented Jul 17, 2023

adelekennedy commented Jul 17, 2023

melvin-bot bot commented Jul 17, 2023

0xmiros commented Jul 17, 2023 • edited Loading

Talha345 commented Aug 11, 2023

francoisl commented Aug 11, 2023

melvin-bot bot commented Aug 11, 2023

Talha345 commented Aug 11, 2023

Talha345 commented Aug 11, 2023

Talha345 commented Aug 12, 2023

melvin-bot bot commented Aug 17, 2023

melvin-bot bot commented Aug 18, 2023

Talha345 commented Aug 30, 2023

situchan commented Aug 30, 2023

francoisl commented Aug 31, 2023

adelekennedy commented Aug 31, 2023

Payouts due:

Talha345 commented Aug 31, 2023

Payouts due:

ayazhussain79 commented Aug 31, 2023

Talha345 commented Aug 31, 2023

situchan commented Aug 31, 2023

adelekennedy commented Aug 31, 2023 • edited Loading

ayazhussain79 commented Aug 31, 2023 • edited Loading

ayazhussain79 commented Aug 31, 2023

kavimuru commented Jul 9, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jul 9, 2023 •

edited by adelekennedy

Loading

tienifr commented Jul 10, 2023 •

edited

Loading

Talha345 commented Jul 11, 2023 •

edited

Loading

0xmiros commented Jul 17, 2023 •

edited

Loading

adelekennedy commented Aug 31, 2023 •

edited

Loading

ayazhussain79 commented Aug 31, 2023 •

edited

Loading