Gate "Submit to my employer" on the workspace billing restriction - #100584
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.
|
🤖 PR description replaced with the content you provided. I read "replace the PR author checklist" as replacing the whole description, since what you pasted was a full PR body (Explanation of Change through Screenshots/Videos), not just the checklist section. I unwrapped the outer I kept the The Two things to look at:
|
| } | ||
| if (filteredPoliciesCount === 0) { | ||
| firstPolicyID = policy.id; | ||
| firstPolicy = {id: policy.id, ownerAccountID: policy.ownerAccountID}; |
There was a problem hiding this comment.
This inline projection is identical to the new billingRestrictionPolicySelector added just below (line 389) — both build a BillingRestrictionPolicy as {id: policy.id, ownerAccountID: policy.ownerAccountID}, and both carry the same "projected to only the fields the billing gate needs" rationale. Two copies means the projection has to be kept in sync by hand if the gate ever reads another field.
policy is already non-null here (guarded by the if (!policy …) continue above) and the target is BillingRestrictionPolicy | undefined, so we can just reuse the selector:
| firstPolicy = {id: policy.id, ownerAccountID: policy.ownerAccountID}; | |
| firstPolicy = billingRestrictionPolicySelector(policy); |
Both are module-scope consts invoked at runtime, so referencing billingRestrictionPolicySelector before its declaration is fine; move its definition above this selector if you'd rather keep read-order clarity.
There was a problem hiding this comment.
Done in 6585a59 — firstPolicy = billingRestrictionPolicySelector(policy);, so the projection exists in one place.
I moved the selector's definition above createFilteredPoliciesInfoSelector rather than relying on hoisting, per your read-order note.
Its doc comment also needed a small change: it used to say "the preferred workspace", which stopped being true once it served both call sites. It now reads "Projects a policy down to just the fields the billing gate reads — see BillingRestrictionPolicy."
| currentUserEmail: RORY_EMAIL, | ||
| currentUserLocalCurrency: '', | ||
| filteredPoliciesCount: 1, | ||
| firstPolicy: restrictedPolicy, |
There was a problem hiding this comment.
NAB: this test uses the default submitDestination (FRIEND), so the EMPLOYER branch is skipped and firstPolicy is never read on this path — only restrictedPreferredPolicy drives the assertion.
firstPolicy: restrictedPolicy is harmless but redundant here; dropping it would make the test isolate the preferred-policy gate more clearly.
There was a problem hiding this comment.
Done in 6585a59 — you're right that it was never read on this path.
One adjustment: firstPolicy is a required key (that was the fix for your earlier fail-open comment), so it's now firstPolicy: undefined rather than dropped, with a comment recording why:
// Left unset so only the preferred-policy gate can drive the assertions below: the default
// `submitDestination` is FRIEND, so the EMPLOYER branch that reads `firstPolicy` never runs.
firstPolicy: undefined,The test still passes, which confirms your reading.
| /** | ||
| * The preferred workspace, set only when the user is restricted to submitting there. One non-nullable value | ||
| * instead of an `isRestrictedToPreferredPolicy`/`preferredPolicyID`/`preferredPolicy` trio: the fast path it | ||
| * unlocks skips the participant picker, which is where the billing restriction is otherwise enforced, so | ||
| * "submit straight to the preferred workspace" and "here is the policy to gate on" have to be the same fact. | ||
| * As three parallel optionals a caller could supply the flag and the ID but not the policy — the flag and the | ||
| * ID come from the security group, the policy from Onyx — and silently disable the gate. | ||
| */ |
There was a problem hiding this comment.
This doc block is one dense argument with a mid-sentence dashed aside nested inside a longer clause, so the actual rule — these facts must travel together or the gate silently turns off — only surfaces after a couple of reads. Lead with the rule, drop the trio enumeration:
| /** | |
| * The preferred workspace, set only when the user is restricted to submitting there. One non-nullable value | |
| * instead of an `isRestrictedToPreferredPolicy`/`preferredPolicyID`/`preferredPolicy` trio: the fast path it | |
| * unlocks skips the participant picker, which is where the billing restriction is otherwise enforced, so | |
| * "submit straight to the preferred workspace" and "here is the policy to gate on" have to be the same fact. | |
| * As three parallel optionals a caller could supply the flag and the ID but not the policy — the flag and the | |
| * ID come from the security group, the policy from Onyx — and silently disable the gate. | |
| */ | |
| /** | |
| * The preferred workspace, set only when the user is restricted to submitting there. Kept as one value | |
| * (not a separate flag + ID) so the fast path that skips the participant picker — where the billing gate | |
| * normally runs — can't be taken with the gate accidentally left off. | |
| */ |
There was a problem hiding this comment.
Done in 6585a59 — applied your wording verbatim. Leading with the rule does read better than making the reader assemble it from the trio enumeration.
| /** | ||
| * The first policy that should be shown to the user, so callers can gate on it without re-reading a | ||
| * separately-timed policy cache. Projected to only the fields the billing gate needs, so this output stays | ||
| * fixed-size (see `policyMapper` above) and no `employeeList`/`customUnits` is deep-compared on a POLICY write. | ||
| */ |
There was a problem hiding this comment.
The "projected so employeeList/customUnits isn't deep-compared on a POLICY write" rationale is stated nearly in full in four places:
src/libs/SubscriptionUtils.ts:507–511(BillingRestrictionPolicytype — the natural canonical home)- here,
src/selectors/Policy.ts:359–363 src/selectors/Policy.ts:367(// Fixed-size output …inline)src/selectors/Policy.ts:388(billingRestrictionPolicySelector)
Keep the full explanation on the BillingRestrictionPolicy type and shorten the rest to a pointer (the billingRestrictionPolicySelector comment at :388 already does this well). Suggested trim for this block:
| /** | |
| * The first policy that should be shown to the user, so callers can gate on it without re-reading a | |
| * separately-timed policy cache. Projected to only the fields the billing gate needs, so this output stays | |
| * fixed-size (see `policyMapper` above) and no `employeeList`/`customUnits` is deep-compared on a POLICY write. | |
| */ | |
| /** The first policy to show the user, projected to the billing-gate fields — see `BillingRestrictionPolicy`. */ |
There was a problem hiding this comment.
Done in 6585a59 — trimmed to your suggested one-liner:
/** The first policy to show the user, projected to the billing-gate fields — see `BillingRestrictionPolicy`. */The full rationale stays on BillingRestrictionPolicy in SubscriptionUtils.ts, and the // Fixed-size output … line above the selector body stays as-is since it explains the short-circuit rather than the projection.
| return; | ||
| } | ||
|
|
||
| const policyExpenseReport = getPolicyExpenseChat(currentUserAccountID, restrictedPreferredPolicy.id); |
There was a problem hiding this comment.
Test coverage — missing the not-restricted (happy) path of this branch.
The new gate above (navigateToRestrictedActionIfNeeded(restrictedPreferredPolicy, …)) has its restricted arm covered by the new "preferred workspace has an expired required payment" test, but the pass-through — restrictedPreferredPolicy set yet not billing-restricted → submit straight to the preferred workspace here → draft rebound → confirmation — has no test. (Confirmed: no test sets restrictedPreferredPolicy on a non-restricted policy.)
Add to tests/actions/IOU/CreateDraftTransactionTest.ts (submitting a tracked expense to an employer describe):
it('should submit straight to the preferred workspace when it is not billing-restricted') — build a preferred policy the user owns with no amountOwed/grace period (so shouldRestrictUserBillableActions is false), create its policy expense chat, call createDraftTransactionAndNavigateToParticipantSelector with restrictedPreferredPolicy set, then assert Navigation.navigate was not called with RESTRICTED_ACTION and the draft's reportID equals the preferred chat's reportID (mirror the existing single-workspace bind test at :512).
There was a problem hiding this comment.
Added in 6585a59 — it('should submit straight to the preferred workspace when it is not billing-restricted'), built as you described: a preferred policy the user owns with amountOwed: 0 and no grace period, so shouldRestrictUserBillableActions is false.
It asserts all three things the pass-through is responsible for, not just the absence of the redirect:
Navigation.navigatewas not called withRESTRICTED_ACTION- the draft's
reportIDand first participant'sreportIDare the preferred chat's (the rebinding) - the confirmation route for that chat was navigated to
That last pair matters — asserting only "no restricted-action navigation" would also pass if the flow silently fell through to the participant picker, which is the other way this branch can fail.
Paired with the existing restricted test, the two now differ only in amountOwed/grace period, so they isolate the gate itself.
| } | ||
| if (filteredPoliciesCount === 0) { | ||
| firstPolicyID = policy.id; | ||
| firstPolicy = {id: policy.id, ownerAccountID: policy.ownerAccountID}; |
There was a problem hiding this comment.
Test coverage — createFilteredPoliciesInfoSelector is not tested directly.
The action tests bypass this selector by passing firstPolicy / filteredPoliciesCount as params, so the selector's own logic — shouldShowPolicy / isTeachersUnitePolicyID filtering, the short-circuit at 2, and the new {id, ownerAccountID} projection on this line — has zero coverage. tests/unit/PolicySelectorTest.ts already tests sibling selectors, so it belongs there.
Add describe('createFilteredPoliciesInfoSelector') with:
- no showable policies →
{filteredPoliciesCount: 0, firstPolicy: undefined} - one showable policy →
firstPolicydeep-equals{id, ownerAccountID}of that policy (assert it's the projection, not the fullPolicy) - a Teachers-Unite / non-showable policy is skipped
- 2+ showable policies → short-circuits at
filteredPoliciesCount: 2withfirstPolicy= the first match
There was a problem hiding this comment.
Added in 6585a59 — describe('createFilteredPoliciesInfoSelector') in tests/unit/PolicySelectorTest.ts, with the four cases you listed:
- no policies →
{filteredPoliciesCount: 0, firstPolicy: undefined} - nothing passes
shouldShowPolicy(a personal policy) → same - one showable policy →
expect(result.firstPolicy).toStrictEqual({id: 'p1', ownerAccountID: 42}) - Teachers Unite policy → skipped
- three showable policies → short-circuits at
filteredPoliciesCount: 2,firstPolicyis the first match
On asserting the projection rather than the full Policy: toStrictEqual against the two-key literal is what does that work — the policy in that case is built with an employeeList, so the assertion fails if the selector ever starts passing the whole object through.
| }; | ||
|
|
||
| /** The preferred workspace, projected to only what the billing gate reads, for the same reason as `firstPolicy` above. */ | ||
| const billingRestrictionPolicySelector = (policy: OnyxEntry<Policy>): BillingRestrictionPolicy | undefined => (policy ? {id: policy.id, ownerAccountID: policy.ownerAccountID} : undefined); |
There was a problem hiding this comment.
Test coverage — new billingRestrictionPolicySelector has no test.
Trivial projection, but new and both arms are unexercised. Quick to cover in tests/unit/PolicySelectorTest.ts:
Add describe('billingRestrictionPolicySelector'):
- a defined policy → returns exactly
{id, ownerAccountID}(and nothing else) undefinedinput → returnsundefined
There was a problem hiding this comment.
Added in 6585a59 — describe('billingRestrictionPolicySelector') with both arms:
- a policy carrying
name,typeand anemployeeList→ returns exactly{id, ownerAccountID}(toStrictEqual, so an extra field fails it) undefined→undefined
Worth a little more now than when you flagged it: per your comment on Policy.ts:378, createFilteredPoliciesInfoSelector now calls this function instead of inlining the projection, so these two tests cover both projection sites.
| ownerBillingGracePeriodEnd, | ||
| isRestrictedToPreferredPolicy, | ||
| preferredPolicyID, | ||
| restrictedPreferredPolicy: isRestrictedToPreferredPolicy ? preferredPolicy : undefined, |
There was a problem hiding this comment.
Test coverage — the isRestrictedToPreferredPolicy ? preferredPolicy : undefined branch is unasserted.
Neither arm of this ternary (true → preferredPolicy, false → undefined) is verified. It's thin wiring over already-tested createDraftTransactionAndNavigateToParticipantSelector logic, so low severity — closing the StartExpenseFlows.ts:451 gap covers the downstream behavior. If you want the wiring itself asserted, drive this component with isRestrictedToPreferredPolicy = true and a present preferredPolicy, and assert the restricted-action navigation.
There was a problem hiding this comment.
Taking your first option: the StartExpenseFlows.ts:451 gap is now closed (6585a59), so both outcomes this ternary selects between are asserted at the action level — restricted → restricted-action screen, not restricted → straight to the preferred workspace.
I left the component wiring untested here deliberately. A test that renders this page to assert isRestrictedToPreferredPolicy ? preferredPolicy : undefined would be asserting the ternary against itself; the risk it would actually catch — passing the flag without the policy — is the one the restrictedPreferredPolicy single-field contract already made unrepresentable.
Happy to add it if you'd rather have the wiring pinned explicitly.
| ownerBillingGracePeriodEnd, | ||
| isRestrictedToPreferredPolicy, | ||
| preferredPolicyID, | ||
| restrictedPreferredPolicy: isRestrictedToPreferredPolicy ? preferredPolicy : undefined, |
There was a problem hiding this comment.
Test coverage — same untested ternary as the other two entry points.
isRestrictedToPreferredPolicy ? preferredPolicy : undefined — neither arm asserted here. Low severity (wiring over tested action logic); covered in practice once StartExpenseFlows.ts:451 gets its not-restricted test. Optionally assert restricted-action navigation by rendering this page with isRestrictedToPreferredPolicy = true and a present preferredPolicy.
There was a problem hiding this comment.
Same as the MoneyRequestHeaderSecondaryActions.tsx:391 thread: the StartExpenseFlows.ts:451 not-restricted test landed in 6585a59, so both arms' downstream behavior is asserted at the action level.
Skipping the render-level assertion here for the same reason — it would pin the ternary to itself rather than to a behavior, and the fail-open shape it used to guard against is gone now that the flag and the policy are one field. Say the word if you want it anyway.
| filteredPoliciesCount: filteredPoliciesInfo?.filteredPoliciesCount ?? 0, | ||
| firstPolicyID: filteredPoliciesInfo?.firstPolicyID, | ||
| firstPolicy: filteredPoliciesInfo?.firstPolicy, | ||
| restrictedPreferredPolicy: isRestrictedToPreferredPolicy ? preferredPolicy : undefined, |
There was a problem hiding this comment.
Test coverage — untested ternary.
isRestrictedToPreferredPolicy ? preferredPolicy : undefined — both arms unasserted. tests/actions/IOUTest/TrackExpenseTest.ts exercises this flow but never sets up a restricted preferred policy to hit the true arm. Low severity; extend that test to drive TrackExpenseButtons with isRestrictedToPreferredPolicy = true + a present preferredPolicy and assert the restricted-action navigation — or rely on the StartExpenseFlows.ts:451 unit gap being closed.
There was a problem hiding this comment.
Going with the fallback you offered: StartExpenseFlows.ts:451 now has its not-restricted test (6585a59), so both arms of this ternary have their behavior covered at the action level.
I didn't extend TrackExpenseTest.ts to drive TrackExpenseButtons with a restricted preferred policy. It would exercise the same action-level branch through a heavier render path, and the collapse to a single restrictedPreferredPolicy field removed the disagreement this was guarding against. Let me know if you'd still like it there.
|
@MelvinBot address the new suggested changes. |
…lector and pass-through tests
- Reuse billingRestrictionPolicySelector in createFilteredPoliciesInfoSelector instead of
repeating the {id, ownerAccountID} projection inline.
- Keep the full projection rationale on BillingRestrictionPolicy; shorten the copies to pointers.
- Lead the restrictedPreferredPolicy doc with the rule instead of the trio enumeration.
- Add the not-restricted pass-through test for the preferred-workspace fast path.
- Cover createFilteredPoliciesInfoSelector and billingRestrictionPolicySelector directly.
- Drop the redundant firstPolicy from the preferred-policy restriction test.
|
🤖 Addressed all 10 review comments in 6585a59 (plus 57d45a5, a spellcheck follow-up). Replied in each thread. Seven changes applied, three declined with reasoning. Applied:
Declined — the three untested-ternary comments ( On the new pass-through testIt asserts three things, not just the absence of the redirect:
Asserting only "no restricted-action navigation" would also pass if the flow silently fell through to the participant picker, which is the other way this branch can fail. Paired with the existing restricted test, the two now differ only in One extra change you didn't ask for
VerificationCI on Locally: 2062 tests across the 9 affected suites pass, plus
|
|
🤖 Nothing new to address — this request was already handled by my reply a few minutes ago, in I re-checked rather than assuming, and the branch tip (
The only red check is If you meant a different set of suggestions — something posted somewhere I'm not seeing, or a review you started but haven't submitted yet — a pending review stays invisible to the API until it's submitted. Submit it and tag me again, or paste the feedback here directly and I'll work from that. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen_Recording_20260916_160309_Expensify.Dev.mp4Android: mWeb ChromeScreen_Recording_20260916_160625_Chrome.mp4iOS: HybridAppScreen.Recording.2026-09-16.at.5.30.25.PM.moviOS: mWeb SafariScreen.Recording.2026-09-16.at.5.04.31.PM.movMacOS: Chrome / SafariScreen.Recording.2026-09-16.at.3.42.59.PM.mov |
|
@MelvinBot Merge the main. |
…erRestrictedWorkspace Co-authored-by: Maruf Sharifi <marufsharifi@users.noreply.github.com>
|
🤖 Merged Two files this PR touches were relocated on
The fix itself is unchanged: the diff against Tests re-run locally on the merge commit — all green:
On the ESLint errorsRunning raw
I could not re-run the manual Next Steps: Reply with view run · no recording available |
|
LGTM, all yours @youssef-lr thanks. |
Nakagawa-master
left a comment
Nakagawa-master
left a comment
There was a problem hiding this comment.
There is still a time-of-check gap after the new entry-point guards that seems distinct from the earlier stale-allPolicies finding.
The PR correctly prevents an already-restricted workspace from reaching confirmation through the single/preferred-workspace fast paths. But once the user has reached the confirmation page, the restriction is no longer checked before the write:
StartExpenseFlowschecksshouldRestrictUserBillableActions(...)before navigation.useExpenseSubmission.requestMoney()performs the final optimistic/write path without that check.- the PR description itself notes that confirmation / validation /
requestMoneydo not re-check the billing restriction.
So this sequence can still cross the boundary:
- workspace is healthy; user chooses “Submit to my employer” and reaches confirmation;
- while the confirmation page remains open (or while the app is offline/backgrounded), the required-payment state becomes restricted and the billing NVPs update;
- user presses the final submit/create action;
- the write path does not consult the now-current restriction state.
The earlier P1 fixed which snapshot is used at the entry check; this is a separate when the authorization-like business gate is checked issue.
A discriminating regression would start the flow unrestricted, reach confirmation, then update the billing state to restricted before invoking the submit handler and assert that no RequestMoney/expense write is issued and the user is sent to RESTRICTED_ACTION.
The strongest place to enforce this is server-side at the write command, because offline/queued clients cannot guarantee freshness. If the backend already rejects this condition, then a confirmation-time client check would still avoid an optimistic expense that later rolls back; if the backend does not, the entry-only guard remains bypassable by this state transition.
|
@youssef-lr, could you please take a look at this when you get a chance. thanks. |
| * Takes the resolved policy rather than an ID on purpose: callers pass the snapshot they already hold, instead of a | ||
| * separately-timed `allPolicies` cache, which can lag a caller's own snapshot and let the gate fail open. |
There was a problem hiding this comment.
| * Takes the resolved policy rather than an ID on purpose: callers pass the snapshot they already hold, instead of a | |
| * separately-timed `allPolicies` cache, which can lag a caller's own snapshot and let the gate fail open. |
These couple of lines are unnecessary
youssef-lr
left a comment
youssef-lr
left a comment
There was a problem hiding this comment.
Minor unnecessary comment that can be removed - but won't block on it.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 youssef-lr 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/youssef-lr in version: 9.4.91-0 🚀
|
|
🤖 No help site changes are required for this PR, so I did not create a draft docs PR. This is a pure bug fix that restores already-intended behavior. It adds no feature, setting, tab, button, or user-facing copy. Why no docs change is neededWhat changed: the billing-restriction gate was only applied in the destination picker, so the single-workspace fast path for Submit to my employer skipped it. The fix extracts the existing check into Docs I checked against it:
The Restricted screen the fix now routes to is existing, unchanged UI — no new strings were added to One optional gap, pre-existing and not caused by this PR: Fix a Billing Issue describes the consequence only as losing "access to paid features." In the app, a workspace past its grace period actually blocks new activity outright — both the owner and members are sent to a Restricted screen when they try to create or submit an expense. If you'd like, reply and I'll open a draft PR adding an FAQ to that article spelling that out (and verify the exact on-screen labels against the live UI first). @marufsharifi, no help site PR was created because this PR changes no documented behavior — please confirm you agree with that call. If you'd rather I document the Restricted screen behavior described above, reply with |
|
Deploy Blocker #101835 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.91-3 🚀
Bundle Size Analysis (Sentry): |
7 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.Fixed Issues
$ #99325
PROPOSAL: #99325 (comment)
Tests
Precondition: The user has only an expired workspace payment requirement.
Offline tests
Same as Tests.
QA Steps
Same as Tests.
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 after merging
main(9b0054c) into the branch, on merge commitaef8ef0:npx tsc --build tsconfig.json(TS 7 native compiler, all 6 referenced projects)npm testontests/actions/IOU/CreateDraftTransactionTest.ts tests/unit/PolicySelectorTest.tsnpm testontests/actions/IOUTest/TrackExpenseTest.ts tests/unit/ReportUtilsTest.ts tests/unit/PolicyUtilsTest.ts tests/unit/SubscriptionUtilsTest.tsnpm testontests/ui/ChatActionableButtonsTest.tsx tests/ui/DynamicReportDetailsPageTest.tsx tests/unit/components/reportDetails/DynamicReportDetailsPageTest.tsx tests/ui/components/AddExpenseActionButtonTest.tsxnpm run spell-changednpx eslinton every changed fileDynamicReportDetailsPage.tsxreturns the identical 11 errors when reverted toorigin/mainTestsstepsThe merge brought in two file relocations from
main, which git followed automatically:src/components/MoneyRequestHeader/MoneyRequestHeaderSecondaryActions.tsx→src/components/MoneyRequestHeaderSecondaryActions.tsx, andsrc/pages/home/report/comment/actionContents/ChatActionableButtons.tsx→src/pages/inbox/report/actionContents/ChatActionableButtons.tsx.npm run lint(seatbelt ratchet) andnpm run react-compiler-compliance-checkcould not run locally becausebunis unavailable in this environment; both remain covered by CI.