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

[CVP] Login -Concierge chat is not created when login in via a link from the expense email #49219

Closed
2 of 6 tasks
IuliiaHerets opened this issue Sep 14, 2024 · 29 comments
Closed
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

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: v9.0.35-0
Reproducible in staging?: Y
Reproducible in production?: N/A, not receiving expense emails on prod
Issue was found when executing this PR: #49130
Email or phone of affected tester (no customers): htad26+yteywys@gmail.com
Issue reported by: Applause Internal Team

Action Performed:

  1. As user A(Gmail account) Submit a manual expense of any amount to someone who doesn’t have an Expensify account(Gmail Account). Call this User B
  2. Navigate to the email of User B and locate the expense email
  3. Login to the new dot app via the "Pay" link in the email

Expected Result:

When the user logs in via the link 'Concierge' chat is created

Actual Result:

When the user logs in via the link 'Concierge' chat is not created

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6603253_1726303070262.2024-09-14_11_30_30.mp4

View all open jobs on GitHub

@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Bug Something is broken. Auto assigns a BugZero manager. labels Sep 14, 2024
Copy link

melvin-bot bot commented Sep 14, 2024

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

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Sep 14, 2024

Triggered auto assignment to @deetergp (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@melvin-bot melvin-bot bot removed the Hourly KSv2 label Sep 14, 2024
@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-collect - Release 1

@IuliiaHerets IuliiaHerets added DeployBlocker Indicates it should block deploying the API and removed Daily KSv2 labels Sep 14, 2024
@mountiny mountiny added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Sep 16, 2024
@mountiny mountiny moved this to CRITICAL in [#whatsnext] #quality Sep 16, 2024
@mountiny
Copy link
Contributor

I have tested this in both staging and production and its same so not a blocker

The concierge chat is present in Onyx but it seems like there is some wrong data/ it is created in a different way, so I think this is a backend issue, but we need to investigate what exactly it is. Because of the wrong data/ format the chat does not show up in the LHN.

Here is staging

Screen.Recording.2024-09-16.at.14.23.57.mp4

And here is production

Screen.Recording.2024-09-16.at.14.28.19.mp4

I put this to newdot quality but its also related to the CVP

@mountiny mountiny changed the title Login -Concierge chat is not created when login in via a link from the expense email [CVP] Login -Concierge chat is not created when login in via a link from the expense email Sep 16, 2024
@mountiny
Copy link
Contributor

Asked in slack here

@kacper-mikolajczak
Copy link
Contributor

Hi, I would like to investigate it!

@kacper-mikolajczak
Copy link
Contributor

kacper-mikolajczak commented Sep 17, 2024

Investigations

After confirming what @mountiny already investigated (thanks for that buddy!) that the Concierge report record is present in the database in the broken flow, let’s check what decides that Concierge is shown.

To understand better what is happening I decided to compare rendering differences of LHN between regular account creation flow (via signing page) with email notification (the broken) one.

In order to show a report, LHN triggers getOrderedReportIDs which fills in reportsToDisplay bucket of report ids.
One of the checks is shouldReportBeInOptionList function and that’s where the distinction between working and broken flows exists.

App/src/libs/ReportUtils.ts

Lines 6059 to 6062 in 2190f62

// Include reports if they are pinned
if (report.isPinned) {
return true;
}

In working flow, Concierge report falls into isPinned check, while in broken flow, the report is missing isPinned property thus opting out of the check. Here's a debugger instance running into isPinned branch:

Screen.Recording.2024-09-17.at.14.33.56.mp4

Here's the direct comparison of database records:

Regular flow Email notification flow
working-isPinned broken-isPinned

Final thoughts

It looks like when accessing the account for the first time via email notification, the Concierge is not set up same way as in regular flow (missing isPinned property and possibly others) which leads to LHN logic skipping it in rendering process.

Let's consult with a backend team if it's something which should be fixed there.

Extra cases

When Concierge report record is removed from local database (indexedDB), it is not revived from the backend on reload as it works with other reports (and with Concierge on regular account).
It may also indicate that something is off server-side with Concierge setup.

Searching for Concierge and starting conversation adds Concierge report correctly, allowing for normal interaction afterwards.

@deetergp
Copy link
Contributor

Nice find, thanks for doing the legwork @kacper-mikolajczak! So in the working case, that is when an organic new user signs in for the first time, we create the Concierge chat and add it to the list of pinned chats in their expensify_chat_pinnedReportIDs NVP on the back end. This happens before they even answer their first onboarding question in the modal. But in the case of invited users, the case where we optimistically create a an account for a non-existent email account that one of our users mentions, adds to a room, etc…, we do not create that chat.

I think the easiest thing for us to do in this situation would be to update CompleteGuidedSetup to have it mark the chat as pinned if it isn't already.

@kacper-mikolajczak
Copy link
Contributor

Great, thanks @deetergp!

Let me know what would need to change client-wise after fixes to CompleteGuidedSetup - I will adjust it 🫡

CC @mountiny

@deetergp
Copy link
Contributor

@kacper-mikolajczak It doesn't look like any client changes will be necessary on the front end, but if they are, I'll be sure to let you know. 👍

@melvin-bot melvin-bot bot added the Overdue label Sep 20, 2024
@alexpensify
Copy link
Contributor

@kacper-mikolajczak let us know if there are any more questions here. Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Sep 20, 2024
@deetergp
Copy link
Contributor

@mountiny I'm bringing this up here since you commented on a draft PR I was working on where I was waiting till onboarding was completed in order to pin the Concierge chat. Looking at Account::createAccount it looks like we intentionally do not allow the creation of the Concierge chat for invited accounts—accounts that are created in the closed state. Is it your opinion that we should change that? Otherwise waiting till they log in and complete onboarding, which is when the Concierge chats are currently being created, to pin them seems like the correct thing to do in this situation.

@mountiny
Copy link
Contributor

@deetergp i see thanks for the explanation. I feel like its odd/ unintuitive to create it in a place like when onboarding is completed. The catch i see is that we are assuming that the user will always complete the onboarding which yes, they should but it relies on fact there wont be any flow/ bug in the frontend where user somehow bypasses completing the onboarding and then they are left with no concierge chat.

What if we create the chat in OpenApp for them? Or the OpenReport, ie the initial interaction with the app?

I think it would be great to get more visibility for this so we are aligned on the best solution, i am not sure if we should rely on the user completing the onboarding modal to create the chat.

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
@alexpensify
Copy link
Contributor

To confirm, should we move this to the Open Source room for a bigger discussion?

@melvin-bot melvin-bot bot removed the Overdue label Sep 24, 2024
@mountiny
Copy link
Contributor

Does not have to be open source, but would be good to discuss in slack @deetergp

@deetergp
Copy link
Contributor

Does not have to be open source, but would be good to discuss in slack @deetergp

@mountiny Yep! I asked here.

@deetergp
Copy link
Contributor

Looks like we've agreed on a plan. I'll start on implementing it later today.

@alexpensify
Copy link
Contributor

Awesome, thanks for the update!

Copy link

melvin-bot bot commented Sep 28, 2024

@deetergp @alexpensify @kacper-mikolajczak this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Sep 30, 2024
Copy link

melvin-bot bot commented Sep 30, 2024

@deetergp, @alexpensify, @kacper-mikolajczak Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@deetergp
Copy link
Contributor

I ran into trouble trying to test this due to my test email being blocked due to changes we made to SES Discussion is here.

@melvin-bot melvin-bot bot removed the Overdue label Sep 30, 2024
@deetergp
Copy link
Contributor

deetergp commented Oct 2, 2024

The Auth PR to create & pin the chats when we re-open invited accounts that were created closed is out in production. I will try to wrap up the web portion for inviting users to the workgroup later today.

@deetergp deetergp added the Reviewing Has a PR in review label Oct 2, 2024
@alexpensify alexpensify added Weekly KSv2 and removed Daily KSv2 labels Oct 7, 2024
@alexpensify
Copy link
Contributor

@deetergp - I see that the Auth update is almost in prod. Any update for the web portion?

@deetergp
Copy link
Contributor

deetergp commented Oct 7, 2024

@alexpensify The automations must have failed to update this GH, but the Web portion of this was deployed to staging a couple hours ago. The auth update is already in prod.

@alexpensify
Copy link
Contributor

Ok, we will wait for the web portion to into prod.

@deetergp
Copy link
Contributor

It's on prod as of a couple days ago ✅

@alexpensify
Copy link
Contributor

Eeek, ok, it looks like payment automation is failing here. I'll keep an eye out next week to see if I need to take manual action.

@alexpensify
Copy link
Contributor

Closing, no payment is needed here since @kacper-mikolajczak is part of Callstack.

@github-project-automation github-project-automation bot moved this from CRITICAL to Done in [#whatsnext] #quality Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Reviewing Has a PR in review Weekly KSv2
Projects
Development

No branches or pull requests

5 participants