Add support for Mailtrap #406

cahna · 2024-11-10T23:26:23Z

Hi, and thanks for creating such a great library! I would love to have Mailtrap support, and I figured it would be helpful if I started on that. This is very much a work-in-progress, as I'm still trying to understand how to handle all of the features that Anymail provides, and whether those features are supported by Mailtrap. I have already caught at least one category of features that Mailtrap does not support: inbound emails ~~and reply-to~~.

If anyone would be willing to provide some feedback on what I have, and any guidance on how to continue, I would appreciate it, and I will be sure to add tests and documentation.

I am also having trouble getting local builds and tests to work, or I would have started on adding tests. If someone could point me in the direction of minimal steps to get tests working on Ubuntu, I would appreciate it. (The contributing docs don't seem to work for me).

Cheers

TODO

anymail/backends/mailtrap.py

cahna · 2024-11-10T23:31:49Z

anymail/backends/mailtrap.py

+            )
+
+        # Not the best status reporting. Mailtrap only says that the order of message-ids will be in this order (but JSON is unordered?)
+        recipient_status_order = [*message.to, *message.cc, *message.bcc]


Mailtrap's status reporting is pretty poor, and I'm not even sure if this will work consistently, given that it is depending on ORDERED JSON responses 🤦

In the response:

message_ids array[string]
An array of message IDs, one per recipient, in order of To, Cc, Bcc

JSON arrays are reliably ordered, just like Python lists. But, it's not safe to use the original message's to/cc/bcc here, as they are formatted addresses that might include display names (and as some Anymail features can alter recipient lists before sending).

Take a look at any of the backends/payloads that use an all_recipients list for an example of how to handle this (e.g., MailgunPayload.set_recipients() and MailgunBackend.parse_recipient_status()); your payload might need to keep the to/cc/bcc recipients separate to ensure they end up in the correct order.

Will do. Do you think there would be any value in sorting the recipients by email address before sending?

I would not try to sort recipients. I don't think it would help anything, and there might be cases where senders are deliberately putting addresses in a specific order.

medmunds · 2024-11-12T02:33:05Z

Thanks for this! It looks pretty good at first glance; I'll try to take a closer look later this week.

Mailtrap does not support: inbound emails and reply-to.

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.)

I am also having trouble getting local builds and tests to work

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 tox -e lint,django51-py312-all,django40-py38-all,docs. But other than that I'd expect it to work. I'm usually on macOS, but the GitHub tests all run on Ubuntu.)

cahna · 2024-11-17T20:20:32Z

@medmunds

Reply-to is a little surprising. Some ESPs require handling this as an extra header.

Aha! I'm no email expert, so TIL! I will add the reply-to-via-headers support momentarily.

cahna · 2024-11-17T20:59:02Z

@medmunds

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 tox -e lint,django51-py312-all,django40-py38-all,docs. But other than that I'd expect it to work. I'm usually on macOS, but the GitHub tests all run on Ubuntu.)

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.

medmunds

@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.]

medmunds · 2024-11-18T00:03:52Z

pyproject.toml

@@ -63,6 +64,7 @@ dependencies = [
    "django>=4.0",
    "requests>=2.4.3",
    "urllib3>=1.25.0",  # requests dependency: fixes RFC 7578 header encoding
+    "typing_extensions>=4.12",  # for older Python compatibility


I'm a little concerned about adding a new dependency for all users. I wonder if some combination of from __future__ import annotations and if typing.TYPE_CHECKING could let typing_extensions be an optional dependency?

(On the other hand, Anymail is moving toward typing at some point in #394, so maybe this is fine?)

Since Anymail already requires Python 3.8, it looks like typing_extensions is only needed for TypedDict (3.9+) and NotRequired (3.11+), so at the very least we could probably make this a version-dependent dependency: "typing_extensions>=4.12; python_version < 3.11".

I'm happy to implement whatever you request. I have started to rely heavily on Python type checking for my own development, but if you want me to remove all type hints once the work part is done, I'm happy to do that, too.

I confirm your assessment that it has only been added for TypedDict and NotRequired support in versions <3.11.

at the very least we could probably make this a version-dependent dependency: "typing_extensions>=4.12; python_version < 3.11"

I will do this for now.

I'm not sure what the best practice is for type hints in libs that support multiple python versions, but my intuition tells me there has to be a better way than this(?):

if sys.version_info < (3, 11): from typing_extensions import Any, Dict, List, Literal, NotRequired, TypedDict else: from typing import Any, Dict, List, Literal, NotRequired, TypedDict

I suppose if #394 is coming, then maybe it is worth making typing_extensions a requirement? That way all of the type imports can come from typing_extensions instead of typing:

from typing_extensions import Any, Dict, List, Literal, NotRequired, TypedDict

I will do some more research on this.

anymail/backends/mailtrap.py

medmunds · 2024-11-18T00:36:38Z

anymail/backends/mailtrap.py

+            )
+
+        # Not the best status reporting. Mailtrap only says that the order of message-ids will be in this order (but JSON is unordered?)
+        recipient_status_order = [*message.to, *message.cc, *message.bcc]


In the response:

message_ids array[string]
An array of message IDs, one per recipient, in order of To, Cc, Bcc

JSON arrays are reliably ordered, just like Python lists. But, it's not safe to use the original message's to/cc/bcc here, as they are formatted addresses that might include display names (and as some Anymail features can alter recipient lists before sending).

Take a look at any of the backends/payloads that use an all_recipients list for an example of how to handle this (e.g., MailgunPayload.set_recipients() and MailgunBackend.parse_recipient_status()); your payload might need to keep the to/cc/bcc recipients separate to ensure they end up in the correct order.

anymail/backends/mailtrap.py

medmunds

OK, the webhooks look really good, too. Handful of minor comments.

Thanks again for your work on this.

anymail/backends/mailtrap.py

anymail/webhooks/mailtrap.py

tests/test_mailtrap_webhooks.py

cahna · 2024-11-24T20:37:12Z

Thanks for your feedback @medmunds! Taking a look now.

FYI: I only have Sundays available to work on this for the foreseeable future.

Co-authored-by: Mike Edmunds <medmunds@gmail.com>

cahna · 2024-11-24T21:31:07Z

Searching for "hidden" within the page showed nothing, so I think I saw everything. LMK if I missed something.

cahna · 2024-11-24T21:54:07Z

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.

cahna · 2024-11-30T20:16:02Z

anymail/backends/mailtrap.py

+        parsed_response = self.deserialize_json_response(response, payload, message)
+
+        # TODO: how to handle fail_silently?
+        if not self.fail_silently and (


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?

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.

cahna · 2024-11-30T20:17:56Z

@medmunds I got started on backend unit tests, but I have left several @unittest.skip("TODO: ...") on tests that either might not be relevant for Mailtrap, or that have outstanding questions on how to adjust the backend code to make the test pass.

medmunds · 2024-11-30T23:50:09Z

@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?

Done: variables ANYMAIL_TEST_MAILTRAP_DOMAIN and ANYMAIL_TEST_MAILTRAP_TEST_INBOX_ID and secret ANYMAIL_TEST_MAILTRAP_API_TOKEN are installed. (Secrets aren't available to PR builds, so the integration tests won't actually run until it's merged into main.)

medmunds

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.)

medmunds · 2024-12-01T00:00:04Z

.github/workflows/integration-test.yml

@@ -44,6 +44,7 @@ jobs:
          - { tox: django41-py310-mailersend, python: "3.12" }
          - { tox: django41-py310-mailgun, python: "3.12" }
          - { tox: django41-py310-mailjet, python: "3.12" }
+          - { tox: django41-py310-mailtrap, python: "3.12" }


You'll also need to add vars.ANYMAIL_TEST_MAILTRAP_DOMAIN, vars. ANYMAIL_TEST_MAILTRAP_TEST_INBOX_ID and secrets. ANYMAIL_TEST_MAILTRAP_API_TOKEN in the big env: section below. (Around line 91; GitHub won't let me add a review comment down there.)

medmunds · 2024-12-01T00:11:28Z

anymail/backends/mailtrap.py

+        self.recipients_to: List[str] = []
+        self.recipients_cc: List[str] = []
+        self.recipients_bcc: List[str] = []


I might instead use something like:

Suggested change

self.recipients_to: List[str] = []

self.recipients_cc: List[str] = []

self.recipients_bcc: List[str] = []

self.recipients: Dict[Literal["to", "cc", "bcc"], List[EmailAddress]] = {"to": [], "cc": [], "bcc": []}

just to simplify the code later in set_recipients() to: self.recipients[recipient_type] = emails. (And then grab the addr_specs in parse_recipients_status().) But what you have now will work, so feel free to ignore this suggestion.

medmunds · 2024-12-01T00:21:16Z

anymail/backends/mailtrap.py

+    def set_track_clicks(self, *args, **kwargs):
+        """Do nothing. Mailtrap supports this, but it is not configured in the send request."""
+        pass
+
+    def set_track_opens(self, *args, **kwargs):
+        """Do nothing. Mailtrap supports this, but it is not configured in the send request."""
+        pass
+


This is (somewhat confusingly) not the correct way to handle this. Anymail's track_clicks and track_opens are meant to override the account defaults for message tracking: e.g., to disable link rewriting on password reset emails, or all tracking for mail to a particular user.

If Mailtrap doesn't support per-message tracking overrides, then these options are indeed "unsupported" and should raise errors if used. (You've correctly labeled them No [#nocontrol]_ in the big feature matrix in the docs.)

Suggested change

def set_track_clicks(self, *args, **kwargs):

"""Do nothing. Mailtrap supports this, but it is not configured in the send request."""

pass

def set_track_opens(self, *args, **kwargs):

"""Do nothing. Mailtrap supports this, but it is not configured in the send request."""

pass

medmunds · 2024-12-01T00:41:17Z

anymail/backends/mailtrap.py

+        parsed_response = self.deserialize_json_response(response, payload, message)
+
+        # TODO: how to handle fail_silently?
+        if not self.fail_silently and (


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 · 2024-12-01T01:00:38Z

anymail/backends/mailtrap.py

+        # 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"]
+                )
+            }


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.

Suggested change

# 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.)

medmunds · 2024-12-01T01:12:16Z

tests/test_mailtrap_backend.py

@@ -0,0 +1,299 @@
+# FILE: tests/test_mailtrap_backend.py


??? (is this some editor convention?)

medmunds · 2024-12-01T01:14:49Z

tests/test_mailtrap_backend.py

+    @unittest.skip("TODO: is this test correct/necessary?")
+    def test_send_with_recipients_refused(self):
+        """Test sending an email with all recipients refused"""


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.)

