fix: move tickets email search to ch#58692
Conversation
|
🎭 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. |
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:104-110
**Email key used for mapping may not match the queried email**
`matched = prop_email or prop_Email or prop_dollar_email` picks the first non-empty value, but the WHERE clause can fire on any of the three columns. If a person has `properties.email = "personal@gmail.com"` and `properties.Email = "work@corp.com"`, and a ticket has `email_from = "work@corp.com"`, the row is returned by the `properties.Email` branch—but `matched` resolves to `"personal@gmail.com"`, so `email_to_uuid["personal@gmail.com"]` is stored. The final `email_to_person.get("work@corp.com")` call then returns `None`, silently dropping the match. The original Postgres query didn't have this issue because it returned the matching `Person` object directly via an OR filter. A fix would be to register the UUID under every non-empty email property, not just the first one, then intersect with the requested `emails` set.
Reviews (1): Last reviewed commit: "fix: move tickets email search to ch" | Re-trigger Greptile |
PR overviewMoves ticket email person lookup to HogQLThis PR replaces the old per-email ORM fallback with a batched HogQL lookup for email-channel ticket person attachment and outbound compose. I checked the ticket API auth/team scoping, the HogQL placeholder usage, and the follow-up UUID fetch; the lookup stays parameterized and scoped to the current team. Security review
Risk: 2/10 |
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:82-95
The emails passed to the ClickHouse `IN` clause are the raw, un-lowercased `email_from` values, but ClickHouse string comparisons via `IN` are case-sensitive. If `email_from` contains any upper-case characters (e.g. `"User@Example.com"` from an RFC-2047 `From:` header) while the person's stored property is lowercase, the `WHERE` clause will not match—even though the Python-side `emails_set` and result-dict keys are already normalised to lowercase. Passing lowercase emails to the query and using `lower()` in the WHERE clause makes the whole lookup consistently case-insensitive.
```suggestion
emails_lower = [e.lower() for e in emails]
with tags_context(product=Product.CONVERSATIONS, feature=Feature.QUERY):
response = execute_hogql_query(
"""
SELECT
id,
properties.email,
properties.Email,
properties.$email
FROM persons
WHERE lower(toString(properties.email)) IN {emails}
OR lower(toString(properties.Email)) IN {emails}
OR lower(toString(properties.$email)) IN {emails}
""",
placeholders={"emails": ast.Constant(value=emails_lower)},
```
Reviews (2): Last reviewed commit: "fix" | Re-trigger Greptile |
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:104
`emails_set` is re-derived from `emails` using the same lowercasing that already produced `emails_lower`, violating the OnceAndOnlyOnce simplicity rule. Use `set(emails_lower)` to avoid the redundant comprehension.
```suggestion
emails_set = set(emails_lower)
```
### Issue 2 of 2
products/conversations/backend/api/tickets.py:488-495
The `if t.email_from` guard inside the loop and the list comprehension are redundant — `unmatched` is already filtered to only include tickets where `t.email_from` is truthy (line 486). Removing the guards reduces superfluous parts.
```suggestion
if unmatched:
emails = [t.email_from for t in unmatched]
email_to_person = _get_persons_by_email(self.team, emails)
for ticket in unmatched:
found = email_to_person.get(ticket.email_from.lower())
if found is not None:
ticket.person = found
```
Reviews (3): Last reviewed commit: "fix" | Re-trigger Greptile |
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:104
`emails_set` recomputes the lower-case operation over `emails` even though `emails_lower` already holds exactly that data. Use `set(emails_lower)` to avoid computing `.lower()` twice on the same input — consistent with the OnceAndOnlyOnce principle.
```suggestion
emails_set = set(emails_lower)
```
### Issue 2 of 2
products/conversations/backend/api/tickets.py:489
The `if t.email_from` guard here is redundant — `unmatched` is already filtered to only include tickets where `t.email_from` is truthy. The extra check is a superfluous part that adds noise without protecting against anything new.
```suggestion
emails = [t.email_from for t in unmatched]
```
Reviews (4): Last reviewed commit: "fix" | Re-trigger Greptile |
There was a problem hiding this comment.
No showstoppers. The change replaces a per-row ORM-based person-email lookup (which bypassed personhog routing with a nosemgrep comment) with a batched ClickHouse HogQL query, enabling the previously commented-out email fallback. All resolved review concerns (case sensitivity, batching) are addressed in the diff, and comprehensive tests are included.
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
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:484
Redundant guard in `emails` list comprehension — `unmatched` already filters for `t.email_from` being truthy (line 481), so the inner `if t.email_from` is superfluous. The list could also benefit from deduplication to avoid issuing CH-side lookups for repeated addresses.
```suggestion
emails = list({t.email_from for t in unmatched})
```
Reviews (5): Last reviewed commit: "email only" | Re-trigger Greptile |
There was a problem hiding this comment.
No showstoppers. The change replaces a nosemgrep-bypassing ORM person lookup with a batched ClickHouse HogQL query. The key reviewer concern (robbie-c flagging the 3-variant email property query as missing skip indexes) is addressed in the current diff — only properties.email is queried. Case sensitivity is handled with lower(), and get_persons_by_uuids properly routes through personhog.
For raising tickets from the app we need a lookup for a person email when distinct_id is not sent.
Removing the postgres search and using only ch.