-
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
Conversation
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
src/libs/actions/Travel.ts
Outdated
if (!activePolicyID) { | ||
return; | ||
} | ||
function bookATrip(policy: Policy, translate: LocaleContextProps['translate'], setCtaErrorMessage: Dispatch<SetStateAction<string>>, ctaErrorMessage = ''): void { |
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.
Do we really need full policy object if we use only activePolicyID
?
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, we check the policy address on it as well as the travelSettings
node
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.
this function will be refactored anyway, as part of this issue
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromeandroid-web.moviOS: Nativeios.movios.movMacOS: Chrome / Safariweb.mov2025-02-05.09.33.51.movMacOS: Desktopdesktop.mov |
src/pages/Search/EmptySearchView.tsx
Outdated
@@ -133,7 +134,13 @@ function EmptySearchView({type, hasResults}: EmptySearchViewProps) { | |||
buttons: [ | |||
{ | |||
buttonText: translate('search.searchResults.emptyTripResults.buttonText'), | |||
buttonAction: () => bookATrip(translate, setCtaErrorMessage, ctaErrorMessage), | |||
buttonAction: () => { | |||
const activePolicy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${activePolicyID}`]; |
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.
Maybe it's better to use getPolicy(activePolicyID, allPolicies)
?
Or usePolicy(activePolicyID)
?
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.
But probably
We will refactor this later
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.
Maybe it's better to use getPolicy(activePolicyID, allPolicies)
getPolicy
still could return undefined and we need to check the activePolicy
before calling bookATrip
. So it won't reduce the lines of code here
Or usePolicy(activePolicyID)
I think it would be redundant since we already have the data in allPolicies
ah yes, same thing, the primary login isn't loaded from Onyx when the function is called.. I'm sure this is reproducible in prod as well |
In that case, I think it's better to fix it in this PR ! |
I agree with this! |
Added WIP to this PR and asked @tgolen for an early review from him. If he approves the refactoring approach, I'll spend time on tests and recordings. |
headerStyles={[styles.emptyStateCardIllustrationContainer, {backgroundColor: colors.ice800}]} | ||
headerContentStyles={styles.pendingStateCardIllustration} | ||
title={translate('workspace.moreFeatures.companyCards.pendingFeedTitle')} | ||
subtitle={subtitle} | ||
body={body} |
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.
This is the actual change in this file, the others are ESLint fixes.
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 really like this cleanup! Seeing this code for the first time was good as it allowed me to point out some of the areas where comments could really help to explain why things are coded the way they are. That'll really help with future maintenance.
type="error" | ||
/> | ||
)} | ||
<BookTravelButton text={translate('search.searchResults.emptyTripResults.buttonText')} /> |
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 would also use children
instead of a text
prop as well. I think it's a shame that our normal <Button>
component doesn't use children
.
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.
Well, BookTravelButton
wraps our normal Button
so in order to add children, I need to refactor the Button
as well, 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.
OK, just a NAB then.
A preview of your ExpensifyHelp changes have been deployed to https://750e57b6.helpdot.pages.dev ⚡️ |
@cristipaval Looks like it's related with this |
src/components/BookTravelButton.tsx
Outdated
return; | ||
} | ||
|
||
// An address is required when the Spotnana entity is created for the policy |
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.
Why is an address required? (answer that in the comment please)
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 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()
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'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 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 !
type="error" | ||
/> | ||
)} | ||
<BookTravelButton text={translate('search.searchResults.emptyTripResults.buttonText')} /> |
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.
OK, just a NAB then.
> | ||
<Text> | ||
{translate('workspace.moreFeatures.companyCards.pendingFeedDescription')} | ||
<TextLink onPress={() => navigateToConciergeChat()}> {CONST?.CONCIERGE_CHAT_NAME}</TextLink>. |
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.
+1
Co-authored-by: Tim Golen <[email protected]>
Co-authored-by: Tim Golen <[email protected]>
Ready once again, @ZhenjaHorbach , @tgolen |
success | ||
large | ||
/> | ||
{!!footer && footer} |
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.
Actually !!footer is also redundant here
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Extra import
We can use ReactNode from React
Co-authored-by: Tim Golen <[email protected]>
And for me changes looks good ! |
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.
APPROVED
LGTM ! |
@MarioExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
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.
Code looks good! Is there any reason we're holding this or it can be merged already?
Go ahead and merge it, @MarioExpensify |
Nice, good work everybody! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/MarioExpensify in version: 9.0.95-0 🚀
|
This PR refactors the
Travel.ts
file to not Navigate from the action functions anymore.Explanation of Change
This PR moves the
Travel.bookATrip
function to a reusableBookTravelButton
component because we want to navigate only from the views and because we also want the code to be DRY.Fixed Issues
$ #55829
$ #55755
PROPOSAL:
Tests
Offline tests
Same as tests.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps./** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
Screen.Recording.2025-02-04.at.11.40.35.mov
MacOS: Chrome / Safari
Screen.Recording.2025-02-04.at.11.19.23.mov
MacOS: Desktop
Screen.Recording.2025-02-04.at.11.22.45.mov