Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions frontend/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@

## Rules
1. **API Integration**: Use `npx ssg` CLI + check `../api` backend
2. **Imports**: Use `common/`, `components/`, `project/` (NO relative imports)
2. **Imports**: Use the `common/`, `components/`, `project/` aliases when the relative path would go up (`../`); use a relative path for same-folder or descendant imports. This is enforced by `@dword-design/import-alias/prefer-alias`, and `eslint --fix` will rewrite an alias back to relative inside its own alias root (e.g. `components/base/forms/X` becomes `./base/forms/X` in a file under `web/components/`).
3. **State**: Redux Toolkit + RTK Query, store in `common/store.ts`
4. **Feature Flags**: When user says "create a feature flag", you MUST: (1) Create it in Flagsmith using MCP tools (`mcp__flagsmith__create_feature`), (2) Implement code with `useFlags` hook. See `.claude/context/feature-flags/` for details
5. **Linting**: ALWAYS run `npx eslint --fix <file>` on any files you modify
6. **Type Enums**: Extract inline union types to named types (e.g., `type Status = 'A' | 'B'` instead of inline)
7. **NO FETCH**: NEVER use `fetch()` directly - ALWAYS use RTK Query mutations/queries (inject endpoints into services in `common/services/`), see api-integration context
8. **Component structure**: Each new component lives in its own folder with an `index.ts` barrel - `ComponentName/ComponentName.tsx`, co-located `ComponentName.scss`, any sub-components, and an `index.ts` that re-exports the default (and public types). Import via the folder (`components/.../ComponentName`), never the inner file. Keep files focused (~100 lines as a target); split by concern, not to hit a number. Data tables/constant maps are exempt.
8. **Component structure**: A component with nothing to keep beside it is a single `ComponentName.tsx`. It gets a folder once it has a co-located `ComponentName.scss`, sub-components, tests or hooks: `ComponentName/ComponentName.tsx` plus an `index.ts` re-exporting the default (and public types). Import via `components/.../ComponentName` either way, never the inner file, so promoting a file to a folder changes no imports. Keep files focused (~100 lines as a target); split by concern, not to hit a number. Data tables/constant maps are exempt.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

## Key Files
- Store: `common/store.ts`
Expand Down
178 changes: 130 additions & 48 deletions frontend/documentation/components/UsageDashboard.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,27 @@
import { FC, useState } from 'react'
import type { Meta, StoryObj } from 'storybook'
import { UsageDashboard } from 'components/pages/usage'
import UsagePageLayout from 'components/pages/usage/components/UsagePageLayout'
import OverLimitBanner from 'components/pages/usage/components/OverLimitBanner'
import SectionHeading from 'components/pages/usage/components/SectionHeading'
import UsageBreakdown, {
useUsageBreakdown,
} from 'components/pages/usage/components/UsageBreakdown'
import UsageMeter from 'components/pages/usage/components/UsageMeter'
import UsageOverTime from 'components/pages/usage/components/UsageOverTime'
import {
allowanceWindow,
contributionNote,
overLimitNote,
planSectionCopy,
} from 'components/pages/usage/copy'
import { overLimitOf } from 'components/pages/usage/overLimit'
import {
allowanceWindow,
isBilledOnAPeriod,
isBillingPeriodSelected,
isChargedForOverages,
periodLabel,
periodsFor,
PeriodSelection,
planSectionCopy,
resolvePeriod,
showsContribution,
showsPlanCeiling,
Expand All @@ -23,6 +32,10 @@ import { PlanLimit } from 'components/shared/UsageBar/utils'
import { Subscription } from 'common/types/responses'
import { toUsageResponse, USAGE_SCENARIOS } from './fixtures/usage'

// UsageFilters sets this in its stylesheet, which the harness never loads:
// it fakes the project select rather than rendering the real component.
const FILTER_WIDTH = { minWidth: 210 }

const PROJECTS = [
'All Projects',
'Checkout',
Expand Down Expand Up @@ -69,6 +82,7 @@ type HarnessProps = {
empty?: boolean
isLoading?: boolean
isError?: boolean
isRestricted?: boolean
}

/**
Expand All @@ -79,6 +93,7 @@ const UsagePage: FC<HarnessProps> = ({
empty,
isError,
isLoading,
isRestricted,
limit,
scale = 1,
subscription,
Expand All @@ -99,10 +114,20 @@ const UsagePage: FC<HarnessProps> = ({
scenarioFor(billingPeriod, !!empty, isFreePlan),
share * scale,
)
const allowanceTotal = toUsageResponse(
const allowance = toUsageResponse(
scenarioFor(allowanceWindow(basis), !!empty, isFreePlan),
scale,
).totals.total
)
const allowanceTotal = allowance.totals.total
const exceeded = overLimitOf(allowanceTotal, limit, allowance)

const contribution = showsContribution(
basis,
billingPeriod,
filtered ? 1 : undefined,
)
? contributionNote(project, scoped.totals.total, allowanceTotal)
: undefined

// The note needs the organisation over the period on screen, not over the
// allowance window, or a project can read as more than all of it.
Expand All @@ -114,53 +139,79 @@ const UsagePage: FC<HarnessProps> = ({
)}`

return (
<UsageDashboard
breakdown={
<UsageBreakdown
{...breakdown}
onChangeDimension={setDimension}
scope={scope}
/>
}
data={scoped}
filters={
<Row className='gap-2'>
<div style={{ minWidth: 210 }}>
<Select
aria-label='Period'
onChange={(option: PeriodOption) => setChosenPeriod(option.value)}
options={periods}
value={periods.find((option) => option.value === billingPeriod)}
/>
</div>
<div style={{ minWidth: 210 }}>
<Select
aria-label='Project'
onChange={(option: { value: string }) => setProject(option.value)}
options={PROJECTS.map((name) => ({ label: name, value: name }))}
value={{ label: project, value: project }}
/>
</div>
</Row>
}
hasBillingPeriod={isBillingPeriodSelected(billingPeriod)}
<UsagePageLayout
isError={isError}
isLoading={isLoading}
limit={limit}
meterNote={
showsContribution(basis, billingPeriod, filtered ? 1 : undefined)
? contributionNote(project, scoped.totals.total, allowanceTotal)
: undefined
alert={
(exceeded || isRestricted) && (
<OverLimitBanner
over={exceeded}
basis={basis}
canUpgrade
isRestricted={isRestricted}
mayBeCharged={
isBilledOnAPeriod(basis) && isChargedForOverages(subscription)
}
/>
)
}
// Nothing to refetch here; passed so FailedToLoad renders its button.
onRetry={() => {}}
periodLabel={periodLabel(periods, billingPeriod)}
planCopy={planSectionCopy(basis, limit)}
showPlanCeiling={showsPlanCeiling(
billingPeriod,
filtered ? 1 : undefined,
)}
total={allowanceTotal}
/>
>
<SectionHeading {...planSectionCopy(basis, limit)} />

<UsageMeter
total={allowanceTotal}
limit={limit}
note={exceeded ? overLimitNote(exceeded) : contribution}
/>

<SectionHeading
title='Explore usage'
hint='Narrow the chart and the breakdown by period or project.'
action={
<Row className='gap-2'>
<div style={FILTER_WIDTH}>
<Select
aria-label='Period'
onChange={(option: PeriodOption) =>
setChosenPeriod(option.value)
}
options={periods}
value={periods.find((option) => option.value === billingPeriod)}
/>
</div>
<div style={FILTER_WIDTH}>
<Select
aria-label='Project'
onChange={(option: { value: string }) =>
setProject(option.value)
}
options={PROJECTS.map((name) => ({ label: name, value: name }))}
value={{ label: project, value: project }}
/>
</div>
</Row>
}
/>

<UsageOverTime
data={scoped}
limit={
showsPlanCeiling(billingPeriod, filtered ? 1 : undefined)
? limit
: undefined
}
isBillingPeriod={isBillingPeriodSelected(billingPeriod)}
periodLabel={periodLabel(periods, billingPeriod)}
/>

<UsageBreakdown
{...breakdown}
onChangeDimension={setDimension}
scope={scope}
/>
</UsagePageLayout>
)
}

Expand All @@ -183,10 +234,30 @@ export const PaidApproachingTheLimit: Story = {
args: { limit: 1400000, subscription: billed },
}

// Billed on a term, so the banner mentions charges.
export const PaidOverTheLimit: Story = {
args: { limit: 900000, subscription: billed },
}

// Only free plans are ever restricted, and this is where they are sent.
export const FreeAndRestricted: Story = {
args: {
isRestricted: true,
limit: 50000,
subscription: subscriptionOf({ plan: 'free' }),
},
}

// The block outlives the overage, and support can set it by hand, so with
// nothing in evidence the banner promises nothing.
export const RestrictedWithNoOverageInEvidence: Story = {
args: {
isRestricted: true,
limit: 5000000,
subscription: subscriptionOf({ plan: 'free' }),
},
}

export const FreeOnARollingWindow: Story = {
args: { limit: 50000, subscription: subscriptionOf({ plan: 'free' }) },
}
Expand All @@ -203,6 +274,17 @@ export const EnterpriseWithoutABillingPeriod: Story = {
},
}

// Invoiced outside Chargebee, so no charge line.
export const EnterpriseOverTheLimit: Story = {
args: {
limit: 1000000,
subscription: subscriptionOf({
payment_method: 'XERO',
plan: 'enterprise',
}),
},
}

// On Chargebee, but no period has arrived. Reads differently from invoiced,
// because this one may resolve itself.
export const ChargebeeWithoutAPeriodYet: Story = {
Expand Down
2 changes: 2 additions & 0 deletions frontend/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ module.exports = {
'^common/(.*)$': '<rootDir>/common/$1',
'^components/(.*)$': '<rootDir>/web/components/$1',
'^project/(.*)$': '<rootDir>/web/project/$1',
// webpack resolves this one too; without it jest cannot follow the app.
'^web/(.*)$': '<rootDir>/web/$1',
},
preset: 'ts-jest',
roots: ['<rootDir>'],
Expand Down
28 changes: 28 additions & 0 deletions frontend/web/__tests__/routePaths.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { isAllowedWhileBlocked } from 'web/routePaths'

// App renders <Blocked /> wherever this is false, so a wrong answer either
// locks a blocked organisation out, or lets it back in.
describe('isAllowedWhileBlocked', () => {
it.each`
pathname | allowed
${'/organisation/7528/usage'} | ${true}
${'/organisations'} | ${true}
${'/organisation/7528/projects'} | ${false}
${'/organisation/7528/settings'} | ${false}
${'/organisation-settings'} | ${false}
${'/project/1/environment/abc/features'} | ${false}
${'/account'} | ${false}
`(
'$pathname is reachable while blocked: $allowed',
({ allowed, pathname }) => {
expect(isAllowedWhileBlocked(pathname)).toBe(allowed)
},
)

// Allowed by pattern, not by prefix.
it('does not open anything nested under the usage page', () => {
expect(isAllowedWhileBlocked('/organisation/7528/usage/breakdown')).toBe(
false,
)
})
})
7 changes: 4 additions & 3 deletions frontend/web/components/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ import Announcement from './Announcement'
import { getBuildVersion } from 'common/services/useBuildVersion'
import AccountProvider from 'common/providers/AccountProvider'
import Nav from './navigation/Nav'
import { isAllowedWhileBlocked } from 'web/routePaths'
import 'project/darkMode'

const App = class extends Component {
static propTypes = {
children: propTypes.element.isRequired,
Expand Down Expand Up @@ -271,9 +273,8 @@ const App = class extends Component {
const environmentId = this.getEnvironmentId(this.props)

if (
AccountStore.getOrganisation() &&
AccountStore.getOrganisation().block_access_to_admin &&
pathname !== '/organisations'
AccountStore.getOrganisation()?.block_access_to_admin &&
!isAllowedWhileBlocked(pathname)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
return <Blocked />
}
Expand Down
5 changes: 5 additions & 0 deletions frontend/web/components/BlockedOrgInfo.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ export default function BlockedOrgInfo() {
<div className='text-nowrap'>
<div>Organisation name: {AccountStore.getOrganisation().name}</div>
<div>Organisation ID: {AccountStore.getOrganisation().id}</div>
<div>
<a href={`/organisation/${AccountStore.getOrganisation().id}/usage`}>
See your usage
</a>
</div>
<div>
<a href='/organisations'>Switch to a different organisation</a>
</div>
Expand Down
Loading
Loading