Open trjExpensify opened 1 month ago
Triggered auto assignment to @abekkala (NewFeature
), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.
Job added to Upwork: https://www.upwork.com/jobs/~021848301297798537201
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Pujan92 (External
)
Edited by proposal-police: This proposal was edited at 2024-10-22 12:42:52 UTC.
Add a step that collects the magic code when adding a VBBA
New feature
!account.validated
check
https://github.com/Expensify/App/blob/8ca4576f77efd6b5312309702feaeb0b49fab17e/src/pages/ReimbursementAccount/BankAccountStep.tsx#L141
https://github.com/Expensify/App/blob/8ca4576f77efd6b5312309702feaeb0b49fab17e/src/pages/ReimbursementAccount/BankAccountStep.tsx#L157ValidateAccountMessage
component
https://github.com/Expensify/App/blob/8ca4576f77efd6b5312309702feaeb0b49fab17e/src/pages/ReimbursementAccount/BankAccountStep.tsx#L167<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]}
/>
isValidateCodeActionModalVisible
is true
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](https://github.com/NJ-2020/Expensify/tree/test/51166)
### Result
https://github.com/user-attachments/assets/10d7581d-b5e5-449c-aed1-3c920c5d9350
And 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
```tsx
<ValidateCodeActionModal
...
clearError={() => User.clearValidateCodeActionError('actionVerified')}
And we're using the same way when we're dismissing the error inside the ValidateCodeActionModal https://github.com/Expensify/App/blob/faf48fcaaf9bb1a8dca6e997e3e1a85606a97ace/src/components/ValidateCodeActionModal/ValidateCodeForm/BaseValidateCodeForm.tsx#L154-L157
Demo video when there's an error
https://github.com/user-attachments/assets/f0e4ec02-b12d-49ae-9844-ed60d40d29fc
Edited by proposal-police: This proposal was edited at 2024-10-21 17:19:42 UTC.
Add a step that collects the magic code when adding a VBBA
New feature
In BankAccountStep
:
Remove error note here: https://github.com/Expensify/App/blob/8ca4576f77efd6b5312309702feaeb0b49fab17e/src/pages/ReimbursementAccount/BankAccountStep.tsx#L167
Add ValidateCodeActionModal
related props:
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
to after here:
<ValidateCodeActionModal
clearError={clearError}
sendValidateCode={() => User.requestValidateCodeAction()}
isVisible={isValidateCodeActionModalVisible}
title={translate('contacts.validateAccount')}
description={translate('contacts.featureRequiresValidate')}
onClose={() => setIsValidateCodeActionModalVisible(false)}
handleSubmitForm={validateAccountAndStartSteps}
/>
onPress
function to set setupType
before opening modal, for example with plaid button:onPress={() => {
if (!!isPlaidDisabled) {
return;
}
if (!account?.validated) {
setSetupType(CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID)
setIsValidateCodeActionModalVisible(true);
return;
}
removeExistingBankAccountDetails();
BankAccounts.openPlaidView();
}}
POC
https://github.com/user-attachments/assets/4315318e-4b75-4e50-b6a2-1087ec6bfece
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.
Add a step that collects the magic code when adding a VBBA
New feature. Currently we have another implementation
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 ValidateCodeActionModal
modal. ( Note:
Point b) requires that this state be defined in ReimbursementAccountPage
component )
const [isValidateCodeActionModalVisible, setIsValidateCodeActionModalVisible] = useState(false)
b) Update this condition to prevent displaying ReimbursementAccountLoadingIndicator
when the validation modal is open
if (
(!hasACHDataBeenLoaded || isLoading) && shouldShowOfflineLoader && (shouldReopenOnfido || !requestorStepRef.current) &&
!(currentStep === CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT && isValidateCodeActionModalVisible)
) {
return <ReimbursementAccountLoadingIndicator onBackButtonPress={goBack} />;
}
c) Pass new state variables to the BankAccountStep component as props
<BankAccountStep
...
isValidateCodeActionModalVisible={isValidateCodeActionModalVisible}
toggleValidateCodeActionModal={setIsValidateCodeActionModalVisible}
/>
2. a) Add new props in BankAccountStepProps
type BankAccountStepProps = {
...
/** Should ValidateCodeActionModal be displayed or not */
isValidateCodeActionModalVisible?: boolean;
/** Toggle ValidateCodeActionModal*/
toggleValidateCodeActionModal?: (isVisible: boolean) => void;
}
b) Add bellow codes in BankAccountStep component body
const [loginList] = useOnyx(ONYXKEYS.LOGIN_LIST);
const contactMethod = account?.primaryLogin ?? '';
const selectedSubStep = useRef('')
const loginData = useMemo(() => loginList?.[contactMethod], [loginList, contactMethod]);
const validateLoginError = ErrorUtils.getEarliestErrorField(loginData, 'validateLogin');
const hasMagicCodeBeenSent = !!loginData?.validateCodeSent;
....
useEffect(() => {
if (!account?.validated) {
return;
}
if(selectedSubStep.current === CONST.BANK_ACCOUNT.SUBSTEP.MANUAL){
BankAccounts.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL);
}else if (selectedSubStep.current === CONST.BANK_ACCOUNT.SUBSTEP.PLAID){
BankAccounts.openPlaidView();
}
}, [account?.validated]);
3. Update this and this callbacks to below
() => {
if(!account?.validated){
selectedSubStep.current = CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID
toggleValidateCodeActionModal?.(true)
return
}
}
....
() => {
if(!account?.validated){
selectedSubStep.current = CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL
toggleValidateCodeActionModal?.(true)
return
}
removeExistingBankAccountDetails();
BankAccounts.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL);
}
4. Add bellow code in this line
<ValidateCodeActionModal
title={translate('contacts.validateAccount')}
description={translate('contacts.featureRequiresValidate')}
isVisible={isValidateCodeActionModalVisible }
hasMagicCodeBeenSent={hasMagicCodeBeenSent}
validatePendingAction={loginData?.pendingFields?.validateCodeSent}
sendValidateCode={() => User.requestValidateCodeAction()}
handleSubmitForm={(validateCode)=> User.validateSecondaryLogin(loginList, contactMethod, validateCode)}
validateError={!isEmptyObject(validateLoginError) ? validateLoginError : ErrorUtils.getLatestErrorField(loginData, 'validateCodeSent')}
clearError={()=> User.clearContactMethodErrors(contactMethod, !isEmptyObject(validateLoginError) ? 'validateLogin' : 'validateCodeSent')}
onClose={() => toggleValidateCodeActionModal?.(false)}
/>
5. Remove this line and the !account?.validated
checks from here and here
6. OPTIONAL. If the options should be grayed out but clickable we can add new shouldGreyOut
prop to MenuItem
to component. And update this conditional style like below
( shouldGreyOut || (shouldGreyOutWhenDisabled && disabled) ) && styles.buttonOpacityDisabled,
I would say continue them down the path rather than bring them back to the prior step. Thoughts?
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: https://github.com/Expensify/App/pull/49445
Change solution to use ValidateCodeActionModal
Updated. Update proposal to use ValidateCodeActionModal
Reviewing today!
Thanks for the proposals, everyone! We shouldn't allow users to continue the VBBA flow if they entered the wrong magic code. @shahinyan11 's proposal here looks good to me as it is the only proposal that handles both cases (valid and invalid magic code). π π π C+ reviewed
Current assignee @mountiny is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
little bump @mountiny
π£ @shahinyan11 π 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 π
Go ahead @shahinyan11
PR will be ready today
@shahinyan11 @hungvu193 I see the linked PR has merged, but the Verify
button has floated to the top it seems reviewing the video here: https://github.com/Expensify/App/pull/51718#issuecomment-2451555738
It should be at the bottom like so:
Can we fix that, please? (CC: @Expensify/design).
While we're here though, @anmurali noticed that the 2FA flow has that old error message in use:
So perhaps what we can do in a new PR is:
ValidateCodeActionModal
step to the 2FA flow to kill that existing messageVerify
button position. Happy to then pay $500 for this issue in its entirety.
forgot about the button position, you are right. this should be a quick fix, though @shahinyan11 @hungvu193, could you do that in one PR and then look into the 2FA section in another one for increased bounty?
forgot about the button position, you are right. this should be a quick fix, though @shahinyan11 @hungvu193, could you do that in one PR and then look into the 2FA section in another one for increased bounty?
Agreed
I started working on both and will share two PR-s here. I have just a question. What Issue I should mention in new PR for 2FA
@trjExpensify @mountiny Is the behavior in the video expected ? Note that the button moves to the top when an error message appears.
https://github.com/user-attachments/assets/021279c1-560d-4440-9015-d9b045904e5a
@shahinyan11 The error should show just above the button instead of below it. Pretty sure Youssef ran into this in another spot and it was as simple as passing some prop to that error message or something simple like that. (Trying to avoid pinging him but we definitely can.)
@trjExpensify @mountiny Can I start working on 2FA ?
The error should show just above the button instead of below it.
Agreed, that's just the standard forms error pattern to follow.
@trjExpensify @mountiny Can I start working on 2FA ?
yeah, go for it.
yeah, go for it.
@trjExpensify Shouldn't a new issue be added for this to mention it in PR?
yeah, go for it.
@trjExpensify Shouldn't a new issue be added for this to mention it in PR?
No need to create new issue. Let's just mention this issue in your PR
Yeah, I think it's fine to just use this one for ease. Please tag design in your PR though to take a look. π
@shahinyan11 Can you combine your 2 PRs into one? I don't think we need 2 PRs.
I have created two PR-s following this comment
@hungvu193 lets try to complete the review tomorrow
β οΈ Looks like this issue was linked to a Deploy Blocker here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.57-10 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-11-13. :confetti_ball:
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:
Hi @trjExpensify. In my option we have one more place where the ValidateCodeActionModal
should be added. The Add bank account
button on the Invoices page is currently disabled for unverified users. What do you think ?
Yep, that's a new one.. but I totally agree. π
Yeah agree... rather than showing it disabled, let's show it enabled and just have it launch the validate screen right?
Can I work on this with increased bounty?
Yeah agree... rather than showing it disabled, let's show it enabled and just have it launch the validate screen right?
Yep!
Can I work on this with increased bounty?
Yep!
@trjExpensify Should I work on it as part of this issue ?
Yep, let's keep it all here and we'll adjust the price when we need to settle up.
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.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: @garrettmknight