feat: add email notifications for issue events (#202) - #278
Merged
nazarli-shabnam merged 4 commits intoJul 8, 2026
Conversation
Adds email notifications for issue events by extending the existing notification pipeline. ### Changes - Added notification email subject/body templates - Added EmailNotificationLog model/store - Enqueued notification emails after in-app notifications - Reused existing RabbitMQ email infrastructure - Logged all notification emails to email_notification_logs - Added unit tests for notification email rendering
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds plaintext email notifications for five notification types, persists email audit logs, enqueues email send jobs through existing RabbitMQ wiring, conditionally enables the flow from router setup, and documents the implementation and verification steps. ChangesEmail Notification Pipeline
Estimated code review effort: 3 (Moderate) | ~30 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
apps/api/internal/service/notification.go (1)
345-347: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid recomputing
actorNameandprojectIdentinenqueueNotificationEmails.These values are already computed in
emit()at lines 290–292. Recomputing them here makes two redundant DB calls (us.GetByIDandps.GetByID) peremitinvocation. Pass them as parameters instead.♻️ Proposed refactor: pass computed values as params
- if s.queue != nil && s.emailLog != nil && s.appURL != "" { - s.enqueueNotificationEmails(ctx, allowed, params) - } + if s.queue != nil && s.emailLog != nil && s.appURL != "" { + s.enqueueNotificationEmails(ctx, allowed, params, actorName, issueRef) + }func (s *NotificationService) enqueueNotificationEmails(ctx context.Context, receivers []uuid.UUID, params emitParams, actorName, issueRef string) { if params.issue == nil || len(receivers) == 0 { return } - actorName := s.actorDisplayName(ctx, params.actorID) - projectIdent := s.projectIdentifier(ctx, params.issue.ProjectID) - issueRef := fmt.Sprintf("%s-%d", projectIdent, params.issue.SequenceID) issueURL := fmt.Sprintf("%s/issue/%s", strings.TrimSuffix(s.appURL, "/"), params.issue.ID)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/service/notification.go` around lines 345 - 347, Avoid recomputing actorName and projectIdent inside enqueueNotificationEmails; they are already resolved in emit(). Update emit and enqueueNotificationEmails to pass the precomputed values through as parameters, and use those parameters when building issueRef so the helper no longer triggers redundant us.GetByID and ps.GetByID calls.apps/api/internal/mail/notification_test.go (1)
117-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for "subscribed" without
FieldName.The existing
TestBuildNotificationEmail_Subscribedalways setsFieldName. A test with emptyFieldNamewould catch the double-space formatting bug in the body builder.♻️ Suggested additional test
func TestBuildNotificationEmail_SubscribedWithoutFieldName(t *testing.T) { data := NotificationEmailData{ ReceiverName: "Oscar", ActorName: "Patty", IssueRef: "TASK-555", IssueTitle: "Refactor module", IssueURL: "https://app.devlane.io/issue/vwx-555", WorkspaceName: "Platform", OldValue: "Low", NewValue: "High", } subject, body := BuildNotificationEmail("subscribed", data) expectedSubject := "Patty updated TASK-555" if subject != expectedSubject { t.Errorf("subject mismatch: got %q, want %q", subject, expectedSubject) } if !strings.Contains(body, "Patty updated TASK-555") { t.Error("body should contain update message without field name") } if strings.Contains(body, "updated on") { t.Error("body should not contain double space when FieldName is empty") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/mail/notification_test.go` around lines 117 - 146, Add a test in TestBuildNotificationEmail coverage for the "subscribed" case when NotificationEmailData.FieldName is empty. Create a new test alongside TestBuildNotificationEmail_Subscribed that calls BuildNotificationEmail with FieldName unset, then assert the subject/body use the no-field wording and do not contain the double-space formatting bug. Use the existing BuildNotificationEmail and NotificationEmailData symbols so the test targets the body builder behavior directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/internal/mail/notification.go`:
- Around line 77-84: The subscribed notification body always injects
data.FieldName, which causes awkward double spacing when it is empty. Update the
"subscribed" branch in notification.go to build the message conditionally the
same way the subject logic does: include the field name only when it is
non-empty, while preserving the existing ActorName, IssueRef, IssueTitle, and
value change formatting in the body.
In `@apps/api/internal/router/router.go`:
- Line 165: Use the computed appBaseURL fallback instead of cfg.AppBaseURL when
wiring NotificationService. The router already derives appBaseURL with a
fallback to cfg.CORSAllowOrigin, so update the SetAppBaseURL call in router
initialization to pass that value, matching the auth handler behavior and
preventing s.appURL from being empty inside NotificationService.
In `@apps/api/internal/service/notification.go`:
- Line 420: The warning log in notification handling currently uses
receiver.Email, which can log a pointer value and expose PII; update the email
queue failure log in the notification flow to use receiverID consistently
instead. Locate the logging call in the same block that already references
emailLog.ID and s.logger().Warn, and replace the receiver field with receiverID
so the log matches the earlier usage in this function and avoids logging email
addresses.
In `@IMPLEMENTATION_SUMMARY.md`:
- Around line 31-48: The two fenced code blocks in the implementation summary
are missing language tags, which trips markdownlint’s fenced-code-language rule.
Update both markdown fences in IMPLEMENTATION_SUMMARY.md to include a language
identifier such as text so the diagram renders and lints consistently; keep the
content unchanged and make sure the same fix is applied to the other referenced
block as well.
- Around line 161-165: The verification step references a log string that
doesn’t match the implemented email flow, so update the checks in
IMPLEMENTATION_SUMMARY to use the actual symbols and messages from the email
pipeline. Point the API log check at the real publish/queue logging from the
send_email flow (for example the queue publish send_email message or the warning
strings) or at the email_notification_logs row, and keep the database and
RabbitMQ verification aligned with those actual outputs.
- Around line 70-80: The Go command examples are written as if the reader is
already inside apps/api, so they fail from the repository root. Update the
commands in IMPLEMENTATION_SUMMARY to use repo-root-relative paths or explicitly
change into the API directory first, so the go test and go build examples remain
runnable from the root. Refer to the existing unit test and build command
snippets in the summary and adjust them consistently.
---
Nitpick comments:
In `@apps/api/internal/mail/notification_test.go`:
- Around line 117-146: Add a test in TestBuildNotificationEmail coverage for the
"subscribed" case when NotificationEmailData.FieldName is empty. Create a new
test alongside TestBuildNotificationEmail_Subscribed that calls
BuildNotificationEmail with FieldName unset, then assert the subject/body use
the no-field wording and do not contain the double-space formatting bug. Use the
existing BuildNotificationEmail and NotificationEmailData symbols so the test
targets the body builder behavior directly.
In `@apps/api/internal/service/notification.go`:
- Around line 345-347: Avoid recomputing actorName and projectIdent inside
enqueueNotificationEmails; they are already resolved in emit(). Update emit and
enqueueNotificationEmails to pass the precomputed values through as parameters,
and use those parameters when building issueRef so the helper no longer triggers
redundant us.GetByID and ps.GetByID calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8277441e-714a-4b45-b907-51ae502614de
📒 Files selected for processing (7)
IMPLEMENTATION_SUMMARY.mdapps/api/internal/mail/notification.goapps/api/internal/mail/notification_test.goapps/api/internal/model/email_notification_log.goapps/api/internal/router/router.goapps/api/internal/service/notification.goapps/api/internal/store/email_notification_log.go
- Use appBaseURL fallback in router (fixes email disable when APP_BASE_URL unset) - Handle empty FieldName in subscribed notification body - Log receiverID instead of email pointer (PII + correctness) - Pass precomputed actorName and issueRef to avoid redundant DB calls
nazarli-shabnam
requested changes
Jul 8, 2026
nazarli-shabnam
left a comment
nazarli-shabnam
left a comment
Member
There was a problem hiding this comment.
remove newly created IMPLEMENTATION_SUMMARY.md file. Thats not needed in the codebase.
Member
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
nazarli-shabnam
approved these changes
Jul 8, 2026
nazarli-shabnam
left a comment
nazarli-shabnam
left a comment
Member
There was a problem hiding this comment.
Thanks for the contribution.
10 tasks
27 tasks
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
3 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.
Adds email notifications for issue events by extending the existing notification pipeline.
Changes
email_notification_logsSupported events
Verification
sent_atupdated after processingSummary by CodeRabbit