KAN-323: Finish Job workspace with GST-inclusive customer balance - #511
Merged
corrin merged 8 commits intoAug 1, 2026
Merged
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH
…cklist 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH
…alance 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds shared invoicing calculations, a Finish Job API, completion checklist tracking, configurable GST, and frontend invoicing and balance workflows. It also updates related tests, documentation, and static-analysis baselines. ChangesFinish Job workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
frontend/src/components/job/__tests__/finishTabFixtures.ts (1)
7-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the fixture overrides against the fixture shape.
Record<string, number>andRecord<string, boolean>accept any key. A misspelled key in a spec adds a new field instead of overriding one, and the assertion then tests the default value. UsePartial<ReturnType<...>>so the compiler rejects unknown keys.♻️ Proposed refactor
-export const finishSummary = (overrides: Record<string, number> = {}) => ({ +const FINISH_SUMMARY_DEFAULTS = { 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, +} + +export const finishSummary = (overrides: Partial<typeof FINISH_SUMMARY_DEFAULTS> = {}) => ({ + ...FINISH_SUMMARY_DEFAULTS, ...overrides, })Apply the same pattern to
checklistState.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/job/__tests__/finishTabFixtures.ts` around lines 7 - 26, Update the finishSummary and checklistState fixture factories to type overrides as Partial<ReturnType<typeof finishSummary>> and Partial<ReturnType<typeof checklistState>> respectively, while preserving their existing defaults and spread behavior so unknown override keys are rejected.frontend/src/components/job/JobInvoiceCard.vue (2)
388-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the shortcut casts and type the error payload correctly.
Two type contracts here do not describe the real data:
- Line 389 casts an incomplete literal with
as XeroInvoiceCreateRequest, then adds fields afterwards. Build the request object in one literal so the compiler checks it.- Line 410 declares the response body as
{ message: string }, but the code reads theerrorkey and casts again. Declare the shape the API returns.♻️ Proposed refactor
- 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 } : {}), + }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) - } + const errorData = (err as AxiosError<{ error?: string }>).response?.data + if (errorData?.error) { + msg = errorData.error + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/job/JobInvoiceCard.vue` around lines 388 - 415, Update the invoice creation flow in JobInvoiceCard so the Xero request body is constructed as a single object literal containing mode and any defined percent or amount fields, allowing it to satisfy XeroInvoiceCreateRequest without a shortcut cast. In the catch block, type AxiosError’s response payload with the actual error property consumed by the code, then read that typed property directly instead of casting the response data again.Source: Coding guidelines
44-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose stable
data-automation-idvalues for the invoice list and its row actions.JobInvoiceCard.vueprovides no automation ids for the invoice list or the per-row buttons, so both test layers select those elements by CSS class, ARIA role, and list position. Any layout change breaks the tests silently.
frontend/src/components/job/JobInvoiceCard.vue#L44-L82: add:data-automation-id="JobInvoiceCard-open-${invoice.id}"to the Xero link button,:data-automation-id="JobInvoiceCard-delete-${invoice.id}"to the delete button, anddata-automation-id="JobInvoiceCard-list"to theul.frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts#L198-L202: replace theh-7class filter and.at(1)index with a lookup on the new delete-button automation id.frontend/tests/job/job-xero-invoice.spec.ts#L32-L33: replacepage.locator('ul[role="list"] li')with a locator scoped to the newJobInvoiceCard-listid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/job/JobInvoiceCard.vue` around lines 44 - 82, Add stable automation identifiers in JobInvoiceCard.vue: set the list’s data-automation-id to JobInvoiceCard-list, and add invoice-id-based identifiers to the Xero link and delete buttons using JobInvoiceCard-open-${invoice.id} and JobInvoiceCard-delete-${invoice.id}. In frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts:198-202, replace the class-and-index lookup with the delete-button identifier. In frontend/tests/job/job-xero-invoice.spec.ts:32-33, scope the invoice locator to JobInvoiceCard-list instead of the role-based list selector.Source: Coding guidelines
frontend/src/components/job/JobFinishTab.vue (1)
369-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the checklist object mutations with immutable updates.
checklistis areactiveobject that is mutated in three places:v-modelon the checkbox,Object.assign(checklist, response.checklist), and the rollbackchecklist[key] = !checked. The coding guidelines require replacement instead of direct mutation. Use arefand assign a new object.♻️ Proposed refactor
-const checklist = reactive<JobCompletionChecklist>({ +const checklist = ref<JobCompletionChecklist>({ foreman_signed_off: false, timesheets_collected: false, materials_checked: false, customer_called: false, released: false, })- Object.assign(checklist, response.checklist) + checklist.value = { ...checklist.value, ...response.checklist }- const checked = checklist[key] + const checked = checklist.value[key] savingChecklistKey.value = key try { const response = await api.job_jobs_finish_partial_update( { [key]: checked }, { params: { job_id: props.jobId } }, ) - Object.assign(checklist, response.checklist) + checklist.value = { ...checklist.value, ...response.checklist } } catch (error) { log('Failed to save checklist item %s: %o', key, error) toast.error('Failed to save checklist') - checklist[key] = !checked + checklist.value = { ...checklist.value, [key]: !checked } } finally {The template binding becomes
v-model="checklist[item.key]"→v-model="checklist.value[item.key]"is not valid in templates; keepv-model="checklist[item.key]"working by binding explicitly instead:- v-model="checklist[item.key]" + :checked="checklist[item.key]" type="checkbox" - `@change`="onChecklistToggle(item.key)" + `@change`="onChecklistToggle(item.key, ($event.target as HTMLInputElement).checked)"Then
onChecklistToggle(key, checked)uses the passed value instead of reading it back.Also applies to: 443-459
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/job/JobFinishTab.vue` around lines 369 - 375, Replace the reactive checklist object with a ref-based checklist state in the component containing onChecklistToggle. Update the checkbox template binding to avoid mutating the ref-backed object directly, pass the new checked value into onChecklistToggle, and replace both Object.assign(checklist, response.checklist) and rollback checklist[key] mutations with assignments that create new checklist objects while preserving the existing toggle and rollback behavior.Source: Coding guidelines
apps/workflow/models/company_defaults.py (1)
23-32: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBound
gst_rateto a valid fraction.The field is admin-editable and now drives customer-facing balances in
build_finish_job_summary. A negative value produces negative GST and a total below the GST-exclusive amount. A value entered as15instead of0.15inflates every balance by 15x. Add validators, or aCheckConstraint, so an invalid rate is rejected at entry instead of reaching the counter.♻️ Proposed validation
gst_rate = models.DecimalField( max_digits=5, decimal_places=4, default=Decimal("0.1500"), + validators=[MinValueValidator(Decimal("0")), MaxValueValidator(Decimal("1"))], help_text=(Import the validators at the top of the module:
from django.core.validators import MaxValueValidator, MinValueValidator🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/workflow/models/company_defaults.py` around lines 23 - 32, Update the gst_rate field in CompanyDefaults to validate values as a fraction between 0 and 1 using Django’s MinValueValidator and MaxValueValidator, while preserving its existing precision and default.apps/accounting/services/invoice_calculation.py (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
Literalforbasis.
basisonly ever holds"quote"or"actual_revenue". ALiteralmakes the contract checkable and prevents a typo from reachingInvoiceCalculationResult.target_basis.♻️ Proposed typing tightening
+JobValuationBasis = Literal["quote", "actual_revenue"] + + `@dataclass`(frozen=True) class JobInvoicingBasis: """What a job is worth excluding tax, and which cost set says so.""" - basis: str + basis: JobValuationBasis target_total: DecimalAs per coding guidelines: "Use strict type annotations as real data contracts ... use named types for complex annotations."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/accounting/services/invoice_calculation.py` around lines 34 - 39, Update the basis annotation in JobInvoicingBasis to a Literal permitting only "quote" and "actual_revenue", importing Literal from typing as needed, so invalid values are rejected before reaching InvoiceCalculationResult.target_basis.Source: Coding guidelines
apps/job/models/job.py (2)
160-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare
COMPLETION_CHECKLIST_FIELDSas a tuple.Ruff reports RUF012 on these lines. The list is a mutable class attribute. Both consumers accept a tuple:
JobCompletionChecklistSerializer.Meta.fieldsandJobCompletionChecklistUpdateSerializer.Meta.fieldsinapps/job/serializers/job_serializer.pypass the value straight to DRF, and DRF accepts a tuple forMeta.fields. A tuple also prevents a caller from mutating the shared ordering.♻️ Proposed fix
- COMPLETION_CHECKLIST_FIELDS = [ + COMPLETION_CHECKLIST_FIELDS = ( "foreman_signed_off", "timesheets_collected", "materials_checked", "customer_called", "released", - ] + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/job/models/job.py` around lines 160 - 166, Change the class attribute COMPLETION_CHECKLIST_FIELDS from a list to a tuple while preserving the existing field order and values. Keep its use by JobCompletionChecklistSerializer.Meta.fields and JobCompletionChecklistUpdateSerializer.Meta.fields unchanged.Source: Linters/SAST tools
1171-1194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving the checklist handlers from a label mapping.
The five entries are identical except for the display label. The comment at Line 157 states that an item is declared once, but adding an item currently requires an edit here as well as in
COMPLETION_CHECKLIST_FIELDS. A single{field: label}mapping would keep the two in step.♻️ Sketch of the mapping approach
Define the labels next to the field list:
COMPLETION_CHECKLIST_LABELS = { "foreman_signed_off": "Foreman sign-off", "timesheets_collected": "Timesheets collected", "materials_checked": "Materials checked", "customer_called": "Customer called", "released": "Job released", } COMPLETION_CHECKLIST_FIELDS = tuple(COMPLETION_CHECKLIST_LABELS)Then replace the five handler entries with one expansion:
- "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", - ), + **{ + field: _handle_boolean_change( + "completion_checklist_updated", + "completion_checklist_updated", + label, + ) + for field, label in COMPLETION_CHECKLIST_LABELS.items() + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/job/models/job.py` around lines 1171 - 1194, Derive the completion checklist handlers from a single field-to-label mapping instead of maintaining five duplicate entries in the handler mapping. Update the checklist definitions near COMPLETION_CHECKLIST_FIELDS to define the labels once, derive the field tuple from that mapping, and generate each handler using the field and corresponding label so adding an item only requires one declaration.apps/workflow/migrations/0019_company_defaults_gst_rate.py (1)
18-23: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider bounding
gst_ratewith validators.
gst_ratefeeds directly into every Finish Job balance calculation (build_finish_job_summarymultiplies it againstremaining_excl). Nothing stops an operator from saving a negative or absurdly large rate through the admin/settings UI, which would silently corrupt the customer-facing total. AddMinValueValidator/MaxValueValidatorto the field.♻️ Proposed validator addition
+from django.core.validators import MaxValueValidator, MinValueValidator + migrations.AddField( model_name="companydefaults", name="gst_rate", field=models.DecimalField( decimal_places=4, default=Decimal("0.1500"), help_text="...", max_digits=5, + validators=[MinValueValidator(Decimal("0")), MaxValueValidator(Decimal("1"))], ), ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/workflow/migrations/0019_company_defaults_gst_rate.py` around lines 18 - 23, Update the gst_rate DecimalField in migration 0019_company_defaults_gst_rate.py to include MinValueValidator and MaxValueValidator bounds appropriate for a fractional GST rate, preventing negative or unreasonably large values while preserving the existing default and precision settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/job/services/job_service.py`:
- Around line 65-77: Update update_completion_checklist to pass an explicit
update_fields collection containing the checklist keys from updates when calling
job.save(staff=staff), matching the scoped-save pattern used by
recalculate_job_invoicing_state and preserving the existing checklist updates
and history behavior.
In `@frontend/schema.yml`:
- Around line 11265-11274: Update the API validation for CompanyDefaultsRequest
and PatchedCompanyDefaultsRequest so gst_rate uses a non-negative DecimalField
with unique_fields=['gst_rate'] and MinValueValidator(0). Apply validation at
write-time where the request serializer/API field is defined, and do not add
fallback handling for negative rates in the API schema.
In `@frontend/src/components/job/JobFinishTab.vue`:
- Around line 106-113: In the JobInvoiceCard invoices-changed binding, replace
the direct loadFinishSummary handler with a new reloadFinishSummary wrapper.
Have reloadFinishSummary await loadFinishSummary, catch failures, log the error,
and show the specified toast instructing the user to reload the job.
---
Nitpick comments:
In `@apps/accounting/services/invoice_calculation.py`:
- Around line 34-39: Update the basis annotation in JobInvoicingBasis to a
Literal permitting only "quote" and "actual_revenue", importing Literal from
typing as needed, so invalid values are rejected before reaching
InvoiceCalculationResult.target_basis.
In `@apps/job/models/job.py`:
- Around line 160-166: Change the class attribute COMPLETION_CHECKLIST_FIELDS
from a list to a tuple while preserving the existing field order and values.
Keep its use by JobCompletionChecklistSerializer.Meta.fields and
JobCompletionChecklistUpdateSerializer.Meta.fields unchanged.
- Around line 1171-1194: Derive the completion checklist handlers from a single
field-to-label mapping instead of maintaining five duplicate entries in the
handler mapping. Update the checklist definitions near
COMPLETION_CHECKLIST_FIELDS to define the labels once, derive the field tuple
from that mapping, and generate each handler using the field and corresponding
label so adding an item only requires one declaration.
In `@apps/workflow/migrations/0019_company_defaults_gst_rate.py`:
- Around line 18-23: Update the gst_rate DecimalField in migration
0019_company_defaults_gst_rate.py to include MinValueValidator and
MaxValueValidator bounds appropriate for a fractional GST rate, preventing
negative or unreasonably large values while preserving the existing default and
precision settings.
In `@apps/workflow/models/company_defaults.py`:
- Around line 23-32: Update the gst_rate field in CompanyDefaults to validate
values as a fraction between 0 and 1 using Django’s MinValueValidator and
MaxValueValidator, while preserving its existing precision and default.
In `@frontend/src/components/job/__tests__/finishTabFixtures.ts`:
- Around line 7-26: Update the finishSummary and checklistState fixture
factories to type overrides as Partial<ReturnType<typeof finishSummary>> and
Partial<ReturnType<typeof checklistState>> respectively, while preserving their
existing defaults and spread behavior so unknown override keys are rejected.
In `@frontend/src/components/job/JobFinishTab.vue`:
- Around line 369-375: Replace the reactive checklist object with a ref-based
checklist state in the component containing onChecklistToggle. Update the
checkbox template binding to avoid mutating the ref-backed object directly, pass
the new checked value into onChecklistToggle, and replace both
Object.assign(checklist, response.checklist) and rollback checklist[key]
mutations with assignments that create new checklist objects while preserving
the existing toggle and rollback behavior.
In `@frontend/src/components/job/JobInvoiceCard.vue`:
- Around line 388-415: Update the invoice creation flow in JobInvoiceCard so the
Xero request body is constructed as a single object literal containing mode and
any defined percent or amount fields, allowing it to satisfy
XeroInvoiceCreateRequest without a shortcut cast. In the catch block, type
AxiosError’s response payload with the actual error property consumed by the
code, then read that typed property directly instead of casting the response
data again.
- Around line 44-82: Add stable automation identifiers in JobInvoiceCard.vue:
set the list’s data-automation-id to JobInvoiceCard-list, and add
invoice-id-based identifiers to the Xero link and delete buttons using
JobInvoiceCard-open-${invoice.id} and JobInvoiceCard-delete-${invoice.id}. In
frontend/src/components/job/__tests__/JobFinishTab.balance.test.ts:198-202,
replace the class-and-index lookup with the delete-button identifier. In
frontend/tests/job/job-xero-invoice.spec.ts:32-33, scope the invoice locator to
JobInvoiceCard-list instead of the role-based list selector.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86752d47-c2d5-43a9-9986-43564b96b43d
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (39)
apps/accounting/services/__init__.pyapps/accounting/services/finish_job_summary.pyapps/accounting/services/invoice_calculation.pyapps/accounting/tests/test_finish_job_summary.pyapps/accounts/tests/test_auth_observability.pyapps/job/migrations/0010_job_completion_checklist_fields.pyapps/job/models/job.pyapps/job/models/job_event.pyapps/job/serializers/__init__.pyapps/job/serializers/job_serializer.pyapps/job/services/__init__.pyapps/job/services/job_service.pyapps/job/tests/test_completion_checklist.pyapps/job/tests/test_job_event_tracking.pyapps/job/tests/test_mcp_tool_integration.pyapps/job/urls_rest.pyapps/job/views/__init__.pyapps/job/views/job_rest_views.pyapps/workflow/accounting/xero/readonly_provider.pyapps/workflow/migrations/0019_company_defaults_gst_rate.pyapps/workflow/models/company_defaults.pyapps/workflow/models/settings_metadata.pyapps/workflow/tests/test_access_logging_middleware.pyapps/workflow/tests/test_backup_scripts.pydocs/urls/job.mdfrontend/schema.ymlfrontend/src/components/job/JobActualTab.vuefrontend/src/components/job/JobFinishTab.vuefrontend/src/components/job/JobInvoiceCard.vuefrontend/src/components/job/JobViewTabs.vuefrontend/src/components/job/__tests__/JobFinishTab.balance.test.tsfrontend/src/components/job/__tests__/JobFinishTab.checklist.test.tsfrontend/src/components/job/__tests__/JobFinishTab.labourHours.test.tsfrontend/src/components/job/__tests__/finishTabFixtures.tsfrontend/src/composables/useJobFinancials.tsfrontend/src/constants/job-tabs.tsfrontend/src/pages/jobs/[id]/(index).vuefrontend/tests/job/job-xero-invoice.spec.tsmypy-baseline.txt
💤 Files with no reviewable changes (4)
- frontend/src/composables/useJobFinancials.ts
- mypy-baseline.txt
- apps/workflow/tests/test_backup_scripts.py
- apps/job/tests/test_mcp_tool_integration.py
…icated 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/job/JobInvoiceCard.vue`:
- Line 54: Add accessible names to the icon-only buttons in JobInvoiceCard,
including the buttons associated with the automation IDs
JobInvoiceCard-open-${invoice.id} and the corresponding deletion action. Use
clear labels that identify the Xero navigation and invoice deletion actions for
screen readers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e06b03c7-7ca5-4fa0-8532-1006bb239335
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (11)
apps/job/services/job_service.pyapps/job/tests/test_completion_checklist.pyapps/workflow/migrations/0020_company_defaults_gst_rate_bounds.pyapps/workflow/models/company_defaults.pyapps/workflow/tests/test_company_defaults_api.pyfrontend/schema.ymlfrontend/src/components/job/JobFinishTab.vuefrontend/src/components/job/JobInvoiceCard.vuefrontend/src/components/job/__tests__/JobFinishTab.balance.test.tsfrontend/src/components/job/__tests__/finishTabFixtures.tsfrontend/tests/job/job-xero-invoice.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/tests/job/job-xero-invoice.spec.ts
- apps/job/tests/test_completion_checklist.py
- frontend/src/components/job/tests/finishTabFixtures.ts
- apps/job/services/job_service.py
- frontend/schema.yml
- frontend/src/components/job/JobFinishTab.vue
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.
corrin
deleted the
KAN-323-build-finish-job-workspace-with-gst-inclusive-customer-balance
branch
August 1, 2026 09:10
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
KAN-323 — also closes KAN-222.
Gives office staff one place to answer the counter question: what does this customer pay, including GST.
What changed
Cost Analysis becomes Finish Job. The tab leads with a server-owned customer balance — subtotal, GST, unpaid invoices, Total to pay — then the invoice card (moved out of Actual into
JobInvoiceCard.vue), then a five-item completion checklist, then the cost comparison retained underneath. Actual keeps materials, adjustments and cost entry.The balance is calculated server-side (
apps/accounting/services/finish_job_summary.py), reusing the existing invoicing basis rather than reimplementing it. It reports over-invoicing as a separate diagnostic instead of netting it off, and offers no invoice action when a job is fully covered rather than raising a ceremonial $0 invoice.CompanyDefaults.gst_rateis new — GST on an uninvoiced amount had no existing source. The cosmetic constant in the Xero read-only provider now reads it.Completion checklist as five booleans on Job, declared once in
Job.COMPLETION_CHECKLIST_FIELDS. Each tick writes a job-history event through the existing_FIELD_HANDLERSmachinery. The deadJob.collectedfield is renamed toreleased— it was never writable, so no data carries forward. Deliberately advisory: tests pin that ticking nothing still lets a job be invoiced.KAN-222 labour budget / used / remaining-or-overrun, in the component being rewritten anyway.
Duplication removed
pricing_methodologywas branched on four times to answer "what is this job worth". There is now oneget_job_invoicing_basis, andget_job_total_valueandrecalculate_job_invoicing_stateboth call it — which is whyfully_invoicedno longer ignores a price cap the invoice respects.useJobFinancials.tsis deleted: a third copy of the same sum in the browser that counted VOIDED invoices and ignored the cap.Bugs fixed
Job.DoesNotExistwas re-raised asValueErrorinside anexceptclause, which a sibling clause cannot catch — every job view answered 500 for a missing job. Now 404, tested.Verified
1123 backend tests, 432 frontend tests,
check_mypy.shclean (baseline shrank by 25), type-check and lint clean.Not verified:
frontend/tests/job/job-xero-invoice.spec.tswas retargeted to the Finish Job tab and strengthened, but has never been executed — it needs Vite and runserver.Deferred
Job.collectedsemantics beyond the rename, cash-vs-account archive conditions, inferred collection after seven days, paid-state sync, archived-job compliance. Separately: the remaining cost-set totals duplication (six representations of the same number) is planned as its own ticket.🤖 Generated with Claude Code
https://claude.ai/code/session_01ACLSprAHJSeQErMxQqLKDH
Summary by CodeRabbit