anymail / django-anymail

Django email backends and webhooks for Amazon SES, Brevo (Sendinblue), MailerSend, Mailgun, Mailjet, Postmark, Postal, Resend, SendGrid, SparkPost, Unisender Go and more
https://anymail.dev
BSD 3-Clause "New" or "Revised" License
1.65k stars 125 forks source link

Support merge_headers in Amazon SES bulk send #371

Closed carrerasrodrigo closed 3 months ago

carrerasrodrigo commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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

medmunds commented 3 months ago

Excellent! Thanks again for this—very nice addition.

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