KAN-281: Restore phone-number display on the job-settings tab (main) - #438
Merged
corrin merged 31 commits intoJul 7, 2026
Merged
Conversation
The 2026-07-06 release moved phones into ClientContactMethod but the job-settings tab was never re-wired, so its Client Information card lost the phone it used to show. Surface the client's and the selected contact's primary phone as read-only text on payloads the tab already loads (no new endpoint, display-only, no data changes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
main is the integration branch; production is what servers run. Releases are promotion PRs main -> production; hotfixes cut from production and back-merge to main. deploy.sh and instance.sh now resolve origin/production by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds shared primary-phone resolution across client/contact models, surfaces phone fields through serializers, REST services, and frontend forms/tables, introduces a Xero read-only accounting provider gated by an ChangesClient and Job Phone Fields
Estimated code review effort: 4 (Complex) | ~75 minutes Xero Read-Only Mode
Estimated code review effort: 3 (Moderate) | ~40 minutes Production Branch Deployment
Estimated code review effort: 2 (Simple) | ~10 minutes Test Infrastructure and Misc Fixes
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/server/deploy.sh (1)
43-61: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the deploy.sh template test to expect
origin/production.apps/workflow/tests/test_xero_instance_templates.py:290-295still assertsTARGET_REF="origin/main", which is now out of sync withscripts/server/deploy.shand will leave the test failing.🤖 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 `@scripts/server/deploy.sh` around lines 43 - 61, The deploy.sh template test is still asserting the old default target ref, so update the test in test_xero_instance_templates to expect TARGET_REF="origin/production" instead of "origin/main". Keep the assertion aligned with the deploy.sh script’s TARGET_REF default and any related template checks that reference the deploy script behavior.Source: Learnings
frontend/src/components/job/JobSettingsTab.vue (2)
1097-1128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear
client_phonewhen changing the job client.confirmClientChangeand the client-save success path leavelocalJobData.client_phoneuntouched, so the old number can keep showing under “Client Phone” after the client changes. Reset it immediately and copyserverJobDetail.client_phoneback into local/header state on the save response.🤖 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/JobSettingsTab.vue` around lines 1097 - 1128, The client-change flow in confirmClientChange and the client-save success path is leaving client_phone stale, so clear localJobData.client_phone when the client is changed and make sure the save response copies serverJobDetail.client_phone back into both local state and the job header. Update the JobSettingsTab.vue client-change logic and the success handler that patches job data so the displayed “Client Phone” always reflects the current client.
1172-1218: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh
contactPhonewhen a client auto-assigns a contactfrontend/src/components/job/JobSettingsTab.vue:1097-1218— the client-change path and the same-contact early return update the name/display only, so the read-only phone field can stay blank afterconfirmClientChangeclears it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/job/JobSettingsTab.vue` around lines 1172 - 1218, The contact selection flow in handleContactSelected leaves contactPhone stale when the client auto-assigns a contact or when the same contact is reselected, so update that path to refresh the phone display too. In JobSettingsTab.vue, make sure both the early return for an unchanged contact_id and the normal contact update path assign contactPhone from the selected contact or fetched updatedContact response, alongside contactDisplayValue and localJobData fields. Ensure the confirmClientChange/autoset contact case restores the read-only phone field instead of only the name.
🧹 Nitpick comments (4)
frontend/src/components/job/__tests__/JobSettingsTab.phone.test.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFilename doesn't match the
ComponentName.test.tsconvention.This spec is named
JobSettingsTab.phone.test.tsrather thanJobSettingsTab.test.ts. As per coding guidelines, "Place unit test specs in a colocated__tests__folder with filenames matchingComponentName.test.ts". Consider merging these cases into the existingJobSettingsTab.test.ts(if one exists) or renaming to fit the convention.🤖 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__/JobSettingsTab.phone.test.ts` at line 1, The test file name does not follow the ComponentName.test.ts convention. Rename or merge the phone-specific spec for JobSettingsTab so it lives alongside the existing JobSettingsTab tests under the colocated __tests__ folder, and ensure the final filename matches JobSettingsTab.test.ts; use the JobSettingsTab test suite to keep the naming consistent with the component.Source: Coding guidelines
apps/client/services/client_rest_service.py (1)
423-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate contact-serialization logic across
get_job_contactandupdate_job_contact.Both methods build an identical dict (
id,name,position,is_primary,notes,phone) from aClientContact. Addingphonehere duplicates the same line in two places; a future field will need the same edit twice. Consider extracting a small_serialize_job_contact(contact)helper shared by both call sites.♻️ Suggested helper
`@staticmethod` def _serialize_job_contact(contact: ClientContact) -> Dict[str, Any]: return { "id": str(contact.id), "name": contact.name, "email": contact.email, "position": contact.position, "is_primary": contact.is_primary, "notes": contact.notes, "phone": contact.primary_phone_value(), }Then call
ClientRestService._serialize_job_contact(contact)in bothget_job_contactandupdate_job_contact.Also applies to: 499-507
🤖 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/client/services/client_rest_service.py` around lines 423 - 432, `get_job_contact` and `update_job_contact` both duplicate the same `ClientContact` serialization dict, including the new `phone` field. Extract the shared mapping into a small helper on `ClientRestService` such as `_serialize_job_contact(contact)` and have both methods call it instead of rebuilding the dict inline. Keep the helper responsible for all fields (`id`, `name`, `email`, `position`, `is_primary`, `notes`, `phone`) so future changes only need one edit.apps/client/models.py (1)
365-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating with
Client.primary_phone_value().The new
ClientContact.primary_phone_value()re-implements the same query/ordering logic thatClient.primary_phone_value()already uses (per the docstring itself: "same ordering as every other primary-phone consumer"). Two independent copies of this rule risk silently diverging if one is ever tweaked without the other.Consider extracting the shared logic into a small helper (e.g., a mixin, or a module-level function taking a
contact_methodsqueryset) that bothClientandClientContactcall.🤖 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/client/models.py` around lines 365 - 379, `ClientContact.primary_phone_value()` duplicates the same phone चयन/order logic already implemented in `Client.primary_phone_value()`, so extract that shared queryset-based lookup into a small helper and have both methods call it. Keep the existing ordering semantics (`-is_primary`, `label`, `value`) in one place, and update the two `primary_phone_value` methods to delegate to the shared helper instead of re-implementing the query.apps/job/serializers/job_serializer.py (1)
822-822: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
get_client_phoneimplementation.Identical body to
JobSerializer.get_client_phone(lines 205-208). Consider extracting a small shared helper (e.g. a mixin or module-level function) both serializers call, to avoid the two copies drifting apart later. Same N+1 caveat noted on the other occurrence applies here too.♻️ Example extraction
+def get_client_phone_value(client) -> str: + """The client's primary phone number, or "" when there is no client.""" + return client.primary_phone_value() if client else "" + + class JobSerializer(serializers.ModelSerializer): ... `@extend_schema_field`(serializers.CharField()) def get_client_phone(self, obj) -> str: - """The client's primary phone number, or "" when it has none.""" - return obj.client.primary_phone_value() if obj.client else "" + return get_client_phone_value(obj.client)Also applies to: 837-849
🤖 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/serializers/job_serializer.py` at line 822, The `JobSerializer.get_client_phone` logic is duplicated in the serializer method fields, so extract the shared phone-formatting lookup into a common helper or mixin that both serializer implementations call. Update the `JobSerializer` and the other serializer method(s) that return `client_phone` to delegate to that shared helper, keeping the existing behavior unchanged while avoiding drift between copies.
🤖 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/__tests__/JobSettingsTab.phone.test.ts`:
- Around line 32-47: The mocked useJobsStore in JobSettingsTab.phone.test.ts
skips the header-mapping logic, so add direct Pinia coverage for jobToHeader and
setHeader to exercise the new client_phone field. Extend the jobs store tests in
frontend/src/stores/__tests__/jobs.test.ts to verify client_phone is mapped into
the header object and persisted back through setHeader, using the existing
jobToHeader/setHeader symbols as the entry points.
---
Outside diff comments:
In `@frontend/src/components/job/JobSettingsTab.vue`:
- Around line 1097-1128: The client-change flow in confirmClientChange and the
client-save success path is leaving client_phone stale, so clear
localJobData.client_phone when the client is changed and make sure the save
response copies serverJobDetail.client_phone back into both local state and the
job header. Update the JobSettingsTab.vue client-change logic and the success
handler that patches job data so the displayed “Client Phone” always reflects
the current client.
- Around line 1172-1218: The contact selection flow in handleContactSelected
leaves contactPhone stale when the client auto-assigns a contact or when the
same contact is reselected, so update that path to refresh the phone display
too. In JobSettingsTab.vue, make sure both the early return for an unchanged
contact_id and the normal contact update path assign contactPhone from the
selected contact or fetched updatedContact response, alongside
contactDisplayValue and localJobData fields. Ensure the
confirmClientChange/autoset contact case restores the read-only phone field
instead of only the name.
In `@scripts/server/deploy.sh`:
- Around line 43-61: The deploy.sh template test is still asserting the old
default target ref, so update the test in test_xero_instance_templates to expect
TARGET_REF="origin/production" instead of "origin/main". Keep the assertion
aligned with the deploy.sh script’s TARGET_REF default and any related template
checks that reference the deploy script behavior.
---
Nitpick comments:
In `@apps/client/models.py`:
- Around line 365-379: `ClientContact.primary_phone_value()` duplicates the same
phone चयन/order logic already implemented in `Client.primary_phone_value()`, so
extract that shared queryset-based lookup into a small helper and have both
methods call it. Keep the existing ordering semantics (`-is_primary`, `label`,
`value`) in one place, and update the two `primary_phone_value` methods to
delegate to the shared helper instead of re-implementing the query.
In `@apps/client/services/client_rest_service.py`:
- Around line 423-432: `get_job_contact` and `update_job_contact` both duplicate
the same `ClientContact` serialization dict, including the new `phone` field.
Extract the shared mapping into a small helper on `ClientRestService` such as
`_serialize_job_contact(contact)` and have both methods call it instead of
rebuilding the dict inline. Keep the helper responsible for all fields (`id`,
`name`, `email`, `position`, `is_primary`, `notes`, `phone`) so future changes
only need one edit.
In `@apps/job/serializers/job_serializer.py`:
- Line 822: The `JobSerializer.get_client_phone` logic is duplicated in the
serializer method fields, so extract the shared phone-formatting lookup into a
common helper or mixin that both serializer implementations call. Update the
`JobSerializer` and the other serializer method(s) that return `client_phone` to
delegate to that shared helper, keeping the existing behavior unchanged while
avoiding drift between copies.
In `@frontend/src/components/job/__tests__/JobSettingsTab.phone.test.ts`:
- Line 1: The test file name does not follow the ComponentName.test.ts
convention. Rename or merge the phone-specific spec for JobSettingsTab so it
lives alongside the existing JobSettingsTab tests under the colocated __tests__
folder, and ensure the final filename matches JobSettingsTab.test.ts; use the
JobSettingsTab test suite to keep the naming consistent with the component.
🪄 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: d02f1d95-a6b7-4276-9d8b-09ffe3aebaaf
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (15)
CLAUDE.mdapps/client/models.pyapps/client/serializers.pyapps/client/services/client_rest_service.pyapps/client/tests/test_contact_methods.pyapps/job/serializers/job_serializer.pydocs/adr/0029-servers-run-the-production-branch.mddocs/adr/README.mddocs/updating.mdfrontend/schema.ymlfrontend/src/components/job/JobSettingsTab.vuefrontend/src/components/job/__tests__/JobSettingsTab.phone.test.tsfrontend/src/stores/jobs.tsscripts/server/deploy.shscripts/server/instance.sh
…ests The guard tests pin the deploy scripts' default ref on purpose; ADR 0029 moves that pin from origin/main to origin/production. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…uery The KAN-281 phone-display fields called primary_phone_value() from serializers, lazily querying contact_methods per request; nplusone (strict in dev) turned that into a 500 on the job-settings tab. Add ClientContactMethod.primary_phone_annotation as the single source of the primary-phone Subquery (and PRIMARY_PHONE_ORDERING as the single copy of the ordering rule), annotate every Job/contact-fetching call site, and back client_phone with AnnotatedCharField so an unannotated queryset crashes loudly instead of silently dropping the key. Migrate the two pre-existing hand-written Subqueries (workshop PDF, supplier search) to the shared helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Strict-mypy compliance for the tests added with the primary-phone annotation fix: typed helpers, _MonkeyPatchedResponse return type for the header GET helper, and module-level query-capture imports. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…phone response-only Annotate primary_phone from ClientContactMethod onto the clients list, client-search hydration, client-create response, and the contact viewset so the Phone column and job-contact phone display again. Split JobContactBaseSerializer out so the response serializer carries the phone while the update serializer never accepts one; regenerate schema and the frontend API client. Surface contact-load failures on the job-settings tab as toast + console.error instead of debugLog. Add regression tests for the job-contact endpoint and the clients-list Phone column. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ethods migration The ClientContactMethod migration deleted the phone UI without replacement in four places. Restore each on the new data model: - Clients list: Phone column, fed by primary_phone_annotation on the list/search/create-response querysets (ClientSearchResult.phone). - Contact lists: per-contact phone on the client detail Contacts card, ContactSelectionModal, and the contact picker label, via an annotated ClientContactViewSet queryset and ClientContactSerializer.phone. - Contact add/edit: the form's Phone input writes through set_primary_phone, which promotes/renumbers the owner's primary ClientContactMethod (ownership conflicts surface as 400) and enqueues a CRM call rematch. - Client create: CreateClientModal's Phone input (create mode only; editing stays in PhoneNumberManager) creates the client's primary method inside the create transaction, so a conflict rolls back the client and skips the Xero push. Blank phone input is a deliberate no-op: deleting numbers belongs to PhoneNumberManager. Phone search is not restored - the old release only ever searched names; its placeholder was aspirational. Also type-clean the KAN-281 test files and sync one resolved mypy-baseline entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three mandatory safety steps when refreshing this checkout's DB from production: back up the prod DB, clear the Xero token, and set accounting_provider to xero_readonly so the copy cannot write to the real Xero organisation. Distinct from the anonymised dev/UAT restore (restore-prod-to-nonprod.md). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (6)
apps/client/serializers.py (1)
52-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnqueue the phone rematch on commit.
set_primary_phone()is also called from_create_client_in_xero()insidetransaction.atomic(), sodelay()can run before the phone rows commit and rematch against incomplete data.Suggested change
- numbers = [number for number in {old_number, method.normalized_value} if number] - rematch_phone_calls_task.delay(sorted(numbers)) + numbers = [number for number in {old_number, method.normalized_value} if number] + transaction.on_commit(lambda: rematch_phone_calls_task.delay(sorted(numbers)))🤖 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/client/serializers.py` around lines 52 - 54, The phone rematch is being queued immediately from set_primary_phone(), which can fire inside transaction.atomic() before the phone records are committed; update the rematch trigger so rematch_phone_calls_task.delay(...) runs only after a successful commit. Use the set_primary_phone() flow in serializers.py as the entry point, and move the enqueue logic to a transaction.on_commit callback (or equivalent commit hook) so _create_client_in_xero() and other atomic callers only schedule the task once the database state is durable.apps/job/tests/test_job_rest_service.py (1)
73-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a phoneless-client case.
The two tests here cover the "has a primary phone" path, mirroring the annotation-preference tests in
test_contact_methods.py. Consider adding a case with noClientContactMethodrows to assertclient_phone == "", matchingprimary_phone_annotation's documented empty-string fallback and guarding against a future regression in these two service methods.🤖 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/tests/test_job_rest_service.py` around lines 73 - 115, Add a negative test case for a client with no ClientContactMethod rows in JobRestServiceClientPhoneTests to cover the empty-phone path. Reuse the existing _job_with_client_phone pattern or add a helper for a phoneless client, then assert that JobRestService.get_job_for_edit and JobRestService.get_job_summary return job["client_phone"] == "" when no primary phone exists. This will align the service tests with primary_phone_annotation’s empty-string fallback and guard both methods against regressions.apps/job/tests/test_job_header_view.py (1)
125-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuppress the Ruff B018 false positive.
Static analysis flags Line 131 as a useless attribute access, but
.dataaccess here is intentional (it triggers serialization insideassertRaisesto exercise the annotated-field guard). Per coding guidelines, add a# noqawith the specific code and justification to keep CI lint clean.🔧 Suggested suppression
with self.assertRaises(Exception) as ctx: - JobHeaderResponseSerializer(unannotated).data + JobHeaderResponseSerializer(unannotated).data # noqa: B018 -- access triggers serialization to assert it raises🤖 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/tests/test_job_header_view.py` around lines 125 - 132, The `.data` access in `test_serializer_requires_annotated_instance` is intentional because it triggers `JobHeaderResponseSerializer` serialization inside `assertRaises`, so suppress the Ruff B018 false positive at that exact expression. Add a targeted `# noqa` with the B018 code and a brief justification near the `JobHeaderResponseSerializer(...).data` access so CI lint remains clean without changing the test behavior.Source: Linters/SAST tools
apps/client/tests/test_contact_methods.py (1)
381-391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
_lazy_phone_querieshelper across three test classes.The same "no standalone
client_clientcontactmethodSELECT" check is copy-pasted inGetJobContactPhoneTests,ClientListPhoneTests, and inlined again inClientContactApiPhoneTests. Extracting it to a shared test mixin/utility would reduce drift risk if the convention changes.♻️ Suggested consolidation
+def assert_no_lazy_phone_queries(captured: CaptureQueriesContext) -> None: + lazy = [ + q["sql"] + for q in captured.captured_queries + if q["sql"].startswith('SELECT "client_clientcontactmethod"') + ] + assert lazy == [], f"Unexpected lazy phone queries: {lazy}"Then replace each class-local
_lazy_phone_queries/inline block with a call to the shared helper.Also applies to: 441-446, 509-514
🤖 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/client/tests/test_contact_methods.py` around lines 381 - 391, The lazy phone query check is duplicated in GetJobContactPhoneTests, ClientListPhoneTests, and ClientContactApiPhoneTests; consolidate it into one shared helper or mixin. Extract the existing _lazy_phone_queries logic into a reusable test utility, then replace each class-local implementation and inline block with calls to that shared helper. Keep the helper behavior identical by preserving the current filter for top-level SELECTs on client_clientcontactmethod.frontend/src/components/CreateClientModal.vue (1)
336-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFine, but slightly awkward unused-var workaround.
The
void _createOnlyPhonepattern to satisfy lint is a bit unusual; destructuring directly without binding an intermediate name would read more cleanly.♻️ Alternative without the unused-var workaround
- // Update existing client. ClientUpdateRequest has no phone field — - // client phone editing lives in PhoneNumberManager on the client detail page. - const { phone: _createOnlyPhone, ...updateFields } = cleanedData - void _createOnlyPhone - const updatePayload: ClientUpdateRequest = { ...updateFields } + // Update existing client. ClientUpdateRequest has no phone field — + // client phone editing lives in PhoneNumberManager on the client detail page. + const { phone, ...updateFields } = cleanedData + const updatePayload: ClientUpdateRequest = { ...updateFields }🤖 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/CreateClientModal.vue` around lines 336 - 341, The client update branch in CreateClientModal.vue uses an awkward unused-variable workaround for stripping phone from cleanedData. Refactor the props.editMode && props.clientId path so the phone field is removed without binding and voiding an intermediate name, while still building the ClientUpdateRequest payload from updateFields; keep the logic localized around the updatePayload construction.frontend/src/composables/useContactManagement.ts (1)
57-85: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFragile positional parsing of
"name - phone - email"indisplayValue.set().Splitting on the literal
' - 'assumes none ofname,phone, or🤖 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/composables/useContactManagement.ts` around lines 57 - 85, The displayValue setter in useContactManagement is using fragile positional parsing for the combined contact string, which can misassign fields when a name, phone, or email contains the separator. Update displayValue.get and displayValue.set to avoid relying on splitting a single formatted string in useContactManagement; instead parse the three fields more robustly or use a non-ambiguous structure so selectedContact and newContactForm are populated correctly even when values contain hyphens or the separator text.
🤖 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.
Nitpick comments:
In `@apps/client/serializers.py`:
- Around line 52-54: The phone rematch is being queued immediately from
set_primary_phone(), which can fire inside transaction.atomic() before the phone
records are committed; update the rematch trigger so
rematch_phone_calls_task.delay(...) runs only after a successful commit. Use the
set_primary_phone() flow in serializers.py as the entry point, and move the
enqueue logic to a transaction.on_commit callback (or equivalent commit hook) so
_create_client_in_xero() and other atomic callers only schedule the task once
the database state is durable.
In `@apps/client/tests/test_contact_methods.py`:
- Around line 381-391: The lazy phone query check is duplicated in
GetJobContactPhoneTests, ClientListPhoneTests, and ClientContactApiPhoneTests;
consolidate it into one shared helper or mixin. Extract the existing
_lazy_phone_queries logic into a reusable test utility, then replace each
class-local implementation and inline block with calls to that shared helper.
Keep the helper behavior identical by preserving the current filter for
top-level SELECTs on client_clientcontactmethod.
In `@apps/job/tests/test_job_header_view.py`:
- Around line 125-132: The `.data` access in
`test_serializer_requires_annotated_instance` is intentional because it triggers
`JobHeaderResponseSerializer` serialization inside `assertRaises`, so suppress
the Ruff B018 false positive at that exact expression. Add a targeted `# noqa`
with the B018 code and a brief justification near the
`JobHeaderResponseSerializer(...).data` access so CI lint remains clean without
changing the test behavior.
In `@apps/job/tests/test_job_rest_service.py`:
- Around line 73-115: Add a negative test case for a client with no
ClientContactMethod rows in JobRestServiceClientPhoneTests to cover the
empty-phone path. Reuse the existing _job_with_client_phone pattern or add a
helper for a phoneless client, then assert that JobRestService.get_job_for_edit
and JobRestService.get_job_summary return job["client_phone"] == "" when no
primary phone exists. This will align the service tests with
primary_phone_annotation’s empty-string fallback and guard both methods against
regressions.
In `@frontend/src/components/CreateClientModal.vue`:
- Around line 336-341: The client update branch in CreateClientModal.vue uses an
awkward unused-variable workaround for stripping phone from cleanedData.
Refactor the props.editMode && props.clientId path so the phone field is removed
without binding and voiding an intermediate name, while still building the
ClientUpdateRequest payload from updateFields; keep the logic localized around
the updatePayload construction.
In `@frontend/src/composables/useContactManagement.ts`:
- Around line 57-85: The displayValue setter in useContactManagement is using
fragile positional parsing for the combined contact string, which can misassign
fields when a name, phone, or email contains the separator. Update
displayValue.get and displayValue.set to avoid relying on splitting a single
formatted string in useContactManagement; instead parse the three fields more
robustly or use a non-ambiguous structure so selectedContact and newContactForm
are populated correctly even when values contain hyphens or the separator text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9959497-5dd8-4b7a-9659-784257759277
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (30)
apps/client/__init__.pyapps/client/models.pyapps/client/serializers.pyapps/client/services/client_rest_service.pyapps/client/tests/test_client_invoice_summary.pyapps/client/tests/test_contact_methods.pyapps/client/tests/test_job_contact_view.pyapps/client/views/client_contact_viewset.pyapps/client/views/client_rest_views.pyapps/job/serializers/__init__.pyapps/job/serializers/job_serializer.pyapps/job/services/job_rest_service.pyapps/job/services/workshop_pdf_service.pyapps/job/tests/test_job_header_view.pyapps/job/tests/test_job_rest_service.pyapps/job/views/job_rest_views.pyapps/purchasing/services/supplier_search_service.pyapps/workflow/tests/test_xero_instance_templates.pyfrontend/schema.ymlfrontend/src/components/ContactSelectionModal.vuefrontend/src/components/CreateClientModal.vuefrontend/src/components/__tests__/ClientLookup.test.tsfrontend/src/components/job/JobSettingsTab.vuefrontend/src/composables/__tests__/useContactManagement.test.tsfrontend/src/composables/useClientLookup.tsfrontend/src/composables/useContactManagement.tsfrontend/src/pages/crm/clients/(index).vuefrontend/src/pages/crm/clients/[id].vuefrontend/tests/fixtures/auth.tsmypy-baseline.txt
💤 Files with no reviewable changes (1)
- mypy-baseline.txt
✅ Files skipped from review due to trivial changes (2)
- frontend/src/components/tests/ClientLookup.test.ts
- apps/client/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/job/JobSettingsTab.vue
E2E runs against a prod-connected backend were writing real entities into the Xero tenant: ~4 contacts, an invoice (with PDF attachment and history note), and a quote per run, none of them ever cleaned up. Add a required XERO_READONLY env var (process-scoped, so a live server or celery worker sharing the DB is unaffected). When true, get_provider() swaps the Xero backend for XeroReadOnlyProvider: every write logs a warning and returns a well-formed fake result (fake UUIDs, INV-E2E-*/ QU-E2E-* numbers, _e2e_stub-marked raw_json) while reads, auth, and token refresh inherit unchanged. run_full_sync is blanket-blocked because it hides a stock push (sync_local_stock_to_xero). /api/xero/ping/ now reports the flag and the E2E global-setup pre-flight hard-fails unless the backend is in readonly mode. The unprefixed "S&T Stainless Limited" supplier in supplier-alias-search.spec.ts gains the [TEST] prefix so cleanup and safety checks can see it. Deployments must add XERO_READONLY=False to their .env (the var is required, matching the XERO_SYNC_PROJECTS convention); the instance template includes it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…number-display-on-the-job-settings-tab
CodeRabbit flagged update_contact returning success with a None external_id. The real provider's update path is an upsert (creates the contact and assigns an ID when none exists), so mirror that instead of failing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…number-display-on-the-job-settings-tab
CI's cold mypy run caught errors a warm local cache had hidden. Fix them at the contract level, no ignores added (three the branch had introduced are removed; net ignore count is down five): - Break the client->crm import cycle (lazy rematch-task import in apps/client/serializers.py); the cycle made mypy mis-infer a phone-annotated Client inside untouched xero/transforms.py. - Type Client.save (keyword-only, matching ClientContact.save; dead _include_auto_now_update_field helper removed), validate_for_xero, get_final_client; declare client: Client in sync_clients. - Contact write responses re-fetch through the primary_phone_annotation queryset instead of poking a synthetic attribute; the clients-list formatter takes WithAnnotations[Client, _ClientSummaryAnnotations]. - XERO_READONLY read via os.environ (required var, crash if absent); XeroQuoteManager.__init__ typed; quote create/delete views guard job.client is None and narrow request.user to Staff. - Readonly-provider test stubs _attach_workshop_pdf via patch.object. mypy-baseline shrinks by 18 resolved entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/workflow/views/xero/xero_view.py (1)
962-971: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication in
xero_disconnectresponse payload.Both branches build the identical
{"connected": False, "xero_readonly": settings.XERO_READONLY}dict.♻️ Optional dedup
+ disconnected_payload = {"connected": False, "xero_readonly": settings.XERO_READONLY} try: cache.delete("xero_tenant_id") try: active = get_active_app() except NoActiveXeroApp: logger.info("xero_disconnect: no active XeroApp; nothing to do") - return Response( - {"connected": False, "xero_readonly": settings.XERO_READONLY}, - status=status.HTTP_200_OK, - ) + return Response(disconnected_payload, status=status.HTTP_200_OK) wipe_tokens_and_quota(active) logger.info(f"Disconnected XeroApp {active.id} ({active.label})") - return Response( - {"connected": False, "xero_readonly": settings.XERO_READONLY}, - status=status.HTTP_200_OK, - ) + return Response(disconnected_payload, status=status.HTTP_200_OK)🤖 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/views/xero/xero_view.py` around lines 962 - 971, The xero_disconnect response payload is duplicated across both branches with the same connected/xero_readonly dict, so factor that shared Response body into a single reusable value in xero_disconnect and return it from both the inactive and active paths. Use the existing xero_disconnect flow, the wipe_tokens_and_quota call, and the logger.info line as anchors while keeping the response content identical.apps/client/models.py (1)
248-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
primary_phone_value()logic betweenClientandClientContact.Both methods filter/order/first on
contact_methodsidentically. Could extract a small shared helper (e.g., a mixin method or standalone function taking thecontact_methodsmanager) to avoid drift if the ordering logic changes later.♻️ Possible consolidation
+def _primary_phone_value(contact_methods) -> str: + method = ( + contact_methods.filter(method_type=ClientContactMethod.MethodType.PHONE) + .order_by(*PRIMARY_PHONE_ORDERING) + .first() + ) + return method.value if method else ""Then both
Client.primary_phone_valueandClientContact.primary_phone_valuecall_primary_phone_value(self.contact_methods).Also applies to: 364-377
🤖 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/client/models.py` around lines 248 - 261, The `primary_phone_value()` implementation in both `Client` and `ClientContact` is duplicated, so extract the shared filter/order/first lookup into a small helper or mixin method that accepts the `contact_methods` manager and returns the primary phone value. Update both `Client.primary_phone_value` and `ClientContact.primary_phone_value` to delegate to that shared helper so the ordering logic stays in one place and won’t drift over time..env.example (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder key alphabetically per dotenv-linter.
Static analysis flags
XERO_READONLYas out of order relative toXERO_SYNC_PROJECTS.Suggested reorder
XERO_DEFAULT_USER_ID=YOUR_XERO_USER_ID_FOR_TIME_ENTRIES +# Suppress all Xero writes for this process (E2E/test backends only; reads stay live) +XERO_READONLY=False XERO_SYNC_PROJECTS=False -# Suppress all Xero writes for this process (E2E/test backends only; reads stay live) -XERO_READONLY=False🤖 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 @.env.example around lines 47 - 48, The dotenv-linter warning is about key ordering in the environment example. Reorder the Xero-related entries in the .env.example section so XERO_READONLY appears in its alphabetical position relative to XERO_SYNC_PROJECTS, keeping the existing comments and values unchanged.Source: Linters/SAST tools
apps/workflow/tests/test_xero_readonly_provider.py (1)
32-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused helper
_no_api.
_no_apiisn't called anywhere in this file — allside_effect=usages pass_API_FORBIDDENdirectly. Consider removing it, or if intentionally kept for future use, annotate asNoReturninstead ofAny.🤖 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/tests/test_xero_readonly_provider.py` around lines 32 - 33, Unused helper _no_api is defined but never referenced in the test file, since the side_effect setup uses _API_FORBIDDEN directly. Remove _no_api if it is not needed, or if you want to keep it for future use, update its signature in the test module to use NoReturn instead of Any so the intent is explicit.
🤖 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/client/services/__init__.py`:
- Line 9: The exported symbols in __init__.py were hand-edited; regenerate the
module using scripts/update_init.py instead of directly changing the
import/__all__ in __init__.py. Review ClientRestService and
_ClientSummaryAnnotations in client_rest_service to decide whether the
underscore-prefixed annotation should remain internal and be removed from
__all__, or be renamed if it is intended to be part of the public API.
---
Nitpick comments:
In @.env.example:
- Around line 47-48: The dotenv-linter warning is about key ordering in the
environment example. Reorder the Xero-related entries in the .env.example
section so XERO_READONLY appears in its alphabetical position relative to
XERO_SYNC_PROJECTS, keeping the existing comments and values unchanged.
In `@apps/client/models.py`:
- Around line 248-261: The `primary_phone_value()` implementation in both
`Client` and `ClientContact` is duplicated, so extract the shared
filter/order/first lookup into a small helper or mixin method that accepts the
`contact_methods` manager and returns the primary phone value. Update both
`Client.primary_phone_value` and `ClientContact.primary_phone_value` to delegate
to that shared helper so the ordering logic stays in one place and won’t drift
over time.
In `@apps/workflow/tests/test_xero_readonly_provider.py`:
- Around line 32-33: Unused helper _no_api is defined but never referenced in
the test file, since the side_effect setup uses _API_FORBIDDEN directly. Remove
_no_api if it is not needed, or if you want to keep it for future use, update
its signature in the test module to use NoReturn instead of Any so the intent is
explicit.
In `@apps/workflow/views/xero/xero_view.py`:
- Around line 962-971: The xero_disconnect response payload is duplicated across
both branches with the same connected/xero_readonly dict, so factor that shared
Response body into a single reusable value in xero_disconnect and return it from
both the inactive and active paths. Use the existing xero_disconnect flow, the
wipe_tokens_and_quota call, and the logger.info line as anchors while keeping
the response content identical.
🪄 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: ea0e73f7-43c3-40c3-b304-83dcc4cede86
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (24)
.env.exampleCLAUDE.mdapps/client/models.pyapps/client/serializers.pyapps/client/services/__init__.pyapps/client/services/client_rest_service.pyapps/client/tests/test_contact_methods.pyapps/workflow/accounting/registry.pyapps/workflow/accounting/xero/readonly_provider.pyapps/workflow/api/xero/reprocess_xero.pyapps/workflow/api/xero/transforms.pyapps/workflow/apps.pyapps/workflow/serializers.pyapps/workflow/tests/test_xero_readonly_provider.pyapps/workflow/views/xero/xero_quote_manager.pyapps/workflow/views/xero/xero_view.pydocketworks/settings.pydocs/development_session.mddocs/restore-prod-to-hotfix.mdfrontend/schema.ymlfrontend/tests/purchasing/supplier-alias-search.spec.tsfrontend/tests/scripts/global-setup.tsmypy-baseline.txtscripts/server/templates/env-instance.template
💤 Files with no reviewable changes (1)
- mypy-baseline.txt
✅ Files skipped from review due to trivial changes (4)
- apps/workflow/api/xero/transforms.py
- frontend/tests/purchasing/supplier-alias-search.spec.ts
- apps/workflow/api/xero/reprocess_xero.py
- docs/development_session.md
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/client/services/client_rest_service.py
- apps/client/serializers.py
- frontend/schema.yml
- apps/client/tests/test_contact_methods.py
| if apps.ready: | ||
| from .client_merge_service import reassign_client_fk_records | ||
| from .client_rest_service import ClientRestService | ||
| from .client_rest_service import ClientRestService, _ClientSummaryAnnotations |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Regenerate __init__.py instead of hand-editing.
This file is autogenerated. Per repo convention, additions/removals of exported symbols should be applied by running python scripts/update_init.py, not by hand-editing the import/__all__ lists directly.
Separately, _ClientSummaryAnnotations is underscore-prefixed (private convention) yet is now exported in __all__, making it part of the public surface — worth reconsidering whether it should be renamed without the leading underscore if it's genuinely meant to be public, or kept out of __all__ if it's meant to stay internal.
Also applies to: 38-38
🤖 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/client/services/__init__.py` at line 9, The exported symbols in
__init__.py were hand-edited; regenerate the module using scripts/update_init.py
instead of directly changing the import/__all__ in __init__.py. Review
ClientRestService and _ClientSummaryAnnotations in client_rest_service to decide
whether the underscore-prefixed annotation should remain internal and be removed
from __all__, or be renamed if it is intended to be part of the public API.
Source: Path instructions
XERO_READONLY became a required setting; CI's .env.precommit did not define it, so settings import crashed (ImproperlyConfigured), taking down the backend test, type-check, and pre-commit jobs in one stroke. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.env.precommit (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: key ordering per dotenv-linter.
dotenv-linter flags that
XERO_READONLYshould be placed beforeXERO_REDIRECT_URIalphabetically. Purely cosmetic, but easy to fix.🤖 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 @.env.precommit at line 37, The dotenv key order in the environment config is out of alphabetical sequence, and `XERO_READONLY` should be placed before `XERO_REDIRECT_URI` to satisfy dotenv-linter. Update the ordering in the env file so the `XERO_*` entries remain alphabetically sorted, using the existing variable names as the reference for placement.Source: Linters/SAST tools
🤖 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/utils/__tests__/authConsoleErrors.test.ts`:
- Around line 1-8: The test file is importing auth-console-errors through a
parent-path reference instead of staying on the `@-alias` path. Update the imports
used by authConsoleErrors.test to resolve
createLoginSessionCheckConsoleAllowance, isUnauthenticatedSessionCheckResponse,
LOGIN_ME_PATH, UNAUTHENTICATED_SESSION_CHECK_CONSOLE_ERROR, and
CapturedBrowserError via a src-local fixture location or a small local test
wrapper. Keep the fixture reachable from frontend/src without using ../../../
traversal.
---
Nitpick comments:
In @.env.precommit:
- Line 37: The dotenv key order in the environment config is out of alphabetical
sequence, and `XERO_READONLY` should be placed before `XERO_REDIRECT_URI` to
satisfy dotenv-linter. Update the ordering in the env file so the `XERO_*`
entries remain alphabetically sorted, using the existing variable names as the
reference for placement.
🪄 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: 3c5ae51f-aa15-4882-8ed9-5de83e941469
📒 Files selected for processing (6)
.env.precommit.vscode/tasks.jsondocs/restore-prod-to-hotfix.mdfrontend/src/utils/__tests__/authConsoleErrors.test.tsfrontend/tests/fixtures/auth-console-errors.tsfrontend/tests/fixtures/auth.ts
✅ Files skipped from review due to trivial changes (2)
- .vscode/tasks.json
- docs/restore-prod-to-hotfix.md
Typing a description into the phantom cost-line row infers kind 'adjust' and recomputed unit_rev immediately, but the row has no unit_cost yet, so apply() threw an uncaught 'Missing cost line unit_cost' pageerror on every free-text entry. Harmless to users (unit_rev derives on unit_cost entry) but the new E2E console-error gate rightly fails the suite on it. Skip the premature derivation when unit_cost is absent; derivation still happens in the onUpdate:unit_cost handler once the user enters a value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_aligned_date_range snaps to Monday/Sunday week boundaries and clamps the start to CompanyDefaults.xero_payroll_start_date. The E2E payroll spec used to hardcode the snapped date, which only held where the clamp field was unset; the alignment oracle now lives here with controlled CompanyDefaults instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The hardcoded 2025-03-31 expectation only holds when CompanyDefaults.xero_payroll_start_date is unset; instances with Xero payroll history clamp the range start to it (correct behavior, verified by apps/accounting/tests/test_payroll_reconciliation_service.py). The E2E test keeps its real job: range in via the UI, report renders real data over live HTTP. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The hotfix in 9b49a99 added read-only Client Phone / Contact Phone fields to the Job Settings tab on the false premise that the tab used to show a phone; the reference production UI never did. Revert the added template blocks and their supporting frontend state, then remove every backend field/annotation/serializer key that existed only to feed them (JobSerializer.client_phone, JobHeaderResponseSerializer .client_phone, AnnotatedCharField, the job/contact-fetch .annotate() calls, JobContactResponseSerializer.phone, and the now-dead ClientContact.primary_phone_value() helper), per ADR 0017. Client list/search phone and the CRM/workshop-PDF phone paths are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding on 549523c: deleting test_job_contact_view.py wholesale left JobContactRestView.get/.put and ClientRestService.get_job_contact with no end-to-end coverage (routing, permissions, response-serializer validation). Restore the file trimmed of phone assertions and payload keys: GET returns the job's contact, PUT reassigns and echoes the new contact, and an unrecognized field in the PUT body (the removed phone key) is ignored rather than written or echoed back. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cost-entry: drop the explicit 15s timeouts on the five mutation waitForResponse helpers so they inherit the 30s actionTimeout; one response exceeding 15s under prod-size load was the only failure mode. Kanban search-then-drag: read the shared job's number from its job page instead of asserting the card is visible on the unfiltered board first — columns cap at 200 jobs and sibling specs demote the shared job, so on a prod-size board the card can be legitimately absent. The post-search and post-drag assertions (the actual regression guard) are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Task 2 of the ClientContactMethod phone-restore work (counterpart to Task 1's job-settings revert): the Edit Client modal lost phone editing when the 2026-07-06 migration moved phone off Client onto ClientContactMethod. Restore it on the new model: - ClientUpdateSerializer/ClientDetailResponseSerializer gain `phone`; update_client() applies non-blank phone via set_primary_phone() inside the same local-update transaction (a conflict rolls back the whole update, matching create's behaviour), and refetches with the primary_phone_annotation afterwards -- which also fixes update_client's return value missing the with_invoice_summary() aggregates _format_client_detail requires (previously an untested, silently-broken path). ValueError now explicitly passes through update_client's exception handling so validation failures (incl. phone ownership conflicts) surface as 400s instead of being swallowed into 500s by persist_and_raise. - get_client_by_id annotates its queryset with primary_phone_annotation so client detail reads carry phone too. - Regenerated schema.yml/api.ts for the new phone fields. - CreateClientModal.vue: phone input now always shown (create and edit), sent on update, and populated/read back through the new contract -- template now matches the old-prod reference exactly; JobSettingsTab.vue regains the two lines feeding the modal's clientData.phone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ename Self-review catch on 00b2335: the previous commit renamed _ClientSummaryAnnotations to _ClientPhoneAnnotations but left one docstring pointing at the old name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The revert's test rewrite dropped the only test exercising the header fetch for a job with client=None; the serializer's nullable client and contact fields were left unguarded. Shell jobs exist in production. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
schema.yml gained error_id in 1b1afad but the generated zod client was not regenerated, so the field would be stripped from parsed error bodies (ADR 0021 makes the generated client the only frontend API surface). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/client/serializers.py (2)
88-117: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrap
ClientContactSerializer.create()/update()in a transaction.set_primary_phone()can raiseserializers.ValidationErrorafter the contact row has already been written, and there’s no request-levelATOMIC_REQUESTSor localtransaction.atomic()here. A conflicting phone can leave a persisted contact or partial field update behind.🤖 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/client/serializers.py` around lines 88 - 117, Wrap ClientContactSerializer.create() and ClientContactSerializer.update() in a transaction so the contact write and set_primary_phone() behave atomically. The issue is that _apply_phone() can raise serializers.ValidationError after super().create() or super().update() has already persisted the ClientContact, leaving partial data behind. Fix this by using transaction.atomic() around the create/update flow (or around the super() call plus _apply_phone()) and keep the error handling in _apply_phone() unchanged.
17-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-digit phone input before calling
set_primary_phoneClientContactMethod.normalize_phone()returns""for strings with no digits, andsave()raisesValueErrorfor that case._apply_phone()only mapsDjangoValidationError, so inputs like"abc"will surface as a 500 instead of a validation 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 `@apps/client/serializers.py` around lines 17 - 61, Non-digit phone values can still reach set_primary_phone and fail later with a ValueError because ClientContactMethod.normalize_phone() returns an empty string. Update the serializer path that calls set_primary_phone, especially _apply_phone, to validate the incoming phone value has digits before delegating and raise a DjangoValidationError instead of letting the save() error escape. Use set_primary_phone and ClientContactMethod.normalize_phone as the key touchpoints when adding this early rejection.apps/client/services/client_rest_service.py (1)
297-401: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap
update_clientin the same error-persistence guard as the sibling methods. Unexpected DB failures and phone-ownershipValueErrors can escape without being written toAppError, breaking the class’s error-handling pattern.🤖 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/client/services/client_rest_service.py` around lines 297 - 401, Wrap ClientRestService.update_client in the same AppError persistence guard used by the sibling update methods so unexpected DB exceptions and phone ownership ValueError paths are captured instead of escaping. Use the existing error-handling pattern around the main body of update_client, including the local update branch and the _update_client_in_xero call, and make sure any caught exception is written to AppError before re-raising or returning.
🧹 Nitpick comments (2)
frontend/tests/kanban/kanban-drag-vanishing.spec.ts (1)
206-207: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid
networkidle— known Playwright anti-pattern for flaky waits.Playwright's docs explicitly mark
networkidleas discouraged: "'networkidle' - DISCOURAGED wait until there are no network connections for at least 500 ms. Don't use this method for testing, rely on web assertions to assess readiness instead." Kanban boards commonly have background polling/websocket updates, so this wait could hang or add unpredictable latency — the opposite of what this flakiness-fix PR intends.The subsequent
expect(getVisibleJobCard(...)).toBeVisible(...)already provides a proper web-first synchronization point; thenetworkidlewait looks unnecessary.♻️ Suggested fix
await page.goto('/kanban') - await page.waitForLoadState('networkidle') + await expect(page.getByPlaceholder('Search jobs...')).toBeVisible()🤖 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/tests/kanban/kanban-drag-vanishing.spec.ts` around lines 206 - 207, The Kanban Playwright test is using the discouraged page.waitForLoadState('networkidle') after page.goto('/kanban'), which can be flaky or hang when the board has ongoing background network activity. Remove that wait from the kanban-drag-vanishing spec and rely on the existing web assertion with getVisibleJobCard(...) / expect(...).toBeVisible() as the readiness signal instead.apps/client/tests/test_contact_methods.py (1)
397-533: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid coverage, but missing a create-time conflict/rollback test.
This class thoroughly covers list/create/update/promote/blank/conflict scenarios for contact phone, but (unlike
ClientCreatePhoneTests.test_create_with_conflicting_phone_rolls_back_clientforClient) there's no test asserting that a create request with a conflicting phone leaves no orphanClientContactbehind. See the related atomicity concern raised onapps/client/serializers.py(ClientContactSerializer.create()/_apply_phone()); a test here would catch that class of bug once the underlying behavior is confirmed/fixed.🤖 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/client/tests/test_contact_methods.py` around lines 397 - 533, Add a create-time rollback coverage test in ClientContactApiPhoneTests to mirror the existing client phone atomicity case: create a second Client with a conflicting phone, POST to ClientContactApiPhoneTests.URL with that phone, and assert the response is 400 and no ClientContact or ClientContactMethod was created for the attempted contact. Use the existing _contact helper and ClientContactSerializer.create()/ _apply_phone behavior as the target area to verify the create path remains atomic when a phone conflict occurs.
🤖 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/accounting/tests/test_payroll_reconciliation_service.py`:
- Line 44: The comment in test_payroll_reconciliation_service.py uses an
ambiguous en dash character that Ruff flags, so replace the `–` in the date
range text with a regular hyphen for consistent ASCII punctuation. Update the
inline comment near the payroll reconciliation test to use standard hyphen-minus
characters only, keeping the meaning the same.
---
Outside diff comments:
In `@apps/client/serializers.py`:
- Around line 88-117: Wrap ClientContactSerializer.create() and
ClientContactSerializer.update() in a transaction so the contact write and
set_primary_phone() behave atomically. The issue is that _apply_phone() can
raise serializers.ValidationError after super().create() or super().update() has
already persisted the ClientContact, leaving partial data behind. Fix this by
using transaction.atomic() around the create/update flow (or around the super()
call plus _apply_phone()) and keep the error handling in _apply_phone()
unchanged.
- Around line 17-61: Non-digit phone values can still reach set_primary_phone
and fail later with a ValueError because ClientContactMethod.normalize_phone()
returns an empty string. Update the serializer path that calls
set_primary_phone, especially _apply_phone, to validate the incoming phone value
has digits before delegating and raise a DjangoValidationError instead of
letting the save() error escape. Use set_primary_phone and
ClientContactMethod.normalize_phone as the key touchpoints when adding this
early rejection.
In `@apps/client/services/client_rest_service.py`:
- Around line 297-401: Wrap ClientRestService.update_client in the same AppError
persistence guard used by the sibling update methods so unexpected DB exceptions
and phone ownership ValueError paths are captured instead of escaping. Use the
existing error-handling pattern around the main body of update_client, including
the local update branch and the _update_client_in_xero call, and make sure any
caught exception is written to AppError before re-raising or returning.
---
Nitpick comments:
In `@apps/client/tests/test_contact_methods.py`:
- Around line 397-533: Add a create-time rollback coverage test in
ClientContactApiPhoneTests to mirror the existing client phone atomicity case:
create a second Client with a conflicting phone, POST to
ClientContactApiPhoneTests.URL with that phone, and assert the response is 400
and no ClientContact or ClientContactMethod was created for the attempted
contact. Use the existing _contact helper and ClientContactSerializer.create()/
_apply_phone behavior as the target area to verify the create path remains
atomic when a phone conflict occurs.
In `@frontend/tests/kanban/kanban-drag-vanishing.spec.ts`:
- Around line 206-207: The Kanban Playwright test is using the discouraged
page.waitForLoadState('networkidle') after page.goto('/kanban'), which can be
flaky or hang when the board has ongoing background network activity. Remove
that wait from the kanban-drag-vanishing spec and rely on the existing web
assertion with getVisibleJobCard(...) / expect(...).toBeVisible() as the
readiness signal instead.
🪄 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: fa4b9d91-e2dd-4849-bf50-d89949e95cf8
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (18)
apps/accounting/tests/test_payroll_reconciliation_service.pyapps/client/models.pyapps/client/serializers.pyapps/client/services/__init__.pyapps/client/services/client_rest_service.pyapps/client/tests/test_contact_methods.pyapps/client/tests/test_job_contact_view.pyapps/job/tests/test_job_header_view.pydocs/restore-prod-to-hotfix.mdfrontend/schema.ymlfrontend/src/components/CreateClientModal.vuefrontend/src/components/__tests__/CreateClientModal.test.tsfrontend/src/components/job/JobSettingsTab.vuefrontend/src/components/shared/SmartCostLinesTable.vuefrontend/src/components/shared/__tests__/SmartCostLinesTable.kindInference.test.tsfrontend/tests/job/job-cost-entry-data.spec.tsfrontend/tests/kanban/kanban-drag-vanishing.spec.tsfrontend/tests/reports/payroll-reconciliation.spec.ts
💤 Files with no reviewable changes (1)
- apps/client/models.py
✅ Files skipped from review due to trivial changes (1)
- docs/restore-prod-to-hotfix.md
corrin
deleted the
KAN-281-Restore-phone-number-display-on-the-job-settings-tab
branch
July 7, 2026 18:56
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-281: https://docketworks.atlassian.net/browse/KAN-281
Same branch as #437 (the
productionhotfix), PR'd tomainso the fix and the ADR 0029 production-branch workflow land on the integration line too — no back-merge needed later.See #437 for the full description and verification.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes