Quote-path hardening: ultrareview + user review findings - #49
Merged
Conversation
…ked deletes Provider quote calls adopt the PO pattern (summarize_errors=False + element-level validation_errors checks) so a rejected status change — e.g. deleting an ACCEPTED quote — can never read as success. The post-create tail moves into _finalize_created_quote: EVERY failure after the remote write now compensates (totals validation, persist, the timestamp bump all void the orphan best-effort with the external id in the error), and the IntegrityError is discriminated by state — a same-xero_id row means the sync mirrored our own quote first and is ADOPTED (linking the job the transform never sets), never voided; only the job-constraint race voids. Deletion no longer requires a Xero-valid company (the quote row carries its own), an unsynced company refuses with a readable 400 instead of a 500, and a retained diagnostic PDF reports its path. Findings: ultrareview (2) + user review (1, 2, 5) over PR #48. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 7x-copied scaffolding (token check, failure-to-payload mapping with the status clamp, success-invariant raise + response construction) collapses into _xero_auth_refusal/_document_failure/_document_success. Wire-identical by construction — ninja serializes every declared field, so explicit Nones equal the fields each endpoint used to omit — and the exported schema is unchanged. The error_type drift (missing only from delete_purchase_order) disappears with the copies. Full capability split of apps/xero stays a recorded backlog item, not a pre-cutover change. Findings: ultrareview sub-cap cleanup + user review (4, partial). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Draft rows derive unit_rev from unit_cost like server rows (a filled phantom used to silently never POST); draft commits skip the send-dedupe so retyping the same value after a failed POST retries; the autosave buffer tracks dirtiness instead of copying the server value in at focus (a focus landing in the same tick as a sibling cell's state-updating blur copied a stale render's value); quantity edits make the phantom real; typed free-form rows infer adjust (v1 rule — material means a stock pick); the PATCH echo merges only its own fields so it cannot clobber an interleaved optimistic edit; a failed delete re-inserts only its line. The quote spec now hard-asserts the fresh job's line state before the repair pass and all-clear after it — a line-creation regression fails instead of being silently healed. Findings: ultrareview (3, 5, 6, 8, 9, 10) + user review (3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The handler-contract gate rightly flagged the IntegrityError handler: its persist lived inside the delegate where the AST cannot see it. The persist moves to the catch, which is where it belonged anyway. 1716 tests, 88.52%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR updates Xero quote validation and persistence, retains diagnostic PDFs for failed inspection, and improves costing draft autosave and optimistic state reconciliation. Tests and project metrics were updated for these behaviors. ChangesXero quote flow
Quote PDF diagnostics
Costing draft and optimistic updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant QuoteEndpoint
participant QuoteProvider
participant Xero
participant QuoteFinalizer
participant LocalDatabase
QuoteEndpoint->>QuoteProvider: create quote
QuoteProvider->>Xero: submit quote with summarize_errors=false
Xero-->>QuoteProvider: quote result and validation_errors
QuoteProvider-->>QuoteFinalizer: DocumentResult
QuoteFinalizer->>LocalDatabase: finalize or compensate local persistence
QuoteFinalizer-->>QuoteEndpoint: success or failure response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
Actionable comments posted: 4
🧹 Nitpick comments (6)
apps/xero/tests/test_quote_manager.py (3)
208-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the adopted totals.
Adoption overwrites
total_excl_taxandtotal_incl_taxfrom the provider raw payload (quote.pyLines 287-288), replacing the mirror's zeros. That overwrite is the part a regression would silently drop, becausenumberandjob_idwould still look correct.💚 Proposed assertion
assert adopted.job_id == job.id assert adopted.number == "QU-RAW-1" + assert adopted.total_excl_tax == Decimal("250.00")+ assert adopted.total_incl_tax == Decimal("287.50")🤖 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 `@apps/xero/tests/test_quote_manager.py` around lines 208 - 243, Extend test_same_xero_id_collision_adopts_the_mirrored_row to assert the adopted Quote’s total_excl_tax and total_incl_tax match the provider result’s raw totals, confirming adoption overwrites the mirrored zero values.
272-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises an assertion the body does not make.
test_post_persist_failure_voids_and_names_the_idasserts the void and the empty table. It never checks that the external id appears anywhere. The raised error isRuntimeError("db gone")from the patched bump, which carries no id. Either assert the void argument or rename the test.💚 Proposed fix
- provider.delete_quote.assert_called_once()+ external_id = provider.create_quote.return_value.external_id+ provider.delete_quote.assert_called_once_with(external_id) assert Quote.objects.count() == 0🤖 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 `@apps/xero/tests/test_quote_manager.py` around lines 272 - 290, Update test_post_persist_failure_voids_and_names_the_id to assert that provider.delete_quote was called with the created quote’s external ID, in addition to verifying the void operation and empty database; ensure the assertion specifically validates the ID value rather than only call count.
244-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the safety property this test exists for.
_resolve_persist_collisiondocuments that a row on another job must not be voided, because that would delete another job's document. The test asserts only the raise. Add the negative assertion so a future change that voids first still fails the test.💚 Proposed assertion
with pytest.raises(ValueError, match="different job"): manager.create_document(breakdown=False) ++ provider.delete_quote.assert_not_called()+ assert Quote.objects.get(xero_id=external_id).job_id == other_job.id🤖 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 `@apps/xero/tests/test_quote_manager.py` around lines 244 - 270, Extend test_mirrored_row_on_another_job_raises to retain the created mirrored Quote and, after manager.create_document raises ValueError, assert that the other job’s Quote remains present and unvoided. Keep the existing “different job” exception assertion and verify the safety property enforced by _resolve_persist_collision.apps/xero/api.py (1)
573-589: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the delete path never needs
job.companysemantics overquote.company.The fallback selects
quote.companywhenjob.companyis None.XeroQuoteManager.delete_documentdoes not callvalidate_company, so the company is only bound for identity and logging. That matches the documented intent. One readability point: the chained conditional on Line 576 packs two decisions into one expression.♻️ Optional: split the fallback
- quote = Quote.objects.filter(job=job).select_related("company").first()- company = job.company if job.company is not None else quote.company if quote else None+ quote = Quote.objects.filter(job=job).select_related("company").first()+ company = job.company+ if company is None and quote is not None:+ company = quote.company🤖 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 `@apps/xero/api.py` around lines 573 - 589, Keep the delete path’s company selection semantics unchanged: prefer job.company, then fall back to quote.company, and return the existing error when neither exists. For readability, split the chained conditional around the delete handler’s company selection into explicit steps while preserving the XeroQuoteManager(company=company, job=job, ...) behavior.apps/xero/tests/test_document_api.py (1)
339-339: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why
untracked_updateis used on a tracked field.
Job.untracked_updatestates: "Use only for migrations and bookkeeping fields that are in UNTRACKED_FIELDS."companyis not inJob.UNTRACKED_FIELDS. The call is reasonable here, because the test must reproduce a legacy row state without emitting aJobEvent. Record that reason inline so the next reader does not treat it as a template for production code.📝 Proposed comment
- Job.objects.filter(pk=job.pk).untracked_update(company=None)+ # untracked_update, not save(staff=...): the state under test is a row+ # whose company was cleared, and a tracked save would add a JobEvent+ # the scenario never produced.+ Job.objects.filter(pk=job.pk).untracked_update(company=None)As per coding guidelines: "Comments must document the rejected obvious alternative and the factual constraint that rejected it."
🤖 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 `@apps/xero/tests/test_document_api.py` at line 339, Document the intentional use of Job.objects.filter(...).untracked_update in this test: state that it creates a legacy row state without emitting a JobEvent, and explicitly note that normal tracked updates are rejected because they would emit that event. Make clear this is test-only behavior and not a production usage pattern.Source: Coding guidelines
apps/xero/provider.py (1)
340-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the validation-error mapping.
The same three steps now appear in
create_quote(Lines 290-297),delete_quote(Lines 349-357), and_create_or_update_purchase_order(Lines 457-465): collectstr(ve.message), log a warning, and return a failedDocumentResultwith the joined text. A small private helper keeps one implementation of the element-error contract.Also note Lines 347-348 build
updated_quotesonly to index element zero. A direct guard reads shorter.♻️ Proposed helper
`@staticmethod`def_validation_failure( element: Any, context: str, external_id: str|None=None ) ->DocumentResult|None: """Return the element-level failure result, or None when Xero accepted it."""ifnotelement.validation_errors: returnNoneerrors= [str(ve.message) forveinelement.validation_errors] logger.warning("Xero %s validation errors: %s", context, errors) returnDocumentResult( success=False, external_id=external_id, error=" | ".join(errors), validation_errors=errors, )- updated_quotes = response.quotes or []- updated = updated_quotes[0] if updated_quotes else None- if updated is not None and updated.validation_errors:- errors = [str(ve.message) for ve in updated.validation_errors]- logger.warning("Xero quote %s delete validation errors: %s", external_id, errors)- return DocumentResult(- success=False,- external_id=external_id,- error=" | ".join(errors),- validation_errors=errors,- )+ updated = next(iter(response.quotes or []), None)+ if updated is not None:+ rejected = self._validation_failure(+ updated, f"quote {external_id} delete", external_id+ )+ if rejected is not None:+ return rejectedAs per coding guidelines: "Use one implementation per concept; search before implementing and extend a near-match instead of creating a parallel implementation."
🤖 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 `@apps/xero/provider.py` around lines 340 - 357, Extract the repeated element-level validation handling from create_quote, delete_quote, and _create_or_update_purchase_order into one private helper, such as _validation_failure, that maps messages, logs the context, and returns the failed DocumentResult or None. Replace each inline implementation with the helper while preserving each operation’s context and external_id, and simplify delete_quote to guard the first response element directly instead of building updated_quotes solely for indexing.Source: Coding guidelines
🤖 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 `@docs/rewrite-status.md`:
- Around line 876-887: Renumber the ordered backlog list in
docs/rewrite-status.md so the new entries around the split and ultrareview
cleanup items remain consistent with all subsequent entries. Update the
following existing numbered items to shift their source numbers accordingly, or
convert the entire list to `1.` markers while preserving its rendered order.
In `@frontend/src/features/job/costing/CostLineGrid.tsx`:
- Around line 324-325: Update persistDraftIfReady and the draft commit flow so
edits made while draft creation is in flight are queued or reconciled and
applied to the newly created line instead of being lost. Cover both the
description edit at frontend/src/features/job/costing/CostLineGrid.tsx:324-325
and numeric edit at frontend/src/features/job/costing/CostLineGrid.tsx:384-385;
retain the draft until all later edits are persisted, and add a delayed-create
test that edits a field before the initial POST resolves.
- Around line 381-383: Update the unit_cost handling in CostLineGrid so derived
unit_rev is recalculated for every unit-cost edit on non-time rows, removing the
draft.unit_rev === null guard. Keep using derivedUnitRev with the current value
and materialsMarkup, matching the existing server-row recalculation behavior.
In `@frontend/src/features/job/costing/useCostLines.test.ts`:
- Around line 50-63: Update the test around restoreDeletedLine to make lineX the
current cached entry with its distinct rejected-optimistic unit_rev, while
keeping snapshotX at 12.00. Preserve the existing assertions that only y is
reinserted and assert restored x retains lineX’s current value, distinguishing
it from the snapshot.
---
Nitpick comments:
In `@apps/xero/api.py`:
- Around line 573-589: Keep the delete path’s company selection semantics
unchanged: prefer job.company, then fall back to quote.company, and return the
existing error when neither exists. For readability, split the chained
conditional around the delete handler’s company selection into explicit steps
while preserving the XeroQuoteManager(company=company, job=job, ...) behavior.
In `@apps/xero/provider.py`:
- Around line 340-357: Extract the repeated element-level validation handling
from create_quote, delete_quote, and _create_or_update_purchase_order into one
private helper, such as _validation_failure, that maps messages, logs the
context, and returns the failed DocumentResult or None. Replace each inline
implementation with the helper while preserving each operation’s context and
external_id, and simplify delete_quote to guard the first response element
directly instead of building updated_quotes solely for indexing.
In `@apps/xero/tests/test_document_api.py`:
- Line 339: Document the intentional use of
Job.objects.filter(...).untracked_update in this test: state that it creates a
legacy row state without emitting a JobEvent, and explicitly note that normal
tracked updates are rejected because they would emit that event. Make clear this
is test-only behavior and not a production usage pattern.
In `@apps/xero/tests/test_quote_manager.py`:
- Around line 208-243: Extend
test_same_xero_id_collision_adopts_the_mirrored_row to assert the adopted
Quote’s total_excl_tax and total_incl_tax match the provider result’s raw
totals, confirming adoption overwrites the mirrored zero values.
- Around line 272-290: Update test_post_persist_failure_voids_and_names_the_id
to assert that provider.delete_quote was called with the created quote’s
external ID, in addition to verifying the void operation and empty database;
ensure the assertion specifically validates the ID value rather than only call
count.
- Around line 244-270: Extend test_mirrored_row_on_another_job_raises to retain
the created mirrored Quote and, after manager.create_document raises ValueError,
assert that the other job’s Quote remains present and unvoided. Keep the
existing “different job” exception assertion and verify the safety property
enforced by _resolve_persist_collision.
🪄 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: 9be30f0d-b9c7-4e0e-8913-25c45c8a6bbf
📒 Files selected for processing (16)
apps/accounting/services/quote_pdf.pyapps/accounting/tests/test_quote_pdf.pyapps/xero/api.pyapps/xero/documents/quote.pyapps/xero/provider.pyapps/xero/tests/test_document_api.pyapps/xero/tests/test_provider_documents.pyapps/xero/tests/test_quote_manager.pydocs/code-quality.mddocs/rewrite-status.mdfrontend/src/features/job/costing/CostLineGrid.test.tsxfrontend/src/features/job/costing/CostLineGrid.tsxfrontend/src/features/job/costing/useAutosaveField.tsfrontend/src/features/job/costing/useCostLines.test.tsfrontend/src/features/job/costing/useCostLines.tsstubs/xero_python/accounting/__init__.pyi
Comment on lines
+876
to
+887
| 5. Split `apps/xero` by capability — routers and provider modules for | ||
| connection, contacts, sales documents, purchasing, sync — keeping | ||
| invoice/quote/PO domain orchestration separate. `api.py` is ~1,200 lines | ||
| and `provider.py` ~600; the shared document-endpoint adapter (landed with | ||
| the quote hardening) stops the scaffolding drift, but the file split is | ||
| deliberate post-cutover structure work. | ||
| 6. Ultrareview sub-cap cleanups from the quote slice: managers read | ||
| provider-private `_sub_total`/`_total` raw keys the readonly provider | ||
| must fabricate; `EMPTY_SERVER_SHAPE` could be a `Pick<CostLineOut, ...>`; | ||
| XeroQuoteCard/JobInvoiceCard are siblings with drift; the item picker's | ||
| stock search fires per keystroke undebounced; the quote tab duplicates | ||
| the HOURS formatter; a dead "No online URL" toast. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Renumber the backlog items.
The new entries are written as 5. (Line 876) and 6. (Line 882). Lines 888 and 891 already use 5. and 6.. Markdown renumbers ordered lists on render, so the published list will read 5, 6, 7, 8, … while the source text keeps the old numbers. Anyone citing "backlog item 9" will then reference a different entry in the source than in the rendered document. Number the new entries 5. and 6. and shift the following entries, or convert the whole list to 1. markers.
🤖 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 `@docs/rewrite-status.md` around lines 876 - 887, Renumber the ordered backlog
list in docs/rewrite-status.md so the new entries around the split and
ultrareview cleanup items remain consistent with all subsequent entries. Update
the following existing numbered items to shift their source numbers accordingly,
or convert the entire list to `1.` markers while preserving its rendered order.
Comment on lines
+324
to
+325
| context.updateDraft(gridRow.localId, patch) | ||
| context.commitDraftField(gridRow.localId) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve edits made while draft creation is in flight.
persistDraftIfReady captures the earlier draft in the create request, ignores later commits while persistingRef contains the local ID, and removes the draft on success. An edit during that request is silently lost.
frontend/src/features/job/costing/CostLineGrid.tsx#L324-L325: queue or reconcile a description edit made after draft creation starts.frontend/src/features/job/costing/CostLineGrid.tsx#L384-L385: queue or reconcile a numeric edit made after draft creation starts.
Keep the draft until later edits are persisted to the created line. Add a delayed-create test that edits a field before the first POST resolves.
📍 Affects 1 file
frontend/src/features/job/costing/CostLineGrid.tsx#L324-L325(this comment)frontend/src/features/job/costing/CostLineGrid.tsx#L384-L385
🤖 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 `@frontend/src/features/job/costing/CostLineGrid.tsx` around lines 324 - 325,
Update persistDraftIfReady and the draft commit flow so edits made while draft
creation is in flight are queued or reconciled and applied to the newly created
line instead of being lost. Cover both the description edit at
frontend/src/features/job/costing/CostLineGrid.tsx:324-325 and numeric edit at
frontend/src/features/job/costing/CostLineGrid.tsx:384-385; retain the draft
until all later edits are persisted, and add a delayed-create test that edits a
field before the initial POST resolves.
Comment on lines
+381
to
+383
| if (fieldName === 'unit_cost' && kind !== 'time' && gridRow.draft.unit_rev === null) { | ||
| patch.unit_rev = derivedUnitRev(value, context.materialsMarkup) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recalculate draft revenue after every unit-cost edit.
After a failed create, a draft can retain derived unit_rev: "12.00". If the user changes unit_cost from "10" to "20", this condition skips recalculation and the retry submits inconsistent values. The server-row path already recalculates revenue for every cost edit.
Proposed fix
- if (fieldName === 'unit_cost' && kind !== 'time' && gridRow.draft.unit_rev === null) {+ if (fieldName === 'unit_cost' && kind !== 'time') {
patch.unit_rev = derivedUnitRev(value, context.materialsMarkup)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if(fieldName==='unit_cost'&&kind!=='time'&&gridRow.draft.unit_rev===null){ | |
| patch.unit_rev=derivedUnitRev(value,context.materialsMarkup) | |
| } | |
| if(fieldName==='unit_cost'&&kind!=='time'){ | |
| patch.unit_rev=derivedUnitRev(value,context.materialsMarkup) | |
| } |
🤖 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 `@frontend/src/features/job/costing/CostLineGrid.tsx` around lines 381 - 383,
Update the unit_cost handling in CostLineGrid so derived unit_rev is
recalculated for every unit-cost edit on non-time rows, removing the
draft.unit_rev === null guard. Keep using derivedUnitRev with the current value
and materialsMarkup, matching the existing server-row recalculation behavior.
Comment on lines
+50
to
+63
| it('re-inserts only the deleted line at its index, not the whole snapshot', () => { | ||
| const lineX = line({ id: 'x', unit_rev: 'rejected-optimistic' }) | ||
| const snapshotX = line({ id: 'x', unit_rev: '12.00' }) | ||
| const lineY = line({ id: 'y' }) | ||
| // The current cache has X already rolled back by its own PATCH failure. | ||
| const current = [line({ id: 'x', unit_rev: '12.00' })] | ||
| const snapshot = [snapshotX, lineY] | ||
| void lineX | ||
| const restored = restoreDeletedLine(current, snapshot, 'y') | ||
| expect(restored.map((entry) => entry.id)).toEqual(['x', 'y']) | ||
| // X keeps its current (rolled-back) value, not the snapshot's state. | ||
| expect(restored[0]!.unit_rev).toBe('12.00') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the preservation assertion distinguish current state from the snapshot.
current[0] and snapshotX both have unit_rev: "12.00". The test therefore passes if rollback replaces the current cache with the full snapshot. Use lineX as the current entry and assert that its distinct value remains.
Proposed fix
- const current = [line({ id: 'x', unit_rev: '12.00' })]+ const current = [lineX]
@@
- expect(restored[0]!.unit_rev).toBe('12.00')+ expect(restored[0]!.unit_rev).toBe('rejected-optimistic')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('re-inserts only the deleted line at its index, not the whole snapshot',()=>{ | |
| constlineX=line({id: 'x',unit_rev: 'rejected-optimistic'}) | |
| constsnapshotX=line({id: 'x',unit_rev: '12.00'}) | |
| constlineY=line({id: 'y'}) | |
| // The current cache has X already rolled back by its own PATCH failure. | |
| constcurrent=[line({id: 'x',unit_rev: '12.00'})] | |
| constsnapshot=[snapshotX,lineY] | |
| voidlineX | |
| constrestored=restoreDeletedLine(current,snapshot,'y') | |
| expect(restored.map((entry)=>entry.id)).toEqual(['x','y']) | |
| // X keeps its current (rolled-back) value, not the snapshot's state. | |
| expect(restored[0]!.unit_rev).toBe('12.00') | |
| it('re-inserts only the deleted line at its index, not the whole snapshot',()=>{ | |
| constlineX=line({id: 'x',unit_rev: 'rejected-optimistic'}) | |
| constsnapshotX=line({id: 'x',unit_rev: '12.00'}) | |
| constlineY=line({id: 'y'}) | |
| // The current cache has X already rolled back by its own PATCH failure. | |
| constcurrent=[lineX] | |
| constsnapshot=[snapshotX,lineY] | |
| voidlineX | |
| constrestored=restoreDeletedLine(current,snapshot,'y') | |
| expect(restored.map((entry)=>entry.id)).toEqual(['x','y']) | |
| // X keeps its current (rolled-back) value, not the snapshot's state. | |
| expect(restored[0]!.unit_rev).toBe('rejected-optimistic') |
🤖 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 `@frontend/src/features/job/costing/useCostLines.test.ts` around lines 50 - 63,
Update the test around restoreDeletedLine to make lineX the current cached entry
with its distinct rejected-optimistic unit_rev, while keeping snapshotX at
12.00. Preserve the existing assertions that only y is reinserted and assert
restored x retains lineX’s current value, distinguishing it from the snapshot.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follow-up to #48, fixing every verified correctness finding from the earmarked ultrareview and the user's own review. Full E2E gate green on this branch (39/39, writes live); 1716 unit tests, coverage 88.52%.
Backend
summarize_errors=False+ element-levelvalidation_errors): deleting an ACCEPTED quote can no longer read as success and silently drop the local mirror while the quote lives on in Xero._finalize_created_quote): totals validation, the insert, and the timestamp bump all void the orphan best-effort with the external id carried in the error — a real Xero quote can no longer become untracked, and a retry can no longer duplicate it. The IntegrityError inside it is discriminated by state: a same-xero_idrow means the sync mirrored our own quote first and is adopted (linking the job the transform never sets) — never voided; only the job-constraint race voids. The persist happens at the catch site, which the handler-contract gate verified the hard way.validate_company()on the delete path, and the endpoint falls back to the quote row's own company whenjob.companywas cleared.error_typedrift disappears with the copies. The fullapps/xerocapability split is recorded as post-cutover backlog, per "finish before improving".Frontend
unit_revfromunit_cost— a filled phantom used to silently never POST.adjust(v1 rule restored — material means a stock pick).Spec
The quote spec now hard-asserts the fresh job's line state before its repair pass (exactly one missing stock binding, nothing else wrong) and all-clear after — a regression in line creation fails the gate instead of being silently healed, while the repair pass keeps exercising the grid.
Declined with evidence in the review threads: the xero-python
get_quote_as_pdfclaim (the pinned SDK has it; the live spec exercises it) and the compact-match regex (equivalent in power to the existing check).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Costing Improvements