Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: use email-templates module for rendering emails #1245

Closed
wants to merge 1 commit into from
Closed

feature: use email-templates module for rendering emails #1245

wants to merge 1 commit into from

Conversation

clarkbw
Copy link

@clarkbw clarkbw commented Mar 26, 2015

I had some time on the bus this morning and have been meaning to do this. Here's what it is:

I'm proposing moving to the email-templates module for email templates in loopback. There exists a loopback.template function which will load ejs templates from a path but its limited to ejs and requires lots of options passed into it; the User.verify() function could be simplified a bit more with this change.

With the change to email-templates we go from a single ejs file to a directory where a number of different template engines can be used by the developer (assuming they npm -S install them).

The current template directory structure looks like this:

  • templates/verify.ejs

With my proposed change loopback would move to a directory structure and files within:

  • templates/verify/
    • html.ejs
    • text.ejs
    • style.css

However it could also look like this if a developer wanted:

  • templates/verify/
    • html.handlebars
    • text.handlebars
    • style.less

As can obviously be seen the text and html representations can now be specified as ejs or other files and as you can see from the commit, my text.ejs can contain the default text so the User.verify code doesn't have to. I think it will be a bit easier for new developers to make these kinds of changes.

I'll be putting up a new repo very soon with this as an example but my personal plan is to include the CSS that I use for the main site, found in the client directory within my emails as well.

So the example repo will look like this:

  • client/css/
    • bootstrap.scss
    • main.scss
  • templates/verify/
    • html.ejs
    • text.ejs
    • style.scss

Allowing my email templates to include the main.scss file from my client directory so they always have the main branding and other variables used within the HTML emails. email-templates uses the juice module to inline the used CSS you import, it is designed to be used with emails where stylesheets don't work.

Obvious things this is missing so far:

  • Tests, all current tests pass with the code changes I've made but I need to write some new tests
  • Backwards compatibility with the old template system and the options that were passed. I don't think this will be too bad to do, though I'd rather not do it so if given the go ahead a breaking change here would be nice.
  • The lazy load of the template is unnecessary but I didn’t find a way to make sure my async call finishes at this stage of initialization.

Thoughts?

@slnode
Copy link

slnode commented Mar 26, 2015

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@clarkbw
Copy link
Author

clarkbw commented Mar 26, 2015

signed the CLA on the first try @rmg!

@clarkbw
Copy link
Author

clarkbw commented Mar 27, 2015

Published the https://github.com/clarkbw/loopback-example-email-templates repo to show an example of an email template sharing the same LESS/CSS as the client application.

@clarkbw
Copy link
Author

clarkbw commented Apr 1, 2015

passing all the tests now, I should have documented that dive into browserify for anyone else who wants to add a server only feature.

@bradplank
Copy link

+1 Sounds like a nice addition, as I currently use email-templates with handlebars and less as well. (Password reset emails though.)

I haven’t done the work to ensure backwards compatibility with the old
template system and the options that were passed.  Will work on some
tests for that soon.

The lazy load might be unnecessary but I didn’t find a way to make
async calls at this stage of initialization.
@slnode
Copy link

slnode commented May 28, 2015

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@rmg
Copy link
Member

rmg commented May 28, 2015

ok to test

@clarkbw
Copy link
Author

clarkbw commented May 29, 2015

I think I've come around and decided to stop this work, I am going to continue the effort but outside the loopback module. I can't find it now, but I believe I read @ritch somewhere say that he was interested in pulling out a lot of the User model functionality in favor of having it as a separate piece. And assuming I'm giving credit correctly I agree with that approach. Pulling all this code into loopback doesn't seem right when I could use the framework to only pull it in as needed and keep loopback smaller and simpler.

I'm working on a new module that handles user verification, password reset, etc all using the email template system I began here. I've been looking though the code in the loopback-faq-user-management and I'm planning on doing something similar but as a module you can import to the framework.

I'm closing the pull request here but I'd appreciate any comments or suggestions from @ritch @raymondfeng @rmg @bajtos or anyone else on how to organize this future module. Currently I'm looking at using a mixin for the User model that would add in various remote functions related to user management (verify, reset, create) with emails and templates to match.

@bajtos
Copy link
Member

bajtos commented Jun 1, 2015

+1 for implementing these features in a standalone module (a LoopBack component). I think the plan to provide a mixin is a good one.

@clarkbw
Copy link
Author

clarkbw commented Jun 10, 2015

Still a bunch of work to get through but I've started in a direction that follows how the resetPassword method works and looks pretty good so far. 64c08c7

The User model will emit events for each user interaction where communication with the user may be desired. Each event should carry with it enough information needed to communicate with a given user if needed.

That list of events looks like this:

  • login:success
  • login:failure
  • logout:success
  • logout:failure

With documented events like those I can create a new module that hooks into each event and provides a template email system for responding to different event types.

I'll open a new issue to track the progress of this work as I go.

@fabien
Copy link
Contributor

fabien commented Jun 11, 2015

@clarkbw perhaps you should consider using operation hooks instead. While emitting events is fire-and-forget, operation hooks can asynchronously handle events, but still provide feedback if necessary.

@clarkbw
Copy link
Author

clarkbw commented Jun 12, 2015

@fabien I looked at using operation hooks but as far as I could see only the remote hooks would really be possible and then would limit me from using User methods directly. I would like your idea of being able to provide feedback and use the operation hook model but I don't see yet how to make it work.

Here are my notes from investigating those options:

Remote Hooks

The remote hooks seem to provide enough control and access to methods as well as CRUD operations that I could see using those. However they would limit the interaction to remote methods instead of all operations on the models.

Options

  • beforeRemote
  • afterRemote
  • afterRemoteError

With User.afterRemote('prototype.login') I could detect a successful login and use the token provided to know which user has logged in if communication was needed.

Something like User.afterRemoteError('prototype.login') would work well to detect a failed login and I could probably improve the error reporting such that you could determine what information failed and why.

However if a system doesn't use a remote method directly but internally calls those methods there are no hooks to watch those changes. For example in my system I've created an end point for a webhook which does some processing and then calls various functions on the User model and because the calls are not remote methods these hooks won't be triggered.

Operation Hooks

For the operation hooks it looks like it would leave me to infer methods used based on properties being changed in the database.

Options

  • User.observe('before save')
  • User.observe('after save')
  • User.observe('access')

When I looked at the User.resetPassword method it looks like I would need to watch for changes to the accessTokens that match the short lived TTL. 🌁 Something like this:

AccessToken.observe('after save', function (ctx, next) {
  // copied from user.js
  var DEFAULT_RESET_PW_TTL = 15 * 60; // 15 mins in seconds

  var UserModel = AccessToken.models.User;
  var token = ctx.instance;
  if (token.ttl === UserModel.settings.resetPasswordTokenTTL || token.ttl === DEFAULT_RESET_PW_TTL) {
    // was likely password reset for token.user
  }
});

Other functions like login and logout seem to behave in a similar way such that I would be monitoring accessTokens for clues as they are removed and added to the system.

Do you have any suggestion on how to make operation hooks work? Its very likely I missed something that could make it happen.

@fabien
Copy link
Contributor

fabien commented Jun 12, 2015

Actually, I meant triggering your own hooks using notifyObserversOf (async, in-the-loop) instead of emitting events (sync/fire-and-forget).

@fabien I looked at using operation hooks but as far as I could see only the remote hooks would really be possible and then would limit me from using User methods directly. I would like your idea of being able to provide feedback and use the operation hook model but I don't see yet how to make it work.

Here are my notes from investigating those options:

Remote Hooks

The remote hooks seem to provide enough control and access to methods as well as CRUD operations that I could see using those. However they would limit the interaction to remote methods instead of all operations on the models.

Options

beforeRemote
afterRemote
afterRemoteError
With User.afterRemote('prototype.login') I could detect a successful login and use the token provided to know which user has logged in if communication was needed.

Something like User.afterRemoteError('prototype.login') would work well to detect a failed login and I could probably improve the error reporting such that you could determine what information failed and why.

However if a system doesn't use a remote method directly but internally calls those methods there are no hooks to watch those changes. For example in my system I've created an end point for a webhook which does some processing and then calls various functions on the User model and because the calls are not remote methods these hooks won't be triggered.

Operation Hooks

For the operation hooks it looks like it would leave me to infer methods used based on properties being changed in the database.

Options

User.observe('before save')
User.observe('after save')
User.observe('access')
When I looked at the User.resetPassword method it looks like I would need to watch for changes to the accessTokens that match the short lived TTL. Something like this:

AccessToken.observe('after save', function (ctx, next) {
// copied from user.js
var DEFAULT_RESET_PW_TTL = 15 * 60; // 15 mins in seconds

var UserModel = AccessToken.models.User;
var token = ctx.instance;
if (token.ttl === UserModel.settings.resetPasswordTokenTTL || token.ttl === DEFAULT_RESET_PW_TTL) {
// was likely password reset for token.user
}
});
Other functions like login and logout seem to behave in a similar way such that I would be monitoring accessTokens for clues as they are removed and added to the system.

Do you have any suggestion on how to make operation hooks work? Its very likely I missed something that could make it happen.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants