Skip to content
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

MailPace: New ESP #355

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
77f9530
Began the backend for MailPace, passing basic test
paul-oms Nov 4, 2023
b4811a8
Added error handling for MP backend with tests passing
paul-oms Nov 4, 2023
9564cbd
Refactor and 100% test coverage of MP Backend
paul-oms Nov 5, 2023
4a8a643
Remove unnecessary imports
paul-oms Nov 5, 2023
a530780
Added MailPace Webhooks
paul-oms Nov 5, 2023
f38f69d
Inbound webhooks, with test coverage
paul-oms Nov 5, 2023
113dc19
Enhacements to inbound webhooks
paul-oms Nov 6, 2023
1a91f0a
Support MailPace Webhook signature validation
paul-oms Nov 12, 2023
217d185
Started the MailPace Documentation
paul-oms Feb 1, 2024
0077419
Fix table
paul-oms Feb 1, 2024
afc7874
Fix docs table and inbound docs
paul-oms Feb 1, 2024
62930f6
Fix linting issues
paul-oms Feb 1, 2024
5158f99
Make pynacl optional
paul-oms Feb 1, 2024
e659634
Make pynacl optional in the mailpace test utils
paul-oms Feb 1, 2024
f8d1d95
Fix undefined LazyError and AnymailImproperlyInstalled
paul-oms Feb 1, 2024
82695fb
Fix import order
paul-oms Feb 1, 2024
662321b
Fix Lazy Error in test utils, make the Webhook tests use the CLient w…
paul-oms Feb 2, 2024
f627a5e
Skip webhook tests if pynacl is unavailable, support Django versions …
paul-oms Feb 4, 2024
3d2183e
Revert skipping validation if webhook key not set
paul-oms Feb 4, 2024
610257a
Fix failing tests, due to unset webhook key, correct warning message
paul-oms Feb 4, 2024
eab63d3
Warn if basic auth is not on, and signature validation is not setup
paul-oms Feb 6, 2024
de7f9a0
PR fededback: Refactor /send endpoint, default to unknown status, rem…
paul-oms Feb 7, 2024
8402107
Fix lint
paul-oms Feb 7, 2024
8e53167
Add integration tests
medmunds Feb 7, 2024
b03feb9
Update esp-feature-matrix in docs
medmunds Feb 20, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/integration-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ jobs:
- { tox: django41-py310-mailersend, python: "3.10" }
- { tox: django41-py310-mailgun, python: "3.10" }
- { tox: django41-py310-mailjet, python: "3.10" }
- { tox: django41-py310-mailpace, python: "3.10" }
- { tox: django41-py310-mandrill, python: "3.10" }
- { tox: django41-py310-postal, python: "3.10" }
- { tox: django41-py310-postmark, python: "3.10" }
Expand Down Expand Up @@ -83,6 +84,8 @@ jobs:
ANYMAIL_TEST_MAILJET_API_KEY: ${{ secrets.ANYMAIL_TEST_MAILJET_API_KEY }}
ANYMAIL_TEST_MAILJET_DOMAIN: ${{ secrets.ANYMAIL_TEST_MAILJET_DOMAIN }}
ANYMAIL_TEST_MAILJET_SECRET_KEY: ${{ secrets.ANYMAIL_TEST_MAILJET_SECRET_KEY }}
ANYMAIL_TEST_MAILPACE_DOMAIN: ${{ secrets.ANYMAIL_TEST_MAILPACE_DOMAIN }}
ANYMAIL_TEST_MAILPACE_SERVER_TOKEN: ${{ secrets.ANYMAIL_TEST_MAILPACE_SERVER_TOKEN }}
ANYMAIL_TEST_MANDRILL_API_KEY: ${{ secrets.ANYMAIL_TEST_MANDRILL_API_KEY }}
ANYMAIL_TEST_MANDRILL_DOMAIN: ${{ secrets.ANYMAIL_TEST_MANDRILL_DOMAIN }}
ANYMAIL_TEST_POSTMARK_DOMAIN: ${{ secrets.ANYMAIL_TEST_POSTMARK_DOMAIN }}
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ Features

* **Brevo:** Add support for batch sending
(`docs <https://anymail.dev/en/latest/esps/brevo/#batch-sending-merge-and-esp-templates>`__).
* **MailPace**: Add support for this ESP
(`docs <https://anymail.dev/en/stable/esps/mailpace/>`__).
* **Resend:** Add support for batch sending
(`docs <https://anymail.dev/en/latest/esps/resend/#batch-sending-merge-and-esp-templates>`__).

Expand Down
1 change: 1 addition & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Anymail currently supports these ESPs:
* **MailerSend**
* **Mailgun**
* **Mailjet**
* **MailPace**
* **Mandrill** (MailChimp transactional)
* **Postal** (self-hosted ESP)
* **Postmark**
Expand Down
202 changes: 202 additions & 0 deletions anymail/backends/mailpace.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
from ..exceptions import AnymailRequestsAPIError
from ..message import AnymailRecipientStatus
from ..utils import CaseInsensitiveCasePreservingDict, get_anymail_setting
from .base_requests import AnymailRequestsBackend, RequestsPayload


class EmailBackend(AnymailRequestsBackend):
"""
MailPace API Email Backend
"""

esp_name = "MailPace"

def __init__(self, **kwargs):
"""Init options from Django settings"""
esp_name = self.esp_name
self.server_token = get_anymail_setting(
"server_token", esp_name=esp_name, kwargs=kwargs, allow_bare=True
)
api_url = get_anymail_setting(
"api_url",
esp_name=esp_name,
kwargs=kwargs,
default="https://app.mailpace.com/api/v1/",
)
if not api_url.endswith("/"):
api_url += "/"
super().__init__(api_url, **kwargs)

def build_message_payload(self, message, defaults):
return MailPacePayload(message, defaults, self)

def raise_for_status(self, response, payload, message):
# We need to handle 400 responses in parse_recipient_status
if response.status_code != 400:
super().raise_for_status(response, payload, message)

def parse_recipient_status(self, response, payload, message):
# Prepare the dict by setting everything to queued without a message id
unknown_status = AnymailRecipientStatus(message_id=None, status="unknown")
recipient_status = CaseInsensitiveCasePreservingDict(
{recip.addr_spec: unknown_status for recip in payload.to_cc_and_bcc_emails}
)

parsed_response = self.deserialize_json_response(response, payload, message)

status_code = str(response.status_code)
json_response = response.json()

# Set the status_msg and id based on the status_code
if status_code == "200":
try:
status_msg = parsed_response["status"]
id = parsed_response["id"]
except (KeyError, TypeError) as err:
raise AnymailRequestsAPIError(
"Invalid MailPace API response format",
email_message=None,
payload=payload,
response=response,
backend=self,
) from err
elif status_code.startswith("4"):
status_msg = "error"
id = None

if status_msg == "queued":
# Add the message_id to all of the recipients
for recip in payload.to_cc_and_bcc_emails:
recipient_status[recip.addr_spec] = AnymailRecipientStatus(
message_id=id, status="queued"
)
elif status_msg == "error":
if "errors" in json_response:
for field in ["to", "cc", "bcc"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by the logic in this section.

The goal is to provide a specific send status for each recipient, if known.

I think this code will end up setting the sent status for every recipient based on the last reported error in the response (for any recipient). So if all "to" addresses were valid, but one of the cc's was on the blocked list, all of the "to" and the "cc" recipients would get reported as "blocked".

(Are there any cases where MailPace would send the message to some recipients, but not to others? Or if there's a problem with any to/cc/bcc recipient, is the entire message always rejected?)

Copy link
Author

Choose a reason for hiding this comment

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

It's the latter - if there's a problem with any recipient (e.g. blocked or invalid) the email will be rejected (error http status from the endpoint) and not sent, as the entire email is technically not valid in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks, that helps. I also did a little testing. [Sorry, this comment is kind of long—there are a bunch of cases to consider.]

The goal of parse_recipient_status is: based on the API response, for each individual "to" recipient, provide the send status if we know it, or "failed" if the message definitely wasn't sent to (queued for) that recipient but we can't exactly say why, or "unknown" if we're not sure whether or not the message was sent to that particular recipient. (This is especially important with some other ESPs that don't fail the entire message, but instead send to some recipients but not others.)

If that status information is also available for "cc" and "bcc" recipients, it's great to provide it. Otherwise, it's OK to just omit cc and bcc recipients.

In my testing, it looks like MailPace's API response currently reports:

  • {"errors":{"to":["is invalid"]}} if at least one (but not necessarily all) of the "to" recipients is invalid
  • {"errors":{"to":["contains a blocked address"]}} if at least one (but not necessarily all) of the "to" recipients is on the blocked list
  • {"errors":{"to":["contains a blocked address", "is invalid"]}} if both conditions apply
  • With multiple "to" addresses, there is no way to tell from the API response which of the addresses did or didn't cause these errors
  • It seems like errors in "cc" or "bcc" addresses are not currently reported—the message is queued and delivered to the "to" recipients. [Aside: it looks like delivery is attempted for a blocked cc. An invalid cc field seems to cause the message to get stuck in the Queued status. I'm guessing these might be bugs?]

With that in mind, here are some scenarios parse_recipient_status needs to cover:

  • One or more "to" recipients, no errors
    • Current implementation: all "to" recipients are reported "queued", which is the desired result
  • Exactly one "to" recipient, invalid address
    • Current implementation: the recipient is reported as "invalid", which is the desired result
  • Exactly one "to" recipient, on the blocked list
    • Current implementation: the recipient is reported as "rejected", which is the desired result
  • Multiple "to" recipients, some invalid and/or blocked (and some not)
    • Current implementation: all "to" recipients are reported as either "invalid" or "rejected", even if that individual recipient was valid. This is misleading. (Also, when both errors apply, whichever happens to be last in the errors list determines the status for all recipients.)
    • Desired result: with multiple recipients and some invalid/blocked errors, use status "failed" for all "to" recipients. (We know the message wasn't sent to that recipient, but for each individual address we can't be sure why—or even that there was a problem with that address.)
  • Any of the above, but also with "cc" and "bcc" recipients
    • Current implementation: "cc" and "bcc" recipients are given the same status as the "to" recipient(s) ("queued" or "invalid" or "rejected"), which may be misleading (because MailPace doesn't currently report errors on cc and bcc, and the cc/bcc recipients aren't necessarily invalid/blocked just because one of the to recipients is)
    • Desired result: probably easiest to just omit "cc" and "bcc" recipients. (Could instead give them status "unknown" if the "to" is "queued", or "failed" if the "to" is invalid/blocked, but that doesn't add much value.)
  • Zero "to" recipients, one or more "cc" and/or "bcc" recipients
    • [Developers sometimes try to use cc/bcc-only messages to implement something like a mailing list]
    • Current implementation: all "cc" and "bcc" recipients are given status "invalid", which will be misleading (the cc and bcc recipients are likely valid email addresses)
    • Desired result: since MailPace doesn't support messages without "to", just let its {"errors":{"to":["undefined field"]}} response be reported as an AnymailRequestsAPIError. (Don't try to handle this through recipient status.)
  • More than 50 "to" recipients
    • Current implementation: all recipients are reported as "invalid", which is misleading
    • Desired result: just let MailPace's "number of email addresses exceeds maximum volume" response be reported as an AnymailRequestsAPIError. (Don't try to handle this through recipient status.)

Here's a suggested approach that I think achieves all of this:

  • Replace to_cc_and_bcc_emails with just to_emails
  • If response.status_code == 200, return status = "queued" for all to_emails
    • Is it necessary to check parsed_response["status"] == "queued"? (Could there ever be any other status with a 200 response?)
  • If response.status_code == 400 and the only field in parsed_response["errors"] is "to" and the errors in parsed_response["errors"]["to"] are only "is invalid" and/or "contains a blocked address":
    • If there is exactly one to_emails:
      • If "is invalid", return status = "invalid" for all to_emails
      • If "contains a blocked address", return status = "rejected" for all to_emails
      • If both, return status = "failed" for all to_emails
    • Else (more than one to_emails) return status = "failed" for all to_emails (we don't know which are invalid or rejected)
  • In all other cases, raise an AnymailRequestsAPIError for the response
    • This includes all other problems that might be reported in parsed_response["errors"]["to"]
    • This also covers the case where MailPace later starts reporting errors in cc/bcc fields
  • (Wrap all parsed_response[...] access to convert KeyError and TypeError to AnymailRequestsAPIError("Unexpected MailPace API response format"...))

Does that make sense? Have I missed anything?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @medmunds - my suggestion here is for us to fix the issues with non-reporting of errors and blocked emails to cc and bcc'd recipients first (plus the other bug reported below), then come back and fix the PR appropriately. We'll get back shortly on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

fix the issues with non-reporting of errors and blocked emails to cc and bcc'd recipients first

Sounds good, thanks.

if field in json_response["errors"]:
error_messages = json_response["errors"][field]
for email in payload.to_cc_and_bcc_emails:
for error_message in error_messages:
if (
"undefined field" in error_message
or "is invalid" in error_message
):
recipient_status[
email.addr_spec
] = AnymailRecipientStatus(
message_id=None, status="invalid"
)
elif "contains a blocked address" in error_message:
recipient_status[
email.addr_spec
] = AnymailRecipientStatus(
message_id=None, status="rejected"
)
elif (
"number of email addresses exceeds maximum volume"
in error_message
):
recipient_status[
email.addr_spec
] = AnymailRecipientStatus(
message_id=None, status="invalid"
)
else:
continue # No errors found in this field; continue to next field
else:
raise AnymailRequestsAPIError(
email_message=message,
payload=payload,
response=response,
backend=self,
)

return dict(recipient_status)


class MailPacePayload(RequestsPayload):
def __init__(self, message, defaults, backend, *args, **kwargs):
headers = {
"Content-Type": "application/json",
"Accept": "application/json",
}
self.server_token = backend.server_token # esp_extra can override
self.to_cc_and_bcc_emails = []
super().__init__(message, defaults, backend, headers=headers, *args, **kwargs)

def get_api_endpoint(self):
return "send"

def get_request_params(self, api_url):
params = super().get_request_params(api_url)
params["headers"]["MailPace-Server-Token"] = self.server_token
return params

def serialize_data(self):
return self.serialize_json(self.data)

#
# Payload construction
#

def init_payload(self):
self.data = {} # becomes json

def set_from_email(self, email):
self.data["from"] = email.address

def set_recipients(self, recipient_type, emails):
assert recipient_type in ["to", "cc", "bcc"]
if emails:
# Creates to, cc, and bcc in the payload
self.data[recipient_type] = ", ".join([email.address for email in emails])
self.to_cc_and_bcc_emails += emails

def set_subject(self, subject):
self.data["subject"] = subject

def set_reply_to(self, emails):
if emails:
reply_to = ", ".join([email.address for email in emails])
self.data["replyto"] = reply_to

paul-oms marked this conversation as resolved.
Show resolved Hide resolved
def set_extra_headers(self, headers):
if "list-unsubscribe" in headers:
self.data["list_unsubscribe"] = headers.pop("list-unsubscribe")
if headers:
self.unsupported_features("extra_headers (other than List-Unsubscribe)")

def set_text_body(self, body):
self.data["textbody"] = body

def set_html_body(self, body):
self.data["htmlbody"] = body

def make_attachment(self, attachment):
"""Returns MailPace attachment dict for attachment"""
att = {
"name": attachment.name or "",
"content": attachment.b64content,
"content_type": attachment.mimetype,
}
if attachment.inline:
att["cid"] = "cid:%s" % attachment.cid
return att

def set_attachments(self, attachments):
if attachments:
self.data["attachments"] = [
self.make_attachment(attachment) for attachment in attachments
]

def set_tags(self, tags):
if tags:
if len(tags) == 1:
self.data["tags"] = tags[0]
else:
self.data["tags"] = tags
paul-oms marked this conversation as resolved.
Show resolved Hide resolved

def set_esp_extra(self, extra):
self.data.update(extra)
# Special handling for 'server_token':
self.server_token = self.data.pop("server_token", self.server_token)
11 changes: 11 additions & 0 deletions anymail/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
)
from .webhooks.mailgun import MailgunInboundWebhookView, MailgunTrackingWebhookView
from .webhooks.mailjet import MailjetInboundWebhookView, MailjetTrackingWebhookView
from .webhooks.mailpace import MailPaceInboundWebhookView, MailPaceTrackingWebhookView
from .webhooks.mandrill import MandrillCombinedWebhookView
from .webhooks.postal import PostalInboundWebhookView, PostalTrackingWebhookView
from .webhooks.postmark import PostmarkInboundWebhookView, PostmarkTrackingWebhookView
Expand Down Expand Up @@ -50,6 +51,11 @@
MailjetInboundWebhookView.as_view(),
name="mailjet_inbound_webhook",
),
path(
"mailpace/inbound/",
MailPaceInboundWebhookView.as_view(),
name="mailpace_inbound_webhook",
),
path(
"postal/inbound/",
PostalInboundWebhookView.as_view(),
Expand Down Expand Up @@ -95,6 +101,11 @@
MailjetTrackingWebhookView.as_view(),
name="mailjet_tracking_webhook",
),
path(
"mailpace/tracking/",
MailPaceTrackingWebhookView.as_view(),
name="mailpace_tracking_webhook",
),
path(
"postal/tracking/",
PostalTrackingWebhookView.as_view(),
Expand Down
Loading
Loading