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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
May 29, 2024
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f9a3198
Bank account and Authorized payer buttons displayed instead of Connec…
ikevin127 Apr 12, 2024
042e04a
Merge branch 'main' into fix/39947
ikevin127 Apr 15, 2024
61afb5b
fixed infinite loading while offline
ikevin127 Apr 16, 2024
0d5b1f2
Merge branch 'main' of https://github.com/Expensify/App into fix/39947
ikevin127 Apr 17, 2024
09990eb
Merge branch 'main' of https://github.com/Expensify/App into fix/39947
ikevin127 May 9, 2024
988e4f8
fixed flow for with BA added
ikevin127 May 9, 2024
b7c067b
Merge branch 'main' of https://github.com/Expensify/App into fix/39947
ikevin127 May 16, 2024
31bddf5
adjusted offline mode styles and spacing
ikevin127 May 20, 2024
f4f6931
fixed type error
ikevin127 May 20, 2024
84a1c4a
Merge branch 'main' of https://github.com/Expensify/App into fix/39947
ikevin127 May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libs/actions/Policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,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 @@ -747,6 +748,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 @@ -758,6 +760,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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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