Switch to proper exception handling system for better user feedback

- Replace implicit return code-system in Email::Receiver with proper exception system
 - Update tests to check for exceptions instead
 - Test the PollMailbox for expected failures
 - Add proper email-handling of problematic emails
"

app/jobs/scheduled/poll_mailbox.rb

@@ -18,6 +18,26 @@ module Jobs
       end
     end
 
+    def handle_mail(mail)
+      begin
+        Email::Receiver.new(mail).process
+      rescue Email::Receiver::UserNotSufficientTrustLevelError => e
+        # inform the user about the rejection
+        @message = Mail::Message.new(mail)
+        clientMessage = RejectionMailer.send_trust_level(@message.from, @message.body)
+        email_sender = Email::Sender.new(clientMessage, :email_reject_trust_level)
+        email_sender.send
+      rescue Email::Receiver::ProcessingError
+        # all other ProcessingErrors are ok to be dropped
+      rescue StandardError => e
+        # Inform Admins about error
+        GroupMessage.create(Group[:admins].name, :email_error_notification,
+            {limit_once_per: false, message_params: {source: mail, error: e}})
+      ensure
+        mail.delete
+      end
+    end
+
     def poll_pop3s
       Net::POP3.enable_ssl(OpenSSL::SSL::VERIFY_NONE)
       Net::POP3.start(SiteSetting.pop3s_polling_host,
@@ -26,15 +46,7 @@ module Jobs
                       SiteSetting.pop3s_polling_password) do |pop|
         unless pop.mails.empty?
           pop.each do |mail|
-            if Email::Receiver.new(mail.pop).process == Email::Receiver.results[:processed]
-              mail.delete
-            else
-                @message = Mail::Message.new(mail.pop)
-                # One for you (mod), and one for me (sender)
-                GroupMessage.create(Group[:moderators].name, :email_reject_notification, {limit_once_per: false, message_params: {from: @message.from, body: @message.body}})
-                clientMessage = RejectionMailer.send_rejection(@message.from, @message.body)
-                Email::Sender.new(clientMessage, :email_reject_notification).send
-            end
+            handle_mail mail.pop
           end
         end
       end

app/mailers/rejection_mailer.rb

@@ -6,4 +6,8 @@ class RejectionMailer < ActionMailer::Base
   def send_rejection(from, body)
     build_email(from, template: 'email_reject_notification', from: from, body: body)
   end
+
+  def send_trust_level(from, body, to)
+    build_email(from, template: 'email_reject_trust_level', to: to)
+  end
 end

config/locales/server.en.yml

@@ -1111,17 +1111,23 @@ en:
       subject_template: "Import completed successfully"
       text_body_template: "The import was successful."
 
-    email_reject_notification:
-      subject_template: "Message posting failed"
+    email_error_notification:
+      subject_template: "Error parsing email"
       text_body_template: |
-        This is an automated message to inform you that the user a message failed to meet topic criteria.
+        This is an automated message to inform you that parsing the following incoming email failed.
 
         Please review the following message.
 
-        From - %{from}
-        
-        Contents - %{body}.
-        
+        Error - %{error}
+
+        %{source}
+
+    email_reject_trust_level:
+      subject_template: "Message rejected"
+      text_body_template: |
+        The message you've send to %{to} was rejected by the system.
+
+        You do not have the required trust to post new topics to this email address.
 
     too_many_spam_flags:
       subject_template: "New account blocked"

lib/email/receiver.rb

@@ -5,9 +5,12 @@
 module Email
   class Receiver
 
-    def self.results
-      @results ||= Enum.new(:unprocessable, :missing, :processed, :error)
-    end
+    class ProcessingError < StandardError; end
+    class EmailUnparsableError < ProcessingError; end
+    class EmptyEmailError < ProcessingError; end
+    class UserNotFoundError < ProcessingError; end
+    class UserNotSufficientTrustLevelError < ProcessingError; end
+    class EmailLogNotFound < ProcessingError; end
 
     attr_reader :body, :reply_key, :email_log
 
@@ -32,21 +35,21 @@ module Email
     end
 
     def process
-      return Email::Receiver.results[:unprocessable] if @raw.blank?
+      raise EmptyEmailError if @raw.blank?
 
       @message = Mail::Message.new(@raw)
 
 
       # First remove the known discourse stuff.
       parse_body
-      return Email::Receiver.results[:unprocessable] if @body.blank?
+      raise EmptyEmailError if @body.blank?
 
       # Then run the github EmailReplyParser on it in case we didn't catch it
       @body = EmailReplyParser.read(@body).visible_text.force_encoding('UTF-8')
 
       discourse_email_parser
 
-      return Email::Receiver.results[:unprocessable] if @body.blank?
+      raise EmailUnparsableError if @body.blank?
 
       if is_in_email?
         @user = User.find_by_email(@message.from.first)
@@ -55,29 +58,25 @@ module Email
           @user = Discourse.system_user
         end
 
-        return Email::Receiver.results[:unprocessable] if @user.blank? or not @user.has_trust_level?(TrustLevel.levels[SiteSetting.email_in_min_trust.to_i])
+        raise UserNotFoundError if @user.blank?
+        raise UserNotSufficientTrustLevelError.new @user if not @user.has_trust_level?(TrustLevel.levels[SiteSetting.email_in_min_trust.to_i])
 
         create_new_topic
-        return Email::Receiver.results[:processed]
+      else
+        @reply_key = @message.to.first
+
+        # Extract the `reply_key` from the format the site has specified
+        tokens = SiteSetting.reply_by_email_address.split("%{reply_key}")
+        tokens.each do |t|
+          @reply_key.gsub!(t, "") if t.present?
+        end
+
+        # Look up the email log for the reply key
+        @email_log = EmailLog.for(reply_key)
+        raise EmailLogNotFound if @email_log.blank?
+
+        create_reply
       end
-
-      @reply_key = @message.to.first
-
-      # Extract the `reply_key` from the format the site has specified
-      tokens = SiteSetting.reply_by_email_address.split("%{reply_key}")
-      tokens.each do |t|
-        @reply_key.gsub!(t, "") if t.present?
-      end
-
-      # Look up the email log for the reply key
-      @email_log = EmailLog.for(reply_key)
-      return Email::Receiver.results[:missing] if @email_log.blank?
-
-      create_reply
-
-      Email::Receiver.results[:processed]
-    rescue
-      Email::Receiver.results[:error]
     end
 
     private

spec/components/email/poll_mailbox_spec.rb

@@ -0,0 +1,107 @@
+# -*- encoding : utf-8 -*-
+
+require 'spec_helper'
+require 'email/receiver'
+require 'jobs/scheduled/poll_mailbox'
+require 'email/message_builder'
+
+describe Jobs::PollMailbox do
+
+  describe "processing email" do
+
+    let!(:poller) { Jobs::PollMailbox.new }
+    let!(:receiver) { mock }
+    let!(:email) { mock }
+
+    before do
+      Email::Receiver.expects(:new).with(email).returns(receiver)
+    end
+
+    describe "all goes fine" do
+
+      it "email gets deleted" do
+        receiver.expects(:process)
+        email.expects(:delete)
+
+        poller.handle_mail(email)
+      end
+    end
+
+    describe "raises Untrusted error" do
+
+      before do
+        receiver.expects(:process).raises(Email::Receiver::UserNotSufficientTrustLevelError)
+        email.expects(:delete)
+
+        Mail::Message.expects(:new).returns(email)
+
+        email.expects(:from)
+        email.expects(:body)
+
+        clientMessage = mock
+        senderMock = mock
+        RejectionMailer.expects(:send_trust_level).returns(clientMessage)
+        Email::Sender.expects(:new).with(
+              clientMessage, :email_reject_trust_level).returns(senderMock)
+        senderMock.expects(:send)
+      end
+
+      it "sends a reply and deletes the email" do
+        poller.handle_mail(email)
+      end
+    end
+
+    describe "raises error" do
+
+      it "deletes email on ProcessingError" do
+        receiver.expects(:process).raises(Email::Receiver::ProcessingError)
+        email.expects(:delete)
+
+        poller.handle_mail(email)
+      end
+
+      it "deletes email on EmailUnparsableError" do
+        receiver.expects(:process).raises(Email::Receiver::EmailUnparsableError)
+        email.expects(:delete)
+
+        poller.handle_mail(email)
+      end
+
+      it "deletes email on EmptyEmailError" do
+        receiver.expects(:process).raises(Email::Receiver::EmptyEmailError)
+        email.expects(:delete)
+
+        poller.handle_mail(email)
+      end
+
+      it "deletes email on UserNotFoundError" do
+        receiver.expects(:process).raises(Email::Receiver::UserNotFoundError)
+        email.expects(:delete)
+
+        poller.handle_mail(email)
+      end
+
+      it "deletes email on EmailLogNotFound" do
+        receiver.expects(:process).raises(Email::Receiver::EmailLogNotFound)
+        email.expects(:delete)
+
+        poller.handle_mail(email)
+      end
+
+
+      it "informs admins on any other error" do
+        receiver.expects(:process).raises(TypeError)
+        email.expects(:delete)
+        GroupMessage.expects(:create) do |args|
+          args[0].should eq "admins"
+          args[1].shouled eq :email_error_notification
+          args[2].message_params.source.should eq email
+          args[2].message_params.error.should_be instance_of(TypeError)
+        end
+
+        poller.handle_mail(email)
+      end
+    end
+  end
+
+end

spec/components/email/receiver_spec.rb

@@ -10,23 +10,13 @@ describe Email::Receiver do
     SiteSetting.stubs(:email_in).returns(false)
   end
 
-  describe "exception raised" do
-    it "returns error if it encountered an error processing" do
-      receiver = Email::Receiver.new("some email")
-      def receiver.parse_body
-        raise "ERROR HAPPENED!"
-      end
-      expect(receiver.process).to eq(Email::Receiver.results[:error])
-    end
-  end
-
   describe 'invalid emails' do
-    it "returns unprocessable if the message is blank" do
-      expect(Email::Receiver.new("").process).to eq(Email::Receiver.results[:unprocessable])
+    it "raises EmptyEmailError if the message is blank" do
+      expect { Email::Receiver.new("").process }.to raise_error(Email::Receiver::EmptyEmailError)
     end
 
-    it "returns unprocessable if the message is not an email" do
-      expect(Email::Receiver.new("asdf" * 30).process).to eq(Email::Receiver.results[:unprocessable])
+    it "raises EmailUnparsableError if the message is not an email" do
+      expect { Email::Receiver.new("asdf" * 30).process}.to raise_error(Email::Receiver::EmptyEmailError)
     end
   end
 
@@ -35,7 +25,7 @@ describe Email::Receiver do
     let(:receiver) { Email::Receiver.new(reply_below) }
 
     it "processes correctly" do
