Add support for testing emails #3483
Conversation
Instead of having public functions dedicated to sending emails, this commit switches to a Emails struct stored in the App. The struct holds the backend configuration and state, and has methods for each email the application can send. Another use of the new struct is during tests: tests now use a new "memory" backend, which stores all sent messages in a Mutex<Vec<_>>. This allows tests to retrieve the mails sent by the code and run assertions on them. Finally, the send_* and try_send_* distinction was removed: all the send_* methods now return an error, and it's the caller's job to swallow the errors if they don't want to fail requests due to it.
Turbo87
added
C-internal 🔧
| &email, | ||
| &req_user.gh_login, | ||
| &self.name, | ||
| &ownership_invitation.token, |
There was a problem hiding this comment.
would it make sense for us to at least log such errors?
| @@ -97,7 +99,8 @@ impl<'a> NewUser<'a> { | |||
| .optional()?; | |||
|
|
|||
| if let Some(token) = token { | |||
| crate::email::send_user_confirm_email(user_email, &user.gh_login, &token); | |||
| // Swallows any error. Some users might insert an invalid email address here. | |||
| let _ = emails.send_user_confirm(user_email, &user.gh_login, &token); | |||
There was a problem hiding this comment.
from a layering perspective it feels a bit wrong to me to have email sending code in the database model struct, but I guess refactoring this is out of scope for this PR
| FileSystem { path: PathBuf }, | ||
| /// Backend used during tests, will keep messages in memory to allow tests to retrieve them. | ||
| Memory { mails: Mutex<Vec<StoredEmail>> }, | ||
| } |
There was a problem hiding this comment.
what we're building here is essentially an interface with three backing implementations. I'm curious why you chose to implement that with an enum. would something like Box<dyn EmailTrait> work too?
There was a problem hiding this comment.
With few variants I personally prefer enums rather than a Box<dyn T>, but either could work just fine.
|
@bors r+ |
|
📌 Commit 57cf070 has been approved by |
|
☀️ Test successful - checks-actions |
Before this PR there wasn't really a practical way to test what the content of an email is, which makes it harder for example to test that the token included in the ownership confirmation email works with the relevant APIs.
The main thing this PR does is refactoring the email handling code, moving from standalone functions to an
Emailsstruct (an instance of it is stored in theApp). This allows to configure which email backend is used at startup instead of automatically detecting it every time we happen to send an email address. It will also allow to persist other email-related information when we'll need to.Thanks to the new email backends support this PR also adds a "memory" backend, used during tests. Instead of sending the email via SMTP or storing it into a file, the "memory" backend stores it in a
Mutex<Vec<_>>which can be inspected by any test.Finally, to test the new email testing capabilities this PR adds a test to ensure a user can accept an ownership invitation with the token included in the email.
This PR is best reviewed commit-by-commit.
r? @jtgeibel or @Turbo87