KAN-278: Complete first-class People management - #453
Conversation
|
Warning Review limit reached
Next review available in: 27 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 (8)
📝 WalkthroughWalkthroughThis PR introduces a person directory with company relationships, contact-method management, phone ownership classification, new REST endpoints, frontend pages, conflict-handling workflows, generated API schemas, tests, and supporting telemetry and routing updates. ChangesPerson directory and ownership
CRM telemetry and filtering maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/crm/calls.vue (1)
351-356: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an early return to prevent API calls with an empty company ID.
When the selected company is cleared,
companyIdbecomes an empty string. The watcher currently proceeds to callloadCompanyPeople(companyId), which may trigger an API error downstream since the backend expects a valid UUID.🐛 Proposed fix
watch(selectedCompanyId, (companyId) => { if (!companyId) { selectedPersonId.value = '' + return } void loadCompanyPeople(companyId) })🤖 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 `@frontend/src/pages/crm/calls.vue` around lines 351 - 356, Update the selectedCompanyId watcher to return immediately after clearing selectedPersonId when companyId is empty, and only call loadCompanyPeople for a valid company ID.
🧹 Nitpick comments (9)
docketworks/settings.py (1)
365-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse title case for enum labels.
The labels for
PhoneOwnershipStatusEnumare lowercase, whereas other enum labels in this file (e.g.,ContactMethodTypeEnum,JobStatusEnum) use title case. Consider capitalizing them for consistency in the generated schema documentation.🎨 Proposed fix
"PhoneOwnershipStatusEnum": ( - ("available", "available"), - ("people", "people"), - ("company", "company"), - ("internal", "internal"), + ("available", "Available"), + ("people", "People"), + ("company", "Company"), + ("internal", "Internal"), ),🤖 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 `@docketworks/settings.py` around lines 365 - 370, Update the labels in PhoneOwnershipStatusEnum to title case while preserving their lowercase enum values: use “Available,” “People,” “Company,” and “Internal” as the display labels for schema documentation consistency.frontend/src/pages/crm/people/__tests__/people-directory.test.ts (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename test files to match the component naming convention.
As per coding guidelines, test specs should have filenames matching
ComponentName.test.ts. Please consider renaming these test files to use PascalCase.
frontend/src/pages/crm/people/__tests__/people-directory.test.ts#L1-L2: Rename this file toPeopleDirectory.test.ts.frontend/src/pages/crm/people/__tests__/person-detail.test.ts#L1-L2: Rename this file toPersonDetailPage.test.ts.🤖 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 `@frontend/src/pages/crm/people/__tests__/people-directory.test.ts` around lines 1 - 2, Rename frontend/src/pages/crm/people/__tests__/people-directory.test.ts to PeopleDirectory.test.ts and frontend/src/pages/crm/people/__tests__/person-detail.test.ts to PersonDetailPage.test.ts, preserving their test contents and matching the PascalCase component naming convention.Source: Coding guidelines
frontend/scripts/capture-screenshots.ts (1)
210-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
waitFor: 'main'to ensure the page has fully loaded.Consider adding
waitFor: 'main'(or an equivalent selector) for thepeople-listscreenshot to prevent capturing a loading state before the data is fetched, ensuring consistency with thecompanies-listconfiguration.🛠️ Proposed fix
{ id: 'people-list', description: 'People directory with company relationships', route: '/crm/people', + waitFor: 'main', },🤖 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 `@frontend/scripts/capture-screenshots.ts` around lines 210 - 213, Update the people-list screenshot configuration to include waitFor: 'main', matching the companies-list configuration, so capture waits for the main page content before taking the screenshot.frontend/src/components/__tests__/PersonSelectionModal.phoneConflict.test.ts (1)
1-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFilename deviates from the
ComponentName.test.tsconvention.As per coding guidelines, unit test specs should be named
ComponentName.test.ts; this file adds a.phoneConflictscope suffix. Splitting a large suite by scenario is reasonable, but consider consolidating intoPersonSelectionModal.test.ts(or adescribeblock within it) if there's no other file already covering this component.🤖 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 `@frontend/src/components/__tests__/PersonSelectionModal.phoneConflict.test.ts` around lines 1 - 125, Rename the test file to follow the ComponentName.test.ts convention by using PersonSelectionModal.test.ts, and preserve the existing phone ownership scenarios under an appropriate describe block. Before renaming, consolidate with any existing PersonSelectionModal test suite if one already covers this component, avoiding duplicate test files.Source: Coding guidelines
apps/company/services/person_service.py (1)
104-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImplicit active/inactive scope depends on incidental prefetch state.
company_links()returns active-only links when the caller'sPrefetchfiltered onis_active=True, and all links otherwise. The two current callers happen to want different scopes, but nothing enforces that pairing explicitly — it's coupled to how each caller built theirPrefetch. A future prefetch change on either side would silently change this method's output shape with no visible signal.♻️ Make the scope explicit
class PersonDirectoryService: `@staticmethod` - def company_links(person: Person) -> list[PersonCompanyLinkData]: - prefetched = getattr(person, "_prefetched_objects_cache", {}).get( - "company_links" - ) - if prefetched is None: - links = list( - CompanyPersonLink.objects.filter(person=person).select_related( - "company" - ) - ) - else: - links = list(person.company_links.all()) + def company_links( + person: Person, *, include_inactive: bool = True + ) -> list[PersonCompanyLinkData]: + links = list( + CompanyPersonLink.objects.filter(person=person).select_related("company") + ) + if not include_inactive: + links = [link for link in links if link.is_active]🤖 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/person_service.py` around lines 104 - 134, Make the active/inactive link scope explicit in the company_links method instead of deriving it from the person’s prefetched company_links queryset. Add an explicit scope parameter with a clear default or required value, apply the corresponding is_active filter when querying links, and update each caller to pass its intended scope while preserving the existing ordering and response mapping.apps/company/tests/test_person_api.py (1)
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated auth/office-staff setup boilerplate.
Same
setUppattern duplicated inapps/company/tests/test_contact_methods.py.🤖 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/tests/test_person_api.py` around lines 10 - 14, Consolidate the duplicated authentication and office-staff setup from test_person_api.py and test_contact_methods.py into a shared test setup helper or base class. Update both test suites to reuse that shared setup while preserving the existing super().setUp(), staff update, and force_authenticate behavior.apps/company/tests/test_contact_methods.py (1)
418-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated auth/office-staff setup boilerplate.
This
setUp(toggleis_office_staff, save, force-authenticate) is duplicated verbatim inapps/company/tests/test_person_api.py. Candidate for a shared helper/mixin inBaseAPITestCase.🤖 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/tests/test_contact_methods.py` around lines 418 - 423, Consolidate the duplicated office-staff authentication setup from this test class and apps/company/tests/test_person_api.py into a shared helper or mixin on BaseAPITestCase. Update each affected setUp to reuse that helper while preserving the existing is_office_staff update and authentication behavior.frontend/src/pages/crm/people/[id].vue (2)
112-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSome interactive buttons lack
data-automation-id, unlike sibling save actions in this same file.The identity/method/link "save" buttons all have stable
data-automation-ids, but the contact-method Edit/Remove/Cancel buttons and the company-link Edit/"Company" buttons don't. Adding them keeps this page consistent for future E2E test authors targeting stable selectors instead of DOM position.Also applies to: 159-176, 201-203, 219-225
🤖 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 `@frontend/src/pages/crm/people/`[id].vue around lines 112 - 121, Add stable data-automation-id attributes to the contact-method Edit, Remove, and Cancel buttons and the company-link Edit and Company buttons in the relevant template sections, matching the naming convention used by the existing identity/method/link save buttons. Keep each identifier unique and tied to its action.
304-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent error surfacing across handlers in this file.
saveLink/removeLink/restoreLink/removeMethodall useextractErrorMessage(err), butsaveMethodfalls back toerr instanceof Error ? err.message : '...'andsaveIdentityshows a fixed string with no error detail at all. Standardizing onextractErrorMessagewould give users consistent, more informative error messages (e.g. surfaced validation detail) for every mutation on this page.♻️ Proposed fix
} catch { - toast.error('Failed to update identity') + toast.error(`Identity not updated: ${extractErrorMessage(undefined)}`) } finally {} catch (err) { - toast.error(err instanceof Error ? err.message : 'Failed to save contact method') + toast.error(`Contact method not saved: ${extractErrorMessage(err)}`) } finally {🤖 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 `@frontend/src/pages/crm/people/`[id].vue around lines 304 - 371, Standardize mutation error handling in saveIdentity and saveMethod by using extractErrorMessage(err) in their catch blocks, matching saveLink, removeLink, restoreLink, and removeMethod. Preserve each handler’s existing contextual toast message while replacing the fixed or Error-only fallback with the extracted error detail.
🤖 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/models.py`:
- Around line 390-393: Update the CompanyPersonLink query in the save flow to
use the save operation’s database alias via using(db), and filter by
company_id=self.company_id instead of dereferencing self.company. Keep the
existing primary, active, and self-exclusion conditions unchanged.
In `@apps/company/services/person_service.py`:
- Around line 259-268: Update the DjangoValidationError handler in the person
service flow to call persist_app_error(exc) before translating the exception.
Re-raise the resulting PersonPhoneConflictError using the project’s
AlreadyLoggedException two-arm dedup pattern, ensuring the caught exception is
persisted exactly once while preserving phone ownership classification.
- Around line 272-312: Update put_company_link and create_person_for_company to
lock the company-wide active CompanyPersonLink rows with select_for_update
before evaluating whether another active link exists and deriving is_primary.
Ensure the lock and primary decision occur inside the existing
transaction.atomic scope, preserving the current behavior for explicit primary
requests and first active links.
In `@apps/company/views/person_views.py`:
- Around line 259-272: Update the patch method’s rematch condition to schedule
phone-call rematching when either the original method type or
updated.method_type is PHONE. Preserve the existing old and new normalized
values in the rematch set, while ensuring non-phone-to-non-phone changes do not
schedule the task.
In `@frontend/src/components/PersonSelector.vue`:
- Around line 184-205: Reset phoneOwnership at the start of the fresh
person-save flow before invoking createNewPerson, including the path that
handles a newly available number. Ensure non-409 create failures reach the
generic error toast and do not retain the stale conflict banner, while
preserving the existing conflict handling behavior.
---
Outside diff comments:
In `@frontend/src/pages/crm/calls.vue`:
- Around line 351-356: Update the selectedCompanyId watcher to return
immediately after clearing selectedPersonId when companyId is empty, and only
call loadCompanyPeople for a valid company ID.
---
Nitpick comments:
In `@apps/company/services/person_service.py`:
- Around line 104-134: Make the active/inactive link scope explicit in the
company_links method instead of deriving it from the person’s prefetched
company_links queryset. Add an explicit scope parameter with a clear default or
required value, apply the corresponding is_active filter when querying links,
and update each caller to pass its intended scope while preserving the existing
ordering and response mapping.
In `@apps/company/tests/test_contact_methods.py`:
- Around line 418-423: Consolidate the duplicated office-staff authentication
setup from this test class and apps/company/tests/test_person_api.py into a
shared helper or mixin on BaseAPITestCase. Update each affected setUp to reuse
that helper while preserving the existing is_office_staff update and
authentication behavior.
In `@apps/company/tests/test_person_api.py`:
- Around line 10-14: Consolidate the duplicated authentication and office-staff
setup from test_person_api.py and test_contact_methods.py into a shared test
setup helper or base class. Update both test suites to reuse that shared setup
while preserving the existing super().setUp(), staff update, and
force_authenticate behavior.
In `@docketworks/settings.py`:
- Around line 365-370: Update the labels in PhoneOwnershipStatusEnum to title
case while preserving their lowercase enum values: use “Available,” “People,”
“Company,” and “Internal” as the display labels for schema documentation
consistency.
In `@frontend/scripts/capture-screenshots.ts`:
- Around line 210-213: Update the people-list screenshot configuration to
include waitFor: 'main', matching the companies-list configuration, so capture
waits for the main page content before taking the screenshot.
In
`@frontend/src/components/__tests__/PersonSelectionModal.phoneConflict.test.ts`:
- Around line 1-125: Rename the test file to follow the ComponentName.test.ts
convention by using PersonSelectionModal.test.ts, and preserve the existing
phone ownership scenarios under an appropriate describe block. Before renaming,
consolidate with any existing PersonSelectionModal test suite if one already
covers this component, avoiding duplicate test files.
In `@frontend/src/pages/crm/people/__tests__/people-directory.test.ts`:
- Around line 1-2: Rename
frontend/src/pages/crm/people/__tests__/people-directory.test.ts to
PeopleDirectory.test.ts and
frontend/src/pages/crm/people/__tests__/person-detail.test.ts to
PersonDetailPage.test.ts, preserving their test contents and matching the
PascalCase component naming convention.
In `@frontend/src/pages/crm/people/`[id].vue:
- Around line 112-121: Add stable data-automation-id attributes to the
contact-method Edit, Remove, and Cancel buttons and the company-link Edit and
Company buttons in the relevant template sections, matching the naming
convention used by the existing identity/method/link save buttons. Keep each
identifier unique and tied to its action.
- Around line 304-371: Standardize mutation error handling in saveIdentity and
saveMethod by using extractErrorMessage(err) in their catch blocks, matching
saveLink, removeLink, restoreLink, and removeMethod. Preserve each handler’s
existing contextual toast message while replacing the fixed or Error-only
fallback with the extracted error detail.
🪄 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: e20ac448-b0fa-4432-a82c-d12eb88ac1d9
⛔ Files ignored due to path filters (1)
frontend/src/api/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (53)
apps/company/__init__.pyapps/company/models.pyapps/company/person_serializers.pyapps/company/services/__init__.pyapps/company/services/person_service.pyapps/company/tests/test_contact_methods.pyapps/company/tests/test_person_api.pyapps/company/urls_people_rest.pyapps/company/urls_rest.pyapps/company/views/__init__.pyapps/company/views/contact_method_viewset.pyapps/company/views/contact_viewset.pyapps/company/views/person_views.pyapps/crm/services/phone_call_service.pyapps/crm/tests/test_phone_call_service.pyapps/crm/views/phone_call_views.pyapps/workflow/migrations/0009_rename_remaining_crm_telemetry_sources.pyapps/workflow/tests/test_search_telemetry_migration.pydocketworks/settings.pydocketworks/urls.pydocs/test_plans/client_contact_management_test_plan.mddocs/test_plans/company_people_management_test_plan.mddocs/urls/client.mddocs/urls/company.mdfrontend/docs/jobview-etag-guide.mdfrontend/router-auto-options.tsfrontend/schema.ymlfrontend/scripts/capture-screenshots.tsfrontend/src/assets/main.cssfrontend/src/components/AppNavbar.vuefrontend/src/components/PersonSelectionModal.vuefrontend/src/components/PersonSelector.vuefrontend/src/components/__tests__/CompanyLookup.test.tsfrontend/src/components/__tests__/PersonSelectionModal.phoneConflict.test.tsfrontend/src/components/crm/PhoneNumberManager.vuefrontend/src/components/job/JobSettingsTab.vuefrontend/src/composables/__tests__/usePersonManagement.test.tsfrontend/src/composables/useCompanyLookup.tsfrontend/src/composables/usePersonManagement.tsfrontend/src/pages/crm/calls.vuefrontend/src/pages/crm/companies/(index).vuefrontend/src/pages/crm/companies/[id].vuefrontend/src/pages/crm/people/(index).vuefrontend/src/pages/crm/people/[id].vuefrontend/src/pages/crm/people/__tests__/people-directory.test.tsfrontend/src/pages/crm/people/__tests__/person-detail.test.tsfrontend/src/pages/jobs/create.vuefrontend/src/pages/purchasing/po/[id].vuefrontend/src/stores/__tests__/companyStore.test.tsfrontend/src/stores/companyStore.tsfrontend/src/typed-router.d.tsfrontend/tests/crm/people.spec.tsmypy-baseline.txt
💤 Files with no reviewable changes (7)
- docs/test_plans/client_contact_management_test_plan.md
- docs/urls/client.md
- apps/company/views/contact_viewset.py
- frontend/src/assets/main.css
- apps/crm/views/phone_call_views.py
- apps/crm/tests/test_phone_call_service.py
- mypy-baseline.txt
| if raw_phone: | ||
| from apps.company.serializers import set_primary_phone | ||
|
|
||
| try: | ||
| set_primary_phone(person, raw_phone) | ||
| except DjangoValidationError as exc: | ||
| ownership = classify_phone_ownership( | ||
| company=company, raw_phone=raw_phone | ||
| ) | ||
| raise PersonPhoneConflictError(ownership) from exc |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Persist the exception before re-raising, per project convention.
DjangoValidationError is caught and translated to PersonPhoneConflictError without going through persist_app_error first. As per coding guidelines: "For Python exception handling, persist every exception once with persist_app_error(exc) and re-raise through the AlreadyLoggedException two-arm dedup pattern."
🛠️ Proposed fix
try:
set_primary_phone(person, raw_phone)
except DjangoValidationError as exc:
ownership = classify_phone_ownership(
company=company, raw_phone=raw_phone
)
- raise PersonPhoneConflictError(ownership) from exc
+ conflict_error = PersonPhoneConflictError(ownership)
+ persist_app_error(conflict_error)
+ raise conflict_error from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if raw_phone: | |
| from apps.company.serializers import set_primary_phone | |
| try: | |
| set_primary_phone(person, raw_phone) | |
| except DjangoValidationError as exc: | |
| ownership = classify_phone_ownership( | |
| company=company, raw_phone=raw_phone | |
| ) | |
| raise PersonPhoneConflictError(ownership) from exc | |
| if raw_phone: | |
| from apps.company.serializers import set_primary_phone | |
| try: | |
| set_primary_phone(person, raw_phone) | |
| except DjangoValidationError as exc: | |
| ownership = classify_phone_ownership( | |
| company=company, raw_phone=raw_phone | |
| ) | |
| conflict_error = PersonPhoneConflictError(ownership) | |
| persist_app_error(conflict_error) | |
| raise conflict_error from exc |
🤖 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/person_service.py` around lines 259 - 268, Update the
DjangoValidationError handler in the person service flow to call
persist_app_error(exc) before translating the exception. Re-raise the resulting
PersonPhoneConflictError using the project’s AlreadyLoggedException two-arm
dedup pattern, ensuring the caught exception is persisted exactly once while
preserving phone ownership classification.
Source: Coding guidelines
| def put_company_link( | ||
| *, person: Person, company: Company, data: CompanyLinkData | ||
| ) -> CompanyPersonLink: | ||
| with transaction.atomic(): | ||
| existing = ( | ||
| CompanyPersonLink.objects.select_for_update() | ||
| .filter(person=person, company=company) | ||
| .first() | ||
| ) | ||
| other_active_exists = ( | ||
| CompanyPersonLink.objects.filter(company=company, is_active=True) | ||
| .exclude(person=person) | ||
| .exists() | ||
| ) | ||
| is_primary = data["is_primary"] or not other_active_exists | ||
| if existing is None: | ||
| link = CompanyPersonLink.objects.create( | ||
| person=person, | ||
| company=company, | ||
| position=data["position"], | ||
| notes=data["notes"], | ||
| is_primary=is_primary, | ||
| is_active=True, | ||
| ) | ||
| else: | ||
| existing.position = data["position"] | ||
| existing.notes = data["notes"] | ||
| existing.is_primary = is_primary | ||
| existing.is_active = True | ||
| existing.save( | ||
| update_fields=[ | ||
| "position", | ||
| "notes", | ||
| "is_primary", | ||
| "is_active", | ||
| "updated_at", | ||
| ] | ||
| ) | ||
| link = existing | ||
| _schedule_person_phone_rematch(person) | ||
| return link |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== apps/company/models.py: relevant definitions ==\n'
ast-grep outline apps/company/models.py --view expanded || true
printf '\n== search for is_primary / uniqueness constraints ==\n'
rg -n "is_primary|UniqueConstraint|unique_together|Constraint|company.*primary|primary.*company" apps/company/models.py apps/company/services/person_service.py apps/company -g '!**/__pycache__/**' || true
printf '\n== file sizes ==\n'
wc -l apps/company/models.py apps/company/services/person_service.py
printf '\n== relevant slices ==\n'
sed -n '1,260p' apps/company/models.py
printf '\n--- person_service slice ---\n'
sed -n '220,340p' apps/company/services/person_service.pyRepository: corrin/docketworks
Length of output: 46864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
models = Path("apps/company/models.py").read_text()
service = Path("apps/company/services/person_service.py").read_text()
# Quick heuristic extraction of constraints mentioning CompanyPersonLink
for label, text in [("models", models), ("service", service)]:
print(f"\n== {label} matches ==")
for m in re.finditer(r".{0,80}(is_primary|UniqueConstraint|unique_together|Constraint|company).{0,120}", text):
line = text.count("\n", 0, m.start()) + 1
print(f"{line}: {m.group(0).replace(chr(10), ' ')}")
PYRepository: corrin/docketworks
Length of output: 11493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect model class around CompanyPersonLink"
rg -n -A80 -B20 "class CompanyPersonLink|class Company" apps/company/models.pyRepository: corrin/docketworks
Length of output: 8722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path("apps/company/models.py").read_text()
patterns = [
r"UniqueConstraint\([^\)]*is_primary",
r"unique_together",
r"company.*is_primary",
r"is_primary.*company",
]
for pat in patterns:
print(f"PATTERN {pat}")
for m in re.finditer(pat, text, re.S):
line = text.count("\n", 0, m.start()) + 1
print(" line", line, ":", m.group(0)[:300].replace("\n", " "))
PYRepository: corrin/docketworks
Length of output: 1202
Serialize primary-link assignment. There isn’t a DB-level uniqueness constraint on (company, is_primary) here; the invariant is enforced in CompanyPersonLink.save(). The remaining gap is that create_person_for_company() and put_company_link() decide is_primary before locking the company-wide active link set, so two concurrent requests can both persist is_primary=True and leave duplicate primaries. Lock the company’s active links before deriving is_primary.
🤖 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/person_service.py` around lines 272 - 312, Update
put_company_link and create_person_for_company to lock the company-wide active
CompanyPersonLink rows with select_for_update before evaluating whether another
active link exists and deriving is_primary. Ensure the lock and primary decision
occur inside the existing transaction.atomic scope, preserving the current
behavior for explicit primary requests and first active links.
| toast.success('Person created successfully!', { | ||
| dismissible: true, | ||
| position: 'top-left', | ||
| }) | ||
| } else { | ||
| toast.error( | ||
| `Failed to ${isEditing.value ? 'update' : 'create'} person. Please check the form and try again.`, | ||
| ) | ||
| } else if (!phoneOwnership.value) { | ||
| toast.error('Failed to create person. Please check the form and try again.') | ||
| } | ||
| } | ||
|
|
||
| const handleEditPerson = (person: CompanyPersonLink) => { | ||
| debugLog('PersonSelector - handleEditPerson:', person) | ||
| startEditPerson(person) | ||
| const handleLinkPerson = async (person: PhonePersonMatch) => { | ||
| toast.info('Linking existing person...', { id: 'save-person' }) | ||
| const success = await linkExistingPerson(person) | ||
| toast.dismiss('save-person') | ||
| if (success) toast.success('Existing person linked successfully') | ||
| } | ||
|
|
||
| const handleDeletePerson = async (personLinkId: string) => { | ||
| debugLog('PersonSelector - handleDeletePerson:', personLinkId) | ||
| toast.info('Deleting person...', { id: 'delete-person' }) | ||
|
|
||
| const success = await deletePerson(personLinkId) | ||
|
|
||
| toast.dismiss('delete-person') | ||
|
|
||
| if (success) { | ||
| toast.success('Person removed successfully') | ||
| } else { | ||
| toast.error('Failed to remove person. Please try again.') | ||
| } | ||
| const handleCreateSeparatePerson = async () => { | ||
| toast.info('Creating separate person...', { id: 'save-person' }) | ||
| const success = await createNewPerson(true) | ||
| toast.dismiss('save-person') | ||
| if (success) toast.success('Person created successfully') | ||
| else toast.error('Failed to create separate person') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'phoneOwnership' frontend/src/composables/usePersonManagement.tsRepository: corrin/docketworks
Length of output: 486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '45,135p' frontend/src/composables/usePersonManagement.ts
printf '\n----\n'
sed -n '200,285p' frontend/src/composables/usePersonManagement.ts
printf '\n----\n'
sed -n '175,215p' frontend/src/components/PersonSelector.vueRepository: corrin/docketworks
Length of output: 6709
🏁 Script executed:
...Repository: corrin/docketworks
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba frontend/src/composables/usePersonManagement.ts | sed -n '50,130p'
printf '\n----\n'
nl -ba frontend/src/composables/usePersonManagement.ts | sed -n '205,280p'
printf '\n----\n'
nl -ba frontend/src/components/PersonSelector.vue | sed -n '175,215p'Repository: corrin/docketworks
Length of output: 196
Clear phoneOwnership before proceeding with a fresh save. phoneOwnership only resets on phone edits or modal close, so if a prior conflict is still in state when the number later becomes available, a subsequent non-409 create failure will skip the generic error toast and leave the stale conflict banner visible.
🤖 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 `@frontend/src/components/PersonSelector.vue` around lines 184 - 205, Reset
phoneOwnership at the start of the fresh person-save flow before invoking
createNewPerson, including the path that handles a newly available number.
Ensure non-409 create failures reach the generic error toast and do not retain
the stale conflict banner, while preserving the existing conflict handling
behavior.
1 participant
📝 Description
Completes the remaining first-class People management work for KAN-278. DocketWorks now treats companies, people, employment links, and contact methods as distinct records throughout the API and UI, while preventing new duplicate identities at data entry.
🔗 Related Jira Work Item
Jira: KAN-278
🚀 Changes
✅ Checklist
Vue.js (Composition API)
usePersonManagementQuality & Formatting
Definition of Done
Validation
manage.py checkpasses andmakemigrations --check --dry-runreports no changes.Production-data rehearsal (no production mutation)
The exact deploy sequence was rehearsed on a separate disposable clone of the designated real-production-data hotfix database, with Xero tokens cleared and
XERO_READONLY=True. The source copy and production were not modified; the rehearsal database was dropped afterward.UAT
Deploy this branch to the normal hotfix/UAT environment restored from production, with
XERO_READONLY=Trueon the backend and every worker/beat process. Verify/crm/people, Person contact methods and Company links, Company Detail People, the job Person selector, same-company shared office numbers, and cross-company existing-Person linking.Then run the full Playwright suite in the foreground and allow global teardown to finish. It was not started locally because the configured backend/frontend/ngrok environment is offline and repository safety instructions explicitly prohibit an agent from starting those services.