-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Create Validation Step #3587
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
This is not ready to be reviewed yet @Luke9389 . I'll let you know! |
Validation Forms Display:
Verifying Gif Display:
Wierdness:
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this 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.
src/libs/actions/BankAccounts.js
Outdated
// @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; |
There was a problem hiding this comment.
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.
Yes, that's perfect! Less is more! |
@Luke9389 @marcaaron Ready for review! |
There was a problem hiding this 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/languages/en.js
Outdated
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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing c in descriptionCTA
if (amount1 && amount2 && amount3) { | ||
const validateCode = [amount1, amount2, amount3].join(','); | ||
|
||
// Make a call to bankAccounts |
There was a problem hiding this comment.
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.
Oh yeah also I'm not 100% sure how to get a bank account to the validation step with a |
Actually scratch that I figured it out. Was putting the wrong name in for the |
Updated |
There was a problem hiding this 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. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging in version: 1.0.73-4🚀
|
@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. |
Its behind a beta. You can check this off, we'll be testing this this again in the future with other PRs |
🚀 Deployed to production in version: 1.0.74-0🚀
|
@Luke9389 @marcaaron
cc @nkuoch @NikkiWines
Details
This is the Validation Step.
Also the Verifying Step
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.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
Screenshots
E.g. Validation Form
E.g. User exceeded max attempts to validate bank account (8 times)
E.g. Verifying state
Web
Mobile Web
Desktop
iOS
Android