Add billing restriction checks to Search submit/approve/pay actions#84107
Add billing restriction checks to Search submit/approve/pay actions#84107
Conversation
The Search view's submit, approve, and pay actions bypass the shouldRestrictUserBillableActions check that exists in the normal report view flow. This adds the missing checks to both the individual action button handler (handleActionButtonPress) and the bulk action handlers in useSearchBulkActions (submit, approve, and pay). Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
@MelvinBot We should pass userBillingGraceEndPeriodCollection to shouldRestrictUserBillableActions |
…eActions Instead of relying on the deprecated module-level Onyx connection, explicitly pass the collection from useOnyx at all call sites. Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
Updated all
|
- Move userBillingGraceEndPeriodCollection useOnyx from HeaderFirstRow to ReportListItemHeader where it's actually used - Add userBillingGraceEndPeriodCollection to dependency arrays in handleApproveWithDEWCheck, onBulkPaySelected, and headerButtonsOptions - Apply Prettier formatting Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
Fixed the failing checks: ESLint errors (2):
TypeScript error:
ESLint warnings (3):
Prettier:
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
The previously failing checks (Prettier, Changed files ESLint, TypeScript) were fixed in the latest commit ( |
|
@MelvinBot Mark it as ready for review |
|
@dukenv0307 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] |
|
Marked this PR as ready for review. |
melvin-bot
bot
requested review from
JmillsExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const selectedPolicyIDList = selectedReports.length | ||
| ? selectedReports.map((report) => report.policyID) | ||
| : Object.values(selectedTransactions).map((transaction) => transaction.policyID); | ||
|
|
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The billing restriction check pattern (map policyIDs -> find restricted -> navigate to RESTRICTED_ACTION) is duplicated three times in this file: once in handleApproveWithDEWCheck, once in onBulkPaySelected, and once in the submit onSelected handler. All three blocks perform the same logic with only minor variations in how they obtain the list of policy IDs.
Extract a shared helper function, for example:
function getRestrictedPolicyID(
items: Array<{policyID?: string}>,
billingGracePeriods: OnyxCollection<BillingGraceEndPeriod>,
): string | undefined {
return items
.map((item) => item.policyID)
.find((policyID): policyID is string =>
!!policyID && shouldRestrictUserBillableActions(policyID, billingGracePeriods),
);
}Then replace each duplicated block with:
const restrictedPolicyID = getRestrictedPolicyID(selectedOptions, userBillingGraceEndPeriodCollection);
if (restrictedPolicyID) {
Navigation.navigate(ROUTES.RESTRICTED_ACTION.getRoute(restrictedPolicyID));
return;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed — extracted a getRestrictedPolicyID helper function that all 3 call sites now use.
src/libs/actions/Search.ts
Outdated
| onDelegateAccessRestricted?.(); | ||
| return; | ||
| } | ||
| if (snapshotReport.policyID && shouldRestrictUserBillableActions(snapshotReport.policyID, userBillingGraceEndPeriodCollection)) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The same billing restriction check is repeated identically three times inside the switch statement (for PAY, APPROVE, and SUBMIT cases):
if (snapshotReport.policyID && shouldRestrictUserBillableActions(snapshotReport.policyID, userBillingGraceEndPeriodCollection)) {
Navigation.navigate(ROUTES.RESTRICTED_ACTION.getRoute(snapshotReport.policyID));
return;
}Since the check is independent of the action type (it only depends on snapshotReport.policyID), move it before the switch statement so it runs once:
if (snapshotReport.policyID && shouldRestrictUserBillableActions(snapshotReport.policyID, userBillingGraceEndPeriodCollection)) {
Navigation.navigate(ROUTES.RESTRICTED_ACTION.getRoute(snapshotReport.policyID));
return;
}
switch (item.action) {
case CONST.SEARCH.ACTION_TYPES.PAY:
// ...Note: If the EXPORT_TO_ACCOUNTING action intentionally should NOT be restricted, guard the check with a condition excluding that action type.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed — moved the billing restriction check before the switch statement. Guarded it with an action type check so it only applies to PAY, APPROVE, and SUBMIT (not EXPORT_TO_ACCOUNTING or the default).
| const {isLargeScreenWidth} = useResponsiveLayout(); | ||
| const {currentSearchHash, currentSearchKey, currentSearchResults} = useSearchStateContext(); | ||
| const [isActionLoading] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportItem.reportID}`, {selector: isActionLoadingSelector}); | ||
| const [userBillingGraceEndPeriodCollection] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
This useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END) subscribes to an entire Onyx collection in a list item component that renders for every row in search results. Each row creates its own subscription to the same data, and any change to the billing grace period collection will cause every row to re-render.
Moreover, this subscription is unnecessary. The data is only passed through to handleActionButtonPress in Search.ts, which is an action file (not a React component). The shouldRestrictUserBillableActions function in SubscriptionUtils.ts already has a module-level Onyx.connect subscription and uses it as a default parameter value (line 508: userBillingGraceEndPeriodCollection: OnyxCollection<BillingGraceEndPeriod> = deprecatedUserBillingGraceEndPeriodCollection). This means Search.ts can call shouldRestrictUserBillableActions(policyID) without the second argument.
Remove this useOnyx call and the userBillingGraceEndPeriodCollection prop from handleActionButtonPress. The function will automatically use its module-level Onyx connection.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed — removed the per-row useOnyx subscription. handleActionButtonPress in Search.ts now relies on the module-level Onyx.connect default in shouldRestrictUserBillableActions. The explicit passing is still done in useSearchBulkActions.ts (a single-instance hook, not a per-row component).
| const theme = useTheme(); | ||
| const {currentSearchHash, currentSearchKey, currentSearchResults: snapshot} = useSearchStateContext(); | ||
| const {isLargeScreenWidth, shouldUseNarrowLayout} = useResponsiveLayout(); | ||
| const [userBillingGraceEndPeriodCollection] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
Same issue as in ExpenseReportListItem.tsx: useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END) subscribes to an entire Onyx collection in a per-row list item component, creating redundant subscriptions.
This subscription is unnecessary because the data is only forwarded to handleActionButtonPress in Search.ts, where shouldRestrictUserBillableActions already has a module-level Onyx.connect default. Remove this useOnyx call and the userBillingGraceEndPeriodCollection parameter from the handleActionButtonPress call.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed — same as ExpenseReportListItem.tsx, removed the per-row useOnyx subscription.
|
@MelvinBot Please check the comments above |
- Extract getRestrictedPolicyID helper in useSearchBulkActions.ts to eliminate duplicated billing restriction check pattern (CONSISTENCY-3) - Move billing restriction check before switch in handleActionButtonPress to avoid repeating the same check in PAY/APPROVE/SUBMIT (CONSISTENCY-3) - Remove per-row useOnyx subscriptions for billing grace period collection from ExpenseReportListItem and ReportListItemHeader to avoid redundant subscriptions in list item components (PERF-11) Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
Addressed all 4 automated review comments: CONSISTENCY-3 ( CONSISTENCY-3 ( PERF-11 ( |
Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
Fixed the failing |
|
I ran out of time before I could finish helping you. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in the #melvinbot-ai. Run details: https://github.com/Expensify/Expensify/actions/runs/22657222030 |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Great add. I agree we need to lock this down.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-03-04.at.22.34.58.movAndroid: mWeb ChromeScreen.Recording.2026-03-04.at.22.25.57.moviOS: HybridAppScreen.Recording.2026-03-04.at.21.36.43.moviOS: mWeb SafariScreen.Recording.2026-03-04.at.22.35.24.movMacOS: Chrome / SafariScreen.Recording.2026-03-04.at.22.21.00.mov |
|
@inimaga all yours! |
|
@inimaga can you please review this PR? Thanks |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
… Search actions Aligns the check ordering in handleActionButtonPress with MoneyReportHeader.tsx so delegate access is handled before the billing restriction redirect for PAY and APPROVE actions. Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
The failing The test runner failed to download required GitHub Actions due to network errors before the tests could even start: No code fix is needed. Please re-run the failed job to resolve this. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @inimaga has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.3.34-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.34-2 🚀
|
5 participants
Explanation of Change
When users have an active billing grace period, the Restricted Action page should appear when submitting, approving, or paying reports from the Reports > Expenses (Search) view. However, the Search bulk actions and individual action button handlers were missing the
shouldRestrictUserBillableActionscheck that the normal report view flow (viaIOU/index.ts) correctly enforces.This PR adds
shouldRestrictUserBillableActionschecks to:handleActionButtonPressinSearch.ts— for individual PAY, APPROVE, and SUBMIT action buttons in the Search viewhandleApproveWithDEWCheckinuseSearchBulkActions.ts— for bulk approveonBulkPaySelectedinuseSearchBulkActions.ts— for bulk payonSelectedinuseSearchBulkActions.ts— for bulk submitWhen a restricted policy is found, navigation redirects to
ROUTES.RESTRICTED_ACTIONinstead of proceeding with the action.Fixed Issues
$ #84030
PROPOSAL: #84030 (comment)
Tests
Onyx.merge('sharedNVP_private_billingGracePeriodEnd_[replaceWithYourAccountID]', {value: 1});in consoleOffline tests
QA Steps
Onyx.merge('sharedNVP_private_billingGracePeriodEnd_[replaceWithYourAccountID]', {value: 1});in consolePR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari