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

Refactor bookATrip function #55852

Merged
merged 29 commits into from
Feb 5, 2025
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
d18f99b
Pass policy to Travel.bookATrip
cristipaval Jan 28, 2025
9ae5e55
Remove activePolicyID from Travel.ts
cristipaval Jan 28, 2025
1a32608
Remove primaryLogin from Travel.ts
cristipaval Jan 28, 2025
bc569f5
Add footer to the FeatureList component
cristipaval Jan 28, 2025
d16741c
Move Travel specific components out of FeatureList
cristipaval Jan 28, 2025
70d8109
Move bookATrip logic to BookTravelButton
cristipaval Jan 28, 2025
358fe95
Get primaryLogin from Onyx
cristipaval Jan 28, 2025
789a92b
Add body prop instead of reusing subtitle for everything
cristipaval Jan 28, 2025
fa0ddbb
Put BookTravelButton in the body of the EmptyStateComponent
cristipaval Jan 28, 2025
22f3617
Removed unused bookATrip function from Travel.ts
cristipaval Jan 28, 2025
c6e30de
Remove antipattern provisionDomain function
cristipaval Jan 28, 2025
b633ba0
Remove antipattern handleProvisioningPermissionDeniedError function
cristipaval Jan 28, 2025
00d6012
Remove antipattern openTravelDotAfterProvisioning function
cristipaval Jan 28, 2025
243a09e
Use body instead of subtitle
cristipaval Jan 28, 2025
46b6485
Make ESLint happt
cristipaval Jan 28, 2025
f9428d6
Add comments
cristipaval Jan 30, 2025
f10ee2e
Move function out of the tsx
cristipaval Jan 30, 2025
cf3e4bd
Add comment
cristipaval Jan 30, 2025
2331d13
New function for navigating to Accept Terms
cristipaval Feb 3, 2025
ab8d5db
Add comments
cristipaval Feb 3, 2025
7dd2c55
children instead of body in EmptyStateComponentProps
cristipaval Feb 3, 2025
f8e248a
Move function in the component since it has a dependency
cristipaval Feb 4, 2025
14ee3bc
Merge remote-tracking branch 'origin/main' into cristi_fix-bookATrip-…
cristipaval Feb 4, 2025
950ee37
Disable root status bar after going back to classic
cristipaval Feb 4, 2025
75e4342
Fix ESLint
cristipaval Feb 4, 2025
e43921a
Update src/components/BookTravelButton.tsx
cristipaval Feb 4, 2025
820a0e5
Update src/components/EmptyStateComponent/index.tsx
cristipaval Feb 4, 2025
f7f58db
Tiny improvements suggested by reviewers
cristipaval Feb 4, 2025
b851e2f
Update src/components/BookTravelButton.tsx
cristipaval Feb 4, 2025
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
120 changes: 120 additions & 0 deletions src/components/BookTravelButton.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
import {Str} from 'expensify-common';
import React, {useCallback, useContext, useState} from 'react';
import {NativeModules} from 'react-native';
import {useOnyx} from 'react-native-onyx';
import useLocalize from '@hooks/useLocalize';
import usePolicy from '@hooks/usePolicy';
import useThemeStyles from '@hooks/useThemeStyles';
import {openTravelDotLink} from '@libs/actions/Link';
import {cleanupTravelProvisioningSession} from '@libs/actions/Travel';
import Log from '@libs/Log';
import Navigation from '@libs/Navigation/Navigation';
import {getAdminsPrivateEmailDomains} from '@libs/PolicyUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import Button from './Button';
import CustomStatusBarAndBackgroundContext from './CustomStatusBarAndBackground/CustomStatusBarAndBackgroundContext';
import DotIndicatorMessage from './DotIndicatorMessage';

type BookTravelButtonProps = {
text: string;
};

const navigateToAcceptTerms = (domain: string) => {
// Remove the previous provision session infromation if any is cached.
cleanupTravelProvisioningSession();
Navigation.navigate(ROUTES.TRAVEL_TCS.getRoute(domain));
};

Comment on lines +25 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor comment
But similar case as here
We usually don't move such functions out of the component 😅

#55852 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, we want it outside of the component since it has no dependency on the component, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right
As I mentioned, it's just a minor comment
Also, keeping the function outside a component prevents it from being recreated on every render
What I meant is that we usually keep wrapper functions inside the component
But I don't mind leaving it as is !

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it's OK where it's at, and I also agree that I kinda wish more pure functions like this were moved outside of the component entirely. It's not seen very often in the app, but should be OK here.

function BookTravelButton({text}: BookTravelButtonProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID);
const policy = usePolicy(activePolicyID);
const [errorMessage, setErrorMessage] = useState('');
const [travelSettings] = useOnyx(ONYXKEYS.NVP_TRAVEL_SETTINGS);
const [account] = useOnyx(ONYXKEYS.ACCOUNT);
const primaryLogin = account?.primaryLogin;
const {setRootStatusBarEnabled} = useContext(CustomStatusBarAndBackgroundContext);

// Flag indicating whether NewDot was launched exclusively for Travel,
// e.g., when the user selects "Trips" from the Expensify Classic menu in HybridApp.
const [wasNewDotLaunchedJustForTravel] = useOnyx(ONYXKEYS.IS_SINGLE_NEW_DOT_ENTRY);

const bookATrip = useCallback(() => {
setErrorMessage('');

// The primary login of the user is where Spotnana sends the emails with booking confirmations, itinerary etc. It can't be a phone number.
if (!primaryLogin || Str.isSMSLogin(primaryLogin)) {
setErrorMessage(translate('travel.phoneError'));
return;
}

// Spotnana requires an address anytime an entity is created for a policy
if (isEmptyObject(policy?.address)) {
Navigation.navigate(ROUTES.WORKSPACE_PROFILE_ADDRESS.getRoute(policy?.id, Navigation.getActiveRoute()));
return;
}

const isPolicyProvisioned = policy?.travelSettings?.spotnanaCompanyID ?? policy?.travelSettings?.associatedTravelDomainAccountID;
if (policy?.travelSettings?.hasAcceptedTerms ?? (travelSettings?.hasAcceptedTerms && isPolicyProvisioned)) {
openTravelDotLink(policy?.id)
?.then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: (and we talked in person about this) but I think it would be better if openTravelDotLink() always returned a promise, no matter what, then this optional chaining isn't necessary. That would be easy to implement by doing something like policy?.id && openTravelDotLink()... and removing the early return inside of openTavelDotLink()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll open a separate PR for this one because changing this will require me to update the calls in other files, requiring me to fix ESLint checks, and I wouldn't extend the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

In some cases we can ignore eslint issues related to imports due to the large volume of a PR
But new PR is also good !

// When a user selects "Trips" in the Expensify Classic menu, the HybridApp opens the ManageTrips page in NewDot.
// The wasNewDotLaunchedJustForTravel flag indicates if NewDot was launched solely for this purpose.
if (!NativeModules.HybridAppModule || !wasNewDotLaunchedJustForTravel) {
return;
}

// Close NewDot if it was opened only for Travel, as its purpose is now fulfilled.
Log.info('[HybridApp] Returning to OldDot after opening TravelDot');
NativeModules.HybridAppModule.closeReactNativeApp(false, false);
setRootStatusBarEnabled(false);
})
?.catch(() => {
setErrorMessage(translate('travel.errorMessage'));
});
} else if (isPolicyProvisioned) {
navigateToAcceptTerms(CONST.TRAVEL.DEFAULT_DOMAIN);
} else {
// Determine the domain to associate with the workspace during provisioning in Spotnana.
// - If all admins share the same private domain, the workspace is tied to it automatically.
// - If admins have multiple private domains, the user must select one.
// - Public domains are not allowed; an error page is shown in that case.
const adminDomains = getAdminsPrivateEmailDomains(policy);
if (adminDomains.length === 0) {
Navigation.navigate(ROUTES.TRAVEL_PUBLIC_DOMAIN_ERROR);
} else if (adminDomains.length === 1) {
navigateToAcceptTerms(adminDomains.at(0) ?? CONST.TRAVEL.DEFAULT_DOMAIN);
} else {
Navigation.navigate(ROUTES.TRAVEL_DOMAIN_SELECTOR);
}
}
}, [policy, wasNewDotLaunchedJustForTravel, travelSettings, translate, primaryLogin, setRootStatusBarEnabled]);

