Added support for dynamicTemplateData #691

therajumandapati · 2018-05-30T23:08:08Z

Fixes

Fixes #689

Checklist

I have made a material change to the repo (functionality, testing, spelling, grammar)
I have read the [Contribution Guide] and my PR follows them.
I updated my branch with the master branch.
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation about the functionality in the appropriate .md file
I have added in line documentation to the code I modified

Short description of what this PR does:

Adds support for SendGrid's latest feature for enabling handlebars syntax in transactional templates
Checks the templateId, and decides whether it is dynamic template or not. (Dynamic template ids start with d-.
Does not send substitutions if the templateId is for a dynamic template
Accepts dynamicTemplateData in the mail object directly and merges with dynamicTemplateData in each personalization object

SendGridDX · 2018-05-30T23:08:25Z

All committers have signed the CLA.

therajumandapati · 2018-05-31T18:43:44Z

@thinkingserious As we're on a time crunch, we published the new packages with this version to our private npm org and we'll be using it until this PR gets reviewed/merged. Hope its okay.

adamreisnz

Thanks for the PR, it mostly looks good to me, just a few comments and recommendations, plus some code style issues to make sure we keep our code base consistent.

If you could look at these and make the necessary changes this should be good to go!

adamreisnz · 2018-06-01T21:00:48Z

packages/helpers/classes/mail.js

@@ -247,7 +273,7 @@ class Mail {
    ) {
      throw new Error('Provide at least one of to, cc or bcc');
    }
-    this.addPersonalization(new Personalization({to, cc, bcc}));


The code style in this package doesn't use leading/trailing spaces after/before curly brackets, could you revert this change?

adamreisnz · 2018-06-01T21:01:26Z

packages/helpers/classes/mail.js

@@ -21,6 +21,7 @@ class Mail {
  constructor(data) {

    //Initialize array and object properties
+    this._isDynamic = false;


I'm thinking just us this.isDynamic for the flag, e.g. without the underscore.

adamreisnz · 2018-06-01T21:01:57Z

packages/helpers/classes/mail.js

+  }
+
+  /**
+   * Set substitutions


Fix comment to avoid confusion

adamreisnz · 2018-06-01T21:03:08Z

packages/helpers/classes/mail.spec.js

@@ -8,10 +8,10 @@ const Mail = require('./mail');
 /**
 * Tests
 */
-describe('Mail', function() {
+describe('Mail', function () {


Can you also undo these style corrections? Please respect the code style as indicated by the editor config and eslint config. If you enable ESLint in your editor it should automatically fix these issues.

adamreisnz · 2018-06-01T21:06:01Z

packages/helpers/classes/personalization.js

+  /**
+   * Reverse merge dynamic template data, preserving existing ones
+   */
+  reverseMergeDynamicTemplateData(dynamicTemplateData) {


Can we call this method deepMergeDynamicTemplateData instead? I think that's what's happening here, so that description would be a bit clearer and a better fit imo.

adamreisnz · 2018-06-01T21:06:30Z

packages/helpers/classes/personalization.js

    } = this;

    //Initialize with mandatory values
-    const json = {to};
+    const json = { to };


adamreisnz · 2018-06-01T21:09:38Z

packages/helpers/helpers/merge-data-deep.js

@@ -0,0 +1,33 @@
+'use strict';


Can we use the existing and proven package deepmerge for this instead of rolling our own helper? From experience, this can be a tricky beast to get right, so I'd rather use an existing package which is being downloaded and used a lot, rather than try to replicate and have to maintain this part of the logic ourselves.

In particular merging of arrays can be tricky, which this package handles very flexibly.

adamreisnz · 2018-06-01T21:14:36Z

@ben-grid Looks good to me, just a few tweaks that I'd recommend, as well as using the external library deepmerge instead of making our own.

If this gets merged I'd probably pick up the fix from #687 as well, as it touches the same code.

ben-grid · 2018-06-01T21:32:34Z

@adamreisnz @mvpspl619 You both friggin rock! Thanks so much for your contributions here!

therajumandapati · 2018-06-13T21:33:07Z

@adamreisnz Can you take a look one more time? Sorry about the delay, I couldn't figure out why my VSCode kept overriding the trailingSpaces config, and then realized it was in my userSettings, so turned it off and voila.

adamreisnz

Thanks for making the changes, looks good to me!

thinkingserious · 2018-06-14T21:19:02Z

Hello @mvpspl619,

Thanks again for the PR!

We want to show our appreciation by sending you some swag. Could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

ron23 · 2018-06-15T05:44:22Z

Hey, may I ask what is dynamic templating and what's the difference between that and the standard "Transactional Templating"?
I also don't get how to create one. If I create one, I automatically get an ID, which doesn't start with a "d-" - does that mean it's not "dynamic"?

adamreisnz · 2018-06-15T05:50:23Z

@ron23 yes I think that's correct. See #221 (comment) for more information on what dynamic templates are (going to be). Essentially it adds support for handlebars style templates with additional templating language syntax, like loops and conditional statements etc.

ben-grid · 2018-06-15T20:18:22Z

@ron23 Adam nailed it, I just wanted to add, we are running a beta of this right now, which is why/how Adam and Raju have access to this functionality and you don't yet. If you'd like to participate just drop me an email @ ben@sendgrid.com and include your username. Cheers!

ron23 · 2018-06-15T20:26:13Z

Thanks guys.
I got really confused because I did see https://sendgrid.com/docs/User_Guide/Transactional_Templates/Using_handlebars.html in the documentation...Nowhere I saw it's a beta thing.

But yea, this issue #221 totally explained everything.

isaachinman · 2018-07-24T15:00:59Z

I cannot believe how long it took me to find this buried in the release notes. This info needs to be in the main READMEs. These dynamic transactional templates are apparently the default now, and I lost almost two hours tracking this down.

nemoeric · 2018-07-24T17:16:56Z

I lost 3 hours looking for this dynamicTemplateData parameter :/
I do agree with isaachinman here, this should be visible more easily as Sendgrid now asks its user to migrate to Transactionnal vs Legacy ... !

isaachinman · 2018-07-24T17:38:01Z

It's great work from @mvpspl619, but it's pretty unfortunate that the maintainers here apparently do not require front-facing documentation to accompany new features. I'm happy to submit a PR which updates the docs if it will be accepted.

Is there an explanation why substitutions cannot still be used, even with the new dynamic templates? Why do we need a new dynamicTemplateData if apparently a check is still being run for this d- pattern? @nemoeric and I, for example, would not even be here if this functionality worked without breaking changes to the API.

thinkingserious · 2018-07-24T21:03:16Z

FYI, here is a repost of the announcement made this morning:

Hey all! Great news––

Dynamic content for Transactional Templates is here!

This morning, our team publicly launched dynamic content for transactional templates. It is now available for all customers sending over v3 of our Mail Send API. Iterate over lists, handle conditionals and more, thanks to native support for a subset of Handlebars syntax!

More information can be found in our blog post announcement.

Documentation:

Thank you, on behalf of the entire SendGrid team, for all of the feedback throughout the beta program. 🙌🏻

thinkingserious · 2018-07-24T21:10:10Z

@isaachinman @nemoeric,

My apologies for the poor experience. I've created this issue and added it to our backlog to improve usability. Please feel free to add to that issue if you feel I missed anything.

Thanks!

With Best Regards,

Elmer

Added support for dynamicTemplateData

e477405

thinkingserious added the status: code review request requesting a community code review or review from Twilio label May 30, 2018

therajumandapati mentioned this pull request May 30, 2018

Support for dynamic_template_data #689

Closed

Fixing broken build for sendgrid#689

2abbbce

therajumandapati force-pushed the master branch from 4020733 to 2abbbce Compare May 31, 2018 18:42

adamreisnz suggested changes Jun 1, 2018

View reviewed changes

Incorporated code review comments for sendgrid#691

afc4e65

adamreisnz approved these changes Jun 14, 2018

View reviewed changes

thinkingserious merged commit 15d9b0a into sendgrid:master Jun 14, 2018

thinkingserious mentioned this pull request Jul 24, 2018

Add Dynamic Template Support #703

Closed

jayprado mentioned this pull request Jul 25, 2018

Substitutions not working #676

Closed

This was referenced Jul 26, 2018

Add Dynamic Template Support sendgrid/sendgrid-php#648

Closed

Add Dynamic Template Support sendgrid/sendgrid-java#447

Closed

Add Dynamic Template Support sendgrid/sendgrid-python#591

Closed

Add Dynamic Template Support sendgrid/sendgrid-csharp#716

Closed

This was referenced Jul 26, 2018

Add Dynamic Template Support sendgrid/sendgrid-go#260

Closed

Add Dynamic Template Support sendgrid/sendgrid-ruby#301

Closed

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade @sendgrid/mail from 6.3.1 to 6.4.0 dyna-dot/fireship.io#5

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade @sendgrid/mail from 6.3.1 to 6.5.5 miasmos/portfolio-server#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for dynamicTemplateData #691

Added support for dynamicTemplateData #691

therajumandapati commented May 30, 2018 •

edited

Loading

SendGridDX commented May 30, 2018 •

edited

Loading

therajumandapati commented May 31, 2018

adamreisnz left a comment

adamreisnz Jun 1, 2018

adamreisnz Jun 1, 2018

adamreisnz Jun 1, 2018

adamreisnz Jun 1, 2018

adamreisnz Jun 1, 2018

adamreisnz Jun 1, 2018

adamreisnz Jun 1, 2018

adamreisnz commented Jun 1, 2018

ben-grid commented Jun 1, 2018

therajumandapati commented Jun 13, 2018

adamreisnz left a comment

thinkingserious commented Jun 14, 2018

ron23 commented Jun 15, 2018

adamreisnz commented Jun 15, 2018

ben-grid commented Jun 15, 2018

ron23 commented Jun 15, 2018

isaachinman commented Jul 24, 2018

nemoeric commented Jul 24, 2018

isaachinman commented Jul 24, 2018

thinkingserious commented Jul 24, 2018

thinkingserious commented Jul 24, 2018

Added support for dynamicTemplateData #691

Added support for dynamicTemplateData #691

Conversation

therajumandapati commented May 30, 2018 • edited Loading

Fixes

Checklist

Short description of what this PR does:

SendGridDX commented May 30, 2018 • edited Loading

therajumandapati commented May 31, 2018

adamreisnz left a comment

Choose a reason for hiding this comment

adamreisnz Jun 1, 2018

Choose a reason for hiding this comment

adamreisnz Jun 1, 2018

Choose a reason for hiding this comment

adamreisnz Jun 1, 2018

Choose a reason for hiding this comment

adamreisnz Jun 1, 2018

Choose a reason for hiding this comment

adamreisnz Jun 1, 2018

Choose a reason for hiding this comment

adamreisnz Jun 1, 2018

Choose a reason for hiding this comment

adamreisnz Jun 1, 2018

Choose a reason for hiding this comment

adamreisnz commented Jun 1, 2018

ben-grid commented Jun 1, 2018

therajumandapati commented Jun 13, 2018

adamreisnz left a comment

Choose a reason for hiding this comment

thinkingserious commented Jun 14, 2018

ron23 commented Jun 15, 2018

adamreisnz commented Jun 15, 2018

ben-grid commented Jun 15, 2018

ron23 commented Jun 15, 2018

isaachinman commented Jul 24, 2018

nemoeric commented Jul 24, 2018

isaachinman commented Jul 24, 2018

thinkingserious commented Jul 24, 2018

Dynamic content for Transactional Templates is here!

thinkingserious commented Jul 24, 2018

therajumandapati commented May 30, 2018 •

edited

Loading

SendGridDX commented May 30, 2018 •

edited

Loading