feat(conversations): create ticket#58392
Merged
Merged
Conversation
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
Contributor
|
Size Change: +4.67 kB (0%) Total Size: 117 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
products/conversations/backend/api/tickets.py:862-882
**Initial compose email sent with "Re:" subject prefix**
The `Comment` created here triggers the existing `send_email_reply_on_team_message` signal, which calls `send_email_reply`. That task unconditionally prepends `"Re: "` to the subject unless it already starts with `"re:"`. For a compose flow initiating first contact (e.g. subject `"Welcome!"`), the customer receives an email with subject `"Re: Welcome!"`. The `send_email_reply` task was designed for replies to inbound tickets, not for the first outbound message. You'll need either a dedicated compose-email task, or an `is_initial_message` flag to skip the "Re:" prefix.
### Issue 2 of 4
products/conversations/frontend/components/ComposeTicket/ComposeTicketModal.tsx:12-13
The `emailConfigsLoading` state provided by kea-loaders is not used. While configs are loading the "From" dropdown shows the placeholder but remains empty with no visual feedback, which may leave users confused if the load takes a moment.
```suggestion
const { isOpen, recipientEmail, emailSubject, emailConfigId, emailConfigs, emailConfigsLoading, composingLoading } =
useValues(composeTicketLogic)
```
### Issue 3 of 4
products/conversations/frontend/components/ComposeTicket/ComposeTicketModal.tsx:50-56
Pass `emailConfigsLoading` to `LemonSelect` so the dropdown shows a spinner while configs load, rather than appearing empty.
```suggestion
<LemonSelect
value={emailConfigId || undefined}
options={emailConfigOptions}
onChange={(value) => value && setEmailConfigId(value)}
placeholder={emailConfigsLoading ? 'Loading...' : 'Select sender address...'}
loading={emailConfigsLoading}
fullWidth
/>
```
### Issue 4 of 4
frontend/src/lib/api.ts:6594-6601
The `channel` field is declared in the request type but `ComposeTicketSerializer` on the backend doesn't include it, so it is silently ignored. Since the endpoint is email-only and hardcodes `Channel.EMAIL`, the field adds noise without effect.
```suggestion
async compose(data: {
message: string
recipient_email: string
email_config_id: string
email_subject?: string
rich_content?: Record<string, unknown> | null
}): Promise<{ id: string; ticket_number: number }> {
```
Reviews (1): Last reviewed commit: "feat: create ticket" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
MCP UI Apps size report
|
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/conversations/backend/api/tickets.py:233-235
The `compose` action is not added to `posthog_feature_flag`, so `PostHogFeatureFlagPermission` (which is in the class-level `permission_classes`) does not gate it. Any authenticated team member can call this endpoint even when `PRODUCT_SUPPORT_CREATE_TICKET` is disabled — bypassing the UI flag check entirely. The `suggest_reply` action shows the established pattern: list the action name in the dict alongside its flag key.
```suggestion
posthog_feature_flag = {
"product-support-ai-suggestion": ["suggest_reply"],
"product-support-create-ticket": ["compose"],
}
```
Reviews (2): Last reviewed commit: "chore: update OpenAPI generated types" | Re-trigger Greptile |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
Medium: Compose can link outbound tickets to arbitrary peopleThis PR adds a compose endpoint for creating outbound email tickets. The endpoint trusts a caller-supplied distinct ID when linking the ticket to a person, which lets a ticket writer attach the new ticket to an unrelated person and then read that person data through the ticket response. Status: 1 new · 1 open · 1 resolved |
…m/PostHog/posthog into feat/support-raise-a-ticker-email
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/conversations/backend/api/tickets.py:815-820
**400 error schema doesn't match actual responses**
The `@extend_schema` decorator annotates the 400 response with `SuggestReplyErrorSerializer` (which has an `error_type` field), but the `compose` action returns standard DRF `{"detail": "..."}` payloads. This will generate an incorrect OpenAPI spec for the endpoint. The `SuggestReplyErrorSerializer` was designed for the `suggest_reply` action where error types like `"no_messages"` are returned. A plain `serializers.Serializer` with a `detail` field (or `OpenApiResponse(description="Bad request")`) would be more accurate here.
### Issue 2 of 2
products/conversations/frontend/components/ComposeTicket/composeTicketLogic.ts:55-62
**Stale `composingLoading` when modal closed during in-flight request**
`closeComposeModal` does not reset `composingLoading`, so if a user clicks Cancel while the API call is still in-flight and then immediately reopens the modal, they'll see the "Send" button stuck in a loading state until the background request resolves. Adding `closeComposeModal` to the `composingLoading` reducer ensures the state is clean on reopen.
```suggestion
composingLoading: [
false,
{
submitCompose: () => true,
submitComposeFinished: () => false,
resetForm: () => false,
closeComposeModal: () => false,
},
],
```
Reviews (3): Last reviewed commit: "Merge branch 'feat/support-raise-a-ticke..." | Re-trigger Greptile |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/conversations/backend/api/tickets.py:428-441
**N+1 queries in `_attach_persons_to_tickets` fallback**
The new loop issues one `get_person_by_email_property` ORM query per unmatched email ticket. With `max_limit = 1000`, a full-page list request containing 1000 such tickets would fire 1000 extra queries (each filtering on a JSONB `properties` column) against the read replica. A single call to `get_person_by_email_property` is already a heavy operation since it must match against three JSON key variants.
The fix is to deduplicate by email address first, execute one query per unique email (or a single `__in`-based query), then distribute the results, similar to how `get_persons_by_distinct_ids` handles the batch above.
### Issue 2 of 2
products/conversations/backend/api/tickets.py:834-840
`permission_classes` on `@action` is ignored by `TeamAndOrgViewSetMixin`
`TeamAndOrgViewSetMixin.get_permissions()` builds the permission list entirely from the class-level `self.permission_classes` attribute — it never reads action-level `permission_classes` from `@action(...)` kwargs. The actual enforced permissions are the full set inherited from the viewset (`IsAuthenticated`, `APIScopePermission`, `AccessControlPermission`, `TeamMemberAccessPermission`, `PostHogFeatureFlagPermission`). Leaving this here is misleading: a reader might believe the compose endpoint is less protected than it actually is.
```suggestion
@action(
detail=False,
methods=["POST"],
pagination_class=None,
throttle_classes=[ComposeTicketBurstThrottle, ComposeTicketSustainedThrottle],
)
```
Reviews (4): Last reviewed commit: "fix" | Re-trigger Greptile |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/conversations/backend/api/tickets.py:436-441
The `if email:` check inside the loop is redundant — the list comprehension on the preceding lines already filters for `t.email_from` being truthy, so `email` is always a non-empty string at this point.
```suggestion
for ticket in unmatched:
found = get_person_by_email_property(self.team_id, ticket.email_from)
if found is not None:
ticket.person = found
```
### Issue 2 of 2
products/conversations/frontend/components/ComposeTicket/composeTicketLogic.ts:117-124
**Field-level API errors silently fall back to generic message**
DRF field validation errors (e.g. an invalid `recipient_email`) return a 400 body like `{"recipient_email": ["Enter a valid email address."]}` — no top-level `detail` key. The check `'detail' in error` will be `false`, so the user sees "Failed to create ticket." instead of the actual validation message. Consider showing the first field-level error message when `detail` is absent, or normalising the error shape from the API client.
Reviews (5): Last reviewed commit: "fix" | Re-trigger Greptile |
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/conversations/backend/api/tickets.py:871-881
The person-resolution block is duplicated verbatim. After the first block runs it may update `distinct_id` to the person's first distinct ID; the second block then calls `get_person_by_distinct_id` again with that already-resolved ID (an extra DB round-trip), and the `distinct_id == recipient_email` guard is now false so the email fallback can never fire anyway. The second block is entirely dead and should be removed.
```suggestion
person = get_person_by_distinct_id(team.id, distinct_id)
if person is None and distinct_id == recipient_email:
person = get_person_by_email_property(team.id, recipient_email)
if person is not None and person.distinct_ids:
distinct_id = person.distinct_ids[0]
```
Reviews (6): Last reviewed commit: "fix" | Re-trigger Greptile |
Contributor
|
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
products/conversations/backend/api/tickets.py:89-93
`email_subject` allows blank strings by default since DRF `CharField` has `allow_blank=True`. Sending `email_subject: ""` will pass validation, be stored as `""`, and then silently fall back to `"Your support request"` in `send_email_reply` (via the `or` fallback). Adding `allow_blank=False` enforces the intent: if the field is present, it must be non-empty.
```suggestion
email_subject = serializers.CharField(
required=False,
allow_blank=False,
max_length=500,
help_text="Email subject line.",
)
```
### Issue 2 of 3
products/conversations/backend/api/tickets.py:107-110
Superfluous check: `not value` is redundant here. Since `message` is a non-nullable `CharField`, `value` is always a `str`. `not value.strip()` already covers the empty-string case (`"".strip() == ""`), so the `not value` short-circuit adds no correctness benefit. Simplify to a single condition.
```suggestion
def validate_message(self, value: str) -> str:
if not value.strip():
raise serializers.ValidationError("Message content is required.")
return value.strip()
```
### Issue 3 of 3
products/conversations/backend/api/tickets.py:884
`email_from` semantically means "the address this email originated from" (the customer for inbound tickets). For compose/outbound tickets the customer is the *recipient*, yet `email_from` is populated with `data["recipient_email"]`. This works today because `send_email_reply` uses `ticket.email_from` as the `to=` address and the fallback person-lookup in `_attach_persons_to_tickets` also keys on it — so functionally it is fine. However, a future maintainer inspecting an outbound ticket will see `email_from = customer@example.com` and reasonably conclude "the customer sent this email", which is the opposite of what happened. A field like `email_to` or a dedicated `direction` column would prevent this confusion as the outbound feature grows.
Reviews (8): Last reviewed commit: "remove" | Re-trigger Greptile |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
There is an unresolved security/behavioral comment from @veria-ai on the current head: the compose action only gates on email_enabled but not conversations_enabled, allowing outbound emails to be sent even after Conversations is turned off for the team. This needs to be addressed before auto-approval. Additionally, the PR is cross-cutting, involves a new API action with permission changes, and warrants a human review.
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/conversations/backend/api/tickets.py:436-441
The `if email:` guard is redundant — the list comprehension above already filters for `t.email_from` being truthy. The variable `email` is unconditionally truthy at this point, so the inner `if` can never be False.
```suggestion
for ticket in unmatched:
found = get_person_by_email_property(self.team_id, ticket.email_from)
if found is not None:
ticket.person = found
```
### Issue 2 of 2
products/conversations/backend/api/tickets.py:877-881
**Lazy `distinct_ids` query after `get_person_by_email_property`**
`get_person_by_email_property` always goes through the ORM path (no personhog RPC, as noted in its docstring) and does a plain `.filter(...).first()` with no `prefetch_related` for `PersonDistinctId`. Accessing `person.distinct_ids[0]` therefore fires an extra lazy DB query. This is only one query per compose request so it's not critical, but adding `.prefetch_related("persondistinctid_set")` inside `get_person_by_email_property` would keep it consistent with how `get_persons_by_distinct_ids` works.
Reviews (9): Last reviewed commit: "fix" | Re-trigger Greptile |
There was a problem hiding this comment.
Gates denied due to size (631 lines, 16 files). Beyond the size gate, this PR adds a new outbound email action that introduces real concerns: an N+1 query per unmatched ticket in _attach_persons_to_tickets (one get_person_by_email_property call per email ticket with no person match, across up to 1000 tickets per page), and the recipient_distinct_id field still allows callers to link a ticket to an arbitrary person's distinct ID independently of the recipient email, leaking that person's properties through TicketSerializer. The stamphog review agent also failed after 3 attempts and flagged the need for human review. Request a human review from the conversations team owner before re-requesting auto-approval.
yasen-posthog
approved these changes
May 14, 2026
Contributor
yasen-posthog
left a comment
yasen-posthog
left a comment
There was a problem hiding this comment.
very good, awesome feature
inkeep Bot
added a commit
to PostHog/posthog.com
that referenced
this pull request
May 14, 2026
Documents the new compose ticket feature from PR PostHog/posthog#58392, which allows support teams to proactively create tickets and send initial emails to customers via the email channel.
inkeep Bot
added a commit
to PostHog/posthog.com
that referenced
this pull request
May 14, 2026
Reflects the compose ticket feature added in PostHog/posthog#58392.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Create ticket (email channel first)