SendGrid: generate unique message_id for each batch recipient

medmunds · medmunds · commit 578bad9a5784 · 2019-02-23T15:01:54.000-08:00
Closes #139
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -39,12 +39,23 @@ Breaking changes
   code is doing something like `message.anymail_status.recipients[email.lower()]`,
   you should remove the `.lower()`
 
+* **SendGrid:** In batch sends, Anymail's SendGrid backend now assigns a separate
+  `message_id` for each "to" recipient, rather than sharing a single id for all
+  recipients. This improves accuracy of tracking and statistics (and matches the
+  behavior of many other ESPs).
+
+  If your code uses batch sending (merge_data with multiple to-addresses) and checks
+  `message.anymail_status.message_id` after sending, that value will now be a *set* of
+  ids. You can obtain each recipient's individual message_id with
+  `message.anymail_status.recipients[to_email].message_id`.
+  See `docs <https://anymail.readthedocs.io/en/latest/esps/sendgrid/#sendgrid-message-id>`__.
+
 Features
 ~~~~~~~~
 
 * Add new `merge_metadata` option for providing per-recipient metadata in batch
   sends. Available for all supported ESPs *except* Amazon SES and SendinBlue.
-  See `docs <https://anymail.readthedocs.io/en/latest/sending/anymail_additions/#anymail.message.AnymailMessage.merge_metadata>`_.
+  See `docs <https://anymail.readthedocs.io/en/latest/sending/anymail_additions/#anymail.message.AnymailMessage.merge_metadata>`__.
   (Thanks `@janneThoft`_ for the idea and SendGrid implementation.)
 
 * **Mailjet:** Remove limitation on using `cc` or `bcc` together with `merge_data`.
diff --git a/anymail/backends/sendgrid.py b/anymail/backends/sendgrid.py
@@ -62,8 +62,9 @@ def parse_recipient_status(self, response, payload, message):
         # (SendGrid uses a non-2xx response for any failures, caught in raise_for_status.)
         # SendGrid v3 doesn't provide any information in the response for a successful send,
         # so simulate a per-recipient status of "queued":
-        status = AnymailRecipientStatus(message_id=payload.message_id, status="queued")
-        return {recipient.addr_spec: status for recipient in payload.all_recipients}
+        return {recip.addr_spec: AnymailRecipientStatus(message_id=payload.message_ids.get(recip.addr_spec),
+                                                        status="queued")
+                for recip in payload.all_recipients}
 
 
 class SendGridPayload(RequestsPayload):
@@ -73,7 +74,7 @@ def __init__(self, message, defaults, backend, *args, **kwargs):
         self.generate_message_id = backend.generate_message_id
         self.workaround_name_quote_bug = backend.workaround_name_quote_bug
         self.use_dynamic_template = False  # how to represent merge_data
-        self.message_id = None  # Message-ID -- assigned in serialize_data unless provided in headers
+        self.message_ids = {}  # recipient -> generated message_id mapping
         self.merge_field_format = backend.merge_field_format
         self.merge_data = {}  # late-bound per-recipient data
         self.merge_global_data = {}
@@ -98,24 +99,25 @@ def init_payload(self):
 
     def serialize_data(self):
         """Performs any necessary serialization on self.data, and returns the result."""
-
-        if self.generate_message_id:
-            self.set_anymail_id()
         if self.is_batch():
             self.expand_personalizations_for_batch()
         self.build_merge_data()
         self.build_merge_metadata()
+        if self.generate_message_id:
+            self.set_anymail_id()
 
         if not self.data["headers"]:
             del self.data["headers"]  # don't send empty headers
 
         return self.serialize_json(self.data)
 
     def set_anymail_id(self):
-        """Ensure message has a known anymail_id for later event tracking"""
-
-        self.message_id = str(uuid.uuid4())
-        self.data.setdefault("custom_args", {})["anymail_id"] = self.message_id
+        """Ensure each personalization has a known anymail_id for later event tracking"""
+        for personalization in self.data["personalizations"]:
+            message_id = str(uuid.uuid4())
+            personalization.setdefault("custom_args", {})["anymail_id"] = message_id
+            for recipient in personalization["to"] + personalization.get("cc", []) + personalization.get("bcc", []):
+                self.message_ids[recipient["email"]] = message_id
 
     def expand_personalizations_for_batch(self):
         """Split data["personalizations"] into individual message for each recipient"""
diff --git a/docs/esps/sendgrid.rst b/docs/esps/sendgrid.rst
@@ -186,6 +186,13 @@ Limitations and quirks
   :setting:`SENDGRID_GENERATE_MESSAGE_ID <ANYMAIL_SENDGRID_GENERATE_MESSAGE_ID>`
   to False in your Anymail settings.
 
+  .. versionchanged:: 6.0
+
+      In batch sends, Anymail generates a distinct anymail_id for *each* "to"
+      recipient. (Previously, a single id was used for all batch recipients.) Check
+      :attr:`anymail_status.recipients[to_email].message_id <anymail.message.AnymailStatus.recipients>`
+      for individual batch-send tracking ids.
+
   .. versionchanged:: 3.0
 
       Previously, Anymail generated a custom :mailheader:`Message-ID`
diff --git a/tests/test_sendgrid_backend.py b/tests/test_sendgrid_backend.py
@@ -63,9 +63,9 @@ def test_send_mail(self):
         self.assertEqual(data['from'], {'email': "from@sender.example.com"})
         self.assertEqual(data['personalizations'], [{
             'to': [{'email': "to@example.com"}],
+            # make sure the backend assigned the anymail_id for event tracking and notification
+            'custom_args': {'anymail_id': 'mocked-uuid-1'},
         }])
-        # make sure the backend assigned the anymail_id for event tracking and notification
-        self.assertEqual(data['custom_args']['anymail_id'], 'mocked-uuid-1')
 
     def test_name_addr(self):
         """Make sure RFC2822 name-addr format (with display-name) is allowed
@@ -115,6 +115,8 @@ def test_email_message(self):
                        {'email': "cc2@example.com", 'name': '"Also CC"'}],
                 'bcc': [{'email': "bcc1@example.com"},
                         {'email': "bcc2@example.com", 'name': '"Also BCC"'}],
+                # make sure custom Message-ID also added to custom_args
+                'custom_args': {'anymail_id': 'mocked-uuid-1'},
             }])
 
         self.assertEqual(data['from'], {'email': "from@example.com"})
@@ -125,8 +127,6 @@ def test_email_message(self):
             'X-MyHeader': "my value",
             'Message-ID': "<mycustommsgid@sales.example.com>",
         })
-        # make sure custom Message-ID also added to custom_args
-        self.assertEqual(data['custom_args']['anymail_id'], 'mocked-uuid-1')
 
     def test_html_message(self):
         text_content = 'This is an important message.'
@@ -454,14 +454,17 @@ def test_merge_data(self):
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'alice@example.com'}],
              'cc': [{'email': 'cc@example.com'}],  # all recipients get the cc
+             'custom_args': {'anymail_id': 'mocked-uuid-1'},
              'dynamic_template_data': {
                  'name': "Alice", 'group': "Developers", 'site': "ExampleCo"}},
             {'to': [{'email': 'bob@example.com', 'name': '"Bob"'}],
              'cc': [{'email': 'cc@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-2'},
              'dynamic_template_data': {
                  'name': "Bob", 'group': "Users", 'site': "ExampleCo"}},
             {'to': [{'email': 'celia@example.com'}],
              'cc': [{'email': 'cc@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-3'},
              'dynamic_template_data': {
                  'group': "Users", 'site': "ExampleCo"}},
         ])
@@ -477,6 +480,7 @@ def test_explicit_dynamic_template(self):
         data = self.get_api_call_json()
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'to@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-1'},
              'dynamic_template_data': {"test": "data"}}])
 
         self.message.template_id = "d-apparently-not-legacy"
@@ -486,6 +490,7 @@ def test_explicit_dynamic_template(self):
         data = self.get_api_call_json()
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'to@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-2'},
              'substitutions': {"<%test%>": "data"}}])
 
     def test_legacy_merge_data(self):
@@ -514,13 +519,16 @@ def test_legacy_merge_data(self):
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'alice@example.com'}],
              'cc': [{'email': 'cc@example.com'}],  # all recipients get the cc
+             'custom_args': {'anymail_id': 'mocked-uuid-1'},
              'substitutions': {':name': "Alice", ':group': "Developers",
                                ':site': "ExampleCo"}},  # merge_global_data merged
             {'to': [{'email': 'bob@example.com', 'name': '"Bob"'}],
              'cc': [{'email': 'cc@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-2'},
              'substitutions': {':name': "Bob", ':group': "Users", ':site': "ExampleCo"}},
             {'to': [{'email': 'celia@example.com'}],
              'cc': [{'email': 'cc@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-3'},
              'substitutions': {':group': "Users", ':site': "ExampleCo"}},
         ])
         self.assertNotIn('sections', data)  # 'sections' no longer used for merge_global_data
@@ -538,9 +546,11 @@ def test_legacy_merge_field_format_setting(self):
         data = self.get_api_call_json()
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'alice@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-1'},
              'substitutions': {':name': "Alice", ':group': "Developers",  # keys changed to :field
                                ':site': "ExampleCo"}},
             {'to': [{'email': 'bob@example.com', 'name': '"Bob"'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-2'},
              'substitutions': {':name': "Bob", ':site': "ExampleCo"}}
         ])
 
@@ -557,8 +567,10 @@ def test_legacy_merge_field_format_esp_extra(self):
         data = self.get_api_call_json()
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'alice@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-1'},
              'substitutions': {'*|name|*': "Alice", '*|group|*': "Developers", '*|site|*': "ExampleCo"}},
             {'to': [{'email': 'bob@example.com', 'name': '"Bob"'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-2'},
              'substitutions': {'*|name|*': "Bob", '*|site|*': "ExampleCo"}}
         ])
         # Make sure our esp_extra merge_field_format doesn't get sent to SendGrid API:
@@ -587,11 +599,11 @@ def test_merge_metadata(self):
         data = self.get_api_call_json()
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'alice@example.com'}],
-             'custom_args': {'order_id': '123'}},
+             # anymail_id added to other custom_args
+             'custom_args': {'anymail_id': 'mocked-uuid-1', 'order_id': '123'}},
             {'to': [{'email': 'bob@example.com', 'name': '"Bob"'}],
-             'custom_args': {'order_id': '678', 'tier': 'premium'}},
+             'custom_args': {'anymail_id': 'mocked-uuid-2', 'order_id': '678', 'tier': 'premium'}},
         ])
-        self.assertEqual(data['custom_args'], {'anymail_id': 'mocked-uuid-1'})
 
     def test_metadata_with_merge_metadata(self):
         # Per SendGrid docs: "personalizations[x].custom_args will be merged
@@ -608,12 +620,11 @@ def test_metadata_with_merge_metadata(self):
         data = self.get_api_call_json()
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'alice@example.com'}],
-             'custom_args': {'order_id': '123'}},
+             'custom_args': {'anymail_id': 'mocked-uuid-1', 'order_id': '123'}},
             {'to': [{'email': 'bob@example.com', 'name': '"Bob"'}],
-             'custom_args': {'order_id': '678', 'tier': 'premium'}},
+             'custom_args': {'anymail_id': 'mocked-uuid-2', 'order_id': '678', 'tier': 'premium'}},
         ])
-        self.assertEqual(data['custom_args'],
-                         {'tier': 'basic', 'batch': 'ax24', 'anymail_id': 'mocked-uuid-1'})
+        self.assertEqual(data['custom_args'], {'tier': 'basic', 'batch': 'ax24'})
 
     def test_merge_metadata_with_merge_data(self):
         # (using dynamic templates)
@@ -641,16 +652,17 @@ def test_merge_metadata_with_merge_data(self):
              'cc': [{'email': 'cc@example.com'}],  # all recipients get the cc
              'dynamic_template_data': {
                  'name': "Alice", 'group': "Developers", 'site': "ExampleCo"},
-             'custom_args': {'order_id': '123'}},
+             'custom_args': {'anymail_id': 'mocked-uuid-1', 'order_id': '123'}},
             {'to': [{'email': 'bob@example.com', 'name': '"Bob"'}],
              'cc': [{'email': 'cc@example.com'}],
              'dynamic_template_data': {
                  'name': "Bob", 'group': "Users", 'site': "ExampleCo"},
-             'custom_args': {'order_id': '678', 'tier': 'premium'}},
+             'custom_args': {'anymail_id': 'mocked-uuid-2', 'order_id': '678', 'tier': 'premium'}},
             {'to': [{'email': 'celia@example.com'}],
              'cc': [{'email': 'cc@example.com'}],
              'dynamic_template_data': {
-                 'group': "Users", 'site': "ExampleCo"}},
+                 'group': "Users", 'site': "ExampleCo"},
+             'custom_args': {'anymail_id': 'mocked-uuid-3'}},
         ])
 
     def test_merge_metadata_with_legacy_template(self):
@@ -677,15 +689,15 @@ def test_merge_metadata_with_legacy_template(self):
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'alice@example.com'}],
              'cc': [{'email': 'cc@example.com'}],  # all recipients get the cc
-             'custom_args': {'order_id': '123'},
+             'custom_args': {'anymail_id': 'mocked-uuid-1', 'order_id': '123'},
              'substitutions': {':name': "Alice", ':group': "Developers", ':site': "ExampleCo"}},
             {'to': [{'email': 'bob@example.com', 'name': '"Bob"'}],
              'cc': [{'email': 'cc@example.com'}],
-             'custom_args': {'order_id': '678', 'tier': 'premium'},
+             'custom_args': {'anymail_id': 'mocked-uuid-2', 'order_id': '678', 'tier': 'premium'},
              'substitutions': {':name': "Bob", ':group': "Users", ':site': "ExampleCo"}},
             {'to': [{'email': 'celia@example.com'}],
              'cc': [{'email': 'cc@example.com'}],
-             # no custom_args
+             'custom_args': {'anymail_id': 'mocked-uuid-3'},
              'substitutions': {':group': "Users", ':site': "ExampleCo"}},
         ])
 
@@ -756,8 +768,10 @@ def test_esp_extra_pesonalizations(self):
         data = self.get_api_call_json()
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'first@example.com', 'name': '"First recipient"'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-1'},
              'future_feature': "works"},
             {'to': [{'email': 'second@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-2'},
              'future_feature': "works"},  # merged into *every* recipient
         ])
 
@@ -770,6 +784,7 @@ def test_esp_extra_pesonalizations(self):
         data = self.get_api_call_json()
         self.assertEqual(data['personalizations'], [
             {'to': [{'email': 'custom@example.com'}],
+             'custom_args': {'anymail_id': 'mocked-uuid-3'},
              'future_feature': "works"},
         ])
 
@@ -784,10 +799,22 @@ def test_send_attaches_anymail_status(self):
         self.assertEqual(msg.anymail_status.status, {'queued'})
         self.assertEqual(msg.anymail_status.message_id, 'mocked-uuid-1')
         self.assertEqual(msg.anymail_status.recipients['to1@example.com'].status, 'queued')
-        self.assertEqual(msg.anymail_status.recipients['to1@example.com'].message_id,
-                         msg.anymail_status.message_id)
+        self.assertEqual(msg.anymail_status.recipients['to1@example.com'].message_id, 'mocked-uuid-1')
         self.assertEqual(msg.anymail_status.esp_response.content, self.DEFAULT_RAW_RESPONSE)
 
+    def test_batch_recipients_get_unique_message_ids(self):
+        """In a batch send, each recipient should get a distinct own message_id"""
+        msg = mail.EmailMessage('Subject', 'Message', 'from@example.com',
+                                ['to1@example.com', 'Someone Else <to2@example.com>'],
+                                cc=['cc@example.com'])
+        msg.merge_data = {}  # force batch send
+        msg.send()
+        self.assertEqual(msg.anymail_status.message_id, {'mocked-uuid-1', 'mocked-uuid-2'})
+        self.assertEqual(msg.anymail_status.recipients['to1@example.com'].message_id, 'mocked-uuid-1')
+        self.assertEqual(msg.anymail_status.recipients['to2@example.com'].message_id, 'mocked-uuid-2')
+        # cc's (and bcc's) get copied for all batch recipients, but we can only communicate one message_id:
+        self.assertEqual(msg.anymail_status.recipients['cc@example.com'].message_id, 'mocked-uuid-2')
+
     @override_settings(ANYMAIL_SENDGRID_GENERATE_MESSAGE_ID=False)
     def test_disable_generate_message_id(self):
         msg = mail.EmailMessage('Subject', 'Message', 'from@example.com', ['to1@example.com'],)
