[HOLD for payment 2023-03-13] [$1000] Deep link for user profile crashes on desktop

[marcochavezf]

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. On web navigate to a user profile
2. Refresh the browser
3. Click on Open Electron to open the user profile

Expected Result:

Open the user profile page in the Desktop app

Actual Result:

Desktop app crashes

Workaround:

Open the user profile on web by clicking on open this link in your browser.

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.69-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
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation: https://expensify.slack.com/archives/C03KN50J0PM/p1676058166922359?thread_ts=1676038239.279419&cid=C03KN50J0PM

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d2a5e261c3af7e82
• Upwork Job ID: 1625199243747147776
• Last Price Increase: 2023-02-17

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @parasharrajat (Internal)

[slafortune]

Based on the conversation in Slack, seems that this will be an internal fix!

[slafortune]

@marcochavezf you are planning on fixing this, yes?

[marcochavezf]

Yeap, this is needed for the ECX. Haven't been able to work on this one yet. I'm going to change it to external in case someone founds a solution during the weekend.

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[allroundexperts]

Proposal

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

User profile deep link opened in desktop app from the web app causes it to crash.

What is the root cause of that problem?

The deep link does not forward query parameters along to the desktop app. Even if it does (it doesn't right now though), the Desktop app is not reading the search params. It reads path of URL only. The DetailsPage component expects route params present. Without it, there is a render error. This causes the crash.

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

We can simply pass on the query parameters to the desktop app from browser. We also have to make sure that the desktop app reads the query parameters passed as well.

We need to change the following in desktop/main.js file, we need to change the following:

deeplinkUrl = `${APP_DOMAIN}${urlObject.pathname}`;

to

deeplinkUrl = `${APP_DOMAIN}${urlObject.pathname}${urlObject.search}`;

In src/components/DeeplinkWrapper/index.website.js, we need to change the following:

const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}${window.location.pathname}`;

to

const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}${window.location.pathname}${window.location.search}`;

Note: Although not required in this case, but we can also add window.location.hash to the above.

What alternative solutions did you explore? (Optional)

[tienifr]

Proposal

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

Opening deep link in Desktop app will crash the app in DetailsPage.

What is the root cause of that problem?

In DetailsPage expects the login URL param to be present, you can see as in

App/src/pages/DetailsPage.js

Line 88 in 8cc88de

const login = lodashGet(this.props.route.params, 'login');

When we deeplink into the Desktop app, we're passing only the pathname to the deep link

App/src/components/DeeplinkWrapper/index.website.js

Line 70 in 8e650d7

const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}${window.location.pathname}`;

We're ignoring other important parts of an URL like query param, hash.

When the DetailsPage do not see the login query param in the URL, it crashes.

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

In order to fix this, we need to pass all parts of the URL to the deeplink, including the query params and the hash, so that the Desktop page has enough information to render the page. The hash here doesn't cause issues on this page but it stills need to be added to make a complete URL, otherwise if any page has the hash it will not work as expected.
In

App/src/components/DeeplinkWrapper/index.website.js

Line 70 in 8e650d7

const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}${window.location.pathname}`;

We need to change to

const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}${window.location.pathname}${window.location.search}${window.location.hash}`;

(Note the window.location.search and window.location.hash are added at the end)

And in

App/desktop/main.js

Line 206 in 90a4e1b

deeplinkUrl = `${APP_DOMAIN}${urlObject.pathname}`;

We need to change to

deeplinkUrl = `${APP_DOMAIN}${urlObject.pathname}${urlObject.search}${urlObject.hash}`;

What alternative solutions did you explore? (Optional)

NA

[mallenexpensify]

@parasharrajat , it appears the PR was ready for review 5 days ago, is there a reason it hasn't been reviewed yet?
#15262 (comment)

[parasharrajat]

Yes, because the Contributor did not follow the process and created the PR before the issue was assigned to him. #15059 (comment)

[mallenexpensify]

ah... thanks @parasharrajat . @allroundexperts you've been working on jobs for ~8 months, I'd expect you to be familiar with all aspects of our CONTRIBUTING.md and processes by now.

For this specific issue, I know the title is specific to Desktop, have we also tested on all other platforms to ensure they're working properly? Asking cuz I (think I) ran into an issue yesterday on iOS safari. iirc. there was discrepancy between me being signed in or not. Please check/test those instances (and, if needed, we can create a separate GH)

[allroundexperts]

ah... thanks @parasharrajat . @allroundexperts you've been working on jobs for ~8 months, I'd expect you to be familiar with all aspects of our CONTRIBUTING.md and processes by now.

For this specific issue, I know the title is specific to Desktop, have we also tested on all other platforms to ensure they're working properly? Asking cuz I (think I) ran into an issue yesterday on iOS safari. iirc. there was discrepancy between me being signed in or not. Please check/test those instances (and, if needed, we can create a separate GH)

@mallenexpensify I actually took a long break in between. But you're right, I should have been more careful.

[mallenexpensify]

👍 @allroundexperts . I didn't mean to sound harsh but also didn't come up with an easy/good way to soften the language.

[mallenexpensify]

PR merged yesterday 🎉
#15262

[MelvinBot]

@marcochavezf, @slafortune, @parasharrajat, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

[marcochavezf]

Not overdue, PR deployed to staging yesterday.

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.78-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Pass down url parameters when the native app is opened from browser #15262

If no regressions arise, payment will be issued on 2023-03-13. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

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:

• [@parasharrajat / @marcochavezf] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat / @marcochavezf] 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:
• [@parasharrajat / @marcochavezf] 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:
• [@allroundexperts] Propose regression test steps to ensure the same bug will not reach production again.
• [@slafortune] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:https://github.com/Expensify/Expensify/issues/271666

[parasharrajat]

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:

• [@parasharrajat / @marcochavezf] The PR that introduced the bug has been identified. Link to the PR: Desktop deeplink #13606
• [@parasharrajat / @marcochavezf] 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: Desktop deeplink #13606 (comment)
• [@parasharrajat / @marcochavezf] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1678208332159099

[allroundexperts]

Regression Test Proposal

1. Go to https://staging.new.expensify.com/ and open any chat.
2. Click on the avatar of any chat participant and make sure that the details page opens.
3. Refresh the page.
4. Click open desktop app on the popup that appears.
5. Verify that the app opens correctly on desktop with the correct user details page and without any crashes.
   Do we agree 👍 or 👎

[MelvinBot]

@marcochavezf, @slafortune, @parasharrajat, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

[slafortune]

@parasharrajat @allroundexperts I sent offers to you both - let me know once you accept those!

[allroundexperts]

@parasharrajat @allroundexperts I sent offers to you both - let me know once you accept those!

Thanks for the offer @slafortune. The amount that shows there seems to be incorrect (500). Can you please double check?

[slafortune]

Ah! Thanks for calling that out! I was using the incorrect dates!

[parasharrajat]

@slafortune Can you please resend the offer? It seems that I accidentally declined it.

[slafortune]

@parasharrajat 👍 resent - https://www.upwork.com/nx/wm/pre-hire/c/8577561/offer/23883082