feat(usage): show when an organisation is over its plan limit - #8432
feat(usage): show when an organisation is over its plan limit#8432talissoncosta wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
front-end
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe usage page now uses dedicated layout, filter, heading, meter, chart, breakdown, and over-limit components. Allowance data is exposed as a raw usage response. New helpers calculate over-limit details and copy, with tests for limits, dates, charges, and notes. Storybook covers paid and enterprise over-limit states. Blocked accounts can access organisation and organisation-usage routes. Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change adds over-limit usage messaging and permits restricted organisations to view usage details while retaining restrictions elsewhere. No concrete merge-blocking risk remains. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fb8ef44 to
c5f44a2
Compare
github-actions
Bot
added
feature
c5f44a2 to
fc3734e
Compare
github-actions
Bot
added
feature
github-actions
Bot
added
feature
github-actions
Bot
added
feature
talissoncosta
changed the title
✅ private-cloud · depot-ubuntu-latest-16 — run #20147 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20147 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20147 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20147 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20146 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20146 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20146 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Skipped testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss ✅ oss · depot-ubuntu-latest-16 — run #20146 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20145 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20145 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20144 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20145 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
|
@themis-blindfold review |
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis review: 🟠 Fix before mergeFree organisations that exceed their allowance receive only generic over-limit copy, despite having a defined seven-day notification period. The completed CI checks passed; the focused frontend Jest command could not run locally because dependencies are not installed.
🟠 Majors
🧹 Nits
📝 Walkthrough
🧪 How to verify
Product take: This is important guidance for organisations at their cap. Paid overage messaging is covered, but free users currently lack the one deadline that tells them how urgently to act. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. A useful warning needs the clock, too · reviewed at 55d5e90 |
| {canUpgrade && ( | ||
| <Button | ||
| className='flex-shrink-0' | ||
| href={Constants.getUpgradeUrl('usage')} |
There was a problem hiding this comment.
getUpgradeUrl: (feature?: string) => {
return Utils.isSaas()
? '/organisation-settings?tab=billing'
: `https://www.flagsmith.com/pricing${
feature ? `?utm_source=${feature}` : ''
}`
organisation-settings is not allowed in App, we need to add it
There was a problem hiding this comment.
Hidden while blocked now, they still get the upgrade options there.
There was a problem hiding this comment.
Correction to my earlier reply: I put the button back. Hiding it left the one page a blocked org can reach with no way out, which Themis flagged. It isn't a dead link either, organisation settings renders the blocked screen, which is the payment screen.
| const chargeWarning = (basis: UsageBasis): string => | ||
| isBilledOnAPeriod(basis) | ||
| ? ` Overage charges may apply over ${allowanceWindowLabel(basis)}.` | ||
| : '' |
There was a problem hiding this comment.
Do we need to check Chargebee here too?
There was a problem hiding this comment.
Chargebee is already implied, only its webhook writes the billing dates. But you were right that something was missing: only Start-Up and Scale-Up get billed, enterprise falls through the match and never is. Gated on plan now instead.
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe over-limit calculation, period handling, and route exception are coherent, and captured CI is green. However, two promised recovery journeys remain incomplete: free organisations are not told about their seven-day window, and restricted organisations can now open Usage but have no restricted-state explanation or recovery action there.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: This is a solid usage-visibility improvement, but the two users most affected by the limit cannot yet see the full next step. Completing those states is important before relying on this page as the recovery surface. 🧭 Assumptions & unverified claimsThe focused Jest suite could not run locally because Jest is not installed in this checkout; the captured Unit Tests check completed successfully. The meter knows the numbers; the blocked user still needs an exit. · reviewed at d024325 |
|
@themis-blindfold review |
|
@themis-blindfold review |
| AccountStore.getOrganisation().block_access_to_admin && | ||
| pathname !== '/organisations' | ||
| AccountStore.getOrganisation()?.block_access_to_admin && | ||
| !isAllowedWhileBlocked(pathname) |
There was a problem hiding this comment.
🟠 Major · ⚡ Quick win
Cover the restricted-route exception.
Observed: the only added tests are pure overLimit/utility tests; none exercises isAllowedWhileBlocked or the App guard. Predicted: a route change could again lock restricted organisations out of usage, or allow another blocked route, with CI still green. Add a routing or component test that asserts the usage URL renders for a blocked organisation and a neighbouring protected URL renders Blocked.
There was a problem hiding this comment.
Not adding it here: isAllowedWhileBlocked reads web/routes, which imports App, so a test importing it pulls in the whole app. That cycle already broke the boot in this PR once. Fixing it properly, moving the route map into a component-free module, is a follow-up and the test comes with it.
There was a problem hiding this comment.
This remains open. The changed guard imports routes from a module that imports App back (frontend/web/components/App.js:32, frontend/web/routes.js:4); extracting the route paths in this PR removes that cycle and allows the required blocked-usage versus protected-route regression test. A follow-up cannot cover this PR's behaviour change.
There was a problem hiding this comment.
Tracked in #8456, which moves the route table into a module that imports no components and brings the test with it. Kept separate because it touches the file every page depends on. Not blocking this one.
There was a problem hiding this comment.
Not fixed. This PR still changes the blocked-route guard at frontend/web/components/App.js:283 using the circular route import at frontend/web/components/App.js:33; its added tests do not exercise either the usage exception or a neighbouring blocked route. #8456 cannot provide regression coverage for this PR's behaviour change. Extract the paths and add the test here, or revert the exception.
There was a problem hiding this comment.
Not fixed: routePaths.test.ts:19 calls only the helper. It never renders the changed blocked-organisation guard at App.js:275, so a regression in that guard can still pass. Add the blocked usage versus protected-route render test.
There was a problem hiding this comment.
Can't render it here: testEnvironment is node and the repo has no RTL or jsdom, nothing renders a component in a test today. The helper is covered by eight cases; what's left uncovered is one && in App. Standing up component tests is its own piece of work.
There was a problem hiding this comment.
Not fixed. routePaths.test.ts:18 only calls the helper; it never executes the blocked-organisation branch in App.js:275. A node unit environment does not require adding a component-test stack: this repo already runs browser tests via Playwright (package.json:15). Add a restricted-organisation browser test for the usage route and a protected route.
There was a problem hiding this comment.
You are right and I was wrong to say it could not be rendered: the repo runs Playwright with a real e2e suite. No test there sets organisation-level flags yet, so it needs new setup rather than a new stack. Tracking it rather than doing it in this PR.
There was a problem hiding this comment.
Not fixed. routePaths.test.ts:18 only exercises the helper; the blocked-organisation render branch at App.js:275 remains untested. Add the restricted-organisation browser coverage here or revert the exception.
⚖️ Themis review: 🟠 Fix before mergeTL;DR: The restricted banner and recovery action are wired to the existing usage state, but the new exception to the blocked-route guard has no regression test. Completed checks, including unit tests and changed-file linting, were successful; the two private-cloud E2E runs were still in progress.
Focused tests could not run locally because this checkout has no Jest executable. 🟠 Majors
🧹 Nits
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: Solid recovery-path improvement for organisations at their most frustrating moment. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. A useful detour for a locked-out user, once the guard gets its seatbelt · reviewed at f1f1408 |
|
@themis-blindfold review |
A restricted organisation is sent to the blocked screen on every route except the organisations list, so the one page that would explain the restriction is the one page it cannot open. The blocked screen links there now too. block_access_to_admin is enforced only in App. No permission, middleware or view in the API acts on it, so the page loads its data as usual once the route is allowed, and every other route still blocks. The rule lives in web/routePaths rather than App, because web/routes imports App and reading a path back from it is a cycle. Keeping it out of that cycle is also what makes it testable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
⚖️ Themis review: 🟠 Fix before mergeThe new over-limit and restricted states are coherent, and the route allowlist keeps the exception narrowly scoped. One failure path still defeats the restricted-state goal: an analytics or limit request error removes the recovery banner along with the charts. Completed lint, unit, build, and security checks passed; end-to-end jobs were still running.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: A solid improvement for customers at their limit, but the recovery guidance needs to survive the transient data failures most likely to occur on this high-traffic page. 🧭 Assumptions & unverified claimsFocused local tests could not run because this checkout has no Jest executable; the completed unit-test check was successful. A useful dashboard at a stressful moment—just keep the exit sign lit when the data hiccups. · reviewed at b7217a7 |
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe restricted-organisation route and paid-limit messaging are well covered, but free organisations still do not receive the promised seven-day warning before their first restriction. The blocked-route exception also lacks an end-to-end regression test. CI is still running; the completed preview and pre-commit checks succeeded.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: This is a solid recovery experience for restricted customers, but the absent free-plan deadline leaves a time-sensitive action unexplained. 🧭 Assumptions & unverified claims
The route map has found its home; the free-plan clock still needs its cue. · reviewed at e4e2afd |
An organisation over its limit gets a banner and a line under the meter saying how far over. The day it crossed is read off the same running total the chart draws, so the two cannot disagree, and nothing new is fetched. A restricted one gets different copy: what gets access back. Upgrading clears the block at once; otherwise it lifts 30 days after usage drops under the limit. That outlives the overage, so the banner keys off the block rather than off being over. Charges are only mentioned to plans that are actually billed for them. charge_for_api_call_count_overages matches Start-Up and Scale-Up and lets enterprise fall through, so an enterprise plan on a Chargebee term was being warned about a charge that cannot happen. The page composes itself from named parts rather than passing fifteen props into one component, and the free plan is deliberately promised no deadline: the seven-day window is spent after the first restriction and the API does not say which case an organisation is in (#8256). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rule 2 forbade all relative imports, which contradicts the lint config and every barrel in the codebase: prefer-alias only requires an alias when the path goes up, and eslint --fix rewrites one back to relative inside its own root. Rule 8 required a folder and a barrel for every component. A barrel re-exporting one file buys nothing, since the import specifier is the same either way, so a file can become a folder later without touching a caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Strings lived in three places. overLimit mixed computing an overage with writing prose about it, utils carried the plan section and the project contribution, and the two banners said the same thing in two slightly different sentences as a result. copy.ts now holds everything the page says about a plan and its limit, so someone asking what a customer is told reads one file. overLimit keeps the arithmetic, utils keeps periods and windows, and the tests split the same way: copy.test asserts prose, overLimit.test asserts numbers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither recovery route works for a block support set by hand: the plan-change hook and the unrestricting task both skip organisations with no APILimitAccessBlock record. So an organisation blocked for any other reason was being told to upgrade, which would not have helped. With no overage in evidence the block could be either that or a usage block whose usage has since dropped, and the API does not say which, so the banner now points at support instead of promising a route that may not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every sentence that does not depend on a number now sits in COPY, so the prose can be read top to bottom without following the logic that picks it. Two more came in from planSectionCopy, which had them inline. The interpolated ones stay as functions: a placeholder in a data file becomes an empty string at runtime, where a template literal is a compile error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeTL;DR: The route exception and over-limit presentation are on the right path, but two restricted-account cases still leave users without the information this page was added to provide. Completed unit, lint, build, and static-analysis checks passed; the two private-cloud E2E jobs were still running.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: This makes the usage page a useful recovery surface for over-limit accounts, which is a solid improvement. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The escape hatch is nearly there; it just needs to say what escaped and stay open in bad weather · reviewed at adeb660 |
3 participants
Changes
Part of #8187.
A restricted organisation can now open the usage page. It was blocked from every route except the organisations list, so the page that explains the restriction was the one page it could not reach. The blocked screen links there too.
An organisation over its limit gets a banner and a line under the meter saying how far over it is. The date it crossed comes from usage the page already loads, so nothing new is fetched.
A restricted one gets different copy: what gets access back. Upgrading clears the block immediately, otherwise it lifts once usage has stayed under the limit for 30 days.
Charges are only mentioned to plans that can actually be billed for an overage, which is Start-Up and Scale-Up. Whether those are charged or covered by the grace period is #8264.
Not here: naming which services are paused, and telling a free organisation it has seven days. Both need fields the API does not return (#8256), and the seven days is consumable so we cannot promise it truthfully.
Screenshots
How did you test this code?
npm run test:unit -- --testPathPatterns="web/components/pages/usage"Storybook,
Pages/Usage Dashboard/Page:PaidOverTheLimitandEnterpriseOverTheLimitshow the banner with and without the charge line,FreeAndRestrictedandRestrictedButBackUnderTheLimitshow the restricted copy with and without an overage left to report.For the route, set
block_access_to_adminon an organisation and check the usage page loads, the blocked screen links to it, and every other route still blocks.QA notes
block_access_to_admin; the usage page still loads and the banner says how to get access back./organisationsload, every other route shows the blocked screen, and that screen links to usage.Not verified against real data: free plans, and anything on a billing period. Free's 50,000 limit is hardcoded, and billing term dates are read-only in admin because only the Chargebee webhook writes them. Both are covered by the Storybook stories above.