Skip to content

Conversation

@trkelly23
Copy link

@trkelly23 trkelly23 commented Mar 20, 2025

This pull request relates #184 email sending functionality using the SMTP protocol. The implementation includes the following key components:

EmailError Enum: Defines various errors that can occur when sending an email, such as message errors, configuration errors, connection errors, and send errors.

SmtpConfig Struct: Represents the configuration for the SMTP email backend. It includes fields for the SMTP server host, port, username, password, fail silently option, and timeout duration. A default implementation is provided.

EmailMessage Struct: Represents an email message with fields for the subject, body, sender email, recipient emails, CC, BCC, reply-to addresses, and alternative parts (e.g., plain text and HTML versions).

EmailBackend Struct: Provides the core functionality for sending emails.

It includes methods to:
Create a new instance with the given configuration.
Open a connection to the SMTP server.
Close the connection to the SMTP server.
Dump the email message to stdout for debugging purposes.
Send a single email message.
Send multiple email messages.
Unit Tests:

Notes
The implementation uses the lettre crate for SMTP transport and message building.
The EmailBackend struct includes a debug flag to enable dumping email messages to stdout for debugging purposes.
The unit tests demonstrate the setup but do not actually send emails, as they mock the transport. In a real test environment, you might use a real SMTP server or a more sophisticated mock.

Future Work
Integration tests with a real SMTP server or a mock server.
Additional configuration options for TLS/SSL support.
Improved error handling and logging.
Integration with API email providers.

@github-actions github-actions bot added the C-lib Crate: cot (main library crate) label Mar 20, 2025
pre-commit-ci bot and others added 4 commits March 20, 2025 00:02
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).
@codecov
Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 85.81688% with 79 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cot/src/email.rs 85.81% 57 Missing and 22 partials ⚠️
Flag Coverage Δ
rust 81.86% <85.81%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cot/src/email.rs 85.81% <85.81%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@seqre seqre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution and interest in Cot! I have some questions and made some suggestions for improvement, I'd appreciate it if you acted on them.

Additionally, it'd be great if you could write more tests to ensure sufficient code coverage!

@trkelly23 trkelly23 requested a review from seqre March 28, 2025 14:46
@m4tx m4tx changed the title (feat): add email support using lettre feat: email support using lettre Mar 30, 2025
Copy link
Member

@seqre seqre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvement, but there's still some work to do!

Comment on lines +394 to +398
let transport = if self.config.username.is_some() && self.config.password.is_some() {
transport_builder.credentials(credentials).build()
} else {
transport_builder.build()
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already did that check above when creating Credentials, no need to duplicate it I think

Suggested change
let transport = if self.config.username.is_some() && self.config.password.is_some() {
transport_builder.credentials(credentials).build()
} else {
transport_builder.build()
};
let transport = transport_builder.credentials(credentials).build();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still current

Copy link
Member

@m4tx m4tx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks fairly good and certainly goes in the right direction! There's still one major issue that needs to be fixed before merging, however.

@trkelly23
Copy link
Author

@seqre I just noticed your comment:
I'm back after a focused effort at work which may have gotten me into a little trouble getting the bootstrapping working with the email configuration. I added the email_backend to the config.rs and project.rs files. However, using it in example/send-email yields no backend initialized.

I think the issue is in project.rs and the bootstrapping timing between WithApps, WithDatabase, and WithEmail.
Could anybody who knows the Project initialization give me any tips or suggestions?

@seqre
Copy link
Member

seqre commented May 15, 2025

Thank you for your patience, @trkelly23! We looked at the code and found out you have just forgotten to call the with_email function that you created. We took the liberty of fixing it and reorganizing slightly to accommodate possible similar additions in the future. We also merged in changes from the newest version, so you don't have to fight with merge conflicts. Now you can continue working on this feature!

As a side note, you might want to install a pre-commit hook, which will ensure your code always follows our formatting standards! Check out our contributing guide for more details!

Additionally, please feel empowered to resolve conversations in PR with comments that you fully applied to the code. Leave open only those where you're unsure or we're actively discussing something. This reduces the amount of work we have to do, thanks!

@trkelly23
Copy link
Author

Thanks for the update and for taking care of the reorganization and merge — much appreciated!

I took a look, but I wasn’t able to find where you inserted or put the with_email function in the codebase. It’s possible I’m missing something — could you point me to where it’s defined or committed? Just want to make sure I’m aligned before continuing.

Thanks again!

@seqre
Copy link
Member

seqre commented May 26, 2025

I took a look, but I wasn’t able to find where you inserted or put the with_email function in the codebase. It’s possible I’m missing something — could you point me to where it’s defined or committed? Just want to make sure I’m aligned before continuing.

Here: https://github.com/cot-rs/cot/pull/250/files#diff-dce3fcfc71d929a6396924afc5e65382abcb0b4587cf05f2ff816844829976e1R1241-R1270

@trkelly23
Copy link
Author

@seqre Thanks for the link. I think I resolved many/all the review comments and completed a working example for others to learn from.
I think one more review is required and maybe some more changes if suggested.

name = "send-email"
version = "0.1.0"
publish = false
description = "Send email - Cot example."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description = "Send email - Cot example."
description = "Send email - Cot example."
license = "MIT OR Apache-2.0"

/// The SMTP server host address.
/// Defaults to "localhost".
#[builder(setter(into, strip_option), default)]
pub smtp_mode: email::SmtpTransportMode,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests to ensure all variants of this enum can be constructed from TOML.

Comment on lines +394 to +398
let transport = if self.config.username.is_some() && self.config.password.is_some() {
transport_builder.credentials(credentials).build()
} else {
transport_builder.build()
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still current

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-lib Crate: cot (main library crate)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants