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

[READY FOR PAYMENT][$250] Workspace - Blank background displayed when redirected from desktop to connect bank #44929

Closed
2 of 6 tasks
lanitochka17 opened this issue Jul 5, 2024 · 26 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 5, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.4-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: N/A
Email or phone of affected tester (no customers): applausetester+shch1@applause.expensifail.com
Issue reported by: Applause - Internal Team

Issue found when executing PR #44517

Action Performed:

  1. Sign into a valid account in the desktop app
  2. Go to Workspace > Workflow > Connect Bank account
  3. In the connect bank account page click on the link please click here to complete this process in a browser.
  4. Verify that: User expects to be redirected to the Safari browser and logged into the same account that was signed in on Desktop

Expected Result:

Workspace chat should be displayed when redirected from desktop to connect bank

Actual Result:

Blank background displayed when redirected from desktop to connect bank

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6533024_1720092723541.2024-07-04_16-30-55.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010505fec65584f108
  • Upwork Job ID: 1810894967577795738
  • Last Price Increase: 2024-07-17
  • Automatic offers:
    • eh2077 | Reviewer | 103162940
    • dominictb | Contributor | 103162941
Issue OwnerCurrent Issue Owner: @JmillsExpensify
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 5, 2024
Copy link

melvin-bot bot commented Jul 5, 2024

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.

@dominictb
Copy link
Contributor

Proposal

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

Blank background displayed when redirected from desktop to connect bank

What is the root cause of that problem?

After clicking the link from desktop, the link will be open to web. We need to logout the previous user

Navigation.goBack();
navigateToExitUrl(exitUrl);

Now the web is redirect to bank-account/new. -> the routes are bottom and right navigator.

Screenshot 2024-07-08 at 18 26 48

Then the current user is login and setUpPoliciesAndNavigate is called.

App/src/libs/actions/App.ts

Lines 428 to 429 in 83be857

Navigation.goBack();
Navigation.navigate(exitTo);

  1. We call goBack -> RightNavigator will be removed
  2. We now navigate to bank-account/new?policyID= -> RightNavigator is added to routes without central navigator

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

We don't need to call goBack in

App/src/libs/actions/App.ts

Lines 428 to 429 in 83be857

Navigation.goBack();
Navigation.navigate(exitTo);

We added it because We must call goBack() to remove the /transition route from history. But we already have the logic to goBack in logoutPreviousUser so we don't need it anymore.

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Jul 8, 2024
Copy link

melvin-bot bot commented Jul 8, 2024

@JmillsExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@JmillsExpensify
Copy link

I think this is technically #wave-collect since it's the VBBA flow. Opening up to external.

@melvin-bot melvin-bot bot removed the Overdue label Jul 10, 2024
@JmillsExpensify JmillsExpensify added External Added to denote the issue can be worked on by a contributor Overdue labels Jul 10, 2024
@melvin-bot melvin-bot bot changed the title Workspace - Blank background displayed when redirected from desktop to connect bank [$250] Workspace - Blank background displayed when redirected from desktop to connect bank Jul 10, 2024
Copy link

melvin-bot bot commented Jul 10, 2024

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

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

melvin-bot bot commented Jul 10, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jul 10, 2024
@eh2077
Copy link
Contributor

eh2077 commented Jul 10, 2024

@dominictb Thanks for your proposal!

The following condition is true

if (!isLoggingInAsNewUser && exitTo) {

if and only if clicking connect bank from desktop right?
<TextLink onPress={() => Link.openExternalLinkWithToken(bankAccountRoute)}>{translate(plaidDesktopMessage)}</TextLink>

If so, then I think it'll be safe to fix it by removing Navigation.goBack(); here

App/src/libs/actions/App.ts

Lines 428 to 429 in 83be857

Navigation.goBack();
Navigation.navigate(exitTo);

@dominictb
Copy link
Contributor

if and only if clicking connect bank from desktop right?

Yes

If so, then I think it'll be safe to fix it by removing Navigation.goBack(); here

It works by my side. What about you?

@eh2077
Copy link
Contributor

eh2077 commented Jul 11, 2024

It works by my side. What about you?

@dominictb However, it doesn’t work on my end

Screen.Recording.2024-07-11.at.10.32.37.PM.mov

@dominictb
Copy link
Contributor

@eh2077 I see you're using production link, right? Can you pls apply the solution and run dev?

@eh2077
Copy link
Contributor

eh2077 commented Jul 12, 2024

@dominictb I opened the link from dev desktop. How to let it open dev domain on browser?

@eh2077
Copy link
Contributor

eh2077 commented Jul 14, 2024

@dominictb It works for me too if I hard-coded the url to dev domain.

0-desktop.mp4

@dominictb 's proposal looks good to me.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jul 14, 2024

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

@dominictb
Copy link
Contributor

@thienlnam What do you think about my proposal above? Thanks

@melvin-bot melvin-bot bot added the Overdue label Jul 17, 2024
@eh2077
Copy link
Contributor

eh2077 commented Jul 17, 2024

Not overdue waiting for @thienlnam 's review

@melvin-bot melvin-bot bot removed the Overdue label Jul 17, 2024
Copy link

melvin-bot bot commented Jul 17, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 17, 2024
Copy link

melvin-bot bot commented Jul 17, 2024

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

Offer link
Upwork job

Copy link

melvin-bot bot commented Jul 17, 2024

📣 @dominictb 🎉 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 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jul 18, 2024
@dominictb
Copy link
Contributor

This should be ready for payment, no @JmillsExpensify @thienlnam?

@thienlnam
Copy link
Contributor

Yeah, looks like the automation did not work here

@thienlnam thienlnam added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Aug 1, 2024
@thienlnam thienlnam changed the title [$250] Workspace - Blank background displayed when redirected from desktop to connect bank [READY FOR PAYMENT][$250] Workspace - Blank background displayed when redirected from desktop to connect bank Aug 1, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

@JmillsExpensify @thienlnam @eh2077 @dominictb this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@thienlnam
Copy link
Contributor

@eh2077 Can you complete the regression test here so we can close this out?

@JmillsExpensify
Copy link

Payment summary:

@melvin-bot melvin-bot bot removed the Overdue label Aug 7, 2024
@JmillsExpensify
Copy link

Contributor paid via Upwork. @eh2077 mind completing the checklist so that I can process your payment as well?

@eh2077
Copy link
Contributor

eh2077 commented Aug 7, 2024

Checklist

  • [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I can't find the PR caused this bug. Though this bug was found during test PR fix: VBA Link does not sign user in when redirected #44517, but the root cause has been there for quite a while.
  • [@eh2077] 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: N/A
  • [@eh2077] 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: N/A
  • [@eh2077] Determine if we should create a regression test for this bug. Yes, it's related to bank setting though this is an edge case.

Regression test

  1. Sign into a valid account in the desktop app
  2. Go to Workspace > Workflow > Connect Bank account
  3. In the connect bank account page click on the link please click here to complete this process in a browser.
  4. At this point, user expects to be redirected to the Safari browser and logged into the same account that was signed in on Desktop
  5. Verify that: Workspace chat should be displayed when redirected from desktop to connect bank, instead of a blank background page

@JmillsExpensify
Copy link

Contributor paid and regression test created. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
Status: Done
Development

No branches or pull requests

5 participants