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

Support merge_headers in Amazon SES bulk send #371

Merged
merged 6 commits into from
May 21, 2024

Conversation

carrerasrodrigo
Copy link
Contributor

Please check ReplacementHeaders
https://docs.aws.amazon.com/ses/latest/APIReference-V2/API_SendBulkEmail.html

This headers are very useful for the new requirements of email clients like gmail, yahoo, etc.

Allows to add headers as a template. This enables, for example, to add an unique unique unsubscribe header for each email.

@carrerasrodrigo
Copy link
Contributor Author

carrerasrodrigo commented May 16, 2024

Please let me know if any change is needed in order to merge it!

Just a quick note, by the time I implemented the headers, there was no published boto3 packages with this functionality, I had to install de latest version from github.

git+https://github.com/boto/s3transfer.git#egg=s3transfer
git+https://github.com/boto/boto3.git#egg=boto3

Thanks a lot for your time!

@medmunds
Copy link
Contributor

Thanks for this! Yes, very relevant with the need for List-Unsubscribe headers.

Two requests:

  1. I'd prefer naming the new attribute merge_headers, to parallel Anymail's existing merge_metadata (which is the per-recipient version of metadata).
  2. If there are no merge headers for a particular recipient, don't add the ReplacementHeaders field to the SES API call at all (rather than adding it with an empty list). This makes it easier to release without requiring a boto3 upgrade. (If people aren't using merge_headers, their current boto3 version will continue to work without error.)

Also, it would be good to test merge_headers in the Amazon SES integration tests, but this will need to wait for a boto3 release that supports them.

I can look into supporting merge_headers for other ESPs, too. It seems like Brevo, Mailjet, Postmark, Resend, and Sendgrid all have reasonably-easy support for per-recipient headers in their batch APIs. Unisender Go supports them (using substitutions), but only for the List-Unsubscribe header. Mailgun may or may not support them. (I can't tell from Mailgun's docs if their "replacement variables" work in header values; will need to test.)

@carrerasrodrigo
Copy link
Contributor Author

Thanks a lot for your feedback @medmunds! I made the changes as requested. Let me know any additional modification!
I think I will have time to checkout Sendgrid and add an additional PR 🤞

@medmunds
Copy link
Contributor

Excellent, thanks.

Sorry, one more thing: just noticed this is expecting the merge_headers for each recipient in the [{"Name": header, "Value": value}, ...] list form used by SES. I think it would be more natural to use a {header: value, ...} dict, matching Django's EmailMessage headers:

    message = AnymailMessage(
        ...,
        headers={
            "List-Unsubscribe-Post": "List-Unsubscribe=One-Click",
        },
        merge_headers={
            "alice@example.com": {
                "List-Unsubscribe": "<https://example.com/a/>",
            },
            "nobody@example.com": {
                "List-Unsubscribe": "<mailto:unsubscribe@example.com>", 
            },
        },
    )

[Btw, we try to use example.com or a similar reserved example or test domain. xyz.com is an actual domain that belongs to someone.]

@carrerasrodrigo
Copy link
Contributor Author

@medmunds pushed the changes as requested.

I did not use headers fields, I got this error:

