Release: first-class People and Company links (KAN-278), person archive, job-create fixes - #466
Merged
Conversation
All instances (prod/UAT/demo) are past the squash and have the baseline rows recorded in their ledgers, so the replaces lists are dead weight (the follow-up noted in PR #434). Removing them now makes the upcoming client->company app-label surgery unambiguous: no stale ("client", ...) entries for the migration loader to resolve. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ated API client) Pure structural rename - no behavioral change, zero row-value changes: - apps/client -> apps/company (app label 'company'), model Client -> Company - FK/field renames: Job.client, Invoice/Bill/CreditNote/Quote.client, PhoneCallRecord.client -> company; CompanyDefaults.shop_client -> shop_company, test_client_name -> test_company_name - ClientContact/ClientContactMethod/Supplier class names unchanged (Stage B) - Index/constraint renames (company_name_fts_idx, company_name_trgm_idx, crm_phone_company_call_idx, unique_company_* etc.) - relabel_client_app surgery command: pre-migrate ledger relabel (deletes the 23 historic ghost rows, keeps 0001_baseline), content-type relabel, client_* -> company_* table renames; wired into deploy.sh before migrate - Rename migrations on top of the squashed baselines: proxy Delete/Create dance around RenameModel (contenttypes trap), Remove-constraint -> RenameField -> Add-constraint ordering (Q()-condition trap) - API surface: api/clients/ -> api/companies/, client_id/client_name -> company_*; schema.yml + generated api.ts regenerated (hand-written frontend swept in the follow-up commit on this branch) - Persisted values deliberately unchanged (Step 3): telemetry domain 'client', JobEvent 'client_changed', kanban 'client_contains', data-quality cross_client/effective_client_id, crm client_match param Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hand-written frontend sweep matching the regenerated API client: - api.clients_* -> api.companies_*, schemas.Client* -> schemas.Company*, client_id/client_name -> company_id/company_name - File renames: CompanyLookup.vue, CreateCompanyModal.vue, CompanyDropdown.vue, InlineEditCompany.vue, useCompanyLookup.ts, companyService.ts, companyStore.ts, company-wrapper.ts - Route /crm/clients -> /crm/companies (typed-router regenerated) - data-automation-id values + Playwright specs updated in lockstep - Kept: src/api/client.ts (HTTP client), schemas.ClientContact* (backend class names unchanged until Stage B), Xero OAuth client_id, and the Step-3 persisted values (domain: 'client', source: 'client_lookup', client_match, cross_client/effective_client_id) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lay-on-the-job-settings-tab KAN-281: Restore phone-number display on the job-settings tab (main)
…mpany-rename # Conflicts: # apps/company/__init__.py # apps/company/models.py # apps/company/serializers.py # apps/company/services/__init__.py # apps/company/services/company_rest_service.py # apps/company/tests/test_company_invoice_summary.py # apps/company/tests/test_contact_methods.py # apps/company/tests/test_job_contact_view.py # apps/company/views/company_rest_views.py # apps/company/views/contact_viewset.py # apps/job/services/workshop_pdf_service.py # apps/purchasing/services/supplier_search_service.py # apps/workflow/api/xero/reprocess_xero.py # apps/workflow/views/xero/xero_quote_manager.py # apps/workflow/views/xero/xero_view.py # frontend/schema.yml # frontend/src/components/CreateCompanyModal.vue # frontend/src/components/job/JobSettingsTab.vue # frontend/src/composables/useCompanyLookup.ts # mypy-baseline.txt
KAN-278 Step 2: pure structural rename Client → Company
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
KAN-278 feat: people and companies first-class
fix: finish KAN-278 terminology cleanup
create.vue's submit catch swallowed every create-or-navigate failure by stashing form state to localStorage and calling window.location.reload(). The reload aborted the in-flight navigation and dropped the user on a blank create page with no error surfaced — a real failure presenting as an E2E timeout (see crm/phone-call-job-link flake). The four diagnostic statements after the reload were dead code. Split submit into two phases: - create: on failure, toast extractErrorMessage(error) and let the user retry (no reload, no permanent-disable state). - navigate: the job already exists, so a router.push rejection is reported as "created but the page did not open", never as a creation failure. Drop the localStorage round-trip and hasCreationError flag (the flag permanently disabled the submit button with no reload to reset it). onMounted no longer restores from localStorage — without a reload the in-memory form state already persists, so it just resets to a clean form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Changing a job's company cleared the linked person and the autosave delta included person_name, which the backend correctly rejects with 400 "Unsupported field 'person_name' in delta payload" — person_name is derived from job.person.name (read_only) and has no writable target. The rejected save meant waitForAutosave never resolved, surfacing as a 120s E2E timeout (edit-job-settings › change company) rather than a visible error. person_name legitimately blanks the displayed person via the optimistic patch. The bug is only that the two wire-payload builders forwarded it to the delta. Fix each at source: - useJobHeaderAutosave saveAdapter: drop person_name from allowedHeaderKeys and the company-change re-add; only person_id (the writable field) clears the person. Remove the now-dead person_name before-value branch. - JobSettingsTab saveAdapter: skip display-only keys when building the partial payload. The optimistic display path is untouched, and the backend's strict rejection remains the loud safety net for any future regression. Adds a regression test asserting a company-change save omits person_name from the wire. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Once jobService.createJob returns 201 the job exists. If the subsequent router.push rejects, the catch reset isSubmitting=false; with the form untouched canSubmit stayed true, so the submit button re-enabled and a second click created a duplicate job. Latch a jobCreated guard on creation success and add it to the submit button's disable binding, so re-submit is blocked regardless of navigation outcome. The create-failure resets are untouched (retry there is correct); the guard latches on success only, so it never blocks a legitimate retry. Addresses CodeRabbit review on PR #465. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
router.push resolves with a NavigationFailure (aborted/cancelled/duplicated) rather than rejecting, so the catch-only handling missed those cases: the flow fell through with isSubmitting stuck true and no error toast — a navigation failure masquerading as a hung spinner. Inspect the resolved result with isNavigationFailure() and route it, alongside the existing catch for thrown errors, through one reportNavFailure handler (log, toast, reset isSubmitting). Genuine success still falls through to unmount. Addresses CodeRabbit review on PR #465. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ange
JobSettingsTab's load watcher called GET /api/companies/jobs/{id}/person/
unconditionally, but consumed only .id/.name — both already present on the
JobHeaderResponse it was reacting to (person_id/person_name). When a company
change clears the person, that endpoint 404s (it raises when job.person is
None); Chromium auto-logs the failed load, tripping the E2E rule-30 guard even
though the app catches the 404. The fetch was vestigial (predates the header
carrying person fields).
Read person_id/person_name straight from the header instead, matching the
save/retry paths. No request -> no 404 -> the read-side 404 fallback goes too.
Also completes the useJobsStore mock (patchHeader) in the labourRate and urgent
JobSettingsTab tests; the person block previously swallowed the missing-mock
error inside its try/catch, which this change removes.
Retiring the endpoint itself is a tracked follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
loadPeople() applied every response unconditionally, so concurrent calls raced: creating a person triggers an unfiltered refetch, and searching triggers a filtered one. When the slower unfiltered response resolved last it overwrote the filtered search result, hiding the just-created person — a timing-dependent flake (E2E crm/people › link lifecycle). Add a monotonic request token so only the most recently started loadPeople applies its response; stale responses are dropped regardless of resolution order. Adds a regression test resolving the two calls out of order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace direct property mutation of localJobData/serverBaseline with spread replacements, per the frontend immutable-update convention (rules 8-9). personDisplayValue stays a direct primitive assignment. Addresses CodeRabbit review on PR #465. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…odal The first-class people cutover replaced ContactSelectionModal with PersonSelectionModal and silently dropped the per-person Edit (pencil) and Delete (trash) hover actions, the delete-confirmation overlay, the edit-mode form, and the composable's edit/delete methods — a regression vs production. Restore them, adapting production's copy (contact->person, client->company): - usePersonManagement: startEditPerson/cancelEdit/updatePerson (change-detected across the identity, company-link, and phone contact-method endpoints) plus deletePerson (unlink, surfacing the backend 400 phone-conflict detail). - PersonSelectionModal: Edit/Delete hover buttons, delete-confirm overlay, amber editing state, edit-mode title/label; new props/emits + automation ids. - PersonSelector: wire emits, prod-aligned toasts. - person_service.remove_company_link: archive the Person (is_active=False) when its last active company link is removed, so no searchable orphans remain. Also institutionalise the gate that would have caught this: a "No feature removal" rule in CLAUDE.md requiring a Feature Parity Inventory before any rewrite/cutover, plus a docs/plans/_template.md scaffold. Tests: backend sole-company-archive case; modal edit/delete unit tests; e2e edit + delete flows on the job-settings person selector. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…estore The archive-on-last-link cascade in 913cd92 broke the existing person link-lifecycle. Once a sole-company person's link was removed the Person was archived (is_active=False), but every person/link endpoint does get_object_or_404(Person, is_active=True) — so the person could no longer be viewed or their link restored (people.spec.ts restore step 404'd). The new model deliberately supports active, company-less people (test_directory_includes_person_without_company), so link removal must leave the person active and restorable. Remove the cascade; the delete button unlinks only, which is exact production parity. Orphan cleanup, if wanted, belongs in a separate explicit archive action, not tied to link removal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Archiving driven by links (last-link removal archives; restore/add un-archives), archived people stay viewable so the restore-link lifecycle keeps working, plus an explicit PersonDetail archive action and a directory show-archived filter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Nine TDD tasks: backend archive/un-archive on link removal/add, relaxed is_active gates, explicit archive endpoint, directory include_archived filter, client regen, directory + PersonDetail UI, and an e2e archive→restore spec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ract PersonSummary/PersonDetail are response-only serializers and always emit is_active, but ModelSerializer inferred required=False from the model's BooleanField(default=True), so the schema marked it optional and the generated zod rendered `is_active: z.boolean().optional()`. The type lied about the contract (ADR 0028), and `!person.is_active` on an absent field would badge every row as archived. Declare is_active explicitly on PersonSummarySerializer so the schema lists it as required; PersonDetailSerializer inherits it. The generated client now has `is_active: z.boolean()`. Drops the `=== false` workaround in the directory badge, and fixes the directory test fixture that omitted is_active (not a valid PersonSummary). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds an "Archived" badge and Archive-person action to the person detail page, and makes restoreLink refresh the full person (not just company links) so the badge clears after un-archiving via link restore.
Address CodeRabbit review on #464. The managers are service objects, not the HTTP boundary: the view is, and it already converts AlreadyLoggedException into a 500 carrying error_id. Their generic arms persisted then returned an error dict, so that path was unreachable. Re-raise instead (ADR 0001), which also fixes real bugs: - Unexpected server errors were reported to the frontend as HTTP 400: the manager set "status": 500 but the view ignores it and hardcodes 400. - The dict path dropped error_id, which ADR 0013 requires. Expected business failures still return success: False dicts -> 4xx. Only unexpected exceptions raise. Also fixes, found while verifying: - delete_document on both managers had no AlreadyLoggedException arm at all, so its bare except re-persisted an already-persisted failure. That is the double-persistence ADR 0001 exists to prevent. CodeRabbit missed this. - xero_quote_manager.delete_document returned "messages" as a list of dicts, but XeroDocumentSuccessResponseSerializer declares ListField(CharField()); DRF rejects a dict, so that path raised on serialization. Fixed the producer to match the contract (ADR 0015). - Four tests never configured xero_sales_branding_theme_id and so failed against the mandatory theme guard this PR added. - xero_view: chain the fallback AssertionError with `from exc` (Ruff B904). ADR 0001 gains an explicit chain-termination clause: the silence about where the chain ends is what made this review ambiguous. Views now pass message= to _build_xero_error_payload so the underlying exception text survives the move to the raise path (ADR 0013). Typing: create_document/delete_document now return a named XeroDocumentResponse TypedDict rather than being unannotated (ADR 0028). It is what surfaced the messages contract bug. mypy-baseline shrinks by 8. Frontend: the branding-theme E2E asserted the selector was enabled before the no-themes skip could run, but an empty theme list is exactly when the control stays disabled by design, so the test timed out instead of skipping. Wait for the load to settle, fail on the error marker, skip on the empty marker, then assert enabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes four regressions found in final review of the person-archive feature: - PersonContactMethodsView.get/post 404'd on archived people, which broke the PersonDetail page's Promise.all load and hid the restore-link button. Drop the is_active filter to match PersonDetailView. - classify_phone_ownership skipped archived people entirely, so a phone conflict with an archived owner surfaced as an unrecoverable "belongs to <empty>" company conflict instead of offering to restore the link. - removeLink refetched only companyLinks, leaving person.value stale so the Archived badge and Archive button never updated after removing the last link. Mirror restoreLink and reload the whole person. - archivePerson used an ad hoc error message instead of extractErrorMessage, inconsistent with its sibling handlers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…wnership The draft put contact-methods gating and phone-ownership for archived people out of scope. Both were load-bearing and are now fixed (d4e4d78): an archived person was unreachable (PersonDetail loads contact-methods in the same Promise.all, so its 404 collapsed the page and hid the restore-link button), and classify_phone_ownership skipping archived people regressed the "Restore company link" recovery path into an unrecoverable dead end. Record what remains genuinely out of scope: reuse of an archived person's number by a different person. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(jobs)+feat(crm): job create/edit failures, restore dropped person edit/delete, and person archive
SOLO_CACHE is None under tests (settings.py:892, settings_test.py:25), so django-solo's get_solo() reads straight through to the DB and clear_cache() short-circuits on `if cache_name:`. There is no cache to leak between tests, so registering clear_cache as a cleanup guards nothing and implies a hazard that cannot occur. Matches the existing convention in test_xero_branding_themes.py and test_xero_quota_floor.py: clear_cache() in setUp, no cleanup registered. Co-Authored-By: Claude Opus 4.8 (1M context) <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 |
…heme fix: apply Xero sales branding theme to quotes and invoices
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.
Release of
maintoproduction— 87 commits, first full release since the KAN-281 hotfix on 2026-07-07. The KAN-281 hotfix that lives only onproduction(#437) is already inmainvia #438, so nothing is lost;mainmerges intoproductioncleanly.Tickets: KAN-278, KAN-281
Headline: first-class People and Company links (KAN-278)
See ADR 0030. The CRM used to treat a company contact as a row owned by one company, mixing human identity (
name,email) with relationship-at-company (position,is_primary, Xero import key). This release splits them:Personowns identity,CompanyPersonLinkowns the relationship. Jobs and phone call records point atPerson. Contact methods are owned by exactly oneCompanyor onePerson; phone sharing is allowed only when all owners trace to a common company (unchanged legacy rows are grandfathered until edited).clientapp/model renamed tocompanyacross backend, migrations, generated API client, and frontend. Legacy CRM bookmarks route to the not-found page (ADR 0017 — no redirect shims).Person archive
Departed people can be retired without breaking history. An explicit archive endpoint plus an
include_archiveddirectory filter,is_activeon summaries, an Archived badge, and restore. An earlier attempt that auto-archived on last-link removal was reverted — it broke link restore; archiving is now explicit, and adding or restoring a company link un-archives.Job create/edit fixes (#465)
router.pushfailures are handled.person_nameis no longer sent in the job delta; the redundant job-person fetch that 404s after a company change is gone.Xero cleanup
XeroApp.tenant_idcolumn (ADR 0017) and the dead connections fallback inget_tenant_id(ADR 0015).Ops, docs, and dependencies
scripts/pull_prod_files.shto restore instance files to the hotfix checkout;docs/restore-prod-to-hotfix.mdupdated.optimizeDepspin for lazily-discovered heavy deps; teardown no longer reads the droppedXeroApp.tenant_idcolumn; hardened Xero safety checks.wsand@typescript-eslint/parserbumps.Deploy notes
21 new migrations, including the
client→companyapp-label rename and thePersonbackfill (company.0004–0008).The app-label rename needs one-time DB surgery — relabelling
django_migrations, content types, andclient_*tables.deploy.shrunsmanage.py relabel_client_appbeforemigrateautomatically, so no manual step is required. The command is idempotent (it keys off rows still recorded under the old label) and deploy aborts the instance with services stopped if it fails. It is marked TEMPORARY: remove the command and its deploy hook once every production instance has cut over and produced a verified company-schema backup.Take a backup before deploying — the data migrations rewrite contact rows into
Person/CompanyPersonLinkand apply the reviewed duplicate cleanup.