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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 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 unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
package.jsonParsing error: Missing semicolon. (2:8) 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
![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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/actions/tests/sponsor-purchases-actions.test.js:113
- After importing T, assert the snackbar payload against T.translate("errors.invoice_generation") so the expectation matches the action's behavior regardless of whether translations are loaded in the test environment.
expect(snackbarErrorMsg).toHaveBeenCalledWith(
expect.objectContaining({ html: "errors.invoice_generation" })
);
src/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/index.js:188
- Replacing the IconButton with a bare CircularProgress removes the focused control after activation, which can cause keyboard focus loss and makes the loading state harder to discover for assistive tech. Keep the IconButton in the DOM and swap its child to a spinner (or set aria-busy) while disabling it.
render: (row) =>
downloadingOrderId === row.id ? (
<CircularProgress size={24} />
) : (
<IconButton
src/pages/sponsors/show-purchase-list-page/index.js:205
- Replacing the IconButton with only a CircularProgress removes the interactive element from the DOM while it is focused/active, which can lead to keyboard focus loss and a less accessible loading state. Prefer keeping the IconButton rendered and swapping its child to a spinner while disabled.
render: (row) =>
downloadingOrderId === row.id ? (
<CircularProgress size={24} />
) : (
<IconButton
src/actions/tests/sponsor-purchases-actions.test.js:23
- This test currently asserts the untranslated i18n key ("errors.invoice_generation"). If i18n is initialized during tests (or behavior changes), the action will pass the translated string instead. Import T and assert against T.translate(...) to keep the test aligned with production behavior.
This issue also appears on line 111 of the same file.
import configureStore from "redux-mock-store";
import thunk from "redux-thunk";
import flushPromises from "flush-promises";
import {
getRequest,
snackbarErrorMsg
} from "openstack-uicore-foundation/lib/utils/actions";
import { generateInvoicePDF } from "openstack-uicore-foundation/lib/components/order-invoice-pdf";
import { downloadSponsorInvoice } from "../sponsor-purchases-actions";
import * as methods from "../../utils/methods";
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
* feat: add invoice pdf download on purchase lists Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: adjust aria labels, actions params Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: adjust test mocks and fileMock directory Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: add normalize function on API order response before generate invoice PDF Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: update uicore version Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: update uicore version Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: unify download action in redux, add catches and tests, guard download icons with loading Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: split download pdf fucntion, rollback params, add tests, disable row on download Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: unify expand for order details, adjust params on sponsor purchases tab, update tests Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: update react-pdf/renderer version Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> --------- Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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