======================================================================
ERROR: test_template (tests.test_amazon_ses_backend.AmazonSESBackendAnymailFeatureTests)
With template_id, Anymail switches to SESv2 SendBulkEmail
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/rodrigocarreras/WORK/django-anymail/.tox/django42-py38-all/lib/python3.8/site-packages/django/test/utils.py", line 461, in inner
    return func(*args, **kwargs)
  File "/Users/rodrigocarreras/WORK/django-anymail/tests/test_amazon_ses_backend.py", line 680, in test_template
    message.send()
  File "/Users/rodrigocarreras/WORK/django-anymail/.tox/django42-py38-all/lib/python3.8/site-packages/django/core/mail/message.py", line 299, in send
    return self.get_connection(fail_silently).send_messages([self])
  File "/Users/rodrigocarreras/WORK/django-anymail/anymail/backends/base.py", line 117, in send_messages
    sent = self._send(message)
  File "/Users/rodrigocarreras/WORK/django-anymail/anymail/backends/amazon_ses.py", line 78, in _send
    return super()._send(message)
  File "/Users/rodrigocarreras/WORK/django-anymail/anymail/backends/base.py", line 146, in _send
    payload = self.build_message_payload(message, self.send_defaults)
  File "/Users/rodrigocarreras/WORK/django-anymail/anymail/backends/amazon_ses.py", line 96, in build_message_payload
    return AmazonSESV2SendBulkEmailPayload(message, defaults, self)
  File "/Users/rodrigocarreras/WORK/django-anymail/anymail/backends/base.py", line 340, in __init__
    setter(value)
  File "/Users/rodrigocarreras/WORK/django-anymail/anymail/backends/base.py", line 412, in process_extra_headers
    self.set_extra_headers(headers)
  File "/Users/rodrigocarreras/WORK/django-anymail/anymail/backends/amazon_ses.py", line 447, in set_extra_headers
    self.unsupported_feature("extra_headers with template")
  File "/Users/rodrigocarreras/WORK/django-anymail/anymail/backends/base.py", line 360, in unsupported_feature
    raise AnymailUnsupportedFeature(
anymail.exceptions.AnymailUnsupportedFeature: Amazon SES does not support extra_headers with template

code

headers={
            "List-Unsubscribe-Post": "List-Unsubscribe=One-Click",
        },

Let me know if i'm missing something.

@medmunds
Copy link
Contributor

I think my suggestion may have been unclear. I believe the new merge_headers Anymail option should take a dict of dicts, not a dict of lists. The desired format is:

        merge_headers={
            "alice@example.com": {  
                # not a list, just a dict of field: value
                "List-Unsubscribe": "<https://example.com/a/>",
                "X-Some-Other-Header": "some-other-value",
            },
            "nobody@example.com": {
                "List-Unsubscribe": "<mailto:unsubscribe@example.com>", 
            },
        },

This gives it the same structure as Django's own email headers (a.k.a. extra_headers), and Anymail's merge_data and merge_metadata.

Then in the SES finalize_payload implementation, we would convert that dict to the list format ses::SendBulkEmail requires. Something like:

            if to.addr_spec in self.merge_headers:
                entry["ReplacementHeaders"] = [
                    {"Name": key, "Value": value} 
                    for key, value in self.merge_headers[to.addr_spec].items()
                ]

Does that seem reasonable?

As for this error…

AnymailUnsupportedFeature: Amazon SES does not support extra_headers with template

…that's this code. Because when it was first implemented, there wasn't a way to pass any email headers to ses::SendBulkEmail. We could support it now, using ReplacementHeaders.

I can add that at some point later, or if you're interested, the idea is that the ReplacementHeaders for each recipient would be the merger of any extra_headers (for all recipients) plus any merge_headers (for that recipient).

@carrerasrodrigo
Copy link
Contributor Author

@medmunds sorry for the misunderstanding, I updated the code to a dict of dicts.

I can add that at some point later, or if you're interested, the idea is that the ReplacementHeaders for each recipient would be the merger of any extra_headers (for all recipients) plus any merge_headers (for that recipient).

I would try to add a new PR supporting this feature 💪

Hope is ready to merge! have a great day!

- Change in the name of the method that sets the template headers
- Change in SES backend, made template headers optional for backward compatibility.
@medmunds medmunds changed the title Added support for extra headers in amazon ses bulk ses Support merge_headers in Amazon SES bulk send May 21, 2024
@medmunds medmunds merged commit 33f6806 into anymail:main May 21, 2024
58 checks passed
@medmunds
Copy link
Contributor

Excellent! Thanks again for this—very nice addition.

I'll open a couple of issues to track related work.

@medmunds medmunds mentioned this pull request Jun 20, 2024
medmunds added a commit that referenced this pull request Jun 20, 2024
Implement and document `merge_headers`
for all other ESPs that can support it. (See #371
for base and Amazon SES implementation.)

Closes #374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants