-
Notifications
You must be signed in to change notification settings - Fork 137
Add support for Mailtrap #406
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: main
Are you sure you want to change the base?
Conversation
8c44826
to
1ef637c
Compare
Thanks for this! It looks pretty good at first glance; I'll try to take a closer look later this week.
Missing inbound support is not unusual; it's fine to just ignore it. Reply-to is a little surprising. Some ESPs require handling this as an extra header. (I haven't had a chance to look at Mailtrap's docs.)
What's going wrong? (In the contributing docs, I notice the "test a representative combination of Python and Django versions" command is outdated—the current version should be |
Aha! I'm no email expert, so TIL! I will add the reply-to-via-headers support momentarily. |
Switching to a dev container, using pyenv to install 3.12 and 3.8, and using the updated tox command got further. I got a complaint that python 3.8 was missing, but creating a symlink /usr/bin/python3.8 to the pyenv installed version fixed that. I think I'm good to go for writing tests. Thanks. |
18307bc
to
ab47bbc
Compare
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.
@cahna this is looking really solid! I have a bunch of suggestions, some responses to your questions, and some questions of my own.
I haven't been through the webhook you just added yet; will take a look at that soon.
[GitHub sometimes hides review comments: please search the page for "Hidden Conversations" to be sure you see them all.]
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.
OK, the webhooks look really good, too. Handful of minor comments.
Thanks again for your work on this.
Thanks for your feedback @medmunds! Taking a look now. FYI: I only have Sundays available to work on this for the foreseeable future. |
Searching for "hidden" within the page showed nothing, so I think I saw everything. LMK if I missed something. |
I will start working on integration tests locally with my own free account. @medmunds, would you be able to setup a free mailtrap account, put the API key and test inbox ID into the repo's github secrets, and expose them to the build as environment variables? FYI: the free mailtrap tier allows 100 test emails/month. |
parsed_response = self.deserialize_json_response(response, payload, message) | ||
|
||
# TODO: how to handle fail_silently? | ||
if not self.fail_silently and ( |
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.
While trying to duplicate test coverage from the other backend tests, I found that the fail_silently test(s) don't work, and this doesn't seem to be the correct way to do this. How should I handle that? Should I?
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.
fail_silently should be handled for you in AnymailBaseBackend.send_messages(). The important part is that any errors raised by this function need to be one of the Anymail errors, not generic Python errors like KeyError.
It's helpful to distinguish a few different cases:
- If the API uses HTTP status codes to indicate an error, that's already been reported in AnymailRequestsBackend.raise_for_status() (so parse_recipients_status() won't be called). It looks to me like that's the case for all Mailtrap error responses.
- (If the API indicates errors by returning HTTP 2xx but with an "errors" field in the response, that needs to be reported as an AnymailRequestsAPIError with the message extracted from the response. This doesn't seem to apply to Mailtrap.)
- If the API indicated success, but the response isn't parseable (e.g., missing keys), that should be reported as an AnymailRequestsAPIError with a message describing the invalid response format. I'll add a separate review comment with a suggestion for that.
@medmunds I got started on backend unit tests, but I have left several |
Done: variables |
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.
Still looking great! And nice start on the docs.
Handful of feedback on the main implementation.
I glanced through the backend tests and tried to respond to your questions. This is the part of adding an ESP where we typically discover all of the little details they haven't documented, so some tests may need adjustment based on that. (The "test_all_options" integration test can be helpful for discovering those.)
(Apologies for not getting feedback on your earlier round sooner—I'm starting to get caught up in the end of the year rush.)
parsed_response = self.deserialize_json_response(response, payload, message) | ||
|
||
# TODO: how to handle fail_silently? | ||
if not self.fail_silently and ( |
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.
fail_silently should be handled for you in AnymailBaseBackend.send_messages(). The important part is that any errors raised by this function need to be one of the Anymail errors, not generic Python errors like KeyError.
It's helpful to distinguish a few different cases:
- If the API uses HTTP status codes to indicate an error, that's already been reported in AnymailRequestsBackend.raise_for_status() (so parse_recipients_status() won't be called). It looks to me like that's the case for all Mailtrap error responses.
- (If the API indicates errors by returning HTTP 2xx but with an "errors" field in the response, that needs to be reported as an AnymailRequestsAPIError with the message extracted from the response. This doesn't seem to apply to Mailtrap.)
- If the API indicated success, but the response isn't parseable (e.g., missing keys), that should be reported as an AnymailRequestsAPIError with a message describing the invalid response format. I'll add a separate review comment with a suggestion for that.
anymail/backends/mailtrap.py
Outdated
# TODO: how to handle fail_silently? | ||
if not self.fail_silently and ( | ||
not parsed_response.get("success") | ||
or ("errors" in parsed_response and parsed_response["errors"]) | ||
or ("message_ids" not in parsed_response) | ||
): | ||
raise AnymailRequestsAPIError( | ||
email_message=message, payload=payload, response=response, backend=self | ||
) | ||
else: | ||
# message-ids will be in this order | ||
recipient_status_order = [ | ||
*payload.recipients_to, | ||
*payload.recipients_cc, | ||
*payload.recipients_bcc, | ||
] | ||
recipient_status = { | ||
email: AnymailRecipientStatus( | ||
message_id=message_id, | ||
status="sent", | ||
) | ||
for email, message_id in zip( | ||
recipient_status_order, parsed_response["message_ids"] | ||
) | ||
} |
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.
Here's an approach for reporting invalid responses. It's also probably worth checking the number of message_ids, since zip() won't raise an error for that.
# TODO: how to handle fail_silently? | |
if not self.fail_silently and ( | |
not parsed_response.get("success") | |
or ("errors" in parsed_response and parsed_response["errors"]) | |
or ("message_ids" not in parsed_response) | |
): | |
raise AnymailRequestsAPIError( | |
email_message=message, payload=payload, response=response, backend=self | |
) | |
else: | |
# message-ids will be in this order | |
recipient_status_order = [ | |
*payload.recipients_to, | |
*payload.recipients_cc, | |
*payload.recipients_bcc, | |
] | |
recipient_status = { | |
email: AnymailRecipientStatus( | |
message_id=message_id, | |
status="sent", | |
) | |
for email, message_id in zip( | |
recipient_status_order, parsed_response["message_ids"] | |
) | |
} | |
try: | |
message_ids = parsed_response["message_ids"] | |
except KeyError as err: | |
raise AnymailRequestsAPIError( | |
"Invalid response format: missing message_ids", | |
email_message=message, payload=payload, response=response, backend=self | |
) from err | |
# message-ids will be in this order | |
recipient_status_order = [ | |
*payload.recipients_to, | |
*payload.recipients_cc, | |
*payload.recipients_bcc, | |
] | |
if len(recipient_status_order) != len(message_ids): | |
raise AnymailRequestsAPIError( | |
"Invalid response format: wrong number of message_ids", | |
email_message=message, payload=payload, response=response, backend=self | |
) | |
recipient_status = { | |
email: AnymailRecipientStatus( | |
message_id=message_id, | |
status="sent", | |
) | |
for email, message_id in zip(recipient_status_order, message_ids) | |
} |
API error responses should already be covered by raise_for_status() in the superclass. (But if you wanted to add a check like if parsed_response.get("errors") or not parsed_response.get("success"): raise AnymailRequestsAPIError("Unexpected errors in {response.status_code} response", ...)
, that wouldn't hurt anything.)
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.
From my integration testing, it seems that there is always exactly one message id returned in message_ids
. Can you find or think of any ways this would return more than one message_id?
I have updated the code to expect only one message id in the response and associated it with all addresses.
tests/test_mailtrap_backend.py
Outdated
@unittest.skip("TODO: is this test correct/necessary?") | ||
def test_send_with_recipients_refused(self): | ||
"""Test sending an email with all recipients refused""" |
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.
This is probably unnecessary, since Mailtrap doesn't seem to provide per-recipient send status.
Mailtrap's error documentation isn't all that descriptive, so we might need to test this. If you try to send a message with two recipients, both on your suppression list (or previous bounces), what happens? (Send API success and later webhook reject? Or send API error?)
Similarly, if you send with one valid recipient and one suppressed recipient, what happens?
(I can do some investigation here if that would be helpful.)
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 have removed this test, but I have not investigated/tested the cases you mentioned.
mail.send_mail("Subject", "Message", "[email protected]", ["[email protected]"]) | ||
errmsg = str(cm.exception) | ||
self.assertRegex(errmsg, r"\bMAILTRAP_API_TOKEN\b") | ||
self.assertRegex(errmsg, r"\bANYMAIL_MAILTRAP_API_TOKEN\b") |
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.
Probably also need to test various combinations of settings, both valid and error cases (api_url, testing_enabled, test_inbox_id), to make sure the right api is called or a helpful error is raised.
Hiya folks! Is there I could help with to help get this ready? |
@cahna did you still want to work on this? If not, I'll try to finish it up. (Though realistically, probably not until sometime next month.) @adamwolf thanks for the offer. There are a couple of things that would be helpful:
|
My apologies for ghosting. Work and life has been very busy. Good news is that this has been running in a production environment for 3+ months without any issues (nearing 2 million emails). I want to finish up the integration tests and outstanding comments, but i can't commit to a deadline at this point. |
I have a question. For this integration test case: def test_all_options(self):
message = AnymailMessage(
subject="Anymail Mailtrap all-options integration test",
body="This is the text body",
from_email=formataddr(("Test From, with comma", self.from_email)),
to=[
"[email protected]",
'Recipient 2 <[email protected]>',
],
cc=["[email protected]", "Copy 2 <[email protected]>"],
bcc=["[email protected]", "Blind Copy 2 <[email protected]>"],
reply_to=[
'"Reply, with comma" <[email protected]>',
"[email protected]",
],
headers={"X-Anymail-Test": "value", "X-Anymail-Count": "3"},
metadata={"meta1": "simple string", "meta2": 2},
# Mailtrap supports only a single tag/category
tags=["tag 1"],
track_clicks=True,
track_opens=True,
)
message.attach("attachment1.txt", "Here is some\ntext for you", "text/plain")
message.attach("attachment2.csv", "ID,Name\n1,Amy Lina", "text/csv")
cid = message.attach_inline_image_file(sample_image_path())
message.attach_alternative(
"<p><b>HTML:</b> with <a href='http://example.com'>link</a>"
f"and image: <img src='cid:{cid}'></div>",
"text/html",
)
message.send()
self.assertEqual(message.anymail_status.status, {"sent"})
self.assertEqual(
message.anymail_status.recipients["[email protected]"].status, "sent"
)
self.assertEqual(
message.anymail_status.recipients["[email protected]"].status, "sent"
) Mailtrap's sandbox receives this raw email:
The {'success': True, 'message_ids': ['5038769279']} I suppose this means that all of the |
For the integration tests of templates, would you please create a template in your mailtrap account? Needed steps: |
0a76d0a
to
bc1fd1d
Compare
My conscience continues to make me feel bad about not finishing this, so I have added minimal integration tests, added fixes for some of the PR comments, squashed the commits, and rebased on latest |
@cahna thanks! I re-rebased to the latest main, so you should pull before any other changes.
That sounds correct: if there's only a single message id, it applies to all recipients. I'll try to do some live testing to confirm. (The main goal is have an ID that could be matched with a later status webhook call somehow.)
That's an interesting pricing differentiator. (Maybe bcc was getting used by spammers? I think there's another ESP who limits bcc recipients to validated domains for that very reason.) We can just omit bcc from the integration test. I'd be inclined to label Mailtrap as "Full" support in the feature matrix: we're able to perform integration testing (on everything except bcc) and the 3500 messages/month on the free plan will way more than cover Anymail's test sending. |
Hmm, If I send to two "to" and two "cc" recipients on a production domain ( {
"success": true,
"message_ids": [
"ed1edba0-795c-11f0-0000-f14311a5578e",
"ed1fc600-795c-11f0-0000-f14311a5578e",
"ed20fe80-795c-11f0-0000-f14311a5578e",
"ed21c1d0-795c-11f0-0000-f14311a5578e"
]
} But on a sandbox domain ( {
"success": true,
"message_ids": [
"5040626968"
]
} |
@medmunds Just a heads-up that the "sandbox" is only limited to 100/month |
Hmm... strange behavior. What do you think would be best to do here? If I have only been using Anymail to send single-recipient emails, so this is not something I had encountered, yet. |
Add support for Mailtrap