-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix stuck Save loading state when cancelling travel monthly limit reduction #99344
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
Draft
MelvinBot
wants to merge
4
commits into
main
Choose a base branch
from
claude-travelBillingMonthlyLimitPressLoading
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+169
−0
Draft
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
a98065c
Disable instant press-loading on travel billing monthly limit form
MelvinBot 7614984
Add regression tests for cancelling vs confirming travel monthly limi…
MelvinBot 303db93
Fix: run oxfmt on WorkspaceTravelBillingMonthlyLimitPageTest to fix i…
MelvinBot 336fc3d
Type jest.requireActual for TravelBilling mock and drop eslint-disables
MelvinBot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
168 changes: 168 additions & 0 deletions
168
tests/ui/WorkspaceTravelBillingMonthlyLimitPageTest.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| import {act, fireEvent, render, screen, waitFor} from '@testing-library/react-native'; | ||
|
|
||
| import ComposeProviders from '@components/ComposeProviders'; | ||
| import {CurrencyListContextProvider} from '@components/CurrencyListContextProvider'; | ||
| import {LocaleContextProvider} from '@components/LocaleContextProvider'; | ||
| import {ModalActions, ModalProvider} from '@components/Modal/Global/ModalContext'; | ||
| import OnyxListItemProvider from '@components/OnyxListItemProvider'; | ||
|
|
||
| import {updateTravelBillingMonthlyLimit} from '@libs/actions/TravelBilling'; | ||
| import type * as TravelBillingActions from '@libs/actions/TravelBilling'; | ||
| import Navigation from '@libs/Navigation/Navigation'; | ||
| import createPlatformStackNavigator from '@libs/Navigation/PlatformStackNavigation/createPlatformStackNavigator'; | ||
| import {getTravelBillingCardSettingsKey} from '@libs/TravelBillingUtils'; | ||
|
|
||
| import type {SettingsNavigatorParamList} from '@navigation/types'; | ||
|
|
||
| import WorkspaceTravelBillingMonthlyLimitPage from '@pages/workspace/travel/WorkspaceTravelBillingMonthlyLimitPage'; | ||
|
|
||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import SCREENS from '@src/SCREENS'; | ||
|
|
||
| import {PortalProvider} from '@gorhom/portal'; | ||
| import {NavigationContainer} from '@react-navigation/native'; | ||
| import React from 'react'; | ||
| import Onyx from 'react-native-onyx'; | ||
|
|
||
| import * as TestHelper from '../utils/TestHelper'; | ||
| import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; | ||
|
|
||
| const POLICY_ID = 'testPolicy123'; | ||
| const FUND_ID = 999888; // Must match the literal returned by the useDefaultFundID mock below. | ||
| const CURRENT_LIMIT_CENTS = 10000; // $100.00 current monthly limit. | ||
| const REDUCED_LIMIT_CENTS = 5000; // $50.00 — a lower value that triggers the "Reduce limit" confirmation. | ||
|
|
||
| // jest.mock() factories are hoisted, so they can only reference literal values (not the constants above). | ||
| jest.mock('@hooks/useDefaultFundID', () => ({ | ||
| __esModule: true, | ||
| default: () => 999888, // Must match FUND_ID. | ||
| })); | ||
|
|
||
| jest.mock('@libs/actions/TravelBilling', () => { | ||
| const actual = jest.requireActual<typeof TravelBillingActions>('@libs/actions/TravelBilling'); | ||
| return { | ||
| ...actual, | ||
| updateTravelBillingMonthlyLimit: jest.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| const mockShowConfirmModal = jest.fn(); | ||
|
|
||
| jest.mock('@hooks/useConfirmModal', () => | ||
| jest.fn().mockImplementation(() => ({ | ||
| showConfirmModal: mockShowConfirmModal, | ||
| closeModal: jest.fn(), | ||
| })), | ||
| ); | ||
|
|
||
| const Stack = createPlatformStackNavigator<SettingsNavigatorParamList>(); | ||
|
|
||
| const getAmountLabel = () => TestHelper.translateLocal('iou.amount'); | ||
| const getSaveLabel = () => TestHelper.translateLocal('common.save'); | ||
|
|
||
| const renderPage = () => | ||
| render( | ||
| <ComposeProviders components={[OnyxListItemProvider, LocaleContextProvider, CurrencyListContextProvider]}> | ||
| <PortalProvider> | ||
| <ModalProvider> | ||
| <NavigationContainer> | ||
| <Stack.Navigator initialRouteName={SCREENS.WORKSPACE.TRAVEL_SETTINGS_MONTHLY_LIMIT}> | ||
| <Stack.Screen | ||
| name={SCREENS.WORKSPACE.TRAVEL_SETTINGS_MONTHLY_LIMIT} | ||
| component={WorkspaceTravelBillingMonthlyLimitPage} | ||
| initialParams={{policyID: POLICY_ID}} | ||
| /> | ||
| </Stack.Navigator> | ||
| </NavigationContainer> | ||
| </ModalProvider> | ||
| </PortalProvider> | ||
| </ComposeProviders>, | ||
| ); | ||
|
|
||
| /** Waits for the form to be interactive (Save button rendered), then returns the enabled Save button. */ | ||
| const waitForSaveButton = async () => { | ||
| await waitForBatchedUpdatesWithAct(); | ||
| await waitFor(() => { | ||
| expect(screen.getByRole(CONST.ROLE.BUTTON, {name: getSaveLabel()})).toBeOnTheScreen(); | ||
| }); | ||
| return screen.getByRole(CONST.ROLE.BUTTON, {name: getSaveLabel()}); | ||
| }; | ||
|
|
||
| /** Enter a lower monthly limit and press Save, which opens the (mocked) "Reduce limit" confirmation. */ | ||
| const reduceLimitAndSave = async () => { | ||
| const input = screen.getByLabelText(getAmountLabel()); | ||
| fireEvent.changeText(input, '50'); // $50.00, below the $100.00 current limit. | ||
| await waitForBatchedUpdatesWithAct(); | ||
|
|
||
| fireEvent.press(screen.getByRole(CONST.ROLE.BUTTON, {name: getSaveLabel()})); | ||
| await waitForBatchedUpdatesWithAct(); | ||
| }; | ||
|
|
||
| describe('WorkspaceTravelBillingMonthlyLimitPage', () => { | ||
| let goBackSpy: jest.SpyInstance; | ||
| let microtaskQueueSpy: jest.SpyInstance; | ||
|
|
||
| beforeAll(() => { | ||
| Onyx.init({keys: ONYXKEYS}); | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| // A real navigation stack is used so the goBack reference passed to the microtask queue is the spied one. | ||
| goBackSpy = jest.spyOn(Navigation, 'goBack').mockImplementation(() => {}); | ||
| microtaskQueueSpy = jest.spyOn(Navigation, 'setNavigationActionToMicrotaskQueue').mockImplementation((callback) => callback?.()); | ||
|
|
||
| // Seed a current monthly spend limit of $100.00 so reducing to $50.00 triggers the confirmation. | ||
| await act(async () => { | ||
| await Onyx.merge(getTravelBillingCardSettingsKey(FUND_ID), { | ||
| [CONST.TRAVEL.PROGRAM_TRAVEL_US]: {monthlySpendLimitPerUser: CURRENT_LIMIT_CENTS}, | ||
| }); | ||
| await waitForBatchedUpdatesWithAct(); | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| jest.clearAllMocks(); | ||
| jest.restoreAllMocks(); | ||
| await act(async () => { | ||
| await Onyx.clear(); | ||
| await waitForBatchedUpdatesWithAct(); | ||
| }); | ||
| }); | ||
|
|
||
| it('leaves the Save button enabled and does not update the limit when the reduce-limit confirmation is cancelled', async () => { | ||
| // Given the confirmation resolves as cancelled (any action other than CONFIRM) | ||
| mockShowConfirmModal.mockResolvedValue({action: ModalActions.CLOSE}); | ||
|
|
||
| renderPage(); | ||
| await waitForSaveButton(); | ||
|
|
||
| // When the user reduces the limit, presses Save, and cancels the confirmation | ||
| await reduceLimitAndSave(); | ||
|
|
||
| // Then the limit is not updated and the page does not navigate away | ||
| expect(updateTravelBillingMonthlyLimit).not.toHaveBeenCalled(); | ||
| expect(microtaskQueueSpy).not.toHaveBeenCalled(); | ||
| expect(goBackSpy).not.toHaveBeenCalled(); | ||
|
|
||
| // And the Save button remains on screen and enabled (no stuck loading spinner) | ||
| const saveButton = screen.getByRole(CONST.ROLE.BUTTON, {name: getSaveLabel()}); | ||
| expect(saveButton).toBeOnTheScreen(); | ||
| expect(saveButton).toBeEnabled(); | ||
| }); | ||
|
|
||
| it('updates the limit and navigates back when the reduce-limit confirmation is confirmed', async () => { | ||
| // Given the confirmation resolves as confirmed | ||
| mockShowConfirmModal.mockResolvedValue({action: ModalActions.CONFIRM}); | ||
|
|
||
| renderPage(); | ||
| await waitForSaveButton(); | ||
|
|
||
| // When the user reduces the limit, presses Save, and confirms the reduction | ||
| await reduceLimitAndSave(); | ||
|
|
||
| // Then the limit is updated with the reduced value (passing the previous limit) and the page navigates back | ||
| expect(updateTravelBillingMonthlyLimit).toHaveBeenCalledWith(FUND_ID, REDUCED_LIMIT_CENTS, CURRENT_LIMIT_CENTS); | ||
| expect(microtaskQueueSpy).toHaveBeenCalledWith(goBackSpy); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
@MelvinBot Could you please add focused regression coverage verifying that Cancel leaves the Save button enabled without updating the limit, while Confirm updates it and navigates back?
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.
Resolved ✅