-
Notifications
You must be signed in to change notification settings - Fork 133
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
paul-oms
wants to merge
25
commits into
anymail:main
Choose a base branch
from
mailpace:feature/mailpace-esp
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
MailPace: New ESP #355
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 b4811a8
Added error handling for MP backend with tests passing
paul-oms 9564cbd
Refactor and 100% test coverage of MP Backend
paul-oms 4a8a643
Remove unnecessary imports
paul-oms a530780
Added MailPace Webhooks
paul-oms f38f69d
Inbound webhooks, with test coverage
paul-oms 113dc19
Enhacements to inbound webhooks
paul-oms 1a91f0a
Support MailPace Webhook signature validation
paul-oms 217d185
Started the MailPace Documentation
paul-oms 0077419
Fix table
paul-oms afc7874
Fix docs table and inbound docs
paul-oms 62930f6
Fix linting issues
paul-oms 5158f99
Make pynacl optional
paul-oms e659634
Make pynacl optional in the mailpace test utils
paul-oms f8d1d95
Fix undefined LazyError and AnymailImproperlyInstalled
paul-oms 82695fb
Fix import order
paul-oms 662321b
Fix Lazy Error in test utils, make the Webhook tests use the CLient w…
paul-oms f627a5e
Skip webhook tests if pynacl is unavailable, support Django versions …
paul-oms 3d2183e
Revert skipping validation if webhook key not set
paul-oms 610257a
Fix failing tests, due to unset webhook key, correct warning message
paul-oms eab63d3
Warn if basic auth is not on, and signature validation is not setup
paul-oms de7f9a0
PR fededback: Refactor /send endpoint, default to unknown status, rem…
paul-oms 8402107
Fix lint
paul-oms 8e53167
Add integration tests
medmunds b03feb9
Update esp-feature-matrix in docs
medmunds File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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"]: | ||
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) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'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?)
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.
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.
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, 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 applyWith that in mind, here are some scenarios
parse_recipient_status
needs to cover:{"errors":{"to":["undefined field"]}}
response be reported as an AnymailRequestsAPIError. (Don't try to handle this through recipient status.)"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:
to_cc_and_bcc_emails
with justto_emails
response.status_code == 200
, returnstatus = "queued"
for allto_emails
parsed_response["status"] == "queued"
? (Could there ever be any other status with a 200 response?)response.status_code == 400
and the only field inparsed_response["errors"]
is"to"
and the errors inparsed_response["errors"]["to"]
are only"is invalid"
and/or"contains a blocked address"
:to_emails
:"is invalid"
, returnstatus = "invalid"
for allto_emails
"contains a blocked address"
, returnstatus = "rejected"
for allto_emails
status = "failed"
for allto_emails
to_emails
) returnstatus = "failed"
for allto_emails
(we don't know which are invalid or rejected)AnymailRequestsAPIError
for the responseparsed_response["errors"]["to"]
parsed_response[...]
access to convert KeyError and TypeError toAnymailRequestsAPIError("Unexpected MailPace API response format"...)
)Does that make sense? Have I missed 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.
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.
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.
Sounds good, thanks.