-
-
Notifications
You must be signed in to change notification settings - Fork 35
feat!: Email support using lettre #419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat!: Email support using lettre #419
Conversation
Initial commit of email.rs using lettre Initial commit of Add email support using lettre crate Added the message printout if debug mode is enabled Added a test and deferred the extra headers and attachment features. Added tests for config and send_email(ignore).
…/cot into feat-add-email-support
…ded to improve test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive email support to the Cot framework using the lettre library, providing both console (development) and SMTP (production) transport backends for sending emails.
Key Changes:
- Implements high-level
EmailAPI withEmailMessagebuilder pattern for constructing and sending emails - Adds console and SMTP transport backends with pluggable architecture via
Transporttrait - Integrates email service into the request context and project configuration system
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
examples/send-email/ |
Complete example application demonstrating email functionality with HTML form and status feedback |
cot/src/email/ |
Core email module with message building, transport abstraction, and backend implementations |
cot/src/config.rs |
Email configuration structures supporting console and SMTP transports with URL-based config |
cot/src/request.rs |
Request extension trait additions to access email service |
cot/src/project.rs |
Email service initialization and integration into project bootstrap lifecycle |
cot/Cargo.toml |
New email feature flag and lettre dependency |
deny.toml |
Apache-2.0 WITH LLVM-exception license allowance for new dependencies |
cot-cli/src/project_template/ |
Default email configuration in project templates |
.github/workflows/rust.yml |
CI toolchain version updates |
.cargo/config.toml |
Windows linker configuration for faster builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
m4tx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a really nice piece of work! I'll need to have another look at the actual email logic in the PR, but this overall looks decent. Please make sure to have a look at the Copilot review as well - it has found a few nitpicks.
This makes the API more ergonomic, as `Arc` should really just be an implementation detail, rather than something exposed to the user. This is a followup to the discussion here: #419 (comment)
m4tx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the last batch of comments - overall, it's really, really good, but I still have a few concerns. Nothing fundamental, mostly nitpicks!
| /// .build()?; | ||
| /// email.send(message).await?; | ||
| /// # Ok(()) | ||
| /// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
| /// .build()?; | ||
| /// email.send_multiple(&[message1, message2]).await?; | ||
| /// # Ok(()) | ||
| /// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
| /// let console_transport = Console::new(); | ||
| /// ``` | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub struct Console; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add #[non_exhaustive] to the structure. I'm not sure if we'll ever add any configuration options to this transport, but if we do, we will be able to do this in an API-compatible way, unlike now.
| pub enum TransportError { | ||
| /// The underlying transport backend returned an error. | ||
| #[error("{ERROR_PREFIX} transport error: {0}")] | ||
| Backend(String), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we have Box<dyn StdError + Send + Sync + 'static> here so that we don't lose the original error? Is there any reason to only keep String here?
| type Error = EmailMessageError; | ||
|
|
||
| fn try_from(message: EmailMessage) -> Result<Self, Self::Error> { | ||
| let from_mailbox = message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we just add server_email: Option<String> to EmailConfig? What do you mean "there's no nice way to do that"?
| /// The default is `Plain`. | ||
| #[derive(Debug, Default, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "lowercase")] | ||
| pub enum Mechanism { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add #[non_exhaustive] here so that when we add new auth mechanisms, it won't be a breaking change.
|
|
||
| impl_into_cot_error!(EmailMessageError); | ||
|
|
||
| impl TryFrom<EmailMessage> for Message { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to smtp.rs and make it a separate function, rather than impl TryFrom<..> for Message? I ask for several reasons:
- This is the file that contains a public API we expose to the users; I don't think that code that's specific to any transport implementation (in this case,
smtp) belongs here. - This sneakily exposes
lettrein our public API. If we ever changelettreto any other library, or even bump thelettre's version, this will be a hidden breaking change for our users (even though probably nobody will actually try to use this).
If we ever add more transports supported by lettre (such as the sendmail transport), we can move this code to some shared module, but I still don't think this belongs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with this
| #[cfg(feature = "email")] | ||
| #[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(tag = "type", rename_all = "snake_case")] | ||
| pub enum EmailTransportTypeConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add #[non_exhaustive] here, as it's highly likely we'll add new transport types in the future.
Co-authored-by: Mateusz Maćkowski <mateusz@mackowski.org>
This is a continuation of #250