Skip to content

Conversation

@janneThoft
Copy link
Contributor

SendGrid supports specifying the metadata per message when using the bulk-sending API, like with merge data. This is really useful in some cases, so here's simple support for it using the same kind of API as merge data.

@medmunds
Copy link
Contributor

Thanks for this! It's a nice addition to Anymail. (I recall Mandrill supports something similar, and think some of the other ESPs may too.)

Code looks good. I'd like to add some tests before merging, in test_sendgrid_backend. (Basic behavior of this feature, and also checking what happens if someone wants to use both merge_data and merge_metadata on the same message.) If you'd like to take a crack at the tests, that's great, or if not I can do it later this week.

Copy link
Contributor

@medmunds medmunds left a comment

Choose a reason for hiding this comment

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

Looks like the tests uncovered some issues that will need to be addressed before this can be merged.

if recipient_metadata:
recipient_custom_args = self.transform_metadata(recipient_metadata)

if self.data.get("custom_args"):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this entire section that merges custom_args (global metadata) is unnecessary. SendGrid supports custom_args in both individual personalizations and at the top level (globally), and merges them as expected. Also...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure about this? We could not make it work with individual custom_args.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. If you expand custom_args in their docs, it says "personalizations[x].custom_args will be merged with message level custom_args, overriding any conflicting keys."

SendGrid docs have certainly been known to, um, not match their actual implementation. But I just ran a quick test, and custom_args personalizations seemed to behave as documented. Is there a specific case that didn't work for you?

Here's the test I ran:

POST https://api.sendgrid.com/v3/mail/send HTTP/1.1
Authorization: Bearer {{SENDGRID_API_KEY}}
Content-Type: application/json

{
  "custom_args": {
    "common": "common value",
    "shared": "default shared value"
  },
  "personalizations": [{
    "to": [{"email": "recip+1@example.com"}],
    "subject": "Custom args test (1)",
    "custom_args": {
      "specific": "specific value 1",
      "shared": "override shared value"
    }
  }, {
    "to": [{"email": "recip+2@example.com"}],
    "subject": "Custom args test (2)",
    "custom_args": {
      "specific": "specific value 2"
    }
  }],
  "from": {"email": "test@{{SENDGRID_TEST_DOMAIN}}"},
  "content": [{
    "type": "text/plain",
    "value": "Testing handling of custom args"
  }]
}

After sending that, SendGrid's email_activity dashboard shows the expected (merged) Unique Args for each message:

recip+1:
recip+1 Unique Args

recip+2:
recip+2 Unique Args

Copy link
Contributor

Choose a reason for hiding this comment

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

@janneThoft do you have any more details about what didn't work with mixing individual and top-level custom_args? They seem to be working for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, it seems we must have been making a mistake when testing then

@medmunds medmunds mentioned this pull request Feb 7, 2019
@zak10
Copy link

zak10 commented Feb 19, 2019

I'd love to see these features land in a release soon. Down to dive in if any help is needed.

@medmunds
Copy link
Contributor

I'm hoping to hear back from @janneThoft on what problem they were seeing with individual custom_args (see thread above).

janneThoft and others added 7 commits February 21, 2019 11:35
(Currently failing: tests uncover several issues.)
SendGrid supports both top-level and individual custom_args, and seems
to merge them properly. Use their API feature to simplify our code.
@medmunds medmunds merged commit 85dce5f into anymail:master Feb 21, 2019
@medmunds medmunds mentioned this pull request Feb 23, 2019
9 tasks
@medmunds
Copy link
Contributor

Released in Anymail v6.0.

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.

3 participants