App icon indicating copy to clipboard operation
App copied to clipboard

[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

Open trjExpensify opened this issue 1 year ago β€’ 74 comments

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:

  1. Go to expensify.com > sign-up > choose "1-9" to be redirected to NewDot
  2. Complete the onboarding modal steps to have a workspace created.
  3. Go to Settings > Workspaces > Click into the workspace created
  4. Go to More features > Enable workflows
  5. Go to Workflows > Make or track payments > Connect bank account

Expected Result:

This is a feature request.

  1. Neither the "Connect online with Plaid" or "Connect manually" option rows are greyed out
  2. There isn't a "Hold up! We need you to..." error message.
  3. When clicking either of the option rows in 1 above, we send a magic code email to the user, and show this validate your account page to collect it:
image

Actual Result:

  1. Both option rows are greyed out
  2. There's an error message on the page which we've since deprecated elsewhere in favour of a better experience to fire off and collect a magic code.
image

Platforms:

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

  • [x] Android: Standalone
  • [x] Android: HybridApp
  • [x] Android: mWeb Chrome
  • [x] iOS: Standalone
  • [x] iOS: HybridApp
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

In-line above.

View all open jobs on GitHub

CC: @shawnborton @mountiny

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021848301297798537201
  • Upwork Job ID: 1848301297798537201
  • Last Price Increase: 2024-10-21
  • Automatic offers:
    • shahinyan11 | Contributor | 104639697
Issue OwnerCurrent Issue Owner: @garrettmknight

trjExpensify avatar Oct 21 '24 09:10 trjExpensify

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.

melvin-bot[bot] avatar Oct 21 '24 09:10 melvin-bot[bot]

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

melvin-bot[bot] avatar Oct 21 '24 09:10 melvin-bot[bot]

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

melvin-bot[bot] avatar Oct 21 '24 09:10 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-10-22 12:42:52 UTC.

Proposal

Please 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?

  1. Remove this !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#L157
  2. Remove this code and the ValidateAccountMessage component https://github.com/Expensify/App/blob/8ca4576f77efd6b5312309702feaeb0b49fab17e/src/pages/ReimbursementAccount/BankAccountStep.tsx#L167
  3. When clicking the Connect button and the user has not yet verified the account, we will show a validation modal
<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]}
/>
  1. We will display the validation modal under here https://github.com/Expensify/App/blob/e74eb8e705bce3e8791e3eb043fe5cbb1cbaecea/src/pages/ReimbursementAccount/BankAccountStep.tsx#L183 if 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

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

<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

What alternative solutions did you explore? (Optional)

NJ-2020 avatar Oct 21 '24 10:10 NJ-2020

Edited by proposal-police: This proposal was edited at 2024-10-21 17:19:42 UTC.

Proposal

Please 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 BankAccountStep:

  1. Remove account.validated check from here and here

  2. Remove error note here: https://github.com/Expensify/App/blob/8ca4576f77efd6b5312309702feaeb0b49fab17e/src/pages/ReimbursementAccount/BankAccountStep.tsx#L167

  3. 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}
                    />
  1. Update 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

What alternative solutions did you explore? (Optional)

daledah avatar Oct 21 '24 10:10 daledah

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?

trjExpensify avatar Oct 21 '24 13:10 trjExpensify

I would say continue them down the path rather than bring them back to the prior step. Thoughts?

shawnborton avatar Oct 21 '24 13:10 shawnborton

Edited by proposal-police: This proposal was edited at 2024-10-21 16:21:02 UTC.

Proposal

Please 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 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,

What alternative solutions did you explore? (Optional)

shahinyan11 avatar Oct 21 '24 16:10 shahinyan11

PROPOSAL UPDATED

  • Add three solutions for https://github.com/Expensify/App/issues/51166#issuecomment-2426655170

daledah avatar Oct 21 '24 17:10 daledah

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!

trjExpensify avatar Oct 22 '24 07:10 trjExpensify

Asking in Slack if we could reassign to the C+ who are familiar with this flow https://expensify.slack.com/archives/C02NK2DQWUX/p1729591653444039

mountiny avatar Oct 22 '24 10:10 mountiny

Proposal Updated

NJ-2020 avatar Oct 22 '24 12:10 NJ-2020

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

hungvu193 avatar Oct 22 '24 13:10 hungvu193

PROPOSAL UPDATED

Change solution to use ValidateCodeActionModal

daledah avatar Oct 22 '24 14:10 daledah

Proposal

Updated. Update proposal to use ValidateCodeActionModal

shahinyan11 avatar Oct 22 '24 21:10 shahinyan11

Proposal Updated

NJ-2020 avatar Oct 22 '24 22:10 NJ-2020

Proposal Updated

NJ-2020 avatar Oct 23 '24 03:10 NJ-2020

Reviewing today!

hungvu193 avatar Oct 24 '24 03:10 hungvu193

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

hungvu193 avatar Oct 24 '24 16:10 hungvu193

Current assignee @mountiny is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] avatar Oct 24 '24 16:10 melvin-bot[bot]

little bump @mountiny

hungvu193 avatar Oct 29 '24 02:10 hungvu193

πŸ“£ @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 πŸ“–

melvin-bot[bot] avatar Oct 29 '24 07:10 melvin-bot[bot]

Go ahead @shahinyan11

mountiny avatar Oct 29 '24 07:10 mountiny

PR will be ready today

shahinyan11 avatar Oct 30 '24 07:10 shahinyan11

@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:

image

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:

image

So perhaps what we can do in a new PR is:

  • Add the ValidateCodeActionModal step to the 2FA flow to kill that existing message
  • Fix the Verify button position.

Happy to then pay $500 for this issue in its entirety.

trjExpensify avatar Nov 01 '24 11:11 trjExpensify

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?

mountiny avatar Nov 01 '24 11:11 mountiny

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

hungvu193 avatar Nov 01 '24 11:11 hungvu193

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

shahinyan11 avatar Nov 01 '24 12:11 shahinyan11

@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 avatar Nov 01 '24 12:11 shahinyan11

@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.)

dannymcclain avatar Nov 01 '24 13:11 dannymcclain