feat(sponsor-reports): add By Item view to Purchase Details report#1009
Conversation
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a third sponsor purchase-details view that fetches all By Item rows, groups them by sponsor and item, supports client-side pagination and expansion, and integrates filters, exports, Redux state, translations, and stale-request handling. ChangesPurchase Details By Item
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new “By Item” view to the Sponsors → Reports → Purchase Details report, including whole-set line fetching, client-side rollups by sponsor + item code, UI rendering (accordion → item table → contributing-orders drill-down), and view-specific paging/filter/export behavior.
Changes:
- Added a new redux slice + thunk (
getPurchaseDetailsByItemRows) to fetch all filtered purchase-detail line rows and store them atomically for client-side aggregation. - Implemented the By Item UI (
ByItemView) and integrated it into the Purchase Details report page with a third view toggle state. - Added i18n strings and comprehensive unit/integration tests for reducer/thunk/page wiring and the new view.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/store.js | Registers and persists the new sponsorReportsPurchaseDetailsByItemState slice. |
| src/reducers/sponsors/sponsor-reports-purchase-details-by-item-reducer.js | New reducer slice for whole-set line rows + client paging state. |
| src/reducers/sponsors/tests/sponsor-reports-purchase-details-by-item-reducer.test.js | Tests reducer transitions for request/receive/paging/error/reset. |
| src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js | Adds “byitem” view: fetch orchestration, summary selection, CSV export, and renders ByItemView. |
| src/pages/sponsors/sponsor-reports/purchase-details-report-page/tests/index.test.js | Tests view switching, filter carryover, export behavior, paging dispatch, and payment-method visibility. |
| src/i18n/en.json | Adds strings for “By Item” view labels/columns/captions. |
| src/components/sponsors/reports/ReportViewToggle.js | Adds toggle button for the new “By Item” view. |
| src/components/sponsors/reports/LinesManifestView.js | Exposes shared paging options and Destination for reuse by ByItemView. |
| src/components/sponsors/reports/ByItemView.js | New By Item rollup UI + groupLinesBySponsorItem aggregation helper. |
| src/components/sponsors/reports/tests/ReportViewToggle.test.js | Tests the new toggle option is rendered and emits byitem. |
| src/components/sponsors/reports/tests/ByItemView.test.js | Tests grouping semantics, rendering, drill-down toggling, status mix, and pagination wiring. |
| src/actions/sponsor-reports-actions.js | Adds By Item action types, sequencing token, whole-set fetch thunk, and client paging action. |
| src/actions/tests/sponsor-reports-actions.test.js | Tests whole-set fetch behavior, sequencing/stale guards, and error suppression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/actions/sponsor-reports-actions.js (1)
494-573: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting a shared bulk-load helper to eliminate duplication with
getSponsorAssetRows.
getPurchaseDetailsByItemRowsis structurally near-identical togetSponsorAssetRows(lines 393-492) — sequence guard setup, token await,isCurrentre-checks,pLimit(TEN)bulk loading, row concatenation, atomicRECEIVEcommit, and theError-only catch pattern. The only meaningful differences are the URL, query builder, action type constants, and sequence instance. A parameterized helper (e.g.createBulkLoadReportRows({ url, buildQuery, requestAction, receiveRowsAction, readErrorAction, seq })) would eliminate ~80 lines of duplicated logic and prevent future divergence when the pattern evolves.🤖 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/actions/sponsor-reports-actions.js` around lines 494 - 573, Extract the duplicated paginated bulk-loading workflow from getSponsorAssetRows and getPurchaseDetailsByItemRows into a shared parameterized helper, such as createBulkLoadReportRows. Pass each flow’s URL, query builder, request/receive/error actions, and sequence guard instance, while preserving token retrieval, guarded dispatches, pLimit(TEN) concurrency, stale-sequence checks, atomic row commits, and Error-only handling; then refactor both thunks to use the helper.src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js (1)
391-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the repeated three-way view branching.
The
view === "orders" ? … : view === "lines" ? … : byItem…shape recurs forcarried,activeSummary,activeFilters,activeReadError, and the export handler. A smallbyView({ orders, lines, byitem })helper (or a per-view config map) would remove the duplication and keep the branches in sync as views evolve.🤖 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 391 - 409, Refactor the repeated view-based ternaries into a shared byView helper or per-view configuration map. Update carried, activeSummary, activeFilters, activeReadError, and the export handler to use it, preserving each view’s existing values and behavior while keeping future view additions centralized.
🤖 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/actions/sponsor-reports-actions.js`:
- Around line 494-573: Extract the duplicated paginated bulk-loading workflow
from getSponsorAssetRows and getPurchaseDetailsByItemRows into a shared
parameterized helper, such as createBulkLoadReportRows. Pass each flow’s URL,
query builder, request/receive/error actions, and sequence guard instance, while
preserving token retrieval, guarded dispatches, pLimit(TEN) concurrency,
stale-sequence checks, atomic row commits, and Error-only handling; then
refactor both thunks to use the helper.
In `@src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js`:
- Around line 391-409: Refactor the repeated view-based ternaries into a shared
byView helper or per-view configuration map. Update carried, activeSummary,
activeFilters, activeReadError, and the export handler to use it, preserving
each view’s existing values and behavior while keeping future view additions
centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37c33ba3-7f2e-477b-95f2-89136fe3e083
📒 Files selected for processing (13)
src/actions/__tests__/sponsor-reports-actions.test.jssrc/actions/sponsor-reports-actions.jssrc/components/sponsors/reports/ByItemView.jssrc/components/sponsors/reports/LinesManifestView.jssrc/components/sponsors/reports/ReportViewToggle.jssrc/components/sponsors/reports/__tests__/ByItemView.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-by-item-reducer.test.jssrc/reducers/sponsors/sponsor-reports-purchase-details-by-item-reducer.jssrc/store.js
|
Re the two CodeRabbit nitpicks: Shared bulk-load helper (sponsor-reports-actions.js): Agreed the two thunks share the same skeleton. Extracting a byView helper for the view ternaries (purchase-details-report-page/index.js): Keeping the ternary chains as is. They extend the existing two-way pattern already in this file, and a |
| perPage: byItemPerPage | ||
| }); | ||
| } | ||
| fetchByItemRows(carried); |
There was a problem hiding this comment.
@caseylocker This branch unconditionally calls fetchByItemRows(carried) every time the user switches into By Item, with no check for "already loaded, filters unchanged." Unlike the Orders/Lines branches above it (cheap single-page server fetches), this drives getPurchaseDetailsByItemRows, a whole-set bulk fetch (page 1, then the remaining pages via pLimit(10) at 100/page). Every Orders → By Item → Orders → By Item round-trip while exploring the report re-triggers that full multi-page burst against sponsor-reports-api, even when nothing changed.
Reproduced locally by mounting the page with byItemFilters: {} and byItemData already populated, then toggling orders → byitem → orders → byitem: getPurchaseDetailsByItemRows fires twice, even though the second entry has identical filters and already-loaded data.
// 1st toggle: orders -> byitem
fireEvent.click(screen.getByText("sponsor_reports_page.view_by_item"));
expect(getPurchaseDetailsByItemRows).toHaveBeenCalledTimes(1);
fireEvent.click(screen.getByText("sponsor_reports_page.view_orders"));
fireEvent.click(screen.getByText("sponsor_reports_page.view_by_item"));
// fetches again despite unchanged filters + already-loaded whole-set:
expect(getPurchaseDetailsByItemRows).toHaveBeenCalledTimes(2);sameFilters(carried, byItemFilters) is already computed a few lines up to decide whether to reset paging — the same condition could gate the fetch itself: skip fetchByItemRows(carried) when byItemData.length > 0 && sameFilters(carried, byItemFilters).
There was a problem hiding this comment.
Gated it. Re-entry now skips the whole-set fetch when the carried filters are unchanged and rows are already loaded, reusing the sameFilters(carried, byItemFilters) already computed for the paging reset. I also added a !byItemReadError check so a failed filtered fetch (which leaves stale rows plus the new filters in the slice) still retries on re-entry instead of serving the stale set as a false hit. Filter changes still refetch through handleApply. While in there I also made the thunk drop its whole-set commit if the summit changed under it, since the per-thunk sequence guard is not summit aware. Covered by tests for the skip, the differing-filters refetch, the errored retry, and the summit-switch drop. 038a266.
| if (isEmptyString(item.label) && !isEmptyString(row.description)) { | ||
| item.label = row.description.trim(); | ||
| } | ||
| item.qty += row.quantity ?? 0; |
There was a problem hiding this comment.
@caseylocker Σ qty and the "N of M items purchased" badge (purchasedCount, line 147) count canceled lines the same as live ones — item.qty += row.quantity ?? 0 never checks row.is_canceled. Concretely: a sponsor whose only line for an item is a canceled quantity-2 line still shows that item as purchased in the sponsor chip ("1 of 1 items purchased") and contributes 2 to the sponsor-header Σ qty, even though the drill-down row for that same line renders struck-through as canceled.
Reproduced against groupLinesBySponsorItem directly:
const rows = [{
sponsor: { id: 17, name: "Acme" },
purchase: { id: 5001, number: "OCP-1", status: "Canceled" },
item_code: "AV1", description: "Audio mixer",
quantity: 2, line_total: 100000, is_canceled: true
}];
const [group] = groupLinesBySponsorItem(rows);
group.items[0].qty // 2 (canceled quantity still counted)
group.purchasedCount // 1 ("1 of 1 items purchased")
group.totalQty // 2 (sponsor-header Σ qty also inflated)I know the canceled-lines-flow-through note in the PR description covers not hiding rows in the drill-down — but that's a different claim from the aggregate badge asserting "purchased." Either exclude canceled lines from qty/purchasedCount (still visible, struck-through, in the drill-down only), or rename the badge to a status-neutral phrase (e.g. "items with activity") if counting canceled activity is intentional.
There was a problem hiding this comment.
Went with the exclude option. Canceled lines are now excluded from qty, money, purchasedCount, and the sponsor-header Σ qty; they stay visible struck-through in the contributing-orders drill-down only. I excluded the canceled line_total from the item total as well, so the money and qty columns stay consistent under the same rule. Added a grouping test asserting a canceled-only line reports qty 0 and null money while still appearing as a contributor. 038a266.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-reports/purchase-details-report-page/__tests__/index.test.js (1)
444-471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also asserting the paging reset when carried filters differ.
The production effect resets
setByItemPaging({ currentPage: DEFAULT_CURRENT_PAGE, perPage: byItemPerPage })whenfiltersUnchangedis false, before callingfetchByItemRows. This test only asserts the refetch call; adding an assertion onsetPurchaseDetailsByItemPagingwould close the loop on this test's own premise (filters differ → new result set → page must reset).Suggested addition
expect(getPurchaseDetailsByItemRows).toHaveBeenCalledWith({ dateFrom: "2026-01-01" }); + expect(setPurchaseDetailsByItemPaging).toHaveBeenCalledWith({ + currentPage: 1, + perPage: 10 + }); });🤖 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/__tests__/index.test.js` around lines 444 - 471, Add an assertion to the test case “DOES re-fetch the By Item whole-set on entry when rows are loaded but the carried filters differ” verifying that setPurchaseDetailsByItemPaging is called with currentPage set to DEFAULT_CURRENT_PAGE and perPage set to byItemPerPage when filters differ. Keep the existing refetch assertion unchanged.
🤖 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/__tests__/index.test.js`:
- Around line 444-471: Add an assertion to the test case “DOES re-fetch the By
Item whole-set on entry when rows are loaded but the carried filters differ”
verifying that setPurchaseDetailsByItemPaging is called with currentPage set to
DEFAULT_CURRENT_PAGE and perPage set to byItemPerPage when filters differ. Keep
the existing refetch assertion unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7781902b-980a-4feb-948c-4698e39b6849
📒 Files selected for processing (6)
src/actions/__tests__/sponsor-reports-actions.test.jssrc/actions/sponsor-reports-actions.jssrc/components/sponsors/reports/ByItemView.jssrc/components/sponsors/reports/__tests__/ByItemView.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/sponsors/reports/ByItemView.js
- src/components/sponsors/reports/tests/ByItemView.test.js
- src/actions/tests/sponsor-reports-actions.test.js
- src/actions/sponsor-reports-actions.js
- src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js
| const sponsorAssetRowsSeq = sequenced(); | ||
| const assetFiltersSeq = sequenced(); | ||
| const sponsorDrilldownSeq = sequenced(); | ||
| const purchaseByItemSeq = sequenced(); |
There was a problem hiding this comment.
I don't think this is a complete solution, two different actions could be calling the same action type, like START_LOADING and have same seq, creating a conflict. Also having to declare one Seq per call is not scalable. @smarcet we should stop building code debt with this and think of a better solution before applying it
There was a problem hiding this comment.
Agreed on the scalability point. A seq instance per thunk is boilerplate and won't scale as more bulk loaders land. One possible solution is to pull the whole bulk-load flow (token, pLimit paging, atomic commit, stale guard) into one helper parameterized by url builder, query builder, action types, and a guard instance, then route both this thunk and getSponsorAssetRows through it.
One thing the helper should own explicitly is the summit-validity guard and the terminal loading cleanup, not just the seq. The two thunks differ there on purpose today: by-item guards an in-flight summit switch because its fetch is view-keyed and nothing re-invokes it to bump the seq, while the asset flow is summit-keyed so a switch re-fetches and supersedes the parked call on its own. Folding them together means standardizing that policy across both the commit and the error path rather than copying either shape. It should probably ship as its own small PR, so the refactor of the already-shipped asset loader gets reviewed on its own merits instead of riding inside a view-only change. I'll defer to you and @smarcet.
One clarification on the conflict: each seq is a private closure with its own counter, so isCurrent only ever compares a call against newer calls of the same thunk, never across thunks or action types. Two thunks both sitting at seq 1 don't interact, and a shared START_LOADING can't collide on it, so it's sound as written today. The boilerplate is the real issue and factoring it is the right fix.
| SET_PURCHASE_DETAILS_BY_ITEM_PAGING | ||
| } from "../../../actions/sponsor-reports-actions"; | ||
|
|
||
| describe("sponsor-reports-purchase-details-by-item-reducer", () => { |
There was a problem hiding this comment.
All these tests are just testing normal redux logic, we shouldn't create tests like this
There was a problem hiding this comment.
Trimmed. Dropped the assertions that only verify a spread copied a field through (the READ_ERROR passthrough and the SET_PAGING field checks), and kept the ones that pin real contracts: summary falling back to null when the envelope omits it, readError clearing on REQUEST/RECEIVE, and the reset to DEFAULT_STATE on SET_CURRENT_SUMMIT and LOGOUT_USER. The SET_PAGING behavior still has integration coverage in the page test. 65bfdc2.
| // D9: Total Refunded tile renders ONLY when total_refunded != null — a defensive | ||
| // presence check (the field is optional in the summary payload). | ||
| const activeSummary = view === "orders" ? summary : linesSummary; | ||
| const activeSummary = |
There was a problem hiding this comment.
please no nested ternary. @smarcet can we prevent this on eslint ?
There was a problem hiding this comment.
no-nested-ternary is already in .eslintrc.json at warn, so these surfaced as warnings rather than errors today. Bumping it to error would trip roughly 19 existing files across the repo, so that's a separate cleanup call for you and @smarcet. For this PR I collapsed the per-view branches (carried, activeSummary, activeFilters, activeReadError, the export handler, and the render block) into a small byView lookup, so the nesting is gone from this file. That also closes the related CodeRabbit note on the same chains. 65bfdc2.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/reducers/sponsors/__tests__/sponsor-reports-purchase-details-by-item-reducer.test.js (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain coverage for paging and read-error transitions.
This change removes coverage for
SET_PURCHASE_DETAILS_BY_ITEM_PAGINGandPURCHASE_DETAILS_BY_ITEM_READ_ERROR. Unless equivalent tests exist elsewhere, keep focused cases for both reducer paths to catch pagination and error-state 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/reducers/sponsors/__tests__/sponsor-reports-purchase-details-by-item-reducer.test.js` at line 8, Restore focused reducer tests for SET_PURCHASE_DETAILS_BY_ITEM_PAGING and PURCHASE_DETAILS_BY_ITEM_READ_ERROR in the sponsor purchase-details-by-item test suite. Cover the expected paging transition and read-error transition, preserving existing RECEIVE_PURCHASE_DETAILS_BY_ITEM_ROWS coverage and avoiding duplicate tests if equivalent cases already exist.
🤖 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/reducers/sponsors/__tests__/sponsor-reports-purchase-details-by-item-reducer.test.js`:
- Line 8: Restore focused reducer tests for SET_PURCHASE_DETAILS_BY_ITEM_PAGING
and PURCHASE_DETAILS_BY_ITEM_READ_ERROR in the sponsor purchase-details-by-item
test suite. Cover the expected paging transition and read-error transition,
preserving existing RECEIVE_PURCHASE_DETAILS_BY_ITEM_ROWS coverage and avoiding
duplicate tests if equivalent cases already exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f428f3e-ea93-4efd-8d5f-484c38f938e9
📒 Files selected for processing (2)
src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.jssrc/reducers/sponsors/__tests__/sponsor-reports-purchase-details-by-item-reducer.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js
Third view on Purchase Details: whole-set fetch of the lines endpoint into a new redux slice, client-side sponsor -> item rollup (sum qty + distinct orders + status mix) with a contributing-orders drill-down, redux-backed client paging, and CSV reuse of the lines export. No backend changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Chevron IconButton per item row (aria-expanded + aria-label, keyboard-operable) following the editable-table buttons-in-cells idiom; row click retained as a convenience. Addresses the Copilot accessibility finding on the clickable row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address review on the By Item view of the Purchase Details report: - Exclude canceled lines from the "purchased" aggregates (qty, money, purchasedCount, sponsor-header sum qty). Canceled lines stay visible, struck-through, in the contributing-orders drill-down only. - Skip the multi-page whole-set refetch when re-entering By Item with unchanged filters and rows already loaded; still retry when the prior fetch errored so stale rows are not served as a false cache hit. - Guard the whole-set commit against a summit switch: the per-thunk sequence is not summit-aware and a switch remounts on Orders without re-invoking By Item, so a late old-summit response could repopulate the reset slice. Balance loading on that bail so the overlay is not left on. - Add tests for canceled exclusion, the cache-gate conjuncts, and the summit-switch late-receive drop. Co-Authored-By: Claude <noreply@anthropic.com>
Collapse the nested per-view ternaries in the Purchase Details page into a byView lookup helper (carried, activeSummary, activeFilters, activeReadError, the export handler, and the render block), clearing the no-nested-ternary warnings. The export branches stay lazy so only the active view's exporter runs. Trim the by-item reducer tests to the cases that pin real contracts (summary null fallback, readError clearing, reset on summit/logout), dropping the spread-passthrough assertions (SET_PAGING field checks, READ_ERROR passthrough). Co-Authored-By: Claude <noreply@anthropic.com>
65bfdc2 to
76ceada
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/sponsors/reports/LinesManifestView.js (1)
43-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
PER_PAGE_OPTIONSdefined in one module.The supplied graph context shows
src/components/sponsors/reports/ByItemView.js:45-50also declaringPER_PAGE_OPTIONS, which conflicts with this “single source” comment. Verify thatByItemViewimports this constant rather than maintaining a duplicate; otherwise future page-size changes can diverge.Based on the supplied graph context,
ByItemView.jsis also shown declaringPER_PAGE_OPTIONS.🤖 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/LinesManifestView.js` around lines 43 - 45, Keep PER_PAGE_OPTIONS defined only in LinesManifestView, and update ByItemView to import and reuse that exported constant instead of declaring its own list. Remove the duplicate declaration while preserving all existing page-size 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/components/sponsors/reports/LinesManifestView.js`:
- Around line 43-45: Keep PER_PAGE_OPTIONS defined only in LinesManifestView,
and update ByItemView to import and reuse that exported constant instead of
declaring its own list. Remove the duplicate declaration while preserving all
existing page-size behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48666c25-b8b5-4be5-b6e8-1b44e0fa1f69
📒 Files selected for processing (13)
src/actions/__tests__/sponsor-reports-actions.test.jssrc/actions/sponsor-reports-actions.jssrc/components/sponsors/reports/ByItemView.jssrc/components/sponsors/reports/LinesManifestView.jssrc/components/sponsors/reports/ReportViewToggle.jssrc/components/sponsors/reports/__tests__/ByItemView.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-by-item-reducer.test.jssrc/reducers/sponsors/sponsor-reports-purchase-details-by-item-reducer.jssrc/store.js
🚧 Files skipped from review as they are similar to previous changes (11)
- src/components/sponsors/reports/ReportViewToggle.js
- src/store.js
- src/components/sponsors/reports/tests/ByItemView.test.js
- src/i18n/en.json
- src/reducers/sponsors/sponsor-reports-purchase-details-by-item-reducer.js
- src/components/sponsors/reports/tests/ReportViewToggle.test.js
- src/pages/sponsors/sponsor-reports/purchase-details-report-page/tests/index.test.js
- src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js
- src/reducers/sponsors/tests/sponsor-reports-purchase-details-by-item-reducer.test.js
- src/actions/tests/sponsor-reports-actions.test.js
- src/actions/sponsor-reports-actions.js
| } | ||
| } | ||
| const purchaseId = row.purchase?.id ?? null; | ||
| if (purchaseId != null) { |
There was a problem hiding this comment.
@caseylocker orders and statusMix don't apply the same is_canceled gate as qty/totalCents, so a canceled-only line for an item still counts as a distinct paid order for that item.
Three lines above (96-102), qty and totalCents are correctly wrapped in if (!row.is_canceled) — that's the fix for the earlier "canceled-only line reads as purchased" bug. But item.orderIds.add(purchaseId) and item.statusOrderIds here only check purchaseId != null, not is_canceled, so they still accumulate for a canceled line. When an order has one canceled line and one live line for different items (a case this PR's own description calls out as normal — "line-level soft-canceled rows inside live orders always flow through"), the canceled item's row ends up showing Qty: 0 next to Orders: 1 and a Paid: 1 status chip — which reads as a contradiction next to the just-fixed qty/purchasedCount semantics.
Failing test (drop into the groupLinesBySponsorItem describe block, next to the "EXCLUDES canceled lines..." test):
it("excludes a canceled-only line's order from orders/statusMix, same as qty/money", () => {
const rows = [
line({
item_code: "Z1",
description: "Canceled only",
quantity: 4,
line_total: 40000,
is_canceled: true,
purchase: { id: 5001, number: "OCP-1", status: "Paid" }
})
];
const [group] = groupLinesBySponsorItem(rows);
const z1 = group.items.find((i) => i.itemCode === "Z1");
expect(z1.qty).toBe(0);
expect(z1.totalCents).toBeNull();
expect(z1.orders).toBe(0); // currently 1
expect(z1.statusMix).toEqual({}); // currently { Paid: 1 }
});Is this intentional (e.g. orders/status meant to reflect all order activity regardless of cancellation) or an oversight from the qty/money fix not being extended to these two fields? If intentional, worth a comment here plus a test pinning it — right now nothing distinguishes "designed" from "missed."
There was a problem hiding this comment.
Good catch, not intended. That was the qty/money gate not getting extended when I added it. Moved the orderIds and statusMix accumulation inside the same !is_canceled block so all four aggregates (qty, money, orders, status) share one gate. Contributors stay unconditional so canceled lines still show struck-through in the drill-down. Mixed orders keep their count since the live line for the same item re-adds the order id. Added your test with the corrected assertions (orders 0, statusMix {}) and tested locally with the summit 73 cancelled stripe order - STRPTST "Test for Stripe" (FNTECH, order 000010, Paid). Pushed in 1621559.
…/statusMix
The is_canceled gate that excluded canceled lines from qty and money did not
cover the orders count or statusMix, so a canceled-only line still counted its
order as a distinct paid order for the item. That produced a contradictory row
(Qty 0 next to Orders 1 / Paid 1) and re-introduced the "reads as purchased"
case the qty/money gate was added to prevent.
Move the orderIds/statusOrderIds accumulation inside the same !is_canceled
block so all four purchased aggregates (qty, money, orders, statusMix) share one
gate. A mixed order keeps its count because the live line for the same item
still adds the order id. contributors.push stays unconditional, so canceled
lines still render struck-through in the drill-down.
Add a test pinning the canceled-only case (orders 0, statusMix {}).
Co-Authored-By: Claude <noreply@anthropic.com>
…1009) * feat(sponsor-reports): add By Item view to Purchase Details report Third view on Purchase Details: whole-set fetch of the lines endpoint into a new redux slice, client-side sponsor -> item rollup (sum qty + distinct orders + status mix) with a contributing-orders drill-down, redux-backed client paging, and CSV reuse of the lines export. No backend changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat: add accessible expand toggle to by-item drill-down rows Chevron IconButton per item row (aria-expanded + aria-label, keyboard-operable) following the editable-table buttons-in-cells idiom; row click retained as a convenience. Addresses the Copilot accessibility finding on the clickable row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sponsor-reports): By Item canceled aggregates and refetch guards Address review on the By Item view of the Purchase Details report: - Exclude canceled lines from the "purchased" aggregates (qty, money, purchasedCount, sponsor-header sum qty). Canceled lines stay visible, struck-through, in the contributing-orders drill-down only. - Skip the multi-page whole-set refetch when re-entering By Item with unchanged filters and rows already loaded; still retry when the prior fetch errored so stale rows are not served as a false cache hit. - Guard the whole-set commit against a summit switch: the per-thunk sequence is not summit-aware and a switch remounts on Orders without re-invoking By Item, so a late old-summit response could repopulate the reset slice. Balance loading on that bail so the overlay is not left on. - Add tests for canceled exclusion, the cache-gate conjuncts, and the summit-switch late-receive drop. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(sponsor-reports): address by-item review feedback Collapse the nested per-view ternaries in the Purchase Details page into a byView lookup helper (carried, activeSummary, activeFilters, activeReadError, the export handler, and the render block), clearing the no-nested-ternary warnings. The export branches stay lazy so only the active view's exporter runs. Trim the by-item reducer tests to the cases that pin real contracts (summary null fallback, readError clearing, reset on summit/logout), dropping the spread-passthrough assertions (SET_PAGING field checks, READ_ERROR passthrough). Co-Authored-By: Claude <noreply@anthropic.com> * fix(sponsor-reports): exclude canceled-only lines from By Item orders/statusMix The is_canceled gate that excluded canceled lines from qty and money did not cover the orders count or statusMix, so a canceled-only line still counted its order as a distinct paid order for the item. That produced a contradictory row (Qty 0 next to Orders 1 / Paid 1) and re-introduced the "reads as purchased" case the qty/money gate was added to prevent. Move the orderIds/statusOrderIds accumulation inside the same !is_canceled block so all four purchased aggregates (qty, money, orders, statusMix) share one gate. A mixed order keeps its count because the live line for the same item still adds the order id. contributors.push stays unconditional, so canceled lines still render struck-through in the drill-down. Add a test pinning the canceled-only case (orders 0, statusMix {}). Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ref: https://app.clickup.com/t/9014802374/86baugd67
Adds a third By Item view to the Purchase Details report (Sponsors → Reports → Purchase Details): one row per catalog item per sponsor, rolling up the line items the sponsor purchased — "how many plants, how many desks" — with Σ qty and distinct order count as separate measures.
What it does
getPurchaseDetailsByItemRowsthunk loop-fetches the existing/purchase-details/linesendpoint (page 1 →last_page, thenp-limit(10)bulk at 100/page), committing rows atomically into a newsponsorReportsPurchaseDetailsByItemStateslice. Same sequence-guard pattern asgetSponsorAssetRows.groupLinesBySponsorItem(named export inByItemView.js) buckets bysponsor.id→ trimmeditem_code, aggregating qty, distinct orders, per-status distinct-order mix, and null-safe money totals. No data filtering — zero-qty and canceled lines flow through.SET_PURCHASE_DETAILS_BY_ITEM_PAGING) so the user's place survives view toggles.Notes for reviewers
item_codeonly (no form identity) — intentional. Item codes are unique per form in purchases-api, but cross-form same-code rows are cloned copies of the same catalog item (ShowFormItem.clone/ managed-form copy); on summit 73, 46 of 75 codes legitimately span 2–3 forms with identical descriptions. Keying by (form, code) would fragment them. The drill-down shows each contributor's form code, so any merge is inspectable.include_cancelled=trueonly when the status filter is Canceled; line-level soft-canceled rows inside live orders always flow through and render struck-through. This view deliberately matches the family default rather than diverging.Testing
13 files (4 new + 9 modified; 6 of the 13 are test files).
Summary by CodeRabbit