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-02-08] [$2000] Deep link routing is inconsistent when the app is open #13691

Closed
kavimuru opened this issue Dec 19, 2022 · 74 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Dec 19, 2022

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


1) Magic sign in Success

  • Create a new account
  • Kill the app
  • Tap the magic sign in deep link
  • After setting a password, you will be taken to the LHN, with the FAB modal opened ✅

2) Magic sign in Failure

  • Create a new account
  • Background, but do not kill the app
  • Tap the magic sign in deep -ink
  • After setting a password, you should be taken to the LHN, but the FAB will not be shown! ❌

3) Admin chat failure

  • Sign in to an account without an open workspace
  • Tap the fab button, and create a workspace
  • Await the Notification from the support team member
  • Do not kill the app
  • Tap this notification
  • You might be taken to the LHN (known, separate issue)
  • Swipe right to the chat
  • It should be the #admins chat, but is often an entirely different chat ❌

Solution

Figure out why the deep-links aren't working as expected while the app is open.

Actual Result :

on signin it navigates to chat screen with concierge

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Android

Version Number: 1.2.41-1
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Screen_Recording_20221217_122455_New.Expensify.mp4
screen-20221219-134725.mp4

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1671280997504769

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0106e69a3f4a655640
  • Upwork Job ID: 1612854400942157824
  • Last Price Increase: 2023-01-25
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 19, 2022
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Dec 19, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jan 4, 2023
@JmillsExpensify JmillsExpensify changed the title shows chat with concierge screen for new user On sign-in, chat navigates to concierge instead of LHN Jan 9, 2023
@JmillsExpensify JmillsExpensify added Daily KSv2 and removed Weekly KSv2 labels Jan 9, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 9, 2023
@dylanexpensify
Copy link
Contributor

On it

@melvin-bot melvin-bot bot removed the Overdue label Jan 10, 2023
@dylanexpensify
Copy link
Contributor

Started convo re: repro steps

@Julesssss Julesssss self-assigned this Jan 10, 2023
@Julesssss
Copy link
Contributor

Commented here. I believe this isn't worth fixing and will cease to be a problem after we rework our app navigation.

@dylanexpensify
Copy link
Contributor

Closing as dupe for 9179

@Julesssss
Copy link
Contributor

Based on our Slack discussion, that might be premature. I'm going to see if I can provide any evidence for my belief that it'll be resolved by our navigation changes.

@Julesssss Julesssss reopened this Jan 10, 2023
@Julesssss
Copy link
Contributor

After a lot of testing, I believe this is a good overview of deep linking:

Magic Sign in deep link

  • If you kill the app before tapping the magic link, the FAB will always be shown ✅
  • If you don’t kill the app before tapping the magic link, the FAB will never be shown ❌

Workspace message deep link

  • If you kill the app before tapping the link, you will always be taken to the LHN, but technically the #admins chat is open. Swiping right will show it ✅ :
  • If you don’t kill the app before tapping the link, there is no guarantee what will happen. Most of the time the behavior is the same as above, but sometimes you’ll be taken to the LHN, but with the Concierge chat shown when swiping right ❌ ❓

So, I believe there are two main problems here:

A) Because of this known issue, the user is taken to the LHN instead of the specific deep link chat. This is because of our weird navigation hierarchy, where both the LHN and Chat page are at the same level (mirroring web/Desktop). This will cease to be a problem once we implement the navigation improvements because LHN and the Chat pages will be completely separated
B) In addition, deep links do not always work correctly when the app is already open. This should be investigated further.

@Julesssss Julesssss added Improvement Item broken or needs improvement. Engineering Internal Requires API changes or must be handled by Expensify staff labels Jan 10, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 10, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jan 10, 2023

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

@trjExpensify
Copy link
Contributor

Nice, thanks Jules. So it's this we focus on here right?

B) In addition, deep links do not always work correctly when the app is already open. This should be investigated further.

@JmillsExpensify
Copy link

Yikes, that one seems like quite the project, right? I'd want to make sure that we also consider whether there are broader improvements, and potentially even a refactor of sorts for consideration.

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Jan 26, 2023
@tienifr
Copy link
Contributor

tienifr commented Jan 26, 2023

@rushatgabhane @Julesssss @mountiny the PR is ready for review #14588
thanks!

@Julesssss
Copy link
Contributor

Thanks for moving this forward all, I'm back and will test it today.

@situchan
Copy link
Contributor

situchan commented Jan 31, 2023

PR which fixes this issue caused regression of #11102

regression

@Julesssss
Copy link
Contributor

Julesssss commented Feb 1, 2023

Yeah, I don't think it's a bad regression that would reduce payment, but I'm taking care of it here.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Feb 1, 2023
@melvin-bot melvin-bot bot changed the title [$2000] Deep link routing is inconsistent when the app is open [HOLD for payment 2023-02-08] [$2000] Deep link routing is inconsistent when the app is open Feb 1, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Feb 1, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Feb 1, 2023

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

If no regressions arise, payment will be issued on 2023-02-08. 🎊

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

@melvin-bot
Copy link

melvin-bot bot commented Feb 1, 2023

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:

@Julesssss
Copy link
Contributor

I wouldn't count this as a regression but is an overlooked case from the initial implementation (a very long time ago). So I checked off the relevant boxes. A regression test would be useful though.

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Feb 2, 2023
@tienifr
Copy link
Contributor

tienifr commented Feb 4, 2023

@rushatgabhane @Julesssss @mountiny

Regression Test Proposal

Bug: Deep link routing is inconsistent when the app is open

Proposed Test Steps:

  • Open the app
  • Login with an existing account that already has workspace / Login to an existing account then create a workspace
  • Logout of the account
  • Register with a new email
  • Go back to home screen
  • Perform 1 of the following
    • Keep the app in background
    • Kill the app in background
  • Go to mailbox and click the magic sign-in link to access the set password page. Set a password and proceed
  • Verify that the FAB modal is opened after signing in
  • Select New workspace to create a new workspace
  • Wait for the Notification from the support team member
  • Perform 1 of the following
    • Keep the app in background
    • Kill the app in background
  • Tap the notification
  • Verify that the app navigates to the correct channel #admins

Do we 👍 or 👎

@Julesssss
Copy link
Contributor

Yep that looks good, thanks. @dylanexpensify, all yours to check off

@dylanexpensify
Copy link
Contributor

Looks great! Nice one @tienifr !

@dylanexpensify
Copy link
Contributor

Added GH for test! Will begin payments tomorrow!

@dylanexpensify
Copy link
Contributor

@tienifr sent offer! @rushatgabhane invited to apply to job!

@gadhiyamanan
Copy link
Contributor

@dylanexpensify please send an offer for reporting bonus

@dylanexpensify
Copy link
Contributor

@gadhiyamanan sent!
@rushatgabhane offer sent!

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 8, 2023
@dylanexpensify
Copy link
Contributor

@rushatgabhane can you accept offer when you get the chance?
@gadhiyamanan sent offer for reporting!

@gadhiyamanan
Copy link
Contributor

@dylanexpensify Accepted

@rushatgabhane
Copy link
Member

@dylanexpensify accepted, thanks!

@dylanexpensify
Copy link
Contributor

All paid out, thanks all!

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. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants