[Due for payment 2025-02-18] Refactor actions in Travel.tsx

cristipaval commented 3 weeks ago

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.

Issue Owner

Current Issue Owner: @strepanier03

melvin-bot[bot] commented 3 weeks ago

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 commented 3 weeks ago

🚨 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.

https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/pages/Search/EmptySearchView.tsx#L64 https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/pages/Travel/ManageTrips.tsx#L38

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.

https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/libs/TripReservationUtils.ts#L103-L111

// 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. https://github.com/Expensify/App/blob/b59f23b37d82a7b3de21a1a75e4c7479e37211f1/src/libs/TripReservationUtils.ts#L98

github-actions[bot] commented 3 weeks ago

⚠️ @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 commented 3 weeks ago

PROPOSAL UPDATED

Added alternative solution

cristipaval commented 2 weeks ago

This isn't external, at least for now. I'll come to this once I get the original PR merged.

melvin-bot[bot] commented 1 week ago

@strepanier03, @cristipaval Whoops! This issue is 2 days overdue. Let's get this updated quick!

cristipaval commented 1 week ago

This is merged. Closing, it was part of the PR that fixed an App issue

melvin-bot[bot] commented 6 days ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 6 days ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.95-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/55852

If no regressions arise, payment will be issued on 2025-02-18. :confetti_ball:

melvin-bot[bot] commented 6 days ago

@cristipaval @strepanier03 @cristipaval The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

Expensify / App