From 890867da6c3d91aa1dfaa3e4231ddbee2facfae8 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 09:12:53 +1200 Subject: [PATCH 1/4] docs+fix: record the Xero-first company merge model, align the stragglers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0034 records what the code already does: duplicate companies are merged in Xero, DocketWorks mirrors the merge into a permanent tombstone and moves everything the loser owned. The duplicate-identities report now tells the operator exactly that, since it is where duplicates surface. Three places contradicted the model and are aligned to it: - The merge_companies management command reassigned FKs and then hard- deleted the loser — the opposite of the tombstone the Xero path leaves. It now calls the same merge_companies() service, skips already-resolved tombstones, and re-runs cleanly. - set_company_fields set xero_archived on ARCHIVED but never cleared it on un-archive (old FIXME). Both flags now follow the archive transitions: archiving disables allow_jobs, un-archiving restores it. Only the transitions act — a steady-state sync never touches the flag, so a manual block on an active company (tax authorities, internal accounts) survives routine syncs, and a merged tombstone never gets jobs re-enabled. - list_companies showed merged tombstones alongside their winners — the same business twice. Browse and search now exclude them; a tombstone stays reachable by id. KAN-325 tracks the remaining gap: merge actions on the report itself and an ongoing person-dedup path. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- .../management/commands/merge_companies.py | 49 ++++++-------- .../0013_alter_company_allow_jobs.py | 21 ++++++ apps/company/models.py | 3 +- apps/company/services/company_rest_service.py | 5 +- apps/company/tests/test_company_fts_search.py | 18 +++++ .../tests/test_company_merge_service.py | 27 ++++++++ apps/workflow/api/xero/reprocess_xero.py | 24 +++---- apps/workflow/tests/test_sync_clients.py | 65 +++++++++++++++++++ .../adr/0034-company-merges-are-xero-first.md | 33 ++++++++++ docs/adr/README.md | 1 + .../data-quality/duplicate-identities.vue | 12 ++++ 11 files changed, 214 insertions(+), 44 deletions(-) create mode 100644 apps/company/migrations/0013_alter_company_allow_jobs.py create mode 100644 docs/adr/0034-company-merges-are-xero-first.md diff --git a/apps/company/management/commands/merge_companies.py b/apps/company/management/commands/merge_companies.py index 467d77d83..5464cacf5 100644 --- a/apps/company/management/commands/merge_companies.py +++ b/apps/company/management/commands/merge_companies.py @@ -1,9 +1,8 @@ from django.core.management.base import BaseCommand -from django.db import transaction from apps.accounts.models import Staff from apps.company.models import Company -from apps.company.services.company_merge_service import reassign_company_fk_records +from apps.company.services.company_merge_service import merge_companies from apps.job.models import Job from apps.workflow.models import CompanyDefaults @@ -39,10 +38,11 @@ def handle(self, *args, **options): f"Looking for duplicate companies with name: '{company_name}'" ) - # Find all companies with this name - duplicate_companies = Company.objects.filter(name=company_name).order_by( - "django_created_at" - ) + # Find all companies with this name. Tombstones are already resolved — + # they are neither duplicates to fix nor eligible primaries. + duplicate_companies = Company.objects.filter( + name=company_name, merged_into__isnull=True + ).order_by("django_created_at") count = duplicate_companies.count() if count == 0: @@ -95,30 +95,19 @@ def handle(self, *args, **options): self.stdout.write(self.style.WARNING("Operation cancelled")) return - # Merge duplicates - with transaction.atomic(): - for company, job_count in companies_with_job_counts[1:]: - self.stdout.write( - f"Merging company {company.pk} into {primary_company.pk}..." - ) - - # Reassign every company-FK record (Jobs, Invoices, Bills, - # Credit Notes, Quotes, POs, supplier references). The prior - # implementation only moved Jobs; the others ended up orphaned - # on the deleted company via the PROTECT constraint failure - # path — or silently lost on cascade with the old pointer. - counts = reassign_company_fk_records( - company, - primary_company, - Staff.get_automation_user(), - logger_prefix="[manual-merge] ", - ) - self.stdout.write(f" Reassigned records: {counts}") - - # Delete the duplicate company — safe now that every PROTECTed - # FK has been moved onto the primary. - company.delete() - self.stdout.write(f" Deleted duplicate company {company.pk}") + # Merge duplicates. Same tombstone semantics as the Xero-driven merge + # (ADR 0034): the loser keeps its row with merged_into set, so any + # late-arriving reference to it still resolves to the primary. + for company, job_count in companies_with_job_counts[1:]: + self.stdout.write( + f"Merging company {company.pk} into {primary_company.pk}..." + ) + counts = merge_companies( + company.pk, + primary_company.pk, + Staff.get_automation_user(), + ) + self.stdout.write(f" Reassigned records: {counts}") self.stdout.write( self.style.SUCCESS( diff --git a/apps/company/migrations/0013_alter_company_allow_jobs.py b/apps/company/migrations/0013_alter_company_allow_jobs.py new file mode 100644 index 000000000..cd44894e9 --- /dev/null +++ b/apps/company/migrations/0013_alter_company_allow_jobs.py @@ -0,0 +1,21 @@ +# Generated by Django 6.0.7 on 2026-08-02 02:00 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("company", "0012_text_unset_constraints"), + ] + + operations = [ + migrations.AlterField( + model_name="company", + name="allow_jobs", + field=models.BooleanField( + default=True, + help_text="If False, this company cannot be selected as the company on a Job. Use for Xero contacts that must exist (tax authorities, internal accounts, etc.) but should never appear on a job. Automatically set to False when a company is archived or merged in Xero, and restored on un-archive (merged companies stay blocked).", + ), + ), + ] diff --git a/apps/company/models.py b/apps/company/models.py index 9fe165c82..a61f7fbdf 100644 --- a/apps/company/models.py +++ b/apps/company/models.py @@ -98,7 +98,8 @@ class Company(models.Model): "If False, this company cannot be selected as the company on a Job. " "Use for Xero contacts that must exist (tax authorities, internal " "accounts, etc.) but should never appear on a job. Automatically " - "set to False when a company is archived or merged in Xero." + "set to False when a company is archived or merged in Xero, and " + "restored on un-archive (merged companies stay blocked)." ), ) xero_last_modified = models.DateTimeField(null=False, blank=False) diff --git a/apps/company/services/company_rest_service.py b/apps/company/services/company_rest_service.py index e982d12b5..82e79581a 100644 --- a/apps/company/services/company_rest_service.py +++ b/apps/company/services/company_rest_service.py @@ -151,8 +151,11 @@ def list_companies( if sort_dir.lower() == "desc": sort_field = f"-{sort_field}" + # Merged tombstones are excluded: their data lives on the winner + # (ADR 0034). They stay reachable by id on the detail endpoint. queryset = ( Company.objects.with_invoice_summary() + .filter(merged_into__isnull=True) .defer("raw_json") .annotate( phone=ContactMethod.primary_phone_annotation( @@ -164,7 +167,7 @@ def list_companies( # Apply search filter if query provided if query: ranked_ids = CompanyRestService._rank_matching_company_ids( - Company.objects.all(), query + Company.objects.filter(merged_into__isnull=True), query ) total_count = len(ranked_ids) offset = (page - 1) * page_size diff --git a/apps/company/tests/test_company_fts_search.py b/apps/company/tests/test_company_fts_search.py index f7d5271d5..051f67513 100644 --- a/apps/company/tests/test_company_fts_search.py +++ b/apps/company/tests/test_company_fts_search.py @@ -301,3 +301,21 @@ def test_search_companies_short_query_is_rejected(db): """The 3-character minimum-query guard returns [] without raising.""" assert CompanyRestService.search_companies("ab", limit=10) == [] assert CompanyRestService.search_companies("", limit=10) == [] + + +def test_merged_tombstone_is_excluded_from_browse_and_search(db: None) -> None: + """A merged company's data lives on the winner (ADR 0034); listing the + tombstone alongside it would show the same business twice.""" + winner = _make_company("Tombstone Winner Ltd") + _make_company("Tombstone Loser Ltd", merged_into=winner, allow_jobs=False) + + browsed = CompanyRestService.list_companies(page=1, page_size=50) + browsed_names = [c["name"] for c in browsed["results"]] + assert "Tombstone Winner Ltd" in browsed_names + assert "Tombstone Loser Ltd" not in browsed_names + + searched = CompanyRestService.list_companies( + query="Tombstone", page=1, page_size=50 + ) + searched_names = [c["name"] for c in searched["results"]] + assert searched_names == ["Tombstone Winner Ltd"] diff --git a/apps/company/tests/test_company_merge_service.py b/apps/company/tests/test_company_merge_service.py index 4a098811c..a49c4e89c 100644 --- a/apps/company/tests/test_company_merge_service.py +++ b/apps/company/tests/test_company_merge_service.py @@ -11,6 +11,7 @@ from decimal import Decimal from unittest.mock import patch +from django.core.management import call_command from django.db import IntegrityError from django.utils import timezone @@ -749,3 +750,29 @@ def test_rolls_back_tombstone_when_reassignment_fails(self) -> None: self.source.refresh_from_db() self.assertIsNone(self.source.merged_into_id) self.assertTrue(self.source.allow_jobs) + + +class MergeCompaniesCommandTests(BaseTestCase): + """The management command must produce the same end state as a + Xero-driven merge (ADR 0034): a tombstone, never a deletion.""" + + def test_command_tombstones_duplicates_and_moves_data(self) -> None: + primary = make_company("Dup Co") + duplicate = make_company("Dup Co") + primary_job = make_job(primary, self.test_staff) + duplicate_job = make_job(duplicate, self.test_staff) + + call_command("merge_companies", "--name", "Dup Co", "--auto") + + duplicate.refresh_from_db() # the old implementation deleted this row + self.assertEqual(duplicate.merged_into_id, primary.id) + self.assertFalse(duplicate.allow_jobs) + primary_job.refresh_from_db() + duplicate_job.refresh_from_db() + self.assertEqual(primary_job.company_id, primary.id) + self.assertEqual(duplicate_job.company_id, primary.id) + + # Re-running must treat the tombstone as resolved, not as a duplicate. + call_command("merge_companies", "--name", "Dup Co", "--auto") + duplicate.refresh_from_db() + self.assertEqual(duplicate.merged_into_id, primary.id) diff --git a/apps/workflow/api/xero/reprocess_xero.py b/apps/workflow/api/xero/reprocess_xero.py index 2d9d8f472..1e500a6c9 100644 --- a/apps/workflow/api/xero/reprocess_xero.py +++ b/apps/workflow/api/xero/reprocess_xero.py @@ -285,20 +285,20 @@ def set_company_fields(company: Company, new_from_xero: bool = False) -> None: if xero_contact_id_from_json: company.xero_contact_id = xero_contact_id_from_json - # Check for archived/merged status from raw_json + # xero_archived mirrors Xero in both directions. allow_jobs follows only + # on the transitions — archiving disables it, un-archiving restores it — + # never on a steady-state sync, so a manual block on an active company + # survives routine syncs (ADR 0034). A merged tombstone stays blocked + # regardless: its jobs belong to the winner. contact_status = raw_json.get("_contact_status", "ACTIVE") - if contact_status == "ARCHIVED": - company.xero_archived = True + was_archived = bool(company.xero_archived) + company.xero_archived = contact_status == "ARCHIVED" + if company.xero_archived: company.allow_jobs = False - # FIXME: asymmetric -- un-archiving in Xero does not reset either - # flag. If a contact is archived then un-archived, `xero_archived` - # and `allow_jobs` stay in the archived state until an admin toggles - # `allow_jobs` back on via the company detail UI. The un-archive - # path is rare enough that we accepted the asymmetry rather than - # introduce a "manually set" protection flag. If un-archive becomes - # common, revisit: (a) auto-reset both flags, which overwrites any - # manual admin-set `allow_jobs=False`; or (b) track admin overrides - # separately so they survive a sync. + elif was_archived and not company.merged_into_id: + company.allow_jobs = True + else: + pass # no archive transition; leave allow_jobs as the admin set it # Check for merge information merged_to_contact_id = raw_json.get("_merged_to_contact_id") diff --git a/apps/workflow/tests/test_sync_clients.py b/apps/workflow/tests/test_sync_clients.py index 6cbfdc218..d4bed4232 100644 --- a/apps/workflow/tests/test_sync_clients.py +++ b/apps/workflow/tests/test_sync_clients.py @@ -150,6 +150,71 @@ def _make_xero_contact(contact_id, name, status="ACTIVE", merged_to=None): return contact +class UnarchiveResetTests(TestCase): + """xero_archived mirrors Xero both ways; allow_jobs follows only on the + archive transitions, so a manual block survives routine syncs.""" + + def test_unarchive_restores_allow_jobs(self) -> None: + company = Company.objects.create( + name="Archive Roundtrip Ltd", + xero_last_modified=timezone.now(), + allow_jobs=True, + raw_json=_make_raw_json( + "contact-1", "Archive Roundtrip Ltd", status="ARCHIVED" + ), + ) + + set_company_fields(company) + self.assertTrue(company.xero_archived) + self.assertFalse(company.allow_jobs) + + company.raw_json = _make_raw_json( + "contact-1", "Archive Roundtrip Ltd", status="ACTIVE" + ) + set_company_fields(company) + + self.assertFalse(company.xero_archived) + self.assertTrue(company.allow_jobs) + + def test_manual_block_survives_steady_state_sync(self) -> None: + """No transition, no touch: an admin's allow_jobs=False on an active + company must not be flipped back by a routine sync.""" + company = Company.objects.create( + name="Blocked But Active Ltd", + xero_last_modified=timezone.now(), + allow_jobs=False, + xero_archived=False, + raw_json=_make_raw_json( + "contact-2", "Blocked But Active Ltd", status="ACTIVE" + ), + ) + + set_company_fields(company) + + self.assertFalse(company.xero_archived) + self.assertFalse(company.allow_jobs) + + def test_unarchived_merged_tombstone_stays_blocked(self) -> None: + """A merged loser's jobs belong to the winner; un-archiving its Xero + contact must not re-open it for jobs.""" + winner = Company.objects.create( + name="Tombstone Winner", xero_last_modified=timezone.now() + ) + loser = Company.objects.create( + name="Tombstone Loser", + xero_last_modified=timezone.now(), + allow_jobs=False, + xero_archived=True, + merged_into=winner, + raw_json=_make_raw_json("contact-3", "Tombstone Loser", status="ACTIVE"), + ) + + set_company_fields(loser) + + self.assertFalse(loser.xero_archived) + self.assertFalse(loser.allow_jobs) + + class SyncClientsArchivedContactTests(TestCase): """Regression tests for archived-contact name collisions during Xero sync. diff --git a/docs/adr/0034-company-merges-are-xero-first.md b/docs/adr/0034-company-merges-are-xero-first.md new file mode 100644 index 000000000..155ab904b --- /dev/null +++ b/docs/adr/0034-company-merges-are-xero-first.md @@ -0,0 +1,33 @@ +# 0034 — Company identity and merges are Xero-first + +Duplicate companies are merged in Xero; DocketWorks mirrors the merge and keeps the loser as a permanent tombstone. + +## Problem + +The same real-world business ends up as two Company rows — imported twice into Xero, typed twice at the counter, or created under a trading name. Each duplicate splits the customer's jobs, invoices, people, and phone numbers across two records, and every consumer that assumes one-company-one-identity (phone-number ownership, receivables, CRM history) degrades. Merging is therefore routine data hygiene, but a merge touches two systems: DocketWorks and Xero both hold the contact, and only one of them can be the source of truth for which record survives. + +## Decision + +Xero is authoritative for company identity. A duplicate pair that exists in Xero is merged **in Xero**; DocketWorks never pushes a merge to Xero. On the next sync or webhook, DocketWorks reads the losing contact's `MergedToContactID`, sets `merged_into` on the local loser, and reassigns everything the loser owned — jobs, invoices, bills, credit notes, quotes, purchase orders, person links, call records, and company-owned contact methods — to the winner via `company_merge_service.merge_companies()`. + +The loser is never deleted. It remains as a tombstone: `merged_into` set, `allow_jobs` false, excluded from lists and pickers, still resolvable by id. `Company.get_final_company()` follows the chain, so a Xero document arriving against a retired contact always lands on the surviving company. Tombstone semantics are the only semantics — every code path that retires a duplicate, including operator tooling for companies that exist only locally, goes through the same service. + +Person records are linked to the winner, not merged: company merge moves `CompanyPersonLink` rows (deduplicating against the winner's existing links) and leaves person-owned contact methods with the person, per ADR 0030. Deduplicating the people themselves is a separate task with its own service. + +`allow_jobs` follows the Xero archive state on its transitions: archiving a contact disables jobs, un-archiving restores them. Only the transitions act — a steady-state sync never touches the flag, so an operator's manual block on an active company survives routine syncs. A merged tombstone is the exception: un-archiving its contact never re-enables jobs, because its jobs belong to the winner. The corollary is that Xero is also where blocking belongs — to durably stop a company taking jobs, archive it in Xero rather than relying on the local toggle. + +## Why + +A merge must happen where the money lives. Xero owns invoices, payments, and the contact ledger; a merge done only in DocketWorks leaves the duplicate active in Xero, still accepting invoices and splitting the customer's receivables — locally tidy, financially wrong. Xero also already broadcasts merges (`MergedToContactID` survives on the archived contact), which gives DocketWorks a durable signal to converge on without any coordination protocol of its own. + +Tombstones rather than deletion because the retired row is load-bearing: it holds the `xero_contact_id` that future webhooks and documents will still reference, and deleting it would turn every late-arriving reference into a dangling id. + +## Alternatives considered + +- **Merge in DocketWorks and push to Xero:** Xero's API does not expose contact merging, so this is not implementable — only archiving, which silently strands the duplicate's Xero-side history. +- **Bidirectional merge with conflict resolution:** two writable sources of identity truth need a reconciliation protocol and still disagree during the window. One authority is simpler and sufficient. +- **Hard-delete the loser after reassignment:** leaves nothing for `get_final_company()` to resolve; any Xero document or webhook that still names the old contact id would create the duplicate again from scratch. + +## Consequences + +Operators need exactly one rule: merge it in Xero and let DocketWorks follow. Detection (the duplicate-identities report) can recommend a canonical company without owning remediation for Xero-linked pairs. The costs: tombstones accumulate forever and must be filtered wherever companies are listed or picked; local-only duplicates need their own operator entry point to the merge service, since Xero cannot trigger those; and a merged company's archived Xero contact keeps its phone numbers, so sync paths must skip merged companies rather than re-import data the winner now owns. diff --git a/docs/adr/README.md b/docs/adr/README.md index b49d67196..5b55dcb82 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -43,3 +43,4 @@ See [`_template.md`](_template.md). Copy, renumber, fill in. | 0031 | One logging gate: the debug library with namespaces | | 0032 | Less code is better: prefer libraries over homegrown implementations | | 0033 | Version constraints record what passed testing, not what is compatible | +| 0034 | Company identity and merges are Xero-first | diff --git a/frontend/src/pages/reports/data-quality/duplicate-identities.vue b/frontend/src/pages/reports/data-quality/duplicate-identities.vue index da37429c1..17c5ab280 100644 --- a/frontend/src/pages/reports/data-quality/duplicate-identities.vue +++ b/frontend/src/pages/reports/data-quality/duplicate-identities.vue @@ -25,6 +25,18 @@ +
+

