fix: add function to unify order ledger on details and invoice - #307
Open
tomrndom wants to merge 3 commits into
Open
fix: add function to unify order ledger on details and invoice#307tomrndom wants to merge 3 commits into
tomrndom wants to merge 3 commits into
Conversation
…t pdf params, add tests 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 Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds ChangesOrder ledger rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Order
participant buildOrderLedger
participant OrderPdf
participant SponsorOrderGrid
Order->>buildOrderLedger: provide raw order data
buildOrderLedger-->>OrderPdf: return typed entries and balances
buildOrderLedger-->>SponsorOrderGrid: return typed entries and balances
OrderPdf->>OrderPdf: map entries to invoice rows
SponsorOrderGrid->>SponsorOrderGrid: map entries to grid rows
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 (2)
src/components/mui/SponsorOrderGrid/__tests__/SponsorOrderGrid.test.js (1)
179-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rendered quantity, not only the amount.
The test name states that quantity defaults to 1, but the assertion checks the
$100.00text. That text also comes from the balance cell, so it does not confirm the quantity value. The details column renders the quantity, so assert it directly.♻️ Proposed refinement
render(<SponsorOrderGrid order={order} />); - expect(screen.getAllByText("$100.00").length).toBeGreaterThan(0); + expect( + screen.getByText(/Booth - sponsor_order_grid\.total: 1/) + ).toBeInTheDocument();🤖 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/mui/SponsorOrderGrid/__tests__/SponsorOrderGrid.test.js` around lines 179 - 186, Update the test for missing quantity in SponsorOrderGrid to assert the details column renders quantity 1 directly, rather than relying on the ambiguous "$100.00" amount text. Keep the existing order setup and render flow unchanged.src/components/order-invoice-pdf/__tests__/ledger-consistency.test.js (1)
77-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScope the row query to exclude the appended total row.
SponsorOrderGridappends aTotalRowinside the same<TableBody>whenwithReconciliationis false, which is the default here. The index-based loop is safe today only because that row comes after the ledger rows. Add a length assertion so a future reordering fails loudly instead of comparing the wrong cells.💚 Proposed hardening
const { container } = render(<SponsorOrderGrid order={purchaseV2Fixture} />); const rows = container.querySelectorAll("tbody tr"); + expect(rows.length).toBeGreaterThanOrEqual(ledger.length);🤖 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/order-invoice-pdf/__tests__/ledger-consistency.test.js` around lines 77 - 79, Update the SponsorOrderGrid test’s row selection and assertions to explicitly account for the appended TotalRow, asserting the expected ledger-row count before the index-based comparison. Keep the running-balance/order comparison scoped to ledger rows and make any future row reordering fail clearly.
🤖 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/components/mui/SponsorOrderGrid/index.js`:
- Line 314: Update the empty-state condition near hasNoForms in SponsorOrderGrid
to derive from the ledger’s rendered rows rather than order.forms.length. Show
the “no_items” row only when the ledger contains no visible form, fee, payment,
refund, or note rows, including when all forms are removed by the ledger
quantity filter.
- Around line 249-258: Update the "discount" case in SponsorOrderGrid to derive
the description with formatDiscount(entry.form.discount_amount,
entry.form.discount_type) instead of entry.form.discount, and pass the formatted
value to DiscountRow's discount prop. Preserve the existing amount, balance,
trailing, and key props.
---
Nitpick comments:
In `@src/components/mui/SponsorOrderGrid/__tests__/SponsorOrderGrid.test.js`:
- Around line 179-186: Update the test for missing quantity in SponsorOrderGrid
to assert the details column renders quantity 1 directly, rather than relying on
the ambiguous "$100.00" amount text. Keep the existing order setup and render
flow unchanged.
In `@src/components/order-invoice-pdf/__tests__/ledger-consistency.test.js`:
- Around line 77-79: Update the SponsorOrderGrid test’s row selection and
assertions to explicitly account for the appended TotalRow, asserting the
expected ledger-row count before the index-based comparison. Keep the
running-balance/order comparison scoped to ledger rows and make any future row
reordering fail clearly.
🪄 Autofix
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 Plus
Run ID: 6456ee60-09a2-456a-9cbc-c9bddfe6e4b2
📒 Files selected for processing (8)
src/components/mui/SponsorOrderGrid/__tests__/SponsorOrderGrid.test.jssrc/components/mui/SponsorOrderGrid/index.jssrc/components/order-invoice-pdf/__tests__/ledger-consistency.test.jssrc/components/order-invoice-pdf/__tests__/order-invoice-pdf.test.jssrc/components/order-invoice-pdf/helpers.jssrc/components/order-invoice-pdf/index.jssrc/utils/__tests__/order-ledger.test.jssrc/utils/order-ledger.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR centralizes the derivation of an order “ledger” (row ordering, row keys, and running balances) into a shared utility so the SponsorOrderGrid (MUI) and invoice PDF stay consistent.
Changes:
- Added
buildOrderLedgeras a single source of truth for ledger entry ordering, row keys, and running balance computation. - Refactored invoice PDF row building (
buildRows) and SponsorOrderGrid rendering to consume the shared ledger. - Added unit + integration/regression tests to prevent the PDF/grid from drifting again.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/order-ledger.js | Introduces shared ledger derivation (entries + running balance). |
| src/utils/tests/order-ledger.test.js | Unit tests for ledger behavior (keys, filtering, ordering, balances). |
| src/components/order-invoice-pdf/index.js | Adjusts PDF to call buildRows(order) with the updated signature. |
| src/components/order-invoice-pdf/helpers.js | Refactors PDF buildRows into a thin mapper over buildOrderLedger. |
| src/components/order-invoice-pdf/tests/order-invoice-pdf.test.js | Updates/relocates coverage to focus on presentational mapping rather than ledger math. |
| src/components/order-invoice-pdf/tests/ledger-consistency.test.js | Adds cross-consumer regression test to ensure PDF/grid match ledger ordering/balances. |
| src/components/mui/SponsorOrderGrid/index.js | Refactors grid rendering to iterate over buildOrderLedger(order) and use ledger balances/keys. |
| src/components/mui/SponsorOrderGrid/tests/SponsorOrderGrid.test.js | Adds regression coverage for previously divergent behaviors now driven by the ledger. |
Suppressed comments (1)
src/components/mui/SponsorOrderGrid/index.js:284
- For refunds where
reason/statusare missing (a case already handled in the invoice PDF), the grid will render an empty reason/status. Normalize these fields before passing the refund toRefundRow(fallback reason tomui_table.refund, status to empty string) to avoid blank rows and keep PDF/grid behavior aligned.
refund={entry.refund}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -79,26 +74,24 @@ const SponsorOrderGrid = ({ | |||
|
|
|||
| const { | |||
| forms = [], | |||
Comment on lines
+270
to
+278
| case "payment": | ||
| return ( | ||
| <PaymentRow | ||
| key={entry.rowKey} | ||
| payment={entry.payment} | ||
| balance={entry.balanceCents} | ||
| trailing={trailingCols} | ||
| /> | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
*adjust pdf params, add tests
ref: https://app.clickup.com/t/9014802374/86bb67u52
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes