bugfixes - #517
Conversation
…lers 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
docs+fix: record the Xero-first company merge model, align the stragglers (KAN-325)
|
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:
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 |
There was a problem hiding this comment.
Pull request overview
This PR formalizes “Xero-first” company identity merges (ADR 0034) and aligns both backend behavior and UI guidance with the tombstone-based merge model, including stricter handling of Xero archive/unarchive status and excluding merged tombstones from browse/search results.
Changes:
- Adds ADR 0034 describing Xero-driven merges and “tombstone” semantics for merged companies.
- Updates Xero company-field processing and adds tests to ensure archive/unarchive transitions are strict and safe.
- Excludes merged tombstones from company listing/search and updates the duplicate-identities report UI with operator guidance.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/reports/data-quality/duplicate-identities.vue | Adds on-page operational instructions for resolving duplicate companies via Xero merge. |
| docs/adr/README.md | Adds ADR 0034 entry to the ADR index. |
| docs/adr/0034-company-merges-are-xero-first.md | New ADR documenting Xero-first merge authority and tombstone behavior. |
| apps/workflow/tests/test_sync_clients.py | Adds regression tests for archive/unarchive and strict status handling. |
| apps/workflow/api/xero/reprocess_xero.py | Makes _contact_status strict and implements archive/unarchive transition logic for allow_jobs. |
| apps/company/tests/test_company_merge_service.py | Adds coverage ensuring the legacy management command produces tombstones, not deletions. |
| apps/company/tests/test_company_fts_search.py | Ensures merged tombstones are excluded from browse/search. |
| apps/company/services/company_rest_service.py | Filters merged tombstones out of list/search querysets. |
| apps/company/models.py | Updates allow_jobs help text to reflect un-archive restoration behavior. |
| apps/company/migrations/0013_alter_company_allow_jobs.py | Migration updating allow_jobs field help text. |
| apps/company/management/commands/merge_companies.py | Switches the dedupe command to use the tombstone-preserving merge service and skips already-merged rows. |
| 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: |
There was a problem hiding this comment.
Verified — the chain is real: the toggle is deliberately overridable (UI code comment says so) and a flipped tombstone would pass create_job's allow_jobs check. Accepted as a known low-severity edge for this release: it requires an admin to override a control explicitly labelled as blocked-because-merged, and the override possibility predates this release — nothing in this diff changes it. The proper fix is a server-side guard rejecting allow_jobs=true when merged_into is set; recorded on KAN-325 with the other tombstone-hygiene items (picker filters).
| # 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}") |
There was a problem hiding this comment.
Not taking this one: each merge_companies() call is internally atomic, so a failure can never leave a half-merged pair — only some pairs completed and the rest untouched. The command is idempotent on re-run (resolved tombstones are excluded from the duplicate query), so partial progress + re-run beats an outer transaction that would roll back valid completed merges because an unrelated pair failed. The command is also slated for replacement (KAN-325).
📝 Description
Short explanation of what you’ve built and why.
🔗 Related Jira Work Item
Jira: KAN-123
Use the uppercase Jira key (
KAN-123) in the PR title or body so GitHub forJira links the development event. Jira Automation owns the transition to Done
when the PR is merged.
🚀 Changes
useChatcomposable, splitChatHistoryandChatInputcomponents, added Pinia store).✅ Checklist
Vue.js (Composition API)
<script setup>& composables), no heavy logic in templatesuseChat, etc.)propsandemit, no direct parent/child ref duplicationQuality & Formatting
npx prettier --check .)npx eslint . --ext .js,.ts,.vue)Definition of Done
anyor loose passthrough types reviewedSee docs/jira-usage.md#required-checks.