medmunds · 2024-12-01T01:22:51Z

tests/test_mailtrap_backend.py

+            mail.send_mail("Subject", "Message", "from@example.com", ["to@example.com"])
+        errmsg = str(cm.exception)
+        self.assertRegex(errmsg, r"\bMAILTRAP_API_TOKEN\b")
+        self.assertRegex(errmsg, r"\bANYMAIL_MAILTRAP_API_TOKEN\b")


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.

cahna commented Nov 10, 2024

View reviewed changes

anymail/backends/mailtrap.py Outdated Show resolved Hide resolved

cahna commented Nov 10, 2024

View reviewed changes

cahna force-pushed the mailtrap branch 5 times, most recently from 8c44826 to 1ef637c Compare November 10, 2024 23:55

cahna mentioned this pull request Nov 11, 2024

Feature request: Mailtrap #345

Open

medmunds added new ESP esp:Mailtrap labels Nov 12, 2024

medmunds marked this pull request as draft November 12, 2024 02:34

cahna force-pushed the mailtrap branch 2 times, most recently from 18307bc to ab47bbc Compare November 17, 2024 22:28

medmunds requested changes Nov 18, 2024

View reviewed changes

cahna and others added 11 commits November 24, 2024 13:23

Add support for Mailtrap

68832b4

Fix typing errors in older python builds

73b9145

Mailtrap: implement MailtrapPayload.set_reply_to

1c59a04

Mailtrap: add webhook tests and update test configs

fba5d23

Mailtrap backend: remove user-agent header

a9ae0d3

Co-authored-by: Mike Edmunds <medmunds@gmail.com>

Mailtrap backend: quote user-provided url parts

d2cc770

Mailtrap backend: reply-to headers suggestions

80224ae

Co-authored-by: Mike Edmunds <medmunds@gmail.com>

Mailtrap backend: remove set_merge_data

d04ed7d

Mailtrap backend: add type hint to set_merge_global_data

c4bf5d1

Mailtrap backend: fix template_id to template_uuid

957a0de

Co-authored-by: Mike Edmunds <medmunds@gmail.com>

Mailtrap backend: make test_inbox_id required if testing_enabled

bac1399

Co-authored-by: Mike Edmunds <medmunds@gmail.com>

cahna and others added 5 commits November 24, 2024 13:23

Mailtrap backend: add set_tags

dfd2de9

Mailtrap webhook: fix for empty category/tag

27d62a5

Co-authored-by: Mike Edmunds <medmunds@gmail.com>

Mailtrap webhook: remove None defaults

43258d9

Mailtrap webhook: fix incorrect reject_reason default

1da6971

Co-authored-by: Mike Edmunds <medmunds@gmail.com>

Mailtrap webhook tests: remove unnecessary json.dumps

7597448

cahna force-pushed the mailtrap branch from f2573bd to 7597448 Compare November 24, 2024 21:23

cahna added 2 commits November 24, 2024 21:24

Mailtrap: only depend on typing_extensions for py<3.11

f5c6302

Mailtrap backend: remove bulk_api_url

7f253b4

cahna added 2 commits November 24, 2024 21:35

Mailtrap: fix dependency version string

747d4bb

Mailtrap backend: update payload to keep track of recipient ordering

c5f9140

Mailtrap: add backend unit tests, set_track_clicks, and set_track_opens

f80dc55

cahna commented Nov 30, 2024

View reviewed changes

cahna requested a review from medmunds November 30, 2024 20:18

cahna added 2 commits November 30, 2024 20:53

Mailtrap: begin work on docs page

cc96be5

Mailtrap: update feature matrix

9162fc0

medmunds requested changes Dec 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Mailtrap #406

Add support for Mailtrap #406

cahna commented Nov 10, 2024 •

edited

Loading

cahna Nov 10, 2024

medmunds Nov 18, 2024

cahna Nov 24, 2024

medmunds Nov 30, 2024

medmunds commented Nov 12, 2024

cahna commented Nov 17, 2024

cahna commented Nov 17, 2024

medmunds left a comment

medmunds Nov 18, 2024

cahna Nov 24, 2024

cahna Nov 24, 2024 •

edited

Loading

medmunds Nov 18, 2024

medmunds left a comment

cahna commented Nov 24, 2024

cahna commented Nov 24, 2024

cahna commented Nov 24, 2024

cahna Nov 30, 2024

medmunds Dec 1, 2024

cahna commented Nov 30, 2024

medmunds commented Nov 30, 2024

medmunds left a comment

medmunds Dec 1, 2024

medmunds Dec 1, 2024

medmunds Dec 1, 2024

medmunds Dec 1, 2024

medmunds Dec 1, 2024

medmunds Dec 1, 2024

medmunds Dec 1, 2024

medmunds Dec 1, 2024

Add support for Mailtrap #406

Are you sure you want to change the base?

Add support for Mailtrap #406

Conversation

cahna commented Nov 10, 2024 • edited Loading

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medmunds commented Nov 12, 2024

cahna commented Nov 17, 2024

cahna commented Nov 17, 2024

medmunds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahna Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medmunds left a comment

Choose a reason for hiding this comment

cahna commented Nov 24, 2024

cahna commented Nov 24, 2024

cahna commented Nov 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahna commented Nov 30, 2024

medmunds commented Nov 30, 2024

medmunds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahna commented Nov 10, 2024 •

edited

Loading

cahna Nov 24, 2024 •

edited

Loading