fix: send immediate emails for encoded task mentions#4599
Conversation
Signed-off-by: Luca Mancini <lmancini@operately.com>
Signed-off-by: Luca Mancini <lmancini@operately.com>
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider reusing
Operately.Activities.Notifications.MentionedPeople.ids/1inOperatelyEmail.Emails.TaskAddingEmail.mentioned?/2instead of callingOperately.RichContent.find_mentioned_ids/2directly, to keep mention detection logic consistent across notifications and emails. - The
content_value/2helper silently returnsnilfor non-map content, which then results inTask.get/2being called with anilid; it may be safer to either pattern-match on a map innotify/3or explicitly handle thenilcase before callingTask.get/2to avoid surprising failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider reusing `Operately.Activities.Notifications.MentionedPeople.ids/1` in `OperatelyEmail.Emails.TaskAddingEmail.mentioned?/2` instead of calling `Operately.RichContent.find_mentioned_ids/2` directly, to keep mention detection logic consistent across notifications and emails.
- The `content_value/2` helper silently returns `nil` for non-map content, which then results in `Task.get/2` being called with a `nil` id; it may be safer to either pattern-match on a map in `notify/3` or explicitly handle the `nil` case before calling `Task.get/2` to avoid surprising failures.
## Individual Comments
### Comment 1
<location path="app/lib/operately_email/emails/task_adding_email.ex" line_range="41-46" />
<code_context>
+ end
+
+ defp mentioned?(person, activity) do
+ mentioned_ids =
+ activity.content
+ |> content_value(:description)
+ |> Operately.RichContent.find_mentioned_ids(:decode_ids)
+
+ person.id in mentioned_ids
+ rescue
+ _ -> false
</code_context>
<issue_to_address>
**suggestion:** Reuse `MentionedPeople.ids/1` for mentioned-id extraction to keep logic consistent with the notification classifier.
This function calls `Operately.RichContent.find_mentioned_ids(:decode_ids)` directly, while `Operately.Notifications.DirectMentionClassifier` now goes through `MentionedPeople.ids/1`. Using two separate code paths for mention ID extraction makes it easier for their behavior to drift over time. Please update this to call `MentionedPeople.ids/1` so all mention detection shares the same abstraction and future fixes.
Suggested implementation:
```
defp mentioned?(person, activity) do
mentioned_ids =
activity.content
|> content_value(:description)
|> MentionedPeople.ids()
person.id in mentioned_ids
rescue
_ -> false
end
```
To complete this change, add an alias for the `MentionedPeople` module at the top of `app/lib/operately_email/emails/task_adding_email.ex`, alongside the other aliases, for example:
```elixir
alias Operately.Notifications.MentionedPeople
```
If this module lives under a different namespace, adjust the alias path accordingly. Ensure there are no name clashes with other `MentionedPeople` modules in this file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| mentioned_ids = | ||
| activity.content | ||
| |> content_value(:description) | ||
| |> Operately.RichContent.find_mentioned_ids(:decode_ids) | ||
|
|
||
| person.id in mentioned_ids |
There was a problem hiding this comment.
suggestion: Reuse MentionedPeople.ids/1 for mentioned-id extraction to keep logic consistent with the notification classifier.
This function calls Operately.RichContent.find_mentioned_ids(:decode_ids) directly, while Operately.Notifications.DirectMentionClassifier now goes through MentionedPeople.ids/1. Using two separate code paths for mention ID extraction makes it easier for their behavior to drift over time. Please update this to call MentionedPeople.ids/1 so all mention detection shares the same abstraction and future fixes.
Suggested implementation:
defp mentioned?(person, activity) do
mentioned_ids =
activity.content
|> content_value(:description)
|> MentionedPeople.ids()
person.id in mentioned_ids
rescue
_ -> false
end
To complete this change, add an alias for the MentionedPeople module at the top of app/lib/operately_email/emails/task_adding_email.ex, alongside the other aliases, for example:
alias Operately.Notifications.MentionedPeopleIf this module lives under a different namespace, adjust the alias path accordingly. Ensure there are no name clashes with other MentionedPeople modules in this file.
related to #1219