docs+fix: record the Xero-first company merge model, align the stragglers (KAN-325) - #516
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
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCompany merges now use the shared merge service and retain duplicate rows as tombstones. Company discovery excludes merged records. Xero archive transitions synchronize ChangesCompany identity lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Xero
participant set_company_fields
participant Company
Xero->>set_company_fields: provide contact archive state
set_company_fields->>Company: update xero_archived
set_company_fields->>Company: disable or restore allow_jobs
Company-->>set_company_fields: persist synchronized state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/company/services/company_rest_service.py (1)
154-170: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFilter tombstones from every company-discovery query.
get_all_companies()still usesCompany.objects.all(), so its dropdown includes every merged tombstone._execute_company_search()filters onlyallow_jobs=True, notmerged_into__isnull=True.Apply
merged_into__isnull=Trueto both queries. Add tests that use a tombstone withallow_jobs=Trueto verify thatget_all_companies()andsearch_companies()still exclude it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/company/services/company_rest_service.py` around lines 154 - 170, Update get_all_companies() and _execute_company_search() to filter Company querysets with merged_into__isnull=True, ensuring merged tombstones are excluded even when allow_jobs=True. Add tests creating an eligible tombstone and verify both get_all_companies() and search_companies() omit it.
🧹 Nitpick comments (1)
apps/company/management/commands/merge_companies.py (1)
101-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiscard the unused tuple value.
job_countis not read in this loop. Ruff B007 reports this binding. Use_for the ignored value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/company/management/commands/merge_companies.py` at line 101, Update the loop over companies_with_job_counts in the merge-companies command to bind the unused job count tuple element to _ instead of job_count, while preserving the existing company iteration behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/company/management/commands/merge_companies.py`:
- Around line 41-45: Update the duplicate selection flow in the merge command to
detect any candidate with a non-null xero_contact_id before choosing a primary,
fail immediately with an instruction to merge the companies in Xero, and prevent
the DocketWorks merge from proceeding. Preserve the existing behavior for
candidate sets that are entirely local, and add a command test covering the
Xero-linked rejection path.
In `@apps/workflow/api/xero/reprocess_xero.py`:
- Around line 293-299: Remove the default value from the _contact_status lookup
in the reprocessing flow and require the status to be present and valid before
updating company.xero_archived or company.allow_jobs. Preserve both fields
unchanged when raw_json is missing or contains an invalid status, and add a
regression test covering an archived company with missing status.
In `@frontend/src/pages/reports/data-quality/duplicate-identities.vue`:
- Around line 34-36: Update the duplicate-identity guidance text to describe the
losing company as an archived, blocked tombstone that remains resolvable by ID
and retains its Xero identity and record data; remove the claim that it is an
empty archived record and preserve the surrounding sync behavior.
---
Outside diff comments:
In `@apps/company/services/company_rest_service.py`:
- Around line 154-170: Update get_all_companies() and _execute_company_search()
to filter Company querysets with merged_into__isnull=True, ensuring merged
tombstones are excluded even when allow_jobs=True. Add tests creating an
eligible tombstone and verify both get_all_companies() and search_companies()
omit it.
---
Nitpick comments:
In `@apps/company/management/commands/merge_companies.py`:
- Line 101: Update the loop over companies_with_job_counts in the
merge-companies command to bind the unused job count tuple element to _ instead
of job_count, while preserving the existing company iteration behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d38d873-b3de-440e-82c3-b2209a4851be
📒 Files selected for processing (11)
apps/company/management/commands/merge_companies.pyapps/company/migrations/0013_alter_company_allow_jobs.pyapps/company/models.pyapps/company/services/company_rest_service.pyapps/company/tests/test_company_fts_search.pyapps/company/tests/test_company_merge_service.pyapps/workflow/api/xero/reprocess_xero.pyapps/workflow/tests/test_sync_clients.pydocs/adr/0034-company-merges-are-xero-first.mddocs/adr/README.mdfrontend/src/pages/reports/data-quality/duplicate-identities.vue
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
There was a problem hiding this comment.
Pull request overview
This PR documents and standardizes the “Xero-first” company-merge process: merges happen in Xero, and DocketWorks mirrors them by tombstoning the losing Company (never deleting it) while moving all owned records onto the winner. It also aligns remaining code paths (manual merge command, archive/unarchive sync behavior, and company browse/search) with those semantics and adds regression tests.
Changes:
- Add ADR 0034 documenting Xero-first company identity/merge semantics and operator guidance on the Duplicate Identities report.
- Update Xero contact sync field-setting to be strict about
_contact_status, mirror archive state, and only toggleallow_jobson archive transitions (with merged tombstones staying blocked). - Update manual merge tooling and company browse/search to preserve tombstones and hide merged losers from listing/search results; add tests covering these behaviors.
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 operator guidance on resolving duplicate companies (merge in Xero). |
| docs/adr/README.md | Adds ADR 0034 to the ADR index. |
| docs/adr/0034-company-merges-are-xero-first.md | New ADR defining Xero-first merge process and tombstone semantics. |
| apps/workflow/tests/test_sync_clients.py | Adds tests for archive/unarchive allow_jobs transitions and strict _contact_status; updates phone-sync fixture raw_json. |
| apps/workflow/api/xero/reprocess_xero.py | Implements strict _contact_status handling and transition-based allow_jobs behavior during sync. |
| apps/company/tests/test_company_merge_service.py | Adds test ensuring merge_companies management command produces tombstones and is idempotent on re-run. |
| apps/company/tests/test_company_fts_search.py | Adds test asserting merged tombstones are excluded from browse/search. |
| apps/company/services/company_rest_service.py | Filters merged tombstones from list/search queries. |
| apps/company/models.py | Updates allow_jobs help text to reflect un-archive restoration semantics (except merged tombstones). |
| apps/company/migrations/0013_alter_company_allow_jobs.py | Applies the allow_jobs help text update via migration. |
| apps/company/management/commands/merge_companies.py | Switches command to use shared merge service and tombstone semantics; skips already-merged rows. |
| 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: |
There was a problem hiding this comment.
Fixed in 2ea5b2f, taking the reject-outright option: GDPRREQUEST is removed from the accepted statuses, so it now raises like any unhandled status instead of falling through as "not archived" and re-enabling jobs. It should never occur on an NZ org; if it ever does, the sync fails loudly for that company and we decide its handling then. Test pins that an archived company stays archived and blocked.
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
Why
The merged-company phone-sync incident (PR #514) exposed that DocketWorks has complete company-merge machinery but no recorded merge process, and three code paths quietly contradicted the model the machinery implements.
What
ADR 0034 (
docs/adr/0034-company-merges-are-xero-first.md): duplicate companies are merged in Xero; DocketWorks mirrors the merge into a permanent tombstone (merged_intoset, never deleted) andmerge_companies()moves everything the loser owned. People are linked, not merged (ADR 0030 boundary).allow_jobsfollows the archive transitions only.Operator guidance on the Duplicate Identities report — the one place duplicates surface now says what to do about them.
Three alignments:
merge_companiesmanagement command: was reassign-then-hard-delete — the opposite of the tombstone the Xero path leaves. Now calls the same service, skips resolved tombstones, re-runs cleanly.set_company_fields:xero_archivednow mirrors Xero both ways (old FIXME), andallow_jobsfollows the archive transitions — archive disables, un-archive restores. Steady-state syncs never touch it, so manual blocks (tax authorities, internal accounts per the field's help text) survive; merged tombstones never get jobs re-enabled.list_companies: merged tombstones no longer appear in browse or search (the same business showed twice); still reachable by id.Tests: command tombstone semantics + idempotent re-run; un-archive restore, steady-state no-touch, merged-tombstone-stays-blocked; tombstone exclusion from list/search. Full
apps.company+test_sync_clientssuites green (151 tests).KAN-325 tracks the remaining gap (merge actions on the report, ongoing person dedup).
🤖 Generated with Claude Code
https://claude.ai/code/session_01PnuHL7w3UisSk6KQ81q3Qf
Summary by CodeRabbit
New Features
Documentation