Email notifications on new comments

thoughtbot / carnival

An unobtrusive, developer-friendly way to add comments

MIT License

499 stars 30 forks source link

Email notifications on new comments #100

Closed pbrisbin closed 9 years ago

pbrisbin commented 9 years ago

Some (tested) logic for building a list of recipients and a body
Added the smtp-mail package but didn't get around to using it yet
Various improvements to the testing harness

jferris commented 9 years ago

Any chance we can separate out the testing improvements into their own pull request to be merged first?

pbrisbin commented 9 years ago

Most definitely. They're not ready yet either though. Once they are, I'll break them out.

pbrisbin commented 9 years ago

Figured the clear-tables change was probably good enough to be its own PR: #102

pbrisbin commented 9 years ago

This is ready for a closer look. It can be deployed to staging and/or production as long as we set the EMAIL_RECIPIENTS override. I don't want to actually turn it on until I have unsubscribe in place though.

jferris commented 9 years ago

I'd like to review this in-person at tonight's project night.

pbrisbin commented 9 years ago

@jferris I switched to using a whitelist for subscriptions, refactored how recipients are built, moved the mail content to a template, and broke out generic, separately-shippable modules for some mail stuff.

This is ready for a second look.

I also experimented with a new way to split model concerns (see Model.User and Model.Subscription), I'm curious what you think. If we like it, I want to replace Helpers.Comment by making a Model.Comment and splitting concerns between it and the Handlers in a similar way.

jferris commented 9 years ago

This is looking great. I like the new Model organization.