Adds support for dynamic template data in personalizations #593

0bsearch · 2018-08-01T17:31:20Z

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 dynamic templates

Fixes #591

codecov · 2018-08-01T17:37:17Z

Codecov Report

Merging #593 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
+ Coverage   86.61%   86.65%   +0.04%     
==========================================
  Files          32       32              
  Lines         979      982       +3     
  Branches      131      132       +1     
==========================================
+ Hits          848      851       +3     
  Misses         67       67              
  Partials       64       64

Impacted Files	Coverage Δ
sendgrid/helpers/mail/personalization.py	`92.94% <100%> (+0.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ac58c6...e1cc256. Read the comment docs.

af4ro · 2018-08-09T16:41:36Z

Thank you so much for your PR @3lnc!
This is a great PR and it covers all the aspects required by the specifications.

af4ro · 2018-08-09T23:35:42Z

use_cases/transational_templates.md

@@ -66,6 +66,38 @@ print(response.body)
 print(response.headers)
 ```

+### With dynamic templates


Might be missing dynamic template updates in the rest of the templates file like here and here

You can check out some of the changes made here in #597

Regarding updates for rest of file: this is left on purpose, as old template system is still in place, as well as code to support it. So, I guess we still need documentation for it.

Or I'm missing the point

thinkingserious · 2018-08-21T00:06:31Z

Hello @3lnc,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

PiDelport · 2018-08-22T09:57:29Z

sendgrid/helpers/mail/personalization.py

@@ -1,6 +1,10 @@
 class Personalization(object):
    """A Personalization defines who should receive an individual message and
    how that message should be handled.
+
+    :var dynamic_template_data: data for dynamic transactional template.
+        Should be JSON-serializeable structure. No pre-processing sill be done


Typo: sill → will

Thanks! Fixed :)

djw27 · 2018-08-29T13:30:52Z

Should this not also remove the use of Substitution from mail_example.py since this has been deprecated?

I also think this is a different type of interface to existing personalizations etc so could be confusing to newcomers as to why dynamic_template_data is added differently, i.e. setting the attribute directly on the personalization rather than using a setter like all other attributes?

Just my thoughts...

0bsearch · 2018-08-29T15:06:37Z

@djw27 by the time this PR was opened I had no info on deprecation of substitution. Could you please provide reference for this info?

Regarding descriptors interface — personally, I really dislike the current state of codebase being overcrowded with getters. This is really not pythonic way, as there's no setters with custom logic. Hiding attributes with _name + @property is actually gives nothing but pain — in case you need to modify helper objects dynamically. But that's IMO only.

Regarding "raw" implementation of dynamic_template_data — this is conscious decision. Dynamic templates requires json. Which is pretty "technical". I mean, if json is needed for handlebars, you probably know what json is and how to map json into python datastructures. All the validation we can do on python side is done while serializing.

Making this interface via setters will require class(es) with recurrent references — all this just to make nested structure. Taking into account that data classes is almost wrappers around dictionaries — I'm pretty sure that raw implementation is better way.

thinkingserious · 2018-08-29T18:10:16Z

Thanks for the feedback @djw27 and @3lnc!

I'm currently working on an update to this SDK where your feedback would be greatly helpful. If you have time, please take a look.

With Best Regards,

Elmer

djw27 · 2018-08-30T08:16:37Z

@3lnc I 100% agree with your points! I don't blame you not knowing that substitutions are being deprecated, I stumbled across the documentation by chance for dynamic templates: https://dynamic-templates.api-docs.io/3.0/mail-send-with-dynamic-transactional-templates/v3-mail-send

I guess it's a wider question for @thinkingserious, but at stage it would be pretty difficult to completely overhaul the interface used for this library. I'll take a look at your link in the mean time!

thinkingserious · 2018-09-05T23:21:01Z

Hello @djw27,

I'm looking forward to your feedback on the SDK update. Thanks!

With Best Regards,

Elmer

Corey-Evans · 2018-11-15T03:52:50Z

@thinkingserious Are substitutions still being deprecated? I remember reading this a few months back. However, now I can't find any reference to this (including on the link referenced above).

0bsearch added 2 commits August 1, 2018 20:16

Adds support for dynamic template data in personalizations

b002197

Example for .md usecase

210a4fa

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

Linking SG dashboard

8ff82a8

thinkingserious added type: community enhancement feature request not on Twilio's roadmap difficulty: medium fix is medium in difficulty labels Aug 4, 2018

thinkingserious mentioned this pull request Aug 4, 2018

Fix dynamic templates personalisation #597

Closed

6 tasks

minor formatting

e1cc256

af4ro reviewed Aug 9, 2018

View reviewed changes

af4ro added status: duplicate duplicate issue and removed status: code review request requesting a community code review or review from Twilio labels Aug 13, 2018

Giaco9 mentioned this pull request Aug 15, 2018

Add Dynamic Template Support #591

Closed

thinkingserious added status: work in progress Twilio or the community is in the process of implementing and removed status: duplicate duplicate issue labels Aug 16, 2018

thinkingserious merged commit e1cc256 into sendgrid:master Aug 21, 2018

thinkingserious added a commit that referenced this pull request Aug 21, 2018

Merge #593

28bd2a5

0bsearch deleted the dynamic_template_support branch August 21, 2018 07:47

PiDelport reviewed Aug 22, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for dynamic template data in personalizations #593

Adds support for dynamic template data in personalizations #593

0bsearch commented Aug 1, 2018 •

edited by thinkingserious

Loading

codecov bot commented Aug 1, 2018 •

edited

Loading

af4ro commented Aug 9, 2018

af4ro Aug 9, 2018 •

edited

Loading

af4ro Aug 9, 2018

0bsearch Aug 13, 2018

thinkingserious commented Aug 21, 2018

PiDelport Aug 22, 2018

thinkingserious Aug 22, 2018

djw27 commented Aug 29, 2018

0bsearch commented Aug 29, 2018

thinkingserious commented Aug 29, 2018

djw27 commented Aug 30, 2018 •

edited

Loading

thinkingserious commented Sep 5, 2018

Corey-Evans commented Nov 15, 2018

Adds support for dynamic template data in personalizations #593

Adds support for dynamic template data in personalizations #593

Conversation

0bsearch commented Aug 1, 2018 • edited by thinkingserious Loading

Checklist

Short description of what this PR does:

codecov bot commented Aug 1, 2018 • edited Loading

Codecov Report

af4ro commented Aug 9, 2018

af4ro Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

af4ro Aug 9, 2018

Choose a reason for hiding this comment

0bsearch Aug 13, 2018

Choose a reason for hiding this comment

thinkingserious commented Aug 21, 2018

PiDelport Aug 22, 2018

Choose a reason for hiding this comment

thinkingserious Aug 22, 2018

Choose a reason for hiding this comment

djw27 commented Aug 29, 2018

0bsearch commented Aug 29, 2018

thinkingserious commented Aug 29, 2018

djw27 commented Aug 30, 2018 • edited Loading

thinkingserious commented Sep 5, 2018

Corey-Evans commented Nov 15, 2018

0bsearch commented Aug 1, 2018 •

edited by thinkingserious

Loading

codecov bot commented Aug 1, 2018 •

edited

Loading

af4ro Aug 9, 2018 •

edited

Loading

djw27 commented Aug 30, 2018 •

edited

Loading