KAN-281: Restore phone-number display on the job-settings tab - #437
Merged
corrin merged 32 commits intoJul 7, 2026
Merged
Conversation
chore: batch Dependabot dependency updates
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…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>
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>
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>
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>
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
The 2026-07-06 release moved phone numbers into
ClientContactMethod; the job-settings tab's Client Information card was never re-wired and lost the phone it used to show. Data was always intact — this is a display-only fix, no migrations, no writes.Changes
ClientContactMethod.primary_phone_annotation(owner=..., outer_ref=...)is the single source of the primary-phone ordering rule (-is_primary, label, value). All Job-fetching call sites annotateclient_phone/contact_phone(header view,get_job_for_edit,get_job_summary,get_job_contact,update_job_contact); the serializers read the annotation viaAnnotatedCharFieldand crash loudly if a call site forgets to annotate. The original per-objectprimary_phone_value()approach tripped the nplusone guard (dev raisesNPlusOneError; prod would silently run extra queries per row) — the two pre-existing hand-written Subquery duplicates (workshop_pdf_service,supplier_search_service) are migrated to the shared helper in the same PR (ADR 0017).phone—JobContactRestViewround-trips payloads throughSerializer(data=...).is_valid()before returning.data, and DRF silently dropsread_only=Truefields from data-bound validation. Declaringphoneread-only therefore removed it from the GET/PUT responses entirely: the generated Zod client (which requiresphone: string) rejected the response, the tab showed "Failed to load contact information", and the Contact Person phone stayed blank. Fixed by splittingJobContactBaseSerializer: the response serializer declares a writable-for-validationphone, the update serializer has no phone field at all, so the write path still can't accept one.JobSettingsTab.vuereports a failed contact load withconsole.error+ toast instead ofdebugLog(frontend rule 30: no silent failures).phoneis no longer marked readOnly).productionbranch;mainis integration.deploy.sh/instance.shnow default toorigin/production; runbook + CLAUDE.md updated.Verification
apps/client/tests/test_job_contact_view.py(new): view-level GET/PUT tests pinning that the response carries the contact's primary phone — this is the test that would have caught the read-only drop.apps/job/tests/test_job_header_view.py(new): phone ordering, empty-phone and no-client cases, exact query counts (N+1 guard), ETag round-trip, and the crash-loudly contract for unannotated jobs.Status
Same branch is also PR'd to
main(#438) so the fix and workflow change exist on both lines.🤖 Generated with Claude Code