App icon indicating copy to clipboard operation
App copied to clipboard

[HOLD for payment 2024-12-17] [$250] No popup to open desktop app for 'Try New Expensify' link

Open IuliiaHerets opened this issue 1 year ago β€’ 19 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.60-2 Reproducible in staging?: Y Reproducible in production?: Y Issue was found when executing this PR: https://github.com/Expensify/App/pull/52255 Email or phone of affected tester (no customers): applausetester+bp2410d@app;ause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Precondition: user is logged in OD.

  1. OD: Go to Settings > Account
  2. Click on Credit Card Import > Import Card/Bank
  3. Click on Privacy at the bottom
  4. Go back to OD
  5. Click on 'Try New Expensify'

Expected Result:

User is redirected to NewDot application. Popup with suggestion to open desktop app appears.

Actual Result:

User is redirected to NewDot application. No popup with suggestion to open desktop app.

Workaround:

Unknown

Platforms:

  • [ ] Android: Standalone
  • [ ] Android: HybridApp
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Standalone
  • [ ] iOS: HybridApp
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/7735fcf3-75af-4a95-92de-2760f1dfa248

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021859311638145427434
  • Upwork Job ID: 1859311638145427434
  • Last Price Increase: 2024-11-27
Issue OwnerCurrent Issue Owner: @JmillsExpensify

IuliiaHerets avatar Nov 13 '24 09:11 IuliiaHerets

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Nov 13 '24 09:11 melvin-bot[bot]

Proposal

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

There is no prompt to open in desktop app.

What is the root cause of that problem?

When we open ND from OD, the link will have a transition path. If it's a transition URL, the prompt will be pending by App.beginDeepLinkRedirectAfterTransition(). https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/components/DeeplinkWrapper/index.website.tsx#L19-L31

beginDeepLinkRedirectAfterTransition will only resolved after signOnTransitionToFinishPromise promise is resolved. https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/libs/actions/App.ts#L501-L503

It will be resolved in setUpPoliciesAndNavigate. https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/libs/actions/App.ts#L441-L447

setUpPoliciesAndNavigate is called when the AuthScreen is mounted. https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L303

But as seen from the code above, the promise is only resolved if exitTo param is not empty. Currently, the exitTo param from OD is empty. exitTo=. So, the promise is never resolved.

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

Fix the OD to provide the exitTo

OR

I think we should set a fallback to the exitTo when calling navigate. If it's empty, we use HOME.

https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/libs/actions/App.ts#L425

Then remove the check for exitTo. https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/libs/actions/App.ts#L441 https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/libs/actions/App.ts#L419-L421

Also, when the exitTo is empty, it will show infinite loading

image

It's because when we navigate to HOME, it won't go anywhere because we are already at HOME, but the transition page is still at the nav stack. https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/libs/actions/App.ts#L441-L446

So, we need to call Navigation.goBack too, but only if exitTo is empty because when exitTo is available, we already popped it out from LogOutPreviousUserPage. (basically we have 2 places of handling transition)

https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/pages/LogOutPreviousUserPage.tsx#L77-L87

if (!isLoggingInAsNewUser) {
    Navigation.waitForProtectedRoutes()
        .then(() => {
            if (isTransitioning && !exitTo) {
                Navigation.goBack();
            }
            Navigation.navigate(exitTo);
        })
        .then(endSignOnTransition);
}

OR

We can immediately call endSignOnTransition when exitTo isn't available.

https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/libs/actions/App.ts#L419-L421 https://github.com/Expensify/App/blob/67d7299079a14ee9dc22c17a5174209b70e9e61c/src/libs/actions/App.ts#L441-L447

if (!session || !currentUrl?.includes('exitTo')) {
    endSignOnTransition();
    return;
}
if (!isLoggingInAsNewUser && exitTo) {
    Navigation.waitForProtectedRoutes()
        .then(() => {
            Navigation.navigate(exitTo);
        })
        .then(endSignOnTransition);
} else {
    endSignOnTransition();
}

bernhardoj avatar Nov 13 '24 13:11 bernhardoj

@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Nov 18 '24 09:11 melvin-bot[bot]

@JmillsExpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] avatar Nov 20 '24 09:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 20 '24 19:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 20 '24 19:11 melvin-bot[bot]

Opened up to the community since we have a proposal

JmillsExpensify avatar Nov 20 '24 19:11 JmillsExpensify

@JmillsExpensify, @Pujan92 Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Nov 26 '24 09:11 melvin-bot[bot]

@Pujan92 proposal above when you have a moment.

JmillsExpensify avatar Nov 26 '24 21:11 JmillsExpensify

@JmillsExpensify @Pujan92 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Nov 27 '24 09:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 27 '24 16:11 melvin-bot[bot]

I think the last option from @bernhardoj's proposal of calling endSignOnTransition when exitTo isn't available looks good to me.

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

Pujan92 avatar Nov 28 '24 13:11 Pujan92

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

melvin-bot[bot] avatar Nov 28 '24 13:11 melvin-bot[bot]

Let's go with @bernhardoj's proposal, by calling endSignOnTransition when exitTo isn't available.

lakchote avatar Nov 28 '24 16:11 lakchote

Will open the PR tomorrow

bernhardoj avatar Nov 29 '24 15:11 bernhardoj

PR is ready

cc: @Pujan92

bernhardoj avatar Nov 30 '24 08:11 bernhardoj

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

melvin-bot[bot] avatar Dec 10 '24 22:12 melvin-bot[bot]

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

  • https://github.com/Expensify/App/pull/53331

If no regressions arise, payment will be issued on 2024-12-17. :confetti_ball:

For reference, here are some details about the assignees on this issue:

  • @Pujan92 requires payment through NewDot Manual Requests
  • @bernhardoj requires payment through NewDot Manual Requests

melvin-bot[bot] avatar Dec 10 '24 22:12 melvin-bot[bot]

@Pujan92 @JmillsExpensify @Pujan92 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot[bot] avatar Dec 10 '24 22:12 melvin-bot[bot]

Payment Summary

Upwork Job

  • Reviewer: @Pujan92 owed $250 via NewDot
  • Reviewer: @bernhardoj owed $250 via NewDot

BugZero Checklist (@JmillsExpensify)

  • [ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • [ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1859311638145427434/hired)
  • [ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • [ ] I have verified the payment summary above is correct

melvin-bot[bot] avatar Dec 17 '24 09:12 melvin-bot[bot]

BugZero Checklist:

  • [X] [Contributor] Classify the bug:
Bug classification

Source of bug:

  • [ ] 1a. Result of the original design (eg. a case wasn't considered)
  • [X] 1b. Mistake during implementation
  • [ ] 1c. Backend bug
  • [ ] 1z. Other:

Where bug was reported:

  • [ ] 2a. Reported on production
  • [ ] 2b. Reported on staging (deploy blocker)
  • [X] 2c. Reported on both staging and production
  • [ ] 2d. Reported on a PR
  • [ ] 2z. Other:

Who reported the bug:

  • [ ] 3a. Expensify user
  • [ ] 3b. Expensify employee
  • [ ] 3c. Contributor
  • [X] 3d. QA
  • [ ] 3z. Other:
  • [X] [Contributor] 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: https://github.com/Expensify/App/pull/17452/files#r1889958483

  • [X] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: N/A

  • [X] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [ ] [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

  1. In OD, press on 'Try New Expensify'
  2. Verify when the NewDot opens, there is no infinite loading and there is a popup to open on a desktop

Do we agree πŸ‘ or πŸ‘Ž

Pujan92 avatar Dec 18 '24 10:12 Pujan92

@JmillsExpensify, @Pujan92, @lakchote, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Dec 20 '24 09:12 melvin-bot[bot]

Payment summary:

  • Contributor: $250 @bernhardoj via New Expensify
  • Reviewer: @Pujan92 $250 via New Expensify

JmillsExpensify avatar Dec 20 '24 20:12 JmillsExpensify

All contributors paid via New Expensify and the payment summary is above, so I'm closing this one.

JmillsExpensify avatar Dec 20 '24 20:12 JmillsExpensify

Requested in ND.

bernhardoj avatar Dec 21 '24 02:12 bernhardoj

Payment summary:

  • Contributor: $250 @bernhardoj via New Expensify
  • Reviewer: @Pujan92 $250 via New Expensify

garrettmknight avatar Dec 23 '24 15:12 garrettmknight

$250 approved for @Pujan92

garrettmknight avatar Dec 30 '24 16:12 garrettmknight

$250 approved for @bernhardoj

JmillsExpensify avatar Jan 08 '25 17:01 JmillsExpensify