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

Create Validation Step #3587

Merged
merged 35 commits into from
Jun 22, 2021
Merged

Create Validation Step #3587

merged 35 commits into from
Jun 22, 2021

Conversation

ctkochan22
Copy link
Contributor

@ctkochan22 ctkochan22 commented Jun 15, 2021

@Luke9389 @marcaaron
cc @nkuoch @NikkiWines

Details

This is the Validation Step.

  • It takes in a form, once submitted will validate the bank account
  • Successfully closes the modal if amounts are correct
  • Tells users if input is invalid or incorrect

Also the Verifying Step

  • Show gif and tells users that we are verifying the BA's info

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/166921

Tests/QA Steps

Add a bank account, via the cash url: /bank-account. Use the Stack Overflow and follow the instructions for the "PENDING" state.

  • Verify that you see the form
  • Input non numeric answers and verify that it tells you "Invalid Amounts"
  • Input incorrect answers
  • Input correct Validation amounts. And verify that you get a success growl and modal is closed.
    • You may need to run a query SELECT validateCode FROM bankAccounts WHERE bankAccountID={insert id};

For testing the verifying state, just add a bank account with whatever information. It should require manual verification from ops, in which it will be in the verifying state. Verify that the gif is there and copy tells you that.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

E.g. Validation Form
image

E.g. User exceeded max attempts to validate bank account (8 times)
image

E.g. Verifying state
image

Web

Mobile Web

Desktop

iOS

Android

@ctkochan22 ctkochan22 self-assigned this Jun 15, 2021
@ctkochan22 ctkochan22 requested a review from a team as a code owner June 15, 2021 07:09
@github-actions
Copy link
Contributor

github-actions bot commented Jun 15, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team June 15, 2021 07:10
@ctkochan22
Copy link
Contributor Author

This is not ready to be reviewed yet @Luke9389 . I'll let you know!

@ctkochan22
Copy link
Contributor Author

ctkochan22 commented Jun 15, 2021

Validation Forms Display:

  • Get form up
  • Get form submission to hit ValidateBankaccount in Web-Expensify
  • Validate the forms input
  • Style the forms
  • Display error messages for invalid input
  • Display error for max attempts reached
  • Take in a bank accountID
  • Build out logic to either show validation or verification

Verifying Gif Display:

  • Display gif

Wierdness:

  • After submitting and hitting the web-expensify api, it will no longer load the bank account and it craps out. I'm thinking it may be due to the state of the bank account

Base automatically changed from marcaaron-setupWithdrawlAccount to main June 15, 2021 18:46
@ctkochan22
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty great overall! Most of my comments are minor or just style callout stuff that you've probably not got to yet, but figured i'd mention anyway.

// @TODO Not sure if we need to do this since for NewDot none of the accounts are pre-existing ones
currentStep = '';
}
failedValidationAttemptsName = CONST.NVP.FAILED_BANK_ACCOUNT_VALIDATIONS_PREFACE + bankAccountID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB, I think this can be

const failedValidationAttemptsName = ...

since the other usage is in the same scope. Unless we do something like this...

    return promiseAllSettled(...);
})
    .then(() => {
        ...
    });   

Which is a pattern we use sometimes when chaining promises.

src/libs/actions/BankAccounts.js Outdated Show resolved Hide resolved
src/libs/actions/BankAccounts.js Outdated Show resolved Hide resolved
src/libs/actions/BankAccounts.js Outdated Show resolved Hide resolved
src/libs/actions/BankAccounts.js Outdated Show resolved Hide resolved
src/pages/ReimbursementAccount/ValidationStep.js Outdated Show resolved Hide resolved
src/pages/ReimbursementAccount/ValidationStep.js Outdated Show resolved Hide resolved
src/pages/ReimbursementAccount/ValidationStep.js Outdated Show resolved Hide resolved
src/pages/ReimbursementAccount/ValidationStep.js Outdated Show resolved Hide resolved
src/pages/ReimbursementAccount/ValidationStep.js Outdated Show resolved Hide resolved
@shawnborton
Copy link
Contributor

Yes, that's perfect! Less is more!

@ctkochan22
Copy link
Contributor Author

@Luke9389 @marcaaron Ready for review!

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty great! Just caught a few small things like typos and nabs, but other than that 💯

Gonna test things out a bit and report back soon.

src/CONST.js Outdated Show resolved Hide resolved
buttonText: 'Finish Setup',
maxAttemptError: 'Validation for this bank account has been disabled due to too many incorrect attempts. Please contact us.',
description: 'A day or two after you add your account to Expensify we send three (3) transactions to your account. They have a merchant line like "Expensify, Inc. Validation"',
desriptionCTA: 'Please enter each transaction amount in the fields below. Example: 1.51',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing c in descriptionCTA

src/languages/es.js Outdated Show resolved Hide resolved
if (amount1 && amount2 && amount3) {
const validateCode = [amount1, amount2, amount3].join(',');

// Make a call to bankAccounts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like maybe this thought wasn't finished. Should we say which command or that we are calling the API? Maybe don't really need the comment at all.

src/pages/ReimbursementAccount/ValidationStep.js Outdated Show resolved Hide resolved
src/pages/ReimbursementAccount/ValidationStep.js Outdated Show resolved Hide resolved
@marcaaron
Copy link
Contributor

Oh yeah also I'm not 100% sure how to get a bank account to the validation step with a PENDING state. But we can maybe test this once the whole flow is tied together. It'd be cool to update the clitools.sh script so it can more easily test some of these flows on dev.

@marcaaron
Copy link
Contributor

Actually scratch that I figured it out. Was putting the wrong name in for the RequestorStep

@marcaaron
Copy link
Contributor

Nice one. Tests perfectly!

One other small design tweak that we can figure out later but the error message is indented a little too far and could maybe be positioned under the 3 amounts instead of above. Looks great otherwise!

2021-06-21_16-59-05

@ctkochan22
Copy link
Contributor Author

Updated

Luke9389
Luke9389 previously approved these changes Jun 22, 2021
Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! this looks good to me too. 👍

marcaaron
marcaaron previously approved these changes Jun 22, 2021
@marcaaron marcaaron dismissed stale reviews from Luke9389 and themself via e0cecca June 22, 2021 17:33
Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@NikkiWines NikkiWines merged commit 318ffa0 into main Jun 22, 2021
@NikkiWines NikkiWines deleted the ckt_validationStep branch June 22, 2021 18:04
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.73-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@isagoico
Copy link

@ctkochan22 Hello! not sure if we're able to test this PR. I'm unable to access the /bank-account URL, it just redirects me to the main page.

@ctkochan22
Copy link
Contributor Author

@marcaaron

@ctkochan22
Copy link
Contributor Author

Its behind a beta. You can check this off, we'll be testing this this again in the future with other PRs

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.74-0🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants