Email Base Notification to Support HTTP and SMTP with similar capability to other notifiers#3516
Draft
RandithaK wants to merge 2 commits into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughAdds email sender API endpoints, backend sender/service/client support, console sender selection, and docs/test updates for SMTP and HTTP email providers. ChangesEmail provider support
Sequence Diagram(s)sequenceDiagram
participant Console
participant Notification API
participant NotificationSenderService
participant EmailExecutor
participant SMTPEmailClient
Console->>Notification API: create/list email sender
Notification API->>NotificationSenderService: persist sender data
EmailExecutor->>NotificationSenderService: SendEmail(senderID, EmailData)
NotificationSenderService->>SMTPEmailClient: Send(email data)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Bundle ReportChanges will decrease total bundle size by 63.7kB (-0.25%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gate-esmAssets Changed:
view changes for bundle: console-esmAssets Changed:
|
Malith-19
added
Type/Improvement
breaking change
RandithaK
force-pushed
the
improvement/email-notifier-smtp-http-support
branch
from
08678a6 to
cf899f6
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
The PR is split into multiple smaller commits for my ease of development, and would be made to a single commit before merging. |
RandithaK
requested review from
DonOmalVindula and
rajithacharith
as code owners
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/content/guides/guides/flows/advanced-configurations.mdx (1)
290-290: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse “Email Provider” here, not SMTP.
Recovery email delivery is provider-based now, so this prerequisite should not narrow the setup to SMTP. Please reword it to require an Email Provider so HTTP-backed senders are covered.
🤖 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 `@docs/content/guides/guides/flows/advanced-configurations.mdx` at line 290, The prerequisite text currently says “SMTP,” but this flow now uses provider-based recovery delivery. Update the wording in the relevant guide section to require an “Email Provider” instead, using the surrounding flow setup copy in advanced-configurations.mdx so HTTP-backed senders are included.docs/content/guides/getting-started/configuration.mdx (1)
658-674: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace the stale SMTP setup section, not just the link.
The surrounding copy still tells readers to configure SMTP in
deployment.yaml, which conflicts with the new runtime-managed Email Providers flow and the updated destination. Please rewrite this whole section to describe API-managed email providers and remove the restart/deployment.yaml guidance.🤖 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 `@docs/content/guides/getting-started/configuration.mdx` around lines 658 - 674, The “Email Configuration” section is stale and still instructs readers to configure SMTP in deployment.yaml, which conflicts with the new runtime-managed Email Providers flow. Rewrite the entire section anchored by “Email Configuration” to describe API-managed email providers, update the surrounding copy and example so it no longer references deployment.yaml or restart-based setup, and point readers to the updated Email Providers destination with the correct runtime configuration guidance.
🧹 Nitpick comments (6)
tests/integration/flow/recovery/basic_recovery_test.go (1)
114-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant sender tracking —
senderIDstored twice.The created email sender ID is saved to both
ts.emailSenderID(Line 128) andts.config.CreatedSenderIDs(Line 129), butTearDownSuiteonly cleans up viats.emailSenderID. The append toCreatedSenderIDsis dead state here. Worth dropping to avoid confusion, especially since other suites iterateCreatedSenderIDswithDeleteMessageNotificationSender— if that pattern is ever shared, an email sender ID in that slice would be deleted against the wrong (/message) endpoint.🤖 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 `@tests/integration/flow/recovery/basic_recovery_test.go` around lines 114 - 132, The email sender ID is being tracked twice in the recovery test setup, but only ts.emailSenderID is used for cleanup. Remove the redundant append to ts.config.CreatedSenderIDs in the test setup around CreateEmailNotificationSender so the sender is tracked in one place only and the teardown path stays unambiguous.backend/internal/notification/client/http_email_client_test.go (1)
70-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent provider for an email FORM fixture.
getValidCustomSenderFORMsetsProvider: common.MessageProviderTypeCustomwhileTypeisNotificationSenderTypeEmail. SincenewHTTPEmailClientis invoked directly here the provider value is effectively ignored, but for clarity and to mirror the real email-routing path it should usecommon.MessageProviderTypeHTTPlike the JSON fixture.♻️ Suggested tweak
- Provider: common.MessageProviderTypeCustom, + Provider: common.MessageProviderTypeHTTP,🤖 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 `@backend/internal/notification/client/http_email_client_test.go` around lines 70 - 81, The email FORM fixture in getValidCustomSenderFORM uses an inconsistent provider for an HTTP-based email client test. Update the NotificationSenderDTO returned by getValidCustomSenderFORM to use common.MessageProviderTypeHTTP so it matches the JSON fixture and the intended HTTP email path, while keeping the NotificationSenderTypeEmail and existing properties unchanged.api/notification-sender.yaml (1)
1139-1142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor schema inconsistency:
propertiesis required for email senders but not for message senders.
EmailNotificationSenderlistspropertiesunderrequired, whereasMessageNotificationSender(Line 1167-1169) only requiresnameandprovider. If this divergence is intentional (email senders cannot be created without provider config), it's fine; otherwise consider aligning the two for a consistent client contract.🤖 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 `@api/notification-sender.yaml` around lines 1139 - 1142, The notification sender schemas are inconsistent because EmailNotificationSender requires properties while MessageNotificationSender does not. Review the sender definitions in notification-sender.yaml and either align MessageNotificationSender with EmailNotificationSender by adding properties to its required list, or confirm the difference is intentional and keep both contracts explicitly consistent in the schema definitions.backend/internal/notification/client/custom_client.go (2)
85-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplication with
HTTPEmailClient.Send. The JSON/FORM request construction, header application, and non-2xx response handling here largely mirrorhttp_email_client.go. Extracting a shared helper (e.g., adoWebhookRequestonhttpWebhookConfig) would reduce drift between the two webhook senders.🤖 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 `@backend/internal/notification/client/custom_client.go` around lines 85 - 140, The request-building and response-handling logic in CustomClient.sendSMS is duplicated from HTTPEmailClient.Send, so factor the shared JSON/FORM request creation, header application, and non-2xx error handling into a common helper such as a doWebhookRequest method on httpWebhookConfig or a shared utility. Keep sendSMS focused on SMS-specific logging and body preparation, and have both senders call the shared helper to avoid drift between custom_client.go and the email client implementation.
85-118: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCustom
httpHeadersare applied after the Content-Type header, so they can silently override it.
req.Header.Set(ContentTypeHeaderName, ...)is set in the JSON/FORM branch, then the loop at Lines 116-118 re-applies all configured headers. If an admin includes aContent-Typeentry inhttp_headers, it overrides the format-derived value and may break encoding. Consider applying custom headers first, or skippingContent-Typein the loop.🤖 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 `@backend/internal/notification/client/custom_client.go` around lines 85 - 118, The CustomClient.sendSMS header setup currently lets c.config.httpHeaders override the Content-Type chosen by the JSON/FORM branches, which can break request encoding. Update sendSMS so the format-derived Content-Type set via req.Header.Set is preserved, either by applying custom headers before setting Content-Type or by skipping Content-Type in the loop over c.config.httpHeaders. Use the sendSMS function and the c.config.httpHeaders iteration as the key places to change.backend/internal/flow/executor/register.go (1)
142-142: 📐 Maintainability & Code Quality | 🔵 TrivialThe
EmailClientdependency field at line 142 is no longer used.The email executor has been migrated to use
NotifSenderSvc. Since this field is no longer accessed anywhere inbackend/internal/flow/executor, it should be removed to reduce technical debt.🤖 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 `@backend/internal/flow/executor/register.go` at line 142, The EmailClient dependency field is unused in the executor dependency struct and should be removed. Update the dependency definition in the register-related executor type that currently includes EmailClient, and make sure any construction or initialization code in backend/internal/flow/executor no longer passes or expects it since the email executor now uses NotifSenderSvc instead.
🤖 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 `@backend/internal/flow/executor/error_constants.go`:
- Around line 561-573: The ErrEmailProviderNotConfigured service error in
error_constants.go references i18n keys that are missing from the en-US
translation bundle. Add both flows.executor.errors.email_provider_not_configured
and flows.executor.errors.email_provider_not_configured_desc to
backend/cmd/server/bootstrap/i18n/en-US.json, matching the wording used in the
ErrEmailProviderNotConfigured I18nMessage so the executor can resolve localized
text instead of falling back to defaults.
In `@backend/internal/notification/client/smtp_email_client.go`:
- Around line 127-129: The CRLF check in Send currently covers only
emailData.Subject, but buildMessage also writes From, To, and Cc directly into
SMTP headers. Update Send and/or buildMessage in smtp_email_client.go to
validate every address-bearing field (configured from-address, recipients, and
cc) before composing the message, rejecting any value with CRLF or parsing
addresses safely with net/mail.ParseAddress. Use the existing Send and
buildMessage flow to locate the validation points and ensure all header inputs
are sanitized consistently.
- Around line 102-106: The SMTP client validation currently allows
enableAuthentication without requiring TLS, which can later fail when PlainAuth
is used. Update the constructor validation path in SmtpEmailClient creation and
the matching config check in the TLS/auth setup block so that when
enableAuthentication is true and TLS is not explicitly enabled, the method
returns a configuration error immediately. Use the existing config flags
(enableAuthentication and enableTLS) in both validation locations to keep the
behavior consistent.
In `@backend/internal/notification/client/utils.go`:
- Around line 39-70: `parseHTTPWebhookConfig` currently returns a config even
when `common.CustomPropKeyURL` is missing or empty, so add an explicit non-empty
URL validation before returning the `httpWebhookConfig`. Update the
`parseHTTPWebhookConfig` function to fail fast with an error if `config.url` was
never set during the sender.Properties loop, keeping the function self-contained
and aligned with the existing URL invariant enforced elsewhere.
In `@backend/internal/notification/utils.go`:
- Around line 159-167: Update validateSMTPProperties to inspect the
enable_authentication property and, when it is true, add username and password
as required fields in the requiredProps map before calling
validateSenderProperties. Keep the existing host, port, and from_address checks
intact, and align the validation rules with newSMTPEmailClient so configurations
with authentication enabled are rejected during validation instead of failing
later at send time.
In `@docs/content/use-cases/b2c/try-it-out/account-recovery.mdx`:
- Around line 21-23: Remove the restart step from the provider setup
instructions in the account recovery walkthrough, since Email providers are now
configured at runtime. Update the surrounding text in the recovery flow so it
only tells users to configure the Email Provider via the Email Providers guide,
and if any later step still requires a restart, mention that separately in the
relevant section rather than here.
In
`@frontend/apps/console/src/features/login-flow/components/resource-property-panel/extended-properties/execution-properties/EmailProperties.tsx`:
- Around line 59-80: The EmailProperties UI is using SMS-specific translation
keys for its sender label, placeholder, and empty-state text. Update the strings
referenced in EmailProperties to use email-appropriate i18n keys (preferably
under an email sender namespace, or a neutral shared namespace) instead of the
current smsOtp.sender.* keys. Make sure the Select, MenuItem placeholder, and
Alert copy all point to the new email-specific translations so the email panel
shows the correct wording.
---
Outside diff comments:
In `@docs/content/guides/getting-started/configuration.mdx`:
- Around line 658-674: The “Email Configuration” section is stale and still
instructs readers to configure SMTP in deployment.yaml, which conflicts with the
new runtime-managed Email Providers flow. Rewrite the entire section anchored by
“Email Configuration” to describe API-managed email providers, update the
surrounding copy and example so it no longer references deployment.yaml or
restart-based setup, and point readers to the updated Email Providers
destination with the correct runtime configuration guidance.
In `@docs/content/guides/guides/flows/advanced-configurations.mdx`:
- Line 290: The prerequisite text currently says “SMTP,” but this flow now uses
provider-based recovery delivery. Update the wording in the relevant guide
section to require an “Email Provider” instead, using the surrounding flow setup
copy in advanced-configurations.mdx so HTTP-backed senders are included.
---
Nitpick comments:
In `@api/notification-sender.yaml`:
- Around line 1139-1142: The notification sender schemas are inconsistent
because EmailNotificationSender requires properties while
MessageNotificationSender does not. Review the sender definitions in
notification-sender.yaml and either align MessageNotificationSender with
EmailNotificationSender by adding properties to its required list, or confirm
the difference is intentional and keep both contracts explicitly consistent in
the schema definitions.
In `@backend/internal/flow/executor/register.go`:
- Line 142: The EmailClient dependency field is unused in the executor
dependency struct and should be removed. Update the dependency definition in the
register-related executor type that currently includes EmailClient, and make
sure any construction or initialization code in backend/internal/flow/executor
no longer passes or expects it since the email executor now uses NotifSenderSvc
instead.
In `@backend/internal/notification/client/custom_client.go`:
- Around line 85-140: The request-building and response-handling logic in
CustomClient.sendSMS is duplicated from HTTPEmailClient.Send, so factor the
shared JSON/FORM request creation, header application, and non-2xx error
handling into a common helper such as a doWebhookRequest method on
httpWebhookConfig or a shared utility. Keep sendSMS focused on SMS-specific
logging and body preparation, and have both senders call the shared helper to
avoid drift between custom_client.go and the email client implementation.
- Around line 85-118: The CustomClient.sendSMS header setup currently lets
c.config.httpHeaders override the Content-Type chosen by the JSON/FORM branches,
which can break request encoding. Update sendSMS so the format-derived
Content-Type set via req.Header.Set is preserved, either by applying custom
headers before setting Content-Type or by skipping Content-Type in the loop over
c.config.httpHeaders. Use the sendSMS function and the c.config.httpHeaders
iteration as the key places to change.
In `@backend/internal/notification/client/http_email_client_test.go`:
- Around line 70-81: The email FORM fixture in getValidCustomSenderFORM uses an
inconsistent provider for an HTTP-based email client test. Update the
NotificationSenderDTO returned by getValidCustomSenderFORM to use
common.MessageProviderTypeHTTP so it matches the JSON fixture and the intended
HTTP email path, while keeping the NotificationSenderTypeEmail and existing
properties unchanged.
In `@tests/integration/flow/recovery/basic_recovery_test.go`:
- Around line 114-132: The email sender ID is being tracked twice in the
recovery test setup, but only ts.emailSenderID is used for cleanup. Remove the
redundant append to ts.config.CreatedSenderIDs in the test setup around
CreateEmailNotificationSender so the sender is tracked in one place only and the
teardown path stays unambiguous.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a10b777d-3a7a-46a3-868f-19b613d794e2
⛔ Files ignored due to path filters (4)
backend/tests/mocks/notification/clientmock/EmailClientInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/notification/clientmock/MessageClientInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/notification/clientmock/NotificationClientInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/notification/notificationmock/NotificationSenderServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (62)
api/notification-sender.yamlbackend/internal/flow/executor/email_executor.gobackend/internal/flow/executor/email_executor_test.gobackend/internal/flow/executor/error_constants.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/sms_executor.gobackend/internal/flow/executor/sms_executor_test.gobackend/internal/notification/NotificationSenderServiceInterface_mock_test.gobackend/internal/notification/client/client.gobackend/internal/notification/client/custom_client.gobackend/internal/notification/client/custom_client_test.gobackend/internal/notification/client/factory.gobackend/internal/notification/client/factory_test.gobackend/internal/notification/client/http_email_client.gobackend/internal/notification/client/http_email_client_test.gobackend/internal/notification/client/smtp_email_client.gobackend/internal/notification/client/smtp_email_client_test.gobackend/internal/notification/client/twilio_client.gobackend/internal/notification/client/twilio_client_test.gobackend/internal/notification/client/utils.gobackend/internal/notification/client/utils_test.gobackend/internal/notification/client/vonage_client.gobackend/internal/notification/client/vonage_client_test.gobackend/internal/notification/common/constants.gobackend/internal/notification/common/model.gobackend/internal/notification/handler.gobackend/internal/notification/handler_test.gobackend/internal/notification/init.gobackend/internal/notification/notification_sender_service.gobackend/internal/notification/notification_sender_service_test.gobackend/internal/notification/otp_service.gobackend/internal/notification/otp_service_test.gobackend/internal/notification/utils.gobackend/internal/notification/utils_test.gobackend/internal/system/i18n/core/defaults.godocs/content/guides/getting-started/configuration.mdxdocs/content/guides/guides/flows/advanced-configurations.mdxdocs/content/guides/guides/notifications/email-providers.mdxdocs/content/guides/guides/smtp-server/smtp-server-configuration.mdxdocs/content/guides/key-concepts/authentication/passwordless/magiclink.mdxdocs/content/use-cases/b2c/configure-it-yourself.mdxdocs/content/use-cases/b2c/try-it-out/account-recovery.mdxdocs/content/use-cases/b2c/try-it-out/onboard-internal-users.mdxdocs/sidebars.tsfrontend/apps/console/src/features/flows/validation/validation-rules.tsfrontend/apps/console/src/features/login-flow/components/LoginFlowBuilder.tsxfrontend/apps/console/src/features/login-flow/components/resource-property-panel/extended-properties/execution-properties/EmailProperties.tsxfrontend/apps/console/src/features/notification-senders/api/useNotificationSenders.tsfrontend/apps/console/src/features/notification-senders/constants/query-keys.tstests/integration/authn/sms_otp_auth_test.gotests/integration/flow/authentication/assurance_test.gotests/integration/flow/authentication/prompt_actions_test.gotests/integration/flow/authentication/sms_auth_test.gotests/integration/flow/common/utils.gotests/integration/flow/recovery/basic_recovery_test.gotests/integration/flow/recovery/sms_otp_recovery_test.gotests/integration/flow/registration/http_request_executor_runtime_data_test.gotests/integration/flow/registration/ou_registration_test.gotests/integration/flow/registration/sms_registration_test.gotests/integration/notification/message/notificationsenderapi_test.gotests/integration/notification/message/otpapi_test.gotests/integration/testutils/api_utils.go
💤 Files with no reviewable changes (1)
- docs/content/guides/guides/smtp-server/smtp-server-configuration.mdx
RandithaK
force-pushed
the
improvement/email-notifier-smtp-http-support
branch
from
09a9e62 to
8ca70a8
Compare
RandithaK
commented
Jun 30, 2026
RandithaK
commented
Jun 30, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/internal/notification/client/utils_test.go`:
- Line 25: The tests in utils_test use assert for prerequisite checks, but later
call err.Error() on the same value, so a failed check can still panic and mask
the real issue. Update the gate checks in these tests to use require.* instead
of assert.* for conditions like error presence, while keeping assert.* for the
remaining value comparisons. Focus on the test cases in utils_test that validate
errors before dereferencing them.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 340eee76-0ffa-4f5f-b518-e05e727af8c1
📒 Files selected for processing (1)
backend/internal/notification/client/utils_test.go
RandithaK
commented
Jun 30, 2026
RandithaK
commented
Jun 30, 2026
| enable_start_tls: true | ||
| enable_authentication: true | ||
| ``` | ||
| 1. Invitation emails are delivered through an Email provider. **Configure an Email Provider** in <ProductName />. See [Email Providers](../../../guides/guides/notifications/email-providers.mdx). |
Contributor
Author
There was a problem hiding this comment.
Let's discus on an approach to keep the config here, without being difficult to maintain
Contributor
Author
|
Follow-Ups - To Be Considered After this PR is merged
|
| ) | ||
|
|
||
| const ( | ||
| smtpLoggerComponentName = "SMTPEmailClient" |
Member
There was a problem hiding this comment.
Let's get rid of these logger constants. Check all other places as well
RandithaK
commented
Jul 1, 2026
RandithaK
commented
Jul 1, 2026
| bodyBytes, _ := io.ReadAll(resp.Body) | ||
| logger.Error(ctx, "Failed to send Email via HTTP client", log.Int("statusCode", resp.StatusCode), | ||
| log.String("response", string(bodyBytes))) | ||
| return fmt.Errorf("HTTP Email send failed, status: %d, response: %s", resp.StatusCode, string(bodyBytes)) |
Contributor
Author
There was a problem hiding this comment.
Client should send a go error.
Logging is done in backend/internal/notification/notification_sender_service.go
| } | ||
|
|
||
| // SMTPEmailClient implements the EmailClientInterface using SMTP. | ||
| type SMTPEmailClient struct { |
Member
There was a problem hiding this comment.
Shall we try to make this private?
RandithaK
commented
Jul 1, 2026
| } | ||
|
|
||
| // newSMTPEmailClient creates a new instance of SMTPEmailClient. | ||
| func newSMTPEmailClient(ctx context.Context, sender common.NotificationSenderDTO) (EmailClientInterface, error) { |
Contributor
Author
There was a problem hiding this comment.
Make a private helper function to pass the sender configs, and return
|
|
||
| // Send dispatches an email notification via SMTP. | ||
| func (c *SMTPEmailClient) Send(ctx context.Context, emailData common.EmailData) error { | ||
| logger := log.GetLogger().With(log.String(log.LoggerKeyComponentName, smtpLoggerComponentName)) |
Member
There was a problem hiding this comment.
Let's initialize logger inside struct and use here rather than initializing in each method
RandithaK
commented
Jul 1, 2026
Comment on lines
+95
to
+109
| if config.from == "" { | ||
| return nil, errors.New("from_address is missing") | ||
| } | ||
| if config.host == "" { | ||
| return nil, errors.New("host is missing") | ||
| } | ||
| if config.port <= 0 { | ||
| return nil, errors.New("port is invalid") | ||
| } | ||
| if config.enableAuthentication { | ||
| if !config.useTLS { | ||
| return nil, errors.New("TLS must be enabled when authentication is enabled") | ||
| } | ||
| if config.username == "" || config.password == "" { | ||
| return nil, errors.New("credentials are required when authentication is enabled") |
Contributor
Author
There was a problem hiding this comment.
Let's do this inline, and not seperately.
Within the switchCase
RandithaK
commented
Jul 1, 2026
|
|
||
| for _, addressList := range [][]string{emailData.To, emailData.CC, emailData.BCC} { | ||
| for _, address := range addressList { | ||
| if strings.ContainsAny(address, "\r\n") { |
Contributor
Author
There was a problem hiding this comment.
Deal with
/r/n
With Go Language Package
RandithaK
commented
Jul 1, 2026
| } | ||
| } | ||
|
|
||
| if strings.ContainsAny(emailData.Subject, "\r\n") { |
Contributor
Author
There was a problem hiding this comment.
Better to deal with
\r\n
With a trim, not a error
RandithaK
commented
Jul 1, 2026
| return errors.New("subject contains invalid characters") | ||
| } | ||
|
|
||
| if strings.ContainsAny(c.config.from, "\r\n") { |
Contributor
Author
There was a problem hiding this comment.
Better to take all these
strings.ContainsAny(c.config.from, "\r\n")
To a external, function.
Check on previous implementation, systemEmailValivator.go
| ) | ||
|
|
||
| // IsValidEmail returns true for syntactically valid email addresses. | ||
| func IsValidEmail(emailAddr string) bool { |
Member
There was a problem hiding this comment.
Better to keep this to perform the email validations inside notification email sender. Move to an appropriate place
| builder.WriteString(fmt.Sprintf("Cc: %s\r\n", strings.Join(emailData.CC, ", "))) | ||
| } | ||
| builder.WriteString(fmt.Sprintf("Subject: %s\r\n", mime.QEncoding.Encode("utf-8", emailData.Subject))) | ||
| builder.WriteString("MIME-Version: 1.0\r\n") |
Member
There was a problem hiding this comment.
Shall we define constants for these?
RandithaK
commented
Jul 1, 2026
Comment on lines
+199
to
+206
| client, err := smtp.NewClient(conn, c.config.host) | ||
| if err != nil { | ||
| _ = conn.Close() | ||
| return fmt.Errorf("smtp connection failed: %w", err) | ||
| } | ||
| defer func() { | ||
| _ = client.Close() | ||
| }() |
Contributor
Author
There was a problem hiding this comment.
If the error scenario is there,
If error, 204, is always executed, not need to have 201.
RandithaK
commented
Jul 1, 2026
| return fmt.Errorf("email send failed: %w", err) | ||
| } | ||
|
|
||
| if err := client.Quit(); err != nil { |
Contributor
Author
There was a problem hiding this comment.
Check why use client.quit...
Case is if client.close, did not work, we force close...
Check where the defer function is actually executed, after registering.
RandithaK
commented
Jul 1, 2026
| return fmt.Errorf("smtp connection failed: %w", err) | ||
| } | ||
| defer func() { | ||
| _ = client.Close() |
Contributor
Author
There was a problem hiding this comment.
If there is an error, better to log it in here.
RandithaK
commented
Jul 1, 2026
Comment on lines
+209
to
+211
| ok, _ := client.Extension("STARTTLS") | ||
| if !ok { | ||
| return errors.New("smtp connection failed: STARTTLS not supported by server") |
Contributor
Author
There was a problem hiding this comment.
Currently useTLS, needs STARTTLS,
Check whether there are clients who needs TLS from the begining.
RandithaK
commented
Jul 1, 2026
Comment on lines
+222
to
+226
| if c.config.enableAuthentication && c.config.username != "" && c.config.password != "" { | ||
| if !c.config.useTLS { | ||
| return errors.New("smtp connection failed: TLS must be enabled when authentication is enabled") | ||
| } | ||
| if err := client.Auth(smtp.PlainAuth("", c.config.username, c.config.password, c.config.host)); err != nil { |
Contributor
Author
There was a problem hiding this comment.
Check, might be correct.
Does TLS need auth and password
Clients, might have username password, but no TLS
Contributor
Author
|
Task
|
RandithaK
force-pushed
the
improvement/email-notifier-smtp-http-support
branch
from
55b20fc to
d7359fc
Compare
| builder.WriteString("MIME-Version: 1.0\r\n") | ||
|
|
||
| if emailData.IsHTML { | ||
| builder.WriteString("Content-Type: text/html; charset=\"utf-8\"\r\n") |
Member
There was a problem hiding this comment.
Shall we define constants for these?
RandithaK
force-pushed
the
improvement/email-notifier-smtp-http-support
branch
from
d7359fc to
ba64e86
Compare
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.
Purpose
fixes #3276
Introduces a unified Notification Sender management system to replace the legacy SMTP-based email sender, with support for both SMTP and HTTP based email support.
🔧 Summary of Breaking Changes
Describe what is changing
Deployment yaml based SMTP senders are no longer used for system level notifications, such as user onboarding and have to set the notifier manually through the flow builder.
💥 Impact
What will break? Who is affected?
All flows that used Email, such as User invite, Magic Link, and Password Reset.
🔄 Migration Guide
How should users update their code/configuration to adapt to the breaking changes? Include examples if helpful
Setup it as a email provider following the documentation.
Approach
NotificationSenderServiceInterfaceto support various underlying notification channels without tightly coupling the executor logic.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Summary
/notification-senders/email/...).senderIdfor email executors and added editor validation to requiresenderId.