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

#39005 - Polish items for Transfer owner flow #39035

Merged
merged 11 commits into from
Mar 28, 2024
Merged

#39005 - Polish items for Transfer owner flow #39035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b317e2d
switched to use of correct currency utility function
burczu Mar 27, 2024
cc3fb59
missing flags setting for add billing card action added
burczu Mar 27, 2024
b69dee8
unnecessary import removed
burczu Mar 27, 2024
e5b480d
issue with stale error messages shown for ms fixed
burczu Mar 27, 2024
3279ebe
issue with glitching between transitions fixed
burczu Mar 27, 2024
b22a60f
unnecessary property removed
burczu Mar 27, 2024
cd5a60f
missing key for list added in the currency modal to highlight correctly
burczu Mar 27, 2024
21a889a
state variable renamed
burczu Mar 28, 2024
e6f1c04
unnecessary default value removed
burczu Mar 28, 2024
ae4c645
lint and prettier
burczu Mar 28, 2024
370c375
Merge branch 'main' into bugfix/39005-polish-items-for-transfer-owner…
burczu Mar 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
2 changes: 1 addition & 1 deletion src/libs/WorkspacesSettingsUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ function getOwnershipChecksDisplayText(
title = translate('workspace.changeOwner.ownerOwesAmountTitle');
text = translate('workspace.changeOwner.ownerOwesAmountText', {
email: ownerOwesAmount?.ownerEmail,
amount: CurrencyUtils.convertAmountToDisplayString(ownerOwesAmount?.amount, ownerOwesAmount?.currency),
amount: CurrencyUtils.convertToDisplayString(ownerOwesAmount?.amount, ownerOwesAmount?.currency),
});
buttonText = translate('workspace.changeOwner.ownerOwesAmountButtonText');
break;
Expand Down
6 changes: 6 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 @@ -1131,6 +1131,8 @@ function addBillingCardAndRequestPolicyOwnerChange(
value: {
errorFields: null,
isLoading: true,
isChangeOwnerSuccessful: false,
isChangeOwnerFailed: false,
},
},
];
Expand All @@ -1141,6 +1143,8 @@ function addBillingCardAndRequestPolicyOwnerChange(
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
value: {
isLoading: false,
isChangeOwnerSuccessful: true,
isChangeOwnerFailed: false,
},
},
];
Expand All @@ -1151,6 +1155,8 @@ function addBillingCardAndRequestPolicyOwnerChange(
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
value: {
isLoading: false,
isChangeOwnerSuccessful: false,
isChangeOwnerFailed: true,
},
},
];
Expand Down
34 changes: 28 additions & 6 deletions src/pages/workspace/members/WorkspaceOwnerChangeCheck.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useCallback} from 'react';
import React, {useCallback, useEffect, useState} from 'react';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
Expand Down Expand Up @@ -33,9 +33,33 @@ type WorkspaceOwnerChangeCheckProps = WorkspaceOwnerChangeCheckOnyxProps & {
function WorkspaceOwnerChangeCheck({personalDetails, policy, accountID, error}: WorkspaceOwnerChangeCheckProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const [displayTexts, setDisplayTexts] = useState({
title: '',
text: '',
buttonText: '',
});

const policyID = policy?.id ?? '';

const updateDisplayTexts = useCallback(() => {
const changeOwnerErrors = Object.keys(policy?.errorFields?.changeOwner ?? {});
if (error !== changeOwnerErrors[0]) {
return;
}

const texts = WorkspaceSettingsUtils.getOwnershipChecksDisplayText(error, translate, policy, personalDetails?.[accountID]?.login);
setDisplayTexts(texts);
}, [accountID, error, personalDetails, policy, translate]);

useEffect(() => {
updateDisplayTexts();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useEffect(() => {
updateDisplayTexts();
}, [updateDisplayTexts]);

Comment on lines +54 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intended to have two useEffects here? 🤔 cc @luacmartins @burczu

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a reason to have both, it seems like we can get rid of the first one without affecting functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

@carlosmiceli I started investigating components that disable react-hooks/exhaustive-deps and react-compiler/react-compiler because of this comment. That's how I noticed this weird duplicate.

I created a follow up issue to react-compiler upgrade PR, could you assign me to it? I don't want to spend too much time on this, but to fix some of obvious mistakes we have in the codebase. I could fix this one there too, let me know what do you think :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you meant to tag @carlosmiceli, but I assigned you to the issue above.

Copy link
Contributor

@blazejkustra blazejkustra Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh, sorry. I meant to tag you of course 🤦

const confirm = useCallback(() => {
if (error === CONST.POLICY.OWNERSHIP_ERRORS.HAS_FAILED_SETTLEMENTS || error === CONST.POLICY.OWNERSHIP_ERRORS.FAILED_TO_CLEAR_BALANCE) {
// cannot transfer ownership if there are failed settlements, or we cannot clear the balance
Expand All @@ -47,17 +71,15 @@ function WorkspaceOwnerChangeCheck({personalDetails, policy, accountID, error}:
PolicyActions.requestWorkspaceOwnerChange(policyID);
}, [accountID, error, policyID]);

const {title, text, buttonText} = WorkspaceSettingsUtils.getOwnershipChecksDisplayText(error, translate, policy, personalDetails?.[accountID]?.login);

return (
<>
<Text style={[styles.textHeadline, styles.mt3, styles.mb2]}>{title}</Text>
<Text style={styles.flex1}>{text}</Text>
<Text style={[styles.textHeadline, styles.mt3, styles.mb2]}>{displayTexts.title}</Text>
<Text style={styles.flex1}>{displayTexts.text}</Text>
<Button
success
large
onPress={confirm}
text={buttonText}
text={displayTexts.buttonText}
/>
</>
);
Expand Down
17 changes: 3 additions & 14 deletions src/pages/workspace/members/WorkspaceOwnerChangeWrapperPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type {StackScreenProps} from '@react-navigation/stack';
import React, {useEffect, useState} from 'react';
import React, {useEffect} from 'react';
import {View} from 'react-native';
import type {ValueOf} from 'type-fest';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
Expand All @@ -25,16 +25,11 @@ type WorkspaceOwnerChangeWrapperPageProps = WithPolicyOnyxProps & StackScreenPro
function WorkspaceOwnerChangeWrapperPage({route, policy}: WorkspaceOwnerChangeWrapperPageProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const [isTransitioning, setIsTransitioning] = useState(false);

const policyID = route.params.policyID;
const accountID = route.params.accountID;
const error = route.params.error;

useEffect(() => {
setIsTransitioning(true);
}, []);

useEffect(() => {
if (!policy || policy?.isLoading) {
return;
Expand Down Expand Up @@ -62,12 +57,7 @@ function WorkspaceOwnerChangeWrapperPage({route, policy}: WorkspaceOwnerChangeWr
return (
<AdminPolicyAccessOrNotFoundWrapper policyID={policyID}>
<PaidPolicyAccessOrNotFoundWrapper policyID={policyID}>
<ScreenWrapper
testID={WorkspaceOwnerChangeWrapperPage.displayName}
onEntryTransitionEnd={() => {
setIsTransitioning(false);
}}
>
<ScreenWrapper testID={WorkspaceOwnerChangeWrapperPage.displayName}>
<HeaderWithBackButton
title={translate('workspace.changeOwner.changeOwnerPageTitle')}
onBackButtonPress={() => {
Expand All @@ -76,9 +66,8 @@ function WorkspaceOwnerChangeWrapperPage({route, policy}: WorkspaceOwnerChangeWr
}}
/>
<View style={[styles.containerWithSpaceBetween, styles.ph5, error === CONST.POLICY.OWNERSHIP_ERRORS.NO_BILLING_CARD ? styles.pb0 : styles.pb5]}>
{(policy?.isLoading ?? isTransitioning) && <FullScreenLoadingIndicator />}
{policy?.isLoading && <FullScreenLoadingIndicator />}
{!policy?.isLoading &&
!isTransitioning &&
(error === CONST.POLICY.OWNERSHIP_ERRORS.NO_BILLING_CARD ? (
<WorkspaceOwnerPaymentCardForm policy={policy} />
) : (
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/members/WorkspaceOwnerPaymentCardCurrencyModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ function WorkspaceOwnerPaymentCardCurrencyModal({isVisible, currencies, currentC
data: currencies.map((currency) => ({
text: currency,
value: currency,
keyForList: currency,
isSelected: currency === currentCurrency,
})),
indexOffset: 0,
Expand Down Expand Up @@ -64,7 +65,6 @@ function WorkspaceOwnerPaymentCardCurrencyModal({isVisible, currencies, currentC
onBackButtonPress={onClose}
/>
<SelectionList
shouldDelayFocus
sections={sections}
onSelectRow={(option) => {
onCurrencyChange?.(option.value);
Expand Down
19 changes: 19 additions & 0 deletions src/pages/workspace/members/WorkspaceOwnerPaymentCardForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,23 @@ function WorkspaceOwnerPaymentCardForm({policy}: WorkspaceOwnerPaymentCardFormPr

const [isCurrencyModalVisible, setIsCurrencyModalVisible] = useState(false);
const [currency, setCurrency] = useState<keyof typeof CONST.CURRENCY>(CONST.CURRENCY.USD);
const [shouldShowPaymentCardForm, setShouldShowPaymentCardForm] = useState(false);

const policyID = policy?.id ?? '';

const checkIfCanBeRendered = useCallback(() => {
const changeOwnerErrors = Object.keys(policy?.errorFields?.changeOwner ?? {});
if (changeOwnerErrors[0] !== CONST.POLICY.OWNERSHIP_ERRORS.NO_BILLING_CARD) {
setShouldShowPaymentCardForm(false);
}

setShouldShowPaymentCardForm(true);
}, [policy?.errorFields?.changeOwner]);

useEffect(
() => {
PaymentMethods.clearDebitCardFormErrorAndSubmit();
checkIfCanBeRendered();

return () => {
PaymentMethods.clearDebitCardFormErrorAndSubmit();
Expand All @@ -57,6 +68,10 @@ function WorkspaceOwnerPaymentCardForm({policy}: WorkspaceOwnerPaymentCardFormPr
[],
);

useEffect(() => {
checkIfCanBeRendered();
}, [checkIfCanBeRendered]);

const validate = (formValues: FormOnyxValues<typeof ONYXKEYS.FORMS.ADD_DEBIT_CARD_FORM>): FormInputErrors<typeof ONYXKEYS.FORMS.ADD_DEBIT_CARD_FORM> => {
const errors = ValidationUtils.getFieldRequiredErrors(formValues, REQUIRED_FIELDS);

Expand Down Expand Up @@ -113,6 +128,10 @@ function WorkspaceOwnerPaymentCardForm({policy}: WorkspaceOwnerPaymentCardFormPr
setIsCurrencyModalVisible(false);
}, []);

if (!shouldShowPaymentCardForm) {
return null;
}

return (
<>
<Text style={[styles.textHeadline, styles.mt3, styles.mb2]}>{translate('workspace.changeOwner.addPaymentCardTitle')}</Text>
Expand Down
Loading