-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Refactor bookATrip function #55852
Changes from all commits
d18f99b
9ae5e55
1a32608
bc569f5
d16741c
70d8109
358fe95
789a92b
fa0ddbb
22f3617
c6e30de
b633ba0
00d6012
243a09e
46b6485
f9428d6
f10ee2e
cf3e4bd
2331d13
ab8d5db
7dd2c55
f8e248a
14ee3bc
950ee37
75e4342
e43921a
820a0e5
f7f58db
b851e2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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)); | ||
}; | ||
|
||
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)) { | ||
cristipaval marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// 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); | ||
cristipaval marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import React from 'react'; | ||
import type {ReactNode} from 'react'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra import |
||
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} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually !!footer is also redundant here |
||
</View> | ||
</Section> | ||
); | ||
|
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 !
There was a problem hiding this comment.
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.