From 9859f7574490afa11aae150e791bbfbe9d4d6d1e Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 9 Aug 2026 20:28:40 +1200 Subject: [PATCH 1/5] Quote hardening B1-B5: explicit validation, full compensation, unbricked deletes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/accounting/services/quote_pdf.py | 36 ++-- apps/accounting/tests/test_quote_pdf.py | 10 +- apps/xero/api.py | 12 +- apps/xero/documents/quote.py | 229 ++++++++++++++------- apps/xero/provider.py | 34 ++- apps/xero/tests/test_document_api.py | 24 +++ apps/xero/tests/test_provider_documents.py | 43 +++- apps/xero/tests/test_quote_manager.py | 127 +++++++++++- stubs/xero_python/accounting/__init__.pyi | 1 + 9 files changed, 414 insertions(+), 102 deletions(-) diff --git a/apps/accounting/services/quote_pdf.py b/apps/accounting/services/quote_pdf.py index 996f6b05..3b791850 100644 --- a/apps/accounting/services/quote_pdf.py +++ b/apps/accounting/services/quote_pdf.py @@ -12,13 +12,18 @@ @dataclass(frozen=True) class QuotePdfInspection: - """Structured evidence from a provider-rendered quote PDF.""" + """Structured evidence from a provider-rendered quote PDF. + + ``retained_pdf_path`` names the kept file when the marker was absent — + an unlocatable diagnostic artifact is as good as a deleted one. + """ quote_id: str remote_branding_theme_id: str | None configured_branding_theme_id: str | None page_count: int contains_expected_text: bool + retained_pdf_path: str | None def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection: @@ -43,7 +48,10 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection: page_count = len(reader.pages) if not page_text: - raise ValueError(f"Quote {quote_id} PDF contains no extractable text") + raise ValueError( + f"Quote {quote_id} PDF contains no extractable text " + f"(file retained at {document.temporary_file_path})" + ) # Xero's text layer sometimes wraps mid-phrase and sometimes drops word # spaces entirely — match both the space-normalised and compact forms so @@ -52,22 +60,22 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection: compact_expected_text = "".join(normalized_expected_text.split()) compact_document_text = "".join(normalized_document_text.split()) configured_theme_id = CompanyDefaults.get_solo().xero_sales_branding_theme_id - inspection = QuotePdfInspection( + contains_expected_text = ( + normalized_expected_text in normalized_document_text + or compact_expected_text in compact_document_text + ) + # Only when the marker was FOUND: an absent marker is exactly the case an + # operator needs the rendered PDF for, so it keeps the file just like the + # exception paths above do — and reports where it is. + if contains_expected_text: + Path(document.temporary_file_path).unlink(missing_ok=True) + return QuotePdfInspection( quote_id=document.external_id, remote_branding_theme_id=document.document_theme_external_id, configured_branding_theme_id=( str(configured_theme_id) if configured_theme_id is not None else None ), page_count=page_count, - contains_expected_text=( - normalized_expected_text in normalized_document_text - or compact_expected_text in compact_document_text - ), + contains_expected_text=contains_expected_text, + retained_pdf_path=None if contains_expected_text else document.temporary_file_path, ) - # Only when the marker was FOUND: an absent marker is exactly the case an - # operator needs the rendered PDF for, so it keeps the file just like the - # exception paths above do. (Improves on the ported behaviour, which - # deleted the file in its own diagnostic case.) - if inspection.contains_expected_text: - Path(document.temporary_file_path).unlink(missing_ok=True) - return inspection diff --git a/apps/accounting/tests/test_quote_pdf.py b/apps/accounting/tests/test_quote_pdf.py index a7dad6c8..558050a7 100644 --- a/apps/accounting/tests/test_quote_pdf.py +++ b/apps/accounting/tests/test_quote_pdf.py @@ -76,6 +76,7 @@ def test_terms_marker_survives_pdf_line_wrapping(self, mock_get_provider: Mock) assert result.page_count == 1 assert result.remote_branding_theme_id == REMOTE_THEME_ID assert result.configured_branding_theme_id == REMOTE_THEME_ID + assert result.retained_pdf_path is None assert not pdf_path.exists() @patch("apps.accounting.services.quote_pdf.get_provider") @@ -90,7 +91,9 @@ def test_missing_or_differently_cased_terms_marker_is_red( assert not result.contains_expected_text # The absent marker is the diagnostic case: the rendered PDF survives - # for the operator, unlike a found marker's cleanup. + # for the operator, and the report says WHERE — an unlocatable + # artifact is as good as a deleted one. + assert result.retained_pdf_path == str(pdf_path) assert pdf_path.exists() pdf_path.unlink() @@ -131,9 +134,10 @@ def test_blank_pages_raise_rather_than_reporting_terms_absent( pdf_path = _write_pdf([]) mock_get_provider.return_value = _provider_for_pdf(quote_id, pdf_path) - with pytest.raises(ValueError, match="no extractable text"): + with pytest.raises(ValueError, match="no extractable text") as caught: inspect_quote_pdf(quote_id, EXPECTED_TERMS) + assert str(pdf_path) in str(caught.value) assert pdf_path.exists() pdf_path.unlink() @@ -154,6 +158,7 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None: configured_branding_theme_id=REMOTE_THEME_ID, page_count=2, contains_expected_text=False, + retained_pdf_path="/var/artifacts/retained.pdf", ) output = StringIO() @@ -170,4 +175,5 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None: "page_count": 2, "quote_id": str(quote_id), "remote_branding_theme_id": REMOTE_THEME_ID, + "retained_pdf_path": "/var/artifacts/retained.pdf", } diff --git a/apps/xero/api.py b/apps/xero/api.py index 337f6f29..219cf08b 100644 --- a/apps/xero/api.py +++ b/apps/xero/api.py @@ -27,7 +27,7 @@ from ninja.errors import HttpError from ninja.responses import Status -from apps.accounting.models import Invoice +from apps.accounting.models import Invoice, Quote from apps.accounting.registry import get_provider from apps.accounting.services.invoice_calculation import ( InvoiceCalculationError, @@ -577,14 +577,18 @@ def xero_delete_quote( XeroDocumentErrorResponse(success=False, error=f"Job with ID {job_id} not found."), ) - if job.company is None: + # The quote row carries its own company (non-null FK): a job whose + # company was cleared after quoting must still be able to delete. + 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 + if company is None: return Status( 400, XeroDocumentErrorResponse( - success=False, error="Job has no client company; cannot delete its quote." + success=False, error="Job has no client company and no Xero quote." ), ) - manager = XeroQuoteManager(company=job.company, job=job, staff=_staff(request)) + manager = XeroQuoteManager(company=company, job=job, staff=_staff(request)) result = manager.delete_document() if not result["success"]: diff --git a/apps/xero/documents/quote.py b/apps/xero/documents/quote.py index e7bd0035..7f92092e 100644 --- a/apps/xero/documents/quote.py +++ b/apps/xero/documents/quote.py @@ -168,61 +168,153 @@ def _bump_job_updated_at(self) -> None: """Bust the job ETag so the tab's refetch sees the new quote state.""" self.job.save(staff=self.staff, update_fields=["updated_at"]) - def _persist_created_quote( + def _void_orphan(self, external_id: str, cause: Exception) -> None: + """Void a Xero quote no local row accounts for; raise if it cannot be. + + Skips when ANY local row carries this xero_id: that row either just + persisted, was adopted, or belongs to another job — in every case the + quote is accounted for and deleting it would destroy a real document. + """ + if Quote.objects.filter(xero_id=external_id).exists(): + return + logger.warning( + "Voiding orphan Xero quote %s for job %s after: %s", + external_id, + self.job.id, + cause, + ) + void_result = self.provider.delete_quote(external_id) + if not void_result.success: + # Deliberately a raise, not a swallow: an unvoidable orphan needs + # an operator, and this message carries the id. + raise ValueError( + f"Job {self.job.job_number}: quote push failed after Xero accepted " + f"it, and the orphan Xero quote {external_id} could not be voided: " + f"{void_result.error}" + ) from cause + + def _finalize_created_quote( self, external_id: str, result: "DocumentResult", payload: QuotePayload, - raw: dict[str, object], - ) -> "Quote | XeroDocumentResponse": - """Store the local mirror row, or lose the push race and compensate. - - A concurrent push can win between the business gate and this insert, - in which case a second REAL quote already exists in Xero. The loser - must void its quote before refusing — an uncompensated orphan leaves - Xero and the app permanently disagreeing, with nothing recording the - orphan's id. + ) -> XeroDocumentResponse: + """Persist the mirror row and side effects; never orphan the quote. + + A REAL quote now exists in Xero, so every failure in this tail — + totals validation, the insert, the timestamp bump — compensates by + voiding it (or adopting the row the sync mirrored first) before the + error propagates. Without that, Xero and the app permanently + disagree and a retry creates a duplicate. """ + raw = result.raw_response or {} try: - # Savepoint: without it the IntegrityError poisons any enclosing - # transaction and the compensation below (which writes an - # AppError row) cannot run. - with transaction.atomic(): - return Quote.objects.create( - xero_id=external_id, - job=self.job, - company=self.company, - # The payload's date, not a fresh localdate(): a request - # spanning midnight must not store a date one day after - # the Xero document's. - date=payload.date, - status=QuoteStatus.DRAFT, - number=result.number, - total_excl_tax=Decimal(str(raw["_sub_total"])), - total_incl_tax=Decimal(str(raw["_total"])), - xero_last_synced=timezone.now(), - xero_last_modified=timezone.now(), - online_url=result.online_url, - raw_json=raw, + # get() is None, not `in`: a null total must get this crafted + # message too, not an opaque InvalidOperation. Never a $0.00 + # fallback either way (ADR 0015). + missing = [key for key in ("_sub_total", "_total") if raw.get(key) is None] + if missing: + raise ValueError( + f"Provider quote payload is missing totals {missing} " + f"(Xero quote {external_id} will be voided): {raw}" ) - except IntegrityError as exc: - persist_app_error(exc, AppErrorContext(job_id=self.job.id)) - logger.warning( - "Concurrent quote push for job %s; voiding orphan Xero quote %s", - self.job.id, - external_id, - ) - void_result = self.provider.delete_quote(external_id) - if not void_result.success: - # Deliberately a raise, not a swallow: an unvoidable orphan - # needs an operator, and the message carries the id the - # AppError row above cannot. + try: + # Savepoint: without it an IntegrityError poisons any + # enclosing transaction and the compensation (which writes an + # AppError row) cannot run. The bump sits inside so a failure + # after the insert rolls the row back and takes the void path + # rather than leaving a row for a quote the user saw fail. + with transaction.atomic(): + quote = Quote.objects.create( + xero_id=external_id, + job=self.job, + company=self.company, + # The payload's date, not a fresh localdate(): a + # request spanning midnight must not store a date one + # day after the Xero document's. + date=payload.date, + status=QuoteStatus.DRAFT, + number=result.number, + total_excl_tax=Decimal(str(raw["_sub_total"])), + total_incl_tax=Decimal(str(raw["_total"])), + xero_last_synced=timezone.now(), + xero_last_modified=timezone.now(), + online_url=result.online_url, + raw_json=raw, + ) + self._bump_job_updated_at() + except IntegrityError as exc: + return self._resolve_persist_collision(exc, external_id, result, raw) + except Exception as exc: + self._void_orphan(external_id, exc) + raise + + logger.info("Quote %s created successfully for job %s", quote.id, self.job.id) + self._add_xero_history_note("quote", external_id) + self._create_job_event("quote_created", {"xero_quote_number": quote.number}) + return self._success_response(quote, external_id) + + def _resolve_persist_collision( + self, + exc: IntegrityError, + external_id: str, + result: "DocumentResult", + raw: dict[str, object], + ) -> XeroDocumentResponse: + """Discriminate the two unique constraints by state, never by guess. + + Same xero_id already present → the sync/webhook mirrored OUR quote + between the Xero create and this insert (the mirror transform never + links a job): adopt it. Otherwise the job's one-quote constraint + fired → a concurrent push won, and OUR quote is a duplicate to void. + """ + persist_app_error(exc, AppErrorContext(job_id=self.job.id)) + mirrored = Quote.objects.filter(xero_id=external_id).first() + if mirrored is not None: + if mirrored.job_id is not None and mirrored.job_id != self.job.id: + # No guessing: our fresh external id on another job's row is + # corruption, and "voiding" it would delete their document. raise ValueError( - f"Job {self.job.job_number}: concurrent quote push left an " - f"orphan Xero quote {external_id} that could not be " - f"voided: {void_result.error}" + f"Xero quote {external_id} created for job {self.job.job_number} " + f"is linked to a different job {mirrored.job_id}" ) from exc - return self._refusal(f"Job {self.job.job_number} already has a Xero quote.") + logger.info("Adopting sync-mirrored quote %s for job %s", external_id, self.job.id) + mirrored.job = self.job + mirrored.number = mirrored.number or result.number + mirrored.online_url = mirrored.online_url or result.online_url + mirrored.raw_json = mirrored.raw_json or raw + mirrored.total_excl_tax = Decimal(str(raw["_sub_total"])) + mirrored.total_incl_tax = Decimal(str(raw["_total"])) + mirrored.save() + self._bump_job_updated_at() + self._add_xero_history_note("quote", external_id) + self._create_job_event("quote_created", {"xero_quote_number": mirrored.number}) + return self._success_response(mirrored, external_id) + + logger.warning( + "Concurrent quote push for job %s; voiding duplicate Xero quote %s", + self.job.id, + external_id, + ) + void_result = self.provider.delete_quote(external_id) + if not void_result.success: + raise ValueError( + f"Job {self.job.job_number}: concurrent quote push left an " + f"orphan Xero quote {external_id} that could not be " + f"voided: {void_result.error}" + ) from exc + return self._refusal(f"Job {self.job.job_number} already has a Xero quote.") + + def _success_response(self, quote: Quote, external_id: str) -> XeroDocumentResponse: + return { + "success": True, + "quote_id": str(quote.id), + "xero_id": external_id, + "company": self.company.name, + "total_excl_tax": str(quote.total_excl_tax), + "total_incl_tax": str(quote.total_incl_tax), + "online_url": quote.online_url, + } def create_document(self, breakdown: bool) -> XeroDocumentResponse: """Create the quote via the provider and persist the local record. @@ -231,7 +323,13 @@ def create_document(self, breakdown: bool) -> XeroDocumentResponse: line carries the quote cost set's total revenue. """ try: - self.validate_company() + try: + self.validate_company() + # deliberate-swallow: a company never synced to Xero is an + # expected state the user fixes from Company Settings — a + # readable 400 like every sibling gate, not a 500 + except ValueError as exc: + return self._refusal(str(exc)) refused = self._check_business_state() if refused is not None: return refused @@ -263,34 +361,7 @@ def create_document(self, breakdown: bool) -> XeroDocumentResponse: if not result.external_id or not result.number: raise ValueError(f"Provider reported quote success without an id/number: {result}") - raw = result.raw_response or {} - # get() is None, not `in`: a null total must get this crafted - # message too, not an opaque InvalidOperation. Never a $0.00 - # fallback either way (ADR 0015). - missing = [key for key in ("_sub_total", "_total") if raw.get(key) is None] - if missing: - raise ValueError(f"Provider quote payload is missing totals {missing}: {raw}") - - persisted = self._persist_created_quote(result.external_id, result, payload, raw) - if not isinstance(persisted, Quote): - return persisted - quote = persisted - - self._bump_job_updated_at() - - logger.info("Quote %s created successfully for job %s", quote.id, self.job.id) - self._add_xero_history_note("quote", result.external_id) - self._create_job_event("quote_created", {"xero_quote_number": quote.number}) - - return { - "success": True, - "quote_id": str(quote.id), - "xero_id": result.external_id, - "company": self.company.name, - "total_excl_tax": str(quote.total_excl_tax), - "total_incl_tax": str(quote.total_incl_tax), - "online_url": result.online_url, - } + return self._finalize_created_quote(result.external_id, result, payload) except Exception as exc: logger.exception("Unexpected error during quote creation for job %s", self.job.id) @@ -298,9 +369,13 @@ def create_document(self, breakdown: bool) -> XeroDocumentResponse: raise def delete_document(self) -> XeroDocumentResponse: - """Delete the quote via the provider and remove the local record.""" + """Delete the quote via the provider and remove the local record. + + No validate_company(): nothing on the delete path uses the contact + id, and requiring a Xero-syncable company to DELETE is what bricks a + job whose company was cleared or never synced. + """ try: - self.validate_company() xero_id = self.get_xero_id() if not xero_id: # Expected, not exceptional: a double-click or a stale tab diff --git a/apps/xero/provider.py b/apps/xero/provider.py index 8f62150c..6e7a44df 100644 --- a/apps/xero/provider.py +++ b/apps/xero/provider.py @@ -276,12 +276,25 @@ def create_quote(self, payload: QuotePayload) -> DocumentResult: terms=payload.terms, ) + # summarize_errors=False, PO-style: element-level failures come + # back explicitly instead of relying on the endpoint family's + # default whole-request 400. response = api.create_quotes( - tenant_id, quotes={"Quotes": [self._to_xero_payload(xero_quote)]} + tenant_id, + quotes={"Quotes": [self._to_xero_payload(xero_quote)]}, + summarize_errors=False, ) if not response.quotes: raise ValueError("Xero returned no quotes for a create_quotes call") created = response.quotes[0] + if created.validation_errors: + errors = [str(ve.message) for ve in created.validation_errors] + logger.warning("Xero quote create validation errors: %s", errors) + return DocumentResult( + success=False, + error=" | ".join(errors), + validation_errors=errors, + ) quote_id = str(created.quote_id) logger.info("Created Xero quote %s (%s)", created.quote_number, quote_id) @@ -324,9 +337,24 @@ def delete_quote(self, external_id: str) -> DocumentResult: contact=Contact(contact_id=existing.contact.contact_id), date=existing.date, ) - api.update_or_create_quotes( - tenant_id, quotes={"Quotes": [self._to_xero_payload(xero_quote)]} + response = api.update_or_create_quotes( + tenant_id, + quotes={"Quotes": [self._to_xero_payload(xero_quote)]}, + summarize_errors=False, ) + # Element-level rejection (e.g. the quote is ACCEPTED) must not + # read as success — the caller keeps the local mirror row. + 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, + ) logger.info("Deleted Xero quote %s", external_id) return DocumentResult(success=True, external_id=external_id) except Exception as exc: # noqa: BLE001 -- persisted, then converted to the result type callers require diff --git a/apps/xero/tests/test_document_api.py b/apps/xero/tests/test_document_api.py index 92da21bc..afd64314 100644 --- a/apps/xero/tests/test_document_api.py +++ b/apps/xero/tests/test_document_api.py @@ -323,6 +323,30 @@ def test_unknown_job_is_404(self, api: Client) -> None: response = api.delete(f"/api/xero/delete_quote/{uuid.uuid4()}") assert response.status_code == 404 + def test_company_cleared_after_quoting_still_deletes(self, api: Client, job: Job) -> None: + """The quote row carries its own company; a cleared job.company must + not brick deletion (nothing on the delete path needs the contact).""" + assert job.company is not None + Quote.objects.create( + xero_id=uuid.uuid4(), + number="QU-E2E-ORPHANED", + company=job.company, + job=job, + date="2026-08-09", + total_excl_tax=Decimal("100"), + total_incl_tax=Decimal("115"), + ) + Job.objects.filter(pk=job.pk).untracked_update(company=None) + + with ( + override_settings(XERO_READONLY=True), + patch("apps.xero.api.get_valid_token", return_value=TOKEN), + ): + response = api.delete(f"/api/xero/delete_quote/{job.id}") + + assert response.status_code == 200, response.content + assert not Quote.objects.filter(job=job).exists() + def test_no_token_is_401(self, api: Client, job: Job) -> None: with patch("apps.xero.api.get_valid_token", return_value=None): response = api.delete(f"/api/xero/delete_quote/{job.id}") diff --git a/apps/xero/tests/test_provider_documents.py b/apps/xero/tests/test_provider_documents.py index 89cf20df..8f4b5422 100644 --- a/apps/xero/tests/test_provider_documents.py +++ b/apps/xero/tests/test_provider_documents.py @@ -359,6 +359,7 @@ def test_success_returns_document_result(self) -> None: created = SimpleNamespace( quote_id=quote_id, quote_number="QU-0042", + validation_errors=None, _sub_total=250.0, _total=287.5, ) @@ -379,7 +380,11 @@ def test_success_returns_document_result(self) -> None: def test_none_reference_is_stripped(self) -> None: provider, api = _provider_with_api() api.create_quotes.return_value = SimpleNamespace( - quotes=[SimpleNamespace(quote_id=str(uuid.uuid4()), quote_number="QU-1")] + quotes=[ + SimpleNamespace( + quote_id=str(uuid.uuid4()), quote_number="QU-1", validation_errors=None + ) + ] ) payload = _quote_payload() payload.reference = None @@ -410,6 +415,23 @@ def test_api_exception_becomes_error_result(self) -> None: assert result.error == "Rate limit exceeded" assert result.status_code == 429 + def test_element_validation_errors_are_a_failure_result(self) -> None: + """summarize_errors=False makes element errors explicit, PO-style.""" + provider, api = _provider_with_api() + rejected = SimpleNamespace( + quote_id=str(uuid.uuid4()), + quote_number=None, + validation_errors=[SimpleNamespace(message="Terms too long")], + ) + api.create_quotes.return_value = SimpleNamespace(quotes=[rejected]) + + result = provider.create_quote(_quote_payload()) + + assert not result.success + assert result.error == "Terms too long" + assert result.validation_errors == ["Terms too long"] + assert api.create_quotes.call_args.kwargs["summarize_errors"] is False + class TestDeleteQuote: def test_pre_reads_then_upserts_deleted(self) -> None: @@ -437,6 +459,25 @@ def test_missing_quote_is_an_error_result(self) -> None: assert not result.success assert result.error is not None and "no quote" in result.error + def test_element_validation_errors_keep_the_quote(self) -> None: + """Deleting an ACCEPTED quote must not read as success.""" + provider, api = _provider_with_api() + external_id = str(uuid.uuid4()) + api.get_quote.return_value = SimpleNamespace( + quotes=[SimpleNamespace(contact=SimpleNamespace(contact_id="c-1"), date="2026-08-01")] + ) + rejected = SimpleNamespace( + quote_id=external_id, + validation_errors=[SimpleNamespace(message="Quote is ACCEPTED")], + ) + api.update_or_create_quotes.return_value = SimpleNamespace(quotes=[rejected]) + + result = provider.delete_quote(external_id) + + assert not result.success + assert result.error == "Quote is ACCEPTED" + assert api.update_or_create_quotes.call_args.kwargs["summarize_errors"] is False + class TestDownloadQuotePdf: def _quote_response(self, quote_id: str, theme_id: str | None) -> SimpleNamespace: diff --git a/apps/xero/tests/test_quote_manager.py b/apps/xero/tests/test_quote_manager.py index 0f1775b4..12da0127 100644 --- a/apps/xero/tests/test_quote_manager.py +++ b/apps/xero/tests/test_quote_manager.py @@ -144,12 +144,18 @@ def test_missing_raw_totals_raise( provider = Mock() provider.get_account_code.return_value = "200" provider.create_quote.return_value = _success_result(raw={"_quote_id": "q"}) + provider.delete_quote.return_value = DocumentResult(success=True) manager = _manager(company, job, office_staff, provider) - with pytest.raises(ValueError, match="_sub_total"): + with pytest.raises(ValueError, match="_sub_total") as caught: manager.create_document(breakdown=False) assert Quote.objects.count() == 0 + # The real quote exists in Xero: it must be voided, and the error + # must carry the external id so a failed void is still traceable. + provider.delete_quote.assert_called_once() + external_id = provider.create_quote.return_value.external_id + assert external_id is not None and external_id in str(caught.value) def test_null_raw_totals_raise_the_same_named_error( self, company: Company, job: Job, office_staff: Staff @@ -160,12 +166,14 @@ def test_null_raw_totals_raise_the_same_named_error( provider.create_quote.return_value = _success_result( raw={"_quote_id": "q", "_sub_total": None, "_total": None} ) + provider.delete_quote.return_value = DocumentResult(success=True) manager = _manager(company, job, office_staff, provider) with pytest.raises(ValueError, match="_sub_total"): manager.create_document(breakdown=False) assert Quote.objects.count() == 0 + provider.delete_quote.assert_called_once() def test_race_loser_voids_its_orphan_and_refuses( self, company: Company, job: Job, office_staff: Staff @@ -197,6 +205,91 @@ def concurrent_winner_lands_first(payload: object) -> DocumentResult: # noqa: A assert Quote.objects.count() == 1 # only the winner's row survives +class TestPostCreateCompensation: + """Every failure after the remote write must void or adopt, never orphan.""" + + def test_same_xero_id_collision_adopts_the_mirrored_row( + self, company: Company, job: Job, office_staff: Staff + ) -> None: + """The sync can mirror our own quote between the Xero create and the + local insert; voiding it would delete a legitimate document.""" + provider = Mock() + provider.get_account_code.return_value = "200" + result = _success_result() + external_id = result.external_id + assert external_id is not None + + def sync_mirrors_first(payload: object) -> DocumentResult: # noqa: ARG001 + Quote.objects.create( + xero_id=external_id, + job=None, + company=company, + date=timezone.localdate(), + total_excl_tax=Decimal("0"), + total_incl_tax=Decimal("0"), + ) + return result + + provider.create_quote.side_effect = sync_mirrors_first + manager = _manager(company, job, office_staff, provider) + + response = manager.create_document(breakdown=False) + + assert response["success"] is True + provider.delete_quote.assert_not_called() + adopted = Quote.objects.get(xero_id=external_id) + assert adopted.job_id == job.id + assert adopted.number == "QU-RAW-1" + + def test_mirrored_row_on_another_job_raises( + self, company: Company, office_staff: Staff, job: Job + ) -> None: + other_job = Job(company=company, name="Other Job", pricing_methodology="fixed_price") + other_job.save(staff=office_staff) + provider = Mock() + provider.get_account_code.return_value = "200" + result = _success_result() + external_id = result.external_id + assert external_id is not None + + def mirrored_to_wrong_job(payload: object) -> DocumentResult: # noqa: ARG001 + Quote.objects.create( + xero_id=external_id, + job=other_job, + company=company, + date=timezone.localdate(), + total_excl_tax=Decimal("0"), + total_incl_tax=Decimal("0"), + ) + return result + + provider.create_quote.side_effect = mirrored_to_wrong_job + manager = _manager(company, job, office_staff, provider) + + with pytest.raises(ValueError, match="different job"): + manager.create_document(breakdown=False) + + def test_post_persist_failure_voids_and_names_the_id( + self, company: Company, job: Job, office_staff: Staff + ) -> None: + provider = Mock() + provider.get_account_code.return_value = "200" + provider.create_quote.return_value = _success_result() + provider.delete_quote.return_value = DocumentResult(success=True) + manager = _manager(company, job, office_staff, provider) + + with ( + patch.object( + XeroQuoteManager, "_bump_job_updated_at", side_effect=RuntimeError("db gone") + ), + pytest.raises(RuntimeError, match="db gone"), + ): + manager.create_document(breakdown=False) + + provider.delete_quote.assert_called_once() + assert Quote.objects.count() == 0 + + class TestCreateBusinessGates: """Expected refusals are 400 values and the provider is never called.""" @@ -222,6 +315,19 @@ def test_already_quoted_job_is_refused( assert "already has a Xero quote" in str(response["error"]) provider.create_quote.assert_not_called() + def test_unsynced_company_is_a_400_not_a_500( + self, company: Company, job: Job, office_staff: Staff + ) -> None: + """A company never pushed to Xero is an expected state, not a crash.""" + Company.objects.filter(pk=company.pk).update(xero_contact_id=None) + company.refresh_from_db() + + response, provider = self._refused(company, job, office_staff) + + assert response["error_type"] == "validation_error" + assert "Xero contact" in str(response["error"]) + provider.create_quote.assert_not_called() + def test_time_materials_job_is_refused(self, company: Company, office_staff: Staff) -> None: tm_job = Job(company=company, name="T&M Job", pricing_methodology="time_materials") tm_job.save(staff=office_staff) @@ -424,6 +530,25 @@ def test_provider_failure_is_a_400_value_and_keeps_local_row( assert response["error"] == "Quote is ACCEPTED" assert Quote.objects.count() == 1 + def test_delete_needs_no_xero_valid_company( + self, company: Company, job: Job, office_staff: Staff + ) -> None: + """Deletion must not require a syncable company — that requirement is + what bricks a job whose company was cleared or never synced.""" + quote = _existing_quote(job, company) + Company.objects.filter(pk=company.pk).update(xero_contact_id=None) + company.refresh_from_db() + provider = Mock() + provider.delete_quote.return_value = DocumentResult( + success=True, external_id=str(quote.xero_id) + ) + manager = _manager(company, job, office_staff, provider) + + response = manager.delete_document() + + assert response["success"] is True + assert Quote.objects.count() == 0 + def test_quote_absent_in_xero_still_cleans_up_locally( self, company: Company, job: Job, office_staff: Staff ) -> None: diff --git a/stubs/xero_python/accounting/__init__.pyi b/stubs/xero_python/accounting/__init__.pyi index 49b58091..588b3575 100644 --- a/stubs/xero_python/accounting/__init__.pyi +++ b/stubs/xero_python/accounting/__init__.pyi @@ -88,6 +88,7 @@ class Quote: branding_theme_id: str | None terms: str | None reference: str | None + validation_errors: list[ValidationError] | None updated_date_utc: Any def __init__(self, **kwargs: Any) -> None: ... def to_dict(self) -> dict[str, Any]: ... From 9cb519fd585af16ba0b21422b3cd3ad2c26069de Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 9 Aug 2026 20:33:22 +1200 Subject: [PATCH 2/5] One document-endpoint adapter: auth refusal, failure map, success build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/xero/api.py | 275 +++++++++++++++++------------------------------ 1 file changed, 96 insertions(+), 179 deletions(-) diff --git a/apps/xero/api.py b/apps/xero/api.py index 219cf08b..9a5b3cf6 100644 --- a/apps/xero/api.py +++ b/apps/xero/api.py @@ -51,6 +51,10 @@ ) from apps.xero.auth import get_valid_token from apps.xero.constants import TENANT_ID_CACHE_KEY + +# The response TypedDict only: base pulls no sync machinery, unlike the +# manager modules whose imports stay call-time. +from apps.xero.documents.base import XeroDocumentResponse from apps.xero.models import XeroApp, XeroPayItem from apps.xero.sync_service import XeroSyncService @@ -243,15 +247,9 @@ def xero_branding_themes_list( request: HttpRequest, ) -> Status[list[XeroBrandingThemeOut] | XeroAuthRequiredOut]: """Return the selectable document themes from the connected Xero organisation.""" - if not get_valid_token(): - return Status( - 401, - XeroAuthRequiredOut( - success=False, - redirect_to_auth=True, - message="Your Xero session has expired. Please log in again.", - ), - ) + auth_refusal = _xero_auth_refusal() + if auth_refusal is not None: + return Status(401, auth_refusal) themes = get_provider().list_document_themes() return Status( 200, @@ -326,6 +324,65 @@ def _decimal_or_none(value: str | None) -> Decimal | None: return Decimal(value) +# The endpoints declare this union; the helpers must too — Status is +# invariant in its payload parameter, so Status[Error] alone would not be +# assignable to the endpoint's declared return. +DocumentPushStatus = Status[ + XeroDocumentSuccessResponse | XeroDocumentErrorResponse | XeroAuthRequiredOut +] + + +def _xero_auth_refusal() -> XeroAuthRequiredOut | None: + """Return the 401 payload when no valid token exists; None when connected.""" + if get_valid_token(): + return None + return XeroAuthRequiredOut( + success=False, + redirect_to_auth=True, + message="Your Xero session has expired. Please log in again.", + ) + + +def _document_failure(result: "XeroDocumentResponse", fallback: str) -> DocumentPushStatus: + """Map a manager failure to the declared error payload (ADR 0038).""" + return Status( + _document_error_status(result.get("status")), + XeroDocumentErrorResponse( + success=False, + error=result.get("error") or fallback, + error_type=result.get("error_type"), + messages=result.get("messages"), + ), + ) + + +def _document_success(result: "XeroDocumentResponse", status: int) -> DocumentPushStatus: + """Build the success payload; success without a xero_id is a raise. + + Passing every field is wire-identical to the per-endpoint hand-builds + this replaced: ninja serializes all declared fields, so an absent key + and an explicit None both travel as null. + """ + xero_id = result.get("xero_id") + if not xero_id: + raise ValueError(f"Document manager reported success without a xero_id: {result}") + return Status( + status, + XeroDocumentSuccessResponse( + success=True, + xero_id=xero_id, + invoice_id=result.get("invoice_id"), + quote_id=result.get("quote_id"), + company=result.get("company"), + total_excl_tax=_decimal_or_none(result.get("total_excl_tax")), + total_incl_tax=_decimal_or_none(result.get("total_incl_tax")), + online_url=result.get("online_url"), + message=result.get("message"), + messages=result.get("messages"), + ), + ) + + def _document_error_status(status: object) -> int: """Clamp a manager-reported failure status to the declared response map. @@ -367,15 +424,9 @@ def xero_create_invoice( # sync engine into every request that touches this router. from apps.xero.documents.invoice import XeroInvoiceManager # noqa: PLC0415 - if not get_valid_token(): - return Status( - 401, - XeroAuthRequiredOut( - success=False, - redirect_to_auth=True, - message="Your Xero session has expired. Please log in again.", - ), - ) + auth_refusal = _xero_auth_refusal() + if auth_refusal is not None: + return Status(401, auth_refusal) try: job = get_job_for_invoice_calculation(job_id) @@ -422,31 +473,8 @@ def xero_create_invoice( ) if not result["success"]: - return Status( - _document_error_status(result.get("status")), - XeroDocumentErrorResponse( - success=False, - error=result.get("error") or "Invoice creation failed.", - error_type=result.get("error_type"), - messages=result.get("messages"), - ), - ) - xero_id = result.get("xero_id") - if not xero_id: - raise ValueError(f"Invoice manager reported success without a xero_id: {result}") - return Status( - 201, - XeroDocumentSuccessResponse( - success=True, - xero_id=xero_id, - invoice_id=result.get("invoice_id"), - company=result.get("company"), - total_excl_tax=_decimal_or_none(result.get("total_excl_tax")), - total_incl_tax=_decimal_or_none(result.get("total_incl_tax")), - online_url=result.get("online_url"), - messages=result.get("messages"), - ), - ) + return _document_failure(result, "Invoice creation failed.") + return _document_success(result, 201) @router.post( @@ -476,15 +504,9 @@ def xero_create_quote( # sync engine into every request that touches this router. from apps.xero.documents.quote import XeroQuoteManager # noqa: PLC0415 - if not get_valid_token(): - return Status( - 401, - XeroAuthRequiredOut( - success=False, - redirect_to_auth=True, - message="Your Xero session has expired. Please log in again.", - ), - ) + auth_refusal = _xero_auth_refusal() + if auth_refusal is not None: + return Status(401, auth_refusal) try: job = Job.objects.select_related("company").get(id=job_id) @@ -507,31 +529,8 @@ def xero_create_quote( result = manager.create_document(breakdown=payload.breakdown) if not result["success"]: - return Status( - _document_error_status(result.get("status")), - XeroDocumentErrorResponse( - success=False, - error=result.get("error") or "Quote creation failed.", - error_type=result.get("error_type"), - messages=result.get("messages"), - ), - ) - xero_id = result.get("xero_id") - if not xero_id: - raise ValueError(f"Quote manager reported success without a xero_id: {result}") - return Status( - 201, - XeroDocumentSuccessResponse( - success=True, - xero_id=xero_id, - quote_id=result.get("quote_id"), - company=result.get("company"), - total_excl_tax=_decimal_or_none(result.get("total_excl_tax")), - total_incl_tax=_decimal_or_none(result.get("total_incl_tax")), - online_url=result.get("online_url"), - messages=result.get("messages"), - ), - ) + return _document_failure(result, "Quote creation failed.") + return _document_success(result, 201) @router.delete( @@ -557,15 +556,9 @@ def xero_delete_quote( """ from apps.xero.documents.quote import XeroQuoteManager # noqa: PLC0415 - if not get_valid_token(): - return Status( - 401, - XeroAuthRequiredOut( - success=False, - redirect_to_auth=True, - message="Your Xero session has expired. Please log in again.", - ), - ) + auth_refusal = _xero_auth_refusal() + if auth_refusal is not None: + return Status(401, auth_refusal) try: job = Job.objects.select_related("company").get(id=job_id) @@ -592,23 +585,8 @@ def xero_delete_quote( result = manager.delete_document() if not result["success"]: - return Status( - _document_error_status(result.get("status")), - XeroDocumentErrorResponse( - success=False, - error=result.get("error") or "Quote deletion failed.", - error_type=result.get("error_type"), - ), - ) - deleted_xero_id = result.get("xero_id") - if not deleted_xero_id: - raise ValueError(f"Quote manager reported success without a xero_id: {result}") - return Status( - 200, - XeroDocumentSuccessResponse( - success=True, xero_id=deleted_xero_id, message=result.get("message") - ), - ) + return _document_failure(result, "Quote deletion failed.") + return _document_success(result, 200) @router.delete( @@ -634,15 +612,9 @@ def xero_delete_invoice( """ from apps.xero.documents.invoice import XeroInvoiceManager # noqa: PLC0415 - if not get_valid_token(): - return Status( - 401, - XeroAuthRequiredOut( - success=False, - redirect_to_auth=True, - message="Your Xero session has expired. Please log in again.", - ), - ) + auth_refusal = _xero_auth_refusal() + if auth_refusal is not None: + return Status(401, auth_refusal) try: job = Job.objects.select_related("company").get(id=job_id) @@ -680,23 +652,8 @@ def xero_delete_invoice( result = manager.delete_document() if not result["success"]: - return Status( - _document_error_status(result.get("status")), - XeroDocumentErrorResponse( - success=False, - error=result.get("error") or "Invoice deletion failed.", - error_type=result.get("error_type"), - ), - ) - deleted_xero_id = result.get("xero_id") - if not deleted_xero_id: - raise ValueError(f"Invoice manager reported success without a xero_id: {result}") - return Status( - 200, - XeroDocumentSuccessResponse( - success=True, xero_id=deleted_xero_id, message=result.get("message") - ), - ) + return _document_failure(result, "Invoice deletion failed.") + return _document_success(result, 200) # --- Purchase-order push --- @@ -726,15 +683,9 @@ def xero_create_purchase_order( """ from apps.xero.documents.po import XeroPurchaseOrderManager # noqa: PLC0415 - if not get_valid_token(): - return Status( - 401, - XeroAuthRequiredOut( - success=False, - redirect_to_auth=True, - message="Your Xero session has expired. Please log in again.", - ), - ) + auth_refusal = _xero_auth_refusal() + if auth_refusal is not None: + return Status(401, auth_refusal) try: purchase_order = PurchaseOrder.objects.select_related("supplier").get(id=purchase_order_id) @@ -759,23 +710,8 @@ def xero_create_purchase_order( result = manager.sync_to_xero() if not result["success"]: - return Status( - _document_error_status(result.get("status")), - XeroDocumentErrorResponse( - success=False, - error=result.get("error") or "Purchase order sync failed.", - error_type=result.get("error_type"), - ), - ) - synced_xero_id = result.get("xero_id") - if not synced_xero_id: - raise ValueError(f"PO manager reported success without a xero_id: {result}") - return Status( - 200, - XeroDocumentSuccessResponse( - success=True, xero_id=synced_xero_id, online_url=result.get("online_url") - ), - ) + return _document_failure(result, "Purchase order sync failed.") + return _document_success(result, 200) @router.delete( @@ -797,15 +733,9 @@ def xero_delete_purchase_order( """Void the PO in Xero; locally the row survives with status deleted.""" from apps.xero.documents.po import XeroPurchaseOrderManager # noqa: PLC0415 - if not get_valid_token(): - return Status( - 401, - XeroAuthRequiredOut( - success=False, - redirect_to_auth=True, - message="Your Xero session has expired. Please log in again.", - ), - ) + auth_refusal = _xero_auth_refusal() + if auth_refusal is not None: + return Status(401, auth_refusal) try: purchase_order = PurchaseOrder.objects.select_related("supplier").get(id=purchase_order_id) @@ -830,21 +760,8 @@ def xero_delete_purchase_order( result = manager.delete_document() if not result["success"]: - return Status( - _document_error_status(result.get("status")), - XeroDocumentErrorResponse( - success=False, error=result.get("error") or "Purchase order deletion failed." - ), - ) - deleted_po_xero_id = result.get("xero_id") - if not deleted_po_xero_id: - raise ValueError(f"PO manager reported success without a xero_id: {result}") - return Status( - 200, - XeroDocumentSuccessResponse( - success=True, xero_id=deleted_po_xero_id, message=result.get("message") - ), - ) + return _document_failure(result, "Purchase order deletion failed.") + return _document_success(result, 200) # --- Sync trigger + status --- From 7f936094137db0d1c1bd925cb05a9a1b200b53ff Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 9 Aug 2026 20:40:12 +1200 Subject: [PATCH 3/5] Grid hardening F1-F6 + assertive spec: drafts persist, retries retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../job/costing/CostLineGrid.test.tsx | 63 +++++++++++++++--- .../src/features/job/costing/CostLineGrid.tsx | 40 +++++++++--- .../features/job/costing/useAutosaveField.ts | 51 +++++++++------ .../features/job/costing/useCostLines.test.ts | 65 +++++++++++++++++++ .../src/features/job/costing/useCostLines.ts | 56 +++++++++++++++- 5 files changed, 234 insertions(+), 41 deletions(-) create mode 100644 frontend/src/features/job/costing/useCostLines.test.ts diff --git a/frontend/src/features/job/costing/CostLineGrid.test.tsx b/frontend/src/features/job/costing/CostLineGrid.test.tsx index 83f8bc6d..8d414bd6 100644 --- a/frontend/src/features/job/costing/CostLineGrid.test.tsx +++ b/frontend/src/features/job/costing/CostLineGrid.test.tsx @@ -397,27 +397,24 @@ describe('CostLineGrid contract', () => { const phantom = rows[1]! await user.type(within(phantom).getByRole('textbox'), 'Bracket') + // Committing the cost derives unit_rev, completing the draft: POST #1. await user.type( document.querySelector( '[data-automation-id="SmartCostLinesTable-unit-cost-1"]', )!, '10', ) - const rev = document.querySelector( - '[data-automation-id="SmartCostLinesTable-unit-rev-1"]', - )! - await user.clear(rev) - await user.type(rev, '12') await user.tab() await waitFor(() => expect(attempts).toBe(1)) await waitFor(() => expect(document.querySelector('[data-sonner-toast]')).not.toBeNull()) - // The draft survives; re-committing a field retries the POST. + // The draft survives; RETYPING the SAME value retries the POST — the + // dedupe belongs to server PATCHes, not draft commits. const revRetry = document.querySelector( '[data-automation-id="SmartCostLinesTable-unit-rev-1"]', )! await user.clear(revRetry) - await user.type(revRetry, '13') + await user.type(revRetry, '12.00') await user.tab() await waitFor(() => expect(attempts).toBe(2)) @@ -456,6 +453,54 @@ describe('CostLineGrid contract', () => { await waitFor(() => expect(patches).toBe(2)) }) + it('derives draft unit_rev from unit_cost so a filled phantom persists', async () => { + const created: unknown[] = [] + server.use( + http.get('*/api/job/jobs/*/cost_sets/quote/', () => HttpResponse.json(costSet([]))), + http.get('*/api/job/jobs/*/labour-rates/', () => HttpResponse.json(labourRates)), + http.post('*/api/job/jobs/*/cost_sets/quote/cost_lines/', async ({ request }) => { + created.push(await request.json()) + return HttpResponse.json({ ...materialLine, id: 'line-derived' }, { status: 201 }) + }), + ) + const user = userEvent.setup() + renderGrid() + const rows = await findRows() + + // Only desc + unit cost typed; unit_rev must derive via the markup like + // a server row's cost edit does, or the draft silently never persists. + await user.type(within(rows[0]!).getByRole('textbox'), 'Freight') + const cost = document.querySelector( + '[data-automation-id="SmartCostLinesTable-unit-cost-0"]', + )! + await user.type(cost, '10') + await user.tab() + + await waitFor(() => expect(created).toHaveLength(1)) + expect(created[0]).toMatchObject({ unit_cost: '10', unit_rev: '12.00' }) + }) + + it('a quantity-only edit makes the phantom a real draft', async () => { + stubGridData([materialLine]) + const user = userEvent.setup() + renderGrid() + const rows = await findRows() + expect(rows).toHaveLength(2) + + const quantity = document.querySelector( + '[data-automation-id="SmartCostLinesTable-quantity-1"]', + )! + await user.clear(quantity) + await user.type(quantity, '5') + await user.tab() + + // The edited row is no longer the empty phantom: a fresh one trails it. + await waitFor(() => { + const table = screen.getByRole('table') + expect(within(table).getAllByRole('row').slice(1)).toHaveLength(3) + }) + }) + it('promotes the phantom row to a POSTed line and appends a fresh phantom', async () => { const created: unknown[] = [] const newLine: CostLineOut = { @@ -496,7 +541,9 @@ describe('CostLineGrid contract', () => { await user.tab() await waitFor(() => expect(created).toHaveLength(1)) - expect(created[0]).toMatchObject({ desc: 'Bracket', kind: 'material' }) + // A typed free-form row is an adjustment (v1 rule); material requires a + // stock pick, time a labour pick. + expect(created[0]).toMatchObject({ desc: 'Bracket', kind: 'adjust' }) // The new server row lands and one fresh empty phantom trails it. await waitFor(async () => { diff --git a/frontend/src/features/job/costing/CostLineGrid.tsx b/frontend/src/features/job/costing/CostLineGrid.tsx index e92535e6..60057d8e 100644 --- a/frontend/src/features/job/costing/CostLineGrid.tsx +++ b/frontend/src/features/job/costing/CostLineGrid.tsx @@ -42,6 +42,7 @@ function freshPhantom(): DraftRow { function draftIsEmpty(draft: DraftLine): boolean { return ( draft.desc.trim() === '' && + draft.quantity === '1' && draft.unit_cost === null && draft.unit_rev === null && draft.labour_subtype === null && @@ -307,15 +308,26 @@ function DescCell({ row, table }: CellProps) { const context = cellMeta(table) const gridRow = row.original const serverValue = gridRow.type === 'server' ? (gridRow.line.desc ?? '') : gridRow.draft.desc - const field = useAutosaveField(serverValue, (value) => { - if (gridRow.type === 'server') { - // ADR 0040: blank clears to null, never an empty string. - context.patchLine(gridRow.line.id, { desc: value.trim() === '' ? null : value }) - } else { - context.updateDraft(gridRow.localId, { desc: value }) - context.commitDraftField(gridRow.localId) - } - }) + const field = useAutosaveField( + serverValue, + (value) => { + if (gridRow.type === 'server') { + // ADR 0040: blank clears to null, never an empty string. + context.patchLine(gridRow.line.id, { desc: value.trim() === '' ? null : value }) + } else { + const patch: Partial = { desc: value } + // A typed free-form row is an adjustment (v1 rule): material means + // a stock pick, time a labour pick — both set kind themselves. + if (!('stock_id' in gridRow.draft.ext_refs) && gridRow.draft.labour_subtype === null) { + patch.kind = 'adjust' + } + context.updateDraft(gridRow.localId, patch) + context.commitDraftField(gridRow.localId) + } + }, + undefined, + gridRow.type === 'server', + ) return (