-
Notifications
You must be signed in to change notification settings - Fork 3.7k
feat: Support partial approval from Reports page #85194
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
Changes from all commits
8d0212a
2b784e8
776d4f4
63ee984
5b6dfe2
63ce308
b19bbaf
f2a300e
100c4d4
40cf57a
6c99580
b6fed9f
07e8c4e
ec2d292
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 |
|---|---|---|
|
|
@@ -314,7 +314,7 @@ describe('handleActionButtonPress', () => { | |
| const snapshotReport = mockSnapshotForItem?.data?.[`${ONYXKEYS.COLLECTION.REPORT}${mockReportItemWithHold.reportID}`] ?? {}; | ||
| const snapshotPolicy = mockSnapshotForItem?.data?.[`${ONYXKEYS.COLLECTION.POLICY}${mockReportItemWithHold.policyID}`] ?? {}; | ||
|
|
||
| test('Should navigate to item when report has one transaction on hold', () => { | ||
| test('Should not navigate to item when report has one transaction on hold and action is approve', () => { | ||
|
Contributor
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. Should we also test that the Hold modal is shown? And test what happens when the two different buttons on the modal are pressed?
Contributor
Author
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. @puneetlath I updated the test to verify the |
||
| const goToItem = jest.fn(() => {}); | ||
| handleActionButtonPress({ | ||
| hash: searchHash, | ||
|
|
@@ -327,8 +327,28 @@ describe('handleActionButtonPress', () => { | |
| ownerBillingGracePeriodEnd: undefined, | ||
| amountOwed: undefined, | ||
| userBillingGracePeriodEnds: undefined, | ||
| onHoldMenuOpen: jest.fn(), | ||
| }); | ||
| expect(goToItem).toHaveBeenCalledTimes(1); | ||
| expect(goToItem).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('Should open the hold menu when the report has one transaction on hold and action is approve', () => { | ||
| const onHoldMenuOpen = jest.fn(); | ||
| handleActionButtonPress({ | ||
| hash: searchHash, | ||
| item: mockReportItemWithHold, | ||
| goToItem: jest.fn(), | ||
| snapshotReport: snapshotReport as Report, | ||
| snapshotPolicy: snapshotPolicy as Policy, | ||
| lastPaymentMethod: mockLastPaymentMethod, | ||
| personalPolicyID: undefined, | ||
| userBillingGracePeriodEnds: undefined, | ||
| ownerBillingGracePeriodEnd: undefined, | ||
| amountOwed: undefined, | ||
| onHoldMenuOpen, | ||
| }); | ||
|
|
||
| expect(onHoldMenuOpen).toHaveBeenCalledWith(mockReportItemWithHold, CONST.IOU.REPORT_ACTION_TYPE.APPROVE); | ||
| }); | ||
|
|
||
| test('Should not navigate to item when the hold is removed', () => { | ||
|
|
||
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.
The new approve-with-hold path returns immediately after
onHoldMenuOpen?.(...), but that callback is optional and several existing callers still omit it (for examplesrc/components/SelectionListWithSections/Search/TransactionListItem.tsxcallshandleActionButtonPresswithoutonHoldMenuOpen). Because this same commit also allowsAPPROVEon held reports ingetActions(), those call sites can now show an Approve action that does nothing (no navigation and no approval request) when a held expense is present.Useful? React with 👍 / 👎.
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.
@nkdengineer thoughts on this 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.
I updated to call
goToItemifonHoldMenuOpenis not present.