Skip to content

feat: attach group properties in all channels#60870

Merged
veryayskiy merged 2 commits into
masterfrom
feat/support-group-events-other-channels
Jun 1, 2026
Merged

feat: attach group properties in all channels#60870
veryayskiy merged 2 commits into
masterfrom
feat/support-group-events-other-channels

Conversation

@veryayskiy
Copy link
Copy Markdown
Contributor

Problem

capture_ticket_created only resolves org $groups when ticket.distinct_id is a real PostHog distinct_id (web widget). Slack/Email/MS Teams set distinct_id to the customer email (or ""), so OrganizationMembership.objects.filter(user__distinct_id=ticket.distinct_id) never matches and no $groups get attached. GitHub has neither a real distinct_id nor an email, so it stays unenriched (out of scope).

Changes

Add an email fallback: when the distinct_id path yields no membership, look up the person by properties.email via ClickHouse (_get_persons_by_email), take that person's real distinct_ids, and resolve the org membership through them.

@assign-reviewers-posthog assign-reviewers-posthog Bot requested a review from a team June 1, 2026 11:26
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 1, 2026

Comments Outside Diff (1)

  1. products/conversations/backend/api/tests/test_events.py, line 287-301 (link)

    P1 Existing tests now silently invoke _get_persons_by_email unmocked

    self.ticket is created in setUp with anonymous_traits={"email": "test@example.com"}. After this PR, whenever the distinct_id path doesn't produce a membership (all cases here), _resolve_org_groups falls through to the email-fallback branch and calls _get_persons_by_email(team, ["test@example.com"]). That function is not mocked in test_capture_ticket_created_no_groups_when (or in test_capture_ticket_created_person_processing), so the tests either make a real ClickHouse query or silently swallow an exception in the outer try/except — both of which cause the assertions to pass for the wrong reason. Both tests should also patch products.conversations.backend.events._get_persons_by_email and assert on how it's called (or not called) in each parameterised case.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: products/conversations/backend/api/tests/test_events.py
    Line: 287-301
    
    Comment:
    **Existing tests now silently invoke `_get_persons_by_email` unmocked**
    
    `self.ticket` is created in `setUp` with `anonymous_traits={"email": "test@example.com"}`. After this PR, whenever the distinct_id path doesn't produce a membership (all cases here), `_resolve_org_groups` falls through to the email-fallback branch and calls `_get_persons_by_email(team, ["test@example.com"])`. That function is not mocked in `test_capture_ticket_created_no_groups_when` (or in `test_capture_ticket_created_person_processing`), so the tests either make a real ClickHouse query or silently swallow an exception in the outer `try/except` — both of which cause the assertions to pass for the wrong reason. Both tests should also patch `products.conversations.backend.events._get_persons_by_email` and assert on how it's called (or not called) in each parameterised case.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
products/conversations/backend/api/tests/test_events.py:287-301
**Existing tests now silently invoke `_get_persons_by_email` unmocked**

`self.ticket` is created in `setUp` with `anonymous_traits={"email": "test@example.com"}`. After this PR, whenever the distinct_id path doesn't produce a membership (all cases here), `_resolve_org_groups` falls through to the email-fallback branch and calls `_get_persons_by_email(team, ["test@example.com"])`. That function is not mocked in `test_capture_ticket_created_no_groups_when` (or in `test_capture_ticket_created_person_processing`), so the tests either make a real ClickHouse query or silently swallow an exception in the outer `try/except` — both of which cause the assertions to pass for the wrong reason. Both tests should also patch `products.conversations.backend.events._get_persons_by_email` and assert on how it's called (or not called) in each parameterised case.

### Issue 2 of 2
products/conversations/backend/events.py:37-62
**Email fallback fires even when an identified person is found in step 1**

When `ticket.distinct_id` resolves to an identified person but no matching `OrganizationMembership` exists, execution falls through to the email branch instead of returning early. For web-widget tickets that include `email` in their `anonymous_traits`, this means a second ClickHouse lookup is attempted, and the ticket might be attributed to an organisation that belongs to a different person who happens to share the same email. The original code stopped at `process_person = False` in this case. If this fall-through is intentional (to handle the case where a web-widget user's org can only be found via email), an explicit test for that path is missing; if it is not intentional, an early `return False, None` should be added after the identified-person check fails to find a membership.

Reviews (1): Last reviewed commit: "feat: attach group properties in all cha..." | Re-trigger Greptile

Comment thread products/conversations/backend/events.py Outdated
Comment thread products/conversations/backend/events.py
@veria-ai
Copy link
Copy Markdown

veria-ai Bot commented Jun 1, 2026

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

@veryayskiy veryayskiy added the stamphog Request AI review from stamphog label Jun 1, 2026
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security concerns flagged in review (group attribution spoofing via widget email, fallback firing for identified persons with no membership) are genuinely fixed in the current diff with the channel allowlist and early-return logic. Tests cover the attack scenarios explicitly. No showstoppers.

@veryayskiy veryayskiy merged commit 1aa6d34 into master Jun 1, 2026
284 of 286 checks passed
@veryayskiy veryayskiy deleted the feat/support-group-events-other-channels branch June 1, 2026 12:40
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented Jun 1, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-06-01 13:08 UTC Run
prod-us ✅ Deployed 2026-06-01 13:24 UTC Run
prod-eu ✅ Deployed 2026-06-01 13:26 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Request AI review from stamphog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant