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

Bank account and Authorized payer buttons displayed instead of Connect bank account #40182

Merged
merged 10 commits into from
May 29, 2024
3 changes: 3 additions & 0 deletions src/libs/actions/Policy/Policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,7 @@ function setWorkspaceReimbursement(policyID: string, reimbursementChoice: ValueO
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
value: {
reimbursementChoice,
isLoadingWorkspaceReimbursement: true,
achAccount: {reimburser: reimburserEmail},
errorFields: {reimbursementChoice: null},
pendingFields: {reimbursementChoice: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE},
Expand All @@ -724,6 +725,7 @@ function setWorkspaceReimbursement(policyID: string, reimbursementChoice: ValueO
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
value: {
isLoadingWorkspaceReimbursement: false,
errorFields: {reimbursementChoice: null},
pendingFields: {reimbursementChoice: null},
},
Expand All @@ -735,6 +737,7 @@ function setWorkspaceReimbursement(policyID: string, reimbursementChoice: ValueO
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
value: {
isLoadingWorkspaceReimbursement: false,
reimbursementChoice: policy.reimbursementChoice ?? null,
achAccount: {reimburser: policy.achAccount?.reimburser ?? null},
errorFields: {reimbursementChoice: ErrorUtils.getMicroSecondOnyxError('common.genericErrorMessage')},
Expand Down
107 changes: 58 additions & 49 deletions src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {useFocusEffect} from '@react-navigation/native';
import type {StackScreenProps} from '@react-navigation/stack';
import React, {useCallback, useMemo, useState} from 'react';
import {View} from 'react-native';
import {ActivityIndicator, View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import ConfirmModal from '@components/ConfirmModal';
Expand All @@ -12,6 +12,7 @@ import Section from '@components/Section';
import Text from '@components/Text';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import * as ErrorUtils from '@libs/ErrorUtils';
Expand Down Expand Up @@ -44,6 +45,7 @@ type WorkspaceWorkflowsPageProps = WithPolicyProps & WorkspaceWorkflowsPageOnyxP

function WorkspaceWorkflowsPage({policy, betas, route}: WorkspaceWorkflowsPageProps) {
const {translate, preferredLocale} = useLocalize();
const theme = useTheme();
const styles = useThemeStyles();
const {isSmallScreenWidth} = useWindowDimensions();

Expand Down Expand Up @@ -83,8 +85,9 @@ function WorkspaceWorkflowsPage({policy, betas, route}: WorkspaceWorkflowsPagePr
);

const optionItems: ToggleSettingOptionRowProps[] = useMemo(() => {
const {accountNumber, addressName, bankName} = policy?.achAccount ?? {};
const hasVBA = !!policy?.achAccount;
const {accountNumber, addressName, bankName, bankAccountID} = policy?.achAccount ?? {};
const shouldShowBankAccount = !!bankAccountID && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES;

let bankDisplayName = bankName ?? addressName;
if (accountNumber && bankDisplayName !== accountNumber) {
bankDisplayName += ` ${accountNumber.slice(-5)}`;
Expand Down Expand Up @@ -168,60 +171,65 @@ function WorkspaceWorkflowsPage({policy, betas, route}: WorkspaceWorkflowsPagePr
let newReimbursementChoice;
if (!isEnabled) {
newReimbursementChoice = CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO;
} else if (hasVBA && !Policy.isCurrencySupportedForDirectReimbursement(policy?.outputCurrency ?? '')) {
} else if (!!policy?.achAccount && !Policy.isCurrencySupportedForDirectReimbursement(policy?.outputCurrency ?? '')) {
newReimbursementChoice = CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL;
} else {
newReimbursementChoice = hasVBA ? CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES : CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL;
newReimbursementChoice = CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES;
}

const newReimburserEmail = policy?.achAccount?.reimburser ?? policy?.owner;
Policy.setWorkspaceReimbursement(policy?.id ?? '', newReimbursementChoice, newReimburserEmail ?? '');
},
subMenuItems: (
<>
<MenuItem
titleStyle={hasVBA ? styles.textLabelSupportingNormal : styles.textLabelSupportingEmptyValue}
descriptionTextStyle={styles.textNormalThemeText}
title={
hasVBA && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES
? translate('common.bankAccount')
: translate('workflowsPage.connectBankAccount')
}
description={bankDisplayName}
disabled={isOffline || !isPolicyAdmin}
Copy link
Contributor

@ishpaul777 ishpaul777 May 17, 2024

Choose a reason for hiding this comment

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

In this screen, why does one row appear to be more faded out than the other?

it seems to done quite intentionally even before this PR. that is for the case when there's no optimistic pending action but user is offline.

Screenshot 2024-05-17 at 8 30 28 PM

we can add a check !policy?.pendingFields?.reimbursementChoice && isOffline to solve this

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I think we should not have it double-faded so let's definitely solve that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

  1. The first disabled grayed-out fade comes from the "Make or track payments" toggle when toggled while offline.
  2. The second one comes from Connect bank account / Bank account option which was and is disabled while offline.
  3. Authorized payer option is enabled while offline -> meaning the Authorized payer can be changed optimistically. This is why this one doesn't get double disabled grayed-out fade when "Make or track payments" is toggled while offline.

ishpaul777:

we can add a check !policy?.pendingFields?.reimbursementChoice && isOffline to solve this

I don't think this logic is good since it would break the logic mentioned at (2.) where Connect bank account / Bank account option was and is supposed to be disabled while offline.

shawnborton:

Okay, I think we should not have it double-faded so let's definitely solve that

To do this, instead I would use Connect bank account / Bank account option shouldGreyOutWhenDisabled to not have the 2nd gray-out fade applied to the option when already applied from "Make or track payments" toggle.

Similarly, we have to do the same for the OfflineWithFeedback wrapper of Authorized payer option (3.) we add:

shouldDisableOpacity={isOffline && policy?.pendingFields?.reimbursementChoice && policy?.pendingFields?.reimburser}

in order to not double gray-out the Authorized payer if already applied from "Make or track payments" toggle, when Authorized payer is changed while offline. This is how it would look in all scenarios:

  1. Without Bank account added:
Offline (can't add / change Connect bank account option)
noba-offline
Offline (with "Make or track payments" Toggled (optimistically))
noba-offline-toggled
  1. With Bank account added:
Offline (can't add / change Bank account option)
ba-offline
Offline (with "Make or track payments" Toggled (optimistically))
ba-offline-toggled-1
Offline (with "Make or track payments" Toggled + Authorized payer changed (optimistically))
ba-offline-toggled-2
Offline (with "Make or track payments" Toggled + Authorized payer changed (optimistically) and hover on Authorized payer)
ba-offline-toggled-2-hover
shouldDisableOpacity={isOffline && policy?.pendingFields?.reimbursementChoice && policy?.pendingFields?.reimburser}

^ - Writing it like this is specifically important for the below case, since we don't want to NOT gray out Authorized payer when changed offline (optimistically) in case "Make or track payments" was NOT toggled, by only writing it like:
shouldDisableOpacity={policy?.pendingFields?.reimburser}
since that would NOT gray out the optimistically set Authorized payer, instead we want to gray it out, maintaining the pattern like so:

Offline (with "Make or track payments" Not Toggled + Authorized payer changed (optimistically))
ba-offline-non-toggled-changed-payer

Please let me know if we're in agreement with the changes mentioned above from a design / code POV.
Once agreed I'll push the changes!

cc @ishpaul777 @shawnborton

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for detailed investigation @ikevin127, code wise looks good to me 👍

onPress={() => {
if (!Policy.isCurrencySupportedForDirectReimbursement(policy?.outputCurrency ?? '')) {
setIsCurrencyModalOpen(true);
return;
}
navigateToBankAccountRoute(route.params.policyID, ROUTES.WORKSPACE_WORKFLOWS.getRoute(route.params.policyID));
}}
shouldShowRightIcon={!isOffline && isPolicyAdmin}
wrapperStyle={containerStyle}
hoverAndPressStyle={[styles.mr0, styles.br2]}
subMenuItems:
!isOffline && policy?.isLoadingWorkspaceReimbursement === true ? (
<ActivityIndicator
size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE}
color={theme.spinner}
style={styles.mt7}
/>
{hasVBA && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES && (
<OfflineWithFeedback
pendingAction={policy?.pendingFields?.reimburser}
errors={ErrorUtils.getLatestErrorField(policy ?? {}, CONST.POLICY.COLLECTION_KEYS.REIMBURSER)}
onClose={() => Policy.clearPolicyErrorField(policy?.id ?? '', CONST.POLICY.COLLECTION_KEYS.REIMBURSER)}
errorRowStyles={[styles.ml7]}
>
<MenuItem
titleStyle={styles.textLabelSupportingNormal}
descriptionTextStyle={styles.textNormalThemeText}
title={translate('workflowsPayerPage.title')}
description={displayNameForAuthorizedPayer}
onPress={() => Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_PAYER.getRoute(route.params.policyID))}
shouldShowRightIcon
wrapperStyle={containerStyle}
hoverAndPressStyle={[styles.mr0, styles.br2]}
brickRoadIndicator={hasReimburserError ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
/>
</OfflineWithFeedback>
)}
</>
),
) : (
<>
<MenuItem
titleStyle={shouldShowBankAccount ? styles.textLabelSupportingNormal : styles.textLabelSupportingEmptyValue}
descriptionTextStyle={styles.textNormalThemeText}
title={shouldShowBankAccount ? translate('common.bankAccount') : translate('workflowsPage.connectBankAccount')}
description={bankDisplayName}
disabled={isOffline || !isPolicyAdmin}
shouldGreyOutWhenDisabled={!policy?.pendingFields?.reimbursementChoice}
onPress={() => {
if (!Policy.isCurrencySupportedForDirectReimbursement(policy?.outputCurrency ?? '')) {
setIsCurrencyModalOpen(true);
return;
}
navigateToBankAccountRoute(route.params.policyID, ROUTES.WORKSPACE_WORKFLOWS.getRoute(route.params.policyID));
}}
shouldShowRightIcon={!isOffline && isPolicyAdmin}
wrapperStyle={containerStyle}
hoverAndPressStyle={[styles.mr0, styles.br2]}
/>
{shouldShowBankAccount && (
<OfflineWithFeedback
pendingAction={policy?.pendingFields?.reimburser}
shouldDisableOpacity={isOffline && !!policy?.pendingFields?.reimbursementChoice && !!policy?.pendingFields?.reimburser}
errors={ErrorUtils.getLatestErrorField(policy ?? {}, CONST.POLICY.COLLECTION_KEYS.REIMBURSER)}
onClose={() => Policy.clearPolicyErrorField(policy?.id ?? '', CONST.POLICY.COLLECTION_KEYS.REIMBURSER)}
errorRowStyles={[styles.ml7]}
>
<MenuItem
titleStyle={styles.textLabelSupportingNormal}
descriptionTextStyle={styles.textNormalThemeText}
title={translate('workflowsPayerPage.title')}
description={displayNameForAuthorizedPayer}
onPress={() => Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_PAYER.getRoute(route.params.policyID))}
shouldShowRightIcon
wrapperStyle={[...containerStyle, styles.mt0]}
hoverAndPressStyle={[styles.mr0, styles.br2]}
brickRoadIndicator={hasReimburserError ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
/>
</OfflineWithFeedback>
)}
</>
),
isEndOptionRow: true,
isActive: policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO,
pendingAction: policy?.pendingFields?.reimbursementChoice,
Expand All @@ -242,6 +250,7 @@ function WorkspaceWorkflowsPage({policy, betas, route}: WorkspaceWorkflowsPagePr
displayNameForAuthorizedPayer,
isOffline,
isPolicyAdmin,
theme,
]);

const renderOptionItem = (item: ToggleSettingOptionRowProps, index: number) => (
Expand Down
3 changes: 3 additions & 0 deletions src/types/onyx/Policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,9 @@ type Policy = OnyxCommon.OnyxValueWithOfflineFeedback<
/** Indicates if the Policy is in loading state */
isLoading?: boolean;

/** Indicates the Policy's SetWorkspaceReimbursement call loading state */
isLoadingWorkspaceReimbursement?: boolean;

/** Indicates if the Policy ownership change is successful */
isChangeOwnerSuccessful?: boolean;

Expand Down
Loading