Skip to content

fix: Notification email delivery state is now updated#4317

Merged
Rockyy174 merged 1 commit into
operately:mainfrom
Rockyy174:update-notification-state
Apr 2, 2026
Merged

fix: Notification email delivery state is now updated#4317
Rockyy174 merged 1 commit into
operately:mainfrom
Rockyy174:update-notification-state

Conversation

@Rockyy174
Copy link
Copy Markdown
Collaborator

No description provided.

Signed-off-by: Adriano Lazzaretti <lazzaretti136@gmail.com>
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • In EmailWorker, deliver_email/3 is just wrapping apply/3 to normalize the return; you could simplify by pattern-matching directly on the apply/3 result in perform/1 and avoid an extra private function.
  • In EmailDelivery.mark_sent/2 for lists, Repo.update_all/3’s return value is ignored and a static {:ok, true} is returned; consider returning or at least checking the affected row count to avoid silently succeeding when nothing was updated (e.g. due to stale IDs).
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `EmailWorker`, `deliver_email/3` is just wrapping `apply/3` to normalize the return; you could simplify by pattern-matching directly on the `apply/3` result in `perform/1` and avoid an extra private function.
- In `EmailDelivery.mark_sent/2` for lists, `Repo.update_all/3`’s return value is ignored and a static `{:ok, true}` is returned; consider returning or at least checking the affected row count to avoid silently succeeding when nothing was updated (e.g. due to stale IDs).

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@Rockyy174 Rockyy174 merged commit e561143 into operately:main Apr 2, 2026
3 checks passed
@Rockyy174 Rockyy174 deleted the update-notification-state branch April 2, 2026 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant