-
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
[HOLD for payment 2024-11-29] [HOLD for payment 2024-11-20] [HOLD for payment 2024-11-13] [$250] Add a step that collects the magic code when adding a VBBA #51166
Comments
Triggered auto assignment to @abekkala ( |
Job added to Upwork: https://www.upwork.com/jobs/~021848301297798537201 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Pujan92 ( |
Edited by proposal-police: This proposal was edited at 2024-10-22 12:42:52 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Add a step that collects the magic code when adding a VBBA What is the root cause of that problem?New feature What changes do you think we should make in order to solve the problem?
<MenuItem
icon={Expensicons.Bank}
title={translate('bankAccount.connectOnlineWithPlaid')}
disabled={!!isPlaidDisabled}
onPress={() => {
if (!!isPlaidDisabled) {
return;
}
if (!account?.validated) {
setConnectMethod(CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID);
setIsValidateCodeActionModalVisible(true);
return;
}
removeExistingBankAccountDetails();
BankAccounts.openPlaidView();
}}
shouldShowRightIcon
wrapperStyle={[styles.sectionMenuItemTopDescription]}
/>
<MenuItem
icon={Expensicons.Connect}
title={translate('bankAccount.connectManually')}
onPress={() => {
if (!account?.validated) {
setConnectMethod(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL);
setIsValidateCodeActionModalVisible(true);
return;
}
removeExistingBankAccountDetails();
BankAccounts.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL);
}}
shouldShowRightIcon
wrapperStyle={[styles.sectionMenuItemTopDescription]}
/>
const [validateCodeAction] = useOnyx(ONYXKEYS.VALIDATE_ACTION_CODE);
const [loginList] = useOnyx(ONYXKEYS.LOGIN_LIST);
const [isValidateCodeActionModalVisible, setIsValidateCodeActionModalVisible] = useState(false);
const [connectMethod, setConnectMethod] = useState('');
const primaryLogin = account?.primaryLogin ?? '';
const hasMagicCodeBeenSent = useMemo(() => !!validateCodeAction?.validateCodeSent, [validateCodeAction]);
const validateUserAndConnect = useCallback((code: string) => {
User.validateSecondaryLogin(loginList, primaryLogin, code)
if(connectMethod === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID) {
removeExistingBankAccountDetails();
BankAccounts.openPlaidView();
}
if(connectMethod === CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL) {
removeExistingBankAccountDetails();
BankAccounts.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL);
}
setIsValidateCodeActionModalVisible(false);
}, [loginList, primaryLogin, connectMethod]);
<ValidateCodeActionModal
hasMagicCodeBeenSent={hasMagicCodeBeenSent}
handleSubmitForm={validateUserAndConnect}
clearError={() => User.clearValidateCodeActionError('actionVerified')}
title={translate('contacts.validateAccount')}
description={translate('contacts.featureRequiresValidate')}
sendValidateCode={() => User.requestValidateCodeAction()}
onClose={() => setIsValidateCodeActionModalVisible(false)}
isVisible={isValidateCodeActionModalVisible}
/> Here's the test branch ResultScreen.Recording.2024-10-22.at.20.09.06.movAnd when there's an error, the error will be cleared automatically when we close the validation modal and it's handle by clearError function here <ValidateCodeActionModal
...
clearError={() => User.clearValidateCodeActionError('actionVerified')} And we're using the same way when we're dismissing the error inside the ValidateCodeActionModal App/src/components/ValidateCodeActionModal/ValidateCodeForm/BaseValidateCodeForm.tsx Lines 154 to 157 in faf48fc
Demo video when there's an error Screen.Recording.2024-10-22.at.20.14.12.movWhat alternative solutions did you explore? (Optional) |
Edited by proposal-police: This proposal was edited at 2024-10-21 17:19:42 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Add a step that collects the magic code when adding a VBBA What is the root cause of that problem?New feature What changes do you think we should make in order to solve the problem?In
const [loginList] = useOnyx(ONYXKEYS.LOGIN_LIST);
const contactMethod = account?.primaryLogin ?? '';
const isUserValidated = !!account?.validated;
const [isValidateCodeActionModalVisible, setIsValidateCodeActionModalVisible] = useState(true);
const [setupType, setSetupType] = useState('')
const validateAccountAndStartSteps = useCallback(
(validateCode: string) => {
User.validateSecondaryLogin(loginList, contactMethod, validateCode);
if (setupType === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID) {
BankAccounts.openPlaidView();
}
if (setupType === CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL) {
BankAccounts.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL);
}
setIsValidateCodeActionModalVisible(false);
},
[setupType, loginList, contactMethod],
); Add <ValidateCodeActionModal
clearError={clearError}
sendValidateCode={() => User.requestValidateCodeAction()}
isVisible={isValidateCodeActionModalVisible}
title={translate('contacts.validateAccount')}
description={translate('contacts.featureRequiresValidate')}
onClose={() => setIsValidateCodeActionModalVisible(false)}
handleSubmitForm={validateAccountAndStartSteps}
/>
onPress={() => {
if (!!isPlaidDisabled) {
return;
}
if (!account?.validated) {
setSetupType(CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID)
setIsValidateCodeActionModalVisible(true);
return;
}
removeExistingBankAccountDetails();
BankAccounts.openPlaidView();
}} POC Screen.Recording.2024-10-22.at.21.05.52.movWhat alternative solutions did you explore? (Optional) |
CC: @mountiny for eyes on this as I believe you've been involved in other flows that introduced it. Looking at the POC video, it overall looks good. My only question is whether we should continue them on in the flow rather than dropping them back on the same page where they have to click the button again. @shawnborton, whatcha' think? |
I would say continue them down the path rather than bring them back to the prior step. Thoughts? |
Edited by proposal-police: This proposal was edited at 2024-10-21 16:21:02 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Add a step that collects the magic code when adding a VBBA What is the root cause of that problem?New feature. Currently we have another implementation What changes do you think we should make in order to solve the problem?I suggest below changes for continue in the flow rather than dropping back on the same page as mentioned in this comment. 1. a) Add new state in ReimbursementAccountPage component for show/hide
b) Update this condition to prevent displaying
c) Pass new state variables to the BankAccountStep component as props
2. a) Add new props in BankAccountStepProps
b) Add bellow codes in BankAccountStep component body
3. Update this and this callbacks to below
4. Add bellow code in this line
5. Remove this line and the
What alternative solutions did you explore? (Optional) |
Agreed, I think that's best. @Pujan92 can you prioritise the proposal reviews today, please? Thanks! |
Asking in Slack if we could reassign to the C+ who are familiar with this flow https://expensify.slack.com/archives/C02NK2DQWUX/p1729591653444039 |
Thanks for proposals everyone. Please update your proposal to reuse our existing component called: ValidateCodeActionModal Here’s example: #49445 |
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.60-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-11-20. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.65-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-11-29. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@mountiny do we already have a regression test for this? |
No, I do not think we have a test for this specific flow @hungvu193 @getusha could you complete the checklist here and propose a test to add please? thanks! |
On it, thanks |
What's the payment summary for this one? I think we created 3 PRs for this issue. |
@hungvu193 can you please list them out for simplicity? @getusha can you then complete the regression tests for these prs please? |
|
Regression Test Proposal Invoices
Workflows
2FA Authentication
Do we agree 👍 or 👎 |
@hungvu193 $750 is the summary |
$750 approved for @hungvu193 |
@getusha request when you're ready. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: v9.0.51-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Applicable on all platforms
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @trjExpensify
Slack conversation: https://expensify.slack.com/archives/C07HPDRELLD/p1729255848407089?thread_ts=1729042472.964519&cid=C07HPDRELLD
Action Performed:
Expected Result:
This is a feature request.
Actual Result:
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
In-line above.
View all open jobs on GitHub
CC: @shawnborton @mountiny
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @garrettmknightThe text was updated successfully, but these errors were encountered: