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 actions in Travel.tsx #55755

Open
cristipaval opened this issue Jan 24, 2025 · 4 comments
Open

Refactor actions in Travel.tsx #55755

cristipaval opened this issue Jan 24, 2025 · 4 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@cristipaval
Copy link
Contributor

Coming from the following comments:

Problem

The actions from Travel break the App guidelines and perform navigation, which should happen only from the views. They also break the naming convention for the acronyms.

Solution

Refactor the actions to comply with the App code style and guidelines.

@cristipaval cristipaval added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 24, 2025
@cristipaval cristipaval self-assigned this Jan 24, 2025
Copy link

melvin-bot bot commented Jan 24, 2025

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jan 25, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-25 01:14:15 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Refactor actions in Travel.tsx

What is the root cause of that problem?

Refactor

What changes do you think we should make in order to solve the problem?

  • We should remove bookATrip from TripReservation Utils.
  • Create a new function bookATrip inside EmptySearchView and ManageTrips.
    const [account] = useOnyx(ONYXKEYS.ACCOUNT);
    const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID);
    const [travelSettings] = useOnyx(ONYXKEYS.NVP_TRAVEL_SETTINGS);
    const [isSingleNewDotEntry] = useOnyx(ONYXKEYS.IS_SINGLE_NEW_DOT_ENTRY);

    const bookATrip = useCallback(() => {
        if (Str.isSMSLogin(account?.primaryLogin ?? '')) {
            setCTAErrorMessage(translate('travel.phoneError'));
            return;
        }
        const policy = PolicyUtils.getPolicy(activePolicyID);
        if (isEmptyObject(policy?.address)) {
            Navigation.navigate(ROUTES.WORKSPACE_PROFILE_ADDRESS.getRoute(activePolicyID, Navigation.getActiveRoute()));
            return;
        }
        if (!travelSettings?.hasAcceptedTerms) {
            Navigation.navigate(ROUTES.TRAVEL_TCS);
            return;
        }
        if (ctaErrorMessage) {
            setCTAErrorMessage('');
        }
        Link.openTravelDotLink(activePolicyID)
            ?.then(() => {
                if (!NativeModules.HybridAppModule || !isSingleNewDotEntry) {
                    return;
                }

                Log.info('[HybridApp] Returning to OldDot after opening TravelDot');
                NativeModules.HybridAppModule.closeReactNativeApp(false, false);
            })
            ?.catch(() => {
                setCTAErrorMessage(translate('travel.errorMessage'));
            });
    }, [account?.primaryLogin, activePolicyID, ctaErrorMessage, isSingleNewDotEntry, translate, travelSettings?.hasAcceptedTerms]);
  • Replace the old bookATrip function with the new one inside EmptySearchView and ManageTrips.
  • Update setCtaErrorMessage to setCTAErrorMessage in both components.

const [ctaErrorMessage, setCtaErrorMessage] = useState('');

const [ctaErrorMessage, setCtaErrorMessage] = useState('');

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA

What alternative solutions did you explore?

  • We can just remove the logic for navigation from bookATrip and add that in the UI component before we call bookATrip.

const policy = getPolicy(activePolicyID);
if (isEmptyObject(policy?.address)) {
Navigation.navigate(ROUTES.WORKSPACE_PROFILE_ADDRESS.getRoute(activePolicyID, Navigation.getActiveRoute()));
return;
}
if (!travelSettings?.hasAcceptedTerms) {
Navigation.navigate(ROUTES.TRAVEL_TCS);
return;
}

// In EmptySearchView and same should be done in other component.
                    buttons: [
                        {
                            buttonText: translate('search.searchResults.emptyTripResults.buttonText'),
                            buttonAction: () => {
                                if (isEmptyObject(policy?.address)) {
                                    Navigation.navigate(ROUTES.WORKSPACE_PROFILE_ADDRESS.getRoute(activePolicyID, Navigation.getActiveRoute()));
                                    return;
                                }
                                if (!travelSettings?.hasAcceptedTerms) {
                                    Navigation.navigate(ROUTES.TRAVEL_TCS);
                                    return;
                                }
                                TripsResevationUtils.bookATrip(translate, setCtaErrorMessage, ctaErrorMessage);
                            },
                            success: true,
                        },
                    ],
  • If we are going with this option then we also need to change setCtaErrorMessage param to setCTAErrorMessage.
    function bookATrip(translate: LocaleContextProps['translate'], setCtaErrorMessage: Dispatch<SetStateAction<string>>, ctaErrorMessage = ''): void {

Copy link
Contributor

⚠️ @Krishna2323 Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

@Krishna2323
Copy link
Contributor

PROPOSAL UPDATED

  • Added alternative solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2
Projects
None yet
Development

No branches or pull requests

3 participants