From 024597e643fb5d70799c28d3a29f8607ee3270ff Mon Sep 17 00:00:00 2001
From: Benjamin Kampmann <ben.kampmann@gmail.com>
Date: Fri, 28 Feb 2014 13:05:09 +0100
Subject: [PATCH] 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 | 30 ++++--
app/mailers/rejection_mailer.rb | 4 +
config/locales/server.en.yml | 20 ++--
lib/email/receiver.rb | 51 +++++-----
spec/components/email/poll_mailbox_spec.rb | 107 +++++++++++++++++++++
spec/components/email/receiver_spec.rb | 96 ++++++------------
6 files changed, 198 insertions(+), 110 deletions(-)
create mode 100644 spec/components/email/poll_mailbox_spec.rb
diff --git a/app/jobs/scheduled/poll_mailbox.rb b/app/jobs/scheduled/poll_mailbox.rb
index edd0852e0..a113a35da 100644
--- a/app/jobs/scheduled/poll_mailbox.rb
+++ b/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
diff --git a/app/mailers/rejection_mailer.rb b/app/mailers/rejection_mailer.rb
index fa9957361..1e060aa55 100644
--- a/app/mailers/rejection_mailer.rb
+++ b/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
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 2361259da..fa5c2f17c 100644
--- a/config/locales/server.en.yml
+++ b/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"
diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb
index f1a6e279b..7ce6102ca 100644
--- a/lib/email/receiver.rb
+++ b/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
diff --git a/spec/components/email/poll_mailbox_spec.rb b/spec/components/email/poll_mailbox_spec.rb
new file mode 100644
index 000000000..1465ca1c7
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb
index 9217020b3..ad0697111 100644
--- a/spec/components/email/receiver_spec.rb
+++ b/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