Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.54k stars 2.89k forks source link

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

Closed IuliiaHerets closed 3 weeks ago

IuliiaHerets commented 1 month ago

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: https://github.com/Expensify/App/pull/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:

Screenshots/Videos

https://github.com/user-attachments/assets/3f40a264-4b10-4f56-8ae9-ae70dad11485

View all open jobs on GitHub

melvin-bot[bot] commented 1 month ago

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.

github-actions[bot] commented 1 month ago

:wave: 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.
melvin-bot[bot] commented 1 month ago

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

IuliiaHerets commented 1 month ago

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

mountiny commented 1 month ago

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

https://github.com/user-attachments/assets/27cd3494-a321-4003-b25d-81547a91bba9

And here is production

https://github.com/user-attachments/assets/0ccdb4d2-26c7-4087-ac66-cb723818ca22

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

mountiny commented 1 month ago

Asked in slack here

kacper-mikolajczak commented 1 month ago

Hi, I would like to investigate it!

kacper-mikolajczak commented 1 month ago

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.

https://github.com/Expensify/App/blob/2190f6279041ed8260752294d095bbdda76faebe/src/libs/ReportUtils.ts#L6059-L6062

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:

https://github.com/user-attachments/assets/ed568869-0c0c-4a54-9428-09def0695c9c

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 commented 1 month ago

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 commented 1 month ago

Great, thanks @deetergp!

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

CC @mountiny

deetergp commented 1 month ago

@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. 👍

alexpensify commented 1 month ago

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

deetergp commented 1 month ago

@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 commented 1 month ago

@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.

alexpensify commented 1 month ago

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

mountiny commented 1 month ago

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

deetergp commented 1 month ago

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

@mountiny Yep! I asked here.

deetergp commented 1 month ago

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

alexpensify commented 1 month ago

Awesome, thanks for the update!

melvin-bot[bot] commented 1 month ago

@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[bot] commented 1 month ago

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

deetergp commented 1 month ago

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.

deetergp commented 1 month ago

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.

alexpensify commented 1 month ago

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

deetergp commented 1 month ago

@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 commented 1 month ago

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

deetergp commented 1 month ago

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

alexpensify commented 1 month ago

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 commented 3 weeks ago

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