[Payment due @truph01] Clear stale billing grace period optimistically on restricted action page#85635
Conversation
…page When the WorkspaceRestrictedActionPage opens, it calls openSubscriptionPage to fetch fresh billing data. However, the cached billing grace period was never cleared client-side, causing the restriction to persist even after the workspace owner resolved their billing issue. This adds an optimistic Onyx update to null out the owner's billing grace period entry when fetching subscription data. If the server still has an active grace period, it will be restored from the API response; if billing was resolved, it stays null and the restriction lifts. Co-authored-by: Rushat Gabhane <rushatgabhane@users.noreply.github.com>
Use Onyx.set directly for the grace period clear instead of mixing key types in the optimisticData array, which required a bare OnyxUpdate[] type that no longer compiles (OnyxUpdate now requires a type argument). Co-authored-by: Rushat Gabhane <rushatgabhane@users.noreply.github.com>
|
Fixed the failing typecheck: |
|
@MelvinBot fix Type widening from Array<OnyxUpdate> to OnyxUpdate[] |
…openSubscriptionPage Consolidates three separate Array<OnyxUpdate<typeof ONYXKEYS.IS_LOADING_SUBSCRIPTION_DATA>> variables into a single OnyxData<typeof ONYXKEYS.IS_LOADING_SUBSCRIPTION_DATA> object, matching the pattern used by other functions in the same file (updateSubscriptionSize, clearOutstandingBalance). This prevents type widening from the specific OnyxUpdate type to the wider OnyxUpdate[]. Co-authored-by: Rushat Gabhane <rushatgabhane@users.noreply.github.com>
|
Fixed the type widening issue. Consolidated the three separate |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot mark ready for review |
melvin-bot
bot
requested review from
JmillsExpensify
and removed request for
a team
![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: 77219b9b8f
ℹ️ 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".
src/libs/actions/Subscription.ts
Outdated
| // Clear the owner's billing grace period optimistically. If the server still has it, | ||
| // it will be restored from the response. If it was deleted (billing resolved), it stays null. | ||
| if (ownerAccountID) { | ||
| Onyx.set(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END}${ownerAccountID}`, null); |
There was a problem hiding this comment.
Clearing SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END with a standalone Onyx.set(..., null) is not tied to API.read's failureData, so a failed OpenSubscriptionPage request (e.g., transient 5xx/network error) leaves the owner’s grace-period entry permanently null in local state. Because restriction checks treat a null entry as “not restricted,” WorkspaceRestrictedActionPage can navigate away once loading flips false, allowing billable flows even though billing is still overdue. This optimistic clear should be part of request onyx data with a rollback path (or explicitly restored on failure).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot What do you think about this comment?
There was a problem hiding this comment.
The concern is valid. Here's the failure scenario:
openSubscriptionPageimmediately callsOnyx.set(gracePeriodKey, null)— clearing the grace periodoptimisticDatasetsIS_LOADING_SUBSCRIPTION_DATA = true- The API call fails (network error, 5xx, etc.)
failureDatasetsIS_LOADING_SUBSCRIPTION_DATA = falseWorkspaceRestrictedActionPage(line 48-54) sees loading is done, checksshouldRestrictUserBillableActions— the grace period is null so no restriction is detected → navigates back- The workspace appears unrestricted even though billing is still overdue
The standalone Onyx.set on line 21 runs outside the API lifecycle, so there's no rollback on failure.
Practical risk is moderate — onReconnect (line 33) would re-trigger the call when connectivity returns, but there's a window where the restriction is incorrectly lifted. Also, API.read commands aren't retried like API.write, so a 5xx failure with working connectivity would leave the grace period permanently null until the next navigation.
The right fix would be to move the grace period clear into optimisticData and capture the previous value for failureData rollback. Something like:
function openSubscriptionPage(ownerAccountID?: number) {
// Read the current value to enable rollback on failure
const currentGracePeriod = /* read from Onyx */;
const onyxData = {
optimisticData: [
{ key: IS_LOADING_SUBSCRIPTION_DATA, value: true, ... },
// Only include if ownerAccountID is provided
...(ownerAccountID ? [{ key: gracePeriodKey, value: null, ... }] : []),
],
failureData: [
{ key: IS_LOADING_SUBSCRIPTION_DATA, value: false, ... },
// Restore previous value on failure
...(ownerAccountID ? [{ key: gracePeriodKey, value: currentGracePeriod, ... }] : []),
],
successData: [
{ key: IS_LOADING_SUBSCRIPTION_DATA, value: false, ... },
],
};
}This keeps the optimistic clear behavior on success while properly rolling back on failure. rushatgabhane — would you like me to implement this fix?
There was a problem hiding this comment.
Implemented the fix. The standalone Onyx.set() has been replaced with proper API lifecycle integration:
optimisticData— clears the grace period when the API call starts (same behavior as before)failureData— restores the previous grace period value if the API call fails (new rollback behavior)
The caller (WorkspaceRestrictedActionPage) now passes the current grace period value via a ref to avoid re-triggering the fetch effect on every optimistic update.
This ensures that a failed OpenSubscriptionPage request (e.g., transient 5xx/network error) won't leave the grace period permanently null.
There was a problem hiding this comment.
Added unit tests for openSubscriptionPage covering the new grace period handling:
- No ownerAccountID — verifies only loading state Onyx data is included
- With ownerAccountID and currentGracePeriod — verifies optimistic clear (null) and failure rollback (restore previous value)
- With ownerAccountID but undefined currentGracePeriod — verifies failure rollback falls back to null
- Falsy ownerAccountID (0) — verifies grace period data is not included
All 4 tests pass locally.
|
@MelvinBot Please merge main |
…aceRestriction Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Merged |
melvin-bot
bot
requested review from
ZhenjaHorbach
and removed request for
a team
|
@ZhenjaHorbach 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] |
|
@ZhenjaHorbach Please ignore it, not sure why Melvin assigns a new C+ here |
The standalone Onyx.set() call was not tied to the API request lifecycle, so a failed OpenSubscriptionPage request would leave the grace period permanently null, incorrectly lifting workspace restrictions. Now the grace period clear is part of optimisticData with the previous value restored via failureData on failure. Co-authored-by: truph01 <truph01@users.noreply.github.com>
…'s key When a policy owner changes, the cached grace period for the old owner is never evicted, leaving stale restriction state. This changes openSubscriptionPage to accept the entire grace period collection and optimistically clear all entries (with rollback on failure), instead of clearing only the single ownerAccountID-specific key. Co-authored-by: Rushat Gabhane <rushatgabhane@users.noreply.github.com>
|
Implemented the change to clear all billing grace period keys instead of only the current owner's key, plus updated unit tests. What changed:
|
@rushatgabhane If the policy owner changes, wouldn’t the pusher update the policy owner accordingly? Anyway, clearing all values seems reasonable. The |
|
@MelvinBot Please merge main |
…aceRestriction Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Merged |
|
@rushatgabhane I tested the latest change, it looks good, all the Screen.Recording.2026-04-08.at.11.15.33.mov |
yeah i thought so too, but it doesn't work for many cases. eg: when the nvp is cleared, it is delete from DB and then we can't share the "cleared" nvp to users that were offline. so it leads to stale nvp that is stuck and the user sees restricted page |
thank you for being so proactive! 🙇 |
|
🚧 @robertjchen 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 9.3.55-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a logic-only fix that clears stale cached billing grace period data optimistically so the workspace restriction lifts immediately after the owner resolves their billing issue. It does not:
The existing help site articles under |
|
@MelvinBot We are not sure if we tested it correctly, but we can repro issue (if it is actually an issue) with a briefly shown loader and RHP close. bandicam.2026-04-09.09-47-46-345.mp4 |
I looked at the video. I agree this shouldn't happen. |
|
@JmillsExpensify should we log a new issue or failed with the original one? |
|
@IuliiaHerets Could you please share the test steps? This would help clarify whether it falls within the scope of this issue. |
|
@truph01 Run snippet in devtools console: |
|
@IuliiaHerets, could you share the expected behavior and the actual behavior, too? |
|
@truph01, We believe you would have better clarity on the expected behavior. |
|
I think we should either: open it and show a spinner, and equally, let's not auto-close the RHP. That's confusing. |
@IuliiaHerets @JmillsExpensify The loading indicator is shown because data is being fetched from the backend. Once the data is retrieved, the system determines that there is nothing to restrict, so the view automatically closes. Isn’t this the expected behavior? Also, it appears that production behaves the same way. You can refer to the attached video in this comment. |
|
Auto-closing is not the expected behavior. I agree we need some kind of loading experience though. |
@JmillsExpensify So what should have happened instead of "auto close"? |
|
If we don't know for certain that we should show the RHP message or not, then the loader shouldn't be in the RHP. It should be on the entire screen. That way, we only open the RHP if it's relevant. |
7 participants
Explanation of Change
When a workspace is restricted due to a billing issue, the
WorkspaceRestrictedActionPagecallsopenSubscriptionPage()to fetch fresh billing data from the server. However, the cachedSHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_ENDcollection entry for the workspace owner was never cleared optimistically on the client. This means if the server response doesn't explicitly update/clear these keys (or if there's any caching delay), the stale grace period data keeps the workspace restricted even after the owner has resolved their billing issue.This fix adds an optimistic Onyx update to
openSubscriptionPagethat nulls out the owner's billing grace period entry before the API call. If the server still has an active grace period, it will be restored from the response. If billing was resolved, it stays null and the restriction lifts immediately.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/597281
Tests
Offline tests
openSubscriptionPagefires viaonReconnectand the restriction lifts if billing was resolvedQA Steps
PR 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
N/A - no visual changes, logic-only fix
Android: mWeb Chrome
N/A - no visual changes, logic-only fix
iOS: Native
N/A - no visual changes, logic-only fix
iOS: mWeb Safari
N/A - no visual changes, logic-only fix
MacOS: Chrome / Safari
N/A - no visual changes, logic-only fix