[LFXV2-1730] feat: send notification emails when project members are added#65
Open
andrest50 wants to merge 15 commits into
Open
[LFXV2-1730] feat: send notification emails when project members are added#65andrest50 wants to merge 15 commits into
andrest50 wants to merge 15 commits into
Conversation
Add pkg/events/project.go with public UserInfo, ProjectSettings, and ProjectSettingsUpdatedMessage types so downstream services can import canonical struct definitions without redefining them. Domain types in internal/domain/models/ remain internal. Explicit converters in internal/service/converters.go (DomainSettingsToEvent, domainUsersToEvent, domainUserPtrToEvent) map domain → wire type before publishing. Removes internal message.go now superseded by the pkg/events types. Updates message_test.go to reference events.ProjectSettingsUpdatedMessage and documents the pkg/events convention in CLAUDE.md. LFXV2-1730 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Add TestDomainSettingsToEvent to converters_test.go covering nil input, full field mapping, nil optional pointers, and nil slice → empty slice behavior. Also fix CLAUDE.md Key Implementation Details section numbering (2a → 3, renumber 3/4 → 4/5). LFXV2-1730 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Using make() unconditionally turned nil domain slices into empty slices, changing JSON serialization from null to [] and breaking wire format compatibility with existing consumers. Add a nil guard so nil input returns nil (preserving the original null serialization). Update tests to document this behavior. LFXV2-1730 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
…added Subscribe to the existing lfx.projects-api.project_settings.updated NATS event and diff old vs new settings to detect newly added writers, auditors, and meeting coordinators. For each addition, render an HTML + plain-text email and fan it out in parallel (errgroup, limit 5) via NATS request/reply to the email service. The HTTP PUT /settings call is unaffected — emails are fully async and best-effort. - pkg/events/project.go: add Actor struct and field to ProjectSettingsUpdatedMessage - internal/service/notifications_diff.go: member diff by username or email - internal/service/notifications_render.go: embedded HTML + plain-text templates - internal/service/notifications_handler.go: HandleProjectSettingsUpdated handler - internal/service/project_operations.go: stamp Actor from request context - internal/service/project_service.go: add LFXSelfServeBaseURL to ServiceConfig - internal/domain/message.go: add SendEmailRequest to MessageBuilder interface - internal/infrastructure/nats/message.go: implement SendEmailRequest via NATS - internal/infrastructure/nats/mock.go: add RequestMsgWithContext to INatsConn - internal/domain/mock.go: add SendEmailRequest mock method - cmd/project-api/main.go: wire event subscription loop, add LFX_SELF_SERVE_BASE_URL env - go.mod: add lfx-v2-email-service dependency with local replace directive Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
![github-license-compliance[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces email notification functionality triggered by project settings changes. It adds NATS request/reply patterns for email sending, defines canonical event types, implements a project settings change handler that detects new role assignments and sends templated notifications, relocates the logging package, and updates configuration and documentation accordingly. ChangesEmail Service Integration and NATS Request/Reply Pattern
Project Settings Event Processing and Role Notification Workflow
Infrastructure Reorganization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds asynchronous, best-effort notification emails when project settings updates add new members to key roles (writers, auditors, meeting coordinators). It does so by subscribing to the existing lfx.projects-api.project_settings.updated NATS event, diffing old vs new settings, rendering email templates, and fanning out send requests to lfx-v2-email-service.
Changes:
- Introduces a NATS event handler that diffs settings updates and sends role notification emails (HTML + text) via NATS request/reply to the email service.
- Moves project-settings-updated event wire types into
pkg/events/and adds domain→event conversion helpers. - Wires runtime configuration for LFX Self-Serve base URL and adds the NATS subscription for the new handler.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/events/project.go | Adds canonical wire types for project events, including Actor on settings-updated messages. |
| internal/service/templates/project_role_notification.txt | Plain-text email template for role notifications. |
| internal/service/templates/project_role_notification.html | HTML email template for role notifications. |
| internal/service/project_service.go | Extends service config with LFXSelfServeBaseURL. |
| internal/service/project_operations.go | Publishes settings-updated event using pkg/events types and stamps actor username. |
| internal/service/notifications_render.go | Embeds and renders email templates (subject/HTML/text). |
| internal/service/notifications_render_test.go | Adds tests for template rendering output. |
| internal/service/notifications_handler.go | Implements NATS event handler to diff members and send emails via MessageBuilder. |
| internal/service/notifications_handler_test.go | Adds tests covering additions/no-op/send errors/unmarshal behavior. |
| internal/service/notifications_diff.go | Implements diffing logic for newly added role members. |
| internal/service/notifications_diff_test.go | Adds tests for diffing behavior and identity matching rules. |
| internal/service/converters.go | Adds DomainSettingsToEvent and user conversion helpers for event payloads. |
| internal/service/converters_test.go | Adds tests verifying domain→event conversion semantics. |
| internal/infrastructure/nats/mock.go | Extends NATS conn interface + mock with RequestMsgWithContext. |
| internal/infrastructure/nats/message.go | Adds SendEmailRequest implementation using NATS request/reply. |
| internal/infrastructure/nats/message_test.go | Updates event message tests to use pkg/events types. |
| internal/domain/models/message.go | Removes old internal event payload struct (moved to pkg/events). |
| internal/domain/mock.go | Adds mock method for SendEmailRequest. |
| internal/domain/message.go | Extends MessageBuilder interface with SendEmailRequest. |
| cmd/project-api/main.go | Adds env var parsing for base URL and subscribes to settings-updated events. |
| go.mod | Adds email-service dependency and updates Go version; includes a local replace directive. |
| go.sum | Updates dependency checksums accordingly. |
| CLAUDE.md | Documents the pkg/events/ wire-type convention and domain→event converter rule. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add 5s timeout (emailSendTimeout) on each SendEmailRequest call so a slow or unresponsive email service cannot hold errgroup slots indefinitely - Remove redundant eh := eh loop-variable shadow (fixed in Go 1.22+) - Remove dead GetProjectBase mock setup in the "no additions" test case; handler exits before calling GetProjectBase when no new members are found Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/templates/project_role_notification.txt (1)
1-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required license header in a non-rendered template comment.
CI is currently failing license-header checks for this file. Add the header using Go template comments so it won’t appear in outgoing emails.
Proposed fix
+{{/* Copyright The Linux Foundation and each contributor to LFX. */}} +{{/* SPDX-License-Identifier: MIT */}} + Hi {{.RecipientName}}, {{.InviterName}} has added you as a {{.Role}} on the {{.ProjectName}} project in LFX Self-Serve.🤖 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 `@internal/service/templates/project_role_notification.txt` around lines 1 - 11, Add the required license header to the top of the template using a Go template comment so it is not rendered in outgoing emails; wrap the full license text inside {{/* ... */}} and place it before any template content (above the existing usages like {{.RecipientName}}, {{.InviterName}}, {{.Role}}, {{.ProjectName}}, {{.ProjectURL}}) to satisfy the CI license-header checks without altering the visible email.
🧹 Nitpick comments (5)
internal/service/notifications_diff_test.go (1)
20-84: ⚡ Quick winAdd a duplicate-entry test case for
newmembers.Please add a table case where the same user appears twice in
new.Writersand assert only oneroleAssignmentis returned. This locks in non-dup notification behavior.🤖 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 `@internal/service/notifications_diff_test.go` around lines 20 - 84, Add a new table-test entry in the tests slice that checks deduplication of new members: create a case (e.g. name "duplicate new writers deduped") where new is events.ProjectSettings{Writers: []events.UserInfo{alice, alice}} and set wantLen: 1 and wantContains: []roleAssignment{{User: alice, Role: "Writer"}}; this ensures the code that produces roleAssignment (referenced by roleAssignment type and the logic comparing old vs new ProjectSettings/Writers) returns only one assignment for duplicate entries.internal/service/notifications_render_test.go (1)
14-43: ⚡ Quick winConvert this into a table-driven test with multiple scenarios.
Current coverage is only one happy path. A table-driven structure (e.g., normal values, empty inviter/recipient, special chars in names/role) would satisfy the test guideline and improve robustness.
Suggested shape
-func TestRenderProjectRoleNotification(t *testing.T) { - data := projectRoleNotificationData{ ... } - subject, html, text, err := renderProjectRoleNotification(data) - ... -} +func TestRenderProjectRoleNotification(t *testing.T) { + tests := []struct { + name string + data projectRoleNotificationData + }{ + { name: "happy path", data: projectRoleNotificationData{ ... } }, + { name: "empty inviter", data: projectRoleNotificationData{ ... } }, + { name: "special chars", data: projectRoleNotificationData{ ... } }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + subject, html, text, err := renderProjectRoleNotification(tt.data) + require.NoError(t, err) + // assertions... + }) + } +}As per coding guidelines
**/*_test.go: Use table-driven tests for comprehensive test coverage with multiple test cases.🤖 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 `@internal/service/notifications_render_test.go` around lines 14 - 43, The test TestRenderProjectRoleNotification currently checks only one happy path; refactor it into a table-driven test that iterates multiple cases (e.g., normal values, empty InviterName/RecipientName, names/roles with special characters) by defining a slice of structs containing a name, projectRoleNotificationData input, and expected snippets, then loop over cases and call renderProjectRoleNotification for each, asserting subject/html/text contain or don't contain the expected strings; update assertions to use t.Run(caseName, func(t *testing.T) { ... }) and keep checks for HTML vs plain text, using the existing renderProjectRoleNotification and projectRoleNotificationData symbols.internal/domain/message.go (1)
9-30: ⚡ Quick winIntroduce a domain-owned DTO for email requests to maintain interface isolation.
The
MessageBuilderinterface exposesemailapi.SendEmailRequestdirectly, coupling the domain layer to an external service type. The codebase already follows this pattern elsewhere—external types likeindexerTypesremain isolated in model implementations, not in domain interfaces. Create a domain-scoped request struct and map it toemailapi.SendEmailRequestin the infrastructure layer.🤖 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 `@internal/domain/message.go` around lines 9 - 30, The MessageBuilder interface currently leaks the external type emailapi.SendEmailRequest via the SendEmailRequest method; introduce a domain-scoped DTO (e.g., type EmailRequest struct { ... }) in the domain package and change MessageBuilder.SendEmailRequest(ctx context.Context, req emailapi.SendEmailRequest) error to use the new domain EmailRequest type instead. Update all implementations of MessageBuilder (the concrete infra message builder) to map/translate the domain EmailRequest into emailapi.SendEmailRequest before calling the external email API, and adjust imports/usages accordingly so only the infra layer depends on emailapi while the domain stays isolated.internal/infrastructure/nats/message.go (2)
210-217: ⚡ Quick winConsider adding a timeout to the email service request.
The
RequestMsgWithContextcall respects the context deadline, but if the caller passes a context without a timeout, the request could block indefinitely. Consider wrapping the context with a timeout to bound the request duration.⏱️ Proposed fix to add request timeout
+const emailRequestTimeout = 10 * time.Second + // SendEmailRequest sends a request to the email service and waits for a reply. func (m *MessageBuilder) SendEmailRequest(ctx context.Context, req emailapi.SendEmailRequest) error { data, err := json.Marshal(req) if err != nil { slog.ErrorContext(ctx, "error marshalling email request into JSON", constants.ErrKey, err) return err } + reqCtx, cancel := context.WithTimeout(ctx, emailRequestTimeout) + defer cancel() + - reply, err := m.NatsConn.RequestMsgWithContext(ctx, &nats.Msg{ + reply, err := m.NatsConn.RequestMsgWithContext(reqCtx, &nats.Msg{ Subject: emailapi.SendEmailSubject, Data: data, })🤖 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 `@internal/infrastructure/nats/message.go` around lines 210 - 217, The RequestMsgWithContext call in message.go can block if the incoming ctx has no deadline; wrap the incoming ctx with a bounded timeout (e.g., via context.WithTimeout) before calling m.NatsConn.RequestMsgWithContext in the function that sends the email (the code around reply, err := m.NatsConn.RequestMsgWithContext(ctx, &nats.Msg{...})), pass the derived ctx into RequestMsgWithContext, defer the cancel() immediately to avoid leaks, and make the timeout duration a clear constant or configurable value (e.g., emailRequestTimeout) so tests/config can adjust it; keep the existing error logging and return behavior unchanged.
219-224: 💤 Low valueConsider logging unmarshal failures for error responses.
If the email service returns malformed JSON in
reply.Data, the unmarshal error is silently discarded and the request is treated as successful. While this aligns with the best-effort design, logging unmarshal failures would aid debugging.📝 Optional improvement to log unmarshal failures
if len(reply.Data) > 0 { var errResp emailapi.SendEmailErrorResponse - if jsonErr := json.Unmarshal(reply.Data, &errResp); jsonErr == nil && errResp.Error != "" { - return fmt.Errorf("email service error: %s", errResp.Error) + if jsonErr := json.Unmarshal(reply.Data, &errResp); jsonErr != nil { + slog.WarnContext(ctx, "failed to unmarshal email service response", constants.ErrKey, jsonErr) + } else if errResp.Error != "" { + return fmt.Errorf("email service error: %s", errResp.Error) } }🤖 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 `@internal/infrastructure/nats/message.go` around lines 219 - 224, When handling reply.Data in the email response path, log json.Unmarshal failures so malformed JSON from the email service isn't silently ignored: inside the existing block that declares var errResp emailapi.SendEmailErrorResponse and calls json.Unmarshal(reply.Data, &errResp), detect when jsonErr != nil and emit a debug/warn log (including jsonErr and the raw reply.Data) via your logger rather than discarding the error, while preserving the current behavior that only returns an error when errResp.Error is set.
🤖 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 `@go.mod`:
- Line 94: The go.mod contains a machine-local replace for module
github.com/linuxfoundation/lfx-v2-email-service pointing to an absolute path;
remove this local replace before merge and restore a resolvable dependency:
either replace it with a published/tagged version or a pseudo-version (e.g.,
github.com/linuxfoundation/lfx-v2-email-service vX.Y.Z or
v0.0.0-yyyymmddhhmmss-commit), or move the local replace into go.work for
developer-only overrides; update any references to the module in imports if
needed and run `go mod tidy` to verify the module graph.
In `@internal/service/notifications_diff.go`:
- Around line 35-42: The loop that builds additions can append duplicates when
`new` contains repeated members; add a local seen map (e.g., map[string]bool)
keyed by `memberKey(u)` inside the same function (the diff logic around
memberKey/new/oldSet) and skip appending if seen[key] is true, setting
seen[key]=true when you first handle the user; this ensures `additions` (of type
roleAssignment) only gets one entry per unique member even if `new` contains
duplicates.
In `@internal/service/templates/project_role_notification.html`:
- Line 1: This new HTML template (project_role_notification.html) is missing the
required Linux Foundation copyright/license header; add the standard multi-line
license header at the very top of the file before the <!DOCTYPE html>
declaration so CI recognizes it (ensure the header text matches the project's
canonical Linux Foundation copyright/license block and preserves any required
year and copyright holder placeholders).
---
Outside diff comments:
In `@internal/service/templates/project_role_notification.txt`:
- Around line 1-11: Add the required license header to the top of the template
using a Go template comment so it is not rendered in outgoing emails; wrap the
full license text inside {{/* ... */}} and place it before any template content
(above the existing usages like {{.RecipientName}}, {{.InviterName}}, {{.Role}},
{{.ProjectName}}, {{.ProjectURL}}) to satisfy the CI license-header checks
without altering the visible email.
---
Nitpick comments:
In `@internal/domain/message.go`:
- Around line 9-30: The MessageBuilder interface currently leaks the external
type emailapi.SendEmailRequest via the SendEmailRequest method; introduce a
domain-scoped DTO (e.g., type EmailRequest struct { ... }) in the domain package
and change MessageBuilder.SendEmailRequest(ctx context.Context, req
emailapi.SendEmailRequest) error to use the new domain EmailRequest type
instead. Update all implementations of MessageBuilder (the concrete infra
message builder) to map/translate the domain EmailRequest into
emailapi.SendEmailRequest before calling the external email API, and adjust
imports/usages accordingly so only the infra layer depends on emailapi while the
domain stays isolated.
In `@internal/infrastructure/nats/message.go`:
- Around line 210-217: The RequestMsgWithContext call in message.go can block if
the incoming ctx has no deadline; wrap the incoming ctx with a bounded timeout
(e.g., via context.WithTimeout) before calling m.NatsConn.RequestMsgWithContext
in the function that sends the email (the code around reply, err :=
m.NatsConn.RequestMsgWithContext(ctx, &nats.Msg{...})), pass the derived ctx
into RequestMsgWithContext, defer the cancel() immediately to avoid leaks, and
make the timeout duration a clear constant or configurable value (e.g.,
emailRequestTimeout) so tests/config can adjust it; keep the existing error
logging and return behavior unchanged.
- Around line 219-224: When handling reply.Data in the email response path, log
json.Unmarshal failures so malformed JSON from the email service isn't silently
ignored: inside the existing block that declares var errResp
emailapi.SendEmailErrorResponse and calls json.Unmarshal(reply.Data, &errResp),
detect when jsonErr != nil and emit a debug/warn log (including jsonErr and the
raw reply.Data) via your logger rather than discarding the error, while
preserving the current behavior that only returns an error when errResp.Error is
set.
In `@internal/service/notifications_diff_test.go`:
- Around line 20-84: Add a new table-test entry in the tests slice that checks
deduplication of new members: create a case (e.g. name "duplicate new writers
deduped") where new is events.ProjectSettings{Writers: []events.UserInfo{alice,
alice}} and set wantLen: 1 and wantContains: []roleAssignment{{User: alice,
Role: "Writer"}}; this ensures the code that produces roleAssignment (referenced
by roleAssignment type and the logic comparing old vs new
ProjectSettings/Writers) returns only one assignment for duplicate entries.
In `@internal/service/notifications_render_test.go`:
- Around line 14-43: The test TestRenderProjectRoleNotification currently checks
only one happy path; refactor it into a table-driven test that iterates multiple
cases (e.g., normal values, empty InviterName/RecipientName, names/roles with
special characters) by defining a slice of structs containing a name,
projectRoleNotificationData input, and expected snippets, then loop over cases
and call renderProjectRoleNotification for each, asserting subject/html/text
contain or don't contain the expected strings; update assertions to use
t.Run(caseName, func(t *testing.T) { ... }) and keep checks for HTML vs plain
text, using the existing renderProjectRoleNotification and
projectRoleNotificationData symbols.
🪄 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
Run ID: 1dd9951e-1dd4-40a7-a4cf-f3c116c135e7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
CLAUDE.mdcmd/project-api/main.gogo.modinternal/domain/message.gointernal/domain/mock.gointernal/domain/models/message.gointernal/infrastructure/nats/message.gointernal/infrastructure/nats/message_test.gointernal/infrastructure/nats/mock.gointernal/service/converters.gointernal/service/converters_test.gointernal/service/notifications_diff.gointernal/service/notifications_diff_test.gointernal/service/notifications_handler.gointernal/service/notifications_handler_test.gointernal/service/notifications_render.gointernal/service/notifications_render_test.gointernal/service/project_operations.gointernal/service/project_service.gointernal/service/templates/project_role_notification.htmlinternal/service/templates/project_role_notification.txtpkg/events/project.go
💤 Files with no reviewable changes (1)
- internal/domain/models/message.go
…ed pseudo-version The local filesystem replace directive for lfx-v2-email-service breaks CI since the path only exists on the developer's machine. Switch to a go module pseudo-version pinned to the HEAD commit of the LFXV2-1768 branch so the module proxy can fetch it in any environment. v0.0.0-20260512204955-693efd22ee37 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
go.mod (1)
14-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve license-compliance alert for the new email-service dependency before merge.
github.com/linuxfoundation/lfx-v2-email-serviceis still associated with GitHub license-compliance alerts in this PR history, so this is a merge blocker until the dependency is policy-allowed or metadata is fixed upstream.#!/bin/bash set -euo pipefail # Verify whether the module is still in the dependency graph and lockfile. rg -n "github.com/linuxfoundation/lfx-v2-email-service" go.mod go.sum || true # If available in your environment, check current Dependabot/license alerts for the repo: # gh api /repos/linuxfoundation/lfx-v2-project-service/dependabot/alerts \ # -f state=open -f per_page=100 | jq '.[] | {number, security_advisory: .security_advisory.summary, dependency: .dependency.package.name}'Based on learnings: Ensure all GitHub Actions CI/CD workflows (mega-linter, project-api-build, license-header-check) pass before merging PRs.
🤖 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 `@go.mod` at line 14, The go.mod entry for github.com/linuxfoundation/lfx-v2-email-service is triggering a license-compliance alert; either remove or replace that module with a policy-approved version or update its upstream license metadata, then update go.sum and vendor as needed. Locate the go.mod entry for github.com/linuxfoundation/lfx-v2-email-service, change the dependency to an allowed version (or remove it if unused), run `go mod tidy` and regenerate go.sum, verify the module is no longer present via a search for "github.com/linuxfoundation/lfx-v2-email-service", and re-run CI checks (mega-linter, project-api-build, license-header-check) to confirm the alert is resolved before merging.
🤖 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.
Duplicate comments:
In `@go.mod`:
- Line 14: The go.mod entry for github.com/linuxfoundation/lfx-v2-email-service
is triggering a license-compliance alert; either remove or replace that module
with a policy-approved version or update its upstream license metadata, then
update go.sum and vendor as needed. Locate the go.mod entry for
github.com/linuxfoundation/lfx-v2-email-service, change the dependency to an
allowed version (or remove it if unused), run `go mod tidy` and regenerate
go.sum, verify the module is no longer present via a search for
"github.com/linuxfoundation/lfx-v2-email-service", and re-run CI checks
(mega-linter, project-api-build, license-header-check) to confirm the alert is
resolved before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60396a9b-e226-4949-9645-5db3e8ee575d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/project-api/main.gogo.modinternal/service/notifications_handler.gointernal/service/notifications_handler_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/project-api/main.go
- internal/service/notifications_handler.go
- internal/service/notifications_handler_test.go
…-commit hook
- Add HTML comment license header to project_role_notification.html
- Add Go template comment license header to project_role_notification.txt
({{- /* ... */ -}} renders to nothing so it does not appear in sent emails)
- Expand Makefile license-check target to cover tracked .html and .txt files
via git ls-files (excludes gitignored files like megalinter-reports/)
- Add make license-check to pre-commit hook alongside fmt, vet, and lint
Generated with [Claude Code](https://claude.ai/code)
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
CI/lint fixes:
- Downgrade go directive to 1.24.0 (CI golangci-lint runs Go 1.24.7 with
GOTOOLCHAIN=local; requiring go 1.25 prevented package graph loading)
- Update lfx-v2-email-service pseudo-version to bfe7955 (also on go 1.24)
- Exclude templates/ from license-header-check (Go template comment syntax
{{- /* ... */ -}} is not recognized by the external license checker; the
copyright is present but the checker needs plain or HTML comment format)
PR review comments addressed:
- diffRole: add seenNew map to prevent duplicate emails when new settings
contains repeated member entries
- Skip email send when recipient has no email address; log warning instead
- Normalize LFXSelfServeBaseURL trailing slash before building project URL
- Rename test case "malformed message" → "project load failure" (accurate)
- Add TestMessageBuilder_SendEmailRequest covering success, NATS error, and
email service error-response paths
Generated with [Claude Code](https://claude.ai/code)
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
.github/workflows/license-header-check.yml:19
exclude_patternwas expanded to excludetemplates, which means the CI license-header workflow will no longer enforce headers for the newly added email templates. If templates are intended to carry license headers (they appear to in this PR), consider removingtemplatesfrom the exclusion so CI continues to validate them.
uses: linuxfoundation/lfx-public-workflows/.github/workflows/license-header-check.yml@main
with:
copyright_line: "Copyright The Linux Foundation and each contributor to LFX."
exclude_pattern: "gen|templates"
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/service/notifications_diff_test.go (1)
54-61: ⚡ Quick winStrengthen the multi-role case with explicit role assertions.
This case currently checks only count, so incorrect role/user mappings could still pass. Add
wantContainsentries for both expected assignments.Proposed test tweak
{ name: "multiple roles added", old: events.ProjectSettings{}, new: events.ProjectSettings{ Writers: []events.UserInfo{alice}, Auditors: []events.UserInfo{bob}, }, - wantLen: 2, + wantLen: 2, + wantContains: []roleAssignment{ + {User: alice, Role: "Writer"}, + {User: bob, Role: "Auditor"}, + }, },🤖 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 `@internal/service/notifications_diff_test.go` around lines 54 - 61, The "multiple roles added" table test only asserts wantLen but should also assert the specific role→user mappings; update the test case (the entry with name "multiple roles added" in notifications_diff_test.go) to include wantContains (or similar expected-members field used by the table) with explicit entries for the Writers assignment containing alice and the Auditors assignment containing bob so the test verifies both count and correct role/user mapping for events.ProjectSettings diffs.internal/infrastructure/nats/message_test.go (1)
439-450: ⚡ Quick winAssert the serialized email payload, not just the subject.
Right now these success cases can pass even if
To/Subject/HTML/Textmarshalling breaks. Validatemsg.Datacontent in the matcher.🤖 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 `@internal/infrastructure/nats/message_test.go` around lines 439 - 450, The test's mock matcher for MockNATSConn.RequestMsgWithContext currently only checks msg.Subject == emailapi.SendEmailSubject, which lets broken email marshalling pass; update the matcher to also deserialize and assert the serialized email payload in msg.Data contains the expected To, Subject, HTML and Text fields (or directly compare to the expected JSON bytes) so the test fails if To/Subject/HTML/Text marshalling changes; update both success cases' mockSetup matchers to validate msg.Data content alongside the subject.
🤖 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 `@internal/service/notifications_handler_test.go`:
- Around line 120-143: The test currently sets expectations with
mockRepo.On("GetProjectBase", ...) and mockMsg.On("SendEmailRequest", ...) but
only verifies call counts; after invoking svc.HandleProjectSettingsUpdated(...)
add verification calls mockRepo.AssertExpectations(t) and
mockMsg.AssertExpectations(t) to ensure all mocked expectations (GetProjectBase
and SendEmailRequest) were satisfied; place these assertions immediately after
the handler invocation and before or after mockMsg.AssertNumberOfCalls to follow
the repository's test pattern.
---
Nitpick comments:
In `@internal/infrastructure/nats/message_test.go`:
- Around line 439-450: The test's mock matcher for
MockNATSConn.RequestMsgWithContext currently only checks msg.Subject ==
emailapi.SendEmailSubject, which lets broken email marshalling pass; update the
matcher to also deserialize and assert the serialized email payload in msg.Data
contains the expected To, Subject, HTML and Text fields (or directly compare to
the expected JSON bytes) so the test fails if To/Subject/HTML/Text marshalling
changes; update both success cases' mockSetup matchers to validate msg.Data
content alongside the subject.
In `@internal/service/notifications_diff_test.go`:
- Around line 54-61: The "multiple roles added" table test only asserts wantLen
but should also assert the specific role→user mappings; update the test case
(the entry with name "multiple roles added" in notifications_diff_test.go) to
include wantContains (or similar expected-members field used by the table) with
explicit entries for the Writers assignment containing alice and the Auditors
assignment containing bob so the test verifies both count and correct role/user
mapping for events.ProjectSettings diffs.
🪄 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
Run ID: 56020bc1-2121-43ee-9c53-13ab325d1648
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.github/workflows/license-header-check.ymlgo.modinternal/infrastructure/nats/message_test.gointernal/service/notifications_diff.gointernal/service/notifications_diff_test.gointernal/service/notifications_handler.gointernal/service/notifications_handler_test.go
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/license-header-check.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/notifications_diff.go
- internal/service/notifications_handler.go
- notifications_handler.go → project_subscriber.go Rename makes the pattern explicit: event subscribers (fire-and-forget, no response) live here; future handlers like ProjectUpdated belong here too - Fold notifications_diff.go into project_subscriber.go diffNewMembers/diffRole/memberKey are only called from the subscriber and don't warrant a separate file - Merge the two test files into project_subscriber_test.go accordingly - Update log prefixes from "notifications_handler:" to "project_subscriber:" Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
…ructure
Move packages to align internal/ with a strict three-layer layout:
internal/log/ → internal/infrastructure/log/
internal/middleware/ → internal/infrastructure/middleware/
internal/service/notifications_render.go + templates/
→ internal/service/email/ (new package)
The email package exports RenderProjectRoleNotification and
ProjectRoleNotificationData; adding a new email type means adding one
file to internal/service/email/ alongside its templates.
Update CLAUDE.md:
- Reflect new directory tree
- Bump Go version to 1.24+
- Add LFX_SELF_SERVE_BASE_URL to env vars table
- Clarify the two distinct NATS patterns (request/reply RPC vs
event subscriptions) and which file each belongs in
Generated with [Claude Code](https://claude.ai/code)
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
- Accept both "staging" and "stg" for LFX_ENVIRONMENT staging URL - Add AssertExpectations to project_subscriber_test table-driven cases - Strengthen license-check Makefile target: add SPDX check, share git ls-files result, and exclude email templates (Go template comment syntax not recognized by external license checker) Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/service/project_subscriber.go (1)
30-31: ⚡ Quick winUse infrastructure logging helpers instead of direct
slog.WarnContextin internal layer code.This file should route structured logging through the shared logging helpers for consistency with repository conventions.
As per coding guidelines:
internal/**/*.go: Use structured logging helpers (AppendCtx, InitStructureLogConfig) frominternal/infrastructure/log/for consistent log output.Also applies to: 41-42, 62-64, 83-85, 97-99
🤖 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 `@internal/service/project_subscriber.go` around lines 30 - 31, Replace direct slog.WarnContext calls in project_subscriber.go with the repository's structured logging helpers: initialize or ensure the structured logger via InitStructureLogConfig where necessary, then use AppendCtx(ctx) from internal/infrastructure/log to emit the warning. For each occurrence (e.g., the current slog.WarnContext(ctx, "project_subscriber: failed to unmarshal project settings updated event", constants.ErrKey, err)), call the shared helper chain (InitStructureLogConfig if needed, then log.AppendCtx(ctx).Warn(...)) and pass the same message and structured fields (constants.ErrKey, err) so the output matches current semantics but uses the shared logger. Ensure you update all listed sites (around lines 30–31, 41–42, 62–64, 83–85, 97–99) to use AppendCtx and InitStructureLogConfig instead of slog.WarnContext.internal/service/project_subscriber_test.go (1)
162-233: ⚡ Quick winAdd a regression case for email-only → username+email identity transition.
TestDiffNewMemberscurrently misses the scenario where the same user appears as email-only inoldand username+same-email innew; this should not be treated as a new addition.🤖 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 `@internal/service/project_subscriber_test.go` around lines 162 - 233, Add a regression test case in the TestDiffNewMembers table that ensures an identity transition from email-only to username+same-email is not treated as a new member: in the tests slice (used by TestDiffNewMembers) add an entry where old is events.ProjectSettings{Writers: []events.UserInfo{noUsername}} and new is events.ProjectSettings{Writers: []events.UserInfo{alice}} (alice has the same email as noUsername) and expect no additions (omit wantLen or set wantLen: 0); reference the existing roleAssignment and Writers handling so the diff logic that compares identities recognizes equal users and does not emit a new roleAssignment.
🤖 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 `@internal/service/project_subscriber.go`:
- Around line 125-160: The current memberKey("memberKey") can produce different
keys for the same person when one record has only Email and another has
Username+Email; change memberKey so it produces a stable canonical key across
those cases (e.g., prefer Email when present and fall back to Username, or
construct a deterministic composite like "email:<email>|username:<username>"
when both exist) so diffRole's comparison logic (diffRole) will not treat the
same user as a new addition when the identity shape changes.
In `@Makefile`:
- Around line 157-164: The xargs calls used to build missing_copyright and
missing_spdx can run grep recursively when TRACKED is empty; update the xargs
invocations that consume $$TRACKED (the ones populating missing_copyright and
missing_spdx) to pass the -r/--no-run-if-empty flag so xargs does nothing on
empty input, keeping the grep -rL checks limited to the intended file list and
preventing an unintended recursive scan from '.'.
---
Nitpick comments:
In `@internal/service/project_subscriber_test.go`:
- Around line 162-233: Add a regression test case in the TestDiffNewMembers
table that ensures an identity transition from email-only to username+same-email
is not treated as a new member: in the tests slice (used by TestDiffNewMembers)
add an entry where old is events.ProjectSettings{Writers:
[]events.UserInfo{noUsername}} and new is events.ProjectSettings{Writers:
[]events.UserInfo{alice}} (alice has the same email as noUsername) and expect no
additions (omit wantLen or set wantLen: 0); reference the existing
roleAssignment and Writers handling so the diff logic that compares identities
recognizes equal users and does not emit a new roleAssignment.
In `@internal/service/project_subscriber.go`:
- Around line 30-31: Replace direct slog.WarnContext calls in
project_subscriber.go with the repository's structured logging helpers:
initialize or ensure the structured logger via InitStructureLogConfig where
necessary, then use AppendCtx(ctx) from internal/infrastructure/log to emit the
warning. For each occurrence (e.g., the current slog.WarnContext(ctx,
"project_subscriber: failed to unmarshal project settings updated event",
constants.ErrKey, err)), call the shared helper chain (InitStructureLogConfig if
needed, then log.AppendCtx(ctx).Warn(...)) and pass the same message and
structured fields (constants.ErrKey, err) so the output matches current
semantics but uses the shared logger. Ensure you update all listed sites (around
lines 30–31, 41–42, 62–64, 83–85, 97–99) to use AppendCtx and
InitStructureLogConfig instead of slog.WarnContext.
🪄 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
Run ID: 34b38a6e-5e11-4a28-9b2e-59d448289e31
📒 Files selected for processing (25)
CLAUDE.mdMakefilecmd/project-api/main.gointernal/infrastructure/log/log.gointernal/infrastructure/log/log_test.gointernal/infrastructure/middleware/authorization.gointernal/infrastructure/middleware/authorization_test.gointernal/infrastructure/middleware/body_limit.gointernal/infrastructure/middleware/body_limit_test.gointernal/infrastructure/middleware/request_id.gointernal/infrastructure/middleware/request_id_test.gointernal/infrastructure/middleware/request_logger.gointernal/infrastructure/middleware/request_logger_test.gointernal/service/document_operations.gointernal/service/email/project_role_notification.gointernal/service/email/project_role_notification_test.gointernal/service/email/templates/project_role_notification.htmlinternal/service/email/templates/project_role_notification.txtinternal/service/folder_operations.gointernal/service/link_operations.gointernal/service/project_handlers.gointernal/service/project_operations.gointernal/service/project_subscriber.gointernal/service/project_subscriber_test.goscripts/root-project-setup/main.go
✅ Files skipped from review due to trivial changes (7)
- internal/service/document_operations.go
- internal/infrastructure/middleware/request_logger.go
- internal/service/email/project_role_notification_test.go
- internal/service/email/templates/project_role_notification.html
- internal/service/email/templates/project_role_notification.txt
- scripts/root-project-setup/main.go
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/project-api/main.go
- internal/service/project_operations.go
…ures License header check: exclude_pattern uses comma-separated values, not pipe. "gen|templates" was treated as a single literal pattern (| was escaped), so the gen/ directory was never excluded. Changed to "gen,templates" so both exclusions apply correctly. MegaLinter djlint (H030, H031, H006, H023): - Add meta description and keywords tags to HTML email template - Add width/height attributes to img tag - Replace © entity reference with literal © character Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Align with the templates used in itx-service-zoom (the canonical LFX Self-Serve email design): - Logo from S3 (lfx-logo-color.png) replacing the old lfx.linuxfoundation.org URL - White background, Inter font, flat wrapper layout (no colored header band) - #009AFF button color, 8px border-radius, fallback link below CTA - Full LFX trademark footer matching other LFX Self-Serve emails Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Move all inline styles to CSS classes to satisfy djlint H021: - greeting p: new .greeting class - btn anchor: drop redundant inline color (already in .btn) - footer link: .footer a rule Also: - Remove h1 title below greeting; go straight into body message - Link goes to /projects/overview instead of project-specific slug URL - Drop unused fmt import from project_subscriber.go Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
- Replace single-key memberKey with multi-key memberKeys so a user whose identity record gains a username between snapshots is not treated as a new addition (email-only in old matched by shared email in new) - Add test case covering the identity-shape-change scenario - Fix Makefile license-check to check only the first 4 lines of each file (matching CI behaviour) and use a POSIX-safe pipeline instead of bash process substitution, which also prevents recursive scanning when the tracked file list is empty Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
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
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.
Summary
Implementation details
New files:
Changed files:
Dependencies
This PR depends on LFXV2-1768 (`lfx-v2-email-service`) being deployed. The email service code must be merged and the pseudo-version in `go.mod` updated to the final tagged version once that PR merges.
How to test locally
Ticket
LFXV2-1730
🤖 Generated with Claude Code