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.56k stars 2.9k forks source link

[$250] [CVP] Account is validated but still being asked to validate to add a VBBA #49813

Closed IuliiaHerets closed 1 week 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: 9.0.39-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: https://expensify.testrail.io/index.php?/tests/view/4992364&group_by=cases:section_id&group_order=asc&group_id=283225 Issue reported by: Applause Internal Team

Action Performed:

  1. Open the app
  2. Log in with a new Expensifail account
  3. Create a workspace
  4. Navigate to Workspace settings - More features
  5. Enable "Workflows"
  6. Navigate to Workflows - Connect bank account
  7. Click on the note to open the process in a browser

Expected Result:

The account is already validated so the message shouldn't appear.

Actual Result:

Web page opened from the app asks to validate my expensifail account.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/322db189-d400-4366-b830-a2d576647a7b

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021841225609645325117
  • Upwork Job ID: 1841225609645325117
  • Last Price Increase: 2024-10-01
  • Automatic offers:
    • suneox | Reviewer | 104246212
melvin-bot[bot] commented 1 month ago

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

IuliiaHerets commented 1 month ago

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

IuliiaHerets commented 1 month ago

@muttmuure FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

melvin-bot[bot] commented 1 month ago

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

trjExpensify commented 1 month ago

I can repro this:

image

@deetergp & I are also running into it in this flow:

        },
        "domainControlled": false,
        "email": "<redacted>",
        "isApprovedAccountant": false,
        "isApprovedAccountantClient": false,
        "isUnapprovedAccountant": false,
        "personalPolicyID": "0BBDE5F6A57A234E",
        "samlRequired": false,
        "samlSupported": false,
        "subscribed": 1,
        "twoFactorAuthEnabled": false,
        "validated": true

but in onyx, account.validated is false

image

This is a critical viral flow, so we'd appreciate a quick turnaround on getting to the bottom of it!

CC: @francoisl as I tagged you for eyes here.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

koko57 commented 1 month ago

Hey! I'm Agata from Callstack and I would like to help with this issue 🙂

melvin-bot[bot] commented 1 month ago

📣 @suneox 🎉 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

trjExpensify commented 1 month ago

Wahoo, thanks!

koko57 commented 1 month ago

I've checked the reproduction steps and I could repro when I wasn't logged in the account - because we're not sending validated in the OpenApp response - shouldn't it be always true after the user logs in to their account with the magic code for the first time (or verify the account the other way)?

Screenshot 2024-10-03 at 21 25 07 Screenshot 2024-10-03 at 21 24 47

So if we're not getting validated - it's undefined and falsy - so we have the error displayed.

I couldn't reproduce the scenario that I was logged in and then validate turned to false, but I wasn't following the steps @trjExpensify mentioned, I will check it tomorrow.

trjExpensify commented 1 month ago

shouldn't it be always true after the user logs in to their account with the magic code for the first time (or verify the account the other way)?

Just so I'm clear, you're talking about this flow yeah?

  1. Sign up for a new account
  2. Get logged-in immediately, but unvalidated.
  3. Sign-out and back in to enter a magic code to validate the account (or otherwise validate the account with a magic code in some in product flow)
  4. Go to the workflows > connect bank account page
  5. Observe being prompted for validation still, when your account is validated at this point.
koko57 commented 1 month ago

@trjExpensify yep, exactly - I tested it on an account that I logged into many times, so I would expect this had already been verified

koko57 commented 1 month ago

@trjExpensify for the flow you were able to recreate this - I've just repro this and validated is false, because that's what we get from SigninUserWithLink. So if we want a new user to be validated when going from the link for the first time it also should be sent from the BE.

Screenshot 2024-10-04 at 16 01 01

But IMHO validated: false makes sense when entering the app from the link for the first time.

koko57 commented 1 month ago

for the case that is in the test steps - we don't send account with SinginWithShortLivedAuthToken or OpenApp, so as I've mentioned earlier we don't have the validated property sent with the account it's treated as falsy and the error is displayed.

As OpenApp call is always sent and we get the account with the response, I think the best option would be to send validated (either true or false) with this one.

trjExpensify commented 1 month ago

I've just repro this and validated is false, because that's what we get from SigninUserWithLink. So if we want a new user to be validated when going from the link for the first time it also should be sent from the BE.

@koko57 for that account in the screenshot have you since validated it or did you not touch it after going through this flow? Because when I look at that account, it's validated (as it should be) on the BE which lines up with the bug I'm seeing where onyx doesn't reflect that:

        "domainControlled": false,
        "email": "agata.kosior+pay2@callstack.com",
        "isApprovedAccountant": false,
        "isApprovedAccountantClient": false,
        "isUnapprovedAccountant": false,
        "personalPolicyID": "7E43D6DD1B1B63EF",
        "samlRequired": false,
        "samlSupported": false,
        "subscribed": 1,
        "twoFactorAuthEnabled": false,
        "validated": true
koko57 commented 1 month ago

@trjExpensify yes, I validated it then. But when I tested it it wasn't validated when I was first redirected from the email.

trjExpensify commented 1 month ago

Cool, can you go through the flow again and don't validate, then send me the email address to check?

koko57 commented 1 month ago

@trjExpensify agata.kosior+testPay@callstack.com

Screenshot 2024-10-07 at 18 09 45
trjExpensify commented 1 month ago

Cool yeah, the account is validated on the backend:

        },
        "canDowngrade": true,
        "closed": "",
        "created": "2024-10-07 11:08:43",
        "delegatedAccess": {
            "delegates": [],
            "delegators": []
        },
        "domainControlled": false,
        "email": "agata.kosior+testpay@callstack.com",
        "isApprovedAccountant": false,
        "isApprovedAccountantClient": false,
        "isUnapprovedAccountant": false,
        "personalPolicyID": "D2FCF31BDA823D00",
        "samlRequired": false,
        "samlSupported": false,
        "subscribed": 1,
        "twoFactorAuthEnabled": false,
        "validated": true
trjExpensify commented 1 month ago

Looking at the code and tracing it back to inception (internal convo ref), It's expected for the account to be validated if you're invited to Expensify and click one of the CTAs in the email.

koko57 commented 1 month ago

@trjExpensify So how it's possible that I have validated: false in Onyx (it was sent with SinginWithShortLivedAuthToken) - so if the value changes after I get the response shouldn't we sent the new value with OpenApp?

trjExpensify commented 1 month ago

@deetergp didn't you have a theory on that?

I bet we set validated: 1 in an onyx update in side of Account::reopen.

trjExpensify commented 1 month ago

CC: @techievivek @NikkiWines for more eyes on this as it's a CVP issue and pretty urgent for us to resolve.

melvin-bot[bot] commented 1 month ago

@suneox, @trjExpensify, @koko57 Eep! 4 days overdue now. Issues have feelings too...

NikkiWines commented 1 month ago

I don't have the bandwidth to handle this at the moment, but perhaps this is tied to this discussion and the validated value for User vs Account

cc: @Beamanator

deetergp commented 1 month ago

Ah, so I think I figured out out. We don't validate in Account::reopen, we do it in SignInUnvalidatedUserForNewDotSession::process but we never send onyx update when we change it. I think we just need to push an update to account key and then we'll have this resolved.

trjExpensify commented 1 month ago

Niceee!

deetergp commented 1 month ago

It turns out we can't send the onyx update from Auth because it's too early in the sign in process for the user to actually receive the update. The change has to be mad in Web, but was relatively easy.

koko57 commented 1 month ago

Is anything on the FE side needed to be done here? 🙂

trjExpensify commented 1 month ago

Nope, I don't think so. Daniel re-tested it here: https://expensify.slack.com/archives/C07HPDRELLD/p1729031899739159?thread_ts=1729024865.063539&cid=C07HPDRELLD

We do have a somewhat related bug, but a proposal is in from Manan here: https://github.com/Expensify/App/issues/49576

trjExpensify commented 4 weeks ago

Cool, closing!

isagoico commented 2 weeks ago

Going to reopen this issue as this is still happening on our side for the Hybrid app CC @AndrewGable

trjExpensify commented 2 weeks ago

Hm, interesting. @koko57 can you give it a re-test and confirm that?

deetergp commented 1 week ago

Any word on the retest @koko57?

koko57 commented 1 week ago

@trjExpensify @deetergp Sorry I missed Tom's message as I was ooo that day. Will retest today

koko57 commented 1 week ago

The original problem is fixed, works ok: https://github.com/user-attachments/assets/ece7e513-ab86-4c17-8ddc-fbf0d71b7068

But I've also checked if the validation error will be shown after creating a new account on the web (and not validating it) and the message is not shown:

Screenshot 2024-11-07 at 09 52 02

I think in this case we want it shown, don't we?

I see that some changes were introduced by this PR https://github.com/Expensify/App/pull/51718. The message should be shown because there is a check for account.validated, but for some reason it doesn't.

I'll be working on refactoring the first step of enabling bank account https://github.com/Expensify/App/issues/50422 https://github.com/Expensify/App/issues/51799 - because there are many issues with this step, so I also can fix it there.

koko57 commented 1 week ago

And I've noticed that we have 2 ways of informing that the workspace currency is not set to USD

for the Expensify Card it looks like this:

Screenshot 2024-11-07 at 09 41 07

and for the workflows:

Screenshot 2024-11-07 at 09 40 45

Is there any reason that for the workflows we have only this info in the RHP and we cannot do this in the modal like for ECard? Or should we make it consistent?

cc @shawnborton

shawnborton commented 1 week ago

I don't think I have strong feelings here, but I always love airing on the side of consistency. cc @Expensify/design @trjExpensify for the quick check here as well.

For the connect bank flow, I assume what's happening is that you click on the connect bank account row and you immediately see the RHP with the message about currency? If it's really the first step in the flow like that, I don't see why it couldn't be a modal either to match the Expensify Card page.

trjExpensify commented 1 week ago

Interestingly, in the global reimbursement doc, we decided not to show the "Connect bank account" row if the workspace currency isn't USD, GBP, EUR, AUD or CAD (supported currencies for global reimbursement):

image

We could pull that out to move forward with it for non-USD for now, and extend it to the other currencies when global reimbursement is implemented? CC: @madmax330 for vis.

trjExpensify commented 1 week ago

But I've also checked if the validation error will be shown after creating a new account on the web (and not validating it) and the message is not shown:

I'm not quite following this. Here's a test on staging web, am I missing something?

https://github.com/user-attachments/assets/d9e1f425-9648-490c-ae18-7783cd50505b

dannymcclain commented 1 week ago

I'm not super passionate but I feel the same as Shawn. If we can make them consistent I think that'd probably be best, and I'd vote for standardizing on the modal approach from the Expensify Card page. I'm happy to defer to what Tom thinks is best though.

koko57 commented 1 week ago

@trjExpensify aaah ok, because of this new feature we no longer have to display this message:

Screenshot 2024-11-08 at 10 31 21

(I meant that this message is not displayed for the accounts that have not been validated)

trjExpensify commented 1 week ago

Yep, exactly!

koko57 commented 1 week ago

@trjExpensify ok, so the original bug is fixed, this issue can be closed 🙂

trjExpensify commented 1 week ago

Great stuff!

We could pull that out to move forward with it for non-USD for now, and extend it to the other currencies when global reimbursement is implemented? CC: @madmax330 for vis.

@madmax330 let us know about the status of this one, and potentially expediting it for non-USD.