How to resolve a duplicate company

+

+ Merge the two contacts in Xero. DocketWorks follows on the next sync: jobs, invoices, + and people move to the surviving company and the duplicate stays behind as an empty + archived record. +

+
+
From 6e2370b3db2e807ea61289cffacb6af9bfd9895e Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 14:57:20 +1200 Subject: [PATCH 2/4] docs: correct the merge ADR to verified sync facts, address review copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading the sync end-to-end corrected two claims before they shipped: - Every company is Xero-linked by construction — CRM create pushes the contact immediately and abandons the local row on failure. The dev DB's 3,409 unlinked companies are a seed artifact (seed_xero_from_database nulls every xero_contact_id and re-links only companies with jobs), not evidence of a local-only population. The ADR now says the merge-in-Xero rule is total, and the exact-name merge_companies command is a KAN-278 relic with no remaining remit, tracked for replacement and deletion on KAN-325 behind a feature-parity inventory. - The report guidance no longer calls the tombstone "empty" — it keeps its name and Xero identity; only what it owned moves. Also binds the unused job_count tuple element to _. KAN-325 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- apps/company/management/commands/merge_companies.py | 2 +- docs/adr/0034-company-merges-are-xero-first.md | 2 +- .../src/pages/reports/data-quality/duplicate-identities.vue | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/company/management/commands/merge_companies.py b/apps/company/management/commands/merge_companies.py index 5464cacf5..28016ca7f 100644 --- a/apps/company/management/commands/merge_companies.py +++ b/apps/company/management/commands/merge_companies.py @@ -98,7 +98,7 @@ def handle(self, *args, **options): # Merge duplicates. Same tombstone semantics as the Xero-driven merge # (ADR 0034): the loser keeps its row with merged_into set, so any # late-arriving reference to it still resolves to the primary. - for company, job_count in companies_with_job_counts[1:]: + for company, _ in companies_with_job_counts[1:]: self.stdout.write( f"Merging company {company.pk} into {primary_company.pk}..." ) diff --git a/docs/adr/0034-company-merges-are-xero-first.md b/docs/adr/0034-company-merges-are-xero-first.md index 155ab904b..5f317d185 100644 --- a/docs/adr/0034-company-merges-are-xero-first.md +++ b/docs/adr/0034-company-merges-are-xero-first.md @@ -30,4 +30,4 @@ Tombstones rather than deletion because the retired row is load-bearing: it hold ## Consequences -Operators need exactly one rule: merge it in Xero and let DocketWorks follow. Detection (the duplicate-identities report) can recommend a canonical company without owning remediation for Xero-linked pairs. The costs: tombstones accumulate forever and must be filtered wherever companies are listed or picked; local-only duplicates need their own operator entry point to the merge service, since Xero cannot trigger those; and a merged company's archived Xero contact keeps its phone numbers, so sync paths must skip merged companies rather than re-import data the winner now owns. +Operators need exactly one rule: merge it in Xero and let DocketWorks follow. This rule is total because every company is Xero-linked by construction — creating a company in the CRM pushes it to Xero immediately and abandons the local row if the push fails — so every duplicate pair is a Xero-linked pair. Detection (the duplicate-identities report) can recommend a canonical company without owning remediation. The costs: tombstones accumulate forever and must be filtered wherever companies are listed or picked, and a merged company's archived Xero contact keeps its phone numbers, so sync paths must skip merged companies rather than re-import data the winner now owns. The legacy `merge_companies` management command (exact-name matching, from the KAN-278 cleanup) predates this decision and has no remaining remit; replacing it with report-driven tooling and deleting it — after a feature-parity inventory — is tracked on KAN-325. diff --git a/frontend/src/pages/reports/data-quality/duplicate-identities.vue b/frontend/src/pages/reports/data-quality/duplicate-identities.vue index 17c5ab280..6965c0500 100644 --- a/frontend/src/pages/reports/data-quality/duplicate-identities.vue +++ b/frontend/src/pages/reports/data-quality/duplicate-identities.vue @@ -32,8 +32,8 @@

How to resolve a duplicate company

Merge the two contacts in Xero. DocketWorks follows on the next sync: jobs, invoices, - and people move to the surviving company and the duplicate stays behind as an empty - archived record. + and people move to the surviving company and the duplicate stays behind as an archived + record.

From 56330c9541d0c1ebd45b53693bc71cee84b93823 Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 16:49:39 +1200 Subject: [PATCH 3/4] fix: require a valid _contact_status before touching archive state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A missing status was guessed as ACTIVE, which since the un-archive transition change would silently un-archive a company and re-open it for jobs on malformed data. The status must now be present and one of ACTIVE/ARCHIVED/GDPRREQUEST or the company's sync fails (ADR 0015 — consumers stay strict). Real Xero payloads always carry it; the one test fixture that didn't now does. KAN-325 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- apps/workflow/api/xero/reprocess_xero.py | 10 +++++++++- apps/workflow/tests/test_sync_clients.py | 22 +++++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/apps/workflow/api/xero/reprocess_xero.py b/apps/workflow/api/xero/reprocess_xero.py index 1e500a6c9..f896689f8 100644 --- a/apps/workflow/api/xero/reprocess_xero.py +++ b/apps/workflow/api/xero/reprocess_xero.py @@ -290,7 +290,15 @@ def set_company_fields(company: Company, new_from_xero: bool = False) -> None: # never on a steady-state sync, so a manual block on an active company # survives routine syncs (ADR 0034). A merged tombstone stays blocked # regardless: its jobs belong to the winner. - contact_status = raw_json.get("_contact_status", "ACTIVE") + # No fallback for a missing status: guessing ACTIVE would un-archive the + # company and re-open it for jobs. Malformed data fails this company's + # sync (ADR 0015 — consumers stay strict). + contact_status = raw_json.get("_contact_status") + if contact_status not in ("ACTIVE", "ARCHIVED", "GDPRREQUEST"): + raise ValueError( + f"Company {company.id} raw_json has missing or unknown " + f"_contact_status {contact_status!r}" + ) was_archived = bool(company.xero_archived) company.xero_archived = contact_status == "ARCHIVED" if company.xero_archived: diff --git a/apps/workflow/tests/test_sync_clients.py b/apps/workflow/tests/test_sync_clients.py index d4bed4232..50f0585ee 100644 --- a/apps/workflow/tests/test_sync_clients.py +++ b/apps/workflow/tests/test_sync_clients.py @@ -176,6 +176,25 @@ def test_unarchive_restores_allow_jobs(self) -> None: self.assertFalse(company.xero_archived) self.assertTrue(company.allow_jobs) + def test_missing_status_fails_rather_than_unarchiving(self) -> None: + """A malformed payload with no _contact_status must not be guessed as + ACTIVE — that would un-archive the company and re-open it for jobs.""" + malformed = _make_raw_json("contact-4", "Malformed Status Ltd") + del malformed["_contact_status"] + company = Company.objects.create( + name="Malformed Status Ltd", + xero_last_modified=timezone.now(), + allow_jobs=False, + xero_archived=True, + raw_json=malformed, + ) + + with self.assertRaisesRegex(ValueError, "_contact_status"): + set_company_fields(company) + + self.assertTrue(company.xero_archived) + self.assertFalse(company.allow_jobs) + def test_manual_block_survives_steady_state_sync(self) -> None: """No transition, no touch: an admin's allow_jobs=False on an active company must not be flipped back by a routine sync.""" @@ -371,6 +390,7 @@ def _client_with_phone( name=name, xero_last_modified=timezone.now(), raw_json={ + "_contact_status": "ACTIVE", "_phones": [ { "_phone_type": phone_type, @@ -378,7 +398,7 @@ def _client_with_phone( "_phone_area_code": "", "_phone_country_code": "", } - ] + ], }, ) From 2ea5b2f58418730fe2cf30c0425691ab1e8237ba Mon Sep 17 00:00:00 2001 From: Corrin Lakeland Date: Sun, 2 Aug 2026 17:06:30 +1200 Subject: [PATCH 4/4] fix: fail on GDPRREQUEST status, make the guidance title a real heading Review follow-ups on PR #516: - GDPRREQUEST passed validation but fell through xero_archived as "not archived", so an erased contact would trip the un-archive transition and re-enable jobs. It should never occur on an NZ org; the status is now unhandled-and-loud instead of silently active, with a test pinning that an archived company stays blocked. - The report guidance title becomes an h2 so screen readers see the section like every other heading on the page. KAN-325 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf --- apps/workflow/api/xero/reprocess_xero.py | 9 ++++++--- apps/workflow/tests/test_sync_clients.py | 19 +++++++++++++++++++ .../data-quality/duplicate-identities.vue | 2 +- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/apps/workflow/api/xero/reprocess_xero.py b/apps/workflow/api/xero/reprocess_xero.py index f896689f8..966706aab 100644 --- a/apps/workflow/api/xero/reprocess_xero.py +++ b/apps/workflow/api/xero/reprocess_xero.py @@ -292,11 +292,14 @@ def set_company_fields(company: Company, new_from_xero: bool = False) -> None: # regardless: its jobs belong to the winner. # No fallback for a missing status: guessing ACTIVE would un-archive the # company and re-open it for jobs. Malformed data fails this company's - # sync (ADR 0015 — consumers stay strict). + # sync (ADR 0015 — consumers stay strict). GDPRREQUEST is deliberately + # not accepted: it should never occur on an NZ org, and treating it as + # active would re-enable jobs for an erased contact — if it ever appears, + # fail loudly and decide its handling then. contact_status = raw_json.get("_contact_status") - if contact_status not in ("ACTIVE", "ARCHIVED", "GDPRREQUEST"): + if contact_status not in ("ACTIVE", "ARCHIVED"): raise ValueError( - f"Company {company.id} raw_json has missing or unknown " + f"Company {company.id} raw_json has missing or unhandled " f"_contact_status {contact_status!r}" ) was_archived = bool(company.xero_archived) diff --git a/apps/workflow/tests/test_sync_clients.py b/apps/workflow/tests/test_sync_clients.py index 50f0585ee..73a98d288 100644 --- a/apps/workflow/tests/test_sync_clients.py +++ b/apps/workflow/tests/test_sync_clients.py @@ -195,6 +195,25 @@ def test_missing_status_fails_rather_than_unarchiving(self) -> None: self.assertTrue(company.xero_archived) self.assertFalse(company.allow_jobs) + def test_gdprrequest_status_fails_rather_than_reenabling_jobs(self) -> None: + """GDPRREQUEST is unhandled: treating an erased contact as active + would re-enable jobs for it. It must fail loudly instead.""" + company = Company.objects.create( + name="GDPR Erased Ltd", + xero_last_modified=timezone.now(), + allow_jobs=False, + xero_archived=True, + raw_json=_make_raw_json( + "contact-5", "GDPR Erased Ltd", status="GDPRREQUEST" + ), + ) + + with self.assertRaisesRegex(ValueError, "_contact_status"): + set_company_fields(company) + + self.assertTrue(company.xero_archived) + self.assertFalse(company.allow_jobs) + def test_manual_block_survives_steady_state_sync(self) -> None: """No transition, no touch: an admin's allow_jobs=False on an active company must not be flipped back by a routine sync.""" diff --git a/frontend/src/pages/reports/data-quality/duplicate-identities.vue b/frontend/src/pages/reports/data-quality/duplicate-identities.vue index 6965c0500..9db4b0a91 100644 --- a/frontend/src/pages/reports/data-quality/duplicate-identities.vue +++ b/frontend/src/pages/reports/data-quality/duplicate-identities.vue @@ -29,7 +29,7 @@ data-automation-id="duplicate-identities-process" class="rounded-lg border border-indigo-200 bg-indigo-50 p-4 text-sm text-indigo-900" > -

How to resolve a duplicate company

+

How to resolve a duplicate company

Merge the two contacts in Xero. DocketWorks follows on the next sync: jobs, invoices, and people move to the surviving company and the duplicate stays behind as an archived