Work in Progress: Reply By Email:

- Add support classes and settings to enable reply by email
- Split out Email builder to be more OO, add many specs

Gemfile

@@ -29,6 +29,8 @@ gem 'fog', require: false
 gem 'has_ip_address'
 gem 'hiredis'
 
+gem 'email_reply_parser'
+
 # note: for image_optim to correctly work you need
 # sudo apt-get install -y advancecomp gifsicle jpegoptim libjpeg-progs optipng pngcrush
 gem 'image_optim'

Gemfile.lock

@@ -165,6 +165,7 @@ GEM
     diffy (2.1.4)
     em-redis (0.3.0)
       eventmachine
+    email_reply_parser (0.5.3)
     ember-rails (0.10.0)
       active_model_serializers
       barber
@@ -490,6 +491,7 @@ DEPENDENCIES
   discourse_emoji!
   discourse_plugin!
   em-redis
+  email_reply_parser
   ember-rails
   ember-source (= 1.0.0.rc5)
   eventmachine

app/assets/javascripts/admin/templates/email_logs.js.handlebars

@@ -4,6 +4,7 @@
     <th>{{i18n user.title}}</th>
     <th>{{i18n admin.email.to_address}}</th>
     <th>{{i18n admin.email.email_type}}</th>
+    <th>{{i18n admin.email.reply_key}}</th>
   </tr>
 
   {{#if model.length}}
@@ -20,6 +21,7 @@
           </td>
           <td><a href='mailto:{{unbound to_address}}'>{{to_address}}</a></td>
           <td>{{email_type}}</td>
+          <td>{{reply_key}}</td>
       {{/collection}}
     {{/group}}
   {{/if}}

app/mailers/invite_mailer.rb

@@ -1,21 +1,18 @@
-require_dependency 'email/builder'
+require_dependency 'email/message_builder'
 
 class InviteMailer < ActionMailer::Base
-  default charset: 'UTF-8'
-  include Email::Builder
+  include Email::BuildEmailHelper
 
   def send_invite(invite)
-
     # Find the first topic they were invited to
     first_topic = invite.topics.order(:created_at).first
 
     # If they were invited to a topic
-    build_email invite.email,
-                'invite_mailer',
+    build_email(invite.email,
+                      template: 'invite_mailer',
                       invitee_name: invite.invited_by.username,
                       invite_link: "#{Discourse.base_url}/invites/#{invite.invite_key}",
-                topic_title: first_topic.try(:title)
-
+                      topic_title: first_topic.try(:title))
   end
 
 end

app/mailers/test_mailer.rb

@@ -1,11 +1,9 @@
-require_dependency 'email/builder'
+require_dependency 'email/message_builder'
 
 class TestMailer < ActionMailer::Base
-  default charset: 'UTF-8'
-  include Email::Builder
+  include Email::BuildEmailHelper
 
   def send_test(to_address)
-    build_email to_address, 'test_mailer'
+    build_email(to_address, template: 'test_mailer')
   end
-
 end

app/mailers/user_notifications.rb

@@ -1,43 +1,43 @@
 require_dependency 'markdown_linker'
-require_dependency 'email/builder'
+require_dependency 'email/message_builder'
 
 class UserNotifications < ActionMailer::Base
   default charset: 'UTF-8'
 
-  include Email::Builder
+  include Email::BuildEmailHelper
 
   def signup(user, opts={})
-    build_email(user.email, "user_notifications.signup", email_token: opts[:email_token])
+    build_email(user.email,
+                      template: "user_notifications.signup",
+                      email_token: opts[:email_token])
   end
 
   def signup_after_approval(user, opts={})
-    build_email(
-      user.email,
-      'user_notifications.signup_after_approval',
+    build_email(user.email,
+                      template: 'user_notifications.signup_after_approval',
                       email_token: opts[:email_token],
-      new_user_tips: SiteContent.content_for(:usage_tips)
-    )
+                      new_user_tips: SiteContent.content_for(:usage_tips))
   end
 
   def authorize_email(user, opts={})
-    build_email(user.email, "user_notifications.authorize_email", email_token: opts[:email_token])
+    build_email(user.email, template: "user_notifications.authorize_email", email_token: opts[:email_token])
   end
 
   def forgot_password(user, opts={})
-    build_email(user.email, "user_notifications.forgot_password", email_token: opts[:email_token])
+    build_email(user.email, template: "user_notifications.forgot_password", email_token: opts[:email_token])
   end
 
   def private_message(user, opts={})
     post = opts[:post]
 
     build_email user.email,
-                "user_notifications.private_message",
+                      template: "user_notifications.private_message",
                       message: post.raw,
                       url: post.url,
                       subject_prefix: "[#{I18n.t('private_message_abbrev')}] #{post.post_number != 1 ? 're: ' : ''}",
                       topic_title: post.topic.title,
                       private_message_from: post.user.name,
-                from: "#{I18n.t(:via, username: post.user.name, site_name: SiteSetting.title)} <#{SiteSetting.notification_email}>",
+                      from_alias: I18n.t(:via, username: post.user.name, site_name: SiteSetting.title),
                       add_unsubscribe_link: true
   end
 
@@ -57,8 +57,8 @@ class UserNotifications < ActionMailer::Base
 
     # Don't send email unless there is content in it
     if @new_topics.present?
-      mail to: user.email,
-           from: "#{I18n.t('user_notifications.digest.from', site_name: SiteSetting.title)} <#{SiteSetting.notification_email}>",
+      build_email user.email,
+                        from_alias: I18n.t('user_notifications.digest.from', site_name: SiteSetting.title),
                         subject: I18n.t('user_notifications.digest.subject_template',
                                         site_name: @site_name,
                                         date: I18n.l(Time.now, format: :short))
@@ -80,16 +80,16 @@ class UserNotifications < ActionMailer::Base
       message: @post.raw,
       url: @post.url,
       username: username,
-      add_unsubscribe_link: true
+      add_unsubscribe_link: true,
+      template: "user_notifications.user_#{notification_type}"
     }
 
     # If we have a display name, change the from address
     if username.present?
-      aliased = I18n.t(:via, username: username, site_name: SiteSetting.title)
-      email_opts[:from] = "#{aliased} <#{SiteSetting.notification_email}>"
+      email_opts[:from_alias] = I18n.t(:via, username: username, site_name: SiteSetting.title)
     end
 
-    email = build_email user.email, "user_notifications.user_#{notification_type}", email_opts
+    build_email(user.email, email_opts)
   end
 
   alias :user_invited_to_private_message :notification_template

app/models/email_log.rb

@@ -3,6 +3,13 @@ class EmailLog < ActiveRecord::Base
   validates_presence_of :email_type
   validates_presence_of :to_address
 
+  before_create do
+    # We only generate a reply
+    if SiteSetting.reply_by_email_enabled?
+      self.reply_key = SecureRandom.hex(16)
+    end
+  end
+
   after_create do
     # Update last_emailed_at if the user_id is present
     User.update_all("last_emailed_at = CURRENT_TIMESTAMP", id: user_id) if user_id.present?

app/models/site_setting.rb

@@ -188,6 +188,10 @@ class SiteSetting < ActiveRecord::Base
   setting(:regular_requires_likes_given, 1)
   setting(:regular_requires_topic_reply_count, 3)
 
+  # Reply by Email Settings
+  setting(:reply_by_email_enabled, false)
+  setting(:reply_by_email_address, nil)
+
   # Entropy checks
   setting(:title_min_entropy, 10)
   setting(:body_min_entropy, 7)

app/serializers/email_log_serializer.rb

@@ -1,6 +1,12 @@
 class EmailLogSerializer < ApplicationSerializer
 
-  attributes :id, :to_address, :email_type, :user_id, :created_at
+  attributes :id,
+             :reply_key,
+             :to_address,
+             :email_type,
+             :user_id,
+             :created_at
+
   has_one :user, serializer: BasicUserSerializer, embed: :objects
 
 end

config/locales/client.en.yml

@@ -1086,6 +1086,7 @@ en:
         html: "html"
         text: "text"
         last_seen_user: "Last Seen User:"
+        reply_key: "Reply Key"
 
       impersonate:
         title: "Impersonate User"

config/locales/server.en.yml

@@ -612,6 +612,9 @@ en:
     newuser_spam_host_threshold: "How many times a new user can post a link to the same host within their `newuser_spam_host_posts` posts before being considered spam."
     staff_like_weight: "Extra weighting factor given to likes when performed by staff."
 
+    reply_by_email_enabled: "Whether this forum supports reply by email"
+    reply_by_email_address: "Template for reply by email address in form, for example: %{reply_key}@reply.myforum.com"
+
   notification_types:
     mentioned: "%{display_username} mentioned you in %{link}"
     liked: "%{display_username} liked your post in %{link}"

db/migrate/20130610201033_add_reply_key_to_email_logs.rb

@@ -0,0 +1,6 @@
+class AddReplyKeyToEmailLogs < ActiveRecord::Migration
+  def change
+    add_column :email_logs, :reply_key, :string, limit: 32
+    add_index :email_logs, :reply_key
+  end
+end

lib/email.rb

@@ -1,5 +1,5 @@
 require 'mail'
-require_dependency 'email/builder'
+require_dependency 'email/message_builder'
 require_dependency 'email/renderer'
 require_dependency 'email/sender'
 require_dependency 'email/styles'

lib/email/builder.rb

@@ -1,32 +0,0 @@
-# Help us build an email
-module Email
-
-  module Builder
-
-    def build_email(to, email_key, params={})
-      params[:site_name] = SiteSetting.title
-      params[:base_url] = Discourse.base_url
-      params[:user_preferences_url] = "#{Discourse.base_url}/user_preferences"
-
-      body = I18n.t("#{email_key}.text_body_template", params)
-
-      # Are we appending an unsubscribe link?
-      if params[:add_unsubscribe_link]
-        body << "\n"
-        body << I18n.t("unsubscribe_link", params)
-        headers 'List-Unsubscribe' => "<#{params[:user_preferences_url]}>"
-      end
-
-      mail_args = {
-        to: to,
-        subject: I18n.t("#{email_key}.subject_template", params),
-        body: body
-      }
-      mail_args[:from] = params[:from] || SiteSetting.notification_email
-      mail_args[:charset] = 'UTF-8'
-      mail(mail_args)
-    end
-
-  end
-
-end

lib/email/incoming_message.rb

@@ -0,0 +1,19 @@
+module Email
+
+  class IncomingMessage
+
+    attr_reader :reply_key,
+                :body_plain
+
+    def initialize(reply_key, body)
+      @reply_key = reply_key
+      @body = body
+    end
+
+    def reply
+      @reply ||= EmailReplyParser.read(@body).visible_text
+    end
+
+  end
+
+end

lib/email/message_builder.rb

@@ -0,0 +1,68 @@
+# Builds a Mail::Mesage we can use for sending. Optionally supports using a template
+# for the body and subject
+module Email
+
+  module BuildEmailHelper
+    def build_email(*builder_args)
+      builder = Email::MessageBuilder.new(*builder_args)
+      headers(builder.header_args) if builder.header_args.present?
+      mail(builder.build_args)
+    end
+  end
+
+  class MessageBuilder
+
+    def initialize(to, opts=nil)
+      @to = to
+      @opts = opts || {}
+    end
+
+    def subject
+      subject = @opts[:subject]
+      subject = I18n.t("#{@opts[:template]}.subject_template", template_args) if @opts[:template]
+      subject
+    end
+
+    def body
+      body = @opts[:body]
+      body = I18n.t("#{@opts[:template]}.text_body_template", template_args) if @opts[:template]
+
+      if @opts[:add_unsubscribe_link]
+        body << "\n"
+        body << I18n.t('unsubscribe_link', template_args)
+      end
+
+      body
+    end
+
+    def template_args
+      @template_args ||= { site_name: SiteSetting.title,
+                           base_url: Discourse.base_url,
+                           user_preferences_url: "#{Discourse.base_url}/user_preferences" }.merge!(@opts)
+    end
+
+    def build_args
+      mail_args = { to: @to,
+                    subject: subject,
+                    body: body,
+                    charset: 'UTF-8' }
+
+      mail_args[:from] = @opts[:from] || SiteSetting.notification_email
+
+      if @opts[:from_alias]
+        mail_args[:from] = "#{@opts[:from_alias]} <#{mail_args[:from]}>"
+      end
+      mail_args
+    end
+
+    def header_args
+      result = {}
+      if @opts[:add_unsubscribe_link]
+        result['List-Unsubscribe'] = "<#{template_args[:user_preferences_url]}>" if @opts[:add_unsubscribe_link]
+      end
+      result
+    end
+
+  end
+
+end

lib/email/receiver.rb

@@ -0,0 +1,30 @@
+#
+# Handles an incoming message
+#
+require_dependency 'email/incoming_message'
+
+module Email
+  class Receiver
+
+    def self.results
+      @results ||= Enum.new(:unprocessable)
+    end
+
+    def initialize(incoming_message)
+      @incoming_message = incoming_message
+    end
+
+    def process
+
+      if @incoming_message.blank? || @incoming_message.reply_key.blank?
+        return Email::Receiver.results[:unprocessable]
+      end
+
+      log = EmailLog.where(reply_key: @incoming_message.reply_key).first
+      return Email::Receiver.results[:unprocessable] if log.blank?
+
+      nil
+    end
+
+  end
+end

lib/jobs/user_email.rb

@@ -47,17 +47,7 @@ module Jobs
         return if notification.read?
       end
 
-      # Check that the post has not been deleted or read
-      if email_args[:post]
-
-        post = email_args[:post]
-
-        # Don't email about deleted topics or user deleted posts
-        return if post.topic.blank? || post.user_deleted?
-
-        # Don't send the email if the user has read the post
-        return if PostTiming.where(topic_id: post.topic_id, post_number: post.post_number, user_id: user.id).present?
-      end
+      return if skip_email_for_post(email_args[:post], user)
 
       # Make sure that mailer exists
       raise Discourse::InvalidParameters.new(:type) unless UserNotifications.respond_to?(args[:type])
@@ -70,7 +60,16 @@ module Jobs
       end
 
       Email::Sender.new(message, args[:type], user).send
+    end
 
+    private
+
+    # If this email has a related post, don't send an email if it's been deleted or seen recently.
+    def skip_email_for_post(post, user)
+      post &&
+      (post.topic.blank? ||
+       post.user_deleted? ||
+       PostTiming.where(topic_id: post.topic_id, post_number: post.post_number, user_id: user.id).present?)
     end
 
   end

spec/components/email/incoming_email_spec.rb

@@ -0,0 +1,16 @@
+require 'spec_helper'
+require 'email/receiver'
+
+describe Email::IncomingMessage do
+
+  let(:message) { Email::IncomingMessage.new("asdf", "hello\n\n> how are you?") }
+
+  it "returns the reply_key" do
+    expect(message.reply_key).to eq("asdf")
+  end
+
+  it "extracts the reply" do
+    expect(message.reply).to eq("hello")
+  end
+
+end

spec/components/email/message_builder_spec.rb

@@ -0,0 +1,121 @@
+require 'spec_helper'
+require 'email/message_builder'
+
+describe Email::MessageBuilder do
+
+  let(:to_address) { "jake@adventuretime.ooo" }
+  let(:subject) { "Tree Trunks has made some apple pie!" }
+  let(:body) { "oh my glob Jake, Tree Trunks just made the tastiest apple pie ever!"}
+  let(:builder) { Email::MessageBuilder.new(to_address, subject: subject, body: body) }
+  let(:build_args) { builder.build_args }
+
+  it "has the correct to address" do
+    expect(build_args[:to]).to eq(to_address)
+  end
+
+  it "has the subject" do
+    expect(builder.subject).to eq(subject)
+  end
+
+  it "has the body" do
+    expect(builder.body).to eq(body)
+  end
+
+  it "has a utf-8 charset" do
+    expect(builder.build_args[:charset]).to eq("UTF-8")
+  end
+
+  context "unsubscribe link" do
+
+    context "with add_unsubscribe_link false" do
+      it "has no unsubscribe header by default" do
+        expect(builder.header_args['List-Unsubscribe']).to be_blank
+      end
+
+      it "doesn't have the user preferences url in the body" do
+        expect(builder.body).not_to match(builder.template_args[:user_preferences_url])
+      end
+
+    end
+
+    context "with add_unsubscribe_link true" do
+
+      let(:message_with_unsubscribe) { Email::MessageBuilder.new(to_address,
+                                                                body: 'hello world',
+                                                                add_unsubscribe_link: true) }
+
+      it "has an List-Unsubscribe header" do
+        expect(message_with_unsubscribe.header_args['List-Unsubscribe']).to be_present
+      end
+
+      it "has the user preferences url in the body" do
+        expect(message_with_unsubscribe.body).to match(builder.template_args[:user_preferences_url])
+      end
+
+    end
+
+  end
+
+  context "template_args" do
+    let(:template_args) { builder.template_args }
+
+    it "has the site name" do
+      expect(template_args[:site_name]).to eq(SiteSetting.title)
+    end
+
+    it "has the base url" do
+      expect(template_args[:base_url]).to eq(Discourse.base_url)
+    end
+
+    it "has the user_preferences_url" do
+      expect(template_args[:user_preferences_url]).to eq("#{Discourse.base_url}/user_preferences")
+    end
+  end
+
+  context "subject_template" do
+
+    let(:templated_builder) { Email::MessageBuilder.new(to_address, template: 'mystery') }
+    let(:rendered_template) { "rendered template" }
+
+    it "has the body rendered from a template" do
+      I18n.expects(:t).with("mystery.text_body_template", templated_builder.template_args).returns(rendered_template)
+      expect(templated_builder.body).to eq(rendered_template)
+    end
+
+    it "has the subject rendered from a template" do
+      I18n.expects(:t).with("mystery.subject_template", templated_builder.template_args).returns(rendered_template)
+      expect(templated_builder.subject).to eq(rendered_template)
+    end
+
+  end
+
+  context "from field" do
+
+    it "has the default from" do
+      expect(build_args[:from]).to eq(SiteSetting.notification_email)
+    end
+
+    let(:finn_email) { 'finn@adventuretime.ooo' }
+    let(:custom_from) { Email::MessageBuilder.new(to_address, from: finn_email).build_args }
+
+    it "allows us to override from" do
+      expect(custom_from[:from]).to eq(finn_email)
+    end
+
+    let(:aliased_from) { Email::MessageBuilder.new(to_address, from_alias: "Finn the Dog") }
+
+    it "allows us to alias the from address" do
+      expect(aliased_from.build_args[:from]).to eq("Finn the Dog <#{SiteSetting.notification_email}>")
+    end
+
+    let(:custom_aliased_from) { Email::MessageBuilder.new(to_address,
+                                                          from_alias: "Finn the Dog",
+                                                          from: finn_email) }
+
+    it "allows us to alias a custom from address" do
+      expect(custom_aliased_from.build_args[:from]).to eq("Finn the Dog <#{finn_email}>")
+    end
+
+  end
+
+end

spec/components/email/receiver_spec.rb

@@ -0,0 +1,19 @@
+require 'spec_helper'
+require 'email/receiver'
+
+describe Email::Receiver do
+
+
+  describe 'invalid key' do
+    let(:incoming) { Email::IncomingMessage.new('asdf', 'hello') }
+
+    it "returns unprocessable for nil message" do
+      expect(Email::Receiver.new(nil).process).to eq(Email::Receiver.results[:unprocessable])
+    end
+
+    it "returns unprocessable for a made up key" do
+      expect(Email::Receiver.new(incoming).process).to eq(Email::Receiver.results[:unprocessable])
+    end
+  end
+
+end

spec/mailers/user_notifications_spec.rb

@@ -22,7 +22,7 @@ describe UserNotifications do
     its(:body) { should be_present }
   end
 
-  describe '.daily_digest' do
+  describe '.digest' do
     subject { UserNotifications.digest(user) }
 
     context "without new topics" do

spec/models/email_log_spec.rb

@@ -8,8 +8,43 @@ describe EmailLog do
   it { should validate_presence_of :email_type }
 
 
-  context 'after_create with user' do
+  context 'after_create' do
 
+    context "reply_key" do
+
+      context "with reply by email enabled" do
+        before do
+          SiteSetting.expects(:reply_by_email_enabled).returns(true)
+        end
+
+        context 'generates a reply key' do
+          let(:reply_key) { Fabricate(:email_log).reply_key }
+
+          it "has a reply key" do
+            expect(reply_key).to be_present
+            expect(reply_key.size).to eq(32)
+          end
+        end
+      end
+
+      context "with reply by email disabled" do
+        before do
+          SiteSetting.expects(:reply_by_email_enabled).returns(false)
+        end
+
+        context 'generates a reply key' do
+          let(:reply_key) { Fabricate(:email_log).reply_key }
+
+          it "has no reply key" do
+            expect(reply_key).to be_blank
+          end
+        end
+      end
+
+    end
+
+
+    context 'with user' do
       let(:user) { Fabricate(:user) }
 
       it 'updates the last_emailed_at value for the user' do
@@ -18,6 +53,7 @@ describe EmailLog do
           user.reload
         }.should change(user, :last_emailed_at)
       end
+    end
 
   end