Add Mailtrap email transport - #27873
Conversation
|
I've used an AI agent to create a PR for me (via the gh cli) to make it easier for me to ensure I've fulfilled all the guidelines outlined in contributing.md file. The code itself was written manually. Happy to answer all the questions you have. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #27873 +/- ##
=======================================
Coverage 70.31% 70.32%
=======================================
Files 2460 2460
Lines 169097 169144 +47
Branches 17143 17158 +15
=======================================
+ Hits 118898 118944 +46
- Misses 50199 50200 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
tsokolovs
changed the title
adeb5aa to
b0c90c2
Compare
9f945af to
1141592
Compare
AlexGaillard
left a comment
AlexGaillard
left a comment
There was a problem hiding this comment.
Thanks for this, just a few small changes here and also TY for pointing out the shortcomings of our mailer.test.ts, it's a tricky one to test 😅 .
| if (env['EMAIL_MAILTRAP_SANDBOX'] !== undefined) mailtrapOptions['sandbox'] = env['EMAIL_MAILTRAP_SANDBOX']; | ||
| if (env['EMAIL_MAILTRAP_INBOX_ID'] !== undefined) mailtrapOptions['testInboxId'] = env['EMAIL_MAILTRAP_INBOX_ID']; | ||
| if (env['EMAIL_MAILTRAP_BULK'] !== undefined) mailtrapOptions['bulk'] = env['EMAIL_MAILTRAP_BULK']; |
There was a problem hiding this comment.
I think this may be better if we can get some more explicit checks in here rather than checking that they're not undefined. mailtrapOptions is expecting and the app will still boot up and the conditional will still pass if EMAIL_MAILTRAP_SANDBOX is defined as a string in the env.
Also a nit, it might be easier to read if we nest the body of the conditions.
ie:
if (env['EMAIL_MAILTRAP_SANDBOX'] === true) {
mailtrapOptions['sandbox'] = env['EMAIL_MAILTRAP_SANDBOX'];
}
There was a problem hiding this comment.
Also, EMAIL_MAILTRAP_SANDBOX depends on EMAIL_MAILTRAP_INBOX_ID in order to be able to use the sandbox correctly, so there should probably be a check for that as well.
There was a problem hiding this comment.
Added more robust checks for Mailtrap and extracted them to the createMailtrapTransport function, in the same mailer.ts file.
| }); | ||
| }); |
There was a problem hiding this comment.
I think you've actually identified the shortcomings of our tests above 😓
That being said, this is still essentially testing nothing.
If we look into the getFreshMailer().transporter we can see the params of the created transporter:
transporter: MailtrapTransport {
name: 'MailtrapTransport',
version: '4.6.1',
client: MailtrapClient {
axios: [Function],
testInboxId: 12345,
accountId: undefined,
organizationId: undefined,
bulk: false,
sandbox: true
}
I think it might be worthwhile testing that those are created correctly, that way if Mailtrap every change any of these it should hopefully catch it.
There was a problem hiding this comment.
Added more meaningful tests for Mailtrap that check the changes and validations in mailer.ts file.
| // Re-import getMailer so it runs against a fresh module (the transporter is | ||
| // cached at module scope, and beforeEach's resetModules only affects fresh | ||
| // imports, not the statically imported getMailer used by the tests above). | ||
| // Without this, the cached transporter short-circuits the mailtrap branch. |
There was a problem hiding this comment.
I think maybe we should add a TODO that this should be applied to the tests above.
55e2309 to
7e6e5d3
Compare
e745b8f to
2575b50
Compare
2575b50 to
7a7da81
Compare
Add a native `mailtrap` option for `EMAIL_TRANSPORT` using Mailtrap's official nodemailer transport. Supports live sending, the bulk stream, and the sandbox testing inbox via EMAIL_MAILTRAP_TOKEN, EMAIL_MAILTRAP_SANDBOX, EMAIL_MAILTRAP_INBOX_ID, and EMAIL_MAILTRAP_BULK.
Re-import getMailer in the Mailtrap test so the module-scoped transporter cache doesn't short-circuit the new branch, ensuring it actually executes under coverage.
The mailtrap@4.6.1 snapshot referenced axios@1.16.1, which has no corresponding entry in the lockfile. Point it at the already-resolved axios@1.18.1 so that pnpm install --frozen-lockfile succeeds.
7a7da81 to
5df0715
Compare
2 participants
What's Changed
Adds Mailtrap as a native option for
EMAIL_TRANSPORT, alongside the existingsendmail,smtp,ses, andmailguntransports. It uses Mailtrap's officialmailtrappackage, which ships a Nodemailer transport (MailtrapTransport).The transport supports all three Mailtrap modes:
Implementation mirrors the existing Mailgun transport in
api/src/mailer.ts(lazyrequire, env-driven options). Options are only passed to the SDK when their env vars are set.New environment variables
EMAIL_MAILTRAP_TOKENEMAIL_MAILTRAP_SANDBOXEMAIL_MAILTRAP_INBOX_IDEMAIL_MAILTRAP_BULKTested Scenarios
api/src/mailer.test.tspasses (5/5),@directus/envpasses (78/78).EMAIL_MAILTRAP_BULK=true, all succeeded.Review Notes / Questions / Concerns
mailtrapdependency pulls inaxios, which dedupesqsto6.15.2in the lockfile. This is the only transitive change beyond the direct addition.Checklist
@directus/specs)@directus/sdk) updated to reflect the changes@directus/types) updated to reflect the changes@directus/system-data) updated for changes to system collections/fields/relations