-      receiver.process
+      expect { receiver.process}.to raise_error(Email::Receiver::ProcessingError)
       expect(receiver.body).to eq(
 "So presumably all the quoted garbage and my (proper) signature will get
 stripped from my reply?")
@@ -47,7 +37,7 @@ stripped from my reply?")
     let(:receiver) { Email::Receiver.new(reply_below) }
 
     it "processes correctly" do
-      receiver.process
+      expect { receiver.process}.to raise_error(Email::Receiver::ProcessingError)
       expect(receiver.body).to eq("The EC2 instance - I've seen that there tends to be odd and " +
                                   "unrecommended settings on the Bitnami installs that I've checked out.")
     end
@@ -58,7 +48,7 @@ stripped from my reply?")
     let(:receiver) { Email::Receiver.new(attachment) }
 
     it "processes correctly" do
-      expect(receiver.process).to eq(Email::Receiver.results[:unprocessable])
+      expect { receiver.process}.to raise_error(Email::Receiver::EmptyEmailError)
       expect(receiver.body).to be_blank
     end
   end
@@ -68,7 +58,7 @@ stripped from my reply?")
     let(:receiver) { Email::Receiver.new(dutch) }
 
     it "processes correctly" do
-      receiver.process
+      expect { receiver.process}.to raise_error(Email::Receiver::ProcessingError)
       expect(receiver.body).to eq("Dit is een antwoord in het Nederlands.")
     end
   end
@@ -79,7 +69,7 @@ stripped from my reply?")
 
     it "processes correctly" do
       I18n.expects(:t).with('user_notifications.previous_discussion').returns('כלטוב')
-      receiver.process
+      expect { receiver.process}.to raise_error(Email::Receiver::ProcessingError)
       expect(receiver.body).to eq("שלום")
     end
   end
@@ -90,7 +80,7 @@ stripped from my reply?")
 
     it "processes correctly" do
       I18n.expects(:t).with('user_notifications.previous_discussion').returns('媽！我上電視了！')
-      receiver.process
+      expect { receiver.process}.to raise_error(Email::Receiver::ProcessingError)
       expect(receiver.body).to eq("媽！我上電視了！")
     end
   end
@@ -100,7 +90,7 @@ stripped from my reply?")
     let(:receiver) { Email::Receiver.new(wrote) }
 
     it "removes via lines if we know them" do
-      receiver.process
+      expect { receiver.process}.to raise_error(Email::Receiver::ProcessingError)
       expect(receiver.body).to eq("Hello this email has content!")
     end
   end
@@ -110,7 +100,7 @@ stripped from my reply?")
     let(:receiver) { Email::Receiver.new(wrote) }
 
     it "processes correctly" do
-      receiver.process
+      expect { receiver.process}.to raise_error(Email::Receiver::ProcessingError)
       expect(receiver.body).to eq("Thanks!")
     end
   end
@@ -120,7 +110,7 @@ stripped from my reply?")
     let(:receiver) { Email::Receiver.new(previous) }
 
     it "processes correctly" do
-      receiver.process
+      expect { receiver.process}.to raise_error(Email::Receiver::ProcessingError)
       expect(receiver.body).to eq("This will not include the previous discussion that is present in this email.")
     end
   end
@@ -130,7 +120,7 @@ stripped from my reply?")
     let(:receiver) { Email::Receiver.new(paragraphs) }
 
     it "processes correctly" do
-      receiver.process
+      expect { receiver.process}.to raise_error(Email::Receiver::ProcessingError)
       expect(receiver.body).to eq(
 "Is there any reason the *old* candy can't be be kept in silos while the new candy
 is imported into *new* silos?
@@ -161,10 +151,8 @@ greatest show ever created. Everyone should watch it.
         EmailLog.expects(:for).returns(nil)
       end
 
-      let!(:result) { receiver.process }
-
-      it "returns missing" do
-        expect(result).to eq(Email::Receiver.results[:missing])
+      it "raises EmailLogNotFoundError" do
+        expect{ receiver.process }.to raise_error(Email::Receiver::EmailLogNotFound)
       end
 
     end
@@ -185,10 +173,6 @@ greatest show ever created. Everyone should watch it.
 
       let!(:result) { receiver.process }
 
-      it "returns a processed result" do
-        expect(result).to eq(Email::Receiver.results[:processed])
-      end
-
       it "extracts the body" do
         expect(receiver.body).to eq(reply_body)
       end
@@ -229,10 +213,8 @@ Jakie" }
         User.expects(:find_by_email).returns(nil)
       end
 
-      let!(:result) { receiver.process }
-
-      it "returns unprocessable" do
-        expect(result).to eq(Email::Receiver.results[:unprocessable])
+      it "raises user not found error" do
+        expect { receiver.process }.to raise_error(Email::Receiver::UserNotFoundError)
       end
 
     end
@@ -244,10 +226,8 @@ Jakie" }
         SiteSetting.stubs(:email_in_min_trust).returns(TrustLevel.levels[:elder].to_s)
       end
 
-      let!(:result) { receiver.process }
-
-      it "returns unprocessable" do
-        expect(result).to eq(Email::Receiver.results[:unprocessable])
+      it "raises untrusted user error" do
+        expect { receiver.process }.to raise_error(Email::Receiver::UserNotSufficientTrustLevelError)
       end
 
     end
@@ -289,10 +269,6 @@ Jakie" }
 
       let!(:result) { receiver.process }
 
-      it "returns a processed result" do
-        expect(result).to eq(Email::Receiver.results[:processed])
-      end
-
       it "extracts the body" do
         expect(receiver.body).to eq(email_body)
       end
@@ -327,10 +303,8 @@ Jakie" }
         Category.expects(:find_by_email).returns(nil)
       end
 
-      let!(:result) { receiver.process }
-
-      it "returns missing" do
-        expect(result).to eq(Email::Receiver.results[:missing])
+      it "raises EmailLogNotFoundError" do
+        expect{ receiver.process }.to raise_error(Email::Receiver::EmailLogNotFound)
       end
 
     end
@@ -343,10 +317,8 @@ Jakie" }
               "discourse-in@appmail.adventuretime.ooo").returns(category)
       end
 
-      let!(:result) { receiver.process }
-
-      it "returns unprocessable" do
-        expect(result).to eq(Email::Receiver.results[:unprocessable])
+      it "raises UserNotFoundError" do
+        expect{ receiver.process }.to raise_error(Email::Receiver::UserNotFoundError)
       end
 
     end
@@ -360,10 +332,8 @@ Jakie" }
         SiteSetting.stubs(:email_in_min_trust).returns(TrustLevel.levels[:elder].to_s)
       end
 
-      let!(:result) { receiver.process }
-
-      it "returns unprocessable" do
-        expect(result).to eq(Email::Receiver.results[:unprocessable])
+      it "raises untrusted user error" do
+        expect { receiver.process }.to raise_error(Email::Receiver::UserNotSufficientTrustLevelError)
       end
 
     end
@@ -408,10 +378,6 @@ Jakie" }
 
       let!(:result) { receiver.process }
 
-      it "returns a processed result" do
-        expect(result).to eq(Email::Receiver.results[:processed])
-      end
-
       it "extracts the body" do
         expect(receiver.body).to eq(email_body)
       end
@@ -449,10 +415,8 @@ Jakie
               "discourse-in@appmail.adventuretime.ooo").returns(non_inbox_email_category)
       end
 
-      let!(:result) { receiver.process }
-
-      it "returns unprocessable" do
-        expect(result).to eq(Email::Receiver.results[:unprocessable])
+      it "raises UserNotFoundError" do
+        expect{ receiver.process }.to raise_error(Email::Receiver::UserNotFoundError)
       end
 
     end
@@ -495,10 +459,6 @@ Jakie
 
       let!(:result) { receiver.process }
 
-      it "returns a processed result" do
-        expect(result).to eq(Email::Receiver.results[:processed])
-      end
-
       it "extracts the body" do
         expect(receiver.body).to eq(email_body)
       end