feat(sponsor-reports): port Sponsor Reports FE into Summit Admin (Sponsors → Reports)#997
feat(sponsor-reports): port Sponsor Reports FE into Summit Admin (Sponsors → Reports)#997caseylocker wants to merge 37 commits into
Conversation
…nsors Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tCSV) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… refund tile) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gial SORT_FIELD_MAP Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…utes) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace placeholder with two MUI card links (Purchase Details, Sponsor Assets); add breadcrumb; add landing_title i18n key; update layout test for card assertions; 6 new tests, 912 total. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, prop, route test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…EAD_ERROR
Remove *_EXPORT_DISABLED action constants (purchase-details, sponsor-asset, drilldown)
and their dead reducer arms. Route onExportDisabled → READ_ERROR in getPurchaseDetailsReport
and getSponsorAssetReport thunks (mirrors the already-fixed drilldown), so a 503
export-disabled on a read path clears loading via the same arm as not-found/unauthorized.
Remove exportDisabled and exportError from all three reducer DEFAULT_STATEs, drop
disabled={exportDisabled} from all three ExportCsvButton usages, and clean up all test
references to the deleted constants and unreachable state. Add 503 export-disabled coverage
for getPurchaseDetailsReport and getSponsorAssetReport matching the existing drilldown tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- SummaryPanel tile labels overline -> subtitle1 (bigger; both reports) - GroupByToggle small -> medium + larger button text - SponsorReportsLayout renders a persistent Reports crumb + per-route page crumb so the trail reads .../Sponsors/Reports/<page> (was dropping to .../Sponsors on sub-pages); landing's own crumb removed (no dup) - Sponsor Assets: gap between Report Filters and summary cards - tests updated (landing breadcrumb moved to layout; layout trail test added) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds sponsor reports configuration, Redux wiring, shared report UI, routes, and two sponsor report pages with supporting utilities and tests. ChangesSponsor reports feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/pages/sponsors/sponsor-reports/__tests__/routing.test.js (1)
33-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest replicates route ordering instead of asserting on the real layout.
Because
renderSwitchhand-copies the<Switch>table rather than mountingsponsor-reports-layout.js, it validates React Router's behavior, not the production ordering. If the real layout's route order drifts, this test won't catch it. The drilldown-vs-landing ordering is already covered against the real component insponsor-reports-layout.test.js, so this can either be removed or driven through the actual layout to guard against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-reports/__tests__/routing.test.js` around lines 33 - 65, This test is duplicating the route table instead of exercising the real sponsor-reports layout, so it won’t catch regressions in the production ordering. Update the routing coverage to mount sponsor-reports-layout.js directly (using the existing SponsorReportsLayout or equivalent wrapper) instead of the local renderSwitch Switch copy, or remove this redundant spec if sponsor-reports-layout.test.js already covers the drilldown-vs-list ordering.src/components/sponsors/reports/FilterBar.js (1)
28-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
draftwon't re-sync if thevalueprop changes after mount.
useState(value)only seeds the initial state; later changes to thevalueprop (e.g. an external filter reset) won't be reflected indraft. Current flows keep parent/child in sync manually (Apply commits the draft, Clear resets both to{}), so there's no active bug, but this coupling is fragile ifvalueever becomes externally driven. Consider syncing via an effect keyed onvalueif that scenario is anticipated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/sponsors/reports/FilterBar.js` around lines 28 - 29, The local draft state in FilterBar is only initialized from the value prop and will not update if that prop changes later. If external changes to value are expected, add synchronization in FilterBar using an effect keyed on value so setDraft stays aligned; keep the update helper unchanged and make sure the draft state refreshes whenever the incoming value object changes.src/reducers/sponsors/__tests__/sponsor-reports-reducers.test.js (1)
110-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
PURCHASE_DETAILS_VALIDATION_CLEAR.This file validates the set path for
validationError, but not the clear action branch; add one test to prevent regressions in inline/toast reset flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/sponsors/__tests__/sponsor-reports-reducers.test.js` around lines 110 - 124, Add a test for the PURCHASE_DETAILS_VALIDATION_CLEAR branch in purchaseDetailsReducer to cover the reset path for validationError and loading state. Use the existing PURCHASE_DETAILS_VALIDATION_ERROR test in sponsor-reports-reducers.test.js as a guide, but assert that dispatching PURCHASE_DETAILS_VALIDATION_CLEAR clears validationError without replacing the current data/body so the inline/toast reset flow stays protected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/actions/sponsor-reports-actions.js`:
- Around line 34-54: Add an early guard in getPurchaseDetailsReport and
getPurchaseDetailsFilters to check that currentSummit and currentSummit.id exist
before calling dispatch(startLoading()) or base(currentSummit.id). If the summit
is missing, exit the thunk immediately (optionally dispatch a validation/error
action) so the synchronous throw never happens and stopLoading() is not skipped.
Keep the fix localized in sponsor-reports-actions.js and use the existing thunk
names to update both purchase-details actions consistently.
In `@src/utils/reports-money.js`:
- Around line 10-13: Update formatUsd in reports-money.js so blank or
whitespace-only strings are treated as missing values instead of being converted
by Number(). Add an explicit check before parsing string input (for example in
the formatUsd path) to return "—" when the value is empty after trimming, while
keeping the existing USD.format behavior for valid numeric values.
---
Nitpick comments:
In `@src/components/sponsors/reports/FilterBar.js`:
- Around line 28-29: The local draft state in FilterBar is only initialized from
the value prop and will not update if that prop changes later. If external
changes to value are expected, add synchronization in FilterBar using an effect
keyed on value so setDraft stays aligned; keep the update helper unchanged and
make sure the draft state refreshes whenever the incoming value object changes.
In `@src/pages/sponsors/sponsor-reports/__tests__/routing.test.js`:
- Around line 33-65: This test is duplicating the route table instead of
exercising the real sponsor-reports layout, so it won’t catch regressions in the
production ordering. Update the routing coverage to mount
sponsor-reports-layout.js directly (using the existing SponsorReportsLayout or
equivalent wrapper) instead of the local renderSwitch Switch copy, or remove
this redundant spec if sponsor-reports-layout.test.js already covers the
drilldown-vs-list ordering.
In `@src/reducers/sponsors/__tests__/sponsor-reports-reducers.test.js`:
- Around line 110-124: Add a test for the PURCHASE_DETAILS_VALIDATION_CLEAR
branch in purchaseDetailsReducer to cover the reset path for validationError and
loading state. Use the existing PURCHASE_DETAILS_VALIDATION_ERROR test in
sponsor-reports-reducers.test.js as a guide, but assert that dispatching
PURCHASE_DETAILS_VALIDATION_CLEAR clears validationError without replacing the
current data/body so the inline/toast reset flow stays protected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 878ce767-9e7a-43b8-97cf-abd302c54c9b
📒 Files selected for processing (57)
.env.examplesrc/actions/__tests__/sponsor-reports-actions.test.jssrc/actions/sponsor-reports-actions.jssrc/app.jssrc/components/menu/menu-definition.jssrc/components/sponsors/reports/ExportCsvButton.jssrc/components/sponsors/reports/FilterBar.jssrc/components/sponsors/reports/GroupByComponentView.jssrc/components/sponsors/reports/GroupBySponsorView.jssrc/components/sponsors/reports/GroupByToggle.jssrc/components/sponsors/reports/OrdersTable.jssrc/components/sponsors/reports/ReportShell.jssrc/components/sponsors/reports/StatusPill.jssrc/components/sponsors/reports/StatusRollupChips.jssrc/components/sponsors/reports/SummaryPanel.jssrc/components/sponsors/reports/TierBadge.jssrc/components/sponsors/reports/__tests__/ExportCsvButton.test.jssrc/components/sponsors/reports/__tests__/FilterBar.test.jssrc/components/sponsors/reports/__tests__/GroupByComponentView.test.jssrc/components/sponsors/reports/__tests__/GroupBySponsorView.test.jssrc/components/sponsors/reports/__tests__/GroupByToggle.test.jssrc/components/sponsors/reports/__tests__/OrdersTable.test.jssrc/components/sponsors/reports/__tests__/ReportShell.test.jssrc/components/sponsors/reports/__tests__/StatusPill.test.jssrc/components/sponsors/reports/__tests__/StatusRollupChips.test.jssrc/components/sponsors/reports/__tests__/SummaryPanel.test.jssrc/components/sponsors/reports/__tests__/TierBadge.test.jssrc/components/sponsors/reports/__tests__/statusTone.test.jssrc/components/sponsors/reports/__tests__/usePrint.test.jssrc/components/sponsors/reports/report-print.csssrc/components/sponsors/reports/statusTone.jssrc/components/sponsors/reports/usePrint.jssrc/i18n/en.jsonsrc/layouts/__tests__/sponsor-reports-layout.test.jssrc/layouts/sponsor-layout.jssrc/layouts/sponsor-reports-layout.jssrc/pages/sponsors/sponsor-reports/__tests__/routing.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/index.jssrc/pages/sponsors/sponsor-reports/reports-landing-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/reports-landing-page/index.jssrc/pages/sponsors/sponsor-reports/sponsor-asset-drilldown-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/sponsor-asset-drilldown-page/index.jssrc/pages/sponsors/sponsor-reports/sponsor-asset-report-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/sponsor-asset-report-page/index.jssrc/reducers/sponsors/__tests__/sponsor-reports-reducers.test.jssrc/reducers/sponsors/sponsor-reports-drilldown-reducer.jssrc/reducers/sponsors/sponsor-reports-purchase-details-reducer.jssrc/reducers/sponsors/sponsor-reports-sponsor-asset-reducer.jssrc/store.jssrc/utils/constants.jssrc/utils/report-errors.jssrc/utils/report-query.jssrc/utils/reports-api.jssrc/utils/reports-money.jssrc/utils/reports-text.jssrc/utils/section-csv-query.js
- guard all report thunks against a missing currentSummit (prevents a stuck spinner from base(currentSummit.id) throwing after startLoading) - formatUsd: treat blank/whitespace strings + non-finite numbers as missing (em dash) instead of $0.00; add reports-money tests - FilterBar: re-sync draft when the committed value prop changes externally - drop redundant routing.test.js (real-layout integration test in sponsor-reports-layout.test.js already covers route ordering) - add PURCHASE_DETAILS_VALIDATION_CLEAR reducer test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Ports the Sponsor Reports frontend (Purchase Details + Sponsor Assets, including per-sponsor drill-down) into summit-admin under Sponsors → Reports, using the existing admin-sponsors access gate and the in-context summit selection.
Changes:
- Adds a new Sponsor Reports layout + routes (landing, purchase details, sponsor assets, drill-down) and wires it into the Sponsors menu.
- Implements report UI building blocks (report shell, filters, grouping views, CSV export, print) plus supporting query/text/money utilities.
- Introduces Redux actions/reducers for sponsor reports and adds extensive unit/integration test coverage + i18n strings.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/section-csv-query.js | Builds section-scoped CSV export query while preserving unrelated query params. |
| src/utils/reports-text.js | HTML-to-plain-text helper for report text fields. |
| src/utils/reports-money.js | USD formatting helper for report summary/rows. |
| src/utils/reports-api.js | Reports API base URL helper + positive-int route ID validator. |
| src/utils/report-query.js | Translates report filter UI state into base-api-utils query params. |
| src/utils/report-errors.js | Normalizes/classifies report API errors and provides a getRequest-shaped handler factory. |
| src/utils/constants.js | Adds HTTP 503 constant used by report error classification. |
| src/utils/tests/reports-money.test.js | Unit tests for USD formatting helper. |
| src/store.js | Registers sponsor reports reducers and excludes their state from persistence. |
| src/reducers/sponsors/sponsor-reports-sponsor-asset-reducer.js | Sponsor Assets grouped report reducer. |
| src/reducers/sponsors/sponsor-reports-purchase-details-reducer.js | Purchase Details report reducer (incl. validation vs read-error split). |
| src/reducers/sponsors/sponsor-reports-drilldown-reducer.js | Sponsor Assets drill-down reducer. |
| src/reducers/sponsors/tests/sponsor-reports-reducers.test.js | Reducer unit tests covering all sponsor reports reducers. |
| src/pages/sponsors/sponsor-reports/sponsor-asset-report-page/index.js | Sponsor Assets grouped report page (group-by, filters, pagination, CSV/print). |
| src/pages/sponsors/sponsor-reports/sponsor-asset-report-page/tests/index.test.js | Sponsor Assets report page tests (dispatching, toggles, states, CSV props). |
| src/pages/sponsors/sponsor-reports/sponsor-asset-drilldown-page/index.js | Per-sponsor drill-down page (section cards, content rendering, per-section CSV). |
| src/pages/sponsors/sponsor-reports/sponsor-asset-drilldown-page/tests/index.test.js | Drill-down page tests including ContentCell behaviors. |
| src/pages/sponsors/sponsor-reports/reports-landing-page/index.js | Reports landing page with two navigation cards. |
| src/pages/sponsors/sponsor-reports/reports-landing-page/tests/index.test.js | Landing page tests for cards and links. |
| src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js | Purchase Details report page (filters, sorting, pagination, CSV/print, tiles). |
| src/pages/sponsors/sponsor-reports/purchase-details-report-page/tests/index.test.js | Purchase Details page tests including filter->query behavior and CSV dispatch. |
| src/layouts/sponsor-reports-layout.js | Sponsor Reports sub-router + breadcrumb composition + admin gate. |
| src/layouts/sponsor-layout.js | Adds /reports route under Sponsors layout. |
| src/layouts/tests/sponsor-reports-layout.test.js | Layout routing + Restrict gate integration tests (landing, unauthorized, drilldown, crumbs). |
| src/i18n/en.json | Adds menu label + Sponsor Reports page strings. |
| src/components/sponsors/reports/usePrint.js | Hook wrapper around window.print() for reports. |
| src/components/sponsors/reports/TierBadge.js | Tier badge chip with explicit tier color mapping. |
| src/components/sponsors/reports/SummaryPanel.js | Shared summary tiles panel component. |
| src/components/sponsors/reports/statusTone.js | Central status->chip-tone mapping helper. |
| src/components/sponsors/reports/StatusRollupChips.js | Renders stable-order rollup chips for grouped Sponsor Assets cards. |
| src/components/sponsors/reports/StatusPill.js | Single status chip component with shared tone mapping. |
| src/components/sponsors/reports/ReportShell.js | Shared report page shell (header, action slot, filter slot, print CSS hook). |
| src/components/sponsors/reports/report-print.css | Print-only CSS to print report body. |
| src/components/sponsors/reports/OrdersTable.js | Purchase Details table built on uicore MuiTable + date/time formatting helpers. |
| src/components/sponsors/reports/GroupByToggle.js | Toggle for sponsor/component grouping mode. |
| src/components/sponsors/reports/GroupBySponsorView.js | Sponsor-grouped card list with drill-down links. |
| src/components/sponsors/reports/GroupByComponentView.js | Component-grouped card list with per-sponsor links and content previews. |
| src/components/sponsors/reports/FilterBar.js | Shared report filter bar (sponsor multiselect + optional search + extra controls). |
| src/components/sponsors/reports/ExportCsvButton.js | CSV export button integrating uicore getCSV and access token retrieval. |
| src/components/sponsors/reports/tests/usePrint.test.js | Unit test for print hook. |
| src/components/sponsors/reports/tests/TierBadge.test.js | Unit tests for TierBadge rendering behavior. |
| src/components/sponsors/reports/tests/SummaryPanel.test.js | Unit tests for SummaryPanel rendering and empty behavior. |
| src/components/sponsors/reports/tests/statusTone.test.js | Unit tests for status tone mapping helper. |
| src/components/sponsors/reports/tests/StatusRollupChips.test.js | Unit tests for rollup chips ordering and null rollup behavior. |
| src/components/sponsors/reports/tests/StatusPill.test.js | Unit tests for StatusPill label behavior and tone mapping re-export. |
| src/components/sponsors/reports/tests/ReportShell.test.js | Unit tests for ReportShell slots and icon rendering. |
| src/components/sponsors/reports/tests/OrdersTable.test.js | Unit tests for table helpers and basic rendering/sort interactions. |
| src/components/sponsors/reports/tests/GroupByToggle.test.js | Unit tests for group toggle behavior (ignore null toggle). |
| src/components/sponsors/reports/tests/GroupBySponsorView.test.js | Unit tests for sponsor-grouped card rendering and link targets. |
| src/components/sponsors/reports/tests/GroupByComponentView.test.js | Unit tests for component-grouped rendering, content hints, and links. |
| src/components/sponsors/reports/tests/FilterBar.test.js | Unit tests for FilterBar sponsorIds emission and optional search box. |
| src/components/sponsors/reports/tests/ExportCsvButton.test.js | Unit tests for CSV dispatch args, disabling, and in-flight click guard. |
| src/components/menu/menu-definition.js | Adds Sponsors → Reports menu entry guarded by admin-sponsors. |
| src/app.js | Injects SPONSOR_REPORTS_API_URL into window config. |
| src/actions/sponsor-reports-actions.js | Thunks for reports endpoints (purchase details, sponsor assets, filters, drilldown). |
| src/actions/tests/sponsor-reports-actions.test.js | Unit tests for sponsor reports thunks and read-error handler behavior. |
| .env.example | Adds SPONSOR_REPORTS_API_URL example env var. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- reports-text: decode uppercase/mixed-case HTML entities (the case-insensitive
match was missing the lowercase ENTITIES map); add toPlainText tests
- report-errors: export-disabled now dispatches { kind, status, message } to
match the other error branches
- report-query: coerce sponsorIds to positive integers and drop the rest so the
filter never emits sponsor_id==NaN/==0; add buildReportQuery tests
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iew) Per Sebastian's PR review: summit-admin now requests the new microservice's own scopes (sponsor-reports/read, sponsor-reports/export) instead of relying on the broad reports/all. Adds SPONSOR_REPORTS_SCOPES and wires it into SCOPES (reports/all kept for the legacy summit-reports-api). Requires the IDP to grant these two scopes to the summit-admin client. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 26: The SCOPES aggregate still includes reports/all, which is broader
than needed and conflicts with the dedicated sponsor report scopes. Update the
SCOPES value in .env.example to remove reports/all while keeping the existing
scoped report permissions such as SPONSOR_REPORTS_SCOPES and the other named
scope variables intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05a2e27c-7d13-497e-bd72-9787506508fa
📒 Files selected for processing (12)
.env.examplesrc/actions/__tests__/sponsor-reports-actions.test.jssrc/actions/sponsor-reports-actions.jssrc/components/sponsors/reports/FilterBar.jssrc/reducers/sponsors/__tests__/sponsor-reports-reducers.test.jssrc/utils/__tests__/report-query.test.jssrc/utils/__tests__/reports-money.test.jssrc/utils/__tests__/reports-text.test.jssrc/utils/report-errors.jssrc/utils/report-query.jssrc/utils/reports-money.jssrc/utils/reports-text.js
✅ Files skipped from review due to trivial changes (2)
- src/utils/tests/reports-text.test.js
- src/utils/tests/reports-money.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/utils/reports-money.js
- src/utils/report-query.js
- src/utils/report-errors.js
- src/reducers/sponsors/tests/sponsor-reports-reducers.test.js
- src/actions/tests/sponsor-reports-actions.test.js
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…istration Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ifest Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tails page Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Two low-severity findings from the deep review — both are code-quality nits with no functional impact on the feature. @caseylocker tagging you for awareness.
| // Defense-in-depth: callers pass route/backend integer ids (the drill-down page | ||
| // validates :sponsorId/:summitId before rendering). Never interpolate a | ||
| // non-integer value into a filter clause sent to the CSV endpoint. | ||
| if (Number.isInteger(sid)) kept.push(`sponsor_id==${sid}`); |
There was a problem hiding this comment.
[LOW] Defense-in-depth guard does not match its comment.
Number.isInteger(0) is true, so a sponsorId of "0" would emit sponsor_id==0 into the filter clause. The drilldown page validates upstream with isPositiveIntId, so there is no live attack path — but the guard here does not live up to the comment above it ("Never interpolate a non-integer value").
Suggestion: change to sid > 0 (also handles NaN since NaN > 0 is false), and same for pid on the next line.
There was a problem hiding this comment.
Done in e06b1e9e. Reworded the comment to match the guard — it now describes the sponsor_id/page_id integer check and drops the stale :summitId reference (this function doesn't touch summit).
| // MuiTable keys rows on row.id; the API exposes purchase_id, not id. | ||
| // The page must call rows.map(r => ({ ...r, id: r.purchase_id })) before | ||
| // passing data, or use this helper for explicit mapping. | ||
| export const getOrderRowId = (row) => row.purchase_id; |
There was a problem hiding this comment.
[LOW] Dead export with a misleading comment.
The comment on lines 43–45 says "The page must call rows.map(r => ({ ...r, id: r.purchase_id })) before passing data, or use this helper" — but OrdersTable already does that remapping internally in the data prop passed to MuiTable. getOrderRowId is never imported anywhere in the codebase.
A future developer reading this comment might add a redundant .map() in the page component, producing double-mapped rows.
Suggestion: either remove the export entirely, or update the comment to clarify the remapping is already done inside OrdersTable.
There was a problem hiding this comment.
Done in e06b1e9e. Confirmed unused — the table maps purchase_id → id inline at the data= prop; only its own test referenced it. Removed the export, comment, and test.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Convention audit against the established codebase patterns. 4 inline findings below — none are blockers, but two are 1-line fixes. @caseylocker
| PURCHASE_DETAILS_VALIDATION_CLEAR | ||
| } from "../../../../actions/sponsor-reports-actions"; | ||
|
|
||
| const DEFAULT_PAGE_SIZE = 10; |
There was a problem hiding this comment.
[Convention] DEFAULT_PAGE_SIZE should be imported from constants.js, not redefined locally.
src/utils/constants.js already exports DEFAULT_PER_PAGE = 10. Defining the same value under a different name here is a 1-line fix:
import { DEFAULT_PER_PAGE } from '../../../../utils/constants';
// then replace DEFAULT_PAGE_SIZE → DEFAULT_PER_PAGE belowThere was a problem hiding this comment.
Done in e06b1e9e — now imports DEFAULT_PER_PAGE from constants.js. (The Line Items view keeps a local LINES_DEFAULT_PAGE_SIZE = 50, since there's no matching constant for 50.)
|
|
||
| // Local pagination/sort state. MuiTable dir = 1 (asc) | -1 (desc). | ||
| const [filters, setFilters] = useState({}); | ||
| const [currentPage, setCurrentPage] = useState(1); |
There was a problem hiding this comment.
[Convention] Pagination/sort state is in local useState instead of Redux — deviation from the list-page pattern.
The established pattern (summit-admin-list-page-pattern.md) stores currentPage, perPage, order, orderDir in the Redux reducer via the 5th argument of getRequest, so the REQUEST_* case can sync them:
// Standard pattern
case REQUEST_ITEMS: {
const { currentPage, perPage, order, orderDir, term } = payload;
return { ...state, currentPage, perPage, order, orderDir, term };
}Here those four values live in useState and REQUEST_PURCHASE_DETAILS only sets loading: true. Same deviation exists in sponsor-asset-report-page.
Practical difference: a page refresh or summit switch always resets pagination to page 1 (because local state is lost on unmount). That may be intentional for a report view — if so, worth a brief comment saying so to signal it was a deliberate choice rather than an oversight.
There was a problem hiding this comment.
Left as local useState for now. These are read-only report views with ephemeral, page-local pagination/sort that nothing else in the app reads — a Redux slice would add actions/reducer for state that never leaves the component. The Line Items view stacked on this PR mirrors the same local pattern, so moving it would be a broader change across both grids. I'd lean toward keeping it local and revisiting if another view ever needs to read this state — happy to match the list-page pattern if you feel strongly.
| // Function form of mapDispatchToProps: injects raw dispatch (needed for the | ||
| // PURCHASE_DETAILS_VALIDATION_CLEAR action in the Snackbar handler) alongside | ||
| // the bound action creators. | ||
| const mapDispatchToProps = (dispatch) => ({ |
There was a problem hiding this comment.
[Convention] mapDispatchToProps in function form — the rest of the codebase uses the object shorthand.
The function form is used here to get raw dispatch for the Snackbar's onClose handler (dispatch({ type: PURCHASE_DETAILS_VALIDATION_CLEAR })). The conventional fix is to export a named action creator and map it normally:
// In sponsor-reports-actions.js
export const clearPurchaseDetailsValidation = () => ({
type: PURCHASE_DETAILS_VALIDATION_CLEAR
});
// In the page
export default connect(mapStateToProps, {
getPurchaseDetailsReport,
getPurchaseDetailsFilters,
clearPurchaseDetailsValidation
})(PurchaseDetailsReportPage);Not a blocker, but keeps the pattern consistent across all pages.
There was a problem hiding this comment.
Done in e06b1e9e. Converted to object shorthand. The function form only needed raw dispatch to clear the validation toast, so I added a clearPurchaseDetailsValidation() action creator and bound it via the object form — the raw dispatch prop is gone.
| // Prints the currently loaded report body only (server-paginated page). | ||
| // v1 caveat: captures only the currently loaded page, not the full filtered view. | ||
| // A true full-report print would need a print-mode fetch of all pages — out of scope. | ||
| const usePrint = () => useCallback(() => window.print(), []); |
There was a problem hiding this comment.
[Convention] Custom hook should live in src/hooks/, not co-located with components.
The project's canonical location for custom hooks is src/hooks/ (see src/hooks/useScrollToError.js). Moving this file there is a 1-line import path change in the two pages that consume it.
There was a problem hiding this comment.
Done in e06b1e9e. Moved to src/hooks/usePrint.js (next to useScrollToError) and updated the three reports pages' imports.
|
|
||
| const query = {}; | ||
| if (filter.length > 0) query["filter[]"] = filter; | ||
| if (search) query.search = search; |
There was a problem hiding this comment.
[Convention] Free-text search param is not passed through escapeFilterValue.
The project rule (summit-admin-redux-actions.md) says: "Always use escapeFilterValue(term) before inserting user input into filters." Here search goes directly as a query param rather than into a filter[] clause, so the immediate injection risk is lower — but the guidance covers user-supplied strings broadly.
Worth confirming with the backend team whether the search param on the reports API needs escaping on the client side, or whether the server handles it. If not needed, a short comment here would clarify why the escape was intentionally omitted.
There was a problem hiding this comment.
The search value goes through as a top-level query param, not into a filter[] clause, so escapeFilterValue (which escapes filter operators) doesn't directly apply.
|
Addressing the two remaining false positives from CodeRabbit's nitpick summary (review #4567585391):
The useEffect(() => {
setDraft(value);
}, [value]);The comment above it even explains the intent: "Re-sync the draft when the committed
The test is present, starting at line 127: describe("PURCHASE_DETAILS_VALIDATION_CLEAR", () => {
it("clears validationError without replacing the body", () => {
...
expect(result.validationError).toBeNull();
});
});CodeRabbit scanned lines 110–124 (the set path) and didn't look at the lines immediately following. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js`:
- Around line 126-141: The three effects in
purchase-details-report-page/index.js are missing the in-context summit as a
dependency, so they won’t rerun when the summit changes after mount. Update the
useEffect hooks that call fetchFilters, fetchReport, and fetchLinesReport to
depend on currentSummit?.id (or the equivalent summit identifier from the
component scope) along with the existing inputs so the page refetches filters
and reports when the active summit is hydrated or switched. Keep the mount-only
behavior only if it still includes summit changes as a trigger for the
data-loading effects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aceb4674-64b2-4907-afd9-8c0ad10a05a7
📒 Files selected for processing (14)
src/actions/__tests__/sponsor-reports-actions.test.jssrc/actions/sponsor-reports-actions.jssrc/components/sponsors/reports/LinesManifestView.jssrc/components/sponsors/reports/ReportViewToggle.jssrc/components/sponsors/reports/__tests__/LinesManifestView.test.jssrc/components/sponsors/reports/__tests__/ReportViewToggle.test.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-reports/purchase-details-report-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/index.jssrc/reducers/sponsors/__tests__/sponsor-reports-purchase-details-lines-reducer.test.jssrc/reducers/sponsors/sponsor-reports-purchase-details-lines-reducer.jssrc/store.jssrc/utils/__tests__/manifest-grouping.test.jssrc/utils/manifest-grouping.js
✅ Files skipped from review due to trivial changes (1)
- src/utils/manifest-grouping.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/store.js
- src/i18n/en.json
- src/actions/tests/sponsor-reports-actions.test.js
- index.js: import DEFAULT_PER_PAGE from constants (drop local DEFAULT_PAGE_SIZE dup) - index.js: mapDispatchToProps to object shorthand via new clearPurchaseDetailsValidation action creator (removes the raw dispatch prop) - OrdersTable.js: remove dead getOrderRowId export + its test (page maps id inline) - usePrint: move to src/hooks/ per the codebase custom-hook convention - section-csv-query.js: align defense-in-depth comment with the sponsor_id/page_id guard Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Purchase Details page mapStateToProps reads sponsorReportsPurchaseDetailsLinesState.data; the SponsorReportsLayout test mounts the real page, so its mock store must include that slice. Missing it threw 'Cannot read properties of undefined (reading data)' under the full yarn test run (CI), though the scoped report-suite runs passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…inks Hashed asset filenames (32-char hex prefix, no spaces) overflowed their card. The link Typography had noWrap (white-space:nowrap). Switch the link to a flex row with overflowWrap:anywhere + minWidth:0 on the filename so the unbroken hash wraps within the card; icons get flexShrink:0 so they don't squash. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The app sets html root font-size to 62.5% (10px), so rem-based sizes render tiny: the toggle override read 8.75px and subtitle1 tile labels 10px. Pin px instead — toggles to 14px/500 to match the Print/Export action buttons, and the summary-tile labels to 14px. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add SponsorAvatar: logo-first with a colored-initials fallback so a no-logo (or white-on-white) sponsor is never an invisible blank. Styled to mimic the ReportShell title icon — single primary.light rounded square with white foreground — and used in both group-by views and the drill-down header. Switch the ReportShell title icon glyph to white to match. Also wrap the component-view asset filename (was truncated by noWrap+maxWidth) so the full name is readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Render the Export CSV button on the Line Items (per-line) view of the Purchase Details report, wired to the new backend GET .../purchase-details/lines/csv endpoint. One ExportCsvButton switched by view; lines query strips only pagination (preserves filters incl. the derived include_cancelled). Orders export unchanged. Label stays 'Export CSV'; filename purchase-details-lines-summit-<id>.csv. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js (1)
196-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold the duplicated CSV url/query derivation into a small helper.
linesCsvUrl/linesCsvQueryrepeat the exact base-URL construction andpage/per_pagestripping already done for the orders export just above. A tiny helper (e.g.buildCsv(path, srcQuery)returning{ url, query }) would remove the copy and keep both exports in sync if the strip rules change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js` around lines 196 - 208, The CSV export setup for purchase details duplicates the same URL construction and pagination-stripping logic already used for the orders export. Extract that repeated logic into a small shared helper in this page (for example, a function that builds the CSV url and query from a path and source query), then use it for both exports so `linesCsvUrl` and `linesCsvQuery` stay aligned with the existing order export behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js`:
- Around line 196-208: The CSV export setup for purchase details duplicates the
same URL construction and pagination-stripping logic already used for the orders
export. Extract that repeated logic into a small shared helper in this page (for
example, a function that builds the CSV url and query from a path and source
query), then use it for both exports so `linesCsvUrl` and `linesCsvQuery` stay
aligned with the existing order export behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a24726cb-2af3-4f33-a505-8cb87decc70c
📒 Files selected for processing (2)
src/pages/sponsors/sponsor-reports/purchase-details-report-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
Hey @caseylocker, this PR doesn't follow our codebase's coding patterns. For example, you added several files to the utils dir that are specific to these reports. No other page or section of the admin does this — if it did, utils would end up with dozens of one-off files, defeating the purpose of having a generic utils dir.
I left a few examples inline. Could you go through the PR and align the solution with summit-admin's coding patterns?
| @@ -0,0 +1,7 @@ | |||
| export const getReportsApiBaseUrl = () => window.SPONSOR_REPORTS_API_URL; | |||
There was a problem hiding this comment.
having one utils file per page is not scalable, please remove this unnecessary helper method and move isPositiveIntId to utils/method.js file since it is a generic helper not tied to reports api
There was a problem hiding this comment.
Done — moved isPositiveIntId to utils/methods.js (fd1296c0) and deleted reports-api.js, repointing the import (6757a71d). No per-page utils file remains.
|
|
||
| // Formats a DOLLAR amount (number or numeric string) as "$1,234.56". | ||
| // Non-numeric / null → em dash. | ||
| export const formatUsd = (value) => { |
There was a problem hiding this comment.
we already have a currency parser in uicore, please use that and remove this file
There was a problem hiding this comment.
Done — switched to uicore currencyAmountFromCents and deleted reports-money.js (a18a6d5c). Since the uicore parser is cents-based, I also moved the reports JSON API to a cents contract (it was returning dollar strings): fntechgit/sponsor-reports-api#29. The CSV export stays in dollars for readability.
Two notes: that backend PR must deploy before/with this one (the FE now feeds these values straight to currencyAmountFromCents), and money renders without a thousands separator ($1536.00) to match the rest of summit-admin.
| " ": " " | ||
| }; | ||
|
|
||
| export const toPlainText = (html) => { |
There was a problem hiding this comment.
we already have a method that does this in uicore, please use that and remove this file
There was a problem hiding this comment.
Done — deleted reports-text.js (500cab97). I checked uicore first: neither utils/methods nor anywhere else in the package exports an html→text helper, and summit-admin's existing htmlToString uses documentElement.textContent, which fuses adjacent-tag text (<p>a</p><b>b</b> → ab). So I added a generic htmlToPlainText to methods.js (fd1296c0) — tags→space + DOMParser entity decode — and pointed both consumers at it. If you did have a specific uicore helper in mind, point me at it and I'll switch.
| // stripping the sponsor clause and re-emitting `status==Paid` as its own bracket turns | ||
| // OR into AND. The v1 query builder never emits mixed brackets (sponsor is always its | ||
| // own bracket), so this is a defensive edge, not a live path. | ||
| export const buildSectionCsvQuery = ( |
There was a problem hiding this comment.
this is only used in reports action file, lets move it there and remove this file
There was a problem hiding this comment.
Done — folded into the exportSponsorAssetSectionCsv action thunk and deleted the file (306ce261).
| setBusy(true); | ||
| try { | ||
| const token = await getAccessTokenSafely(); | ||
| dispatch(getCSV(url, { ...query, access_token: token }, filename)); |
There was a problem hiding this comment.
create a method in actions file the builds the query based on page, id , filters props like every other export method in summit_admin. We need to keep a pattern in our code ir order to make it easier to understand it
There was a problem hiding this comment.
Done — all four CSV exports are now action thunks that own url/params/filename like exportEventRsvpsCSV: exportPurchaseDetailsCsv/exportPurchaseDetailsLinesCsv (a0310a18) and exportSponsorAssetCsv/exportSponsorAssetSectionCsv (306ce261); the pages dispatch them from a plain MUI button and ExportCsvButton.js is gone.
Two intentional consequences worth flagging: (1) the plain buttons drop the old in-flight double-click guard — that matches every other export in summit-admin (none debounce), so I left it off rather than re-add a bespoke wrapper; (2) the thunks put access_token first ({ access_token, ...query }) to match the sibling fetch thunks in the same file, so the serialized query-string order differs from the old button — param values are identical and the backend is order-independent.
…ethods.js Generic helpers move to the shared methods.js home (convention). htmlToPlainText preserves tag-boundary whitespace + decodes entities (htmlToString fuses words). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…se query builders Move report-query under the feature folder (feature helper convention). Add buildPurchaseQuery/buildPurchaseLinesQuery so the on-screen fetch and the upcoming export thunks share one date/order normalization (no duplication, no regression). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…text util Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move orders/lines CSV URL+params+filename building into exportPurchaseDetailsCsv/ exportPurchaseDetailsLinesCsv (cf. exportEventRsvpsCSV); page dispatches them from a plain MUI Button. Orders CSV keeps the on-screen sort. base() now reads window.SPONSOR_REPORTS_API_URL directly (drops getReportsApiBaseUrl import). currentSummit removed from component props/mapStateToProps (no longer needed after CSV URL building moved to the thunks). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…; drop ExportCsvButton Add exportSponsorAssetCsv/exportSponsorAssetSectionCsv (section logic folded in, simplified); pages dispatch from plain buttons. Remove the ported ExportCsvButton and section-csv-query util. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tId from methods
- sponsor-asset-report-page/index.js: repoint isPositiveIntId import to utils/methods
- sponsor-asset-drilldown-page/index.js: merge htmlToPlainText + isPositiveIntId into one
import from utils/methods (fixes import/no-duplicates)
- sponsor-asset-report-page/__tests__: remove jest.mock("reports-api") — pure fn, no mock needed
- sponsor-asset-drilldown-page/__tests__: same
- layouts/__tests__/sponsor-reports-layout.test.js: remove obsolete reports-api mock + comment
- purchase-details-report-page/__tests__: rename stale "ExportCsvButton" test label to
"export button" (authorized micro-cleanup; component deleted in Task 5)
- git rm src/utils/reports-api.js — no longer consumed by any source or test
- 115 suites / 966 tests passing; 0 new lint errors
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t of shared utils Both files moved into src/pages/sponsors/sponsor-reports/ per §2 (feature helpers never in shared utils/); test for reports-money co-located alongside. Internal ./constants import in report-errors updated to ../../../utils/constants. formatUsd logic unchanged — uicore/cents contract deferred to PR-B. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rors.js ./constants was not re-staged after the path update; this corrects it to ../../../utils/constants (resolves from the new feature folder location). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…998) Keep the branch current with the master tip per smarcet's request (Core Team Sync 2026-06-26). Picks up 350c73d (#998 speaker-list activity count); clean auto-merge, en.json edits are in non-overlapping regions. No sponsor-reports files affected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Backend now emits money fields as integer cents. Drop the bespoke reports-money.js formatter (per santi PR #997 review) and use the platform-wide currencyAmountFromCents from openstack-uicore-foundation. Null guard via inline/local const rather than a new shared helper file. Test fixtures updated to cents input; assertions to no-comma "$X.XX" output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex follow-up: the reducer passes summary payload through unchanged, so the dollar-string fixtures were not a live bug, but they're stale under the cents contract. Switch total_paid fixtures to integer cents to stay representative. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@santipalenque Thank you very much for the excellent review. These are great callouts and will help me to align closer with code patterns in the project going forward. I believe that I've addressed all of your concerns with the latest push. Let me know if I missed something, especially with this one #997 (comment). |
ref: https://app.clickup.com/t/86bak754x
Ports the two sponsor reports (Purchase Details + Sponsor Assets, incl. the per-sponsor asset drill-down) out of
sponsor-servicesand into summit-admin as an admin-only "Reports" sub-item under the Sponsors menu, reading the in-context summit. Backend (sponsor-reports-api) is unchanged. This is Phase 1 — parity port; the JP-pivot logistics/finance features are Phase 2 follow-ons.What's included
Restrict(withRouter(SponsorReportsLayout), "admin-sponsors")— an explicit admin-only gate that reuses the existingadmin-sponsorskey and closes the direct-URL gap for the reports area. Summit is in-context (summit picker +/reports/summitsdropped).connect()(no hooks), react-router 4withRouter(no v6 hooks), i18n-reactT.translate(no react-i18next), uicoreMuiTable(no@mui/x-data-grid), CSV via uicoregetCSV.OrdersTablerebuilt onMuiTable;formatCheckoutTimehandles both ISO and epoch; Total Refunded tile renders only whensummary.total_refunded != null(auto-appears once that field ships — no FE change needed)./app/summits/:summitId/sponsors/reports/sponsor-assets/sponsors/:sponsorId.SPONSOR_REPORTS_API_URLinjected inapp.js+.env/.env.example.Tests / quality
yarn jest: 915 passing, 0 regressions vs master.yarn lint: 0 new errors (only the repo's pre-existing prop-types warnings).Deploy note (not blocking this PR)
Backend auth wiring (D5) is done on dev — repointed the deployed
sponsor-reports-apiscope env toreports/all+ added the admin groups; verified both reports load real data on dev. Staging/prod need the same env at launch (tracked in the ClickUp ticket). No IDP change was needed (summit-admin already carriesreports/all).🤖 Generated with Claude Code
Summary by CodeRabbit