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
2.99k stars 2.5k forks source link

[$500] BA - Unable to confirm on double check Personal info page, error appears #38201

Open kavimuru opened 2 months ago

kavimuru commented 2 months 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: 1.4.51-1 Reproducible in staging?: y Reproducible in production?: no If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4418483 Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: applause internal team Slack conversation:

Action Performed:

Pre-requisite: the user must be logged in and have created a Workspace.

  1. Go to Workspace > Bank account.
  2. Select Connect manually.
  3. Enter "011401533" and "1111222233331111" for Routing number and Account number.
  4. On Personal info, use "First" and "Last" for first name and last name.
  5. Enter random data on the rest of the fields.
  6. On double check Personal info, try to confirm.

    Expected Result:

    The user should be able to confirm on double check Personal info page and proceed to Onfido flow.

Actual Result:

The user is not able to confirm on double check Personal info page.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/43996225/a8c34176-3694-4853-bc83-5e322cd830b2

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f8390f05597b3838
  • Upwork Job ID: 1770107180384727040
  • Last Price Increase: 2024-03-19
  • Automatic offers:
    • s77rt | Reviewer | 0
    • tienifr | Contributor | 0
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

github-actions[bot] commented 2 months 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 2 months ago

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

kavimuru commented 2 months ago

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

shubham1206agra commented 2 months ago

Unable to repro this.

mountiny commented 2 months ago

@kavimuru can you share the console/ networks tab to see what was the error of the API call?

From the logs, it seems like we were not able to verify the name and date of birth and hence it failed Please verify your name and date of birth. If the information is correct

Is this supposed to work for other than the Alberta Bobbeth Charleson names?

mountiny commented 2 months ago

I think this can be demoted, but wont do so until its the last blocker

cristipaval commented 2 months ago

I could reproduce this, and I have similar logs It's the error stated by @mountiny above. I think we should show a better error message, but I wouldn't block the deploy on this.

mountiny commented 2 months ago

Confirmed with @MariaHCD this is expected given if we use different name than Alberta Charleson the checks are being done so in this case it fails.

I agree with Cristi that better error should be shown, not sure if you want to handle that here, I assume we can add this to Wave collect as polish

melvin-bot[bot] commented 2 months ago

@abekkala, @cristipaval Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

GandalfGwaihir commented 2 months ago

Proposing the solution based on comments by @mountiny

Proposal

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

Unable to confirm on double check Personal info page, error appears

What is the root cause of that problem?

Over here we only Validate the values against the Regex, but don't check if the values match the already existing legal name on profile page. https://github.com/Expensify/App/blob/e34f10ec01a21ecddebc4b12c721705ba7bd514d/src/pages/ReimbursementAccount/PersonalInfo/substeps/FullName.tsx#L31-L41

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

I propose we show the error on the Full Name page itself, this will avoid further confusion.


if (values.firstName && !ValidationUtils.doesLegalFirstNameMatchProfile(values.firstName)) {
        errors.firstName = 'bankAccount.error.legalFirstNameDoesNotMatch';
    }

    if (values.lastName && !ValidationUtils.doesLegalLastNameMatchProfile(values.lastName)) {
        errors.lastName = 'bankAccount.error.legalLastNameDoesNotMatch';
    }

We need to define the text for legalFirstNameDoesNotMatch and legalLastNameDoesNotMatch for both engilsh and spanish languages.

The doesLegalFirstNameMatchProfile and doesLegalLastNameMatchProfile util function will check if the current name value matches with the previously stored legal name on profile page.

Note: If no legal name is stored on profile page, then we can return true in the util functions

What alternative solutions did you explore? (Optional)

N/A

tienifr commented 2 months ago

Proposal

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

The user is not able to confirm on double check Personal info page.

What is the root cause of that problem?

In here, here and here, we're only showing generic error when Onfido error occurs.

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

If the Onfido error returned here (and similar places) has user-readable part, we should show it in the Growl instead of the translate('onfidoStep.genericError'). From my testing on web, it does return a user-readable message in the onError, so we can probably use it as is.

If not and it's only an error code, we should map the error code to a user-readable error in client side and show in Growl.

If there's no error content provided, we can fallback to the generic error.

What alternative solutions did you explore? (Optional)

NA

s77rt commented 2 months ago

I'm not able to reproduce this. Anyone able to reproduce on main?

GandalfGwaihir commented 2 months ago

Was able to reproduce on staging :

https://github.com/Expensify/App/assets/110545952/3934680c-5403-45f0-9d92-c00ba956d31b

mountiny commented 2 months ago

I think we want the errors from Onfido, not our own validation in this case

GandalfGwaihir commented 2 months ago

ahh, I see

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

abekkala commented 1 month ago

@garrettmknight I'm ooo until April 08 - I'll be taking back any issues taht are still Open once I return.


CURRENT STATUS:

Issue is still being reviewed and looking for a suitable proposal for fix.

s77rt commented 1 month ago

@mountiny Is this supposed to be internal?

s77rt commented 1 month ago

Sorry I missed to re-test this. Turns out this is not reproducible on dev. But I'm able to reproduce on production/staging

s77rt commented 1 month ago

@GandalfGwaihir Thanks for the proposal. Unfortunately it aims to achieve something different than the expected behaviour

s77rt commented 1 month ago

@tienifr Thanks for the proposal. Your RCA is correct. The solution looks good to me.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed Link to proposal

melvin-bot[bot] commented 1 month ago

Current assignee @cristipaval is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 1 month ago

πŸ“£ @s77rt πŸŽ‰ 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

melvin-bot[bot] commented 1 month ago

πŸ“£ @tienifr πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

tienifr commented 1 month ago

I'm sorry I linked the wrong issue.

tienifr commented 1 month ago

PR ready for review https://github.com/Expensify/App/pull/39012.

abekkala commented 1 month ago

I'm back from ooo and taking this one back

abekkala commented 1 month ago

PR has not been merged yet

tienifr commented 1 month ago

I found the root problem of this: Screenshot 2024-04-09 at 18 45 34 The Content-Security-Policy set in the backend is lacking https://sdk.onfido.com/v14/Onfido.is endpoint: Screenshot 2024-04-09 at 19 14 43 This happens on both staging and production. This is very critical since access to Onfido SDK is no longer working. And this requires a fix the backend. There's a suggested solution by Onfido here. It has nothing to do with the personal data.

@mountiny @cristipaval We found the root cause of this issue is about Content-Security-Policy. I think that is a very critical issue happening on production and needs immediate action.

Also, we don't need to show Onfido errors because:

So I think we can close the PR and fix root cause in the BE as mentioned above.

mountiny commented 1 month ago

Created an internal PR to address this https://github.com/Expensify/Cloudflare-Workers/pull/137/files

mountiny commented 1 month ago

@tienifr @s77rt can you please retest now? the backend fix should be out

s77rt commented 1 month ago

@mountiny https://sdk.onfido.com was added to the style-src whitelist. It should be script-src

mountiny commented 1 month ago

Thanks, waiting for deployment of the update 🀦

Its correct for adhoc builds only, overlooked that the assets.onfido been for style-src as well as script-src

tienifr commented 3 weeks ago

The fix is live on production. I remembered the font issue was already handled somewhere else πŸ€” Just want to ask if I and @s77rt got compensated because large efforts were put into PR implementation, review, testing and finding the RCA of this issue.

Screenshot 2024-04-22 at 10 19 53
tienifr commented 1 week ago

@mountiny Could you please check the above ^ when you had time?