Gate "Submit to my employer" on the workspace billing restriction - #100584
Gate "Submit to my employer" on the workspace billing restriction#100584MelvinBot wants to merge 7 commits into
Conversation
The single-workspace and preferred-workspace fast paths skip the destination picker, which is the only place the SUBMIT flow checked shouldRestrictUserBillableActions. Apply the same gate before those fast paths navigate, matching the sibling CATEGORIZE branch. Co-authored-by: Maruf Sharifi <marufsharifi@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch because 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. |
|
🤖 Android test steps verification results: Regression path verified: with a single healthy workspace, "Submit to my employer" from the self DM correctly binds to that workspace's confirmation page (not "None") and submits cleanly. The restricted precondition (single workspace with an expired required payment) cannot be created from a fresh test account and is reported as blocked rather than fabricated.
No console errors were observed. The billing-restricted path still needs a human with an account whose only workspace has an expired required payment; the unit tests in |
|
🤖 Web test steps verification results: Regression path passes on dev NewDot web: "Submit to my employer" from the self DM lands on the confirmation page correctly bound to the single healthy workspace, not "None". The Restricted-screen precondition (single workspace with an expired required payment) could not be created because this session's account had zero workspaces, which is outside what UI driving can simulate.
Console errors could not be read directly — The billing-restricted path still needs a human with an account whose only workspace has an expired required payment. It matches the Android run, where the same step was blocked for the same reason; today the only coverage of it is the unit tests in view run · no recording available |
|
@MelvinBot Mark the pr as ready for review |
|
@marufsharifi 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. One thing to fix before a reviewer picks it up: the Tests, Offline tests, and QA Steps sections still hold my |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Exactly one accessible workspace: skip the destination picker and submit straight to that workspace. | ||
| if (filteredPoliciesCount === 1 && firstPolicyID) { | ||
| // The destination picker we skip here is where the billing restriction is normally enforced, so gate it here too. | ||
| const firstPolicy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${firstPolicyID}`]; |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This billing-restriction gate is added twice in this PR (here and again in the isRestrictedToPreferredPolicy branch below), and it repeats a pattern that already appears four other times in this file (lines ~3295, ~3312, ~3329, ~12288): look up the policy from allPolicies, call shouldRestrictUserBillableActions(...), and Navigation.navigate(ROUTES.RESTRICTED_ACTION.getRoute(...)). Duplicating the same guard raises the risk of the copies drifting apart as the restriction logic evolves.
Extract a small helper and call it from each site, e.g.:
function navigateToRestrictedActionIfNeeded(policyID: string | undefined): boolean {
const policy = policyID ? allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`] : undefined;
if (policy && shouldRestrictUserBillableActions(policy, ownerBillingGracePeriodEnd, userBillingGracePeriodEnds, amountOwed, currentUserAccountID)) {
Navigation.navigate(ROUTES.RESTRICTED_ACTION.getRoute(policyID));
return true;
}
return false;
}
// then at each call site:
if (navigateToRestrictedActionIfNeeded(firstPolicyID)) {
return;
}Reviewed at: 3e68c46 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Done in 1905451. Extracted navigateToRestrictedActionIfNeeded and routed all six sites through it — the four you listed plus the two this PR added.
One change from your sketch: the helper takes the resolved policy instead of a policyID, so it can't reach for allPolicies itself. That also settles the sibling comment about the module cache being stale, and it matches the four pre-existing sites, which already had a Policy on hand.
The CREATE_NEW_EXPENSE site keeps its policy.type !== CONST.POLICY.TYPE.PERSONAL check at the call site, since that condition is specific to that entry point.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e68c46e45
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| const firstPolicy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${firstPolicyID}`]; | ||
| if (firstPolicy && shouldRestrictUserBillableActions(firstPolicy, ownerBillingGracePeriodEnd, userBillingGracePeriodEnds, amountOwed, currentUserAccountID)) { |
There was a problem hiding this comment.
When the caller's policy selector has received a newly loaded or updated workspace before this module-level Onyx.connect cache has caught up, firstPolicy is undefined and the new check fails open, so the function binds the draft to the restricted workspace and navigates to confirmation. This file already documents that allPolicies is independently timed and can be stale or missing policies available in a caller's snapshot; pass the selected policy from the callers alongside firstPolicyID (and do the same for the preferred-policy branch) instead of reading this cache.
AGENTS.md reference: AGENTS.md:L1-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 1905451. The gate now takes a resolved policy from the caller, and allPolicies is gone from both branches.
Single-workspace branch: createFilteredPoliciesInfoSelector now also returns firstPolicy, and the callers pass it alongside firstPolicyID. Because both come out of the same pass over the same snapshot, firstPolicy is set whenever firstPolicyID is — so this path can no longer fail open.
Preferred-workspace branch: the three callers (ChatActionableButtons, DynamicReportDetailsPage, MoneyRequestHeaderSecondaryActions) subscribe to the preferred policy key and pass preferredPolicy. usePreferredPolicy only carries the ID from the security group, and it has ~39 consumers, so I added the subscription in the three callers rather than widening the hook.
Both new params are optional, matching preferredPolicyID. Say the word if you'd rather firstPolicy were required to pair with firstPolicyID — it's the safer contract, but it means adding firstPolicy: undefined to roughly 29 existing test call sites.
|
@MelvinBot address the comments. |
Two review comments on the previous commit: CONSISTENCY-3 (github-actions): the gate was written out six times in ReportUtils.ts. Extract navigateToRestrictedActionIfNeeded and call it from all six sites so the restriction logic can only evolve in one place. P1 (codex): the two new gates resolved the policy from the module-level allPolicies cache, which this file already documents as independently timed and able to lag a caller's own snapshot. When it lags, the policy is undefined and the gate fails open, binding the draft to the restricted workspace. The helper now takes a resolved policy, and the callers pass the snapshot they already hold: - createFilteredPoliciesInfoSelector also returns firstPolicy, so the gate reads the same snapshot that produced filteredPoliciesCount/firstPolicyID. If firstPolicyID is set, firstPolicy is set by construction. - The three callers subscribe to the preferred policy key and pass preferredPolicy for the preferred-workspace branch. Co-authored-by: Maruf Sharifi <marufsharifi@users.noreply.github.com>
|
🤖 Addressed both review comments in 1905451 and replied in each thread. CONSISTENCY-3 ( P1 (
Checks run locally on this branch
Two things still open, both for a human:
|
|
@youssef-lr 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] |
|
No code fix is needed — nothing is actually broken. Both failing checks are human gates, not automated failures:
Every automated check passes: ESLint, Oxfmt, typecheck, spellcheck, Bun tests, all 8 Jest shards, Storybook tests, knip, unused styles, React Compiler Compliance, and all four builds. The two perf-test jobs are still There is nothing I can push to make these two go green — they clear when a reviewer posts the checklist and approves. |
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 workspace `firstPolicyID` refers to, from the same caller snapshot that produced the count above. */ | ||
| firstPolicy?: OnyxEntry<Policy>; |
There was a problem hiding this comment.
firstPolicy? being optional makes the security gate fail open silently — and the second new test already demonstrates it.
navigateToRestrictedActionIfNeeded opens with if (!policy || ...) return false. So a caller that passes firstPolicyID and forgets firstPolicy silently disables the gate — no type error, no lint error, no runtime signal. The expense goes straight to the restricted workspace.
This is already happening in this PR's own test at CreateDraftTransactionTest.ts:624-625, which passes filteredPoliciesCount: 1 + firstPolicyID with no firstPolicy. It only passes because submitDestination is unset. Add submitDestination: EMPLOYER to that test later and it flips from "asserts the gate works" to "asserts nothing", with no failure.
firstPolicyID is now fully derivable as firstPolicy?.id, so drop the parallel ID and make the policy the single source of truth:
| /** The workspace `firstPolicyID` refers to, from the same caller snapshot that produced the count above. */ | |
| firstPolicy?: OnyxEntry<Policy>; | |
| filteredPoliciesCount: number; | |
| /** The single accessible workspace, from the same caller snapshot that produced the count above. */ | |
| firstPolicy: OnyxEntry<Policy>; |
then at L12398:
if (filteredPoliciesCount === 1 && firstPolicy) {
if (navigateToRestrictedActionIfNeeded(firstPolicy, ...)) {
return;
}
const policyExpenseReport = getPolicyExpenseChat(deprecatedCurrentUserAccountID, firstPolicy.id);Now "I have a workspace to submit to" and "I have the policy to gate on" are the same fact — it becomes impossible to have one without the other.
There was a problem hiding this comment.
Done in 121218a — firstPolicyID is gone, firstPolicy is the only handle on that workspace, and the branch is now filteredPoliciesCount === 1 && firstPolicy.
Two follow-ons from making the policy the source of truth:
- The CATEGORIZE branch also read
firstPolicyID; it now readsfirstPolicy?.id, so there is no remaining way to name that workspace without holding the policy. firstPolicyis a required key (firstPolicy: BillingRestrictionPolicy | undefined, notfirstPolicy?:), so omitting it is a type error rather than a silently ungated call. That is the 29 test call sites you flagged earlier — updated in this commit.
The test you called out is now firstPolicy: restrictedPolicy with filteredPoliciesCount: 1, so adding submitDestination: EMPLOYER to it would exercise the single-workspace gate rather than asserting nothing.
| /** The preferred workspace itself, so the billing gate reads the caller's snapshot rather than this file's `allPolicies` cache. */ | ||
| preferredPolicy?: OnyxEntry<Policy>; |
There was a problem hiding this comment.
Same fail-open shape for preferredPolicy — but here the ID and the policy genuinely come from different sources, so they need to be bound at the type level.
preferredPolicyID comes from the security group (usePreferredPolicy), preferredPolicy from an Onyx read. A caller can supply the first without the second and the gate at L12438 no-ops.
Make the trio inseparable with a discriminated union rather than three independent optionals:
| /** The preferred workspace itself, so the billing gate reads the caller's snapshot rather than this file's `allPolicies` cache. */ | |
| preferredPolicy?: OnyxEntry<Policy>; | |
| } & ( | |
| | { | |
| isRestrictedToPreferredPolicy: true; | |
| preferredPolicyID: string; | |
| /** Required alongside the ID: the gate at the preferred-policy fast path no-ops without it. */ | |
| preferredPolicy: OnyxEntry<Policy>; | |
| } | |
| | {isRestrictedToPreferredPolicy?: false; preferredPolicyID?: undefined; preferredPolicy?: undefined} | |
| ); |
At minimum, make preferredPolicy non-optional. As written, the compiler cannot tell the difference between "not restricted" and "restricted but I forgot the policy" — and those have opposite security outcomes.
There was a problem hiding this comment.
Agreed on the problem; fixed in 121218a, but with one non-nullable field instead of the discriminated union.
The union as written does not compile at the three call sites. All of them do:
const {isRestrictedToPreferredPolicy, preferredPolicyID} = usePreferredPolicy();isRestrictedToPreferredPolicy is a boolean, not a literal, so TypeScript cannot narrow the object literal to either arm — boolean is assignable to neither true nor false | undefined. Making it work would mean changing usePreferredPolicy to return a discriminated result, and it has ~39 consumers.
The union also would not have closed the hole on its own: its true arm types the policy as OnyxEntry<Policy>, which includes undefined, so {isRestrictedToPreferredPolicy: true, preferredPolicyID: 'x', preferredPolicy: undefined} still type-checks and still no-ops the gate.
So the trio collapsed into one field:
/** The preferred workspace, set only when the user is restricted to submitting there. ... */
restrictedPreferredPolicy?: BillingRestrictionPolicy;Non-nullable, and the branch is just if (restrictedPreferredPolicy). The ID comes off .id, so there is nothing left to forget. Callers pass restrictedPreferredPolicy: isRestrictedToPreferredPolicy ? preferredPolicy : undefined — one place where the flag and the policy meet, instead of three parallel params that can disagree.
One behavior change worth naming: when the user is restricted but the policy has not loaded from Onyx yet, the fast path is now skipped and the flow falls through to the participant picker, which enforces the restriction itself. Previously it took the fast path ungated. Practically this is a load-order edge — getPolicyExpenseChat would not have found the destination chat in that window either — and falling back to the picker is the safe direction.
| /** The first policy itself, so callers can gate on it without re-reading a separately-timed policy cache */ | ||
| firstPolicy: OnyxEntry<Policy>; |
There was a problem hiding this comment.
Putting the whole Policy in this selector's output puts an employeeList/customUnits deep-compare in a hot path — which is exactly what the sibling selector 40 lines above warns against.
useOnyx wraps every selector in createMemoizedSelector, which falls back to fast-equals deepEqual on the output (react-native-onyx/dist/createMemoizedSelector.js:29). The output went from {number, string} to a full Policy, so every write anywhere in the POLICY collection now deep-walks employeeList, customUnits, rules, taxRates, connections, mccGroup.
This file already documents the hazard, at L310:
// Fixed-size output: same shape on 5 workspaces or 5000, so no employeeList/customUnits deepEqual and no growing ID list
and the convention for "I need policy fields in a selector" is a Pick projection — policyMapper / type PolicySelector = Pick<Policy, 'type' | 'role' | ...> at L282, and again in usePersonalPolicy.ts:8.
The gate needs exactly two fields. shouldRestrictUserBillableActions only calls isPolicyOwner(policy, id), which reads policy.ownerAccountID (PolicyUtils.ts#L855), plus policy.id for the route:
| /** The first policy itself, so callers can gate on it without re-reading a separately-timed policy cache */ | |
| firstPolicy: OnyxEntry<Policy>; | |
| /** Only the fields the billing gate needs, so the output stays fixed-size (see `policyMapper` above) */ | |
| firstPolicy: Pick<Policy, 'id' | 'ownerAccountID'> | undefined; |
and at L368:
firstPolicy = {id: policy.id, ownerAccountID: policy.ownerAccountID};Then widen shouldRestrictUserBillableActions's parameter to Pick<Policy, 'id' | 'ownerAccountID'> — no cast needed, and it documents what the check actually depends on.
Symptom if unaddressed: dropped frames scrolling a chat with several track-expense whispers on a high-traffic account with a large employeeList, and a stutter in the report-details RHP on any workspace write.
There was a problem hiding this comment.
Good catch — fixed in 121218a. Confirmed createMemoizedSelector falls back to deepEqual on the output, so the full Policy was putting employeeList/customUnits in the comparison on every POLICY write.
The projection is a named type next to the check it serves, in SubscriptionUtils.ts:
type BillingRestrictionPolicy = Pick<Policy, 'id' | 'ownerAccountID'>;and shouldRestrictUserBillableActions now takes OnyxEntry<Pick<Policy, 'ownerAccountID'>> — isPolicyOwner widened to match, so no cast anywhere. As you said, ownerAccountID is all the check reads; id is only there for the route.
Applied in two places, not one. firstPolicy in createFilteredPoliciesInfoSelector is the one you flagged. The preferredPolicy read in all three callers had the same problem — a bare useOnyx(POLICY + id) pulling the whole policy — so it now goes through a module-level billingRestrictionPolicySelector.
| isSmallScreenWidth, | ||
| isRestrictedToPreferredPolicy, | ||
| preferredPolicyID, | ||
| preferredPolicy, |
There was a problem hiding this comment.
Adding the whole preferredPolicy object to this dependency array rebuilds the entire report-details menu on any field change to that policy.
This memo produces the full menu-item list. With the object itself as a dep, it now invalidates when a member is added, a category is edited, or pendingFields flips during any workspace write — none of which affect the menu.
Once the selector returns a Pick projection (see my comment on selectors/Policy.ts:352), depend on the scalar the gate actually reads instead:
| preferredPolicy, | |
| preferredPolicyID, | |
| preferredPolicy?.ownerAccountID, |
Same applies to filteredPoliciesInfo?.firstPolicy at L819.
There was a problem hiding this comment.
Fixed in 121218a, at the Onyx read rather than in the dependency array.
Swapping the dep to preferredPolicy?.ownerAccountID while the memo body still closes over preferredPolicy trips react-hooks/exhaustive-deps, and it leaves the underlying read pulling the whole policy. So instead both reads are projected:
preferredPolicynow usesbillingRestrictionPolicySelector, a module-levelPick<Policy, 'id' | 'ownerAccountID'>projection.filteredPoliciesInfo.firstPolicyis the same projection, per your comment onselectors/Policy.ts:352.
Because useOnyx wraps selectors in createMemoizedSelector, a fixed-size output means deepEqual returns the previous reference when nothing relevant changed. So depending on the object is now equivalent to depending on the scalars: a member add, a category edit, or a pendingFields flip no longer invalidates the menu.
filteredPoliciesInfo?.firstPolicyID is out of the dep array (the param is gone), and so is preferredPolicyID — it was only there to be passed through, and ESLint flagged it as unnecessary once restrictedPreferredPolicy replaced it.
|
@MelvinBot check the new comments. |
…ted fast path - Drop firstPolicyID; firstPolicy is now the only handle on the single accessible workspace, so the count and the policy cannot come apart. - Collapse isRestrictedToPreferredPolicy/preferredPolicyID/preferredPolicy into one non-nullable restrictedPreferredPolicy, so the preferred-workspace fast path cannot be entered without the policy its gate reads. - Project both to Pick<Policy, 'id' | 'ownerAccountID'> (BillingRestrictionPolicy) so the useOnyx selectors keep a fixed-size output and no employeeList / customUnits deep-compare lands in a hot path. Co-authored-by: Maruf Sharifi <marufsharifi@users.noreply.github.com>
|
Addressed all four review comments in
One deviation to flag. The discriminated union does not compile. All three callers destructure restrictedPreferredPolicy?: BillingRestrictionPolicy;One behavior change worth naming. If the user is restricted to a preferred workspace but that policy has not loaded from Onyx yet, the fast path is now skipped and the flow falls through to the participant picker, which enforces the restriction itself. Previously it took the fast path ungated. This is a load-order edge — Verification
Full table is in the PR description. |
|
@MelvinBot Merge the main |
…erRestrictedWorkspace # Conflicts: # src/pages/DynamicReportDetailsPage.tsx
|
Merged One conflict, in Everything else auto-merged. Verification on the merge commit
Full table is in the PR description. |
…erRestrictedWorkspace Co-authored-by: Maruf Sharifi <marufsharifi@users.noreply.github.com>
Co-authored-by: Maruf Sharifi <marufsharifi@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
🤖 Fixed the failing check and pushed to this branch. What was failing: Fix: ran
The only other red check is Local verification on the fix commit
view run · no recording available |
…dWorkspace' into claude-submitToEmployerRestrictedWorkspace Co-authored-by: Maruf Sharifi <marufsharifi@users.noreply.github.com>
|
Merged One conflict-free auto-merge in Two merge commits rather than one — a formatting fix ( Verified after the merge
I included |
|
@MelvinBot Merge the main. |
|
I didn't merge — Why: #100164 ("Move expense-flow starters out of ReportUtils into actions/IOU to break import cycles") moved both functions this PR rewrites out of Only 2 files conflict ( What the resolution needs
No changes to The guardrail is that I don't resolve conflicts where |
|
@MelvinBot Merge the main |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@MelvinBot Merge the main |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@MelvinBot refill the pr author checklist with verified one DetailsExplanation of Change"Submit to my employer" from a self DM could create an expense on a workspace with an expired required payment, instead of showing the "Restricted" screen. The billing-restriction gate for this flow lived only in the destination picker, in the row handler at That is why the bug needs the exact preconditions in the report — one workspace, and it's expired. With two or more workspaces you hit the picker and correctly land on the Restricted screen. This change applies the same gate the sibling
All four inputs ( Fixed Issues$ #99325 TestsPrecondition: The user has only an expired workspace payment requirement.
Offline testsSame as Tests. QA StepsSame as Tests.
PR Author Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safari |
2 participants
Explanation of Change
"Submit to my employer" from a self DM could create an expense on a workspace with an expired required payment, instead of showing the "Restricted" screen.
The billing-restriction gate for this flow lived only in the destination picker, in the row handler at
src/pages/iou/request/ParticipantSearchResults.tsx. ButcreateDraftTransactionAndNavigateToParticipantSelectorhas a fast path that skips that picker when the user belongs to exactly one workspace — it binds the draft straight to that workspace's expense chat and navigates to the confirmation page. Skip the picker, skip the gate. Nothing downstream re-checks: the confirmation page,confirmAction, the validation hook, andrequestMoneycontain no billing-restriction checks, so the expense is created against the expired workspace.That is why the bug needs the exact preconditions in the report — one workspace, and it's expired. With two or more workspaces you hit the picker and correctly land on the Restricted screen.
This change applies the same gate the sibling
CATEGORIZEbranch already applies, before the fast paths navigate:EMPLOYERfast path now resolves the policy for the one accessible workspace and, whenshouldRestrictUserBillableActions(...)is true, navigates toROUTES.RESTRICTED_ACTIONand returns.All four inputs (
ownerBillingGracePeriodEnd,userBillingGracePeriodEnds,amountOwed,currentUserAccountID) were already threaded into this function for theCATEGORIZEcheck, so no new plumbing was needed. Fixing it inside this helper covers all three "Submit to my employer" entry points at once, since they all call it: the self-DM whisper buttons, the report-details menu, and the expense header menu.Structure of the gate (after review)
The check is a shared helper,
navigateToRestrictedActionIfNeeded, used by all six billable entry points inReportUtils— the four that already had the pattern inline plus the two this PR adds.It takes a resolved policy rather than an ID, so it never reads this file's independently-timed
allPoliciescache, which can lag a caller's own snapshot and let the gate fail open.Each fast path has exactly one handle on its workspace, and that handle is the policy:
firstPolicy(no parallelfirstPolicyID) for the single-workspace path.restrictedPreferredPolicy, one non-nullable field replacing theisRestrictedToPreferredPolicy/preferredPolicyID/preferredPolicytrio, for the preferred-workspace path.So "I have a workspace to submit to" and "I have the policy to gate on" are the same fact, and a caller cannot enter a fast path with the gate silently disabled.
Both are typed as
BillingRestrictionPolicy—Pick<Policy, 'id' | 'ownerAccountID'>, the only fields the restriction depends on. That keeps theuseOnyxselectors that produce them fixed-size, so noemployeeList/customUnitsdeep-compare lands in the report-details or track-expense-whisper render paths.Alternative considered
Filtering restricted workspaces out of the
filteredPoliciesCountselector. Rejected: it would push the single-expired-workspace case into thecount === 0branch, which silently spins up a brand-new Submit workspace instead of showing the Restricted screen.Automated tests added
Two regression tests in
tests/actions/IOU/CreateDraftTransactionTest.ts, both confirmed to fail onmainand pass with this change:should show the restricted action screen when the only accessible workspace has an expired required payment— also asserts the draft is left unbound, so nothing can be submitted to the restricted workspace.should show the restricted action screen when the preferred workspace has an expired required paymentThe existing happy-path test (
should bind the draft transaction to the destination chat when exactly one workspace is accessible) still passes, so the non-restricted flow is unchanged.Fixed Issues
$ #99325
PROPOSAL: #99325 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate the change works.
//
// Reaching the buggy state needs a workspace with an expired required payment, which depends on billing NVPs and is not
// reproducible from a fresh test account. Suggested coverage for whoever has such an account:
// 1. As the owner of a single workspace with an expired required payment, open the self DM.
// 2. Create a new expense -> "Submit to my employer" -> enter a merchant -> Create expense.
// 3. Verify you land on the "Restricted" screen with the "Add a payment card to unlock!" badge.
// 4. Regression: repeat with a single healthy workspace and verify you still land on the expense confirmation page,
// bound to that workspace (the workspace field must not read "None").
Offline tests
// TODO: The human co-author must fill out the expected offline behavior before marking this PR as "ready for review".
// This change only affects client-side navigation before the expense is written, so behavior is expected to be unchanged
// offline, but please confirm.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
// The human co-author must fill out the QA steps before marking this PR as "ready for review".
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
AI Tests
Run locally by MelvinBot on this branch, on the merge commit
78c1706(mainmerged in at0858963):npm run typechecknpm test -- tests/actions/IOU/CreateDraftTransactionTest.ts tests/unit/ReportUtilsTest.ts tests/actions/IOUTest/TrackExpenseTest.tsnpm test -- tests/ui/ChatActionableButtonsTest.tsx tests/ui/DynamicReportDetailsPageTest.tsx tests/unit/components/reportDetails/DynamicReportDetailsPageTest.tsx tests/unit/PolicySelectorTest.ts tests/unit/PolicyUtilsTest.ts tests/unit/SubscriptionUtilsTest.tsnpm run lint-changednpx eslinton every changed fileDynamicReportDetailsPage.tsx, the one file with a merge conflict.npm run spell-changedmainmerge (Childr,shoul,Fsearch,AexpenseinDecisionModal.tsx,FeatureTrainingModal.tsx,PopoverMenu/index.tsx,FilterPopupButton.tsx,useReportSubmitToPopover.tsx,UpdateMoneyRequest.ts,enableGlobalReimbursementsDynamicRouteTest.ts). Reporting rather than fixing, since they are unrelated.npm run react-compiler-compliance-check check-changedGITHUB_BASE_REFis empty in this environment, so the script resolves the base ref toorigin/and aborts. The ESLint runs above apply the React Compiler rules per file and show no newCompilation Skippederrors, so no memoization regression.npm run prettier