feat: add invoice pdf download on purchase lists - #1022
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughSponsor purchase views now support invoice PDF downloads. They retrieve sponsor orders with sponsor identifiers, prevent duplicate requests, display translated errors, and expose accessible download controls. Order rendering and normalization also handle mismatched or incomplete data. ChangesSponsor invoice downloads
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PurchaseView
participant downloadSponsorInvoice
participant SponsorOrdersAPI
participant generateInvoicePDF
PurchaseView->>downloadSponsorInvoice: Dispatch orderId and sponsorId
downloadSponsorInvoice->>SponsorOrdersAPI: Fetch sponsor order
SponsorOrdersAPI-->>downloadSponsorInvoice: Return order data
downloadSponsorInvoice->>generateInvoicePDF: Generate invoice PDF with logo
generateInvoicePDF-->>PurchaseView: Settle download
PurchaseView-->>PurchaseView: Restore download controls
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
…oice PDF Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
401f228 to
db013e2
Compare
| rejectSponsorPurchase: jest.fn(() => () => Promise.resolve()) | ||
| })); | ||
|
|
||
| jest.mock( |
There was a problem hiding this comment.
@tomrndom This mock keeps the existing suite green after the new imports, but no test exercises the feature the PR adds — the PR description lists "Enhanced test coverage for invoice downloads" under Chores/Tests, and there is no such test here. None of the 16 it(...) blocks in this file touch the download path, show-purchase-list-page has no test file at all, and getSponsorOrder's new sponsorId argument (plus its currentSponsorState fallback) has no action test — there is no sponsor-purchases-actions.test.js in src/actions/__tests__/. The concurrency guard and the error path are the parts most likely to regress and are entirely unverified.
Suggested fix: add a test that mocks openstack-uicore-foundation/lib/components/order-invoice-pdf and asserts (a) clicking the download icon calls getSponsorOrder(orderId, sponsorId) and then generateInvoicePDF with the normalized order and currentSummit, (b) a second click while the first is pending does not call it again, and (c) a rejected fetch leaves the row interactive. An action test covering the sponsorId argument and the fallback to currentSponsorState.entity.id would close the other half.
There was a problem hiding this comment.
@tomrndom Closing note: this one landed in full, and two of the sub-asks in my original "Suggested fix" have since gone stale — recording that so nobody chases them.
Against the current tip (5641d86):
sponsor-purchases-list.test.jsnow has anInvoice downloadblock (lines 492–554) covering the dispatch and the concurrency guard — the file goes from 15 to 17it(...)blocks.show-purchase-list-pagehas a test file now, with three download tests, including one asserting that only the downloading row swaps to a spinner while the others go disabled.src/actions/__tests__/sponsor-purchases-actions.test.jsexists, with four tests covering the request URL, thegenerateInvoicePDFcall, the fetch-rejection path and the PDF-rejection path.
The two stale sub-asks:
- "asserts …
getSponsorOrder(orderId, sponsorId)… with the normalized order" — both premises are gone. There is a dedicateddownloadSponsorInvoicethunk now rather than a widenedgetSponsorOrder, and passing the raw order is correct:normalizeOrderis inert on the PDF path inopenstack-uicore-foundation@5.0.44(evidence on the duplicated-handler thread, feat: add invoice pdf download on purchase lists #1022 (comment)). - "(c) a rejected fetch leaves the row interactive" — not reachable, so not worth a test. The thunk's internal
.catch(() => {})makes its promise always resolve, so the component's.finallynever sees a rejection. The case is covered where it actually lives, in the action test"swallows the order-fetch rejection silently…".
Net: nothing left to do here. Worth noting the direction of travel is now the opposite one — the open thread on sponsor-purchases-actions.test.js:69 asks to prune this set down to the tests carrying real behaviour, which is the right lens to apply here rather than growing it further.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…nload icons with loading Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
… row on download Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
@tomrndom changes look good but too many redundant tests, please review tests one by one and only leave the ones meaningful
| delete window.PURCHASES_API_URL; | ||
| }); | ||
|
|
||
| it("builds the request URL using currentSponsorState.entity.id", async () => { |
There was a problem hiding this comment.
@tomrndom review all tests and only test pieces that have at least some logic or behavior
| const params = { | ||
| access_token: accessToken, | ||
| expand: | ||
| "forms,forms.items,forms.items.meta_fields,forms.items.type,refunds,payments,notes,fees" |
There was a problem hiding this comment.
@tomrndom The order expand list is now duplicated byte-for-byte between getSponsorOrder (line 314) and this thunk (line 342), and the two copies feed different renderers of the same order — the on-screen SponsorOrderGrid and the invoice PDF.
Concrete failure: whoever next changes the detail page's expand (a new relation, or one renamed on the purchases-api side) gets no signal that a second copy exists. In openstack-uicore-foundation@5.0.44, buildRows sources the entire line-item table, fees, payments, refunds and notes from expand-gated relations, while the "Amount Due" comes from getOrderTotal(order) → order.amount_due, which is a plain allowed_fields value on PurchaseV2Serializer and is not expand-gated. So the drift yields an invoice with an empty itemization and a still-correct total — a financial document that reads as authoritative after silently losing its line items, with nothing failing loudly.
This is the case .claude/rules/summit-admin-reuse-before-build.md § "Duplicate Mapping Tables" describes: "Two copies of the same mapping drift silently when one gets updated and the other doesn't." Worth flagging that this copy is new — it was created by consolidating the two page-level handlers into this thunk (#1022 (comment)). That consolidation was the right call; it just moved the duplication from the handlers down into the expand string.
Suggested fix — hoist it to a single module-level constant:
const ORDER_DETAIL_EXPAND =
"forms,forms.items,forms.items.meta_fields,forms.items.type,refunds,payments,notes,fees";and use expand: ORDER_DETAIL_EXPAND in both getSponsorOrder and downloadSponsorInvoice.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-page/tabs/sponsor-purchases-tab/index.js`:
- Around line 88-93: Update handleInvoiceDownload to pass the current sponsor.id
as the second argument to downloadSponsorInvoice instead of item.sponsor_id.
Update the related test fixture and assertion to use the current sponsor ID.
🪄 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: 8ba14953-e8e5-464a-b970-e142118713d6
📒 Files selected for processing (9)
src/actions/__tests__/sponsor-purchases-actions.test.jssrc/actions/sponsor-purchases-actions.jssrc/pages/sponsors/show-purchase-list-page/__tests__/index.test.jssrc/pages/sponsors/show-purchase-list-page/index.jssrc/pages/sponsors/sponsor-page/__tests__/utils.test.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/__tests__/sponsor-purchases-list.test.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/sponsor-order-details.jssrc/pages/sponsors/sponsor-page/utils.js
…es tab, update tests Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/actions/__tests__/sponsor-purchases-actions.test.js (2)
65-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert loading cleanup for every outcome.
downloadSponsorInvoicedispatchesstartLoading()before the request andstopLoading()infinally. These tests only assert PDF and snackbar effects. A regression that omitsstopLoading()would leave the purchase views stuck in a loading state while all three tests pass. Assert the loading action sequence in the success, fetch-rejection, and PDF-rejection cases. (raw.githubusercontent.com)Also applies to: 87-99, 101-115
🤖 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/__tests__/sponsor-purchases-actions.test.js` around lines 65 - 85, Update the three downloadSponsorInvoice test cases to assert the loading action sequence for success, fetch rejection, and PDF rejection: startLoading must be dispatched before stopLoading, with both occurring exactly once. Reuse the existing store/action-state assertions so the tests verify cleanup without changing the PDF or snackbar expectations.
65-85: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the
getRequestmock exercise the full contract.The success test name promises that the invoice fetch does not touch shared order-detail state, but the mock never invokes the supplied receive-action creator. It also ignores the request parameters. The test can pass if the thunk dispatches
RECEIVE_SPONSOR_ORDERorCLEAR_SPONSOR_ORDER, or stops sendingaccess_tokenandORDER_DETAIL_EXPAND. Capture the parameters, dispatch the supplied action creator, and assert the forbidden action types and required fields. (github.com)Suggested test adjustment
+import { + CLEAR_SPONSOR_ORDER, + RECEIVE_SPONSOR_ORDER +} from "../sponsor-purchases-actions"; +let capturedParams; -getRequest.mockImplementation((reqAC, recAC, url) => { +getRequest.mockImplementation((reqAC, receiveAction, url) => { capturedUrl = url; - return () => () => Promise.resolve({ response: fetchedOrder }); + return (params) => (dispatch) => { + capturedParams = params; + dispatch(receiveAction({})); + return Promise.resolve({ response: fetchedOrder }); + }; }); +expect(capturedParams).toEqual( + expect.objectContaining({ + access_token: "TOKEN", + expand: expect.stringContaining("forms") + }) +); +const dispatchedTypes = store.getActions().map(({ type }) => type); +expect(dispatchedTypes).not.toContain(RECEIVE_SPONSOR_ORDER); +expect(dispatchedTypes).not.toContain(CLEAR_SPONSOR_ORDER);🤖 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/__tests__/sponsor-purchases-actions.test.js` around lines 65 - 85, Strengthen the success test around downloadSponsorInvoice by updating the getRequest mock to capture request parameters, invoke the supplied receive-action creator, and dispatch the resulting action. Assert the request includes the required access_token and ORDER_DETAIL_EXPAND fields, and verify no RECEIVE_SPONSOR_ORDER or CLEAR_SPONSOR_ORDER actions are dispatched while preserving the existing invoice PDF assertions.
🤖 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/__tests__/sponsor-purchases-actions.test.js`:
- Around line 65-85: Update the three downloadSponsorInvoice test cases to
assert the loading action sequence for success, fetch rejection, and PDF
rejection: startLoading must be dispatched before stopLoading, with both
occurring exactly once. Reuse the existing store/action-state assertions so the
tests verify cleanup without changing the PDF or snackbar expectations.
- Around line 65-85: Strengthen the success test around downloadSponsorInvoice
by updating the getRequest mock to capture request parameters, invoke the
supplied receive-action creator, and dispatch the resulting action. Assert the
request includes the required access_token and ORDER_DETAIL_EXPAND fields, and
verify no RECEIVE_SPONSOR_ORDER or CLEAR_SPONSOR_ORDER actions are dispatched
while preserving the existing invoice PDF assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a22da29f-c627-4829-86d5-1b356f63490a
📒 Files selected for processing (5)
src/actions/__tests__/sponsor-purchases-actions.test.jssrc/actions/sponsor-purchases-actions.jssrc/pages/sponsors/show-purchase-list-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/__tests__/sponsor-purchases-list.test.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/index.js
💤 Files with no reviewable changes (1)
- src/pages/sponsors/show-purchase-list-page/tests/index.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/tests/sponsor-purchases-list.test.js
- src/actions/sponsor-purchases-actions.js
- src/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/index.js
ref: https://app.clickup.com/t/9014802374/86bb31qvd
depends on OpenStackweb/openstack-uicore-foundation#282 (comment)
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit