Merge pull request #2614 from riking/email-tests

Email tests, and include posting error reason

app/jobs/scheduled/poll_mailbox.rb

@@ -24,40 +24,52 @@ module Jobs
         mail_string = mail.pop
         Email::Receiver.new(mail_string).process
       rescue => e
-        message_template = nil
+        handle_failure(mail_string, e)
-        case e
-          when Email::Receiver::UserNotSufficientTrustLevelError
-            message_template = :email_reject_trust_level
-          when Email::Receiver::UserNotFoundError
-            message_template = :email_reject_no_account
-          when Email::Receiver::EmptyEmailError
-            message_template = :email_reject_empty
-          when Email::Receiver::EmailUnparsableError
-            message_template = :email_reject_parsing
-          when Email::Receiver::EmailLogNotFound
-            message_template = :email_reject_reply_key
-          when ActiveRecord::Rollback
-            message_template = :email_reject_post_error
-          when Email::Receiver::InvalidPost
-            # TODO there is a message in this exception, place it in email
-            message_template = :email_reject_post_error
-          else
-            message_template = nil
-        end
-
-        if message_template
-          # inform the user about the rejection
-          message = Mail::Message.new(mail_string)
-          client_message = RejectionMailer.send_rejection(message.from, message.body, message.subject, message.to, message_template)
-          Email::Sender.new(client_message, message_template).send
-        else
-          Discourse.handle_exception(e, error_context(@args, "Unrecognized error type when processing incoming email", mail: mail_string))
-        end
       ensure
         mail.delete
       end
     end
 
+    def handle_failure(mail_string, e)
+      template_args = {}
+      case e
+        when Email::Receiver::UserNotSufficientTrustLevelError
+          message_template = :email_reject_trust_level
+        when Email::Receiver::UserNotFoundError
+          message_template = :email_reject_no_account
+        when Email::Receiver::EmptyEmailError
+          message_template = :email_reject_empty
+        when Email::Receiver::EmailUnparsableError
+          message_template = :email_reject_parsing
+        when Email::Receiver::EmailLogNotFound
+          message_template = :email_reject_reply_key
+        when ActiveRecord::Rollback
+          message_template = :email_reject_post_error
+        when Email::Receiver::InvalidPost
+          if e.message.length < 6
+            message_template = :email_reject_post_error
+          else
+            message_template = :email_reject_post_error_specified
+            template_args[:post_error] = e.message
+          end
+
+        else
+          message_template = nil
+      end
+
+      if message_template
+        # inform the user about the rejection
+        message = Mail::Message.new(mail_string)
+        template_args[:former_title] = message.subject
+        template_args[:destination] = message.to
+
+        client_message = RejectionMailer.send_rejection(message_template, message.from, template_args)
+        Email::Sender.new(client_message, message_template).send
+      else
+        Discourse.handle_exception(e, error_context(@args, "Unrecognized error type when processing incoming email", mail: mail_string))
+      end
+    end
+
     def poll_pop3s
       if !SiteSetting.pop3s_polling_insecure
         Net::POP3.enable_ssl(OpenSSL::SSL::VERIFY_NONE)

app/mailers/rejection_mailer.rb

@@ -3,12 +3,27 @@ require_dependency 'email/message_builder'
 class RejectionMailer < ActionMailer::Base
   include Email::BuildEmailHelper
 
-  def send_rejection(message_from, message_body, message_subject, forum_address, template)
+  DISALLOWED_TEMPLATE_ARGS = [:to, :from, :site_name, :base_url,
-    build_email(message_from,
+                              :user_preferences_url,
-                template: "system_messages.#{template}",
+                              :include_respond_instructions, :html_override,
-                source: message_body,
+                              :add_unsubscribe_link, :respond_instructions,
-                former_title: message_subject,
+                              :style, :body, :post_id, :topic_id, :subject,
-                destination: forum_address)
+                              :template, :allow_reply_by_email,
+                              :private_reply, :from_alias]
+
+  # Send an email rejection message.
+  #
+  # template - i18n key under system_messages
+  # message_from - Who to send the rejection messsage to
+  # template_args - arguments to pass to i18n for interpolation into the message
+  #     Certain keys are disallowed in template_args to avoid confusing the
+  #     BuildEmailHelper. You can see the list in DISALLOWED_TEMPLATE_ARGS.
+  def send_rejection(template, message_from, template_args)
+    if template_args.keys.any? { |k| DISALLOWED_TEMPLATE_ARGS.include? k }
+      raise ArgumentError.new('Reserved key in template arguments')
+    end
+
+    build_email(message_from, template_args.merge(template: "system_messages.#{template}"))
   end
 
 end

config/locales/server.en.yml

@@ -1419,7 +1419,18 @@ en:
       text_body_template: |
         We're sorry, but your email message to %{destination} (titled %{former_title}) didn't work.
 
-        Some possible causes are: complex formatting, message too large, message too small. Please try again.
+        Some possible causes are: complex formatting, message too large, message too small. Please try again, or post via the website if this continues.
+
+    email_reject_post_error_specified:
+      subject_template: "Email issue -- Posting error"
+      text_body_template: |
+        We're sorry, but your email message to %{destination} (titled %{former_title}) didn't work.
+
+        Rejection message:
+
+        %{post_error}
+
+        Please attempt to fix the errors and try again.
 
     email_reject_reply_key:
       subject_template: "Email issue -- Bad Reply Key"

spec/fixtures/emails/valid_incoming.cooked

@@ -0,0 +1,5 @@
+<p>Hey folks,</p>
+
+<p>I was thinking. Wouldn't it be great if we could post topics via email? Yes it would!</p>
+
+<p>Jakie</p>

spec/fixtures/emails/valid_reply.cooked

@@ -0,0 +1,4 @@
+<p>I could not disagree more. I am obviously biased but adventure time is the
+greatest show ever created. Everyone should watch it.</p>
+
+<ul><li>Jake out</li></ul>

spec/jobs/poll_mailbox_spec.rb

@@ -41,70 +41,170 @@ describe Jobs::PollMailbox do
 
   end
 
-  describe "processing email" do
+  # Testing mock for the email objects that you get
+  # from Net::POP3.start { |pop| pop.mails }
+  class MockPop3EmailObject
+    def initialize(mail_string)
+      @message = mail_string
+      @delete_called = 0
+    end
 
-    let!(:receiver) { mock }
+    def pop
-    let!(:email_string) { fixture_file("emails/valid_incoming.eml") }
+      @message
-    let!(:email) { mock }
+    end
+
+    def delete
+      @delete_called += 1
+    end
+
+    # call 'assert email.deleted?' at the end of the test
+    def deleted?
+      @delete_called == 1
+    end
+  end
+
+  def expect_success
+    poller.expects(:handle_failure).never
+  end
+
+  def expect_exception(clazz)
+    poller.expects(:handle_failure).with(anything, instance_of(clazz))
+  end
+
+  describe "processing emails" do
+    let(:category) { Fabricate(:category) }
+    let(:user) { Fabricate(:user) }
 
     before do
-      email.stubs(:pop).returns(email_string)
+      SiteSetting.email_in = true
-      Email::Receiver.expects(:new).with(email_string).returns(receiver)
+      SiteSetting.reply_by_email_address = "reply+%{reply_key}@appmail.adventuretime.ooo"
+      category.email_in = 'incoming+amazing@appmail.adventuretime.ooo'
+      category.save
+      user.change_trust_level! :regular
+      user.username = 'Jake'
+      user.email = 'jake@adventuretime.ooo'
+      user.save
     end
 
-    describe "all goes fine" do
+    describe "a valid incoming email" do
+      let(:email) {
+        # this string replacing is kinda dumb
+        str = fixture_file('emails/valid_incoming.eml')
+        str = str.gsub("FROM", 'jake@adventuretime.ooo').gsub("TO", 'incoming+amazing@appmail.adventuretime.ooo')
+        MockPop3EmailObject.new str
+      }
+      let(:expected_post) { fixture_file('emails/valid_incoming.cooked') }
 
-      it "email gets deleted" do
+      it "posts a new topic with the correct content" do
-        receiver.expects(:process)
+        expect_success
-        email.expects(:delete)
 
         poller.handle_mail(email)
+
+        topic = Topic.where(category: category).where.not(id: category.topic_id).last
+        topic.should be_present
+        topic.title.should == "We should have a post-by-email-feature"
+
+        post = topic.posts.first
+        post.cooked.strip.should == expected_post.strip
+
+        email.should be_deleted
       end
-    end
 
-    describe "raises Untrusted error" do
+      describe "with insufficient trust" do
+        before do
+          user.change_trust_level! :newuser
+        end
 
-      it "sends a reply and deletes the email" do
+        it "raises a UserNotSufficientTrustLevelError" do
-        receiver.expects(:process).raises(Email::Receiver::UserNotSufficientTrustLevelError)
+          expect_exception Email::Receiver::UserNotSufficientTrustLevelError
-        email.expects(:delete)
-
-        message = Mail::Message.new(email_string)
-        Mail::Message.expects(:new).with(email_string).returns(message)
-
-        client_message = mock
-        sender_object = mock
-
-        RejectionMailer.expects(:send_rejection).with(
-            message.from, message.body, message.subject, message.to, :email_reject_trust_level
-        ).returns(client_message)
-        Email::Sender.expects(:new).with(client_message, :email_reject_trust_level).returns(sender_object)
-        sender_object.expects(:send)
-
-        poller.handle_mail(email)
-      end
-    end
-
-    describe "raises error" do
-
-      [ Email::Receiver::ProcessingError,
-        Email::Receiver::EmailUnparsableError,
-        Email::Receiver::EmptyEmailError,
-        Email::Receiver::UserNotFoundError,
-        Email::Receiver::EmailLogNotFound,
-        ActiveRecord::Rollback,
-        TypeError
-      ].each do |exception|
-
-        it "deletes email on #{exception}" do
-          receiver.expects(:process).raises(exception)
-          email.expects(:delete)
-
-          Discourse.stubs(:handle_exception)
 
           poller.handle_mail(email)
         end
 
+        it "posts the topic if allow_strangers is true" do
+          begin
+            category.email_in_allow_strangers = true
+            category.save
+
+            expect_success
+            poller.handle_mail(email)
+            topic = Topic.where(category: category).where.not(id: category.topic_id).last
+            topic.should be_present
+            topic.title.should == "We should have a post-by-email-feature"
+          ensure
+            category.email_in_allow_strangers = false
+            category.save
+          end
+        end
       end
+    end
+
+    describe "a valid reply" do
+      let(:email) { MockPop3EmailObject.new fixture_file('emails/valid_reply.eml')}
+      let(:expected_post) { fixture_file('emails/valid_reply.cooked')}
+      let(:topic) { Fabricate(:topic) }
+      let(:first_post) { Fabricate(:post, topic: topic, post_number: 1)}
+
+      before do
+        first_post.save
+        EmailLog.create(to_address: 'jake@email.example.com',
+                        email_type: 'user_posted',
+                        reply_key: '59d8df8370b7e95c5a49fbf86aeb2c93',
+                        user: user,
+                        post: first_post,
+                        topic: topic)
+      end
+
+      it "creates a new post" do
+        expect_success
+
+        poller.handle_mail(email)
+
+        new_post = Post.find_by(topic: topic, post_number: 2)
+        assert new_post.present?
+        assert_equal expected_post.strip, new_post.cooked.strip
+
+        email.should be_deleted
+      end
+
+      describe "with the wrong reply key" do
+        let(:email) { MockPop3EmailObject.new fixture_file('emails/wrong_reply_key.eml')}
+
+        it "raises an EmailLogNotFound error" do
+          expect_exception Email::Receiver::EmailLogNotFound
+
+          poller.handle_mail(email)
+          email.should be_deleted
+        end
+      end
+    end
+
+    describe "in failure conditions" do
+
+      it "a valid reply without an email log raises an EmailLogNotFound error" do
+        email = MockPop3EmailObject.new fixture_file('emails/valid_reply.eml')
+        expect_exception Email::Receiver::EmailLogNotFound
+
+        poller.handle_mail(email)
+        email.should be_deleted
+      end
+
+      it "a no content reply raises an EmailUnparsableError" do
+        email = MockPop3EmailObject.new fixture_file('emails/no_content_reply.eml')
+        expect_exception Email::Receiver::EmailUnparsableError
+
+        poller.handle_mail(email)
+        email.should be_deleted
+      end
+
+      it "a fully empty email raises an EmptyEmailError" do
+        email = MockPop3EmailObject.new fixture_file('emails/empty.eml')
+        expect_exception Email::Receiver::EmptyEmailError
+
+        poller.handle_mail(email)
+        email.should be_deleted
+      end
+
 
     end
   end