Add the ability for mailmason to generate and upload/update templates.
#3
Conversation
if server with the specified name exists, its config is returned in exactly the same form as a create response.
|
Would be great to hear what @hybernaut thinks of this. Couldn't wait on #1 to be merged so I had to roll my own solution for now, though I'll gladly move to whatever goes onto |
|
@derekrushforth @hybernaut Any comments on this or ActiveCampaign/mailmason#34? |
There was a problem hiding this comment.
Hey @randytarampi,
Thanks for taking the time to make this better!
Took a first pass at testing this and ran into a few issues with the default configuration. Take a look at my notes and let me know what you think. I'm also having issues generating the template output file, but I'll hold off on posting those details once we streamline the configuration a bit.
Happy to jump in and make changes to the codebase if you don't have time. Just let me know!
example_config.json
Outdated
| // | ||
| // 2. Delete these comments from the configuration so Grunt doesn't get confused | ||
| // | ||
| // 3. Visit the following page for details: |
There was a problem hiding this comment.
Let's remove this step since grunt-postmark isn't dependent on MailMason.
example_secrets.json
Outdated
| // | ||
| // 2. Delete these comments from the configuration so Grunt doesn't get confused | ||
| // | ||
| // 3. Visit the following page for details: |
There was a problem hiding this comment.
Let's remove this step since grunt-postmark isn't dependent on MailMason.
| @@ -20,6 +20,8 @@ After the plugin is installed, it can be enabled in your Gruntfile: | |||
| grunt.loadNpmTasks('grunt-postmark'); | |||
| ``` | |||
|
|
|||
| You'll need to add a [`config.json`](https://github.com/wildbit/mailmason/wiki/Getting-Started#create-configjson-required) and a [`secrets.json`](https://github.com/wildbit/mailmason/wiki/Getting-Started#create-secretsjson-optional) per the `mailmason` configuration. | |||
There was a problem hiding this comment.
There are several details in this wiki entry that aren't relevant to grunt-postmark so this might confuse people who aren't using MailMason.
We should replace this with setup instructions specific to grunt-postmark. I can create the wiki page and link it once we've merged this PR.
Gruntfile.js
Outdated
| }, | ||
|
|
||
| // you can either specify the template configuration here, or in templates.json | ||
| 'postmark-templates-upload': config.templates && config.templates |
There was a problem hiding this comment.
Optionally storing the template data in a separate JSON file is a good idea, but this might be a roadblock for people who are configuring this for the first time.
example_config.json references templates.json so I ran into issues during setup because it wasn't clear whether the plugin was using the data from the template config or from the target config.
Instead of reading from templates.json we should remove this and document it as an optional step. This would reduce the friction during the initial setup process.
tasks/grunt-postmark-templates.js
Outdated
| grunt.fail.warn('Missing Postmark server token \n'); | ||
| } | ||
|
|
||
| if (!template.Name) { |
There was a problem hiding this comment.
Let's use camel case variables instead of title case. Same goes for subject, htmlBody, and textBody validation.
I was using the default variables specified from the gruntfile and kept running into validation errors because the variables didn't match.
Gruntfile.js
Outdated
| test_email: { | ||
| name: 'testing-postmark-templates-js1-' + new Date().valueOf(), | ||
| subject: 'Testing grunt-postmark-templates', | ||
| htmlBody: 'test/email.html', |
There was a problem hiding this comment.
Looks like the upload task is writing the string to the Postmark template instead of reading and posting the contents of the file.
Am I missing something?
|
Hey @randytarampi, just checking in to see how this is going. Let me know and I'll help out with some of these code changes. |
|
Whoops. Just saw this email now. :\ I'll get these fixes done this weekend. |
Per @derekrushforth's comments on ActiveCampaign#3. [This](ActiveCampaign#3 (comment)) will break things for folks who are already using these changes (@gcoombe and @jmas) as we're now looking for `lowerCamel` variable names instead of `Title` cased ones. Just means a quick find/replace for `Name`/`name, `Subject`/`subject`, etc. I've internally changed the implementation of `postmark-templates-upload` to support in-memory `htmlBody` and `textBody` as well as lazy loaded paths `htmlSrc` and `textSrc` as @hybernaut originally had it. I didn't actually notice how far my changes had deviated from the default options for the `postmark-templates-upload` task when I filed the PR, but I actually like the idea of loading the template files instead of expecting them to be passed along as task data.
Per @derekrushforth's comments on ActiveCampaign/grunt-postmark#3. [This](ActiveCampaign/grunt-postmark#3 (comment)) will break things for folks who are already using these changes (@gcoombe and @jmas) as we're now looking for `lowerCamel` variable names instead of `Title` cased ones. Just means a quick find/replace for `Name`/`name, `Subject`/`subject`, etc. I've internally changed the implementation of `postmark-templates-upload` to support in-memory `htmlBody` and `textBody` as well as lazy loaded paths `htmlSrc` and `textSrc` as @hybernaut originally had it. I didn't actually notice how far my changes had deviated from the default options for the `postmark-templates-upload` task when I filed the PR, but I actually like the idea of loading the template files instead of expecting them to be passed along as task data.
|
Hey @derekrushforth, still around, or have you taken off for the holidays yet? Any comments on my last changeset? |
|
Hey @randytarampi, sorry to keep you waiting. I'm back from holiday vacation now so I'll take a look at your latest changes. |
|
Looking great @randytarampi! Thanks for taking the time to make these changes. After testing the workflow some more, I think the ideal approach is using the JSON template config right off the bat and using that as the source of truth rather than using the task target config. I know I asked you to remove the logic from the gruntfile, but we should make this super clear in the setup instructions. Do you want to take a stab at adding instructions to the readme? If you don't have time I can always merge the PR and update the readme separately. |
|
Can you also bump the version in package.json too? |
|
Cool. I'll write things up this weekend. |
Support both the original task target based upload that `ActiveCampaign/mailmason#34` is dependent on, as well as a stripped down version of its [`postmark-templates-generate`](ActiveCampaign/mailmason@9af44cc#diff-84d8d4faaac8ca7bfe33ab2e934307c1R9) task, which just blindly reads email template configurations and applies them as task target configuration for `postmark-templates-upload`. I'm not too keen on the duplication between this and mailmason, but I don't know how else I'd iron things out other than by making mailmason write a file in its `postmark-templates-generate`, but that seems overly wasteful.
And its constituent and related tasks – `postmark-templates-upload`, `postmark-templates-output`, `postmark-templates-parse`, `postmark-templates-from-targets`, and `postmark-templates-from-file`. Per ActiveCampaign#3 (comment).
My take on #1 , but built in a less standalone way and more to support a mailmason centric workflow – this is coupled heavily/ a prerequisite with ActiveCampaign/mailmason#34.
I've reworked a couple of things here:
postmark-templatesbecomespostmark-templates-uploadpostmark-templates-uploadis now a task that reads someHtmlBodyandTextBodyfrom the templates file instead of reading a file specified in the target configuration.postmark-templates-outputas dumb as possible, and to facilitate an easier, more transparent coupling withmailmason(via a proposedpostmark-templates-generatetask).templates.jsonfile clean, you can specify aconfig.templates.clean_outputoption which will not write theHtmlBodyandTextBodyback out.