return (
<>
{!!errorMessage && (
<DotIndicatorMessage
style={styles.mb1}
messages={{error: errorMessage}}
type="error"
/>
)}
<Button
text={text}
onPress={bookATrip}
accessibilityLabel={translate('travel.bookTravel')}
style={styles.w100}
success
large
/>
</>
);
}

BookTravelButton.displayName = 'BookTravelButton';

export default BookTravelButton;
4 changes: 3 additions & 1 deletion src/components/EmptyStateComponent/index.tsx
Original file line number Diff line number Diff line change
@@ -23,6 +23,7 @@ function EmptyStateComponent({
title,
titleStyles,
subtitle,
children,
headerStyles,
headerContentStyles,
lottieWebViewStyles,
@@ -99,7 +100,8 @@ function EmptyStateComponent({
<View style={[styles.emptyStateHeader(headerMediaType === CONST.EMPTY_STATE_MEDIA.ILLUSTRATION), headerStyles]}>{HeaderComponent}</View>
<View style={shouldUseNarrowLayout ? styles.p5 : styles.p8}>
<Text style={[styles.textAlignCenter, styles.textHeadlineH1, styles.mb2, titleStyles]}>{title}</Text>
{typeof subtitle === 'string' ? <Text style={[styles.textAlignCenter, styles.textSupporting, styles.textNormal]}>{subtitle}</Text> : subtitle}
<Text style={[styles.textAlignCenter, styles.textSupporting, styles.textNormal]}>{subtitle}</Text>
{children}
<View style={[styles.gap2, styles.mt5, !shouldUseNarrowLayout ? styles.flexRow : undefined]}>
{buttons?.map(({buttonText, buttonAction, success, icon, isDisabled}, index) => (
<View
3 changes: 2 additions & 1 deletion src/components/EmptyStateComponent/types.ts
Original file line number Diff line number Diff line change
@@ -15,7 +15,8 @@ type SharedProps<T> = {
SkeletonComponent: ValidSkeletons;
title: string;
titleStyles?: StyleProp<TextStyle>;
subtitle: string | React.ReactNode;
subtitle?: string;
children?: React.ReactNode;
buttons?: Button[];
containerStyles?: StyleProp<ViewStyle>;
headerStyles?: StyleProp<ViewStyle>;
55 changes: 15 additions & 40 deletions src/components/FeatureList.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react';
import type {ReactNode} from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra import
We can use ReactNode from React

import {View} from 'react-native';
import type {StyleProp, TextStyle, ViewStyle} from 'react-native';
import useLocalize from '@hooks/useLocalize';
@@ -7,7 +8,6 @@ import variables from '@styles/variables';
import type {TranslationPaths} from '@src/languages/types';
import type IconAsset from '@src/types/utils/IconAsset';
import Button from './Button';
import DotIndicatorMessage from './DotIndicatorMessage';
import type DotLottieAnimation from './LottieAnimations/types';
import MenuItem from './MenuItem';
import Section from './Section';
@@ -33,15 +33,6 @@ type FeatureListProps = {
/** Action to call on cta button press */
onCtaPress?: () => void;

/** Text of the secondary button button */
secondaryButtonText?: string;

/** Accessibility label for the secondary button */
secondaryButtonAccessibilityLabel?: string;

/** Action to call on secondary button press */
onSecondaryButtonPress?: () => void;

/** A list of menuItems representing the feature list. */
menuItems: FeatureListItem[];

@@ -60,30 +51,27 @@ type FeatureListProps = {
/** The style used for the title */
titleStyles?: StyleProp<TextStyle>;

/** The error message to display for the CTA button */
ctaErrorMessage?: string;

/** Padding for content on large screens */
contentPaddingOnLargeScreens?: {padding: number};

/** Custom content to display in the footer */
footer?: ReactNode;
};

function FeatureList({
title,
subtitle = '',
ctaText = '',
ctaAccessibilityLabel = '',
onCtaPress = () => {},
secondaryButtonText = '',
secondaryButtonAccessibilityLabel = '',
onSecondaryButtonPress = () => {},
ctaErrorMessage,
ctaText,
ctaAccessibilityLabel,
onCtaPress,
menuItems,
illustration,
illustrationStyle,
illustrationBackgroundColor,
illustrationContainerStyle,
titleStyles,
contentPaddingOnLargeScreens,
footer,
}: FeatureListProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
@@ -122,30 +110,17 @@ function FeatureList({
</View>
))}
</View>
{!!secondaryButtonText && (
{!!ctaText && (
<Button
text={secondaryButtonText}
onPress={onSecondaryButtonPress}
accessibilityLabel={secondaryButtonAccessibilityLabel}
style={[styles.w100, styles.mb3]}
text={ctaText}
onPress={onCtaPress}
accessibilityLabel={ctaAccessibilityLabel}
style={styles.w100}
success
large
/>
)}
{!!ctaErrorMessage && (
<DotIndicatorMessage
style={styles.mb1}
messages={{error: ctaErrorMessage}}
type="error"
/>
)}
<Button
text={ctaText}
onPress={onCtaPress}
accessibilityLabel={ctaAccessibilityLabel}
style={styles.w100}
success
large
/>
{!!footer && footer}
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually !!footer is also redundant here

</View>
</Section>
);
117 changes: 3 additions & 114 deletions src/libs/actions/Travel.ts
Original file line number Diff line number Diff line change
@@ -1,54 +1,10 @@
import {Str} from 'expensify-common';
import type {Dispatch, SetStateAction} from 'react';
import {Linking, NativeModules} from 'react-native';
import type {OnyxEntry, OnyxUpdate} from 'react-native-onyx';
import type {OnyxUpdate} from 'react-native-onyx';
import Onyx from 'react-native-onyx';
import type {LocaleContextProps} from '@components/LocaleContextProvider';
import * as API from '@libs/API';
import type {AcceptSpotnanaTermsParams} from '@libs/API/parameters';
import {WRITE_COMMANDS} from '@libs/API/types';
import {getMicroSecondOnyxErrorWithTranslationKey} from '@libs/ErrorUtils';
import Log from '@libs/Log';
import Navigation from '@libs/Navigation/Navigation';
import {getAdminsPrivateEmailDomains, getPolicy} from '@libs/PolicyUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type {TravelSettings} from '@src/types/onyx';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import {buildTravelDotURL, openTravelDotLink} from './Link';

let travelSettings: OnyxEntry<TravelSettings>;
Onyx.connect({
key: ONYXKEYS.NVP_TRAVEL_SETTINGS,
callback: (val) => {
travelSettings = val;
},
});

let activePolicyID: OnyxEntry<string>;
Onyx.connect({
key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
callback: (val) => {
activePolicyID = val;
},
});

let primaryLogin: string;
Onyx.connect({
key: ONYXKEYS.ACCOUNT,
callback: (val) => {
primaryLogin = val?.primaryLogin ?? '';
},
});

let isSingleNewDotEntry: boolean | undefined;
Onyx.connect({
key: ONYXKEYS.IS_SINGLE_NEW_DOT_ENTRY,
callback: (val) => {
isSingleNewDotEntry = val;
},
});

/**
* Accept Spotnana terms and conditions to receive a proper token used for authenticating further actions
@@ -98,76 +54,9 @@ function acceptSpotnanaTerms(domain?: string) {
API.write(WRITE_COMMANDS.ACCEPT_SPOTNANA_TERMS, params, {optimisticData, successData, failureData});
}

function handleProvisioningPermissionDeniedError(domain: string) {
Navigation.navigate(ROUTES.TRAVEL_DOMAIN_PERMISSION_INFO.getRoute(domain));
Onyx.merge(ONYXKEYS.TRAVEL_PROVISIONING, null);
}

function openTravelDotAfterProvisioning(spotnanaToken: string) {
Navigation.closeRHPFlow();
function cleanupTravelProvisioningSession() {
Onyx.merge(ONYXKEYS.TRAVEL_PROVISIONING, null);
Linking.openURL(buildTravelDotURL(spotnanaToken));
}

function provisionDomain(domain: string) {
Onyx.merge(ONYXKEYS.TRAVEL_PROVISIONING, null);
Navigation.navigate(ROUTES.TRAVEL_TCS.getRoute(domain));
}

function bookATrip(
translate: LocaleContextProps['translate'],
setCtaErrorMessage: Dispatch<SetStateAction<string>>,
setRootStatusBarEnabled: (isEnabled: boolean) => void,
ctaErrorMessage = '',
): void {
if (!activePolicyID) {
return;
}
if (Str.isSMSLogin(primaryLogin)) {
setCtaErrorMessage(translate('travel.phoneError'));
return;
}
const policy = getPolicy(activePolicyID);
if (isEmptyObject(policy?.address)) {
Navigation.navigate(ROUTES.WORKSPACE_PROFILE_ADDRESS.getRoute(activePolicyID, Navigation.getActiveRoute()));
return;
}

const isPolicyProvisioned = policy?.travelSettings?.spotnanaCompanyID ?? policy?.travelSettings?.associatedTravelDomainAccountID;
if (policy?.travelSettings?.hasAcceptedTerms ?? (travelSettings?.hasAcceptedTerms && isPolicyProvisioned)) {
openTravelDotLink(activePolicyID)
?.then(() => {
if (!NativeModules.HybridAppModule || !isSingleNewDotEntry) {
return;
}

Log.info('[HybridApp] Returning to OldDot after opening TravelDot');
NativeModules.HybridAppModule.closeReactNativeApp(false, false);
setRootStatusBarEnabled(false);
})
?.catch(() => {
setCtaErrorMessage(translate('travel.errorMessage'));
});
if (ctaErrorMessage) {
setCtaErrorMessage('');
}
} else if (isPolicyProvisioned) {
Onyx.merge(ONYXKEYS.TRAVEL_PROVISIONING, null);
Navigation.navigate(ROUTES.TRAVEL_TCS.getRoute(CONST.TRAVEL.DEFAULT_DOMAIN));
} else {
const adminDomains = getAdminsPrivateEmailDomains(policy);
let routeToNavigateTo;
if (adminDomains.length === 0) {
routeToNavigateTo = ROUTES.TRAVEL_PUBLIC_DOMAIN_ERROR;
} else if (adminDomains.length === 1) {
Onyx.merge(ONYXKEYS.TRAVEL_PROVISIONING, null);
routeToNavigateTo = ROUTES.TRAVEL_TCS.getRoute(adminDomains.at(0) ?? CONST.TRAVEL.DEFAULT_DOMAIN);
} else {
routeToNavigateTo = ROUTES.TRAVEL_DOMAIN_SELECTOR;
}
Navigation.navigate(routeToNavigateTo);
}
}

// eslint-disable-next-line import/prefer-default-export
export {acceptSpotnanaTerms, handleProvisioningPermissionDeniedError, openTravelDotAfterProvisioning, provisionDomain, bookATrip};
export {acceptSpotnanaTerms, cleanupTravelProvisioningSession};
Loading