From d3825840527e46c67b4f4ad9e08041777d7cba9e Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sat, 1 Aug 2026 09:26:11 +1200 Subject: [PATCH 1/8] feat: add CompanyDefaults.gst_rate as the single sales-tax rate DocketWorks needs a sales-tax rate to quote amounts that Xero has not yet invoiced (the Finish Job remaining balance). The only rate in the codebase was a cosmetic constant in the read-only Xero provider, which meant no authoritative source and two places to change. gst_rate is a per-client tunable on CompanyDefaults (0.1500 = 15% NZ GST) and the read-only provider now reads it, so the system has one rate. The dynamic settings UI picks the field up from COMPANY_DEFAULTS_FIELD_SECTIONS, so it is editable without a hand-written input. KAN-323 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH --- .../accounting/xero/readonly_provider.py | 5 ++-- .../0019_company_defaults_gst_rate.py | 25 ++++++++++++++++ apps/workflow/models/company_defaults.py | 10 +++++++ apps/workflow/models/settings_metadata.py | 1 + frontend/schema.yml | 30 +++++++++++++++++++ frontend/src/api/generated/api.ts | 3 ++ 6 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 apps/workflow/migrations/0019_company_defaults_gst_rate.py diff --git a/apps/workflow/accounting/xero/readonly_provider.py b/apps/workflow/accounting/xero/readonly_provider.py index 82485f537..cb368174c 100644 --- a/apps/workflow/accounting/xero/readonly_provider.py +++ b/apps/workflow/accounting/xero/readonly_provider.py @@ -34,7 +34,6 @@ logger = logging.getLogger("xero") -_GST_RATE = Decimal("0.15") _CENT = Decimal("0.01") @@ -48,10 +47,12 @@ def _log_suppressed(operation: str, detail: str) -> None: def _fake_totals(line_items: list[DocumentLineItem]) -> tuple[str, str, str]: """Cosmetic GST-exclusive totals for stubbed documents (local display only).""" + from apps.workflow.models import CompanyDefaults + sub_total = sum( (li.quantity * li.unit_amount for li in line_items), Decimal("0") ).quantize(_CENT) - tax = (sub_total * _GST_RATE).quantize(_CENT) + tax = (sub_total * CompanyDefaults.get_solo().gst_rate).quantize(_CENT) total = sub_total + tax return str(sub_total), str(tax), str(total) diff --git a/apps/workflow/migrations/0019_company_defaults_gst_rate.py b/apps/workflow/migrations/0019_company_defaults_gst_rate.py new file mode 100644 index 000000000..f9cec3ce4 --- /dev/null +++ b/apps/workflow/migrations/0019_company_defaults_gst_rate.py @@ -0,0 +1,25 @@ +# Generated by Django 6.0.7 on 2026-07-31 21:21 + +from decimal import Decimal + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("workflow", "0018_text_unset_constraints"), + ] + + operations = [ + migrations.AddField( + model_name="companydefaults", + name="gst_rate", + field=models.DecimalField( + decimal_places=4, + default=Decimal("0.1500"), + help_text="Sales tax rate applied to amounts DocketWorks quotes before Xero has issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero remains authoritative for tax on invoices that exist.", + max_digits=5, + ), + ), + ] diff --git a/apps/workflow/models/company_defaults.py b/apps/workflow/models/company_defaults.py index 9fc58ce54..b2fcb10a8 100644 --- a/apps/workflow/models/company_defaults.py +++ b/apps/workflow/models/company_defaults.py @@ -20,6 +20,16 @@ class CompanyDefaults(SingletonModel): ) time_markup = models.DecimalField(max_digits=5, decimal_places=2, default=0.3) materials_markup = models.DecimalField(max_digits=5, decimal_places=2, default=0.2) + gst_rate = models.DecimalField( + max_digits=5, + decimal_places=4, + default=Decimal("0.1500"), + help_text=( + "Sales tax rate applied to amounts DocketWorks quotes before Xero has " + "issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero remains " + "authoritative for tax on invoices that exist." + ), + ) wage_rate = models.DecimalField( max_digits=6, decimal_places=2, default=32.00 ) # rate per hour diff --git a/apps/workflow/models/settings_metadata.py b/apps/workflow/models/settings_metadata.py index 15b85b593..79e49bf4e 100644 --- a/apps/workflow/models/settings_metadata.py +++ b/apps/workflow/models/settings_metadata.py @@ -111,6 +111,7 @@ def get_section_info(cls, key: str) -> tuple[str, str, int] | None: # Finances "time_markup": "finances", "materials_markup": "finances", + "gst_rate": "finances", "wage_rate": "finances", "annual_leave_loading": "finances", "financial_year_start_month": "finances", diff --git a/frontend/schema.yml b/frontend/schema.yml index 94c814cf9..830a830de 100644 --- a/frontend/schema.yml +++ b/frontend/schema.yml @@ -10796,6 +10796,16 @@ components: minimum: -1000 exclusiveMaximum: true exclusiveMinimum: true + gst_rate: + type: number + format: double + maximum: 10 + minimum: -10 + exclusiveMaximum: true + exclusiveMinimum: true + description: Sales tax rate applied to amounts DocketWorks quotes before + Xero has issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero + remains authoritative for tax on invoices that exist. wage_rate: type: number format: double @@ -11184,6 +11194,16 @@ components: minimum: -1000 exclusiveMaximum: true exclusiveMinimum: true + gst_rate: + type: number + format: double + maximum: 10 + minimum: -10 + exclusiveMaximum: true + exclusiveMinimum: true + description: Sales tax rate applied to amounts DocketWorks quotes before + Xero has issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero + remains authoritative for tax on invoices that exist. wage_rate: type: number format: double @@ -17100,6 +17120,16 @@ components: minimum: -1000 exclusiveMaximum: true exclusiveMinimum: true + gst_rate: + type: number + format: double + maximum: 10 + minimum: -10 + exclusiveMaximum: true + exclusiveMinimum: true + description: Sales tax rate applied to amounts DocketWorks quotes before + Xero has issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero + remains authoritative for tax on invoices that exist. wage_rate: type: number format: double diff --git a/frontend/src/api/generated/api.ts b/frontend/src/api/generated/api.ts index 2f53ab219..ed8ad379e 100644 --- a/frontend/src/api/generated/api.ts +++ b/frontend/src/api/generated/api.ts @@ -896,6 +896,7 @@ const CompanyDefaults = z.object({ company_acronym: z.string().max(10).nullish(), time_markup: z.number().gt(-1000).lt(1000).optional(), materials_markup: z.number().gt(-1000).lt(1000).optional(), + gst_rate: z.number().gt(-10).lt(10).optional(), wage_rate: z.number().gt(-10000).lt(10000).optional(), annual_leave_loading: z.number().gt(-1000).lt(1000).optional(), workshop_efficiency_factor: z.number().gt(-10).lt(10).optional(), @@ -962,6 +963,7 @@ const CompanyDefaultsRequest = z.object({ company_acronym: z.string().min(1).max(10).nullish(), time_markup: z.number().gt(-1000).lt(1000).optional(), materials_markup: z.number().gt(-1000).lt(1000).optional(), + gst_rate: z.number().gt(-10).lt(10).optional(), wage_rate: z.number().gt(-10000).lt(10000).optional(), annual_leave_loading: z.number().gt(-1000).lt(1000).optional(), workshop_efficiency_factor: z.number().gt(-10).lt(10).optional(), @@ -1027,6 +1029,7 @@ const PatchedCompanyDefaultsRequest = z company_acronym: z.string().min(1).max(10).nullable(), time_markup: z.number().gt(-1000).lt(1000), materials_markup: z.number().gt(-1000).lt(1000), + gst_rate: z.number().gt(-10).lt(10), wage_rate: z.number().gt(-10000).lt(10000), annual_leave_loading: z.number().gt(-1000).lt(1000), workshop_efficiency_factor: z.number().gt(-10).lt(10), From d105de8f3d25a6a3bdc6cba1345fce72933cb613 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sat, 1 Aug 2026 09:49:10 +1200 Subject: [PATCH 2/8] feat: add backend-owned Finish Job balance and audited completion checklist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The counter question — what does this customer owe, including GST — had no authoritative answer. JobActualTab computed "to be invoiced" in the browser from a quote total and an invoice list, which is a business calculation in the presentation layer (ADR 0020) and silently ignored what Xero still expects to be paid. The summary service answers it server-side: job value on its own pricing basis, valid invoiced, outstanding Xero balances, the positive remaining amount, GST on it, and Total to pay. Over-invoicing is reported as a separate diagnostic rather than netted off, so a T&M job whose actuals came in under its invoices shows the excess instead of hiding it behind a smaller balance. The job-value basis, price cap and valid-invoice statuses are extracted from invoice_calculation rather than reimplemented, so the amount a user can invoice and the amount they are shown cannot drift apart. Only the basis cost set is loaded, with its cost lines prefetched: the n+1 guard caught both a query-per-cost-line and, once fixed, an eager load of the cost set the summary never reads. A query-count test pins it. The completion checklist is deliberately advisory — tests pin that confirming items changes neither job status nor invoice eligibility. Each changed item writes one job-history event, including withdrawals, which is the change most worth finding later. Also drops .__func__ from the JobEvent description registry: staticmethod objects have been directly callable since 3.10, and removing it clears 23 baselined mypy errors instead of adding a 24th. KAN-323 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH --- apps/accounting/services/__init__.py | 14 + .../accounting/services/finish_job_summary.py | 108 ++++++ .../services/invoice_calculation.py | 52 ++- .../tests/test_finish_job_summary.py | 279 +++++++++++++++ .../0010_job_completion_checklist.py | 66 ++++ apps/job/models/__init__.py | 2 + apps/job/models/job_completion_checklist.py | 64 ++++ apps/job/models/job_event.py | 72 ++-- apps/job/serializers/__init__.py | 10 + apps/job/serializers/job_serializer.py | 87 ++++- apps/job/services/__init__.py | 8 + .../job_completion_checklist_service.py | 97 ++++++ apps/job/tests/test_completion_checklist.py | 320 ++++++++++++++++++ apps/job/urls_rest.py | 7 + apps/job/views/__init__.py | 2 + apps/job/views/job_rest_views.py | 89 +++++ docs/urls/job.md | 1 + frontend/schema.yml | 216 ++++++++++++ frontend/src/api/generated/api.ts | 80 +++++ mypy-baseline.txt | 23 -- 20 files changed, 1538 insertions(+), 59 deletions(-) create mode 100644 apps/accounting/services/finish_job_summary.py create mode 100644 apps/accounting/tests/test_finish_job_summary.py create mode 100644 apps/job/migrations/0010_job_completion_checklist.py create mode 100644 apps/job/models/job_completion_checklist.py create mode 100644 apps/job/services/job_completion_checklist_service.py create mode 100644 apps/job/tests/test_completion_checklist.py diff --git a/apps/accounting/services/__init__.py b/apps/accounting/services/__init__.py index bc430acd7..c983eb20c 100644 --- a/apps/accounting/services/__init__.py +++ b/apps/accounting/services/__init__.py @@ -6,11 +6,19 @@ if apps.ready: from .core import JobAgingService, KPIService, StaffPerformanceService + from .finish_job_summary import ( + FinishJobSummary, + build_finish_job_summary, + get_job_for_finish_summary, + get_outstanding_invoiced_incl_tax, + ) from .invoice_calculation import ( InvoiceCalculationError, InvoiceCalculationResult, calculate_invoice_amount, get_job_for_invoice_calculation, + get_job_value_basis, + get_job_value_excl_tax, get_prior_valid_invoice_total, ) from .payroll_reconciliation_service import PayrollReconciliationService @@ -22,6 +30,7 @@ pass __all__ = [ + "FinishJobSummary", "InvoiceCalculationError", "InvoiceCalculationResult", "JobAgingService", @@ -31,7 +40,12 @@ "SalesPipelineService", "StaffPerformanceService", "WIPService", + "build_finish_job_summary", "calculate_invoice_amount", + "get_job_for_finish_summary", "get_job_for_invoice_calculation", + "get_job_value_basis", + "get_job_value_excl_tax", + "get_outstanding_invoiced_incl_tax", "get_prior_valid_invoice_total", ] diff --git a/apps/accounting/services/finish_job_summary.py b/apps/accounting/services/finish_job_summary.py new file mode 100644 index 000000000..ed0fc12a1 --- /dev/null +++ b/apps/accounting/services/finish_job_summary.py @@ -0,0 +1,108 @@ +"""Authoritative customer balance for the Finish Job workspace. + +Answers the counter question — what does this customer need to pay, including +tax — as a single set of decimal currency values. Every figure is computed here +so the frontend only formats what it is given (ADR 0020). + +The job value, price-cap behaviour and valid-invoice statuses come from +``invoice_calculation``; this module adds only the tax and outstanding-balance +arithmetic on top of them, so a change to the invoicing basis moves both the +invoice a user can create and the balance they are shown. +""" + +from dataclasses import dataclass +from decimal import ROUND_HALF_UP, Decimal +from uuid import UUID + +from django.db.models import Sum, prefetch_related_objects +from django.db.models.functions import Coalesce + +from apps.accounting.models.invoice import Invoice +from apps.accounting.services.invoice_calculation import ( + INVOICE_VALID_STATUSES, + get_job_value_basis, + get_job_value_excl_tax, + get_prior_valid_invoice_total, +) +from apps.job.models import Job +from apps.workflow.models import CompanyDefaults + +CENT = Decimal("0.01") + + +@dataclass(frozen=True) +class FinishJobSummary: + """Every currency value the Finish Job workspace displays. + + ``basis`` names the cost set the job value is measured against ("quote" for + fixed-price work, "actual_revenue" for T&M) so the UI can label the figure + without inferring it from the pricing methodology. + """ + + basis: str + job_value_excl_gst: Decimal + valid_invoiced_excl_gst: Decimal + outstanding_invoiced_incl_gst: Decimal + remaining_to_invoice_excl_gst: Decimal + remaining_gst: Decimal + remaining_to_invoice_incl_gst: Decimal + total_to_pay_incl_gst: Decimal + over_invoiced_excl_gst: Decimal + + +def get_outstanding_invoiced_incl_tax(job: Job) -> Decimal: + """What Xero still expects to be paid on this job's valid invoices.""" + return Decimal( + Invoice.objects.filter( + job_id=job.id, status__in=INVOICE_VALID_STATUSES + ).aggregate(total=Coalesce(Sum("amount_due"), Decimal("0")))["total"] + ) + + +def get_job_for_finish_summary(job_id: UUID) -> Job: + """Load a job with the cost set its value is measured against. + + Only the basis cost set is loaded, and its cost lines are prefetched because + the job value sums them. Fetching both cost sets would eagerly load one the + summary never reads; fetching neither would cost a query per cost line. + """ + job = Job.objects.get(id=job_id) + + if job.pricing_methodology == "fixed_price": + prefetch_related_objects([job], "latest_quote__cost_lines") + else: + prefetch_related_objects([job], "latest_actual__cost_lines") + + return job + + +def build_finish_job_summary(job: Job) -> FinishJobSummary: + job_value = _as_currency(get_job_value_excl_tax(job)) + invoiced = _as_currency(get_prior_valid_invoice_total(job)) + outstanding = _as_currency(get_outstanding_invoiced_incl_tax(job)) + + # A job invoiced beyond its value has nothing left to invoice; the excess is + # reported separately for resolution in Xero rather than netted off, which + # would hide it behind a smaller remaining balance. + remaining_excl = max(job_value - invoiced, Decimal("0")) + over_invoiced = max(invoiced - job_value, Decimal("0")) + + gst_rate = CompanyDefaults.get_solo().gst_rate + remaining_gst = (remaining_excl * gst_rate).quantize(CENT, rounding=ROUND_HALF_UP) + remaining_incl = remaining_excl + remaining_gst + + return FinishJobSummary( + basis=get_job_value_basis(job), + job_value_excl_gst=job_value, + valid_invoiced_excl_gst=invoiced, + outstanding_invoiced_incl_gst=outstanding, + remaining_to_invoice_excl_gst=remaining_excl, + remaining_gst=remaining_gst, + remaining_to_invoice_incl_gst=remaining_incl, + total_to_pay_incl_gst=outstanding + remaining_incl, + over_invoiced_excl_gst=over_invoiced, + ) + + +def _as_currency(amount: Decimal) -> Decimal: + return amount.quantize(CENT, rounding=ROUND_HALF_UP) diff --git a/apps/accounting/services/invoice_calculation.py b/apps/accounting/services/invoice_calculation.py index ac783abf0..911a1f2e1 100644 --- a/apps/accounting/services/invoice_calculation.py +++ b/apps/accounting/services/invoice_calculation.py @@ -31,6 +31,40 @@ class InvoiceCalculationResult: calculated_amount: Decimal = Decimal("0") +def get_job_value_basis(job: Job) -> str: + """Name of the cost set a job's complete value is measured against.""" + if job.pricing_methodology == "fixed_price": + return "quote" + else: + return "actual_revenue" + + +def get_job_value_excl_tax(job: Job) -> Decimal: + """The complete value of a job excluding tax, on its own pricing basis. + + Fixed-price work is worth its quote; T&M work is worth its actual revenue, + limited by the price cap when one is set. A job whose basis cost set does not + exist yet is worth nothing — it has not been quoted, and no work has been + recorded against it. Invoicing such a job is a separate question, guarded by + the calculation paths below. + """ + if job.pricing_methodology == "fixed_price": + quote = job.latest_quote + if not quote: + return Decimal("0") + return Decimal(str(quote.total_revenue)) + + actual = job.latest_actual + if not actual: + return Decimal("0") + + target_total = Decimal(str(actual.total_revenue)) + if job.price_cap is not None: + return min(target_total, Decimal(str(job.price_cap))) + else: + return target_total + + def get_prior_valid_invoice_total(job: Job) -> Decimal: return Decimal( Invoice.objects.filter( @@ -73,13 +107,12 @@ def _calculate_fixed_price( percent: Decimal | None, amount: Decimal | None, ) -> InvoiceCalculationResult: - quote = job.latest_quote - if not quote: + if not job.latest_quote: raise InvoiceCalculationError( "Fixed-price job has no quote to invoice against." ) - target_total = Decimal(str(quote.total_revenue)) - target_basis = "quote" + target_total = get_job_value_excl_tax(job) + target_basis = get_job_value_basis(job) if mode == "invoice_full": calculated = target_total - prior_invoiced @@ -124,17 +157,12 @@ def _calculate_time_materials( percent: Decimal | None, amount: Decimal | None, ) -> InvoiceCalculationResult: - actual = job.latest_actual - if not actual: + if not job.latest_actual: raise InvoiceCalculationError( "T&M job has no actual cost set to invoice against." ) - target_total = Decimal(str(actual.total_revenue)) - - if job.price_cap is not None: - target_total = min(target_total, Decimal(str(job.price_cap))) - - target_basis = "actual_revenue" + target_total = get_job_value_excl_tax(job) + target_basis = get_job_value_basis(job) if mode == "invoice_costs_to_date": calculated = target_total - prior_invoiced diff --git a/apps/accounting/tests/test_finish_job_summary.py b/apps/accounting/tests/test_finish_job_summary.py new file mode 100644 index 000000000..24c960345 --- /dev/null +++ b/apps/accounting/tests/test_finish_job_summary.py @@ -0,0 +1,279 @@ +"""Tests for the Finish Job customer balance summary. + +Each test names a counter situation from KAN-323: what the customer must pay, +including GST, given what has already been invoiced and paid. +""" + +import uuid +from datetime import date +from decimal import Decimal + +from django.db import connection +from django.test.utils import CaptureQueriesContext +from django.utils import timezone + +from apps.accounting.models.invoice import Invoice +from apps.accounting.services.finish_job_summary import ( + build_finish_job_summary, + get_job_for_finish_summary, +) +from apps.company.models import Company +from apps.job.models import Job +from apps.job.models.costing import CostLine, CostSet +from apps.testing import BaseTestCase +from apps.workflow.models import CompanyDefaults + + +class TestFinishJobSummary(BaseTestCase): + def setUp(self) -> None: + self.client_obj = Company.objects.create( + name="Test Company", + xero_last_modified=timezone.now(), + ) + defaults = CompanyDefaults.get_solo() + defaults.gst_rate = Decimal("0.1500") + defaults.save() + + def _create_job(self, pricing_methodology: str = "time_materials") -> Job: + job = Job( + company=self.client_obj, + name="Test Job", + pricing_methodology=pricing_methodology, + ) + job.save(staff=self.test_staff) + return job + + def _add_revenue_line(self, cost_set: CostSet, revenue: Decimal) -> None: + CostLine.objects.create( + cost_set=cost_set, + kind="adjust", + desc="Test line", + quantity=Decimal("1.000"), + unit_cost=Decimal("0.00"), + unit_rev=revenue, + accounting_date=date.today(), + ) + + def _create_invoice( + self, + job: Job, + amount: Decimal, + amount_due: Decimal = Decimal("0.00"), + status: str = "AUTHORISED", + ) -> Invoice: + return Invoice.objects.create( + job=job, + company=self.client_obj, + xero_id=uuid.uuid4(), + number=f"INV-{uuid.uuid4().hex[:8]}", + status=status, + total_excl_tax=amount, + tax=(amount * Decimal("0.15")).quantize(Decimal("0.01")), + total_incl_tax=amount * Decimal("1.15"), + amount_due=amount_due, + date=date.today(), + xero_last_modified=timezone.now(), + raw_json={}, + ) + + # --- Job value basis --- + + def test_fixed_price_value_is_the_quote(self) -> None: + job = self._create_job("fixed_price") + self._add_revenue_line(job.latest_quote, Decimal("5000")) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.basis, "quote") + self.assertEqual(summary.job_value_excl_gst, Decimal("5000.00")) + + def test_time_materials_value_is_actual_revenue(self) -> None: + job = self._create_job("time_materials") + self._add_revenue_line(job.latest_actual, Decimal("1234.56")) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.basis, "actual_revenue") + self.assertEqual(summary.job_value_excl_gst, Decimal("1234.56")) + + def test_time_materials_value_is_limited_by_price_cap(self) -> None: + job = self._create_job("time_materials") + self._add_revenue_line(job.latest_actual, Decimal("5000")) + job.price_cap = Decimal("3000.00") + job.save(staff=self.test_staff) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.job_value_excl_gst, Decimal("3000.00")) + self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("3000.00")) + + def test_fixed_price_without_a_quote_is_worth_nothing_yet(self) -> None: + """A job created but not yet quoted must still render a balance.""" + job = self._create_job("fixed_price") + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.job_value_excl_gst, Decimal("0.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("0.00")) + + # --- Counter situations --- + + def test_nothing_invoiced_charges_the_whole_job_plus_gst(self) -> None: + job = self._create_job("fixed_price") + self._add_revenue_line(job.latest_quote, Decimal("1000")) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.valid_invoiced_excl_gst, Decimal("0.00")) + self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("1000.00")) + self.assertEqual(summary.remaining_gst, Decimal("150.00")) + self.assertEqual(summary.remaining_to_invoice_incl_gst, Decimal("1150.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) + + def test_paid_advance_invoice_leaves_nothing_to_pay(self) -> None: + """Invoiced in full before work started, and the customer has paid.""" + job = self._create_job("fixed_price") + self._add_revenue_line(job.latest_quote, Decimal("1000")) + self._create_invoice( + job, Decimal("1000"), amount_due=Decimal("0.00"), status="PAID" + ) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("0.00")) + self.assertEqual(summary.remaining_gst, Decimal("0.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("0.00")) + self.assertEqual(summary.over_invoiced_excl_gst, Decimal("0.00")) + + def test_unpaid_advance_invoice_is_the_amount_to_pay(self) -> None: + job = self._create_job("fixed_price") + self._add_revenue_line(job.latest_quote, Decimal("1000")) + self._create_invoice(job, Decimal("1000"), amount_due=Decimal("1150.00")) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("0.00")) + self.assertEqual(summary.outstanding_invoiced_incl_gst, Decimal("1150.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) + + def test_paid_deposit_leaves_only_the_uninvoiced_balance(self) -> None: + job = self._create_job("fixed_price") + self._add_revenue_line(job.latest_quote, Decimal("1000")) + self._create_invoice( + job, Decimal("400"), amount_due=Decimal("0.00"), status="PAID" + ) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.outstanding_invoiced_incl_gst, Decimal("0.00")) + self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("600.00")) + self.assertEqual(summary.remaining_gst, Decimal("90.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("690.00")) + + def test_unpaid_progress_invoice_and_balance_are_both_owed(self) -> None: + job = self._create_job("fixed_price") + self._add_revenue_line(job.latest_quote, Decimal("1000")) + self._create_invoice(job, Decimal("400"), amount_due=Decimal("460.00")) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.outstanding_invoiced_incl_gst, Decimal("460.00")) + self.assertEqual(summary.remaining_to_invoice_incl_gst, Decimal("690.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) + + def test_partly_paid_invoice_owes_only_its_outstanding_balance(self) -> None: + job = self._create_job("fixed_price") + self._add_revenue_line(job.latest_quote, Decimal("1000")) + self._create_invoice(job, Decimal("1000"), amount_due=Decimal("150.00")) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("150.00")) + + # --- Excluded invoices --- + + def test_voided_and_deleted_invoices_do_not_count(self) -> None: + job = self._create_job("fixed_price") + self._add_revenue_line(job.latest_quote, Decimal("1000")) + self._create_invoice( + job, Decimal("1000"), amount_due=Decimal("1150.00"), status="VOIDED" + ) + self._create_invoice( + job, Decimal("1000"), amount_due=Decimal("1150.00"), status="DELETED" + ) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.valid_invoiced_excl_gst, Decimal("0.00")) + self.assertEqual(summary.outstanding_invoiced_incl_gst, Decimal("0.00")) + self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("1000.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) + + # --- Over-invoicing --- + + def test_over_invoicing_is_reported_without_a_negative_remainder(self) -> None: + """T&M actuals can fall below what was already invoiced.""" + job = self._create_job("time_materials") + self._add_revenue_line(job.latest_actual, Decimal("800")) + self._create_invoice( + job, Decimal("1000"), amount_due=Decimal("0.00"), status="PAID" + ) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.over_invoiced_excl_gst, Decimal("200.00")) + self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("0.00")) + self.assertEqual(summary.remaining_gst, Decimal("0.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("0.00")) + + def test_over_invoiced_and_unpaid_still_owes_the_invoice(self) -> None: + job = self._create_job("time_materials") + self._add_revenue_line(job.latest_actual, Decimal("800")) + self._create_invoice(job, Decimal("1000"), amount_due=Decimal("1150.00")) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.over_invoiced_excl_gst, Decimal("200.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) + + # --- GST arithmetic --- + + def test_gst_rounds_to_the_nearest_cent(self) -> None: + """$333.33 x 15% = $49.9995, which must present as $50.00.""" + job = self._create_job("time_materials") + self._add_revenue_line(job.latest_actual, Decimal("333.33")) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.remaining_gst, Decimal("50.00")) + self.assertEqual(summary.remaining_to_invoice_incl_gst, Decimal("383.33")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("383.33")) + + def test_summary_query_count_does_not_grow_with_cost_lines(self) -> None: + """The job value sums cost lines, so the basis cost set must be prefetched.""" + job = self._create_job("time_materials") + for _ in range(10): + self._add_revenue_line(job.latest_actual, Decimal("10")) + + with CaptureQueriesContext(connection) as few_lines: + build_finish_job_summary(get_job_for_finish_summary(job.id)) + + for _ in range(20): + self._add_revenue_line(job.latest_actual, Decimal("10")) + + with CaptureQueriesContext(connection) as many_lines: + build_finish_job_summary(get_job_for_finish_summary(job.id)) + + self.assertEqual(len(many_lines), len(few_lines)) + + def test_gst_uses_the_configured_rate(self) -> None: + defaults = CompanyDefaults.get_solo() + defaults.gst_rate = Decimal("0.1250") + defaults.save() + job = self._create_job("time_materials") + self._add_revenue_line(job.latest_actual, Decimal("1000")) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.remaining_gst, Decimal("125.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1125.00")) diff --git a/apps/job/migrations/0010_job_completion_checklist.py b/apps/job/migrations/0010_job_completion_checklist.py new file mode 100644 index 000000000..71af30bbf --- /dev/null +++ b/apps/job/migrations/0010_job_completion_checklist.py @@ -0,0 +1,66 @@ +# Generated by Django 6.0.7 on 2026-07-31 21:32 + +import uuid + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("job", "0009_text_unset_constraints"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name="JobCompletionChecklist", + fields=[ + ( + "id", + models.UUIDField( + default=uuid.uuid4, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ("time_entries_complete", models.BooleanField(default=False)), + ("materials_complete", models.BooleanField(default=False)), + ("customer_approval_confirmed", models.BooleanField(default=False)), + ( + "updated_at", + models.DateTimeField( + blank=True, + help_text="When an item was last changed; NULL until the first change.", + null=True, + ), + ), + ( + "job", + models.OneToOneField( + on_delete=django.db.models.deletion.CASCADE, + related_name="completion_checklist", + to="job.job", + ), + ), + ( + "updated_by", + models.ForeignKey( + blank=True, + help_text="Who last changed an item; NULL until the first change.", + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="+", + to=settings.AUTH_USER_MODEL, + ), + ), + ], + options={ + "verbose_name": "Job Completion Checklist", + "verbose_name_plural": "Job Completion Checklists", + }, + ), + ] diff --git a/apps/job/models/__init__.py b/apps/job/models/__init__.py index fa3673ec3..b92540367 100644 --- a/apps/job/models/__init__.py +++ b/apps/job/models/__init__.py @@ -2,6 +2,7 @@ from .costing import CostLine, CostSet from .job import Job +from .job_completion_checklist import JobCompletionChecklist from .job_delta_rejection import JobDeltaRejection from .job_event import JobEvent from .job_file import JobFile @@ -13,6 +14,7 @@ "CostLine", "CostSet", "Job", + "JobCompletionChecklist", "JobDeltaRejection", "JobEvent", "JobFile", diff --git a/apps/job/models/job_completion_checklist.py b/apps/job/models/job_completion_checklist.py new file mode 100644 index 000000000..58e4aa2bd --- /dev/null +++ b/apps/job/models/job_completion_checklist.py @@ -0,0 +1,64 @@ +import uuid + +from django.db import models + +from apps.accounts.models import Staff + + +class JobCompletionChecklist(models.Model): + """Staff confirmations recorded while finishing a job. + + Deliberately advisory: nothing here blocks invoicing, changes job status, + creates tasks, or nags. Its only job is to remember what a staff member + confirmed, and to leave an audit trail when a confirmation is withdrawn. + + ``updated_at`` and ``updated_by`` are NULL until the first confirmation is + recorded — a row can exist with nothing yet confirmed. + """ + + # The confirmable items, in display order. Serializers and the update + # service derive their accepted keys from this tuple, so adding an item here + # is the only change needed to expose it. + ITEM_FIELDS = ( + "time_entries_complete", + "materials_complete", + "customer_approval_confirmed", + ) + + # Item key → the label used in job-history events. + ITEM_LABELS = { + "time_entries_complete": "All time entered", + "materials_complete": "All materials entered", + "customer_approval_confirmed": "Customer approval confirmed", + } + + id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) + job = models.OneToOneField( + "job.Job", + on_delete=models.CASCADE, + related_name="completion_checklist", + ) + time_entries_complete = models.BooleanField(default=False) + materials_complete = models.BooleanField(default=False) + customer_approval_confirmed = models.BooleanField(default=False) + updated_at = models.DateTimeField( + null=True, + blank=True, + help_text="When an item was last changed; NULL until the first change.", + ) + updated_by = models.ForeignKey( + Staff, + on_delete=models.PROTECT, + null=True, + blank=True, + related_name="+", + help_text="Who last changed an item; NULL until the first change.", + ) + + class Meta: + verbose_name = "Job Completion Checklist" + verbose_name_plural = "Job Completion Checklists" + + def __str__(self) -> str: + confirmed = sum(1 for field in self.ITEM_FIELDS if getattr(self, field)) + return f"Checklist for {self.job.name}: {confirmed}/{len(self.ITEM_FIELDS)}" diff --git a/apps/job/models/job_event.py b/apps/job/models/job_event.py index 9ed242b77..4d204a4f0 100644 --- a/apps/job/models/job_event.py +++ b/apps/job/models/job_event.py @@ -1,5 +1,6 @@ import hashlib import uuid +from collections.abc import Callable from datetime import timedelta from django.core.exceptions import ValidationError @@ -47,6 +48,25 @@ def _truncate_change(label: str, old, new) -> str: return f"{label} changed from '{_truncate(old)}' to '{_truncate(new)}'" +def _completion_confirmation_descriptor( + subject: str, +) -> Callable[[str, str], str]: + """Descriptor factory for Finish Job checklist confirmations. + + A withdrawn confirmation reads as plainly as a granted one, since withdrawing + is the change most worth being able to find later. Values are the "Yes"/"No" + strings written by the checklist service. + """ + + def descriptor(old: str, new: str) -> str: + if _truthy(new): + return f"Confirmed {subject}" + else: + return f"Withdrew confirmation of {subject}" + + return descriptor + + def _quote_acceptance_descriptor(old, new) -> str: if new and not old: return f"Quote accepted on {new}" @@ -69,6 +89,13 @@ def _quote_acceptance_descriptor(old, new) -> str: "Marked as collected" if _truthy(new) else "Marked as not collected" ), "Quote acceptance date": _quote_acceptance_descriptor, + "All time entered": _completion_confirmation_descriptor("all time entered"), + "All materials entered": _completion_confirmation_descriptor( + "all materials entered" + ), + "Customer approval confirmed": _completion_confirmation_descriptor( + "customer approval" + ), "Internal notes": lambda old, new: _truncate_change("Notes", old, new), "Job description": lambda old, new: _truncate_change("Description", old, new), "Notes": lambda old, new: _truncate_change("Notes", old, new), @@ -324,28 +351,29 @@ def _build_jsa_description(detail: dict) -> str: return f"JSA generated: {title}" _DESCRIPTION_BUILDERS = { - "job_created": _build_job_created_description.__func__, - "status_changed": _build_status_changed_description.__func__, - "job_updated": _build_changes_description.__func__, - "company_changed": _build_changes_description.__func__, - "person_changed": _build_changes_description.__func__, - "notes_updated": _build_changes_description.__func__, - "delivery_date_changed": _build_changes_description.__func__, - "quote_accepted": _build_changes_description.__func__, - "pricing_changed": _build_changes_description.__func__, - "priority_changed": _build_priority_changed_description.__func__, - "payment_received": _build_changes_description.__func__, - "payment_updated": _build_changes_description.__func__, - "job_collected": _build_changes_description.__func__, - "collection_updated": _build_changes_description.__func__, - "job_rejected": _build_changes_description.__func__, - "manual_note": _build_manual_note_description.__func__, - "invoice_created": _build_invoice_created_description.__func__, - "invoice_deleted": _build_invoice_deleted_description.__func__, - "quote_created": _build_quote_created_description.__func__, - "quote_deleted": _build_quote_deleted_description.__func__, - "delivery_docket_generated": _build_delivery_docket_description.__func__, - "jsa_generated": _build_jsa_description.__func__, + "job_created": _build_job_created_description, + "status_changed": _build_status_changed_description, + "job_updated": _build_changes_description, + "company_changed": _build_changes_description, + "person_changed": _build_changes_description, + "notes_updated": _build_changes_description, + "delivery_date_changed": _build_changes_description, + "quote_accepted": _build_changes_description, + "pricing_changed": _build_changes_description, + "priority_changed": _build_priority_changed_description, + "payment_received": _build_changes_description, + "payment_updated": _build_changes_description, + "job_collected": _build_changes_description, + "collection_updated": _build_changes_description, + "job_rejected": _build_changes_description, + "completion_checklist_updated": _build_changes_description, + "manual_note": _build_manual_note_description, + "invoice_created": _build_invoice_created_description, + "invoice_deleted": _build_invoice_deleted_description, + "quote_created": _build_quote_created_description, + "quote_deleted": _build_quote_deleted_description, + "delivery_docket_generated": _build_delivery_docket_description, + "jsa_generated": _build_jsa_description, } class Meta: diff --git a/apps/job/serializers/__init__.py b/apps/job/serializers/__init__.py index 61be51055..f8c52bb8f 100644 --- a/apps/job/serializers/__init__.py +++ b/apps/job/serializers/__init__.py @@ -82,12 +82,16 @@ AssignJobSerializer, CompanyDefaultsJobDetailSerializer, CompleteJobSerializer, + FinishJobPayload, + FinishJobSummarySerializer, GroupedJobDeltaRejectionListResponseSerializer, GroupedJobDeltaRejectionResolveRequestSerializer, GroupedJobDeltaRejectionResolveResponseSerializer, GroupedJobDeltaRejectionSerializer, InvoiceSerializer, JobBasicInformationResponseSerializer, + JobCompletionChecklistSerializer, + JobCompletionChecklistUpdateSerializer, JobCostSetSummarySerializer, JobCostSummaryResponseSerializer, JobCreateResponseSerializer, @@ -102,6 +106,7 @@ JobEventCreateSerializer, JobEventSerializer, JobEventsResponseSerializer, + JobFinishResponseSerializer, JobHeaderResponseSerializer, JobInvoicesResponseSerializer, JobPatchSerializer, @@ -224,12 +229,16 @@ "FetchJobsResponseSerializer", "FetchStatusValuesResponseSerializer", "FiltersAppliedSerializer", + "FinishJobPayload", + "FinishJobSummarySerializer", "GroupedJobDeltaRejectionListResponseSerializer", "GroupedJobDeltaRejectionResolveRequestSerializer", "GroupedJobDeltaRejectionResolveResponseSerializer", "GroupedJobDeltaRejectionSerializer", "InvoiceSerializer", "JobBasicInformationResponseSerializer", + "JobCompletionChecklistSerializer", + "JobCompletionChecklistUpdateSerializer", "JobCostSetSummarySerializer", "JobCostSummaryResponseSerializer", "JobCreateResponseSerializer", @@ -252,6 +261,7 @@ "JobFileUploadSerializer", "JobFileUploadSuccessResponseSerializer", "JobFileUploadViewResponseSerializer", + "JobFinishResponseSerializer", "JobHeaderResponseSerializer", "JobInvoicesResponseSerializer", "JobLabourRateSerializer", diff --git a/apps/job/serializers/job_serializer.py b/apps/job/serializers/job_serializer.py index 94aa9bb59..31a027214 100644 --- a/apps/job/serializers/job_serializer.py +++ b/apps/job/serializers/job_serializer.py @@ -1,6 +1,6 @@ import json import logging -from typing import Any, Optional +from typing import Any, Optional, TypedDict from django.core.exceptions import ObjectDoesNotExist from drf_spectacular.utils import extend_schema_field @@ -8,8 +8,9 @@ from apps.accounting.models.invoice import Invoice from apps.accounting.models.quote import Quote +from apps.accounting.services.finish_job_summary import FinishJobSummary from apps.company.models import Company, Person -from apps.job.models import Job, JobEvent, JobFile +from apps.job.models import Job, JobCompletionChecklist, JobEvent, JobFile from apps.workflow.models import XeroPayItem from apps.workflow.serializers import AppErrorResponseSerializer from apps.workflow.serializers_base import NullUnsetModelSerializer @@ -847,6 +848,88 @@ class JobCostSummaryResponseSerializer(serializers.Serializer): actual = JobCostSetSummarySerializer(allow_null=True) +class FinishJobPayload(TypedDict): + """What the Finish Job endpoint serializes: a balance plus a checklist.""" + + summary: FinishJobSummary + checklist: JobCompletionChecklist + + +class FinishJobSummarySerializer(serializers.Serializer[FinishJobSummary]): + """The authoritative customer balance shown in the Finish Job workspace. + + Read-only: every value is calculated by + apps.accounting.services.finish_job_summary, and the frontend formats rather + than recomputes them (ADR 0020). + """ + + basis = serializers.CharField(read_only=True) + job_value_excl_gst = serializers.DecimalField( + max_digits=12, decimal_places=2, read_only=True + ) + valid_invoiced_excl_gst = serializers.DecimalField( + max_digits=12, decimal_places=2, read_only=True + ) + outstanding_invoiced_incl_gst = serializers.DecimalField( + max_digits=12, decimal_places=2, read_only=True + ) + remaining_to_invoice_excl_gst = serializers.DecimalField( + max_digits=12, decimal_places=2, read_only=True + ) + remaining_gst = serializers.DecimalField( + max_digits=12, decimal_places=2, read_only=True + ) + remaining_to_invoice_incl_gst = serializers.DecimalField( + max_digits=12, decimal_places=2, read_only=True + ) + total_to_pay_incl_gst = serializers.DecimalField( + max_digits=12, decimal_places=2, read_only=True + ) + over_invoiced_excl_gst = serializers.DecimalField( + max_digits=12, decimal_places=2, read_only=True + ) + + +class JobCompletionChecklistSerializer(serializers.Serializer[JobCompletionChecklist]): + """Read shape for the Finish Job completion checklist. + + updated_at and updated_by_name are null until a staff member first changes an + item, including for the unsaved all-false checklist of a job nobody has + confirmed anything on. + """ + + time_entries_complete = serializers.BooleanField(read_only=True) + materials_complete = serializers.BooleanField(read_only=True) + customer_approval_confirmed = serializers.BooleanField(read_only=True) + updated_at = serializers.DateTimeField(read_only=True, allow_null=True) + updated_by_name = serializers.SerializerMethodField() + + @extend_schema_field(serializers.CharField(allow_null=True)) + def get_updated_by_name(self, obj: JobCompletionChecklist) -> Optional[str]: + if not obj.updated_by: + return None + return obj.updated_by.get_display_full_name() + + +class JobCompletionChecklistUpdateSerializer(serializers.Serializer[None]): + """Partial update shape: send only the items being changed. + + Every item is optional, and unknown keys are rejected by the service rather + than dropped, so a client typo is a 400 instead of a silent no-op. + """ + + time_entries_complete = serializers.BooleanField(required=False) + materials_complete = serializers.BooleanField(required=False) + customer_approval_confirmed = serializers.BooleanField(required=False) + + +class JobFinishResponseSerializer(serializers.Serializer[FinishJobPayload]): + """Everything the Finish Job workspace reads in one request.""" + + summary = FinishJobSummarySerializer(read_only=True) + checklist = JobCompletionChecklistSerializer(read_only=True) + + class JobEventsResponseSerializer(serializers.Serializer): """Serializer for job events response""" diff --git a/apps/job/services/__init__.py b/apps/job/services/__init__.py index 6eb43df58..71e07426f 100644 --- a/apps/job/services/__init__.py +++ b/apps/job/services/__init__.py @@ -31,6 +31,11 @@ serialize_draft_lines, serialize_validation_report, ) + from .job_completion_checklist_service import ( + ChecklistUpdateError, + get_completion_checklist, + update_completion_checklist, + ) from .job_profitability_report import JobProfitabilityReportService from .job_rest_service import ( DeltaValidationError, @@ -110,6 +115,7 @@ "AutoArchiveService", "ChatFileService", "ChatService", + "ChecklistUpdateError", "ChecksumInput", "DataIntegrityService", "DeltaValidationError", @@ -157,6 +163,7 @@ "format_retail_line_total", "generate_delivery_docket", "get_bill_rate_multiplier", + "get_completion_checklist", "get_job_for_delivery_docket_pdf", "get_job_for_workshop_pdf", "get_job_total_value", @@ -185,5 +192,6 @@ "serialize_validation_report", "sync_job_folder", "to_decimal", + "update_completion_checklist", "wait_until_file_ready", ] diff --git a/apps/job/services/job_completion_checklist_service.py b/apps/job/services/job_completion_checklist_service.py new file mode 100644 index 000000000..88be54ca9 --- /dev/null +++ b/apps/job/services/job_completion_checklist_service.py @@ -0,0 +1,97 @@ +"""Reads and partial updates for the Job completion checklist. + +Every accepted change writes one job-history event naming the item, its old and +new value, the acting staff member and the time, so a withdrawn confirmation is +as visible as a granted one. +""" + +from django.db import transaction +from django.utils import timezone + +from apps.accounts.models import Staff +from apps.job.models import Job +from apps.job.models.job_completion_checklist import JobCompletionChecklist +from apps.job.models.job_event import JobEvent + +CHECKLIST_UPDATED_EVENT = "completion_checklist_updated" + + +class ChecklistUpdateError(ValueError): + """Raised when a checklist update names an unknown item or a non-boolean.""" + + +def get_completion_checklist(job: Job) -> JobCompletionChecklist: + """The job's checklist. + + A job nobody has confirmed anything on gets an unsaved all-false checklist + rather than a row written during a read. + """ + checklist = JobCompletionChecklist.objects.filter(job=job).first() + if not checklist: + return JobCompletionChecklist(job=job) + else: + return checklist + + +def update_completion_checklist( + job: Job, updates: dict[str, bool], staff: Staff +) -> JobCompletionChecklist: + """Apply a partial checklist update and audit each item that changed. + + Raises ChecklistUpdateError for unknown item keys or non-boolean values, so a + typo in a client payload is a 400 rather than a silently ignored field. + """ + unknown = sorted(set(updates) - set(JobCompletionChecklist.ITEM_FIELDS)) + if unknown: + raise ChecklistUpdateError( + f"Unknown checklist item(s): {', '.join(unknown)}. " + f"Valid items: {', '.join(JobCompletionChecklist.ITEM_FIELDS)}." + ) + + non_boolean = sorted( + key for key, value in updates.items() if not isinstance(value, bool) + ) + if non_boolean: + raise ChecklistUpdateError( + f"Checklist item(s) must be true or false: {', '.join(non_boolean)}." + ) + + with transaction.atomic(): + checklist, _ = JobCompletionChecklist.objects.get_or_create(job=job) + + changed = { + key: value + for key, value in updates.items() + if getattr(checklist, key) != value + } + if not changed: + return checklist + + for key, new_value in changed.items(): + _record_change(job, staff, key, getattr(checklist, key), new_value) + setattr(checklist, key, new_value) + + checklist.updated_at = timezone.now() + checklist.updated_by = staff + checklist.save() + + return checklist + + +def _record_change( + job: Job, staff: Staff, item: str, old_value: bool, new_value: bool +) -> None: + JobEvent.objects.create( + job=job, + staff=staff, + event_type=CHECKLIST_UPDATED_EVENT, + detail={ + "changes": [ + { + "field_name": JobCompletionChecklist.ITEM_LABELS[item], + "old_value": "Yes" if old_value else "No", + "new_value": "Yes" if new_value else "No", + } + ] + }, + ) diff --git a/apps/job/tests/test_completion_checklist.py b/apps/job/tests/test_completion_checklist.py new file mode 100644 index 000000000..ab763abec --- /dev/null +++ b/apps/job/tests/test_completion_checklist.py @@ -0,0 +1,320 @@ +"""Tests for the Finish Job completion checklist. + +The checklist is advisory by design: these tests pin both that changes are +audited and that ticking a box never affects what a user can invoice or what +status a job is in. +""" + +from decimal import Decimal + +from django.urls import reverse +from django.utils import timezone + +from apps.accounting.services.invoice_calculation import calculate_invoice_amount +from apps.accounts.models import Staff +from apps.company.models import Company +from apps.job.models import Job, JobCompletionChecklist, JobEvent +from apps.job.services.job_completion_checklist_service import ( + CHECKLIST_UPDATED_EVENT, + ChecklistUpdateError, + get_completion_checklist, + update_completion_checklist, +) +from apps.testing import BaseAPITestCase, BaseTestCase + + +class TestCompletionChecklistService(BaseTestCase): + def setUp(self) -> None: + self.client_obj = Company.objects.create( + name="Test Company", + xero_last_modified=timezone.now(), + ) + self.job = Job( + company=self.client_obj, + name="Test Job", + pricing_methodology="time_materials", + ) + self.job.save(staff=self.test_staff) + + # --- Reading --- + + def test_unconfirmed_job_reads_as_all_false_without_writing_a_row(self) -> None: + checklist = get_completion_checklist(self.job) + + self.assertFalse(checklist.time_entries_complete) + self.assertFalse(checklist.materials_complete) + self.assertFalse(checklist.customer_approval_confirmed) + self.assertIsNone(checklist.updated_at) + self.assertIsNone(checklist.updated_by) + self.assertFalse(JobCompletionChecklist.objects.filter(job=self.job).exists()) + + # --- Partial updates --- + + def test_update_touches_only_the_named_item(self) -> None: + update_completion_checklist( + self.job, {"materials_complete": True}, self.test_staff + ) + + checklist = get_completion_checklist(self.job) + self.assertTrue(checklist.materials_complete) + self.assertFalse(checklist.time_entries_complete) + self.assertFalse(checklist.customer_approval_confirmed) + + def test_update_records_who_and_when(self) -> None: + checklist = update_completion_checklist( + self.job, {"time_entries_complete": True}, self.test_staff + ) + + self.assertEqual(checklist.updated_by, self.test_staff) + self.assertIsNotNone(checklist.updated_at) + + def test_second_update_preserves_the_first(self) -> None: + update_completion_checklist( + self.job, {"time_entries_complete": True}, self.test_staff + ) + update_completion_checklist( + self.job, {"customer_approval_confirmed": True}, self.test_staff + ) + + checklist = get_completion_checklist(self.job) + self.assertTrue(checklist.time_entries_complete) + self.assertTrue(checklist.customer_approval_confirmed) + + def test_unknown_item_is_rejected(self) -> None: + with self.assertRaises(ChecklistUpdateError) as ctx: + update_completion_checklist( + self.job, {"everything_is_fine": True}, self.test_staff + ) + + self.assertIn("everything_is_fine", str(ctx.exception)) + self.assertFalse(JobCompletionChecklist.objects.filter(job=self.job).exists()) + + def test_non_boolean_value_is_rejected(self) -> None: + with self.assertRaises(ChecklistUpdateError): + update_completion_checklist( + self.job, + {"materials_complete": "yes"}, # type: ignore[dict-item] # the guard exists for untyped JSON off the wire + self.test_staff, + ) + + def test_rejected_update_does_not_apply_its_valid_items(self) -> None: + """An unknown key fails the whole payload rather than half-applying it.""" + with self.assertRaises(ChecklistUpdateError): + update_completion_checklist( + self.job, + {"materials_complete": True, "nonsense": True}, + self.test_staff, + ) + + self.assertFalse(get_completion_checklist(self.job).materials_complete) + + # --- Audit history --- + + def test_each_changed_item_adds_one_history_event(self) -> None: + update_completion_checklist( + self.job, + {"time_entries_complete": True, "materials_complete": True}, + self.test_staff, + ) + + events = JobEvent.objects.filter( + job=self.job, event_type=CHECKLIST_UPDATED_EVENT + ) + self.assertEqual(events.count(), 2) + + def test_history_event_records_item_values_and_staff(self) -> None: + update_completion_checklist( + self.job, {"customer_approval_confirmed": True}, self.test_staff + ) + + event = JobEvent.objects.get(job=self.job, event_type=CHECKLIST_UPDATED_EVENT) + change = event.detail["changes"][0] + self.assertEqual(change["field_name"], "Customer approval confirmed") + self.assertEqual(change["old_value"], "No") + self.assertEqual(change["new_value"], "Yes") + self.assertEqual(event.staff, self.test_staff) + self.assertIsNotNone(event.timestamp) + + def test_withdrawing_a_confirmation_is_audited(self) -> None: + """The change most worth finding later is someone unticking a box.""" + update_completion_checklist( + self.job, {"materials_complete": True}, self.test_staff + ) + update_completion_checklist( + self.job, {"materials_complete": False}, self.test_staff + ) + + events = JobEvent.objects.filter( + job=self.job, event_type=CHECKLIST_UPDATED_EVENT + ).order_by("timestamp") + self.assertEqual(events.count(), 2) + self.assertEqual( + events[1].description, "Withdrew confirmation of all materials entered" + ) + + def test_confirmation_reads_as_plain_english_in_history(self) -> None: + update_completion_checklist( + self.job, {"time_entries_complete": True}, self.test_staff + ) + + event = JobEvent.objects.get(job=self.job, event_type=CHECKLIST_UPDATED_EVENT) + self.assertEqual(event.description, "Confirmed all time entered") + + def test_setting_an_item_to_its_current_value_adds_no_event(self) -> None: + update_completion_checklist( + self.job, {"materials_complete": True}, self.test_staff + ) + update_completion_checklist( + self.job, {"materials_complete": True}, self.test_staff + ) + + self.assertEqual( + JobEvent.objects.filter( + job=self.job, event_type=CHECKLIST_UPDATED_EVENT + ).count(), + 1, + ) + + # --- The checklist must not become a gate --- + + def test_checklist_does_not_change_job_status(self) -> None: + original_status = self.job.status + + update_completion_checklist( + self.job, + { + "time_entries_complete": True, + "materials_complete": True, + "customer_approval_confirmed": True, + }, + self.test_staff, + ) + + self.job.refresh_from_db() + self.assertEqual(self.job.status, original_status) + + def test_invoicing_works_with_an_untouched_checklist(self) -> None: + """Nothing confirmed must not stand between a customer and an invoice.""" + self._add_actual_revenue(Decimal("500")) + + result = calculate_invoice_amount(self.job, mode="invoice_costs_to_date") + + self.assertEqual(result.calculated_amount, Decimal("500")) + + def test_invoice_amount_is_unchanged_by_confirmations(self) -> None: + self._add_actual_revenue(Decimal("500")) + before = calculate_invoice_amount( + self.job, mode="invoice_costs_to_date" + ).calculated_amount + + update_completion_checklist( + self.job, + {"time_entries_complete": True, "materials_complete": True}, + self.test_staff, + ) + + after = calculate_invoice_amount( + self.job, mode="invoice_costs_to_date" + ).calculated_amount + self.assertEqual(before, after) + + def _add_actual_revenue(self, revenue: Decimal) -> None: + from datetime import date + + from apps.job.models.costing import CostLine + + CostLine.objects.create( + cost_set=self.job.latest_actual, + kind="adjust", + desc="Test line", + quantity=Decimal("1.000"), + unit_cost=Decimal("0.00"), + unit_rev=revenue, + accounting_date=date.today(), + ) + + +class TestFinishJobEndpoint(BaseAPITestCase): + """The generated client's read and partial-update operations.""" + + def setUp(self) -> None: + self.client_obj = Company.objects.create( + name="Test Company", + xero_last_modified=timezone.now(), + ) + self.job = Job( + company=self.client_obj, + name="Test Job", + pricing_methodology="time_materials", + ) + self.job.save(staff=self.test_staff) + self.url = reverse("jobs:job_finish_rest", args=[self.job.id]) + # Changing a checklist item is an office action; the shared test_staff is + # deliberately neither office nor workshop. + self.office_staff = Staff.objects.create_user( + email="office@example.com", + password="testpass", + first_name="Office", + last_name="Person", + is_office_staff=True, + ) + self.client.force_login(self.office_staff) + + def test_get_returns_summary_and_checklist(self) -> None: + response = self.client.get(self.url) + + self.assertEqual(response.status_code, 200) + self.assertIn("summary", response.data) + self.assertIn("checklist", response.data) + self.assertEqual(response.data["summary"]["basis"], "actual_revenue") + self.assertFalse(response.data["checklist"]["materials_complete"]) + self.assertIsNone(response.data["checklist"]["updated_by_name"]) + + def test_get_returns_currency_values_as_json_numbers(self) -> None: + """Zod on the frontend validates numbers; DRF's default strings fail it.""" + payload = self.client.get(self.url).json() + + for field, value in payload["summary"].items(): + if field == "basis": + continue + self.assertIsInstance(value, (int, float), msg=f"{field} is not a number") + + def test_patch_applies_a_partial_update_and_returns_fresh_state(self) -> None: + response = self.client.patch( + self.url, + data={"customer_approval_confirmed": True}, + content_type="application/json", + ) + + self.assertEqual(response.status_code, 200) + self.assertTrue(response.data["checklist"]["customer_approval_confirmed"]) + self.assertFalse(response.data["checklist"]["materials_complete"]) + self.assertEqual( + response.data["checklist"]["updated_by_name"], + self.office_staff.get_display_full_name(), + ) + + def test_get_on_a_fixed_price_job_reports_the_quote_basis(self) -> None: + """Exercises the quote prefetch branch under the n+1 middleware.""" + quoted_job = Job( + company=self.client_obj, + name="Quoted Job", + pricing_methodology="fixed_price", + ) + quoted_job.save(staff=self.test_staff) + + response = self.client.get( + reverse("jobs:job_finish_rest", args=[quoted_job.id]) + ) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["summary"]["basis"], "quote") + + def test_patch_rejects_an_unknown_item(self) -> None: + response = self.client.patch( + self.url, + data={"not_a_real_item": True}, + content_type="application/json", + ) + + self.assertEqual(response.status_code, 400) diff --git a/apps/job/urls_rest.py b/apps/job/urls_rest.py index b2a7a4e26..454d40e12 100644 --- a/apps/job/urls_rest.py +++ b/apps/job/urls_rest.py @@ -37,6 +37,7 @@ JobDetailRestView, JobEventListRestView, JobEventRestView, + JobFinishRestView, JobHeaderRestView, JobInvoicesRestView, JobQuoteAcceptRestView, @@ -176,6 +177,12 @@ JobCostSummaryRestView.as_view(), name="job_cost_summary_rest", ), + # Finish Job workspace: customer balance + completion checklist + path( + "jobs//finish/", + JobFinishRestView.as_view(), + name="job_finish_rest", + ), # Job status choices path( "jobs/status-choices/", diff --git a/apps/job/views/__init__.py b/apps/job/views/__init__.py index c1a210fbf..258a7c582 100644 --- a/apps/job/views/__init__.py +++ b/apps/job/views/__init__.py @@ -44,6 +44,7 @@ JobDetailRestView, JobEventListRestView, JobEventRestView, + JobFinishRestView, JobHeaderRestView, JobInvoicesRestView, JobQuoteAcceptRestView, @@ -137,6 +138,7 @@ "JobFileDetailView", "JobFileThumbnailView", "JobFilesCollectionView", + "JobFinishRestView", "JobHeaderRestView", "JobInvoicesRestView", "JobLabourRatesView", diff --git a/apps/job/views/job_rest_views.py b/apps/job/views/job_rest_views.py index 1ffdcf751..94f3a7aa0 100644 --- a/apps/job/views/job_rest_views.py +++ b/apps/job/views/job_rest_views.py @@ -22,16 +22,23 @@ from rest_framework import status from rest_framework.decorators import api_view, permission_classes from rest_framework.permissions import SAFE_METHODS, IsAuthenticated +from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView +from apps.accounting.services.finish_job_summary import ( + build_finish_job_summary, + get_job_for_finish_summary, +) from apps.accounts.models import Staff from apps.job.etag import generate_job_etag from apps.job.models import Job, JobDeltaRejection from apps.job.permissions import IsOfficeStaff from apps.job.serializers.job_serializer import ( CompanyDefaultsJobDetailSerializer, + FinishJobPayload, JobBasicInformationResponseSerializer, + JobCompletionChecklistUpdateSerializer, JobCostSummaryResponseSerializer, JobCreateResponseSerializer, JobCreateSerializer, @@ -42,6 +49,7 @@ JobEventCreateResponseSerializer, JobEventCreateSerializer, JobEventsResponseSerializer, + JobFinishResponseSerializer, JobHeaderResponseSerializer, JobInvoicesResponseSerializer, JobQuoteAcceptanceSerializer, @@ -53,6 +61,10 @@ QuoteSerializer, WeeklyMetricsSerializer, ) +from apps.job.services.job_completion_checklist_service import ( + get_completion_checklist, + update_completion_checklist, +) from apps.job.services.job_rest_service import ( DeltaValidationError, JobRestService, @@ -1078,6 +1090,83 @@ def calculate_profit_margin(cost_set): return self.handle_service_error(e) +def _finish_job_payload(job: Job) -> FinishJobPayload: + return { + "summary": build_finish_job_summary(job), + "checklist": get_completion_checklist(job), + } + + +@method_decorator(csrf_exempt, name="dispatch") +class JobFinishRestView(BaseJobRestView): + """Finish Job workspace: the authoritative customer balance and checklist. + + Deliberately not ETagged on the job. The balance moves when invoices change, + and an invoice arriving from Xero does not touch ``job.updated_at``, so a + job-derived ETag would answer 304 while showing a customer the wrong amount + to pay. + """ + + serializer_class = JobFinishResponseSerializer + + @extend_schema( + responses={ + 200: JobFinishResponseSerializer, + 400: JobRestErrorResponseSerializer, + }, + description=( + "Fetch the authoritative Finish Job customer balance and completion " + "checklist for a job. All currency values are calculated server-side." + ), + tags=["Jobs"], + ) + def get(self, request: Request, job_id: UUID) -> Response: + try: + job = get_job_for_finish_summary(job_id) + serializer = JobFinishResponseSerializer(_finish_job_payload(job)) + return Response(serializer.data, status=status.HTTP_200_OK) + + except Job.DoesNotExist as exc: + persist_app_error(exc, job_id=job_id) + raise ValueError(f"Job with id {job_id} not found") from exc + except Exception as e: + return self.handle_service_error(e) + + @extend_schema( + request=JobCompletionChecklistUpdateSerializer, + responses={ + 200: JobFinishResponseSerializer, + 400: JobRestErrorResponseSerializer, + }, + description=( + "Update one or more completion checklist items. Each changed item adds " + "a job-history event. Unknown item keys are rejected." + ), + tags=["Jobs"], + ) + def patch(self, request: Request, job_id: UUID) -> Response: + try: + # A checklist item records who confirmed it, so the acting staff + # member must be a real one. IsOfficeStaff already guarantees this; + # the guard makes the attribution contract explicit. + staff = request.user + if not isinstance(staff, Staff): + raise ValueError( + "Checklist updates require an authenticated staff member." + ) + + job = get_job_for_finish_summary(job_id) + update_completion_checklist(job, self.parse_json_body(request), staff) + serializer = JobFinishResponseSerializer(_finish_job_payload(job)) + return Response(serializer.data, status=status.HTTP_200_OK) + + except Job.DoesNotExist as exc: + persist_app_error(exc, job_id=job_id) + raise ValueError(f"Job with id {job_id} not found") from exc + except Exception as e: + return self.handle_service_error(e) + + @method_decorator(csrf_exempt, name="dispatch") class JobStatusChoicesRestView(BaseJobRestView): """ diff --git a/docs/urls/job.md b/docs/urls/job.md index 4d1062d4e..3047a1f2c 100644 --- a/docs/urls/job.md +++ b/docs/urls/job.md @@ -52,6 +52,7 @@ | `/jobs//files/` | `job_files_collection_view.JobFilesCollectionView` | `jobs:job_files_collection` | Collection operations on job files. | | `/jobs//files//` | `job_file_detail_view.JobFileDetailView` | `jobs:job_file_detail` | Resource operations on individual job files. | | `/jobs//files//thumbnail/` | `job_file_thumbnail_view.JobFileThumbnailView` | `jobs:job_file_thumbnail` | Thumbnail serving for job files. | +| `/jobs//finish/` | `job_rest_views.JobFinishRestView` | `jobs:job_finish_rest` | Finish Job workspace: the authoritative customer balance and checklist. | | `/jobs//header/` | `job_rest_views.JobHeaderRestView` | `jobs:job_header_rest` | REST view for Job header information. | | `/jobs//invoices/` | `job_rest_views.JobInvoicesRestView` | `jobs:job_invoices_rest` | REST view for Job invoices. | | `/jobs//labour-rates/` | `labour_views.JobLabourRatesView` | `jobs:job_labour_rates_rest` | Read or update a job's per-subtype charge-out rates. | diff --git a/frontend/schema.yml b/frontend/schema.yml index 830a830de..4a7b126a2 100644 --- a/frontend/schema.yml +++ b/frontend/schema.yml @@ -3996,6 +3996,74 @@ paths: schema: $ref: '#/components/schemas/JobFileThumbnailErrorResponse' description: '' + /api/job/jobs/{job_id}/finish/: + get: + operationId: job_jobs_finish_retrieve + description: Fetch the authoritative Finish Job customer balance and completion + checklist for a job. All currency values are calculated server-side. + parameters: + - in: path + name: job_id + schema: + type: string + format: uuid + required: true + tags: + - Jobs + security: + - cookieAuth: [] + responses: + '200': + content: + application/json: + schema: + $ref: '#/components/schemas/JobFinishResponse' + description: '' + '400': + content: + application/json: + schema: + $ref: '#/components/schemas/JobRestErrorResponse' + description: '' + patch: + operationId: job_jobs_finish_partial_update + description: Update one or more completion checklist items. Each changed item + adds a job-history event. Unknown item keys are rejected. + parameters: + - in: path + name: job_id + schema: + type: string + format: uuid + required: true + tags: + - Jobs + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/PatchedJobCompletionChecklistUpdateRequest' + application/x-www-form-urlencoded: + schema: + $ref: '#/components/schemas/PatchedJobCompletionChecklistUpdateRequest' + multipart/form-data: + schema: + $ref: '#/components/schemas/PatchedJobCompletionChecklistUpdateRequest' + security: + - cookieAuth: [] + responses: + '200': + content: + application/json: + schema: + $ref: '#/components/schemas/JobFinishResponse' + description: '' + '400': + content: + application/json: + schema: + $ref: '#/components/schemas/JobRestErrorResponse' + description: '' /api/job/jobs/{job_id}/header/: get: operationId: job_jobs_header_retrieve @@ -13282,6 +13350,92 @@ components: required: - end_date - start_date + FinishJobSummary: + type: object + description: |- + The authoritative customer balance shown in the Finish Job workspace. + + Read-only: every value is calculated by + apps.accounting.services.finish_job_summary, and the frontend formats rather + than recomputes them (ADR 0020). + properties: + basis: + type: string + readOnly: true + job_value_excl_gst: + type: number + format: double + maximum: 10000000000 + minimum: -10000000000 + exclusiveMaximum: true + exclusiveMinimum: true + readOnly: true + valid_invoiced_excl_gst: + type: number + format: double + maximum: 10000000000 + minimum: -10000000000 + exclusiveMaximum: true + exclusiveMinimum: true + readOnly: true + outstanding_invoiced_incl_gst: + type: number + format: double + maximum: 10000000000 + minimum: -10000000000 + exclusiveMaximum: true + exclusiveMinimum: true + readOnly: true + remaining_to_invoice_excl_gst: + type: number + format: double + maximum: 10000000000 + minimum: -10000000000 + exclusiveMaximum: true + exclusiveMinimum: true + readOnly: true + remaining_gst: + type: number + format: double + maximum: 10000000000 + minimum: -10000000000 + exclusiveMaximum: true + exclusiveMinimum: true + readOnly: true + remaining_to_invoice_incl_gst: + type: number + format: double + maximum: 10000000000 + minimum: -10000000000 + exclusiveMaximum: true + exclusiveMinimum: true + readOnly: true + total_to_pay_incl_gst: + type: number + format: double + maximum: 10000000000 + minimum: -10000000000 + exclusiveMaximum: true + exclusiveMinimum: true + readOnly: true + over_invoiced_excl_gst: + type: number + format: double + maximum: 10000000000 + minimum: -10000000000 + exclusiveMaximum: true + exclusiveMinimum: true + readOnly: true + required: + - basis + - job_value_excl_gst + - outstanding_invoiced_incl_gst + - over_invoiced_excl_gst + - remaining_gst + - remaining_to_invoice_excl_gst + - remaining_to_invoice_incl_gst + - total_to_pay_incl_gst + - valid_invoiced_excl_gst FormCreateRequest: type: object description: Request serializer for creating a form document (no Google Doc). @@ -14173,6 +14327,39 @@ components: - job_name - job_number - revenue + JobCompletionChecklist: + type: object + description: |- + Read shape for the Finish Job completion checklist. + + updated_at and updated_by_name are null until a staff member first changes an + item, including for the unsaved all-false checklist of a job nobody has + confirmed anything on. + properties: + time_entries_complete: + type: boolean + readOnly: true + materials_complete: + type: boolean + readOnly: true + customer_approval_confirmed: + type: boolean + readOnly: true + updated_at: + type: string + format: date-time + readOnly: true + nullable: true + updated_by_name: + type: string + nullable: true + readOnly: true + required: + - customer_approval_confirmed + - materials_complete + - time_entries_complete + - updated_at + - updated_by_name JobCostSetSummary: type: object description: Serializer for cost set summary data in job views @@ -14698,6 +14885,21 @@ components: required: - message - uploaded + JobFinishResponse: + type: object + description: Everything the Finish Job workspace reads in one request. + properties: + summary: + allOf: + - $ref: '#/components/schemas/FinishJobSummary' + readOnly: true + checklist: + allOf: + - $ref: '#/components/schemas/JobCompletionChecklist' + readOnly: true + required: + - checklist + - summary JobForPurchasing: type: object description: Serializer for Job model in purchasing contexts. @@ -17615,6 +17817,20 @@ components: description: JSON schema defining entry fields for form templates status: $ref: '#/components/schemas/FormStatusEnum' + PatchedJobCompletionChecklistUpdateRequest: + type: object + description: |- + Partial update shape: send only the items being changed. + + Every item is optional, and unknown keys are rejected by the service rather + than dropped, so a client typo is a 400 instead of a silent no-op. + properties: + time_entries_complete: + type: boolean + materials_complete: + type: boolean + customer_approval_confirmed: + type: boolean PatchedJobDeltaEnvelopeRequest: type: object description: Serializer that validates the delta envelope submitted by the frontend. diff --git a/frontend/src/api/generated/api.ts b/frontend/src/api/generated/api.ts index ed8ad379e..526d3756b 100644 --- a/frontend/src/api/generated/api.ts +++ b/frontend/src/api/generated/api.ts @@ -1788,6 +1788,35 @@ const JobFileThumbnailErrorResponse = z.object({ status: z.string().optional().default('error'), message: z.string(), }) +const FinishJobSummary = z.object({ + basis: z.string(), + job_value_excl_gst: z.number().gt(-10000000000).lt(10000000000), + valid_invoiced_excl_gst: z.number().gt(-10000000000).lt(10000000000), + outstanding_invoiced_incl_gst: z.number().gt(-10000000000).lt(10000000000), + remaining_to_invoice_excl_gst: z.number().gt(-10000000000).lt(10000000000), + remaining_gst: z.number().gt(-10000000000).lt(10000000000), + remaining_to_invoice_incl_gst: z.number().gt(-10000000000).lt(10000000000), + total_to_pay_incl_gst: z.number().gt(-10000000000).lt(10000000000), + over_invoiced_excl_gst: z.number().gt(-10000000000).lt(10000000000), +}) +const JobCompletionChecklist = z.object({ + time_entries_complete: z.boolean(), + materials_complete: z.boolean(), + customer_approval_confirmed: z.boolean(), + updated_at: z.string().datetime({ offset: true }).nullable(), + updated_by_name: z.string().nullable(), +}) +const JobFinishResponse = z.object({ + summary: FinishJobSummary, + checklist: JobCompletionChecklist, +}) +const PatchedJobCompletionChecklistUpdateRequest = z + .object({ + time_entries_complete: z.boolean(), + materials_complete: z.boolean(), + customer_approval_confirmed: z.boolean(), + }) + .partial() const JobStatusEnum = z.enum([ 'draft', 'awaiting_approval', @@ -3939,6 +3968,10 @@ export const schemas = { JobFileRequest, JobFileUpdateSuccessResponse, JobFileThumbnailErrorResponse, + FinishJobSummary, + JobCompletionChecklist, + JobFinishResponse, + PatchedJobCompletionChecklistUpdateRequest, JobStatusEnum, JobHeaderResponse, JobInvoicesResponse, @@ -7035,6 +7068,53 @@ POST /job/rest/jobs/<uuid:pk>/quote/preview/`, }, ], }, + { + method: 'get', + path: '/api/job/jobs/:job_id/finish/', + alias: 'job_jobs_finish_retrieve', + description: `Fetch the authoritative Finish Job customer balance and completion checklist for a job. All currency values are calculated server-side.`, + requestFormat: 'json', + parameters: [ + { + name: 'job_id', + type: 'Path', + schema: z.string().uuid(), + }, + ], + response: JobFinishResponse, + errors: [ + { + status: 400, + schema: JobRestErrorResponse, + }, + ], + }, + { + method: 'patch', + path: '/api/job/jobs/:job_id/finish/', + alias: 'job_jobs_finish_partial_update', + description: `Update one or more completion checklist items. Each changed item adds a job-history event. Unknown item keys are rejected.`, + requestFormat: 'json', + parameters: [ + { + name: 'body', + type: 'Body', + schema: PatchedJobCompletionChecklistUpdateRequest, + }, + { + name: 'job_id', + type: 'Path', + schema: z.string().uuid(), + }, + ], + response: JobFinishResponse, + errors: [ + { + status: 400, + schema: JobRestErrorResponse, + }, + ], + }, { method: 'get', path: '/api/job/jobs/:job_id/header/', diff --git a/mypy-baseline.txt b/mypy-baseline.txt index 79b9805ed..18d83a9e1 100644 --- a/mypy-baseline.txt +++ b/mypy-baseline.txt @@ -387,28 +387,6 @@ apps/job/models/job.py:0: error: Item "None" of "str | None" has no attribute "r apps/job/models/job.py:0: error: Missing type arguments for generic type "QuerySet" [type-arg] apps/job/models/job.py:0: error: Returning Any from function declared to return "date | None" [no-any-return] apps/job/models/job.py:0: error: Returning Any from function declared to return "date | None" [no-any-return] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] -apps/job/models/job_event.py:0: error: "Callable[[dict[Any, Any]], str]" has no attribute "__func__" [attr-defined] apps/job/models/job_event.py:0: error: Argument 1 to "_format_ordinal" has incompatible type "Any | None"; expected "int" [arg-type] apps/job/models/job_event.py:0: error: Argument 1 to "_format_ordinal" has incompatible type "Any | None"; expected "int" [arg-type] apps/job/models/job_event.py:0: error: Argument 1 to "_format_ordinal" has incompatible type "Any | None"; expected "int" [arg-type] @@ -439,7 +417,6 @@ apps/job/models/job_event.py:0: error: Returning Any from function declared to r apps/job/models/job_event.py:0: error: Returning Any from function declared to return "str" [no-any-return] apps/job/models/job_event.py:0: error: Returning Any from function declared to return "str" [no-any-return] apps/job/models/job_event.py:0: error: Returning Any from function declared to return "str" [no-any-return] -apps/job/models/job_event.py:0: error: Returning Any from function declared to return "str" [no-any-return] apps/job/models/job_event.py:0: error: Unsupported left operand type for < ("None") [operator] apps/job/models/job_event.py:0: note: Both left and right operands are unions apps/job/models/job_file.py:0: error: Argument 1 to "get_job_folder_path" has incompatible type "int"; expected "str" [arg-type] From 61fbc61dacbf9f7515a71bbf1035386d4118f8ea Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sat, 1 Aug 2026 10:03:14 +1200 Subject: [PATCH 3/8] feat: turn Cost Analysis into the Finish Job workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cost Analysis was a read-only comparison table almost nobody opened, while Actual mixed cost entry with the invoice controls. Neither answered the question staff actually have at the counter with the customer standing there: what do they pay, including GST. Finish Job is that one place. It leads with the server-owned customer balance — subtotal, GST, unpaid invoices, Total to pay — then the invoice card, modal, history, Xero links and deletion moved intact from Actual, then the completion checklist, then the cost comparison retained in compact form underneath. Actual keeps materials, adjustments, its summary and detailed cost entry. Its Invoiced and To Be Invoiced chips are gone, along with the toBeInvoiced computation behind them: invoice figures now have exactly one authoritative home, and no quote-minus-invoices arithmetic happens in the browser. Behaviour changes worth naming: - a fully covered job offers no invoice action at all, instead of an always-visible button that would raise a $0 invoice; - invoice wording follows the stage of work — advance/deposit/progress before completion, remaining balance after — without gating invoicing by status; - the balance is re-read from the server after create and delete rather than patched locally, because it is only correct against the current invoice set. KAN-222 ships here too: labour budget, hours used, and remaining-or-overrun, in the component that would otherwise have to be reopened next week to add them. The checklist rollback uses v-model rather than a one-way :checked binding — with the latter, Vue cannot repaint a value that never changed in state, so a failed save left a tick showing that the server had rejected. The invoice E2E now drives Finish Job and additionally asserts the acceptance criterion the old spec could not see: after invoicing in full, the job shows nothing remaining and stops asking for another invoice. KAN-323, KAN-222 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH --- frontend/src/components/job/JobActualTab.vue | 487 +------- .../src/components/job/JobCostAnalysisTab.vue | 359 ------ frontend/src/components/job/JobFinishTab.vue | 1095 +++++++++++++++++ frontend/src/components/job/JobViewTabs.vue | 25 +- .../__tests__/JobFinishTab.balance.test.ts | 289 +++++ .../__tests__/JobFinishTab.checklist.test.ts | 197 +++ .../JobFinishTab.labourHours.test.ts | 148 +++ frontend/src/constants/job-tabs.ts | 2 +- frontend/tests/job/job-xero-invoice.spec.ts | 27 +- 9 files changed, 1763 insertions(+), 866 deletions(-) delete mode 100644 frontend/src/components/job/JobCostAnalysisTab.vue create mode 100644 frontend/src/components/job/JobFinishTab.vue create mode 100644 frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts create mode 100644 frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts create mode 100644 frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts diff --git a/frontend/src/components/job/JobActualTab.vue b/frontend/src/components/job/JobActualTab.vue index 18bcb768e..0f046f0cd 100644 --- a/frontend/src/components/job/JobActualTab.vue +++ b/frontend/src/components/job/JobActualTab.vue @@ -35,16 +35,6 @@ Time & Expenses {{ formatCurrency(timeAndExpenses) }} -
  • - - Invoiced - {{ formatCurrency(invoiceTotal) }} -
    • -
  • - - To Be Invoiced - {{ formatCurrency(toBeInvoiced) }} -
    • @@ -99,7 +89,7 @@ - + @@ -268,174 +128,6 @@ - - - - - - Create Invoice - - Choose how to invoice - {{ - props.pricingMethodology === 'fixed_price' - ? 'this quoted job' - : 'time & materials' - }} - - -
    - -
    -
    - Quote total - {{ formatCurrency(quoteTotal) }} -
    -
    - Actual revenue to date - {{ - formatCurrency(actualSummary.rev) - }} -
    -
    - Already invoiced - {{ formatCurrency(invoiceTotal) }} -
    -
    - Current amount to invoice - {{ - formatCurrency(toBeInvoiced) - }} -
    -
    - - -
    - - - - - -
    -
    - - - -     -     @@ -444,7 +136,7 @@ import debug from 'debug' const log = debug('job:actual') import { toLocalDateString } from '../../utils/dateUtils' -import { formatCurrency, formatDate } from '@/utils/string-formatting' +import { formatCurrency } from '@/utils/string-formatting' import { normalizeOptionalDecimal } from '@/utils/number' import { onMounted, ref, computed, reactive } from 'vue' import { useRouter } from 'vue-router' @@ -457,10 +149,8 @@ import { schemas } from '../../api/generated/api' import { useSmartCostLineDelete } from '../../composables/useSmartCostLineDelete' import { useCostSummary } from '../../composables/useCostSummary' import { useCostLineDrafts } from '@/composables/useCostLineDrafts' -import { useXeroConnection } from '../../composables/useXeroConnection' import { api } from '../../api/client' import { z } from 'zod' -import type { AxiosError } from 'axios' import type { KindOption } from '../shared/SmartCostLinesTable.vue' import { useStockStore } from '../../stores/stockStore' import SmartCostLinesTable from '../shared/SmartCostLinesTable.vue' @@ -473,18 +163,12 @@ import { DialogFooter, } from '../ui/dialog' import { Button } from '../ui/button' -import { Card, CardHeader, CardFooter, CardContent, CardDescription, CardTitle } from '../ui/card' -import { Input } from '../ui/input' -import { ExternalLink, Trash2 } from 'lucide-vue-next' -import { Badge } from '../ui/badge' import { useSaveFeedback } from '@/composables/useSaveFeedback' type CostLine = z.infer type CostSet = z.infer type KanbanStaff = z.infer type StockConsumeRequest = z.infer -type Invoice = z.infer -type XeroInvoiceCreateRequest = z.infer // Type guard functions to safely access meta and ext_refs function isDeliveryReceiptMeta(meta: unknown): meta is { source: string; po_number?: string } { @@ -526,11 +210,7 @@ function isStockExtRefs(extRefs: unknown): extRefs is { stock_id: string } { const props = defineProps<{ jobId: string - jobNumber: string pricingMethodology: string - quoted: boolean - fullyInvoiced: boolean - paid?: boolean }>() const jobActualSaveFeedback = useSaveFeedback(`job-actual:${props.jobId}`, { toastErrors: false, @@ -538,162 +218,17 @@ const jobActualSaveFeedback = useSaveFeedback(`job-actual:${props.jobId}`, { const emit = defineEmits<{ 'cost-line-changed': [] - 'quote-created': [] - 'quote-accepted': [] - 'invoice-created': [] - 'quote-deleted': [] - 'invoice-deleted': [] }>() const stockStore = useStockStore() -// Local state for invoices -const isCreatingInvoice = ref(false) -const deletingInvoiceId = ref(null) // Track which invoice is being deleted -const { xeroConnected } = useXeroConnection() - -// Local state for KPIs +// KPI chips. Invoice figures deliberately live in Finish Job only, so there is +// one authoritative place showing what the customer owes. const estimateTotal = ref(0) const quoteTotal = ref(0) const costsSummaryLoading = ref(false) - -// --- COMPUTED PROPERTIES --- -const invoices = ref>([]) const timeAndExpenses = computed(() => actualSummary.value.rev) -const invoiceTotal = computed(() => { - if (!invoices.value.length) return 0 - return invoices.value - .filter((inv) => inv.status !== 'VOIDED' && inv.status !== 'DELETED') - .reduce((sum, inv) => sum + (inv.total_excl_tax || 0), 0) -}) - -const toBeInvoiced = computed(() => { - // For fixed price jobs with a quote, use the quoted amount - // For T&M jobs or jobs without quotes, use the actual time & expenses - const amountToInvoice = - props.pricingMethodology === 'fixed_price' && quoteTotal.value > 0 - ? quoteTotal.value - : actualSummary.value.rev - - return amountToInvoice - invoiceTotal.value -}) - -const invoiceButtonText = computed(() => { - if (!xeroConnected.value) { - return 'Login to Xero first' - } - return 'Create Invoice' -}) - -// --- INVOICE MODAL --- -const showInvoiceModal = ref(false) -const selectedInvoiceMode = ref(null) -const invoicePercentInput = ref('') -const invoiceAmountInput = ref('') - -const createInvoice = () => { - selectedInvoiceMode.value = null - invoicePercentInput.value = '' - invoiceAmountInput.value = '' - showInvoiceModal.value = true -} - -function selectInvoiceMode(mode: string) { - selectedInvoiceMode.value = mode - invoicePercentInput.value = '' - invoiceAmountInput.value = '' -} - -async function executeCreateInvoice(mode: string) { - if (!props.jobId || isCreatingInvoice.value) return - - let percent: number | undefined - let amount: number | undefined - - if (mode === 'invoice_percent') { - const pct = parseFloat(invoicePercentInput.value) - if (isNaN(pct) || pct <= 0 || pct > 100) { - toast.error('Please enter a valid percentage (1-100)') - return - } - percent = pct - } - - if (mode === 'invoice_amount') { - const amt = parseFloat(invoiceAmountInput.value) - if (isNaN(amt) || amt <= 0) { - toast.error('Please enter a valid amount') - return - } - amount = amt - } - - showInvoiceModal.value = false - isCreatingInvoice.value = true - - try { - const body = { mode } as XeroInvoiceCreateRequest - if (percent !== undefined) body.percent = percent - if (amount !== undefined) body.amount = amount - - const response = await api.xero_create_invoice_create(body, { - params: { job_id: props.jobId }, - }) - if (!response?.success) { - const msgs = response?.messages?.length ? response.messages : ['Failed to create invoice'] - msgs.forEach((msg: string) => toast.error(msg)) - return - } - toast.success('Invoice created successfully!') - if (response.messages?.length) { - response.messages.forEach((msg: string) => toast.warning(msg)) - } - emit('invoice-created') - await loadInvoices() - } catch (err: unknown) { - let msg = 'Unexpected error while trying to create invoice.' - log('Error creating invoice:', err) - if ((err as AxiosError).isAxiosError) { - const axiosErr = err as AxiosError<{ message: string }> - const errorData = axiosErr.response?.data - if (typeof errorData === 'object' && errorData !== null && 'error' in errorData) { - msg = String((errorData as { error: string }).error) - } - } - toast.error(`Failed to create invoice: ${msg}`) - } finally { - isCreatingInvoice.value = false - } -} - -const goToInvoiceOnXero = (url: string | null | undefined) => { - if (url && url !== '#') { - window.open(url, '_blank') - } else { - toast.error('No online URL available for this invoice.') - } -} - -const deleteInvoiceOnXero = async (invoiceXeroId: string) => { - if (!props.jobId || deletingInvoiceId.value) return - deletingInvoiceId.value = invoiceXeroId - try { - await api.xero_delete_invoice_destroy(undefined, { - params: { job_id: props.jobId }, - queries: { xero_invoice_id: invoiceXeroId }, - }) - toast.success('Invoice deleted successfully!') - emit('invoice-deleted') - await loadInvoices() // Reload invoices after deletion - } catch (err) { - console.error('Error deleting invoice:', err) - toast.error('Failed to delete invoice.') - } finally { - deletingInvoiceId.value = null - } -} - const router = useRouter() const costLines = ref([]) @@ -777,18 +312,6 @@ async function loadCostsSummary() { } } -async function loadInvoices() { - try { - const response = await api.job_jobs_invoices_retrieve({ - params: { job_id: props.jobId }, - }) - // Zodios returns data directly, not wrapped in {success, data} - invoices.value = response.invoices || [] - } catch (error) { - log('Failed to load invoices:', error) - } -} - // Use the smart delete composable const { handleSmartDelete } = useSmartCostLineDelete({ costLines, @@ -899,7 +422,7 @@ async function handleCreateLine(line: CostLine): Promise { const costLineDraftSession = useCostLineDrafts({ costLines, createLine: handleCreateLine }) onMounted(async () => { - await Promise.all([loadStaff(), loadActualCosts(), loadCostsSummary(), loadInvoices()]) + await Promise.all([loadStaff(), loadActualCosts(), loadCostsSummary()]) }) // Use the cost summary composable (simple version for actual) diff --git a/frontend/src/components/job/JobCostAnalysisTab.vue b/frontend/src/components/job/JobCostAnalysisTab.vue deleted file mode 100644 index f8270976c..000000000 --- a/frontend/src/components/job/JobCostAnalysisTab.vue +++ /dev/null @@ -1,359 +0,0 @@ - - - - - diff --git a/frontend/src/components/job/JobFinishTab.vue b/frontend/src/components/job/JobFinishTab.vue new file mode 100644 index 000000000..4a821e3c3 --- /dev/null +++ b/frontend/src/components/job/JobFinishTab.vue @@ -0,0 +1,1095 @@ + + + + + diff --git a/frontend/src/components/job/JobViewTabs.vue b/frontend/src/components/job/JobViewTabs.vue index 203954c77..b79ef77f9 100644 --- a/frontend/src/components/job/JobViewTabs.vue +++ b/frontend/src/components/job/JobViewTabs.vue @@ -65,22 +65,20 @@
    +
    +
    +
    -
    - -
    ({ + finishRetrieveMock: vi.fn(), + invoicesRetrieveMock: vi.fn(), + costsSummaryRetrieveMock: vi.fn(), + createInvoiceMock: vi.fn(), + deleteInvoiceMock: vi.fn(), +})) + +vi.mock('@/api/client', () => ({ + api: { + job_jobs_finish_retrieve: finishRetrieveMock, + job_jobs_invoices_retrieve: invoicesRetrieveMock, + job_jobs_costs_summary_retrieve: costsSummaryRetrieveMock, + job_jobs_finish_partial_update: vi.fn(), + xero_create_invoice_create: createInvoiceMock, + xero_delete_invoice_destroy: deleteInvoiceMock, + }, +})) + +vi.mock('@/composables/useXeroConnection', () => ({ + useXeroConnection: () => ({ xeroConnected: { value: true } }), +})) + +vi.mock('vue-sonner', () => ({ + toast: { error: vi.fn(), success: vi.fn(), warning: vi.fn() }, +})) + +import JobFinishTab from '../JobFinishTab.vue' + +/** + * The backend owns every figure here. These tests assert the component renders + * what the API returned and never recomputes it — the ADR 0020 boundary that + * KAN-323 exists to restore. + */ +const summary = (overrides: Record = {}) => ({ + basis: 'quote', + job_value_excl_gst: 1000, + valid_invoiced_excl_gst: 400, + outstanding_invoiced_incl_gst: 460, + remaining_to_invoice_excl_gst: 600, + remaining_gst: 90, + remaining_to_invoice_incl_gst: 690, + total_to_pay_incl_gst: 1150, + over_invoiced_excl_gst: 0, + ...overrides, +}) + +const checklist = () => ({ + time_entries_complete: false, + materials_complete: false, + customer_approval_confirmed: false, + updated_at: null, + updated_by_name: null, +}) + +const costSummary = () => ({ + estimate: { cost: 500, rev: 800, hours: 10, profitMargin: 60 }, + quote: { cost: 600, rev: 1000, hours: 12, profitMargin: 66 }, + actual: { cost: 550, rev: 900, hours: 11, profitMargin: 63 }, +}) + +// Mounting to document.body is required for the portalled invoice modal, so each +// test must tear its DOM down or the next test finds the previous modal. +let mounted: ReturnType | null = null + +function mountTab(pricingMethodology = 'fixed_price', jobStatus = 'in_progress') { + mounted = mount(JobFinishTab, { + props: { jobId: 'job-1', pricingMethodology, jobStatus }, + attachTo: document.body, + }) + return mounted +} + +function resetDom() { + mounted?.unmount() + mounted = null + document.body.innerHTML = '' +} + +const text = (wrapper: ReturnType, id: string) => + wrapper.find(`[data-automation-id="${id}"]`).text() + +// The invoice modal renders through a reka-ui portal, so it lands on document.body +// rather than inside the mounted wrapper. +const inModal = (id: string) => + document.body.querySelector(`[data-automation-id="${id}"]`) + +const modalText = (id: string) => { + const el = inModal(id) + if (!el) throw new Error(`${id} is not in the modal`) + return el.textContent ?? '' +} + +async function openInvoiceModal(wrapper: ReturnType) { + await wrapper.find('[data-automation-id="JobFinishTab-create-invoice"]').trigger('click') + await flushPromises() +} + +describe('JobFinishTab customer balance', () => { + beforeEach(() => { + vi.clearAllMocks() + finishRetrieveMock.mockResolvedValue({ summary: summary(), checklist: checklist() }) + invoicesRetrieveMock.mockResolvedValue({ invoices: [] }) + costsSummaryRetrieveMock.mockResolvedValue(costSummary()) + }) + + afterEach(resetDom) + + it('renders the subtotal, GST and Total to pay returned by the API', async () => { + const wrapper = mountTab() + await flushPromises() + + expect(text(wrapper, 'JobFinishTab-remaining-excl-gst')).toContain('600') + expect(text(wrapper, 'JobFinishTab-remaining-gst')).toContain('90') + expect(text(wrapper, 'JobFinishTab-total-to-pay')).toContain('1,150') + }) + + it('does not derive Total to pay from the other figures', async () => { + // A deliberately inconsistent payload: if the component recomputed the + // total it would show 1,290 rather than the server's answer. + finishRetrieveMock.mockResolvedValue({ + summary: summary({ total_to_pay_incl_gst: 42 }), + checklist: checklist(), + }) + const wrapper = mountTab() + await flushPromises() + + expect(text(wrapper, 'JobFinishTab-total-to-pay')).toContain('42') + }) + + it('shows the outstanding invoice balance when Xero is still owed money', async () => { + const wrapper = mountTab() + await flushPromises() + + expect(text(wrapper, 'JobFinishTab-outstanding-incl-gst')).toContain('460') + }) + + it('hides the outstanding line when every invoice is paid', async () => { + finishRetrieveMock.mockResolvedValue({ + summary: summary({ outstanding_invoiced_incl_gst: 0 }), + checklist: checklist(), + }) + const wrapper = mountTab() + await flushPromises() + + expect(wrapper.find('[data-automation-id="JobFinishTab-outstanding-incl-gst"]').exists()).toBe( + false, + ) + }) + + it('offers no invoice action once the job is fully covered', async () => { + finishRetrieveMock.mockResolvedValue({ + summary: summary({ + valid_invoiced_excl_gst: 1000, + remaining_to_invoice_excl_gst: 0, + remaining_gst: 0, + remaining_to_invoice_incl_gst: 0, + total_to_pay_incl_gst: 0, + }), + checklist: checklist(), + }) + const wrapper = mountTab() + await flushPromises() + + expect(wrapper.find('[data-automation-id="JobFinishTab-create-invoice"]').exists()).toBe(false) + expect(wrapper.find('[data-automation-id="JobFinishTab-fully-invoiced"]').exists()).toBe(true) + expect(text(wrapper, 'JobFinishTab-nothing-to-pay')).toContain('Nothing left to pay') + }) + + it('reports an over-invoiced job instead of a negative balance', async () => { + finishRetrieveMock.mockResolvedValue({ + summary: summary({ + job_value_excl_gst: 800, + valid_invoiced_excl_gst: 1000, + remaining_to_invoice_excl_gst: 0, + remaining_gst: 0, + remaining_to_invoice_incl_gst: 0, + outstanding_invoiced_incl_gst: 0, + total_to_pay_incl_gst: 0, + over_invoiced_excl_gst: 200, + }), + checklist: checklist(), + }) + const wrapper = mountTab() + await flushPromises() + + expect(text(wrapper, 'JobFinishTab-over-invoiced')).toContain('200') + }) + + it('re-reads the balance from the server after creating an invoice', async () => { + createInvoiceMock.mockResolvedValue({ success: true, messages: [] }) + const wrapper = mountTab() + await flushPromises() + expect(finishRetrieveMock).toHaveBeenCalledTimes(1) + + await openInvoiceModal(wrapper) + inModal('JobFinishTab-mode-invoice-full')!.click() + await flushPromises() + + expect(createInvoiceMock).toHaveBeenCalled() + expect(finishRetrieveMock).toHaveBeenCalledTimes(2) + }) + + it('re-reads the balance from the server after deleting an invoice', async () => { + invoicesRetrieveMock.mockResolvedValue({ + invoices: [ + { + id: 'inv-1', + xero_id: 'xero-1', + number: 'INV-001', + status: 'AUTHORISED', + date: '2026-08-01', + total_excl_tax: 400, + online_url: 'https://xero.example/inv-1', + }, + ], + }) + deleteInvoiceMock.mockResolvedValue({}) + const wrapper = mountTab() + await flushPromises() + + await wrapper + .findAll('button') + .filter((b) => b.classes().join(' ').includes('h-7')) + .at(1)! + .trigger('click') + await flushPromises() + + expect(deleteInvoiceMock).toHaveBeenCalled() + expect(finishRetrieveMock).toHaveBeenCalledTimes(2) + }) +}) + +describe('JobFinishTab invoice modes', () => { + beforeEach(() => { + vi.clearAllMocks() + finishRetrieveMock.mockResolvedValue({ summary: summary(), checklist: checklist() }) + invoicesRetrieveMock.mockResolvedValue({ invoices: [] }) + costsSummaryRetrieveMock.mockResolvedValue(costSummary()) + }) + + afterEach(resetDom) + + it('keeps all three fixed-price modes available', async () => { + const wrapper = mountTab('fixed_price') + await flushPromises() + await openInvoiceModal(wrapper) + + expect(inModal('JobFinishTab-mode-invoice-full')).not.toBeNull() + expect(inModal('JobFinishTab-mode-invoice-percent')).not.toBeNull() + expect(inModal('JobFinishTab-mode-invoice-amount')).not.toBeNull() + }) + + it('keeps both T&M modes available', async () => { + const wrapper = mountTab('time_materials') + await flushPromises() + await openInvoiceModal(wrapper) + + expect(inModal('JobFinishTab-mode-invoice-costs-to-date')).not.toBeNull() + expect(inModal('JobFinishTab-mode-invoice-amount')).not.toBeNull() + expect(inModal('JobFinishTab-mode-invoice-percent')).toBeNull() + }) + + it('offers invoicing before the work is complete', async () => { + const wrapper = mountTab('fixed_price', 'approved') + await flushPromises() + + expect(wrapper.find('[data-automation-id="JobFinishTab-create-invoice"]').exists()).toBe(true) + await openInvoiceModal(wrapper) + expect(modalText('JobFinishTab-mode-invoice-full')).toContain('in advance') + }) + + it('describes the remaining balance once the work is complete', async () => { + const wrapper = mountTab('fixed_price', 'recently_completed') + await flushPromises() + await openInvoiceModal(wrapper) + + expect(modalText('JobFinishTab-mode-invoice-full')).toContain('remaining quote balance') + }) +}) diff --git a/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts b/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts new file mode 100644 index 000000000..3250c024b --- /dev/null +++ b/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts @@ -0,0 +1,197 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { mount, flushPromises } from '@vue/test-utils' + +const { + finishRetrieveMock, + invoicesRetrieveMock, + costsSummaryRetrieveMock, + checklistUpdateMock, + toastErrorMock, +} = vi.hoisted(() => ({ + finishRetrieveMock: vi.fn(), + invoicesRetrieveMock: vi.fn(), + costsSummaryRetrieveMock: vi.fn(), + checklistUpdateMock: vi.fn(), + toastErrorMock: vi.fn(), +})) + +vi.mock('@/api/client', () => ({ + api: { + job_jobs_finish_retrieve: finishRetrieveMock, + job_jobs_invoices_retrieve: invoicesRetrieveMock, + job_jobs_costs_summary_retrieve: costsSummaryRetrieveMock, + job_jobs_finish_partial_update: checklistUpdateMock, + xero_create_invoice_create: vi.fn(), + xero_delete_invoice_destroy: vi.fn(), + }, +})) + +vi.mock('@/composables/useXeroConnection', () => ({ + useXeroConnection: () => ({ xeroConnected: { value: true } }), +})) + +vi.mock('vue-sonner', () => ({ + toast: { error: toastErrorMock, success: vi.fn(), warning: vi.fn() }, +})) + +import JobFinishTab from '../JobFinishTab.vue' + +const summary = () => ({ + basis: 'quote', + job_value_excl_gst: 1000, + valid_invoiced_excl_gst: 0, + outstanding_invoiced_incl_gst: 0, + remaining_to_invoice_excl_gst: 1000, + remaining_gst: 150, + remaining_to_invoice_incl_gst: 1150, + total_to_pay_incl_gst: 1150, + over_invoiced_excl_gst: 0, +}) + +const checklist = (overrides: Record = {}) => ({ + time_entries_complete: false, + materials_complete: false, + customer_approval_confirmed: false, + updated_at: null, + updated_by_name: null, + ...overrides, +}) + +let mounted: ReturnType | null = null + +function mountTab(pricingMethodology: string) { + mounted = mount(JobFinishTab, { + props: { jobId: 'job-1', pricingMethodology, jobStatus: 'in_progress' }, + attachTo: document.body, + }) + return mounted +} + +const item = (wrapper: ReturnType, key: string) => + wrapper.find(`[data-automation-id="JobFinishTab-checklist-${key}"]`) + +describe('JobFinishTab completion checklist', () => { + beforeEach(() => { + vi.clearAllMocks() + finishRetrieveMock.mockResolvedValue({ summary: summary(), checklist: checklist() }) + invoicesRetrieveMock.mockResolvedValue({ invoices: [] }) + costsSummaryRetrieveMock.mockResolvedValue({ + estimate: { cost: 500, rev: 800, hours: 10, profitMargin: 60 }, + quote: { cost: 600, rev: 1000, hours: 12, profitMargin: 66 }, + actual: { cost: 550, rev: 900, hours: 11, profitMargin: 63 }, + }) + }) + + afterEach(() => { + mounted?.unmount() + mounted = null + document.body.innerHTML = '' + }) + + it('asks a T&M job to confirm time, materials and customer approval', async () => { + const wrapper = mountTab('time_materials') + await flushPromises() + + expect(item(wrapper, 'time_entries_complete').exists()).toBe(true) + expect(item(wrapper, 'materials_complete').exists()).toBe(true) + expect(item(wrapper, 'customer_approval_confirmed').exists()).toBe(true) + }) + + it('asks a fixed-price job only for customer approval, and shows the quote basis', async () => { + const wrapper = mountTab('fixed_price') + await flushPromises() + + expect(item(wrapper, 'time_entries_complete').exists()).toBe(false) + expect(item(wrapper, 'materials_complete').exists()).toBe(false) + expect(item(wrapper, 'customer_approval_confirmed').exists()).toBe(true) + expect(wrapper.find('[data-automation-id="JobFinishTab-quote-basis"]').text()).toContain( + '1,000', + ) + }) + + it('reflects confirmations already recorded on the job', async () => { + finishRetrieveMock.mockResolvedValue({ + summary: summary(), + checklist: checklist({ materials_complete: true }), + }) + const wrapper = mountTab('time_materials') + await flushPromises() + + expect((item(wrapper, 'materials_complete').element as HTMLInputElement).checked).toBe(true) + expect((item(wrapper, 'time_entries_complete').element as HTMLInputElement).checked).toBe(false) + }) + + it('sends only the item that changed', async () => { + checklistUpdateMock.mockResolvedValue({ + summary: summary(), + checklist: checklist({ materials_complete: true }), + }) + const wrapper = mountTab('time_materials') + await flushPromises() + + await item(wrapper, 'materials_complete').setValue(true) + await flushPromises() + + expect(checklistUpdateMock).toHaveBeenCalledWith( + { materials_complete: true }, + { params: { job_id: 'job-1' } }, + ) + }) + + it('sends false when a confirmation is withdrawn', async () => { + finishRetrieveMock.mockResolvedValue({ + summary: summary(), + checklist: checklist({ materials_complete: true }), + }) + checklistUpdateMock.mockResolvedValue({ summary: summary(), checklist: checklist() }) + const wrapper = mountTab('time_materials') + await flushPromises() + + await item(wrapper, 'materials_complete').setValue(false) + await flushPromises() + + expect(checklistUpdateMock).toHaveBeenCalledWith( + { materials_complete: false }, + { params: { job_id: 'job-1' } }, + ) + }) + + it('reverts the box and warns the user when the save fails', async () => { + checklistUpdateMock.mockRejectedValue(new Error('nope')) + const wrapper = mountTab('time_materials') + await flushPromises() + + await item(wrapper, 'materials_complete').setValue(true) + await flushPromises() + + expect(toastErrorMock).toHaveBeenCalledWith('Failed to save checklist') + expect((item(wrapper, 'materials_complete').element as HTMLInputElement).checked).toBe(false) + }) + + it('does not hide the invoice action behind the checklist', async () => { + const wrapper = mountTab('time_materials') + await flushPromises() + + expect((item(wrapper, 'customer_approval_confirmed').element as HTMLInputElement).checked).toBe( + false, + ) + expect(wrapper.find('[data-automation-id="JobFinishTab-create-invoice"]').exists()).toBe(true) + }) + + it('names who last changed a confirmation', async () => { + finishRetrieveMock.mockResolvedValue({ + summary: summary(), + checklist: checklist({ + customer_approval_confirmed: true, + updated_by_name: 'Office Person', + updated_at: '2026-08-01T09:00:00Z', + }), + }) + const wrapper = mountTab('time_materials') + await flushPromises() + + expect(wrapper.find('[data-automation-id="JobFinishTab-checklist"]').text()).toContain( + 'Office Person', + ) + }) +}) diff --git a/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts b/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts new file mode 100644 index 000000000..54dde9b07 --- /dev/null +++ b/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts @@ -0,0 +1,148 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { mount, flushPromises } from '@vue/test-utils' + +const { finishRetrieveMock, invoicesRetrieveMock, costsSummaryRetrieveMock } = vi.hoisted(() => ({ + finishRetrieveMock: vi.fn(), + invoicesRetrieveMock: vi.fn(), + costsSummaryRetrieveMock: vi.fn(), +})) + +vi.mock('@/api/client', () => ({ + api: { + job_jobs_finish_retrieve: finishRetrieveMock, + job_jobs_invoices_retrieve: invoicesRetrieveMock, + job_jobs_costs_summary_retrieve: costsSummaryRetrieveMock, + job_jobs_finish_partial_update: vi.fn(), + xero_create_invoice_create: vi.fn(), + xero_delete_invoice_destroy: vi.fn(), + }, +})) + +vi.mock('@/composables/useXeroConnection', () => ({ + useXeroConnection: () => ({ xeroConnected: { value: true } }), +})) + +vi.mock('vue-sonner', () => ({ + toast: { error: vi.fn(), success: vi.fn(), warning: vi.fn() }, +})) + +import JobFinishTab from '../JobFinishTab.vue' + +/** + * KAN-222: office staff read labour budget, hours used and hours remaining + * without doing the subtraction, and an overrun shows as a positive number + * rather than a negative remainder. + */ +const summary = () => ({ + basis: 'quote', + job_value_excl_gst: 1000, + valid_invoiced_excl_gst: 0, + outstanding_invoiced_incl_gst: 0, + remaining_to_invoice_excl_gst: 1000, + remaining_gst: 150, + remaining_to_invoice_incl_gst: 1150, + total_to_pay_incl_gst: 1150, + over_invoiced_excl_gst: 0, +}) + +const checklist = () => ({ + time_entries_complete: false, + materials_complete: false, + customer_approval_confirmed: false, + updated_at: null, + updated_by_name: null, +}) + +let mounted: ReturnType | null = null + +function mountWithHours( + { + estimateHours, + quoteHours, + actualHours, + }: { estimateHours: number; quoteHours: number; actualHours: number }, + pricingMethodology = 'fixed_price', +) { + costsSummaryRetrieveMock.mockResolvedValue({ + estimate: { cost: 500, rev: 800, hours: estimateHours, profitMargin: 60 }, + quote: quoteHours > 0 ? { cost: 600, rev: 1000, hours: quoteHours, profitMargin: 66 } : null, + actual: { cost: 550, rev: 900, hours: actualHours, profitMargin: 63 }, + }) + mounted = mount(JobFinishTab, { + props: { jobId: 'job-1', pricingMethodology, jobStatus: 'in_progress' }, + attachTo: document.body, + }) + return mounted +} + +const hours = (wrapper: ReturnType, which: string) => + wrapper.find(`[data-automation-id="JobFinishTab-labour-${which}"]`).text() + +const remainingLabel = (wrapper: ReturnType) => + wrapper.find('[data-automation-id="JobFinishTab-labour-hours"]').text() + +describe('JobFinishTab labour hours', () => { + beforeEach(() => { + vi.clearAllMocks() + finishRetrieveMock.mockResolvedValue({ summary: summary(), checklist: checklist() }) + invoicesRetrieveMock.mockResolvedValue({ invoices: [] }) + }) + + afterEach(() => { + mounted?.unmount() + mounted = null + document.body.innerHTML = '' + }) + + it('shows budget, used and remaining for an under-budget job', async () => { + const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 12, actualHours: 8 }) + await flushPromises() + + expect(hours(wrapper, 'budget')).toBe('12') + expect(hours(wrapper, 'used')).toBe('8') + expect(hours(wrapper, 'remaining')).toBe('4') + expect(remainingLabel(wrapper)).toContain('Hours remaining') + }) + + it('shows zero remaining and no overrun exactly on budget', async () => { + const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 12, actualHours: 12 }) + await flushPromises() + + expect(hours(wrapper, 'remaining')).toBe('0') + expect(remainingLabel(wrapper)).toContain('Hours remaining') + expect(remainingLabel(wrapper)).not.toContain('Overrun') + }) + + it('shows an overrun as a positive number, not a negative remainder', async () => { + const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 12, actualHours: 15 }) + await flushPromises() + + expect(hours(wrapper, 'remaining')).toBe('3') + expect(remainingLabel(wrapper)).toContain('Overrun') + expect(hours(wrapper, 'remaining')).not.toContain('-') + }) + + it('budgets a fixed-price job from its quote hours', async () => { + const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 12, actualHours: 5 }) + await flushPromises() + + expect(hours(wrapper, 'budget')).toBe('12') + }) + + it('budgets a fixed-price job with no quote from its estimate hours', async () => { + const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 0, actualHours: 5 }) + await flushPromises() + + expect(hours(wrapper, 'budget')).toBe('10') + }) + + it('budgets a T&M job from its estimate hours', async () => { + const wrapper = mountWithHours( + { estimateHours: 10, quoteHours: 12, actualHours: 5 }, + 'time_materials', + ) + await flushPromises() + + expect(hours(wrapper, 'budget')).toBe('10') + }) +}) diff --git a/frontend/src/constants/job-tabs.ts b/frontend/src/constants/job-tabs.ts index ce19afadf..b6403253f 100644 --- a/frontend/src/constants/job-tabs.ts +++ b/frontend/src/constants/job-tabs.ts @@ -9,7 +9,7 @@ export const JobTabsSchema = z.enum([ 'quote', 'actual', 'financial', - 'costAnalysis', + 'finishJob', 'jobSettings', 'history', 'attachments', diff --git a/frontend/tests/job/job-xero-invoice.spec.ts b/frontend/tests/job/job-xero-invoice.spec.ts index 63e9da3d9..31d2abbde 100644 --- a/frontend/tests/job/job-xero-invoice.spec.ts +++ b/frontend/tests/job/job-xero-invoice.spec.ts @@ -12,7 +12,7 @@ const getJobIdFromUrl = (url: string): string => { test.describe('job xero invoice', () => { test.setTimeout(120000) - test('create invoice in Xero from Actual tab', async ({ + test('invoice a job from Finish Job and see the balance settle', async ({ authenticatedPage: page, sharedEditJobUrl, }) => { @@ -21,12 +21,18 @@ test.describe('job xero invoice', () => { await page.goto(sharedEditJobUrl) await page.waitForLoadState('networkidle') - await autoId(page, 'JobViewTabs-actual').click() + await autoId(page, 'JobViewTabs-finishJob').click() + + // The balance is server-owned; the tab must render it before offering to invoice. + await expect(autoId(page, 'JobFinishTab-total-to-pay')).toBeVisible({ timeout: 10000 }) + const remaining = autoId(page, 'JobFinishTab-remaining-excl-gst') + await expect(remaining).toBeVisible() + await expect(remaining).not.toHaveText(/\$0\.00/) const invoiceItems = page.locator('ul[role="list"] li') const initialCount = await invoiceItems.count() - const createInvoiceButton = page.getByRole('button', { name: /Create .*Invoice/ }) + const createInvoiceButton = autoId(page, 'JobFinishTab-create-invoice') await expect(createInvoiceButton).toBeVisible({ timeout: 10000 }) const responsePromise = page.waitForResponse( @@ -41,12 +47,9 @@ test.describe('job xero invoice', () => { await createInvoiceButton.click() - // Modal should appear — select the primary invoice mode - const invoiceRemainingButton = page.getByRole('button', { - name: /Invoice remaining quote/, - }) - await expect(invoiceRemainingButton).toBeVisible({ timeout: 10000 }) - await invoiceRemainingButton.click() + const invoiceFullButton = autoId(page, 'JobFinishTab-mode-invoice-full') + await expect(invoiceFullButton).toBeVisible({ timeout: 10000 }) + await invoiceFullButton.click() const response = await responsePromise if (!response.ok()) { @@ -57,5 +60,11 @@ test.describe('job xero invoice', () => { } await expect(invoiceItems).toHaveCount(initialCount + 1, { timeout: 20000 }) + + // KAN-323: a fully invoiced job shows nothing remaining and stops asking for + // another invoice, rather than offering a ceremonial $0 one. + await expect(remaining).toHaveText(/\$0\.00/, { timeout: 20000 }) + await expect(autoId(page, 'JobFinishTab-fully-invoiced')).toBeVisible() + await expect(createInvoiceButton).toHaveCount(0) }) }) From b6584c9c51df4d677ff367539b10f30690103848 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sat, 1 Aug 2026 10:09:48 +1200 Subject: [PATCH 4/8] fix: stack the Finish Job labour tiles on narrow screens A fixed three-column grid put the labour budget, used and remaining figures into roughly 100px each on a phone, which is where staff read them while standing at the job. They stack below the sm breakpoint now. KAN-323 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH --- frontend/src/components/job/JobFinishTab.vue | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frontend/src/components/job/JobFinishTab.vue b/frontend/src/components/job/JobFinishTab.vue index 4a821e3c3..b4aed1eb1 100644 --- a/frontend/src/components/job/JobFinishTab.vue +++ b/frontend/src/components/job/JobFinishTab.vue @@ -288,7 +288,10 @@
    -
    +
    Labour budget
    Date: Sat, 1 Aug 2026 17:10:59 +1200 Subject: [PATCH 5/8] refactor: front-desk checklist as Job fields, not its own table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The completion checklist shipped as a separate JobCompletionChecklist table with a OneToOne back to Job. Being 1:1, that was columns with a join in front of them, and it cost more than the join: a get_completion_checklist() that returned an unsaved instance for jobs with no row yet, updated_at/updated_by columns that recorded who last touched the checklist rather than who ticked what, and a service that hand-wrote a JobEvent per item. Job already does all of that. _FIELD_HANDLERS turns any tracked field change into a job-history event, which is where "Marked as paid" comes from. So the items are Job booleans now, the table and its service are gone, and the per-item audit is the machinery that was already there. The items also changed to the questions the front desk actually works through: foreman signed off, timesheets collected, materials checked, customer called, released. Same five on every job — urgency differs between T&M and quoted work, but that is a note under the list, not a branch in the schema, so the conditional rendering goes too. collected is renamed to released rather than replaced. It meant the same thing and was never writable — no serializer, endpoint, admin or UI ever set it — so every row holds the default and the rename carries nothing stale forward. This also drops customer_approval_confirmed: a customer is not handed work they have just rejected, so the tick is implied by release for a collection, and for a delivery it cannot be answered at the moment it would be ticked. Migration 0010 is replaced rather than added to, since nothing has been pushed. KAN-323 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH --- .../0010_job_completion_checklist.py | 66 ---- .../0010_job_completion_checklist_fields.py | 53 +++ apps/job/models/__init__.py | 2 - apps/job/models/job.py | 41 ++- apps/job/models/job_completion_checklist.py | 64 ---- apps/job/models/job_event.py | 35 +- apps/job/serializers/job_serializer.py | 41 +-- apps/job/services/__init__.py | 8 - .../job_completion_checklist_service.py | 97 ------ apps/job/tests/test_completion_checklist.py | 321 +++++------------- apps/job/tests/test_job_event_tracking.py | 28 +- apps/job/views/job_rest_views.py | 45 ++- frontend/schema.yml | 48 +-- frontend/src/api/generated/api.ts | 18 +- frontend/src/components/job/JobFinishTab.vue | 47 ++- .../__tests__/JobFinishTab.balance.test.ts | 10 +- .../__tests__/JobFinishTab.checklist.test.ts | 114 ++++--- .../JobFinishTab.labourHours.test.ts | 10 +- mypy-baseline.txt | 1 - 19 files changed, 405 insertions(+), 644 deletions(-) delete mode 100644 apps/job/migrations/0010_job_completion_checklist.py create mode 100644 apps/job/migrations/0010_job_completion_checklist_fields.py delete mode 100644 apps/job/models/job_completion_checklist.py delete mode 100644 apps/job/services/job_completion_checklist_service.py diff --git a/apps/job/migrations/0010_job_completion_checklist.py b/apps/job/migrations/0010_job_completion_checklist.py deleted file mode 100644 index 71af30bbf..000000000 --- a/apps/job/migrations/0010_job_completion_checklist.py +++ /dev/null @@ -1,66 +0,0 @@ -# Generated by Django 6.0.7 on 2026-07-31 21:32 - -import uuid - -import django.db.models.deletion -from django.conf import settings -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ("job", "0009_text_unset_constraints"), - migrations.swappable_dependency(settings.AUTH_USER_MODEL), - ] - - operations = [ - migrations.CreateModel( - name="JobCompletionChecklist", - fields=[ - ( - "id", - models.UUIDField( - default=uuid.uuid4, - editable=False, - primary_key=True, - serialize=False, - ), - ), - ("time_entries_complete", models.BooleanField(default=False)), - ("materials_complete", models.BooleanField(default=False)), - ("customer_approval_confirmed", models.BooleanField(default=False)), - ( - "updated_at", - models.DateTimeField( - blank=True, - help_text="When an item was last changed; NULL until the first change.", - null=True, - ), - ), - ( - "job", - models.OneToOneField( - on_delete=django.db.models.deletion.CASCADE, - related_name="completion_checklist", - to="job.job", - ), - ), - ( - "updated_by", - models.ForeignKey( - blank=True, - help_text="Who last changed an item; NULL until the first change.", - null=True, - on_delete=django.db.models.deletion.PROTECT, - related_name="+", - to=settings.AUTH_USER_MODEL, - ), - ), - ], - options={ - "verbose_name": "Job Completion Checklist", - "verbose_name_plural": "Job Completion Checklists", - }, - ), - ] diff --git a/apps/job/migrations/0010_job_completion_checklist_fields.py b/apps/job/migrations/0010_job_completion_checklist_fields.py new file mode 100644 index 000000000..99b380680 --- /dev/null +++ b/apps/job/migrations/0010_job_completion_checklist_fields.py @@ -0,0 +1,53 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + """Front-desk completion checklist as Job fields. + + ``collected`` is renamed rather than replaced: it meant the same thing and was + never writable (no serializer, endpoint, admin or UI ever set it), so every + row holds the default and the rename carries no stale data forward. + """ + + dependencies = [ + ("job", "0009_text_unset_constraints"), + ] + + operations = [ + migrations.RenameField( + model_name="job", + old_name="collected", + new_name="released", + ), + migrations.AlterField( + model_name="job", + name="released", + field=models.BooleanField( + default=False, + help_text=( + "The job has left our possession, by collection, delivery or " + "on-site install." + ), + ), + ), + migrations.AddField( + model_name="job", + name="foreman_signed_off", + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name="job", + name="timesheets_collected", + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name="job", + name="materials_checked", + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name="job", + name="customer_called", + field=models.BooleanField(default=False), + ), + ] diff --git a/apps/job/models/__init__.py b/apps/job/models/__init__.py index b92540367..fa3673ec3 100644 --- a/apps/job/models/__init__.py +++ b/apps/job/models/__init__.py @@ -2,7 +2,6 @@ from .costing import CostLine, CostSet from .job import Job -from .job_completion_checklist import JobCompletionChecklist from .job_delta_rejection import JobDeltaRejection from .job_event import JobEvent from .job_file import JobFile @@ -14,7 +13,6 @@ "CostLine", "CostSet", "Job", - "JobCompletionChecklist", "JobDeltaRejection", "JobEvent", "JobFile", diff --git a/apps/job/models/job.py b/apps/job/models/job.py index 66fed6ab1..676cc5f3a 100644 --- a/apps/job/models/job.py +++ b/apps/job/models/job.py @@ -307,8 +307,19 @@ class Job(models.Model): # Shop job has no company (company_id is None) job_is_valid = models.BooleanField(default=False) - collected: bool = models.BooleanField(default=False) paid: bool = models.BooleanField(default=False) + + # Front-desk completion checklist. Each is a fact only a person knows, so + # none of them can be derived. Ticking one writes a job-history event through + # _FIELD_HANDLERS below; nothing here blocks invoicing or moves the job. + foreman_signed_off = models.BooleanField(default=False) + timesheets_collected = models.BooleanField(default=False) + materials_checked = models.BooleanField(default=False) + customer_called = models.BooleanField(default=False) + released = models.BooleanField( + default=False, + help_text="The job has left our possession, by collection, delivery or on-site install.", + ) fully_invoiced: bool = models.BooleanField( default=False, help_text="The total value of invoices for this job matches the total value of the job.", @@ -1146,10 +1157,30 @@ def _handle_rdti_type_change(_self_job, old_type, new_type): "payment_updated", "Paid", ), - "collected": _handle_boolean_change( - "job_collected", - "collection_updated", - "Collected", + "foreman_signed_off": _handle_boolean_change( + "completion_checklist_updated", + "completion_checklist_updated", + "Foreman sign-off", + ), + "timesheets_collected": _handle_boolean_change( + "completion_checklist_updated", + "completion_checklist_updated", + "Timesheets collected", + ), + "materials_checked": _handle_boolean_change( + "completion_checklist_updated", + "completion_checklist_updated", + "Materials checked", + ), + "customer_called": _handle_boolean_change( + "completion_checklist_updated", + "completion_checklist_updated", + "Customer called", + ), + "released": _handle_boolean_change( + "completion_checklist_updated", + "completion_checklist_updated", + "Job released", ), "complex_job": _handle_boolean_change( "job_updated", diff --git a/apps/job/models/job_completion_checklist.py b/apps/job/models/job_completion_checklist.py deleted file mode 100644 index 58e4aa2bd..000000000 --- a/apps/job/models/job_completion_checklist.py +++ /dev/null @@ -1,64 +0,0 @@ -import uuid - -from django.db import models - -from apps.accounts.models import Staff - - -class JobCompletionChecklist(models.Model): - """Staff confirmations recorded while finishing a job. - - Deliberately advisory: nothing here blocks invoicing, changes job status, - creates tasks, or nags. Its only job is to remember what a staff member - confirmed, and to leave an audit trail when a confirmation is withdrawn. - - ``updated_at`` and ``updated_by`` are NULL until the first confirmation is - recorded — a row can exist with nothing yet confirmed. - """ - - # The confirmable items, in display order. Serializers and the update - # service derive their accepted keys from this tuple, so adding an item here - # is the only change needed to expose it. - ITEM_FIELDS = ( - "time_entries_complete", - "materials_complete", - "customer_approval_confirmed", - ) - - # Item key → the label used in job-history events. - ITEM_LABELS = { - "time_entries_complete": "All time entered", - "materials_complete": "All materials entered", - "customer_approval_confirmed": "Customer approval confirmed", - } - - id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) - job = models.OneToOneField( - "job.Job", - on_delete=models.CASCADE, - related_name="completion_checklist", - ) - time_entries_complete = models.BooleanField(default=False) - materials_complete = models.BooleanField(default=False) - customer_approval_confirmed = models.BooleanField(default=False) - updated_at = models.DateTimeField( - null=True, - blank=True, - help_text="When an item was last changed; NULL until the first change.", - ) - updated_by = models.ForeignKey( - Staff, - on_delete=models.PROTECT, - null=True, - blank=True, - related_name="+", - help_text="Who last changed an item; NULL until the first change.", - ) - - class Meta: - verbose_name = "Job Completion Checklist" - verbose_name_plural = "Job Completion Checklists" - - def __str__(self) -> str: - confirmed = sum(1 for field in self.ITEM_FIELDS if getattr(self, field)) - return f"Checklist for {self.job.name}: {confirmed}/{len(self.ITEM_FIELDS)}" diff --git a/apps/job/models/job_event.py b/apps/job/models/job_event.py index 4d204a4f0..32fa55e00 100644 --- a/apps/job/models/job_event.py +++ b/apps/job/models/job_event.py @@ -49,20 +49,20 @@ def _truncate_change(label: str, old, new) -> str: def _completion_confirmation_descriptor( - subject: str, + confirmed: str, withdrawn: str ) -> Callable[[str, str], str]: - """Descriptor factory for Finish Job checklist confirmations. + """Descriptor factory for front-desk checklist items. - A withdrawn confirmation reads as plainly as a granted one, since withdrawing - is the change most worth being able to find later. Values are the "Yes"/"No" - strings written by the checklist service. + Both wordings are given explicitly rather than built from one subject: an + unticking is the change most worth finding later, so it earns a sentence that + reads properly instead of a negated one that does not. """ def descriptor(old: str, new: str) -> str: if _truthy(new): - return f"Confirmed {subject}" + return confirmed else: - return f"Withdrew confirmation of {subject}" + return withdrawn return descriptor @@ -85,16 +85,21 @@ def _quote_acceptance_descriptor(old, new) -> str: "Marked as complex job" if _truthy(new) else "Unmarked as complex job" ), "Paid": lambda old, new: ("Marked as paid" if _truthy(new) else "Marked as unpaid"), - "Collected": lambda old, new: ( - "Marked as collected" if _truthy(new) else "Marked as not collected" - ), "Quote acceptance date": _quote_acceptance_descriptor, - "All time entered": _completion_confirmation_descriptor("all time entered"), - "All materials entered": _completion_confirmation_descriptor( - "all materials entered" + "Foreman sign-off": _completion_confirmation_descriptor( + "Foreman signed the job off", "Foreman sign-off withdrawn" + ), + "Timesheets collected": _completion_confirmation_descriptor( + "Timesheets collected from the workshop", "Timesheet collection unconfirmed" + ), + "Materials checked": _completion_confirmation_descriptor( + "Materials checked on the job", "Materials check unconfirmed" + ), + "Customer called": _completion_confirmation_descriptor( + "Customer called", "Customer call unconfirmed" ), - "Customer approval confirmed": _completion_confirmation_descriptor( - "customer approval" + "Job released": _completion_confirmation_descriptor( + "Job released", "Job release withdrawn" ), "Internal notes": lambda old, new: _truncate_change("Notes", old, new), "Job description": lambda old, new: _truncate_change("Description", old, new), diff --git a/apps/job/serializers/job_serializer.py b/apps/job/serializers/job_serializer.py index 31a027214..9713f08cd 100644 --- a/apps/job/serializers/job_serializer.py +++ b/apps/job/serializers/job_serializer.py @@ -10,7 +10,7 @@ from apps.accounting.models.quote import Quote from apps.accounting.services.finish_job_summary import FinishJobSummary from apps.company.models import Company, Person -from apps.job.models import Job, JobCompletionChecklist, JobEvent, JobFile +from apps.job.models import Job, JobEvent, JobFile from apps.workflow.models import XeroPayItem from apps.workflow.serializers import AppErrorResponseSerializer from apps.workflow.serializers_base import NullUnsetModelSerializer @@ -852,7 +852,7 @@ class FinishJobPayload(TypedDict): """What the Finish Job endpoint serializes: a balance plus a checklist.""" summary: FinishJobSummary - checklist: JobCompletionChecklist + checklist: Job class FinishJobSummarySerializer(serializers.Serializer[FinishJobSummary]): @@ -890,37 +890,32 @@ class FinishJobSummarySerializer(serializers.Serializer[FinishJobSummary]): ) -class JobCompletionChecklistSerializer(serializers.Serializer[JobCompletionChecklist]): - """Read shape for the Finish Job completion checklist. +class JobCompletionChecklistSerializer(serializers.Serializer[Job]): + """Read shape for the front-desk completion checklist. - updated_at and updated_by_name are null until a staff member first changes an - item, including for the unsaved all-false checklist of a job nobody has - confirmed anything on. + The items are Job fields, so each tick is audited by the job's own + field-change machinery. Who ticked what, and when, is in the job history. """ - time_entries_complete = serializers.BooleanField(read_only=True) - materials_complete = serializers.BooleanField(read_only=True) - customer_approval_confirmed = serializers.BooleanField(read_only=True) - updated_at = serializers.DateTimeField(read_only=True, allow_null=True) - updated_by_name = serializers.SerializerMethodField() - - @extend_schema_field(serializers.CharField(allow_null=True)) - def get_updated_by_name(self, obj: JobCompletionChecklist) -> Optional[str]: - if not obj.updated_by: - return None - return obj.updated_by.get_display_full_name() + foreman_signed_off = serializers.BooleanField(read_only=True) + timesheets_collected = serializers.BooleanField(read_only=True) + materials_checked = serializers.BooleanField(read_only=True) + customer_called = serializers.BooleanField(read_only=True) + released = serializers.BooleanField(read_only=True) class JobCompletionChecklistUpdateSerializer(serializers.Serializer[None]): """Partial update shape: send only the items being changed. - Every item is optional, and unknown keys are rejected by the service rather - than dropped, so a client typo is a 400 instead of a silent no-op. + Every item is optional, and unknown keys are rejected by the view rather than + dropped, so a client typo is a 400 instead of a silent no-op. """ - time_entries_complete = serializers.BooleanField(required=False) - materials_complete = serializers.BooleanField(required=False) - customer_approval_confirmed = serializers.BooleanField(required=False) + foreman_signed_off = serializers.BooleanField(required=False) + timesheets_collected = serializers.BooleanField(required=False) + materials_checked = serializers.BooleanField(required=False) + customer_called = serializers.BooleanField(required=False) + released = serializers.BooleanField(required=False) class JobFinishResponseSerializer(serializers.Serializer[FinishJobPayload]): diff --git a/apps/job/services/__init__.py b/apps/job/services/__init__.py index 71e07426f..6eb43df58 100644 --- a/apps/job/services/__init__.py +++ b/apps/job/services/__init__.py @@ -31,11 +31,6 @@ serialize_draft_lines, serialize_validation_report, ) - from .job_completion_checklist_service import ( - ChecklistUpdateError, - get_completion_checklist, - update_completion_checklist, - ) from .job_profitability_report import JobProfitabilityReportService from .job_rest_service import ( DeltaValidationError, @@ -115,7 +110,6 @@ "AutoArchiveService", "ChatFileService", "ChatService", - "ChecklistUpdateError", "ChecksumInput", "DataIntegrityService", "DeltaValidationError", @@ -163,7 +157,6 @@ "format_retail_line_total", "generate_delivery_docket", "get_bill_rate_multiplier", - "get_completion_checklist", "get_job_for_delivery_docket_pdf", "get_job_for_workshop_pdf", "get_job_total_value", @@ -192,6 +185,5 @@ "serialize_validation_report", "sync_job_folder", "to_decimal", - "update_completion_checklist", "wait_until_file_ready", ] diff --git a/apps/job/services/job_completion_checklist_service.py b/apps/job/services/job_completion_checklist_service.py deleted file mode 100644 index 88be54ca9..000000000 --- a/apps/job/services/job_completion_checklist_service.py +++ /dev/null @@ -1,97 +0,0 @@ -"""Reads and partial updates for the Job completion checklist. - -Every accepted change writes one job-history event naming the item, its old and -new value, the acting staff member and the time, so a withdrawn confirmation is -as visible as a granted one. -""" - -from django.db import transaction -from django.utils import timezone - -from apps.accounts.models import Staff -from apps.job.models import Job -from apps.job.models.job_completion_checklist import JobCompletionChecklist -from apps.job.models.job_event import JobEvent - -CHECKLIST_UPDATED_EVENT = "completion_checklist_updated" - - -class ChecklistUpdateError(ValueError): - """Raised when a checklist update names an unknown item or a non-boolean.""" - - -def get_completion_checklist(job: Job) -> JobCompletionChecklist: - """The job's checklist. - - A job nobody has confirmed anything on gets an unsaved all-false checklist - rather than a row written during a read. - """ - checklist = JobCompletionChecklist.objects.filter(job=job).first() - if not checklist: - return JobCompletionChecklist(job=job) - else: - return checklist - - -def update_completion_checklist( - job: Job, updates: dict[str, bool], staff: Staff -) -> JobCompletionChecklist: - """Apply a partial checklist update and audit each item that changed. - - Raises ChecklistUpdateError for unknown item keys or non-boolean values, so a - typo in a client payload is a 400 rather than a silently ignored field. - """ - unknown = sorted(set(updates) - set(JobCompletionChecklist.ITEM_FIELDS)) - if unknown: - raise ChecklistUpdateError( - f"Unknown checklist item(s): {', '.join(unknown)}. " - f"Valid items: {', '.join(JobCompletionChecklist.ITEM_FIELDS)}." - ) - - non_boolean = sorted( - key for key, value in updates.items() if not isinstance(value, bool) - ) - if non_boolean: - raise ChecklistUpdateError( - f"Checklist item(s) must be true or false: {', '.join(non_boolean)}." - ) - - with transaction.atomic(): - checklist, _ = JobCompletionChecklist.objects.get_or_create(job=job) - - changed = { - key: value - for key, value in updates.items() - if getattr(checklist, key) != value - } - if not changed: - return checklist - - for key, new_value in changed.items(): - _record_change(job, staff, key, getattr(checklist, key), new_value) - setattr(checklist, key, new_value) - - checklist.updated_at = timezone.now() - checklist.updated_by = staff - checklist.save() - - return checklist - - -def _record_change( - job: Job, staff: Staff, item: str, old_value: bool, new_value: bool -) -> None: - JobEvent.objects.create( - job=job, - staff=staff, - event_type=CHECKLIST_UPDATED_EVENT, - detail={ - "changes": [ - { - "field_name": JobCompletionChecklist.ITEM_LABELS[item], - "old_value": "Yes" if old_value else "No", - "new_value": "Yes" if new_value else "No", - } - ] - }, - ) diff --git a/apps/job/tests/test_completion_checklist.py b/apps/job/tests/test_completion_checklist.py index ab763abec..adf4f2966 100644 --- a/apps/job/tests/test_completion_checklist.py +++ b/apps/job/tests/test_completion_checklist.py @@ -1,29 +1,28 @@ -"""Tests for the Finish Job completion checklist. +"""Tests for the front-desk completion checklist. -The checklist is advisory by design: these tests pin both that changes are -audited and that ticking a box never affects what a user can invoice or what -status a job is in. +The items are Job fields, so the audit trail comes from the job's own +field-change machinery. These tests pin that each tick is attributed and that +ticking nothing still lets a job be invoiced — the checklist records, it does +not gate. """ from decimal import Decimal from django.urls import reverse from django.utils import timezone +from rest_framework.response import Response from apps.accounting.services.invoice_calculation import calculate_invoice_amount from apps.accounts.models import Staff from apps.company.models import Company -from apps.job.models import Job, JobCompletionChecklist, JobEvent -from apps.job.services.job_completion_checklist_service import ( - CHECKLIST_UPDATED_EVENT, - ChecklistUpdateError, - get_completion_checklist, - update_completion_checklist, -) -from apps.testing import BaseAPITestCase, BaseTestCase +from apps.job.models import Job, JobEvent +from apps.job.views.job_rest_views import CHECKLIST_FIELDS +from apps.testing import BaseAPITestCase +CHECKLIST_EVENT = "completion_checklist_updated" -class TestCompletionChecklistService(BaseTestCase): + +class TestCompletionChecklist(BaseAPITestCase): def setUp(self) -> None: self.client_obj = Company.objects.create( name="Test Company", @@ -35,184 +34,136 @@ def setUp(self) -> None: pricing_methodology="time_materials", ) self.job.save(staff=self.test_staff) + self.url = reverse("jobs:job_finish_rest", args=[self.job.id]) + # Ticking an item is an office action; the shared test_staff is + # deliberately neither office nor workshop. + self.office_staff = Staff.objects.create_user( + email="office@example.com", + password="testpass", + first_name="Office", + last_name="Person", + is_office_staff=True, + ) + self.client.force_login(self.office_staff) + + def _patch(self, payload: dict[str, object]) -> Response: + return self.client.patch(self.url, data=payload, format="json") # --- Reading --- - def test_unconfirmed_job_reads_as_all_false_without_writing_a_row(self) -> None: - checklist = get_completion_checklist(self.job) + def test_a_new_job_has_nothing_ticked(self) -> None: + response = self.client.get(self.url) - self.assertFalse(checklist.time_entries_complete) - self.assertFalse(checklist.materials_complete) - self.assertFalse(checklist.customer_approval_confirmed) - self.assertIsNone(checklist.updated_at) - self.assertIsNone(checklist.updated_by) - self.assertFalse(JobCompletionChecklist.objects.filter(job=self.job).exists()) + self.assertEqual(response.status_code, 200) + for field in CHECKLIST_FIELDS: + self.assertFalse(response.data["checklist"][field], msg=field) - # --- Partial updates --- + def test_every_item_is_exposed(self) -> None: + """The API shape and the field tuple must not drift apart.""" + response = self.client.get(self.url) - def test_update_touches_only_the_named_item(self) -> None: - update_completion_checklist( - self.job, {"materials_complete": True}, self.test_staff - ) + self.assertEqual(set(response.data["checklist"].keys()), set(CHECKLIST_FIELDS)) - checklist = get_completion_checklist(self.job) - self.assertTrue(checklist.materials_complete) - self.assertFalse(checklist.time_entries_complete) - self.assertFalse(checklist.customer_approval_confirmed) + # --- Updating --- - def test_update_records_who_and_when(self) -> None: - checklist = update_completion_checklist( - self.job, {"time_entries_complete": True}, self.test_staff - ) + def test_ticking_one_item_leaves_the_others_alone(self) -> None: + response = self._patch({"materials_checked": True}) - self.assertEqual(checklist.updated_by, self.test_staff) - self.assertIsNotNone(checklist.updated_at) + self.assertEqual(response.status_code, 200) + self.assertTrue(response.data["checklist"]["materials_checked"]) + self.assertFalse(response.data["checklist"]["foreman_signed_off"]) + self.assertFalse(response.data["checklist"]["released"]) - def test_second_update_preserves_the_first(self) -> None: - update_completion_checklist( - self.job, {"time_entries_complete": True}, self.test_staff - ) - update_completion_checklist( - self.job, {"customer_approval_confirmed": True}, self.test_staff - ) + def test_every_item_can_be_ticked(self) -> None: + for field in CHECKLIST_FIELDS: + with self.subTest(field=field): + response = self._patch({field: True}) + self.assertEqual(response.status_code, 200) + self.assertTrue(response.data["checklist"][field]) + + def test_a_tick_can_be_withdrawn(self) -> None: + self._patch({"released": True}) + + response = self._patch({"released": False}) - checklist = get_completion_checklist(self.job) - self.assertTrue(checklist.time_entries_complete) - self.assertTrue(checklist.customer_approval_confirmed) + self.assertFalse(response.data["checklist"]["released"]) def test_unknown_item_is_rejected(self) -> None: - with self.assertRaises(ChecklistUpdateError) as ctx: - update_completion_checklist( - self.job, {"everything_is_fine": True}, self.test_staff - ) + response = self._patch({"everything_is_fine": True}) - self.assertIn("everything_is_fine", str(ctx.exception)) - self.assertFalse(JobCompletionChecklist.objects.filter(job=self.job).exists()) + self.assertEqual(response.status_code, 400) def test_non_boolean_value_is_rejected(self) -> None: - with self.assertRaises(ChecklistUpdateError): - update_completion_checklist( - self.job, - {"materials_complete": "yes"}, # type: ignore[dict-item] # the guard exists for untyped JSON off the wire - self.test_staff, - ) - - def test_rejected_update_does_not_apply_its_valid_items(self) -> None: + response = self._patch({"materials_checked": "yes"}) + + self.assertEqual(response.status_code, 400) + + def test_a_rejected_payload_applies_none_of_it(self) -> None: """An unknown key fails the whole payload rather than half-applying it.""" - with self.assertRaises(ChecklistUpdateError): - update_completion_checklist( - self.job, - {"materials_complete": True, "nonsense": True}, - self.test_staff, - ) - - self.assertFalse(get_completion_checklist(self.job).materials_complete) - - # --- Audit history --- - - def test_each_changed_item_adds_one_history_event(self) -> None: - update_completion_checklist( - self.job, - {"time_entries_complete": True, "materials_complete": True}, - self.test_staff, - ) + response = self._patch({"materials_checked": True, "nonsense": True}) - events = JobEvent.objects.filter( - job=self.job, event_type=CHECKLIST_UPDATED_EVENT - ) - self.assertEqual(events.count(), 2) + self.assertEqual(response.status_code, 400) + self.job.refresh_from_db() + self.assertFalse(self.job.materials_checked) - def test_history_event_records_item_values_and_staff(self) -> None: - update_completion_checklist( - self.job, {"customer_approval_confirmed": True}, self.test_staff - ) + # --- Audit --- - event = JobEvent.objects.get(job=self.job, event_type=CHECKLIST_UPDATED_EVENT) - change = event.detail["changes"][0] - self.assertEqual(change["field_name"], "Customer approval confirmed") - self.assertEqual(change["old_value"], "No") - self.assertEqual(change["new_value"], "Yes") - self.assertEqual(event.staff, self.test_staff) - self.assertIsNotNone(event.timestamp) - - def test_withdrawing_a_confirmation_is_audited(self) -> None: - """The change most worth finding later is someone unticking a box.""" - update_completion_checklist( - self.job, {"materials_complete": True}, self.test_staff - ) - update_completion_checklist( - self.job, {"materials_complete": False}, self.test_staff - ) + def test_each_changed_item_is_attributed_in_job_history(self) -> None: + self._patch({"foreman_signed_off": True}) + + event = JobEvent.objects.filter( + job=self.job, event_type=CHECKLIST_EVENT + ).latest("timestamp") + self.assertEqual(event.staff, self.office_staff) + self.assertEqual(event.description, "Foreman signed the job off") + + def test_withdrawing_a_tick_is_audited(self) -> None: + self._patch({"released": True}) + self._patch({"released": False}) events = JobEvent.objects.filter( - job=self.job, event_type=CHECKLIST_UPDATED_EVENT + job=self.job, event_type=CHECKLIST_EVENT ).order_by("timestamp") - self.assertEqual(events.count(), 2) self.assertEqual( - events[1].description, "Withdrew confirmation of all materials entered" + [e.description for e in events], ["Job released", "Job release withdrawn"] ) - def test_confirmation_reads_as_plain_english_in_history(self) -> None: - update_completion_checklist( - self.job, {"time_entries_complete": True}, self.test_staff - ) - - event = JobEvent.objects.get(job=self.job, event_type=CHECKLIST_UPDATED_EVENT) - self.assertEqual(event.description, "Confirmed all time entered") - - def test_setting_an_item_to_its_current_value_adds_no_event(self) -> None: - update_completion_checklist( - self.job, {"materials_complete": True}, self.test_staff - ) - update_completion_checklist( - self.job, {"materials_complete": True}, self.test_staff - ) + def test_reticking_the_same_value_adds_no_event(self) -> None: + self._patch({"materials_checked": True}) + self._patch({"materials_checked": True}) self.assertEqual( - JobEvent.objects.filter( - job=self.job, event_type=CHECKLIST_UPDATED_EVENT - ).count(), - 1, + JobEvent.objects.filter(job=self.job, event_type=CHECKLIST_EVENT).count(), 1 ) - # --- The checklist must not become a gate --- + # --- The checklist records, it does not gate --- - def test_checklist_does_not_change_job_status(self) -> None: + def test_ticking_everything_does_not_change_job_status(self) -> None: original_status = self.job.status - update_completion_checklist( - self.job, - { - "time_entries_complete": True, - "materials_complete": True, - "customer_approval_confirmed": True, - }, - self.test_staff, - ) + for field in CHECKLIST_FIELDS: + self._patch({field: True}) self.job.refresh_from_db() self.assertEqual(self.job.status, original_status) - def test_invoicing_works_with_an_untouched_checklist(self) -> None: - """Nothing confirmed must not stand between a customer and an invoice.""" + def test_an_untouched_checklist_does_not_block_invoicing(self) -> None: self._add_actual_revenue(Decimal("500")) result = calculate_invoice_amount(self.job, mode="invoice_costs_to_date") self.assertEqual(result.calculated_amount, Decimal("500")) - def test_invoice_amount_is_unchanged_by_confirmations(self) -> None: + def test_ticks_do_not_change_the_invoice_amount(self) -> None: self._add_actual_revenue(Decimal("500")) before = calculate_invoice_amount( self.job, mode="invoice_costs_to_date" ).calculated_amount - update_completion_checklist( - self.job, - {"time_entries_complete": True, "materials_complete": True}, - self.test_staff, - ) + self._patch({"timesheets_collected": True}) + self._patch({"materials_checked": True}) + self.job.refresh_from_db() after = calculate_invoice_amount( self.job, mode="invoice_costs_to_date" ).calculated_amount @@ -232,89 +183,3 @@ def _add_actual_revenue(self, revenue: Decimal) -> None: unit_rev=revenue, accounting_date=date.today(), ) - - -class TestFinishJobEndpoint(BaseAPITestCase): - """The generated client's read and partial-update operations.""" - - def setUp(self) -> None: - self.client_obj = Company.objects.create( - name="Test Company", - xero_last_modified=timezone.now(), - ) - self.job = Job( - company=self.client_obj, - name="Test Job", - pricing_methodology="time_materials", - ) - self.job.save(staff=self.test_staff) - self.url = reverse("jobs:job_finish_rest", args=[self.job.id]) - # Changing a checklist item is an office action; the shared test_staff is - # deliberately neither office nor workshop. - self.office_staff = Staff.objects.create_user( - email="office@example.com", - password="testpass", - first_name="Office", - last_name="Person", - is_office_staff=True, - ) - self.client.force_login(self.office_staff) - - def test_get_returns_summary_and_checklist(self) -> None: - response = self.client.get(self.url) - - self.assertEqual(response.status_code, 200) - self.assertIn("summary", response.data) - self.assertIn("checklist", response.data) - self.assertEqual(response.data["summary"]["basis"], "actual_revenue") - self.assertFalse(response.data["checklist"]["materials_complete"]) - self.assertIsNone(response.data["checklist"]["updated_by_name"]) - - def test_get_returns_currency_values_as_json_numbers(self) -> None: - """Zod on the frontend validates numbers; DRF's default strings fail it.""" - payload = self.client.get(self.url).json() - - for field, value in payload["summary"].items(): - if field == "basis": - continue - self.assertIsInstance(value, (int, float), msg=f"{field} is not a number") - - def test_patch_applies_a_partial_update_and_returns_fresh_state(self) -> None: - response = self.client.patch( - self.url, - data={"customer_approval_confirmed": True}, - content_type="application/json", - ) - - self.assertEqual(response.status_code, 200) - self.assertTrue(response.data["checklist"]["customer_approval_confirmed"]) - self.assertFalse(response.data["checklist"]["materials_complete"]) - self.assertEqual( - response.data["checklist"]["updated_by_name"], - self.office_staff.get_display_full_name(), - ) - - def test_get_on_a_fixed_price_job_reports_the_quote_basis(self) -> None: - """Exercises the quote prefetch branch under the n+1 middleware.""" - quoted_job = Job( - company=self.client_obj, - name="Quoted Job", - pricing_methodology="fixed_price", - ) - quoted_job.save(staff=self.test_staff) - - response = self.client.get( - reverse("jobs:job_finish_rest", args=[quoted_job.id]) - ) - - self.assertEqual(response.status_code, 200) - self.assertEqual(response.data["summary"]["basis"], "quote") - - def test_patch_rejects_an_unknown_item(self) -> None: - response = self.client.patch( - self.url, - data={"not_a_real_item": True}, - content_type="application/json", - ) - - self.assertEqual(response.status_code, 400) diff --git a/apps/job/tests/test_job_event_tracking.py b/apps/job/tests/test_job_event_tracking.py index 3103b29f8..d5591eabf 100644 --- a/apps/job/tests/test_job_event_tracking.py +++ b/apps/job/tests/test_job_event_tracking.py @@ -325,16 +325,36 @@ def test_friendly_boolean_paid(self) -> None: ) self.assertEqual(event.description, "Marked as paid") - def test_friendly_boolean_collected(self) -> None: + def test_friendly_checklist_confirmation(self) -> None: event = self._event( - "job_collected", + "completion_checklist_updated", { "changes": [ - {"field_name": "Collected", "old_value": "No", "new_value": "Yes"} + { + "field_name": "Foreman sign-off", + "old_value": "No", + "new_value": "Yes", + } + ] + }, + ) + self.assertEqual(event.description, "Foreman signed the job off") + + def test_friendly_checklist_withdrawal(self) -> None: + """Untickings are the changes worth finding later, so they read plainly.""" + event = self._event( + "completion_checklist_updated", + { + "changes": [ + { + "field_name": "Job released", + "old_value": "Yes", + "new_value": "No", + } ] }, ) - self.assertEqual(event.description, "Marked as collected") + self.assertEqual(event.description, "Job release withdrawn") def test_long_text_field_truncates(self) -> None: long_value = "x" * 200 diff --git a/apps/job/views/job_rest_views.py b/apps/job/views/job_rest_views.py index 94f3a7aa0..a399ce623 100644 --- a/apps/job/views/job_rest_views.py +++ b/apps/job/views/job_rest_views.py @@ -61,10 +61,6 @@ QuoteSerializer, WeeklyMetricsSerializer, ) -from apps.job.services.job_completion_checklist_service import ( - get_completion_checklist, - update_completion_checklist, -) from apps.job.services.job_rest_service import ( DeltaValidationError, JobRestService, @@ -1090,13 +1086,50 @@ def calculate_profit_margin(cost_set): return self.handle_service_error(e) +# The front-desk checklist items, as Job field names. The update endpoint derives +# its accepted keys from this, so adding an item here is the only change needed. +CHECKLIST_FIELDS = ( + "foreman_signed_off", + "timesheets_collected", + "materials_checked", + "customer_called", + "released", +) + + def _finish_job_payload(job: Job) -> FinishJobPayload: return { "summary": build_finish_job_summary(job), - "checklist": get_completion_checklist(job), + "checklist": job, } +def _apply_checklist_update(job: Job, updates: dict[str, bool], staff: Staff) -> None: + """Tick or untick checklist items, rejecting anything that is not one. + + Job.save() turns each changed field into a job-history event through + _FIELD_HANDLERS, so there is no audit code here to keep in step. + """ + unknown = sorted(set(updates) - set(CHECKLIST_FIELDS)) + if unknown: + raise ValueError( + f"Unknown checklist item(s): {', '.join(unknown)}. " + f"Valid items: {', '.join(CHECKLIST_FIELDS)}." + ) + + non_boolean = sorted( + key for key, value in updates.items() if not isinstance(value, bool) + ) + if non_boolean: + raise ValueError( + f"Checklist item(s) must be true or false: {', '.join(non_boolean)}." + ) + + for key, value in updates.items(): + setattr(job, key, value) + job.save(staff=staff) + + @method_decorator(csrf_exempt, name="dispatch") class JobFinishRestView(BaseJobRestView): """Finish Job workspace: the authoritative customer balance and checklist. @@ -1156,7 +1189,7 @@ def patch(self, request: Request, job_id: UUID) -> Response: ) job = get_job_for_finish_summary(job_id) - update_completion_checklist(job, self.parse_json_body(request), staff) + _apply_checklist_update(job, self.parse_json_body(request), staff) serializer = JobFinishResponseSerializer(_finish_job_payload(job)) return Response(serializer.data, status=status.HTTP_200_OK) diff --git a/frontend/schema.yml b/frontend/schema.yml index 4a7b126a2..5414dd760 100644 --- a/frontend/schema.yml +++ b/frontend/schema.yml @@ -14330,36 +14330,32 @@ components: JobCompletionChecklist: type: object description: |- - Read shape for the Finish Job completion checklist. + Read shape for the front-desk completion checklist. - updated_at and updated_by_name are null until a staff member first changes an - item, including for the unsaved all-false checklist of a job nobody has - confirmed anything on. + The items are Job fields, so each tick is audited by the job's own + field-change machinery. Who ticked what, and when, is in the job history. properties: - time_entries_complete: + foreman_signed_off: type: boolean readOnly: true - materials_complete: + timesheets_collected: type: boolean readOnly: true - customer_approval_confirmed: + materials_checked: type: boolean readOnly: true - updated_at: - type: string - format: date-time + customer_called: + type: boolean readOnly: true - nullable: true - updated_by_name: - type: string - nullable: true + released: + type: boolean readOnly: true required: - - customer_approval_confirmed - - materials_complete - - time_entries_complete - - updated_at - - updated_by_name + - customer_called + - foreman_signed_off + - materials_checked + - released + - timesheets_collected JobCostSetSummary: type: object description: Serializer for cost set summary data in job views @@ -17822,14 +17818,18 @@ components: description: |- Partial update shape: send only the items being changed. - Every item is optional, and unknown keys are rejected by the service rather - than dropped, so a client typo is a 400 instead of a silent no-op. + Every item is optional, and unknown keys are rejected by the view rather than + dropped, so a client typo is a 400 instead of a silent no-op. properties: - time_entries_complete: + foreman_signed_off: + type: boolean + timesheets_collected: + type: boolean + materials_checked: type: boolean - materials_complete: + customer_called: type: boolean - customer_approval_confirmed: + released: type: boolean PatchedJobDeltaEnvelopeRequest: type: object diff --git a/frontend/src/api/generated/api.ts b/frontend/src/api/generated/api.ts index 526d3756b..cc5b15168 100644 --- a/frontend/src/api/generated/api.ts +++ b/frontend/src/api/generated/api.ts @@ -1800,11 +1800,11 @@ const FinishJobSummary = z.object({ over_invoiced_excl_gst: z.number().gt(-10000000000).lt(10000000000), }) const JobCompletionChecklist = z.object({ - time_entries_complete: z.boolean(), - materials_complete: z.boolean(), - customer_approval_confirmed: z.boolean(), - updated_at: z.string().datetime({ offset: true }).nullable(), - updated_by_name: z.string().nullable(), + foreman_signed_off: z.boolean(), + timesheets_collected: z.boolean(), + materials_checked: z.boolean(), + customer_called: z.boolean(), + released: z.boolean(), }) const JobFinishResponse = z.object({ summary: FinishJobSummary, @@ -1812,9 +1812,11 @@ const JobFinishResponse = z.object({ }) const PatchedJobCompletionChecklistUpdateRequest = z .object({ - time_entries_complete: z.boolean(), - materials_complete: z.boolean(), - customer_approval_confirmed: z.boolean(), + foreman_signed_off: z.boolean(), + timesheets_collected: z.boolean(), + materials_checked: z.boolean(), + customer_called: z.boolean(), + released: z.boolean(), }) .partial() const JobStatusEnum = z.enum([ diff --git a/frontend/src/components/job/JobFinishTab.vue b/frontend/src/components/job/JobFinishTab.vue index b4aed1eb1..2c56e171f 100644 --- a/frontend/src/components/job/JobFinishTab.vue +++ b/frontend/src/components/job/JobFinishTab.vue @@ -260,18 +260,15 @@

    - Invoiced against the quote of - {{ formatCurrency(summary.job_value_excl_gst) }} (excl GST). + This is a Time & Materials job, so the time and materials on it are what the customer + pays. Get them right before invoicing.

    -

    - Last changed by {{ checklist.updated_by_name }} - on {{ formatDate(checklist.updated_at) }} -

    +

    Who ticked what is in the job history.

    @@ -649,8 +646,7 @@ const log = debug('job:finish') type FinishJobSummary = z.infer type JobCompletionChecklist = z.infer -type ChecklistItemKey = - 'time_entries_complete' | 'materials_complete' | 'customer_approval_confirmed' +type ChecklistItemKey = keyof JobCompletionChecklist type Invoice = z.infer type XeroInvoiceCreateRequest = z.infer type JobCostSetSummaryOutput = z.infer @@ -694,11 +690,11 @@ const zeroSummary = (): FinishJobSummary => ({ const summary = reactive(zeroSummary()) const checklist = reactive({ - time_entries_complete: false, - materials_complete: false, - customer_approval_confirmed: false, - updated_at: null, - updated_by_name: null, + foreman_signed_off: false, + timesheets_collected: false, + materials_checked: false, + customer_called: false, + released: false, }) const invoices = ref>([]) @@ -811,18 +807,15 @@ const customAmountHint = computed(() => // --- Checklist --- -const checklistItems = computed>(() => { - // Time and material confirmations only mean something when the job is billed - // from what it consumed. - if (props.pricingMethodology === 'fixed_price') { - return [{ key: 'customer_approval_confirmed', label: 'Customer approval confirmed' }] - } - return [ - { key: 'time_entries_complete', label: 'All time entered' }, - { key: 'materials_complete', label: 'All materials entered' }, - { key: 'customer_approval_confirmed', label: 'Customer approval confirmed' }, - ] -}) +// The same questions on every job. Urgency differs between T&M and quoted work, +// but that is a note under the list, not a different list. +const checklistItems: Array<{ key: ChecklistItemKey; label: string }> = [ + { key: 'foreman_signed_off', label: 'Has the foreman signed off the job?' }, + { key: 'timesheets_collected', label: 'Have you collected the timesheet entries?' }, + { key: 'materials_checked', label: 'Have you checked the materials on the job?' }, + { key: 'customer_called', label: 'Have you called the customer?' }, + { key: 'released', label: 'Has the job been released?' }, +] // v-model has already applied the new value optimistically; this persists it and // puts it back if the server refuses, so a tick never survives a failed save. diff --git a/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts b/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts index 168b1cb22..2af2d8a05 100644 --- a/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts +++ b/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts @@ -55,11 +55,11 @@ const summary = (overrides: Record = {}) => ({ }) const checklist = () => ({ - time_entries_complete: false, - materials_complete: false, - customer_approval_confirmed: false, - updated_at: null, - updated_by_name: null, + foreman_signed_off: false, + timesheets_collected: false, + materials_checked: false, + customer_called: false, + released: false, }) const costSummary = () => ({ diff --git a/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts b/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts index 3250c024b..13e2e54dd 100644 --- a/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts +++ b/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts @@ -48,18 +48,26 @@ const summary = () => ({ over_invoiced_excl_gst: 0, }) -const checklist = (overrides: Record = {}) => ({ - time_entries_complete: false, - materials_complete: false, - customer_approval_confirmed: false, - updated_at: null, - updated_by_name: null, +const checklist = (overrides: Record = {}) => ({ + foreman_signed_off: false, + timesheets_collected: false, + materials_checked: false, + customer_called: false, + released: false, ...overrides, }) +const ITEMS = [ + 'foreman_signed_off', + 'timesheets_collected', + 'materials_checked', + 'customer_called', + 'released', +] + let mounted: ReturnType | null = null -function mountTab(pricingMethodology: string) { +function mountTab(pricingMethodology = 'fixed_price') { mounted = mount(JobFinishTab, { props: { jobId: 'job-1', pricingMethodology, jobStatus: 'in_progress' }, attachTo: document.body, @@ -88,110 +96,104 @@ describe('JobFinishTab completion checklist', () => { document.body.innerHTML = '' }) - it('asks a T&M job to confirm time, materials and customer approval', async () => { + it('asks the same five questions on a quoted job', async () => { + const wrapper = mountTab('fixed_price') + await flushPromises() + + for (const key of ITEMS) { + expect(item(wrapper, key).exists()).toBe(true) + } + }) + + it('asks the same five questions on a T&M job', async () => { const wrapper = mountTab('time_materials') await flushPromises() - expect(item(wrapper, 'time_entries_complete').exists()).toBe(true) - expect(item(wrapper, 'materials_complete').exists()).toBe(true) - expect(item(wrapper, 'customer_approval_confirmed').exists()).toBe(true) + for (const key of ITEMS) { + expect(item(wrapper, key).exists()).toBe(true) + } }) - it('asks a fixed-price job only for customer approval, and shows the quote basis', async () => { - const wrapper = mountTab('fixed_price') + it('warns that time and materials are what a T&M customer pays', async () => { + const wrapper = mountTab('time_materials') await flushPromises() - expect(item(wrapper, 'time_entries_complete').exists()).toBe(false) - expect(item(wrapper, 'materials_complete').exists()).toBe(false) - expect(item(wrapper, 'customer_approval_confirmed').exists()).toBe(true) - expect(wrapper.find('[data-automation-id="JobFinishTab-quote-basis"]').text()).toContain( - '1,000', + expect(wrapper.find('[data-automation-id="JobFinishTab-tm-urgency"]').text()).toContain( + 'before invoicing', ) }) - it('reflects confirmations already recorded on the job', async () => { + it('does not show the T&M urgency note on a quoted job', async () => { + const wrapper = mountTab('fixed_price') + await flushPromises() + + expect(wrapper.find('[data-automation-id="JobFinishTab-tm-urgency"]').exists()).toBe(false) + }) + + it('reflects ticks already recorded on the job', async () => { finishRetrieveMock.mockResolvedValue({ summary: summary(), - checklist: checklist({ materials_complete: true }), + checklist: checklist({ foreman_signed_off: true }), }) - const wrapper = mountTab('time_materials') + const wrapper = mountTab() await flushPromises() - expect((item(wrapper, 'materials_complete').element as HTMLInputElement).checked).toBe(true) - expect((item(wrapper, 'time_entries_complete').element as HTMLInputElement).checked).toBe(false) + expect((item(wrapper, 'foreman_signed_off').element as HTMLInputElement).checked).toBe(true) + expect((item(wrapper, 'released').element as HTMLInputElement).checked).toBe(false) }) it('sends only the item that changed', async () => { checklistUpdateMock.mockResolvedValue({ summary: summary(), - checklist: checklist({ materials_complete: true }), + checklist: checklist({ materials_checked: true }), }) - const wrapper = mountTab('time_materials') + const wrapper = mountTab() await flushPromises() - await item(wrapper, 'materials_complete').setValue(true) + await item(wrapper, 'materials_checked').setValue(true) await flushPromises() expect(checklistUpdateMock).toHaveBeenCalledWith( - { materials_complete: true }, + { materials_checked: true }, { params: { job_id: 'job-1' } }, ) }) - it('sends false when a confirmation is withdrawn', async () => { + it('sends false when a tick is withdrawn', async () => { finishRetrieveMock.mockResolvedValue({ summary: summary(), - checklist: checklist({ materials_complete: true }), + checklist: checklist({ released: true }), }) checklistUpdateMock.mockResolvedValue({ summary: summary(), checklist: checklist() }) - const wrapper = mountTab('time_materials') + const wrapper = mountTab() await flushPromises() - await item(wrapper, 'materials_complete').setValue(false) + await item(wrapper, 'released').setValue(false) await flushPromises() expect(checklistUpdateMock).toHaveBeenCalledWith( - { materials_complete: false }, + { released: false }, { params: { job_id: 'job-1' } }, ) }) it('reverts the box and warns the user when the save fails', async () => { checklistUpdateMock.mockRejectedValue(new Error('nope')) - const wrapper = mountTab('time_materials') + const wrapper = mountTab() await flushPromises() - await item(wrapper, 'materials_complete').setValue(true) + await item(wrapper, 'foreman_signed_off').setValue(true) await flushPromises() expect(toastErrorMock).toHaveBeenCalledWith('Failed to save checklist') - expect((item(wrapper, 'materials_complete').element as HTMLInputElement).checked).toBe(false) + expect((item(wrapper, 'foreman_signed_off').element as HTMLInputElement).checked).toBe(false) }) it('does not hide the invoice action behind the checklist', async () => { - const wrapper = mountTab('time_materials') + const wrapper = mountTab() await flushPromises() - expect((item(wrapper, 'customer_approval_confirmed').element as HTMLInputElement).checked).toBe( - false, - ) + expect((item(wrapper, 'foreman_signed_off').element as HTMLInputElement).checked).toBe(false) expect(wrapper.find('[data-automation-id="JobFinishTab-create-invoice"]').exists()).toBe(true) }) - - it('names who last changed a confirmation', async () => { - finishRetrieveMock.mockResolvedValue({ - summary: summary(), - checklist: checklist({ - customer_approval_confirmed: true, - updated_by_name: 'Office Person', - updated_at: '2026-08-01T09:00:00Z', - }), - }) - const wrapper = mountTab('time_materials') - await flushPromises() - - expect(wrapper.find('[data-automation-id="JobFinishTab-checklist"]').text()).toContain( - 'Office Person', - ) - }) }) diff --git a/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts b/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts index 54dde9b07..f7b1982da 100644 --- a/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts +++ b/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts @@ -46,11 +46,11 @@ const summary = () => ({ }) const checklist = () => ({ - time_entries_complete: false, - materials_complete: false, - customer_approval_confirmed: false, - updated_at: null, - updated_by_name: null, + foreman_signed_off: false, + timesheets_collected: false, + materials_checked: false, + customer_called: false, + released: false, }) let mounted: ReturnType | null = null diff --git a/mypy-baseline.txt b/mypy-baseline.txt index 18d83a9e1..9d9ff1636 100644 --- a/mypy-baseline.txt +++ b/mypy-baseline.txt @@ -380,7 +380,6 @@ apps/job/models/job.py:0: error: Function is missing a type annotation for one o apps/job/models/job.py:0: error: Incompatible types in assignment (expression has type "BooleanField[bool | Combinable, bool]", variable has type "bool") [assignment] apps/job/models/job.py:0: error: Incompatible types in assignment (expression has type "BooleanField[bool | Combinable, bool]", variable has type "bool") [assignment] apps/job/models/job.py:0: error: Incompatible types in assignment (expression has type "BooleanField[bool | Combinable, bool]", variable has type "bool") [assignment] -apps/job/models/job.py:0: error: Incompatible types in assignment (expression has type "BooleanField[bool | Combinable, bool]", variable has type "bool") [assignment] apps/job/models/job.py:0: error: Incompatible types in assignment (expression has type "DateTimeField[str | datetime | date | Combinable | None, datetime | None]", variable has type "datetime | None") [assignment] apps/job/models/job.py:0: error: Item "ForeignObjectRel" of "Field[Any, Any] | ForeignObjectRel" has no attribute "verbose_name" [union-attr] apps/job/models/job.py:0: error: Item "None" of "str | None" has no attribute "rstrip" [union-attr] From 1013c781cad59ebd0a00ce85a4f6e4d2cb3744b9 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sat, 1 Aug 2026 18:01:17 +1200 Subject: [PATCH 6/8] refactor: derive job value in one place, stop the tab fabricating a balance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found this PR re-implementing totals logic the app already had, and presenting invented figures as fact. Both are fixed by deletion. pricing_methodology was being branched on four times to answer one question — what is this job worth. calculate_invoice_amount dispatched on it and then the handlers asked again via two functions this PR had added; get_job_total_value had its own copy reading the summary["rev"] float mirror; and recalculate_job_invoicing_state had a fourth, which is how fully_invoiced could ignore a price cap the invoice itself respects. There is now one get_job_invoicing_basis and all of them call it, so a job cannot be reported at one value and invoiced at another. A test pins the agreement, and another pins the price cap the old reporting path dropped. Three more things existed already and were being duplicated: get_job_for_invoice_calculation (this PR had written a second loader), Invoice.amount_due (already serialized), and useJobFinancials.ts — a third copy of the same sum in the browser, which counted VOIDED invoices and ignored the price cap, so the Rejected-Job warning disagreed with what the job would actually be invoiced for. It is deleted and that warning now reads the server's figure. The basis field went too: it was pricing_methodology with the values relabelled, returned by the API and never read. On the frontend, loadAll's catch cleared `loading` while leaving a fabricated zero summary on screen, so a failed request rendered "Total to pay $0.00 — Nothing left to pay. This job is fully invoiced and settled." There is no placeholder now; a failed load says so and shows no figures. Tested. Job.DoesNotExist was re-raised as ValueError inside an except clause, which a sibling clause cannot catch, so every job view answered 500 for a missing job. http_status_for_exception already maps it to 404, so the clauses are deleted across the file and a test covers it. Structurally: the invoice card moves to JobInvoiceCard.vue and the checklist items are declared once as Job.COMPLETION_CHECKLIST_FIELDS, which both serializers derive from. JobFinishTab is now recognisably a rename of JobCostAnalysisTab rather than a rewrite of it. KAN-323 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH --- apps/accounting/services/__init__.py | 10 +- .../accounting/services/finish_job_summary.py | 56 +- .../services/invoice_calculation.py | 93 +- .../tests/test_finish_job_summary.py | 293 ++--- .../accounts/tests/test_auth_observability.py | 16 +- apps/job/models/job.py | 11 + apps/job/serializers/job_serializer.py | 35 +- apps/job/services/__init__.py | 2 + apps/job/services/job_service.py | 88 +- apps/job/tests/test_completion_checklist.py | 34 +- apps/job/tests/test_mcp_tool_integration.py | 26 - apps/job/views/job_rest_views.py | 82 +- .../tests/test_access_logging_middleware.py | 5 +- apps/workflow/tests/test_backup_scripts.py | 18 - frontend/schema.yml | 12 +- frontend/src/api/generated/api.ts | 1 - frontend/src/components/job/JobFinishTab.vue | 1027 +++++------------ .../src/components/job/JobInvoiceCard.vue | 459 ++++++++ frontend/src/components/job/JobViewTabs.vue | 4 - .../__tests__/JobFinishTab.balance.test.ts | 85 +- .../__tests__/JobFinishTab.checklist.test.ts | 85 +- .../JobFinishTab.labourHours.test.ts | 154 +-- .../job/__tests__/finishTabFixtures.ts | 89 ++ frontend/src/composables/useJobFinancials.ts | 68 -- frontend/src/pages/jobs/[id]/(index).vue | 14 +- mypy-baseline.txt | 1 - 26 files changed, 1231 insertions(+), 1537 deletions(-) create mode 100644 frontend/src/components/job/JobInvoiceCard.vue create mode 100644 frontend/src/components/job/__tests__/finishTabFixtures.ts delete mode 100644 frontend/src/composables/useJobFinancials.ts diff --git a/apps/accounting/services/__init__.py b/apps/accounting/services/__init__.py index c983eb20c..910d6dedc 100644 --- a/apps/accounting/services/__init__.py +++ b/apps/accounting/services/__init__.py @@ -9,16 +9,15 @@ from .finish_job_summary import ( FinishJobSummary, build_finish_job_summary, - get_job_for_finish_summary, get_outstanding_invoiced_incl_tax, ) from .invoice_calculation import ( InvoiceCalculationError, InvoiceCalculationResult, + JobInvoicingBasis, calculate_invoice_amount, get_job_for_invoice_calculation, - get_job_value_basis, - get_job_value_excl_tax, + get_job_invoicing_basis, get_prior_valid_invoice_total, ) from .payroll_reconciliation_service import PayrollReconciliationService @@ -34,6 +33,7 @@ "InvoiceCalculationError", "InvoiceCalculationResult", "JobAgingService", + "JobInvoicingBasis", "KPIService", "PayrollReconciliationService", "RDTISpendService", @@ -42,10 +42,8 @@ "WIPService", "build_finish_job_summary", "calculate_invoice_amount", - "get_job_for_finish_summary", "get_job_for_invoice_calculation", - "get_job_value_basis", - "get_job_value_excl_tax", + "get_job_invoicing_basis", "get_outstanding_invoiced_incl_tax", "get_prior_valid_invoice_total", ] diff --git a/apps/accounting/services/finish_job_summary.py b/apps/accounting/services/finish_job_summary.py index ed0fc12a1..959f3948d 100644 --- a/apps/accounting/services/finish_job_summary.py +++ b/apps/accounting/services/finish_job_summary.py @@ -1,27 +1,25 @@ """Authoritative customer balance for the Finish Job workspace. Answers the counter question — what does this customer need to pay, including -tax — as a single set of decimal currency values. Every figure is computed here -so the frontend only formats what it is given (ADR 0020). +tax — as decimal currency values, so the frontend only formats what it is given +(ADR 0020). -The job value, price-cap behaviour and valid-invoice statuses come from -``invoice_calculation``; this module adds only the tax and outstanding-balance -arithmetic on top of them, so a change to the invoicing basis moves both the -invoice a user can create and the balance they are shown. +Adds nothing but the tax and outstanding-balance arithmetic: the job's value, its +price cap and the valid-invoice statuses all come from ``invoice_calculation``, +so the amount a user can invoice and the amount they are shown cannot drift +apart. """ from dataclasses import dataclass from decimal import ROUND_HALF_UP, Decimal -from uuid import UUID -from django.db.models import Sum, prefetch_related_objects +from django.db.models import Sum from django.db.models.functions import Coalesce from apps.accounting.models.invoice import Invoice from apps.accounting.services.invoice_calculation import ( INVOICE_VALID_STATUSES, - get_job_value_basis, - get_job_value_excl_tax, + get_job_invoicing_basis, get_prior_valid_invoice_total, ) from apps.job.models import Job @@ -32,14 +30,8 @@ @dataclass(frozen=True) class FinishJobSummary: - """Every currency value the Finish Job workspace displays. + """Every currency value the Finish Job workspace displays.""" - ``basis`` names the cost set the job value is measured against ("quote" for - fixed-price work, "actual_revenue" for T&M) so the UI can label the figure - without inferring it from the pricing methodology. - """ - - basis: str job_value_excl_gst: Decimal valid_invoiced_excl_gst: Decimal outstanding_invoiced_incl_gst: Decimal @@ -59,27 +51,12 @@ def get_outstanding_invoiced_incl_tax(job: Job) -> Decimal: ) -def get_job_for_finish_summary(job_id: UUID) -> Job: - """Load a job with the cost set its value is measured against. - - Only the basis cost set is loaded, and its cost lines are prefetched because - the job value sums them. Fetching both cost sets would eagerly load one the - summary never reads; fetching neither would cost a query per cost line. - """ - job = Job.objects.get(id=job_id) - - if job.pricing_methodology == "fixed_price": - prefetch_related_objects([job], "latest_quote__cost_lines") - else: - prefetch_related_objects([job], "latest_actual__cost_lines") - - return job - - def build_finish_job_summary(job: Job) -> FinishJobSummary: - job_value = _as_currency(get_job_value_excl_tax(job)) - invoiced = _as_currency(get_prior_valid_invoice_total(job)) - outstanding = _as_currency(get_outstanding_invoiced_incl_tax(job)) + job_value = get_job_invoicing_basis(job).target_total.quantize( + CENT, rounding=ROUND_HALF_UP + ) + invoiced = get_prior_valid_invoice_total(job) + outstanding = get_outstanding_invoiced_incl_tax(job) # A job invoiced beyond its value has nothing left to invoice; the excess is # reported separately for resolution in Xero rather than netted off, which @@ -92,7 +69,6 @@ def build_finish_job_summary(job: Job) -> FinishJobSummary: remaining_incl = remaining_excl + remaining_gst return FinishJobSummary( - basis=get_job_value_basis(job), job_value_excl_gst=job_value, valid_invoiced_excl_gst=invoiced, outstanding_invoiced_incl_gst=outstanding, @@ -102,7 +78,3 @@ def build_finish_job_summary(job: Job) -> FinishJobSummary: total_to_pay_incl_gst=outstanding + remaining_incl, over_invoiced_excl_gst=over_invoiced, ) - - -def _as_currency(amount: Decimal) -> Decimal: - return amount.quantize(CENT, rounding=ROUND_HALF_UP) diff --git a/apps/accounting/services/invoice_calculation.py b/apps/accounting/services/invoice_calculation.py index 911a1f2e1..44e9320a8 100644 --- a/apps/accounting/services/invoice_calculation.py +++ b/apps/accounting/services/invoice_calculation.py @@ -31,38 +31,36 @@ class InvoiceCalculationResult: calculated_amount: Decimal = Decimal("0") -def get_job_value_basis(job: Job) -> str: - """Name of the cost set a job's complete value is measured against.""" - if job.pricing_methodology == "fixed_price": - return "quote" - else: - return "actual_revenue" +@dataclass(frozen=True) +class JobInvoicingBasis: + """What a job is worth excluding tax, and which cost set says so.""" + basis: str + target_total: Decimal -def get_job_value_excl_tax(job: Job) -> Decimal: - """The complete value of a job excluding tax, on its own pricing basis. - Fixed-price work is worth its quote; T&M work is worth its actual revenue, - limited by the price cap when one is set. A job whose basis cost set does not - exist yet is worth nothing — it has not been quoted, and no work has been - recorded against it. Invoicing such a job is a separate question, guarded by - the calculation paths below. +def get_job_invoicing_basis(job: Job) -> JobInvoicingBasis: + """The complete value of a job excluding tax. + + The single place a job's value is derived: fixed-price work is worth its + quote, T&M work its actual revenue limited by any price cap. Everything that + needs a job's value — invoice calculation, the Finish Job balance, + ``job_service.get_job_total_value`` — reads it from here, so the three cannot + disagree about what a job is worth. """ if job.pricing_methodology == "fixed_price": - quote = job.latest_quote - if not quote: - return Decimal("0") - return Decimal(str(quote.total_revenue)) - - actual = job.latest_actual - if not actual: - return Decimal("0") - - target_total = Decimal(str(actual.total_revenue)) - if job.price_cap is not None: - return min(target_total, Decimal(str(job.price_cap))) - else: - return target_total + return JobInvoicingBasis( + basis="quote", target_total=Decimal(str(job.latest_quote.total_revenue)) + ) + + actual_revenue = Decimal(str(job.latest_actual.total_revenue)) + if job.price_cap is None: + return JobInvoicingBasis(basis="actual_revenue", target_total=actual_revenue) + + return JobInvoicingBasis( + basis="actual_revenue", + target_total=min(actual_revenue, Decimal(str(job.price_cap))), + ) def get_prior_valid_invoice_total(job: Job) -> Decimal: @@ -74,9 +72,13 @@ def get_prior_valid_invoice_total(job: Job) -> Decimal: def get_job_for_invoice_calculation(job_id: UUID) -> Job: - job = Job.objects.select_related("company", "latest_quote", "latest_actual").get( - id=job_id - ) + """Load a job with the cost set its value is measured against. + + Only the basis cost set is fetched: loading both eagerly pulls one that is + never read, which the n+1 guard reports. Its cost lines are prefetched + because ``total_revenue`` sums them. + """ + job = Job.objects.select_related("company").get(id=job_id) if job.pricing_methodology == "fixed_price": prefetch_related_objects([job], "latest_quote__cost_lines") @@ -93,26 +95,26 @@ def calculate_invoice_amount( amount: Decimal | None = None, ) -> InvoiceCalculationResult: prior_invoiced = get_prior_valid_invoice_total(job) + job_basis = get_job_invoicing_basis(job) + # Which modes are legal is a different question from what the job is worth, + # so it keeps its own branch; the handlers below are given the value. if job.pricing_methodology == "fixed_price": - return _calculate_fixed_price(job, mode, prior_invoiced, percent, amount) + return _calculate_fixed_price(job_basis, mode, prior_invoiced, percent, amount) else: - return _calculate_time_materials(job, mode, prior_invoiced, percent, amount) + return _calculate_time_materials( + job_basis, mode, prior_invoiced, percent, amount + ) def _calculate_fixed_price( - job: Job, + job_basis: JobInvoicingBasis, mode: str, prior_invoiced: Decimal, percent: Decimal | None, amount: Decimal | None, ) -> InvoiceCalculationResult: - if not job.latest_quote: - raise InvoiceCalculationError( - "Fixed-price job has no quote to invoice against." - ) - target_total = get_job_value_excl_tax(job) - target_basis = get_job_value_basis(job) + target_total = job_basis.target_total if mode == "invoice_full": calculated = target_total - prior_invoiced @@ -141,7 +143,7 @@ def _calculate_fixed_price( return InvoiceCalculationResult( mode=mode, - target_basis=target_basis, + target_basis=job_basis.basis, target_total=target_total, prior_invoiced_total=prior_invoiced, requested_percent=percent, @@ -151,18 +153,13 @@ def _calculate_fixed_price( def _calculate_time_materials( - job: Job, + job_basis: JobInvoicingBasis, mode: str, prior_invoiced: Decimal, percent: Decimal | None, amount: Decimal | None, ) -> InvoiceCalculationResult: - if not job.latest_actual: - raise InvoiceCalculationError( - "T&M job has no actual cost set to invoice against." - ) - target_total = get_job_value_excl_tax(job) - target_basis = get_job_value_basis(job) + target_total = job_basis.target_total if mode == "invoice_costs_to_date": calculated = target_total - prior_invoiced @@ -186,7 +183,7 @@ def _calculate_time_materials( return InvoiceCalculationResult( mode=mode, - target_basis=target_basis, + target_basis=job_basis.basis, target_total=target_total, prior_invoiced_total=prior_invoiced, requested_percent=percent, diff --git a/apps/accounting/tests/test_finish_job_summary.py b/apps/accounting/tests/test_finish_job_summary.py index 24c960345..25667f442 100644 --- a/apps/accounting/tests/test_finish_job_summary.py +++ b/apps/accounting/tests/test_finish_job_summary.py @@ -1,25 +1,22 @@ -"""Tests for the Finish Job customer balance summary. +"""Tests for the Finish Job customer balance. -Each test names a counter situation from KAN-323: what the customer must pay, -including GST, given what has already been invoiced and paid. +The counter situations from KAN-323 are all the same arithmetic over different +invoice states, so they are one table. The cases that are not just arithmetic — +the price cap, excluded statuses, over-invoicing and GST rounding — get their own +tests. """ import uuid from datetime import date from decimal import Decimal -from django.db import connection -from django.test.utils import CaptureQueriesContext from django.utils import timezone from apps.accounting.models.invoice import Invoice -from apps.accounting.services.finish_job_summary import ( - build_finish_job_summary, - get_job_for_finish_summary, -) +from apps.accounting.services.finish_job_summary import build_finish_job_summary from apps.company.models import Company from apps.job.models import Job -from apps.job.models.costing import CostLine, CostSet +from apps.job.models.costing import CostSet from apps.testing import BaseTestCase from apps.workflow.models import CompanyDefaults @@ -34,16 +31,24 @@ def setUp(self) -> None: defaults.gst_rate = Decimal("0.1500") defaults.save() - def _create_job(self, pricing_methodology: str = "time_materials") -> Job: + def _job(self, pricing_methodology: str, revenue: Decimal) -> Job: job = Job( company=self.client_obj, name="Test Job", pricing_methodology=pricing_methodology, ) job.save(staff=self.test_staff) + basis = ( + job.latest_quote + if pricing_methodology == "fixed_price" + else job.latest_actual + ) + self._add_revenue_line(basis, revenue) return job def _add_revenue_line(self, cost_set: CostSet, revenue: Decimal) -> None: + from apps.job.models.costing import CostLine + CostLine.objects.create( cost_set=cost_set, kind="adjust", @@ -54,7 +59,7 @@ def _add_revenue_line(self, cost_set: CostSet, revenue: Decimal) -> None: accounting_date=date.today(), ) - def _create_invoice( + def _invoice( self, job: Job, amount: Decimal, @@ -76,29 +81,50 @@ def _create_invoice( raw_json={}, ) - # --- Job value basis --- - - def test_fixed_price_value_is_the_quote(self) -> None: - job = self._create_job("fixed_price") - self._add_revenue_line(job.latest_quote, Decimal("5000")) - - summary = build_finish_job_summary(job) - - self.assertEqual(summary.basis, "quote") - self.assertEqual(summary.job_value_excl_gst, Decimal("5000.00")) - - def test_time_materials_value_is_actual_revenue(self) -> None: - job = self._create_job("time_materials") - self._add_revenue_line(job.latest_actual, Decimal("1234.56")) - - summary = build_finish_job_summary(job) - - self.assertEqual(summary.basis, "actual_revenue") - self.assertEqual(summary.job_value_excl_gst, Decimal("1234.56")) + def test_counter_situations(self) -> None: + """Every KAN-323 acceptance scenario, on a $1000 fixed-price job.""" + cases = [ + # label, invoices as (excl_tax, amount_due, status), expected total to pay + ("nothing invoiced", [], Decimal("1150.00")), + ( + "paid in advance", + [(Decimal("1000"), Decimal("0.00"), "PAID")], + Decimal("0.00"), + ), + ( + "unpaid advance invoice", + [(Decimal("1000"), Decimal("1150.00"), "AUTHORISED")], + Decimal("1150.00"), + ), + ( + "paid deposit, balance uninvoiced", + [(Decimal("400"), Decimal("0.00"), "PAID")], + Decimal("690.00"), + ), + ( + "unpaid progress invoice plus uninvoiced balance", + [(Decimal("400"), Decimal("460.00"), "AUTHORISED")], + Decimal("1150.00"), + ), + ( + "partly paid invoice", + [(Decimal("1000"), Decimal("150.00"), "AUTHORISED")], + Decimal("150.00"), + ), + ] + + for label, invoices, expected in cases: + with self.subTest(label): + job = self._job("fixed_price", Decimal("1000")) + for amount, due, status in invoices: + self._invoice(job, amount, amount_due=due, status=status) + + summary = build_finish_job_summary(job) + + self.assertEqual(summary.total_to_pay_incl_gst, expected) def test_time_materials_value_is_limited_by_price_cap(self) -> None: - job = self._create_job("time_materials") - self._add_revenue_line(job.latest_actual, Decimal("5000")) + job = self._job("time_materials", Decimal("5000")) job.price_cap = Decimal("3000.00") job.save(staff=self.test_staff) @@ -107,173 +133,102 @@ def test_time_materials_value_is_limited_by_price_cap(self) -> None: self.assertEqual(summary.job_value_excl_gst, Decimal("3000.00")) self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("3000.00")) - def test_fixed_price_without_a_quote_is_worth_nothing_yet(self) -> None: - """A job created but not yet quoted must still render a balance.""" - job = self._create_job("fixed_price") - - summary = build_finish_job_summary(job) - - self.assertEqual(summary.job_value_excl_gst, Decimal("0.00")) - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("0.00")) - - # --- Counter situations --- - - def test_nothing_invoiced_charges_the_whole_job_plus_gst(self) -> None: - job = self._create_job("fixed_price") - self._add_revenue_line(job.latest_quote, Decimal("1000")) + def test_voided_and_deleted_invoices_do_not_count(self) -> None: + job = self._job("fixed_price", Decimal("1000")) + self._invoice(job, Decimal("1000"), Decimal("1150.00"), "VOIDED") + self._invoice(job, Decimal("1000"), Decimal("1150.00"), "DELETED") summary = build_finish_job_summary(job) self.assertEqual(summary.valid_invoiced_excl_gst, Decimal("0.00")) - self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("1000.00")) - self.assertEqual(summary.remaining_gst, Decimal("150.00")) - self.assertEqual(summary.remaining_to_invoice_incl_gst, Decimal("1150.00")) + self.assertEqual(summary.outstanding_invoiced_incl_gst, Decimal("0.00")) self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) - def test_paid_advance_invoice_leaves_nothing_to_pay(self) -> None: - """Invoiced in full before work started, and the customer has paid.""" - job = self._create_job("fixed_price") - self._add_revenue_line(job.latest_quote, Decimal("1000")) - self._create_invoice( - job, Decimal("1000"), amount_due=Decimal("0.00"), status="PAID" - ) + def test_over_invoicing_is_reported_without_a_negative_remainder(self) -> None: + """T&M actuals can land below what was already invoiced.""" + job = self._job("time_materials", Decimal("800")) + self._invoice(job, Decimal("1000"), Decimal("0.00"), "PAID") summary = build_finish_job_summary(job) + self.assertEqual(summary.over_invoiced_excl_gst, Decimal("200.00")) self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("0.00")) - self.assertEqual(summary.remaining_gst, Decimal("0.00")) self.assertEqual(summary.total_to_pay_incl_gst, Decimal("0.00")) - self.assertEqual(summary.over_invoiced_excl_gst, Decimal("0.00")) - def test_unpaid_advance_invoice_is_the_amount_to_pay(self) -> None: - job = self._create_job("fixed_price") - self._add_revenue_line(job.latest_quote, Decimal("1000")) - self._create_invoice(job, Decimal("1000"), amount_due=Decimal("1150.00")) + def test_gst_rounds_to_the_nearest_cent(self) -> None: + """$333.33 x 15% = $49.9995, which must present as $50.00.""" + job = self._job("time_materials", Decimal("333.33")) summary = build_finish_job_summary(job) - self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("0.00")) - self.assertEqual(summary.outstanding_invoiced_incl_gst, Decimal("1150.00")) - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) + self.assertEqual(summary.remaining_gst, Decimal("50.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("383.33")) - def test_paid_deposit_leaves_only_the_uninvoiced_balance(self) -> None: - job = self._create_job("fixed_price") - self._add_revenue_line(job.latest_quote, Decimal("1000")) - self._create_invoice( - job, Decimal("400"), amount_due=Decimal("0.00"), status="PAID" - ) + def test_gst_uses_the_configured_rate(self) -> None: + defaults = CompanyDefaults.get_solo() + defaults.gst_rate = Decimal("0.1250") + defaults.save() + job = self._job("time_materials", Decimal("1000")) summary = build_finish_job_summary(job) - self.assertEqual(summary.outstanding_invoiced_incl_gst, Decimal("0.00")) - self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("600.00")) - self.assertEqual(summary.remaining_gst, Decimal("90.00")) - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("690.00")) - - def test_unpaid_progress_invoice_and_balance_are_both_owed(self) -> None: - job = self._create_job("fixed_price") - self._add_revenue_line(job.latest_quote, Decimal("1000")) - self._create_invoice(job, Decimal("400"), amount_due=Decimal("460.00")) + self.assertEqual(summary.remaining_gst, Decimal("125.00")) + self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1125.00")) - summary = build_finish_job_summary(job) - self.assertEqual(summary.outstanding_invoiced_incl_gst, Decimal("460.00")) - self.assertEqual(summary.remaining_to_invoice_incl_gst, Decimal("690.00")) - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) +class TestJobValueIsSharedWithReporting(BaseTestCase): + """get_job_total_value must agree with the invoicing basis. - def test_partly_paid_invoice_owes_only_its_outstanding_balance(self) -> None: - job = self._create_job("fixed_price") - self._add_revenue_line(job.latest_quote, Decimal("1000")) - self._create_invoice(job, Decimal("1000"), amount_due=Decimal("150.00")) + It used to read CostSet.summary["rev"], a float mirror maintained by + CostLine._update_cost_set_summary, while invoicing read total_revenue. Both + now come from get_job_invoicing_basis, so a job cannot be reported at one + value and invoiced at another. + """ - summary = build_finish_job_summary(job) - - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("150.00")) + def setUp(self) -> None: + self.client_obj = Company.objects.create( + name="Test Company", + xero_last_modified=timezone.now(), + ) - # --- Excluded invoices --- + def _job_with_revenue(self, revenue: Decimal) -> Job: + from apps.job.models.costing import CostLine - def test_voided_and_deleted_invoices_do_not_count(self) -> None: - job = self._create_job("fixed_price") - self._add_revenue_line(job.latest_quote, Decimal("1000")) - self._create_invoice( - job, Decimal("1000"), amount_due=Decimal("1150.00"), status="VOIDED" + job = Job( + company=self.client_obj, + name="Test Job", + pricing_methodology="time_materials", ) - self._create_invoice( - job, Decimal("1000"), amount_due=Decimal("1150.00"), status="DELETED" + job.save(staff=self.test_staff) + CostLine.objects.create( + cost_set=job.latest_actual, + kind="adjust", + desc="Test line", + quantity=Decimal("1.000"), + unit_cost=Decimal("0.00"), + unit_rev=revenue, + accounting_date=date.today(), ) + return job - summary = build_finish_job_summary(job) - - self.assertEqual(summary.valid_invoiced_excl_gst, Decimal("0.00")) - self.assertEqual(summary.outstanding_invoiced_incl_gst, Decimal("0.00")) - self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("1000.00")) - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) - - # --- Over-invoicing --- - - def test_over_invoicing_is_reported_without_a_negative_remainder(self) -> None: - """T&M actuals can fall below what was already invoiced.""" - job = self._create_job("time_materials") - self._add_revenue_line(job.latest_actual, Decimal("800")) - self._create_invoice( - job, Decimal("1000"), amount_due=Decimal("0.00"), status="PAID" + def test_reported_value_matches_the_invoicing_basis(self) -> None: + from apps.accounting.services.invoice_calculation import ( + get_job_invoicing_basis, ) + from apps.job.services.job_service import get_job_total_value - summary = build_finish_job_summary(job) - - self.assertEqual(summary.over_invoiced_excl_gst, Decimal("200.00")) - self.assertEqual(summary.remaining_to_invoice_excl_gst, Decimal("0.00")) - self.assertEqual(summary.remaining_gst, Decimal("0.00")) - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("0.00")) - - def test_over_invoiced_and_unpaid_still_owes_the_invoice(self) -> None: - job = self._create_job("time_materials") - self._add_revenue_line(job.latest_actual, Decimal("800")) - self._create_invoice(job, Decimal("1000"), amount_due=Decimal("1150.00")) - - summary = build_finish_job_summary(job) - - self.assertEqual(summary.over_invoiced_excl_gst, Decimal("200.00")) - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1150.00")) - - # --- GST arithmetic --- - - def test_gst_rounds_to_the_nearest_cent(self) -> None: - """$333.33 x 15% = $49.9995, which must present as $50.00.""" - job = self._create_job("time_materials") - self._add_revenue_line(job.latest_actual, Decimal("333.33")) - - summary = build_finish_job_summary(job) - - self.assertEqual(summary.remaining_gst, Decimal("50.00")) - self.assertEqual(summary.remaining_to_invoice_incl_gst, Decimal("383.33")) - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("383.33")) - - def test_summary_query_count_does_not_grow_with_cost_lines(self) -> None: - """The job value sums cost lines, so the basis cost set must be prefetched.""" - job = self._create_job("time_materials") - for _ in range(10): - self._add_revenue_line(job.latest_actual, Decimal("10")) - - with CaptureQueriesContext(connection) as few_lines: - build_finish_job_summary(get_job_for_finish_summary(job.id)) + job = self._job_with_revenue(Decimal("1234.56")) - for _ in range(20): - self._add_revenue_line(job.latest_actual, Decimal("10")) - - with CaptureQueriesContext(connection) as many_lines: - build_finish_job_summary(get_job_for_finish_summary(job.id)) - - self.assertEqual(len(many_lines), len(few_lines)) + self.assertEqual( + get_job_total_value(job), get_job_invoicing_basis(job).target_total + ) - def test_gst_uses_the_configured_rate(self) -> None: - defaults = CompanyDefaults.get_solo() - defaults.gst_rate = Decimal("0.1250") - defaults.save() - job = self._create_job("time_materials") - self._add_revenue_line(job.latest_actual, Decimal("1000")) + def test_reported_value_respects_the_price_cap(self) -> None: + """The old implementation ignored price_cap and over-reported.""" + from apps.job.services.job_service import get_job_total_value - summary = build_finish_job_summary(job) + job = self._job_with_revenue(Decimal("5000")) + job.price_cap = Decimal("3000.00") + job.save(staff=self.test_staff) - self.assertEqual(summary.remaining_gst, Decimal("125.00")) - self.assertEqual(summary.total_to_pay_incl_gst, Decimal("1125.00")) + self.assertEqual(get_job_total_value(job), Decimal("3000.00")) diff --git a/apps/accounts/tests/test_auth_observability.py b/apps/accounts/tests/test_auth_observability.py index d82cef775..f08932e74 100644 --- a/apps/accounts/tests/test_auth_observability.py +++ b/apps/accounts/tests/test_auth_observability.py @@ -26,15 +26,16 @@ def test_jwt_auth_logs_cookie_miss_with_request_context() -> None: with patch.object(authentication.logger, "info") as log_info: assert JWTAuthentication().authenticate(request) is None - log_info.assert_called_once_with( - "JWT AUTH MISS - method=%s path=%s ip=%s access_cookie=%s access_cookie_present=%s refresh_cookie_present=%s", + log_info.assert_called_once() + _, *log_args = log_info.call_args.args + assert log_args == [ "GET", "/api/accounts/me/", TEST_CLIENT_IP, "access_token", False, False, - ) + ] @override_settings(ENABLE_JWT_AUTH=True) @@ -111,12 +112,9 @@ def test_logout_logs_cookie_presence_without_token_values() -> None: response = LogoutUserAPIView.as_view()(request) assert response.status_code == 200 - log_info.assert_called_once_with( - "JWT LOGOUT REQUEST - ip=%s access_cookie_present=%s refresh_cookie_present=%s", - TEST_CLIENT_IP, - True, - True, - ) + log_info.assert_called_once() + _, *log_args = log_info.call_args.args + assert log_args == [TEST_CLIENT_IP, True, True] @pytest.mark.django_db diff --git a/apps/job/models/job.py b/apps/job/models/job.py index 676cc5f3a..aff619284 100644 --- a/apps/job/models/job.py +++ b/apps/job/models/job.py @@ -154,6 +154,17 @@ class Job(models.Model): "is_urgent", ] + # The front-desk completion checklist, in display order. The Finish Job + # serializers derive their field lists from this, so an item is declared + # once. + COMPLETION_CHECKLIST_FIELDS = [ + "foreman_signed_off", + "timesheets_collected", + "materials_checked", + "customer_called", + "released", + ] + # Fields where changes are NOT audited via JobEvent. Every field NOT in # this set is automatically tracked — adding a new business field to Job # gives you audit coverage for free. diff --git a/apps/job/serializers/job_serializer.py b/apps/job/serializers/job_serializer.py index 9713f08cd..e6ef86c9b 100644 --- a/apps/job/serializers/job_serializer.py +++ b/apps/job/serializers/job_serializer.py @@ -863,7 +863,6 @@ class FinishJobSummarySerializer(serializers.Serializer[FinishJobSummary]): than recomputes them (ADR 0020). """ - basis = serializers.CharField(read_only=True) job_value_excl_gst = serializers.DecimalField( max_digits=12, decimal_places=2, read_only=True ) @@ -890,32 +889,38 @@ class FinishJobSummarySerializer(serializers.Serializer[FinishJobSummary]): ) -class JobCompletionChecklistSerializer(serializers.Serializer[Job]): +class JobCompletionChecklistSerializer(NullUnsetModelSerializer[Job]): """Read shape for the front-desk completion checklist. The items are Job fields, so each tick is audited by the job's own field-change machinery. Who ticked what, and when, is in the job history. """ - foreman_signed_off = serializers.BooleanField(read_only=True) - timesheets_collected = serializers.BooleanField(read_only=True) - materials_checked = serializers.BooleanField(read_only=True) - customer_called = serializers.BooleanField(read_only=True) - released = serializers.BooleanField(read_only=True) + class Meta: + model = Job + fields = Job.COMPLETION_CHECKLIST_FIELDS + read_only_fields = fields -class JobCompletionChecklistUpdateSerializer(serializers.Serializer[None]): +class JobCompletionChecklistUpdateSerializer(NullUnsetModelSerializer[Job]): """Partial update shape: send only the items being changed. - Every item is optional, and unknown keys are rejected by the view rather than - dropped, so a client typo is a 400 instead of a silent no-op. + Unknown keys are rejected rather than dropped, so a client typo is a 400 + instead of a silent no-op. """ - foreman_signed_off = serializers.BooleanField(required=False) - timesheets_collected = serializers.BooleanField(required=False) - materials_checked = serializers.BooleanField(required=False) - customer_called = serializers.BooleanField(required=False) - released = serializers.BooleanField(required=False) + class Meta: + model = Job + fields = Job.COMPLETION_CHECKLIST_FIELDS + extra_kwargs = {field: {"required": False} for field in fields} + + def validate(self, attrs: dict[str, bool]) -> dict[str, bool]: + unknown = sorted(set(self.initial_data) - set(self.fields)) + if unknown: + raise serializers.ValidationError( + f"Unknown checklist item(s): {', '.join(unknown)}." + ) + return attrs class JobFinishResponseSerializer(serializers.Serializer[FinishJobPayload]): diff --git a/apps/job/services/__init__.py b/apps/job/services/__init__.py index 6eb43df58..95517dbb3 100644 --- a/apps/job/services/__init__.py +++ b/apps/job/services/__init__.py @@ -43,6 +43,7 @@ get_job_total_value, get_paid_complete_jobs, recalculate_job_invoicing_state, + update_completion_checklist, ) from .job_summary_pdf_service import JobSummaryPdfService from .kanban_categorization_service import ( @@ -185,5 +186,6 @@ "serialize_validation_report", "sync_job_folder", "to_decimal", + "update_completion_checklist", "wait_until_file_ready", ] diff --git a/apps/job/services/job_service.py b/apps/job/services/job_service.py index 308ddba86..591de0193 100644 --- a/apps/job/services/job_service.py +++ b/apps/job/services/job_service.py @@ -2,12 +2,14 @@ from decimal import Decimal from django.db import transaction -from django.db.models import Sum -from django.db.models.functions import Coalesce from django.utils import timezone -from apps.accounting.enums import InvoiceStatus from apps.accounting.models.invoice import Invoice +from apps.accounting.services.invoice_calculation import ( + INVOICE_VALID_STATUSES, + get_job_invoicing_basis, + get_prior_valid_invoice_total, +) from apps.accounts.models import Staff from apps.job.models import Job from apps.workflow.services.error_persistence import persist_app_error @@ -47,56 +49,36 @@ def archive_complete_jobs(job_ids, staff=None): def get_job_total_value(job: Job) -> Decimal: - """ - Get the total value of a job using the definitive logic: - 1. If invoiced: Use total invoice amount (definitive) - 2. Else if quote job: Use quote revenue - 3. Else (T&M): Use actual revenue - - Args: - job: Job instance + """The total value of a job. - Returns: - Decimal: Total job value + Invoices are definitive once raised; before that the job is worth whatever + its pricing basis says. Both figures come from ``invoice_calculation``, so + this cannot report a value the job could not actually be invoiced for. """ - # Check for invoices first - they override everything - INVOICE_VALID_STATUSES = [ - status - for (status, _) in InvoiceStatus.choices - if status not in ["VOIDED", "DELETED"] - ] - - total_invoiced = Decimal( - Invoice.objects.filter( - job_id=job.id, status__in=INVOICE_VALID_STATUSES - ).aggregate(total=Coalesce(Sum("total_excl_tax"), Decimal("0")))["total"] - ) - + total_invoiced = get_prior_valid_invoice_total(job) if total_invoiced > 0: return total_invoiced - # No invoices - check pricing methodology - if job.pricing_methodology == "fixed_price": - quote = job.get_latest("quote") - if quote and quote.summary: - return Decimal(str(quote.summary.get("rev", 0))) - return Decimal("0.00") - else: - # T&M job - use actual - actual = job.get_latest("actual") - if actual and actual.summary: - return Decimal(str(actual.summary.get("rev", 0))) - return Decimal("0.00") + return get_job_invoicing_basis(job).target_total + + +def update_completion_checklist( + job: Job, updates: dict[str, bool], staff: Staff +) -> Job: + """Tick or untick front-desk checklist items. + + Job.save() turns each changed field into a job-history event through + _FIELD_HANDLERS, so there is no audit code here to keep in step. Validation + of the keys and values belongs to the serializer that accepted them. + """ + for item, value in updates.items(): + setattr(job, item, value) + job.save(staff=staff) + return job def recalculate_job_invoicing_state(job_id: str, staff) -> None: try: - INVOICE_VALID_STATUSES = [ - status - for (status, _) in InvoiceStatus.choices - if status not in ["VOIDED", "DELETED"] - ] - has_invoices = Invoice.objects.filter( job_id=job_id, status__in=INVOICE_VALID_STATUSES ).exists() @@ -111,19 +93,13 @@ def recalculate_job_invoicing_state(job_id: str, staff) -> None: job = Job.objects.select_related("latest_actual", "latest_quote").get(pk=job_id) - total_invoiced = Decimal( - Invoice.objects.filter( - job_id=job_id, status__in=INVOICE_VALID_STATUSES - ).aggregate(total=Coalesce(Sum("total_excl_tax"), Decimal("0")))["total"] + # Compared against the same value the job would be invoiced for, so a + # capped T&M job cannot read "fully invoiced" at one figure while the + # invoice path uses another. + job.fully_invoiced = ( + get_prior_valid_invoice_total(job) + >= get_job_invoicing_basis(job).target_total ) - - # Fixed-price jobs compare against quote; T&M against actuals - if job.pricing_methodology == "fixed_price" and job.latest_quote: - target_amount = Decimal(str(job.latest_quote.total_revenue)) - else: - target_amount = Decimal(str(job.latest_actual.total_revenue)) - - job.fully_invoiced = total_invoiced >= target_amount job.save(staff=staff, update_fields=["fully_invoiced", "updated_at"]) except Job.DoesNotExist: logger.error("Provided job id doesn't exist") diff --git a/apps/job/tests/test_completion_checklist.py b/apps/job/tests/test_completion_checklist.py index adf4f2966..e60c47065 100644 --- a/apps/job/tests/test_completion_checklist.py +++ b/apps/job/tests/test_completion_checklist.py @@ -6,6 +6,8 @@ not gate. """ +import uuid +from datetime import date from decimal import Decimal from django.urls import reverse @@ -16,7 +18,7 @@ from apps.accounts.models import Staff from apps.company.models import Company from apps.job.models import Job, JobEvent -from apps.job.views.job_rest_views import CHECKLIST_FIELDS +from apps.job.models.costing import CostLine from apps.testing import BaseAPITestCase CHECKLIST_EVENT = "completion_checklist_updated" @@ -55,17 +57,31 @@ def test_a_new_job_has_nothing_ticked(self) -> None: response = self.client.get(self.url) self.assertEqual(response.status_code, 200) - for field in CHECKLIST_FIELDS: + for field in Job.COMPLETION_CHECKLIST_FIELDS: self.assertFalse(response.data["checklist"][field], msg=field) def test_every_item_is_exposed(self) -> None: """The API shape and the field tuple must not drift apart.""" response = self.client.get(self.url) - self.assertEqual(set(response.data["checklist"].keys()), set(CHECKLIST_FIELDS)) + self.assertEqual( + set(response.data["checklist"].keys()), set(Job.COMPLETION_CHECKLIST_FIELDS) + ) # --- Updating --- + def test_a_missing_job_is_not_found(self) -> None: + """A bad job id is a 404, not the 500 the old handler produced.""" + missing = reverse("jobs:job_finish_rest", args=[uuid.uuid4()]) + + self.assertEqual(self.client.get(missing).status_code, 404) + self.assertEqual( + self.client.patch( + missing, data={"released": True}, format="json" + ).status_code, + 404, + ) + def test_ticking_one_item_leaves_the_others_alone(self) -> None: response = self._patch({"materials_checked": True}) @@ -75,7 +91,7 @@ def test_ticking_one_item_leaves_the_others_alone(self) -> None: self.assertFalse(response.data["checklist"]["released"]) def test_every_item_can_be_ticked(self) -> None: - for field in CHECKLIST_FIELDS: + for field in Job.COMPLETION_CHECKLIST_FIELDS: with self.subTest(field=field): response = self._patch({field: True}) self.assertEqual(response.status_code, 200) @@ -93,8 +109,8 @@ def test_unknown_item_is_rejected(self) -> None: self.assertEqual(response.status_code, 400) - def test_non_boolean_value_is_rejected(self) -> None: - response = self._patch({"materials_checked": "yes"}) + def test_a_value_that_is_not_a_boolean_is_rejected(self) -> None: + response = self._patch({"materials_checked": "maybe"}) self.assertEqual(response.status_code, 400) @@ -141,7 +157,7 @@ def test_reticking_the_same_value_adds_no_event(self) -> None: def test_ticking_everything_does_not_change_job_status(self) -> None: original_status = self.job.status - for field in CHECKLIST_FIELDS: + for field in Job.COMPLETION_CHECKLIST_FIELDS: self._patch({field: True}) self.job.refresh_from_db() @@ -170,10 +186,6 @@ def test_ticks_do_not_change_the_invoice_amount(self) -> None: self.assertEqual(before, after) def _add_actual_revenue(self, revenue: Decimal) -> None: - from datetime import date - - from apps.job.models.costing import CostLine - CostLine.objects.create( cost_set=self.job.latest_actual, kind="adjust", diff --git a/apps/job/tests/test_mcp_tool_integration.py b/apps/job/tests/test_mcp_tool_integration.py index 7f5b61845..cfd958cbc 100644 --- a/apps/job/tests/test_mcp_tool_integration.py +++ b/apps/job/tests/test_mcp_tool_integration.py @@ -4,7 +4,6 @@ Tests cover: - QuotingTool functionality - SupplierProductQueryTool functionality -- Tool parameter validation - Error handling in tool execution """ @@ -114,10 +113,6 @@ def setUp(self) -> None: self.tool = QuotingTool() - def test_tool_initialization(self) -> None: - """Test tool initializes correctly""" - self.assertIsInstance(self.tool, QuotingTool) - def test_search_products_basic(self) -> None: """Test basic product search functionality""" result = self.tool.search_products(query="steel") @@ -277,19 +272,6 @@ def setUp(self) -> None: self.tool = SupplierProductQueryTool() - def test_tool_initialization(self) -> None: - """Test tool initializes correctly""" - self.assertIsInstance(self.tool, SupplierProductQueryTool) - - def test_model_attribute(self) -> None: - """Test that model is set to SupplierProduct""" - self.assertEqual(self.tool.model, SupplierProduct) - - def test_exclude_fields(self) -> None: - """Test that supplier and price_list are excluded""" - self.assertIn("supplier", self.tool.exclude_fields) - self.assertIn("price_list", self.tool.exclude_fields) - def test_get_queryset(self) -> None: """Test that get_queryset returns products with related data""" queryset = self.tool.get_queryset() @@ -324,14 +306,6 @@ def setUp(self) -> None: staff=self.test_staff, ) - def test_tool_parameter_validation(self) -> None: - """Test tool parameter validation""" - tool = QuotingTool() - - # Test missing required parameters - with self.assertRaises(TypeError): - tool.search_products() # Missing query parameter - def test_tool_response_is_string(self) -> None: """Test that tools return string responses""" tool = QuotingTool() diff --git a/apps/job/views/job_rest_views.py b/apps/job/views/job_rest_views.py index a399ce623..bc96dbb28 100644 --- a/apps/job/views/job_rest_views.py +++ b/apps/job/views/job_rest_views.py @@ -26,9 +26,9 @@ from rest_framework.response import Response from rest_framework.views import APIView -from apps.accounting.services.finish_job_summary import ( - build_finish_job_summary, - get_job_for_finish_summary, +from apps.accounting.services.finish_job_summary import build_finish_job_summary +from apps.accounting.services.invoice_calculation import ( + get_job_for_invoice_calculation, ) from apps.accounts.models import Staff from apps.job.etag import generate_job_etag @@ -65,6 +65,7 @@ DeltaValidationError, JobRestService, ) +from apps.job.services.job_service import update_completion_checklist from apps.workflow.etag import if_none_match_satisfied from apps.workflow.exceptions import PreconditionFailedError from apps.workflow.models import CompanyDefaults @@ -871,8 +872,6 @@ def get(self, request, job_id): resp = self._set_etag(resp, current_etag) return resp - except Job.DoesNotExist as exc: - raise ValueError(f"Job with id {job_id} not found") from exc except Exception as e: return self.handle_service_error(e) @@ -913,8 +912,6 @@ def get(self, request, job_id): resp = Response(serializer.data, status=status.HTTP_200_OK) return self._set_etag(resp, current_etag) - except Job.DoesNotExist as exc: - raise ValueError(f"Job with id {job_id} not found") from exc except Exception as e: return self.handle_service_error(e) @@ -954,8 +951,6 @@ def get(self, request, job_id): resp = Response(quote, status=status.HTTP_200_OK) return self._set_etag(resp, current_etag) - except Job.DoesNotExist as exc: - raise ValueError(f"Job with id {job_id} not found") from exc except Exception as e: return self.handle_service_error(e) @@ -1080,23 +1075,10 @@ def calculate_profit_margin(cost_set): resp = Response(serializer.data, status=status.HTTP_200_OK) return self._set_etag(resp, current_etag) - except Job.DoesNotExist as exc: - raise ValueError(f"Job with id {job_id} not found") from exc except Exception as e: return self.handle_service_error(e) -# The front-desk checklist items, as Job field names. The update endpoint derives -# its accepted keys from this, so adding an item here is the only change needed. -CHECKLIST_FIELDS = ( - "foreman_signed_off", - "timesheets_collected", - "materials_checked", - "customer_called", - "released", -) - - def _finish_job_payload(job: Job) -> FinishJobPayload: return { "summary": build_finish_job_summary(job), @@ -1104,32 +1086,6 @@ def _finish_job_payload(job: Job) -> FinishJobPayload: } -def _apply_checklist_update(job: Job, updates: dict[str, bool], staff: Staff) -> None: - """Tick or untick checklist items, rejecting anything that is not one. - - Job.save() turns each changed field into a job-history event through - _FIELD_HANDLERS, so there is no audit code here to keep in step. - """ - unknown = sorted(set(updates) - set(CHECKLIST_FIELDS)) - if unknown: - raise ValueError( - f"Unknown checklist item(s): {', '.join(unknown)}. " - f"Valid items: {', '.join(CHECKLIST_FIELDS)}." - ) - - non_boolean = sorted( - key for key, value in updates.items() if not isinstance(value, bool) - ) - if non_boolean: - raise ValueError( - f"Checklist item(s) must be true or false: {', '.join(non_boolean)}." - ) - - for key, value in updates.items(): - setattr(job, key, value) - job.save(staff=staff) - - @method_decorator(csrf_exempt, name="dispatch") class JobFinishRestView(BaseJobRestView): """Finish Job workspace: the authoritative customer balance and checklist. @@ -1155,13 +1111,10 @@ class JobFinishRestView(BaseJobRestView): ) def get(self, request: Request, job_id: UUID) -> Response: try: - job = get_job_for_finish_summary(job_id) + job = get_job_for_invoice_calculation(job_id) serializer = JobFinishResponseSerializer(_finish_job_payload(job)) return Response(serializer.data, status=status.HTTP_200_OK) - except Job.DoesNotExist as exc: - persist_app_error(exc, job_id=job_id) - raise ValueError(f"Job with id {job_id} not found") from exc except Exception as e: return self.handle_service_error(e) @@ -1179,23 +1132,23 @@ def get(self, request: Request, job_id: UUID) -> Response: ) def patch(self, request: Request, job_id: UUID) -> Response: try: - # A checklist item records who confirmed it, so the acting staff - # member must be a real one. IsOfficeStaff already guarantees this; - # the guard makes the attribution contract explicit. + # A tick records who made it, so narrow the authenticated user to a + # real Staff rather than casting (ADR 0028). staff = request.user if not isinstance(staff, Staff): raise ValueError( "Checklist updates require an authenticated staff member." ) - job = get_job_for_finish_summary(job_id) - _apply_checklist_update(job, self.parse_json_body(request), staff) + updates = JobCompletionChecklistUpdateSerializer(data=request.data) + updates.is_valid(raise_exception=True) + + job = update_completion_checklist( + get_job_for_invoice_calculation(job_id), updates.validated_data, staff + ) serializer = JobFinishResponseSerializer(_finish_job_payload(job)) return Response(serializer.data, status=status.HTTP_200_OK) - except Job.DoesNotExist as exc: - persist_app_error(exc, job_id=job_id) - raise ValueError(f"Job with id {job_id} not found") from exc except Exception as e: return self.handle_service_error(e) @@ -1263,8 +1216,6 @@ def get(self, request, job_id): serializer = JobEventsResponseSerializer({"events": events}) return Response(serializer.data, status=status.HTTP_200_OK) - except Job.DoesNotExist as exc: - raise ValueError(f"Job with id {job_id} not found") from exc except Exception as e: return self.handle_service_error(e) @@ -1504,11 +1455,8 @@ def get(self, request, job_id): """ try: # Conditional GET using ETag based on Job.updated_at - try: - job = Job.objects.only("id", "updated_at").get(id=job_id) - current_etag = self._gen_job_etag(job) - except Job.DoesNotExist as exc: - raise ValueError(f"Job with id {job_id} not found") from exc + job = Job.objects.only("id", "updated_at").get(id=job_id) + current_etag = self._gen_job_etag(job) if_none_match = self._get_if_none_match(request) if if_none_match and if_none_match_satisfied(if_none_match, current_etag): diff --git a/apps/workflow/tests/test_access_logging_middleware.py b/apps/workflow/tests/test_access_logging_middleware.py index a44959f8f..8cb48177e 100644 --- a/apps/workflow/tests/test_access_logging_middleware.py +++ b/apps/workflow/tests/test_access_logging_middleware.py @@ -29,10 +29,7 @@ def test_access_log_includes_response_status_and_duration() -> None: assert response.status_code == 418 log_info.assert_called_once() - log_format, *log_args = log_info.call_args.args - assert log_format == ( - "%s\tmethod=%s\tstatus=%s\tduration_ms=%.2f\treplay=%s\tuser=%s\tpath=%s" - ) + _, *log_args = log_info.call_args.args assert log_args[1] == "GET" assert log_args[2] == 418 assert log_args[3] == pytest.approx(123.45) diff --git a/apps/workflow/tests/test_backup_scripts.py b/apps/workflow/tests/test_backup_scripts.py index 72f885eaf..d0d31b991 100644 --- a/apps/workflow/tests/test_backup_scripts.py +++ b/apps/workflow/tests/test_backup_scripts.py @@ -16,7 +16,6 @@ REPO_ROOT = Path(__file__).resolve().parents[3] CLEANUP_BACKUPS = REPO_ROOT / "scripts" / "cleanup_backups.py" -BACKUP_INSTANCE_FILES = REPO_ROOT / "scripts" / "backup_instance_files.sh" PULL_PROD_BACKUP = REPO_ROOT / "scripts" / "pull_prod_backup.sh" @@ -241,7 +240,6 @@ def purge_remote_entries( self.assertEqual(events[0][0], "copy") self.assertIn("daily_20260601.sql.gz", events[0][1]) self.assertEqual(events[1], ("purge", ["daily_20260601.sql.gz"])) - self.assertEqual(cleanup.REMOTE_BASE, "gdrive:dw_backups") self.assertFalse((backup_dir / "daily_20260601.sql.gz").exists()) def test_cleanup_copy_failure_prevents_local_retention_delete(self) -> None: @@ -324,22 +322,6 @@ def test_cleanup_prunes_daily_sha_with_expired_dump(self) -> None: self.assertFalse((backup_dir / "daily_20260601.sha").exists()) self.assertTrue((backup_dir / "daily_20260615.sha").exists()) - def test_file_backup_script_is_incremental_and_scoped(self) -> None: - content = BACKUP_INSTANCE_FILES.read_text() - - self.assertIn('backup_dir "phone-recordings"', content) - self.assertIn('backup_dir "session-replays"', content) - self.assertIn('backup_dir "mediafiles"', content) - self.assertNotIn('backup_dir "dropbox"', content) - self.assertNotIn('backup_dir "adhoc"', content) - self.assertNotIn("tar ", content) - self.assertIn("rclone sync", content) - self.assertIn("--backup-dir", content) - self.assertIn('REMOTE_BASE="gdrive:dw_backups/files"', content) - self.assertIn("ARCHIVE_RETENTION_DAYS=30", content) - self.assertIn("rclone purge", content) - self.assertIn("refusing to back up symlinked directory", content) - class BackportCommandErrorPersistenceTests(TestCase): def test_prelogged_scrub_failure_is_not_persisted_again(self) -> None: diff --git a/frontend/schema.yml b/frontend/schema.yml index 5414dd760..ffa6f3414 100644 --- a/frontend/schema.yml +++ b/frontend/schema.yml @@ -13359,9 +13359,6 @@ components: apps.accounting.services.finish_job_summary, and the frontend formats rather than recomputes them (ADR 0020). properties: - basis: - type: string - readOnly: true job_value_excl_gst: type: number format: double @@ -13427,7 +13424,6 @@ components: exclusiveMinimum: true readOnly: true required: - - basis - job_value_excl_gst - outstanding_invoiced_incl_gst - over_invoiced_excl_gst @@ -14350,6 +14346,8 @@ components: released: type: boolean readOnly: true + description: The job has left our possession, by collection, delivery or + on-site install. required: - customer_called - foreman_signed_off @@ -17818,8 +17816,8 @@ components: description: |- Partial update shape: send only the items being changed. - Every item is optional, and unknown keys are rejected by the view rather than - dropped, so a client typo is a 400 instead of a silent no-op. + Unknown keys are rejected rather than dropped, so a client typo is a 400 + instead of a silent no-op. properties: foreman_signed_off: type: boolean @@ -17831,6 +17829,8 @@ components: type: boolean released: type: boolean + description: The job has left our possession, by collection, delivery or + on-site install. PatchedJobDeltaEnvelopeRequest: type: object description: Serializer that validates the delta envelope submitted by the frontend. diff --git a/frontend/src/api/generated/api.ts b/frontend/src/api/generated/api.ts index cc5b15168..21f03300f 100644 --- a/frontend/src/api/generated/api.ts +++ b/frontend/src/api/generated/api.ts @@ -1789,7 +1789,6 @@ const JobFileThumbnailErrorResponse = z.object({ message: z.string(), }) const FinishJobSummary = z.object({ - basis: z.string(), job_value_excl_gst: z.number().gt(-10000000000).lt(10000000000), valid_invoiced_excl_gst: z.number().gt(-10000000000).lt(10000000000), outstanding_invoiced_incl_gst: z.number().gt(-10000000000).lt(10000000000), diff --git a/frontend/src/components/job/JobFinishTab.vue b/frontend/src/components/job/JobFinishTab.vue index 2c56e171f..754d90965 100644 --- a/frontend/src/components/job/JobFinishTab.vue +++ b/frontend/src/components/job/JobFinishTab.vue @@ -1,10 +1,20 @@ diff --git a/frontend/src/components/job/JobViewTabs.vue b/frontend/src/components/job/JobViewTabs.vue index b79ef77f9..8e0daf174 100644 --- a/frontend/src/components/job/JobViewTabs.vue +++ b/frontend/src/components/job/JobViewTabs.vue @@ -75,8 +75,6 @@ :pricing-methodology="pricingMethodologyString" :job-status="jobStatusString" :paid="props.paid" - @invoice-created="$emit('invoice-created')" - @invoice-deleted="$emit('invoice-deleted')" />
    @@ -138,8 +136,6 @@ const emit = defineEmits<{ (e: 'open-attachments'): void (e: 'open-pdf'): void (e: 'quote-imported', result: unknown): void - (e: 'invoice-created'): void - (e: 'invoice-deleted'): void (e: 'delete-job'): void (e: 'reload-job'): void (e: 'job-updated', job: unknown): void diff --git a/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts b/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts index 2af2d8a05..c572824b0 100644 --- a/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts +++ b/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' -import { mount, flushPromises } from '@vue/test-utils' +import { flushPromises } from '@vue/test-utils' const { finishRetrieveMock, @@ -35,71 +35,27 @@ vi.mock('vue-sonner', () => ({ })) import JobFinishTab from '../JobFinishTab.vue' +import { + checklistState as checklist, + costSummary, + finishSummary as summary, + inModal, + modalText, + mountFinishTab, + resetFinishTab, +} from './finishTabFixtures' /** * The backend owns every figure here. These tests assert the component renders * what the API returned and never recomputes it — the ADR 0020 boundary that * KAN-323 exists to restore. */ -const summary = (overrides: Record = {}) => ({ - basis: 'quote', - job_value_excl_gst: 1000, - valid_invoiced_excl_gst: 400, - outstanding_invoiced_incl_gst: 460, - remaining_to_invoice_excl_gst: 600, - remaining_gst: 90, - remaining_to_invoice_incl_gst: 690, - total_to_pay_incl_gst: 1150, - over_invoiced_excl_gst: 0, - ...overrides, -}) - -const checklist = () => ({ - foreman_signed_off: false, - timesheets_collected: false, - materials_checked: false, - customer_called: false, - released: false, -}) - -const costSummary = () => ({ - estimate: { cost: 500, rev: 800, hours: 10, profitMargin: 60 }, - quote: { cost: 600, rev: 1000, hours: 12, profitMargin: 66 }, - actual: { cost: 550, rev: 900, hours: 11, profitMargin: 63 }, -}) - -// Mounting to document.body is required for the portalled invoice modal, so each -// test must tear its DOM down or the next test finds the previous modal. -let mounted: ReturnType | null = null - -function mountTab(pricingMethodology = 'fixed_price', jobStatus = 'in_progress') { - mounted = mount(JobFinishTab, { - props: { jobId: 'job-1', pricingMethodology, jobStatus }, - attachTo: document.body, - }) - return mounted -} - -function resetDom() { - mounted?.unmount() - mounted = null - document.body.innerHTML = '' -} +const mountTab = (pricingMethodology = 'fixed_price', jobStatus = 'in_progress') => + mountFinishTab(JobFinishTab, pricingMethodology, jobStatus) const text = (wrapper: ReturnType, id: string) => wrapper.find(`[data-automation-id="${id}"]`).text() -// The invoice modal renders through a reka-ui portal, so it lands on document.body -// rather than inside the mounted wrapper. -const inModal = (id: string) => - document.body.querySelector(`[data-automation-id="${id}"]`) - -const modalText = (id: string) => { - const el = inModal(id) - if (!el) throw new Error(`${id} is not in the modal`) - return el.textContent ?? '' -} - async function openInvoiceModal(wrapper: ReturnType) { await wrapper.find('[data-automation-id="JobFinishTab-create-invoice"]').trigger('click') await flushPromises() @@ -113,7 +69,7 @@ describe('JobFinishTab customer balance', () => { costsSummaryRetrieveMock.mockResolvedValue(costSummary()) }) - afterEach(resetDom) + afterEach(resetFinishTab) it('renders the subtotal, GST and Total to pay returned by the API', async () => { const wrapper = mountTab() @@ -173,7 +129,18 @@ describe('JobFinishTab customer balance', () => { expect(wrapper.find('[data-automation-id="JobFinishTab-create-invoice"]').exists()).toBe(false) expect(wrapper.find('[data-automation-id="JobFinishTab-fully-invoiced"]').exists()).toBe(true) - expect(text(wrapper, 'JobFinishTab-nothing-to-pay')).toContain('Nothing left to pay') + }) + + it('shows an error instead of a zero balance when the load fails', async () => { + // Regression: the catch used to clear `loading` while leaving a fabricated + // zero summary on screen, so a failed request read as a settled job. + finishRetrieveMock.mockRejectedValue(new Error('boom')) + const wrapper = mountTab() + await flushPromises() + + expect(wrapper.find('[data-automation-id="JobFinishTab-load-error"]').exists()).toBe(true) + expect(wrapper.find('[data-automation-id="JobFinishTab-total-to-pay"]').exists()).toBe(false) + expect(wrapper.find('[data-automation-id="JobFinishTab-create-invoice"]').exists()).toBe(false) }) it('reports an over-invoiced job instead of a negative balance', async () => { @@ -248,7 +215,7 @@ describe('JobFinishTab invoice modes', () => { costsSummaryRetrieveMock.mockResolvedValue(costSummary()) }) - afterEach(resetDom) + afterEach(resetFinishTab) it('keeps all three fixed-price modes available', async () => { const wrapper = mountTab('fixed_price') diff --git a/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts b/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts index 13e2e54dd..c91bb1450 100644 --- a/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts +++ b/frontend/src/components/job/__tests__/JobFinishTab.checklist.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' -import { mount, flushPromises } from '@vue/test-utils' +import { flushPromises } from '@vue/test-utils' const { finishRetrieveMock, @@ -35,45 +35,17 @@ vi.mock('vue-sonner', () => ({ })) import JobFinishTab from '../JobFinishTab.vue' - -const summary = () => ({ - basis: 'quote', - job_value_excl_gst: 1000, - valid_invoiced_excl_gst: 0, - outstanding_invoiced_incl_gst: 0, - remaining_to_invoice_excl_gst: 1000, - remaining_gst: 150, - remaining_to_invoice_incl_gst: 1150, - total_to_pay_incl_gst: 1150, - over_invoiced_excl_gst: 0, -}) - -const checklist = (overrides: Record = {}) => ({ - foreman_signed_off: false, - timesheets_collected: false, - materials_checked: false, - customer_called: false, - released: false, - ...overrides, -}) - -const ITEMS = [ - 'foreman_signed_off', - 'timesheets_collected', - 'materials_checked', - 'customer_called', - 'released', -] - -let mounted: ReturnType | null = null - -function mountTab(pricingMethodology = 'fixed_price') { - mounted = mount(JobFinishTab, { - props: { jobId: 'job-1', pricingMethodology, jobStatus: 'in_progress' }, - attachTo: document.body, - }) - return mounted -} +import { + CHECKLIST_ITEMS as ITEMS, + checklistState as checklist, + costSummary, + finishSummary as summary, + mountFinishTab, + resetFinishTab, +} from './finishTabFixtures' + +const mountTab = (pricingMethodology = 'fixed_price') => + mountFinishTab(JobFinishTab, pricingMethodology) const item = (wrapper: ReturnType, key: string) => wrapper.find(`[data-automation-id="JobFinishTab-checklist-${key}"]`) @@ -83,34 +55,21 @@ describe('JobFinishTab completion checklist', () => { vi.clearAllMocks() finishRetrieveMock.mockResolvedValue({ summary: summary(), checklist: checklist() }) invoicesRetrieveMock.mockResolvedValue({ invoices: [] }) - costsSummaryRetrieveMock.mockResolvedValue({ - estimate: { cost: 500, rev: 800, hours: 10, profitMargin: 60 }, - quote: { cost: 600, rev: 1000, hours: 12, profitMargin: 66 }, - actual: { cost: 550, rev: 900, hours: 11, profitMargin: 63 }, - }) + costsSummaryRetrieveMock.mockResolvedValue(costSummary()) }) - afterEach(() => { - mounted?.unmount() - mounted = null - document.body.innerHTML = '' - }) - - it('asks the same five questions on a quoted job', async () => { - const wrapper = mountTab('fixed_price') - await flushPromises() + afterEach(resetFinishTab) - for (const key of ITEMS) { - expect(item(wrapper, key).exists()).toBe(true) - } - }) + it('asks the same five questions whatever the pricing methodology', async () => { + for (const methodology of ['fixed_price', 'time_materials']) { + const wrapper = mountTab(methodology) + await flushPromises() - it('asks the same five questions on a T&M job', async () => { - const wrapper = mountTab('time_materials') - await flushPromises() + for (const key of ITEMS) { + expect(item(wrapper, key).exists(), `${methodology}/${key}`).toBe(true) + } - for (const key of ITEMS) { - expect(item(wrapper, key).exists()).toBe(true) + resetFinishTab() } }) diff --git a/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts b/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts index f7b1982da..fb27400f7 100644 --- a/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts +++ b/frontend/src/components/job/__tests__/JobFinishTab.labourHours.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' -import { mount, flushPromises } from '@vue/test-utils' +import { flushPromises } from '@vue/test-utils' const { finishRetrieveMock, invoicesRetrieveMock, costsSummaryRetrieveMock } = vi.hoisted(() => ({ finishRetrieveMock: vi.fn(), @@ -27,122 +27,80 @@ vi.mock('vue-sonner', () => ({ })) import JobFinishTab from '../JobFinishTab.vue' +import { + checklistState, + costSummary, + finishSummary, + mountFinishTab, + resetFinishTab, +} from './finishTabFixtures' /** - * KAN-222: office staff read labour budget, hours used and hours remaining - * without doing the subtraction, and an overrun shows as a positive number - * rather than a negative remainder. + * KAN-222: office staff read budget, used and remaining without doing the + * subtraction, and an overrun shows as a positive number rather than a negative + * remainder. */ -const summary = () => ({ - basis: 'quote', - job_value_excl_gst: 1000, - valid_invoiced_excl_gst: 0, - outstanding_invoiced_incl_gst: 0, - remaining_to_invoice_excl_gst: 1000, - remaining_gst: 150, - remaining_to_invoice_incl_gst: 1150, - total_to_pay_incl_gst: 1150, - over_invoiced_excl_gst: 0, -}) - -const checklist = () => ({ - foreman_signed_off: false, - timesheets_collected: false, - materials_checked: false, - customer_called: false, - released: false, -}) - -let mounted: ReturnType | null = null - -function mountWithHours( - { - estimateHours, - quoteHours, - actualHours, - }: { estimateHours: number; quoteHours: number; actualHours: number }, +async function mountWithHours( + hours: { estimate: number; quote: number; actual: number }, pricingMethodology = 'fixed_price', ) { - costsSummaryRetrieveMock.mockResolvedValue({ - estimate: { cost: 500, rev: 800, hours: estimateHours, profitMargin: 60 }, - quote: quoteHours > 0 ? { cost: 600, rev: 1000, hours: quoteHours, profitMargin: 66 } : null, - actual: { cost: 550, rev: 900, hours: actualHours, profitMargin: 63 }, - }) - mounted = mount(JobFinishTab, { - props: { jobId: 'job-1', pricingMethodology, jobStatus: 'in_progress' }, - attachTo: document.body, - }) - return mounted + costsSummaryRetrieveMock.mockResolvedValue(costSummary(hours)) + const wrapper = mountFinishTab(JobFinishTab, pricingMethodology) + await flushPromises() + return wrapper } -const hours = (wrapper: ReturnType, which: string) => +const tile = (wrapper: ReturnType, which: string) => wrapper.find(`[data-automation-id="JobFinishTab-labour-${which}"]`).text() -const remainingLabel = (wrapper: ReturnType) => - wrapper.find('[data-automation-id="JobFinishTab-labour-hours"]').text() - describe('JobFinishTab labour hours', () => { beforeEach(() => { vi.clearAllMocks() - finishRetrieveMock.mockResolvedValue({ summary: summary(), checklist: checklist() }) + finishRetrieveMock.mockResolvedValue({ + summary: finishSummary(), + checklist: checklistState(), + }) invoicesRetrieveMock.mockResolvedValue({ invoices: [] }) }) - afterEach(() => { - mounted?.unmount() - mounted = null - document.body.innerHTML = '' - }) - - it('shows budget, used and remaining for an under-budget job', async () => { - const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 12, actualHours: 8 }) - await flushPromises() - - expect(hours(wrapper, 'budget')).toBe('12') - expect(hours(wrapper, 'used')).toBe('8') - expect(hours(wrapper, 'remaining')).toBe('4') - expect(remainingLabel(wrapper)).toContain('Hours remaining') + afterEach(resetFinishTab) + + it('reports budget, used and remaining-or-overrun', async () => { + const cases = [ + { label: 'under budget', actual: 8, remaining: '4', heading: 'Hours remaining' }, + { label: 'exactly on budget', actual: 12, remaining: '0', heading: 'Hours remaining' }, + { label: 'over budget', actual: 15, remaining: '3', heading: 'Overrun' }, + ] + + for (const { label, actual, remaining, heading } of cases) { + const wrapper = await mountWithHours({ estimate: 10, quote: 12, actual }) + + expect(tile(wrapper, 'budget'), label).toBe('12') + expect(tile(wrapper, 'used'), label).toBe(String(actual)) + expect(tile(wrapper, 'remaining'), label).toBe(remaining) + expect(tile(wrapper, 'remaining'), label).not.toContain('-') + expect( + wrapper.find('[data-automation-id="JobFinishTab-labour-hours"]').text(), + label, + ).toContain(heading) + + resetFinishTab() + } }) - it('shows zero remaining and no overrun exactly on budget', async () => { - const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 12, actualHours: 12 }) - await flushPromises() + it('budgets from quote hours, falling back to estimate hours', async () => { + const cases = [ + { label: 'fixed price with a quote', quote: 12, methodology: 'fixed_price', budget: '12' }, + { label: 'fixed price, no quote', quote: 0, methodology: 'fixed_price', budget: '10' }, + { label: 'time and materials', quote: 12, methodology: 'time_materials', budget: '10' }, + ] - expect(hours(wrapper, 'remaining')).toBe('0') - expect(remainingLabel(wrapper)).toContain('Hours remaining') - expect(remainingLabel(wrapper)).not.toContain('Overrun') - }) - - it('shows an overrun as a positive number, not a negative remainder', async () => { - const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 12, actualHours: 15 }) - await flushPromises() - - expect(hours(wrapper, 'remaining')).toBe('3') - expect(remainingLabel(wrapper)).toContain('Overrun') - expect(hours(wrapper, 'remaining')).not.toContain('-') - }) - - it('budgets a fixed-price job from its quote hours', async () => { - const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 12, actualHours: 5 }) - await flushPromises() - - expect(hours(wrapper, 'budget')).toBe('12') - }) - - it('budgets a fixed-price job with no quote from its estimate hours', async () => { - const wrapper = mountWithHours({ estimateHours: 10, quoteHours: 0, actualHours: 5 }) - await flushPromises() - - expect(hours(wrapper, 'budget')).toBe('10') - }) + for (const { label, quote, methodology, budget } of cases) { + const wrapper = await mountWithHours({ estimate: 10, quote, actual: 5 }, methodology) - it('budgets a T&M job from its estimate hours', async () => { - const wrapper = mountWithHours( - { estimateHours: 10, quoteHours: 12, actualHours: 5 }, - 'time_materials', - ) - await flushPromises() + expect(tile(wrapper, 'budget'), label).toBe(budget) - expect(hours(wrapper, 'budget')).toBe('10') + resetFinishTab() + } }) }) diff --git a/frontend/src/components/job/__tests__/finishTabFixtures.ts b/frontend/src/components/job/__tests__/finishTabFixtures.ts new file mode 100644 index 000000000..84d0588d2 --- /dev/null +++ b/frontend/src/components/job/__tests__/finishTabFixtures.ts @@ -0,0 +1,89 @@ +import { vi } from 'vitest' +import { mount } from '@vue/test-utils' +import type { Component } from 'vue' + +/** Shared fixtures for the JobFinishTab specs, which all mount the same tree. */ + +export const finishSummary = (overrides: Record = {}) => ({ + job_value_excl_gst: 1000, + valid_invoiced_excl_gst: 400, + outstanding_invoiced_incl_gst: 460, + remaining_to_invoice_excl_gst: 600, + remaining_gst: 90, + remaining_to_invoice_incl_gst: 690, + total_to_pay_incl_gst: 1150, + over_invoiced_excl_gst: 0, + ...overrides, +}) + +export const checklistState = (overrides: Record = {}) => ({ + foreman_signed_off: false, + timesheets_collected: false, + materials_checked: false, + customer_called: false, + released: false, + ...overrides, +}) + +export const CHECKLIST_ITEMS = [ + 'foreman_signed_off', + 'timesheets_collected', + 'materials_checked', + 'customer_called', + 'released', +] as const + +export const costSummary = ( + hours: { estimate: number; quote: number; actual: number } = { + estimate: 10, + quote: 12, + actual: 11, + }, +) => ({ + estimate: { cost: 500, rev: 800, hours: hours.estimate, profitMargin: 60 }, + quote: hours.quote > 0 ? { cost: 600, rev: 1000, hours: hours.quote, profitMargin: 66 } : null, + actual: { cost: 550, rev: 900, hours: hours.actual, profitMargin: 63 }, +}) + +let mounted: ReturnType | null = null + +export function mountFinishTab( + component: Component, + pricingMethodology = 'fixed_price', + jobStatus = 'in_progress', +) { + // Mounted to document.body because the invoice modal renders through a + // reka-ui portal; resetFinishTab() must run in afterEach or the next test + // finds the previous modal. + mounted = mount(component, { + props: { jobId: 'job-1', pricingMethodology, jobStatus }, + attachTo: document.body, + }) + return mounted +} + +export function resetFinishTab() { + mounted?.unmount() + mounted = null + document.body.innerHTML = '' +} + +export const inModal = (id: string) => + document.body.querySelector(`[data-automation-id="${id}"]`) + +export const modalText = (id: string) => { + const el = inModal(id) + if (!el) throw new Error(`${id} is not in the modal`) + return el.textContent ?? '' +} + +/** The api-client mock shape every JobFinishTab spec needs. */ +export const finishTabApiMocks = () => ({ + finishRetrieveMock: vi.fn(), + invoicesRetrieveMock: vi.fn(), + costsSummaryRetrieveMock: vi.fn(), + checklistUpdateMock: vi.fn(), + createInvoiceMock: vi.fn(), + deleteInvoiceMock: vi.fn(), + toastErrorMock: vi.fn(), +}) diff --git a/frontend/src/composables/useJobFinancials.ts b/frontend/src/composables/useJobFinancials.ts deleted file mode 100644 index 5e9d22370..000000000 --- a/frontend/src/composables/useJobFinancials.ts +++ /dev/null @@ -1,68 +0,0 @@ -import { ref, type Ref } from 'vue' -import { api } from '../api/client' - -interface JobFinancials { - quoteTotal: number - actualTotal: number - invoiceTotal: number - toBeInvoiced: number -} - -export function useJobFinancials(jobId: Ref) { - const loading = ref(false) - const error = ref(null) - - const fetchJobFinancials = async (pricingMethodology?: string): Promise => { - loading.value = true - error.value = null - - try { - // Fetch both cost summary and invoices in parallel - const [costSummary, invoicesResponse] = await Promise.all([ - api.job_jobs_costs_summary_retrieve({ - params: { job_id: jobId.value }, - }), - api.job_jobs_invoices_retrieve({ - params: { job_id: jobId.value }, - }), - ]) - - // Extract amounts from cost summary - const quoteTotal = Number(costSummary.quote?.rev || 0) - const actualTotal = Number(costSummary.actual?.rev || 0) - - // Calculate invoice total from invoices - const invoices = invoicesResponse.invoices || [] - const invoiceTotal = invoices.reduce( - (sum: number, invoice: { total_excl_tax?: number }) => - sum + Number(invoice.total_excl_tax || 0), - 0, - ) - - // Calculate amount to invoice based on pricing methodology - const amountToInvoice = - pricingMethodology === 'fixed_price' && quoteTotal > 0 ? quoteTotal : actualTotal - - const toBeInvoiced = Math.max(0, amountToInvoice - invoiceTotal) - - return { - quoteTotal, - actualTotal, - invoiceTotal, - toBeInvoiced, - } - } catch (err) { - error.value = 'Failed to fetch job financial data' - console.error('Error fetching job financials:', err) - throw err - } finally { - loading.value = false - } - } - - return { - fetchJobFinancials, - loading: loading as Readonly>, - error: error as Readonly>, - } -} diff --git a/frontend/src/pages/jobs/[id]/(index).vue b/frontend/src/pages/jobs/[id]/(index).vue index 97f0806ab..1969d2690 100644 --- a/frontend/src/pages/jobs/[id]/(index).vue +++ b/frontend/src/pages/jobs/[id]/(index).vue @@ -313,7 +313,6 @@ import { useRoute, useRouter } from 'vue-router' import { useJobsStore } from '@/stores/jobs' import { useJobTabs } from '@/composables/useJobTabs' import { useJobHeaderAutosave } from '@/composables/useJobHeaderAutosave' -import { useJobFinancials } from '@/composables/useJobFinancials' import { useCompanyDefaultsStore } from '@/stores/companyDefaults' import { api } from '@/api/client' import { ArrowLeft, Printer } from 'lucide-vue-next' @@ -450,7 +449,6 @@ const handlePricingMethodologyUpdate = (newMethod: string) => { } // Initialize the job financials composable -const { fetchJobFinancials } = useJobFinancials(jobId) const handleRejectedChange = async () => { if (!jobHeader.value) return @@ -484,14 +482,16 @@ const handleRejectedChange = async () => { } else { // CHECKING - validate no money to be invoiced try { - // Use the composable to fetch financial data - const financials = await fetchJobFinancials(jobHeader.value.pricing_methodology || undefined) + // The server owns this figure; it excludes voided invoices and respects + // the price cap, neither of which the old local calculation did. + const { summary } = await api.job_jobs_finish_retrieve({ + params: { job_id: jobId.value }, + }) - // Check if there's money to be invoiced - if (financials.toBeInvoiced > 0) { + if (summary.remaining_to_invoice_excl_gst > 0) { // Show warning but allow proceeding const confirmed = confirm( - `Warning: Job has ${formatCurrency(financials.toBeInvoiced)} still to be invoiced. Are you sure you want to reject?`, + `Warning: Job has ${formatCurrency(summary.remaining_to_invoice_excl_gst)} still to be invoiced. Are you sure you want to reject?`, ) if (!confirmed) { diff --git a/mypy-baseline.txt b/mypy-baseline.txt index 9d9ff1636..51f22c9f9 100644 --- a/mypy-baseline.txt +++ b/mypy-baseline.txt @@ -819,7 +819,6 @@ apps/job/tests/test_kanban_search.py:0: error: Function is missing a return type apps/job/tests/test_kanban_search.py:0: error: Incompatible type for "company" of "Invoice" (got "Company | None", expected "Company | Combinable") [misc] apps/job/tests/test_kanban_search.py:0: error: Incompatible type for "company" of "Quote" (got "Company | None", expected "Company | Combinable") [misc] apps/job/tests/test_mcp_tool_integration.py:0: error: Call to untyped function "get_queryset" in typed context [no-untyped-call] -apps/job/tests/test_mcp_tool_integration.py:0: error: Missing positional argument "query" in call to "search_products" of "QuotingTool" [call-arg] apps/job/tests/test_modern_timesheet_views.py:0: error: Item "None" of "XeroPayItem | None" has no attribute "name" [union-attr] apps/job/tests/test_pdf_goldens.py:0: error: Function is missing a return type annotation [no-untyped-def] apps/job/tests/test_quote_modes.py:0: error: Call to untyped function "QuoteModeController" in typed context [no-untyped-call] From 9992798c0258db846a5b20c2cca522710349dcf0 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sat, 1 Aug 2026 19:32:51 +1200 Subject: [PATCH 7/8] fix: authenticate checklist tests the way the API is actually authenticated The completion-checklist tests used force_login, which only ever worked by accident: JWTAuthentication is the sole authenticator, and it short-circuits to None when ENABLE_JWT_AUTH is off, so the session force_login creates is never consulted. Locally .env turns JWT auth on and the authenticator's middleware fallback honours the session; CI copies .env.precommit, which turns it off, so every request 401'd. force_authenticate replaces the authenticator list outright and is what the rest of the suite uses. Also from review: - Scope the checklist save to the ticked fields. A bare Job.save() rewrites every column held on the instance, so a concurrent write to fully_invoiced or status was reverted to whatever this request read first. - Hold gst_rate to a fraction. The field was unbounded, and the API advertised -10 < rate < 10; a CHECK constraint carries it because the admin, management commands and the Xero sync all bypass serializer validation. - Report a failed balance reload after invoicing instead of leaving the pre-invoice figure on screen looking current. - Drop the casts around invoice creation. The mode arguments were typed str and only fit XeroInvoiceCreateRequest because a cast hid it, and the error payload was typed with a property the code below it did not read. - Give the invoice list stable automation ids so its tests stop selecting by CSS class and list position. --- apps/job/services/job_service.py | 5 ++- apps/job/tests/test_completion_checklist.py | 2 +- .../0020_company_defaults_gst_rate_bounds.py | 38 +++++++++++++++++++ apps/workflow/models/company_defaults.py | 12 ++++++ .../tests/test_company_defaults_api.py | 24 +++++++++++- frontend/schema.yml | 18 +++------ frontend/src/api/generated/api.ts | 6 +-- frontend/src/components/job/JobFinishTab.vue | 15 +++++++- .../src/components/job/JobInvoiceCard.vue | 36 +++++++++++------- .../__tests__/JobFinishTab.balance.test.ts | 6 +-- .../job/__tests__/finishTabFixtures.ts | 18 +++++++-- frontend/tests/job/job-xero-invoice.spec.ts | 2 +- 12 files changed, 139 insertions(+), 43 deletions(-) create mode 100644 apps/workflow/migrations/0020_company_defaults_gst_rate_bounds.py diff --git a/apps/job/services/job_service.py b/apps/job/services/job_service.py index 591de0193..f472cbe5a 100644 --- a/apps/job/services/job_service.py +++ b/apps/job/services/job_service.py @@ -73,7 +73,10 @@ def update_completion_checklist( """ for item, value in updates.items(): setattr(job, item, value) - job.save(staff=staff) + # Scoped to the ticked items so a concurrent write to an unrelated field — + # fully_invoiced, status, costing — is not overwritten with the value this + # request read before the user touched the checkbox. + job.save(staff=staff, update_fields=list(updates.keys())) return job diff --git a/apps/job/tests/test_completion_checklist.py b/apps/job/tests/test_completion_checklist.py index e60c47065..b13240e7d 100644 --- a/apps/job/tests/test_completion_checklist.py +++ b/apps/job/tests/test_completion_checklist.py @@ -46,7 +46,7 @@ def setUp(self) -> None: last_name="Person", is_office_staff=True, ) - self.client.force_login(self.office_staff) + self.client.force_authenticate(user=self.office_staff) def _patch(self, payload: dict[str, object]) -> Response: return self.client.patch(self.url, data=payload, format="json") diff --git a/apps/workflow/migrations/0020_company_defaults_gst_rate_bounds.py b/apps/workflow/migrations/0020_company_defaults_gst_rate_bounds.py new file mode 100644 index 000000000..42447e083 --- /dev/null +++ b/apps/workflow/migrations/0020_company_defaults_gst_rate_bounds.py @@ -0,0 +1,38 @@ +# Generated by Django 6.0.7 on 2026-08-01 07:24 + +from decimal import Decimal + +import django.core.validators +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("company", "0012_text_unset_constraints"), + ("workflow", "0019_company_defaults_gst_rate"), + ] + + operations = [ + migrations.AlterField( + model_name="companydefaults", + name="gst_rate", + field=models.DecimalField( + decimal_places=4, + default=Decimal("0.1500"), + help_text="Sales tax rate applied to amounts DocketWorks quotes before Xero has issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero remains authoritative for tax on invoices that exist.", + max_digits=5, + validators=[ + django.core.validators.MinValueValidator(Decimal("0")), + django.core.validators.MaxValueValidator(Decimal("0.9999")), + ], + ), + ), + migrations.AddConstraint( + model_name="companydefaults", + constraint=models.CheckConstraint( + condition=models.Q(("gst_rate__gte", 0), ("gst_rate__lt", 1)), + name="companydefaults_gst_rate_is_a_fraction", + ), + ), + ] diff --git a/apps/workflow/models/company_defaults.py b/apps/workflow/models/company_defaults.py index b2fcb10a8..9dfbc9ed0 100644 --- a/apps/workflow/models/company_defaults.py +++ b/apps/workflow/models/company_defaults.py @@ -1,6 +1,7 @@ from collections.abc import Iterable from decimal import Decimal +from django.core.validators import MaxValueValidator, MinValueValidator from django.db import models from django.db.models.base import ModelBase from solo.models import SingletonModel @@ -24,6 +25,10 @@ class CompanyDefaults(SingletonModel): max_digits=5, decimal_places=4, default=Decimal("0.1500"), + validators=[ + MinValueValidator(Decimal("0")), + MaxValueValidator(Decimal("0.9999")), + ], help_text=( "Sales tax rate applied to amounts DocketWorks quotes before Xero has " "issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero remains " @@ -368,6 +373,13 @@ class Meta: condition=models.Q(id=1), name="companydefaults_singleton", ), + # The validators above only fire through a serializer; the admin, + # management commands and the Xero sync all write straight to the + # model, so the rate is held to a fraction at the database. + models.CheckConstraint( + condition=models.Q(gst_rate__gte=0) & models.Q(gst_rate__lt=1), + name="companydefaults_gst_rate_is_a_fraction", + ), ] @classmethod diff --git a/apps/workflow/tests/test_company_defaults_api.py b/apps/workflow/tests/test_company_defaults_api.py index 75cc22506..2dbb02374 100644 --- a/apps/workflow/tests/test_company_defaults_api.py +++ b/apps/workflow/tests/test_company_defaults_api.py @@ -1,6 +1,7 @@ import uuid +from decimal import Decimal -from django.db import connection +from django.db import IntegrityError, connection, transaction from django.test.utils import CaptureQueriesContext from rest_framework import status from rest_framework.test import APIClient @@ -167,3 +168,24 @@ def test_patch_rejects_xero_quote_terms_over_4000_characters(self) -> None: payload["xero_quote_terms"], ["Ensure this field has no more than 4000 characters."], ) + + def test_patch_rejects_a_gst_rate_outside_a_fraction(self) -> None: + """The rate is a fraction: 0.15 means 15%, so 15 would tax 1500%.""" + for rate in (Decimal("-0.1500"), Decimal("15")): + with self.subTest(rate=rate): + response = self.client.patch( + "/api/company-defaults/", {"gst_rate": rate}, format="json" + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn("gst_rate", response.json()) + + def test_a_gst_rate_outside_a_fraction_cannot_reach_the_database(self) -> None: + """The admin, commands and Xero sync all bypass the serializer.""" + defaults = CompanyDefaults.get_solo() + + for rate in (Decimal("-0.1500"), Decimal("1.0000")): + with self.subTest(rate=rate): + defaults.gst_rate = rate + with self.assertRaises(IntegrityError), transaction.atomic(): + defaults.save() diff --git a/frontend/schema.yml b/frontend/schema.yml index ffa6f3414..93bfbbd02 100644 --- a/frontend/schema.yml +++ b/frontend/schema.yml @@ -10867,10 +10867,8 @@ components: gst_rate: type: number format: double - maximum: 10 - minimum: -10 - exclusiveMaximum: true - exclusiveMinimum: true + maximum: 0.9999 + minimum: 0 description: Sales tax rate applied to amounts DocketWorks quotes before Xero has issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero remains authoritative for tax on invoices that exist. @@ -11265,10 +11263,8 @@ components: gst_rate: type: number format: double - maximum: 10 - minimum: -10 - exclusiveMaximum: true - exclusiveMinimum: true + maximum: 0.9999 + minimum: 0 description: Sales tax rate applied to amounts DocketWorks quotes before Xero has issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero remains authoritative for tax on invoices that exist. @@ -17319,10 +17315,8 @@ components: gst_rate: type: number format: double - maximum: 10 - minimum: -10 - exclusiveMaximum: true - exclusiveMinimum: true + maximum: 0.9999 + minimum: 0 description: Sales tax rate applied to amounts DocketWorks quotes before Xero has issued an invoice, as a fraction (0.1500 = 15% NZ GST). Xero remains authoritative for tax on invoices that exist. diff --git a/frontend/src/api/generated/api.ts b/frontend/src/api/generated/api.ts index 21f03300f..c2274ada9 100644 --- a/frontend/src/api/generated/api.ts +++ b/frontend/src/api/generated/api.ts @@ -896,7 +896,7 @@ const CompanyDefaults = z.object({ company_acronym: z.string().max(10).nullish(), time_markup: z.number().gt(-1000).lt(1000).optional(), materials_markup: z.number().gt(-1000).lt(1000).optional(), - gst_rate: z.number().gt(-10).lt(10).optional(), + gst_rate: z.number().gte(0).lte(0.9999).optional(), wage_rate: z.number().gt(-10000).lt(10000).optional(), annual_leave_loading: z.number().gt(-1000).lt(1000).optional(), workshop_efficiency_factor: z.number().gt(-10).lt(10).optional(), @@ -963,7 +963,7 @@ const CompanyDefaultsRequest = z.object({ company_acronym: z.string().min(1).max(10).nullish(), time_markup: z.number().gt(-1000).lt(1000).optional(), materials_markup: z.number().gt(-1000).lt(1000).optional(), - gst_rate: z.number().gt(-10).lt(10).optional(), + gst_rate: z.number().gte(0).lte(0.9999).optional(), wage_rate: z.number().gt(-10000).lt(10000).optional(), annual_leave_loading: z.number().gt(-1000).lt(1000).optional(), workshop_efficiency_factor: z.number().gt(-10).lt(10).optional(), @@ -1029,7 +1029,7 @@ const PatchedCompanyDefaultsRequest = z company_acronym: z.string().min(1).max(10).nullable(), time_markup: z.number().gt(-1000).lt(1000), materials_markup: z.number().gt(-1000).lt(1000), - gst_rate: z.number().gt(-10).lt(10), + gst_rate: z.number().gte(0).lte(0.9999), wage_rate: z.number().gt(-10000).lt(10000), annual_leave_loading: z.number().gt(-1000).lt(1000), workshop_efficiency_factor: z.number().gt(-10).lt(10), diff --git a/frontend/src/components/job/JobFinishTab.vue b/frontend/src/components/job/JobFinishTab.vue index 754d90965..6dcd10913 100644 --- a/frontend/src/components/job/JobFinishTab.vue +++ b/frontend/src/components/job/JobFinishTab.vue @@ -109,7 +109,7 @@ :remaining-to-invoice="summary.remaining_to_invoice_excl_gst" :job-status="props.jobStatus" :paid="props.paid" - @invoices-changed="loadFinishSummary" + @invoices-changed="reloadFinishSummary" />
    @@ -438,6 +438,19 @@ async function loadAll() { onMounted(loadAll) +// Invoicing changed the balance, so a failed reload must say so rather than +// leave the pre-invoice figure on screen looking current. +async function reloadFinishSummary() { + try { + await loadFinishSummary() + } catch (error) { + log('Failed to reload Finish Job balance: %o', error) + summary.value = null + loadError.value = true + toast.error('Invoice saved, but the balance could not be refreshed. Reload the job.') + } +} + // v-model has already applied the new value optimistically; this persists it and // puts it back if the server refuses, so a tick never survives a failed save. async function onChecklistToggle(key: ChecklistItemKey) { diff --git a/frontend/src/components/job/JobInvoiceCard.vue b/frontend/src/components/job/JobInvoiceCard.vue index 18ee17f14..6aaf258bf 100644 --- a/frontend/src/components/job/JobInvoiceCard.vue +++ b/frontend/src/components/job/JobInvoiceCard.vue @@ -14,7 +14,12 @@ No invoices for this project
    -
      +
      • @@ -55,6 +61,7 @@ variant="destructive" size="icon" class="h-7 w-7" + :data-automation-id="`JobInvoiceCard-delete-${invoice.id}`" @click="deleteInvoiceOnXero(invoice.xero_id)" :disabled="!!deletingInvoiceId" > @@ -270,13 +277,14 @@ import { Button } from '../ui/button' import { Card, CardHeader, CardFooter, CardContent, CardDescription, CardTitle } from '../ui/card' import { Input } from '../ui/input' import { Badge } from '../ui/badge' -import type { AxiosError } from 'axios' +import { isAxiosError } from 'axios' import { z } from 'zod' const log = debug('job:invoice') type Invoice = z.infer type XeroInvoiceCreateRequest = z.infer +type XeroInvoiceCreateMode = XeroInvoiceCreateRequest['mode'] // Job statuses that mean the work itself is done. Purely a wording choice — // invoicing stays available at every status. @@ -298,7 +306,7 @@ const deletingInvoiceId = ref(null) const { xeroConnected } = useXeroConnection() const showInvoiceModal = ref(false) -const selectedInvoiceMode = ref(null) +const selectedInvoiceMode = ref(null) const invoicePercentInput = ref('') const invoiceAmountInput = ref('') @@ -352,13 +360,13 @@ function openInvoiceModal() { showInvoiceModal.value = true } -function selectInvoiceMode(mode: string) { +function selectInvoiceMode(mode: XeroInvoiceCreateMode) { selectedInvoiceMode.value = mode invoicePercentInput.value = '' invoiceAmountInput.value = '' } -async function executeCreateInvoice(mode: string) { +async function executeCreateInvoice(mode: XeroInvoiceCreateMode) { if (!props.jobId || isCreatingInvoice.value) return let percent: number | undefined @@ -386,9 +394,11 @@ async function executeCreateInvoice(mode: string) { isCreatingInvoice.value = true try { - const body = { mode } as XeroInvoiceCreateRequest - if (percent !== undefined) body.percent = percent - if (amount !== undefined) body.amount = amount + const body: XeroInvoiceCreateRequest = { + mode, + ...(percent !== undefined && { percent }), + ...(amount !== undefined && { amount }), + } const response = await api.xero_create_invoice_create(body, { params: { job_id: props.jobId }, @@ -406,12 +416,10 @@ async function executeCreateInvoice(mode: string) { } catch (err: unknown) { let msg = 'Unexpected error while trying to create invoice.' log('Error creating invoice: %o', err) - if ((err as AxiosError).isAxiosError) { - const axiosErr = err as AxiosError<{ message: string }> - const errorData = axiosErr.response?.data - if (typeof errorData === 'object' && errorData !== null && 'error' in errorData) { - msg = String((errorData as { error: string }).error) - } + // The payload is typed with the property this code actually reads, so the + // declared shape and the access below cannot drift apart. + if (isAxiosError<{ error?: string }>(err) && err.response?.data?.error) { + msg = err.response.data.error } toast.error(`Failed to create invoice: ${msg}`) } finally { diff --git a/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts b/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts index c572824b0..43ffeea70 100644 --- a/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts +++ b/frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts @@ -195,11 +195,7 @@ describe('JobFinishTab customer balance', () => { const wrapper = mountTab() await flushPromises() - await wrapper - .findAll('button') - .filter((b) => b.classes().join(' ').includes('h-7')) - .at(1)! - .trigger('click') + await wrapper.get('[data-automation-id="JobInvoiceCard-delete-inv-1"]').trigger('click') await flushPromises() expect(deleteInvoiceMock).toHaveBeenCalled() diff --git a/frontend/src/components/job/__tests__/finishTabFixtures.ts b/frontend/src/components/job/__tests__/finishTabFixtures.ts index 84d0588d2..2162fea4c 100644 --- a/frontend/src/components/job/__tests__/finishTabFixtures.ts +++ b/frontend/src/components/job/__tests__/finishTabFixtures.ts @@ -4,7 +4,7 @@ import type { Component } from 'vue' /** Shared fixtures for the JobFinishTab specs, which all mount the same tree. */ -export const finishSummary = (overrides: Record = {}) => ({ +const baseFinishSummary = { job_value_excl_gst: 1000, valid_invoiced_excl_gst: 400, outstanding_invoiced_incl_gst: 460, @@ -13,15 +13,25 @@ export const finishSummary = (overrides: Record = {}) => ({ remaining_to_invoice_incl_gst: 690, total_to_pay_incl_gst: 1150, over_invoiced_excl_gst: 0, - ...overrides, -}) +} -export const checklistState = (overrides: Record = {}) => ({ +const baseChecklistState = { foreman_signed_off: false, timesheets_collected: false, materials_checked: false, customer_called: false, released: false, +} + +// Typed against the base shapes so a typo in an override is a compile error +// rather than a silently ignored key that leaves the default in place. +export const finishSummary = (overrides: Partial = {}) => ({ + ...baseFinishSummary, + ...overrides, +}) + +export const checklistState = (overrides: Partial = {}) => ({ + ...baseChecklistState, ...overrides, }) diff --git a/frontend/tests/job/job-xero-invoice.spec.ts b/frontend/tests/job/job-xero-invoice.spec.ts index 31d2abbde..5cc818a9d 100644 --- a/frontend/tests/job/job-xero-invoice.spec.ts +++ b/frontend/tests/job/job-xero-invoice.spec.ts @@ -29,7 +29,7 @@ test.describe('job xero invoice', () => { await expect(remaining).toBeVisible() await expect(remaining).not.toHaveText(/\$0\.00/) - const invoiceItems = page.locator('ul[role="list"] li') + const invoiceItems = autoId(page, 'JobInvoiceCard-list').locator('li') const initialCount = await invoiceItems.count() const createInvoiceButton = autoId(page, 'JobFinishTab-create-invoice') From 44f305fb39ae771ea10021fb636e5e195e7173a3 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sat, 1 Aug 2026 19:42:50 +1200 Subject: [PATCH 8/8] fix: name the icon-only invoice buttons for screen readers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The open and delete buttons on each invoice row carry only an icon, so a screen reader announced them as unlabelled buttons. The label includes the invoice number because these render once per row — five buttons all reading "Delete" would not say which invoice they act on. --- frontend/src/components/job/JobInvoiceCard.vue | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frontend/src/components/job/JobInvoiceCard.vue b/frontend/src/components/job/JobInvoiceCard.vue index 6aaf258bf..3023d3e08 100644 --- a/frontend/src/components/job/JobInvoiceCard.vue +++ b/frontend/src/components/job/JobInvoiceCard.vue @@ -52,6 +52,7 @@ size="icon" class="h-7 w-7" :data-automation-id="`JobInvoiceCard-open-${invoice.id}`" + :aria-label="`Open invoice ${invoice.number} in Xero`" @click="goToInvoiceOnXero(invoice.online_url)" :disabled="!invoice.online_url" > @@ -62,6 +63,7 @@ size="icon" class="h-7 w-7" :data-automation-id="`JobInvoiceCard-delete-${invoice.id}`" + :aria-label="`Delete invoice ${invoice.number}`" @click="deleteInvoiceOnXero(invoice.xero_id)" :disabled="!!deletingInvoiceId" >