[HOLD for payment 2023-03-31] [$1000] https://new.expensify.com/concierge doesn't redirect to Concierge chat after sign-up/sign-in

[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!

---

Coming from this thread.

Action Performed:

1. Ensure you're not logged in
2. Open https://new.expensify.com/concierge
3. Sign-up or sign-in

Expected Result:

The user should be redirected to the Concierge chat

Actual Result:

The user is redirected to the last opened chat

Workaround:

Open Concierge manually or open https://new.expensify.com/concierge if the user is logged in

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: v1.2.80-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

Screen.Recording.2023-03-09.at.11.53.28.mov

Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01693e7548c561524b
• Upwork Job ID: 1634326085596753920
• Last Price Increase: 2023-03-10

[MelvinBot]

Triggered auto assignment to @mallenexpensify (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

[mallenexpensify]

Checking to see if this should be put on hold pending Nav reboot

• [Main Tracking] App Navigation Reboot #11768 (comment)

[marcaaron]

From a quick glance - I'd guess it has more to do with some API issue than a navigation issue. Though, we need to understand the root cause in order to say for sure whether it is or isn't related.

[MelvinBot]

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

[mallenexpensify]

Thanks @marcaaron , @marcochavezf , I added the engineering label, as you're already assigned, it's back to you. Not sure if it should be internal or external

[mountiny]

I agree with Marc, I think this is not strictly an issue of the navigation, I will give this on the Callstack list for investigation

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[mountiny]

@marcochavezf btw asked here and they might get back with someone to look at this on Monday, thanks

[tienifr]

Proposal

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

The user is redirected to the last opened chat rather than the Concierge chat when go to /concierge route when unauthenticated, then login.

What is the root cause of that problem?

The root cause is that we're having the ConciergePage in the PublicScreens here

App/src/libs/Navigation/AppNavigator/PublicScreens.js

Line 36 in 2f72ad0

name="Concierge"

. I believe the intention was to have the logic of re-route to the login page if the user is unauthenticated inside the ConciergePage, as mentioned in this comment

App/src/pages/ConciergePage.js

Line 25 in 2f72ad0

* - Else re-route to the login page

.

When we go to the /concierge route, it's a public route so we don't have the logic of saving the route and navigate to it after logging in, thus this issue occurs.

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

We shouldn't have the ConciergePage in the PublicScreens since we always need to be logged in to chat to Concierge, so it should be in the AuthScreens only.

We need to remove the Concierge screen from the PublicScreens here

App/src/libs/Navigation/AppNavigator/PublicScreens.js

Line 36 in 2f72ad0

name="Concierge"

, and optionally remove the redirect to login logic inside ConciergePage as well since it's not necessary now because that logic is built in when we navigate to any screen in the AuthScreens .

After this change, it will redirect to /concierge correctly after logging in.

What alternative solutions did you explore?

NA

Result

Working well after the fix:

Screen.Recording.2023-03-11.at.15.04.46.mov

[marcochavezf]

Ah @mountiny @mallenexpensify I already created a PR to fix this #15788, the problem is that we're not handling the concierge route after the app is launched

[tienifr]

Regression Test Proposal

Bug: https://new.expensify.com/concierge doesn't redirect to Concierge chat after sign-up/sign-in

Proposed Test Steps:

1. Sign-in
2. Click on a chat not Concierge chat (to ensure your last opened chat is not Concierge)
3. Logout
4. Open https://new.expensify.com/concierge
5. Sign-in the same account in step 1
6. Verify that you will be redirected to the Concierge chat (not the last opened chat)

To navigate to https://new.expensify.com/concierge on dev/staging/production, please read the instruction in the PR:

Do we 👍 or 👎

[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.87-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix/15785 https://new.expensify.com/concierge doesn't redirect to Concierge chat after sign-up/sign-in #15974
• Fix navigation to Concierge chat after sign-up/sign-in #15788

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

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:

• [@mollfpr / @marcochavezf] The PR that introduced the bug has been identified. Link to the PR:
• [@mollfpr / @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:
• [@mollfpr / @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:
• [@mallenexpensify] Determine if we should create a regression test for the bug.
• [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to the appropriate location to ensure the same bug will not reach production again.
• [@mallenexpensify] Review the proposed regression test steps and location.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MelvinBot]

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

• Fix navigation to Concierge chat after sign-up/sign-in #15788

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

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:

• [@mollfpr / @marcochavezf] The PR that introduced the bug has been identified. Link to the PR: This was a uncovered edge case, so there's no PR that introduced the bug.
• [@mollfpr / @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: N/A
• [@mollfpr / @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: I think just adding QA tests to TR will be enough. It won't be necessary to add a new item to the checklist.
• [@mallenexpensify] Determine if we should create a regression test for this bug.
• [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/272879

[marcochavezf]

The regression test steps are the same as the proposed here

[mallenexpensify]

This issue's a lil tricky but.. it appears
@tienifr assigned on March 15th.
Merged on March 16th.

From what I can tell, the other linked issue and PR don't affect payment here, please comment if that's not accurate. So both @tienifr and @mollfpr should be due $1500 which includes the $500 timeliness bonus.

Gonna leave this open for a couple days before I pay to be sure the above is accurate. Everyone's already hired for the job here

[MelvinBot]

@marcochavezf, @mallenexpensify, @mollfpr, @tienifr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[marcochavezf]

For this one, the proposed solution was merged, but there was an uncovered edge case that was fixed in another PR. The steps to catch that case were a bit tricky, so I agree we can consider it as a different issue and don't affect payment here.

[mallenexpensify]

Thanks @marcochavezf , @tienifr and @mollfpr have been $1500 each, closing