[HOLD for payment 2024-08-09] [$250] Bank account - No error message is displayed for incorrect validation amounts

izarutskaya 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: v9.0.6-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/4709268 Email or phone of affected tester (no customers): applausetester+vd_ios071024@applause.expensifail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Pre-requisite: user must have created a Workspace and have enabled Workflows.

Go to Settings > Workspaces > Workspace > Workflows.
Tap on "Connect bank account".
Select "Connect manually".
Go trough the add bank account process using the testing credentials.
Once you reach the "Validate your bank account" page, enter 3 random amounts and tap on "Validate" button.
Submit many times.

Expected Result:

"The validate code you entered is incorrect, please try again" error should be displayed. Also, the validation should be disabled due to many tries.

Actual Result:

The error message for incorrect validation amounts is not displayed. Also, the validation is not disabled due to many tries.

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[ ] Android: mWeb Chrome
[x] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/15c0c1aa-b91f-470d-84d3-fdefbf7603d2

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~016e89ad61491fffbc
Upwork Job ID: 1811487532517648861
Last Price Increase: 2024-07-11

Issue Owner

Current Issue Owner: @abekkala

melvin-bot[bot] commented 2 months ago

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

izarutskaya commented 2 months ago

We think this issue might be related to the #collect project.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

Krishna2323 commented 2 months ago

Proposal

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

Bank account - No error message is displayed for incorrect validation amounts

What is the root cause of that problem?

No validation is added for amount limit. https://github.com/Expensify/App/blob/3fd16441c2d4606e28f7696fd86943a3525355ce/src/pages/ReimbursementAccount/ConnectBankAccount/components/BankAccountValidationForm.tsx#L65-L81

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

Add a if block which will compare max/min amount with filteredValue and add error using errors[key as keyof AmountValues] = translate('common.error.invalidAmount');. Error message can be created or can be used from any other form.

What alternative solutions did you explore? (Optional)

Krishna2323 commented 2 months ago

@izarutskaya @abekkala, is there any min/max limit for these inputs? When we should throw an error? I don't understand the bug exactly.

tienifr commented 2 months ago

Proposal

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

The error message for incorrect validation amounts is not displayed. Also, the validation is not disabled due to many tries.

What is the root cause of that problem?

In failure data of VALIDATE_BANK_ACCOUNT_WITH_TRANSACTIONS in https://github.com/Expensify/App/blob/58e257532290aa0228f1c668ed461a15f1b2b136/src/libs/actions/BankAccounts.ts#L336

We don't set the errors, so when the data returned by the back-end does not have the errors set, like in the case of 402 error code (back-end temporary issue) here, no errors will be shown to the users and it looks like the app is not responsive and broken.

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

Add optimistic errors for code validation similar to the approach here https://github.com/Expensify/App/blob/main/src/libs/actions/BankAccounts.ts#L165

In this add

errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('validationStep.validateCodeFailure'),

What alternative solutions did you explore? (Optional)

Store only the error translation key in Onyx, and use translate to translate it when we display the error

hungvu193 commented 2 months ago

I remember we discuss about this validation a while ago but couldn't find the slack thread. So I lean toward to the adding optimistic errors proposal. Let's hear another thoughts from internal engineer too.

🎀 👀 🎀

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

tienifr commented 2 months ago

@hungvu193 FYI we're already validating the amount here (including validating the length, see {0,8})

Also I don't think it's related to this issue, it happens not because the amount length is invalid

cc @youssef-lr

melvin-bot[bot] commented 2 months ago

@youssef-lr, @abekkala, @hungvu193 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

hungvu193 commented 2 months ago

Bump @youssef-lr for your decision

youssef-lr commented 2 months ago

Setting an error optimistically sounds good to me!

hungvu193 commented 2 months ago

Cool. So let's go with @tienifr 's proposal 😄

hungvu193 commented 2 months ago

@youssef-lr can you assign @tienifr so he can start working on the issue?

hungvu193 commented 2 months ago

@tienifr Do you have any ETA for the PR? 😄

abekkala commented 2 months ago

@tienifr can you give a date for your PR? Thanks!

hungvu193 commented 2 months ago

Still waiting for the PR

abekkala commented 2 months ago

not deployed to prod yet

melvin-bot[bot] commented 1 month ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 month ago

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

https://github.com/Expensify/App/pull/45732

If no regressions arise, payment will be issued on 2024-08-09. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@hungvu193 requires payment through NewDot Manual Requests
@tienifr requires payment through NewDot Manual Requests

melvin-bot[bot] commented 1 month ago

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:

[ ] [@hungvu193] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@hungvu193] 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:
[ ] [@hungvu193] 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:
[ ] [@hungvu193] Determine if we should create a regression test for this bug.
[ ] [@hungvu193] 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.
[ ] [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

abekkala commented 1 month ago

PAYMENT SUMMARY FOR AUG 09, if no regressions

Fix: @tienifr [$250] payment via NewDot
PR Review: @hungvu193 [$250] payment via NewDot please complete checklist

abekkala commented 1 month ago

not overdue payment is tomorrow

hungvu193 commented 1 month ago

[x] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/6230
[x] 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: https://github.com/Expensify/App/pull/6230/files#r1710967043
[x] 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:
[x] Determine if we should create a regression test for this bug: Yes

Regression test:

Go to Settings > Workspaces > Workspace > Workflows.
Tap on "Connect bank account".
Select "Connect manually" or "Connect with Plaid".
Go through the add bank account process using the testing credentials.
Once you reach the "Validate your bank account" page, enter 3 random amounts and click on the "Validate" button.
Submit many times.
Verify that the error message display at the bottom of the page.

Do we 👍 or 👎 ?

abekkala commented 1 month ago

PAYMENT SUMMARY FOR AUG 09, if no regressions

Fix: @tienifr [$250] payment via NewDot
PR Review: @hungvu193 [$250] payment via NewDot

JmillsExpensify commented 1 month ago

$250 approved for @hungvu193

garrettmknight commented 3 weeks ago

$250 approved for @tienifr

Expensify / App