[#13455] Allow real email providers in dev server, and implement SMTP sender by iZUMi-kyouka · Pull Request #13575 · TEAMMATES/teammates

iZUMi-kyouka · 2026-03-08T08:06:11Z

Outline of Solution

Allow real email providers in EmailSender.java
Add new SmtpService.java using Jakarta Mail with Angus Mail as its implementation
Add tests for SmtpService in EmailSenderTest.java
Add new configs in Config.java and in build.template.properties

…ion key

Copilot

Pull request overview

Adds SMTP as a supported email provider (including in dev server) and wires it into configuration and the email-sending selection logic.

Changes:

Introduced SmtpService (Jakarta Mail/Angus Mail) implementing EmailSenderService
Updated EmailSender to select SMTP based on config (alongside Sendgrid/Mailgun/Mailjet)
Added SMTP config keys and test coverage for SMTP message conversion/validation

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/main/java/teammates/logic/external/SmtpService.java	Implements SMTP-backed email sending and MIME message construction
src/main/java/teammates/logic/api/EmailSender.java	Adds SMTP to the service-selection chain (and allows real providers in dev)
src/main/java/teammates/common/util/Config.java	Adds SMTP config keys and `isUsingSmtp()` gating logic
src/main/resources/build.template.properties	Documents new SMTP configuration properties
src/test/java/teammates/logic/api/EmailSenderTest.java	Adds tests for SMTP MIME conversion and invalid protocol validation
build.gradle	Adds Jakarta Mail API + Angus Mail runtime dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-12T05:30:29Z

+        message.setRecipient(Message.RecipientType.TO, new InternetAddress(wrapper.getRecipient()));
+        if (wrapper.getBcc() != null && !wrapper.getBcc().isEmpty()) {
+            message.setRecipient(Message.RecipientType.BCC, new InternetAddress(wrapper.getBcc()));
+        }
+        message.setReplyTo(new InternetAddress[] { new InternetAddress(wrapper.getReplyTo()) });


new InternetAddress(String) and setRecipient(...) only handle a single address; if wrapper.getRecipient(), wrapper.getBcc(), or wrapper.getReplyTo() contain a comma-separated list (a common representation), this will either fail parsing or silently treat it as one address. Use InternetAddress.parse(...) and message.setRecipients(...) / message.setReplyTo(...) with the parsed array to correctly support multiple recipients.

Suggested change

message.setRecipient(Message.RecipientType.TO, new InternetAddress(wrapper.getRecipient()));

if (wrapper.getBcc() != null && !wrapper.getBcc().isEmpty()) {

message.setRecipient(Message.RecipientType.BCC, new InternetAddress(wrapper.getBcc()));

}

message.setReplyTo(new InternetAddress[] { new InternetAddress(wrapper.getReplyTo()) });

message.setRecipients(Message.RecipientType.TO, InternetAddress.parse(wrapper.getRecipient()));

if (wrapper.getBcc() != null && !wrapper.getBcc().isEmpty()) {

message.setRecipients(Message.RecipientType.BCC, InternetAddress.parse(wrapper.getBcc()));

}

if (wrapper.getReplyTo() != null && !wrapper.getReplyTo().isEmpty()) {

message.setReplyTo(InternetAddress.parse(wrapper.getReplyTo()));

}

Currently, there seems to be no use case of sending or BCC-ing to multiple email address. I think this should not be necessary as of now.

samuelfangjw · 2026-03-20T13:45:39Z

One last thing to note, I ran this file through AI and it brought up (justifiably) that the way Transport.send() is used results in new connections per email that could result in significant overhead. However, I don't think the complexity of setting up a connection pool that's thread safe is worth it at them moment, especially since we will primarily be using sendgrid. Just something to keep in mind. We can revisit this again if it becomes a significant constraint.

iZUMi-kyouka · 2026-03-23T21:21:28Z

Some changes made:

Moved all SmtpService tests into its own test class
Created stub to allow mocking of Transport.send() behaviour to tests for the SMTP to HTTP code mapping
Add mapping of SMTP to HTTP error code
Added app.smtp.auth config to allow toggling of auth
Removed the timeout configs

I will smoke test with these fixes on staging soon.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T06:07:01Z

+        } catch (SMTPSendFailedException sfe) {
+            // SMTP 5xx errors indicates a permanent failure, while 4xx indicates a transient failure.
+            // Since HTTP 5xx errors are retried by default while HTTP 4xx errors are not, map the codes accordingly.
+
+            int replyCode = sfe.getReturnCode();
+            // Permanent SMTP send failure, do not retry
+            if (replyCode >= 500 && replyCode < 600) {
+                throw new EmailSendingException(sfe, HttpStatus.SC_BAD_REQUEST);
+            }
+
+            // Transient SMTP send failure, retry may succeed
+            throw new EmailSendingException(sfe, HttpStatus.SC_BAD_GATEWAY);
+        } catch (MessagingException me) {


The SMTP reply-code → HTTP status mapping is documented as a way to control Cloud Tasks retry behavior, but the current queue worker (SendEmailWorkerAction) retries on any non-2xx regardless of the underlying EmailSendingStatus code. Either adjust the worker to respect “permanent” vs “transient” failures (e.g., return a 2xx for permanent failures to stop retries), or simplify/remove this mapping/comment to avoid misleading future maintainers.

but the current queue worker (SendEmailWorkerAction) retries on any non-2xx regardless of the underlying EmailSendingStatus code.

Let's fix this in another issue, mapping all errors to http 502 in the SendEmailWorkerAction is another code smell that should be resolved.

Copilot · 2026-03-24T06:07:01Z

    EmailSender() {
-        if (Config.IS_DEV_SERVER) {
-            service = new EmptyEmailService();
+        if (Config.isUsingSendgrid()) {
+            service = new SendgridService();
+        } else if (Config.isUsingMailgun()) {
+            service = new MailgunService();
+        } else if (Config.isUsingMailjet()) {
+            service = new MailjetService();
+        } else if (Config.isUsingSmtp()) {
+            service = new SmtpService();
        } else {
-            if (Config.isUsingSendgrid()) {
-                service = new SendgridService();
-            } else if (Config.isUsingMailgun()) {
-                service = new MailgunService();
-            } else if (Config.isUsingMailjet()) {
-                service = new MailjetService();
-            } else {
-                service = new EmptyEmailService();
-            }
+            service = new EmptyEmailService();
        }


EmailSender’s provider selection is now config-driven (including dev server), but there isn’t any test coverage asserting the chosen service matches the configured provider (especially the new SMTP branch). Consider adding a focused unit test for the selection logic (may require a small refactor to make the selection testable without relying on static Config initialization).

If we want to test the logic in Config.java (such as the isUsingSmtp method that verifies all SMTP config params to return true), might not be currently feasible with the way all fields are declared as static final.

As for other classes that depend on Config.java, un general I feel like Config.java is also quite hard to mock as everything is a public static field. While some of the logic is quite trivial, some of the validation I add for isUsingSmtp might not be so.

What's your opinion on this? (though as a defensive and secure practice, for example requiring explicit values for smtp auth and security protocol, these Smtp configs are also checked at runtime, with the appropriate tests written for them, so maybe not so critical to test in the Config.java level)?

I don't think it's necessary at the moment. Thanks for looking into this!

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

iZUMi-kyouka · 2026-03-24T10:40:25Z

Summary of issues to be addressed in future PRs:

SendEmailWorkerAction returning 502 regardless of the HTTP SC from the email sender
No thread-safe connection pool in SmtpService to send emails which might become an overhead when sending a lot of emails
parseEmail method in EmailSender is publicly exposed despite only used for testing

samuelfangjw

Thanks for the great work!

…ent SMTP sender (TEAMMATES#13575)

iZUMi-kyouka added 23 commits March 8, 2026 12:38

Add eclipse angus as dependencies

ed17a58

Add smtp configs in Config.java and build.template.properties

e4b46c7

Implement SmtpEmailService basic implementation

0aab74d

Add SmtpEmailService in EmailSender

77e6acf

Add SMTP socket read write timeout configs

fdc1d89

Implement setting of SMTP socket connection timeouts

08d43de

Add SMTP security protol config

1b8bbd4

Implement SMTP security protol config logic

84d71d3

Make default text encoding a constant

d3d0b76

Refactor exception handling and shorten constructor

210662e

Fix lint

4ec8b55

Set default smtp security protocol to starttls if invalid or not set

729a3e1

Add test for SMTP service

4785cbe

Shorten SmtpEmailService name to SmptService

a2b517f

Improve accuracy of HTTP Status Code returned

5b0775f

Require STARTTLS when TLS is used

c622808

Rename text encoding constant

9d8f2a5

Rename SMTP socket config build.properties key to reflect actual sess…

121224c

…ion key

Explicitly require SMTP security to be either ssl or starttls

8d2d79d

Refactor SmtpService to allow DI for config values for testing

678bbad

Test exception thrown when security protocol is invalid

949387a

Fix comment

f9d14d9

Revert index.yamlc change

f234ee7

iZUMi-kyouka requested review from DhiraPT and mingyuancode March 8, 2026 08:09

Require non-empty SMTP security protocol config in isUsingSmtp

6a416ca

iZUMi-kyouka marked this pull request as ready for review March 9, 2026 04:09

Merge branch 'master' into feature/smtp-email-sender

4e00a62

mingyuancode requested a review from Copilot March 12, 2026 05:28

Copilot AI reviewed Mar 12, 2026

View reviewed changes

samuelfangjw self-requested a review March 20, 2026 12:08

samuelfangjw reviewed Mar 20, 2026

View reviewed changes

iZUMi-kyouka added 9 commits March 23, 2026 12:37

Merge branch 'master' into feature/smtp-email-sender

c221ebe

Remove SMTP timeout configurations

f0b37bd

Import RecipientType from Message in SmtpService.java

952980c

Add auth enable configuration

8af2eae

Remove angus mail test dependency in build.gradle

81bda2f

Add mapping of SMTP error code to HTTP error code

83a841a

Rename doSendMessage to sendMessageWithWrapper

9701a77

Improve SmtpServiceTest

f6a0800

Merge branch 'master' into feature/smtp-email-sender

1ce0f61

iZUMi-kyouka added 2 commits March 24, 2026 05:22

Revert accidental removal of empty line

704de71

Rename to getTypicalEmailWrapper in SmtpServiceTest

093addc

iZUMi-kyouka force-pushed the feature/smtp-email-sender branch from 1917039 to 093addc Compare March 23, 2026 21:27

iZUMi-kyouka requested a review from Copilot March 24, 2026 06:01

Copilot started reviewing on behalf of iZUMi-kyouka March 24, 2026 06:02 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

Fix grammar

913c331

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Merge branch 'master' into feature/smtp-email-sender

a4f1931

samuelfangjw approved these changes Mar 26, 2026

View reviewed changes

Merge branch 'master' into feature/smtp-email-sender

bef111f

samuelfangjw enabled auto-merge (squash) March 26, 2026 14:31

Merge branch 'master' into feature/smtp-email-sender

f7baed7

samuelfangjw merged commit 5c9db6b into TEAMMATES:master Mar 26, 2026
8 checks passed

samuelfangjw added this to the V9.0.0-beta.7 milestone Mar 26, 2026

WeeJean pushed a commit to WeeJean/teammates that referenced this pull request Apr 7, 2026

[TEAMMATES#13455] Allow real email providers in dev server and implem…

a38f540

…ent SMTP sender (TEAMMATES#13575)

iZUMi-kyouka added a commit to iZUMi-kyouka/teammates that referenced this pull request Apr 11, 2026

[TEAMMATES#13455] Allow real email providers in dev server and implem…

c2c3fe6

…ent SMTP sender (TEAMMATES#13575)

Conversation

iZUMi-kyouka commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

iZUMi-kyouka Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samuelfangjw commented Mar 20, 2026

Uh oh!

iZUMi-kyouka commented Mar 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

samuelfangjw Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

iZUMi-kyouka Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelfangjw Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iZUMi-kyouka commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samuelfangjw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iZUMi-kyouka commented Mar 8, 2026 •

edited

Loading

iZUMi-kyouka Mar 24, 2026 •

edited

Loading

iZUMi-kyouka commented Mar 24, 2026 •

edited

Loading