diff --git a/apps/company/management/commands/merge_companies.py b/apps/company/management/commands/merge_companies.py index 467d77d83..28016ca7f 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, _ 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..966706aab 100644 --- a/apps/workflow/api/xero/reprocess_xero.py +++ b/apps/workflow/api/xero/reprocess_xero.py @@ -285,20 +285,31 @@ 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 - contact_status = raw_json.get("_contact_status", "ACTIVE") - if contact_status == "ARCHIVED": - company.xero_archived = True + # 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. + # 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). 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"): + raise ValueError( + f"Company {company.id} raw_json has missing or unhandled " + f"_contact_status {contact_status!r}" + ) + 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..73a98d288 100644 --- a/apps/workflow/tests/test_sync_clients.py +++ b/apps/workflow/tests/test_sync_clients.py @@ -150,6 +150,109 @@ 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_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_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.""" + 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. @@ -306,6 +409,7 @@ def _client_with_phone( name=name, xero_last_modified=timezone.now(), raw_json={ + "_contact_status": "ACTIVE", "_phones": [ { "_phone_type": phone_type, @@ -313,7 +417,7 @@ def _client_with_phone( "_phone_area_code": "", "_phone_country_code": "", } - ] + ], }, ) 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..5f317d185 --- /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. 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/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..9db4b0a91 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 archived + record. +

+
+