From 55593a12dac6e141e5eb05050e9533a55c94b37a Mon Sep 17 00:00:00 2001 From: Jon Willesen Date: Tue, 23 Oct 2012 17:09:25 -0600 Subject: [PATCH] fix sending bounce emails The incoming message processor wasn't actually sending bounce emails. The Message record was being created, but because it wasn't assigned a user or communication channel, the message never got sent. Did some refactoring so user and communication channel is initialized for the bounce message when we know them. If we don't have enough information to identify the communication channel, then we will just try to send the bounce message directly without using the usual mechanisms. This should be good enough for now. refs #11068 test plan: - create a course with a teacher and a student - set student's notification for annoucements to asap - create an announcement as the teacher and then lock the announcement topic - find the annoucement message for the student and reply to it - the student should receive a bounce message for trying to reply to a locked topic Change-Id: I7c1a80dd72ba25931ac3bad77d4d972108064e5b Reviewed-on: https://gerrit.instructure.com/14688 Tested-by: Jenkins Reviewed-by: Mark Ericksen Reviewed-by: Zach Pendleton Reviewed-by: Marc LeGendre --- lib/incoming_message_processor.rb | 63 +++++++++----- spec/lib/incoming_message_processor_spec.rb | 94 +++++++++++++-------- 2 files changed, 105 insertions(+), 52 deletions(-) diff --git a/lib/incoming_message_processor.rb b/lib/incoming_message_processor.rb index 5c18b086b0c..8583ee72fa4 100644 --- a/lib/incoming_message_processor.rb +++ b/lib/incoming_message_processor.rb @@ -30,37 +30,37 @@ class IncomingMessageProcessor Iconv.conv('UTF-8//TRANSLIT//IGNORE', encoding, string) rescue TextHelper.strip_invalid_utf8(string) end - def self.process_single(message, secure_id, message_id) - if message.multipart? && part = message.parts.find { |p| p.content_type.try(:match, %r{^text/html(;|$)}) } + def self.process_single(incoming_message, secure_id, message_id) + if incoming_message.multipart? && part = incoming_message.parts.find { |p| p.content_type.try(:match, %r{^text/html(;|$)}) } html_body = utf8ify(part.body.decoded, part.charset) end - html_body = utf8ify(message.body.decoded, message.charset) if !message.multipart? && message.content_type.try(:match, %r{^text/html(;|$)}) - if message.multipart? && part = message.parts.find { |p| p.content_type.try(:match, %r{^text/plain(;|$)}) } + html_body = utf8ify(incoming_message.body.decoded, incoming_message.charset) if !incoming_message.multipart? && incoming_message.content_type.try(:match, %r{^text/html(;|$)}) + if incoming_message.multipart? && part = incoming_message.parts.find { |p| p.content_type.try(:match, %r{^text/plain(;|$)}) } body = utf8ify(part.body.decoded, part.charset) end - body ||= utf8ify(message.body.decoded, message.charset) + body ||= utf8ify(incoming_message.body.decoded, incoming_message.charset) if !html_body self.extend TextHelper html_body = format_message(body).first end begin - msg = Message.find_by_id(message_id) - raise IncomingMessageProcessor::UnknownAddressError unless msg - msg.shard.activate do - context = msg.context if secure_id == msg.reply_to_secure_id - user = msg.user + original_message = Message.find_by_id(message_id) + raise IncomingMessageProcessor::UnknownAddressError unless original_message + original_message.shard.activate do + context = original_message.context if secure_id == original_message.reply_to_secure_id + user = original_message.user raise IncomingMessageProcessor::UnknownAddressError unless user && context && context.respond_to?(:reply_from) context.reply_from({ :purpose => 'general', :user => user, - :subject => utf8ify(message.subject, message.header[:subject].try(:charset)), + :subject => utf8ify(incoming_message.subject, incoming_message.header[:subject].try(:charset)), :html => html_body, :text => body }) end rescue IncomingMessageProcessor::ReplyFromError => error - IncomingMessageProcessor.ndr(message.from.first, message.subject, error) if message.from.try(:first) + IncomingMessageProcessor.ndr(original_message, incoming_message, error) end end @@ -82,20 +82,45 @@ class IncomingMessageProcessor end end - def self.ndr(from, subject, error = IncomingMessageProcessor::UnknownAddressError) - ndr_subject, ndr_body = IncomingMessageProcessor.ndr_strings(subject, error) + def self.ndr(original_message, incoming_message, error) + incoming_from = incoming_message.from.try(:first) + incoming_subject = incoming_message.subject + return unless incoming_from - message = Message.create!( - :to => from, + ndr_subject, ndr_body = IncomingMessageProcessor.ndr_strings(incoming_subject, error) + outgoing_message = Message.new({ + :to => incoming_from, :from => HostUrl.outgoing_email_address, :subject => ndr_subject, :body => ndr_body, :delay_for => 0, :context => nil, :path_type => 'email', - :from_name => "Instructure" - ) - message.deliver + :from_name => "Instructure", + }) + + outgoing_message_delivered = false + if original_message + original_message.shard.activate do + comch = CommunicationChannel.active.find_by_path_and_path_type(incoming_from, 'email') + outgoing_message.communication_channel = comch + outgoing_message.user = comch.try(:user) + if outgoing_message.communication_channel && outgoing_message.user + outgoing_message.save + outgoing_message.deliver + outgoing_message_delivered = true + end + end + end + + unless outgoing_message_delivered + # Can't use our usual mechanisms, so just try to send it once now + begin + res = Mailer.deliver_message(outgoing_message) + rescue => e + # TODO: put some kind of error logging here? + end + end end def self.ndr_strings(subject, error) diff --git a/spec/lib/incoming_message_processor_spec.rb b/spec/lib/incoming_message_processor_spec.rb index 12e1c020c4c..f0a5be2c92a 100644 --- a/spec/lib/incoming_message_processor_spec.rb +++ b/spec/lib/incoming_message_processor_spec.rb @@ -19,7 +19,39 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb') describe IncomingMessageProcessor do + def setup_test_outgoing_mail + @original_delivery_method = ActionMailer::Base.delivery_method + @original_perform_deliveries = ActionMailer::Base.perform_deliveries + ActionMailer::Base.delivery_method = :test + ActionMailer::Base.perform_deliveries = true + end + + def restore_original_outgoing_mail + ActionMailer::Base.delivery_method = @original_delivery_method if @original_delivery_method + ActionMailer::Base.perform_deliveries = @original_perform_deliveries if @original_perform_deliveries + end + + def simple_mail_from_user + Mail.new(:body => "body", :from => @user.email_channel.path) + end + + def check_new_message(bounce_type) + Message.count.should == @previous_message_count + 1 + @new_message = Message.find(:first, :order => 'created_at DESC') + @new_message.subject.should match(/Reply Failed/) + @new_message.body.should match(case bounce_type + when :unknown then /unknown mailbox/ + when :locked then /topic is locked/ + end) + # new checks to make sure these messages are getting sent + @new_message.user_id.should == @user.id + @new_message.communication_channel_id.should == @user.email_channel.id + @new_message.should be_sent + end + before(:all) do + setup_test_outgoing_mail + DiscussionTopic.class_eval { alias_method :old_reply_from, :reply_from def reply_from(opts) @@ -55,12 +87,16 @@ describe IncomingMessageProcessor do DiscussionTopic.reply_from_result = DiscussionEntry.new discussion_topic_model + @cc = @user.communication_channels.build(:path_type => 'email', :path => "user@example.com") + @cc.confirm + @cc.save! @message = Message.create(:context => @topic, :user => @user) @previous_message_count = Message.count @previous_message_count.should == 1 end after(:all) do + restore_original_outgoing_mail DiscussionTopic.class_eval { alias_method :reply_from, :old_reply_from } end @@ -100,57 +136,49 @@ describe IncomingMessageProcessor do describe "when data is not found" do it "should send a bounce reply when sent a bogus secure_id" do - IncomingMessageProcessor.process_single(Mail.new { body "body"; from "a@b.c" }, + IncomingMessageProcessor.process_single(simple_mail_from_user, "deadbeef", @message.id) - Message.count.should == @previous_message_count + 1 - # new_message = Message.find_by_id(@message.id + 1) - new_message = Message.find(:first, :order => 'created_at DESC') - new_message.subject.should match(/Reply Failed/) - new_message.body.should match(/unknown mailbox/) - end - - it "should send a bounce reply when sent a bogus message id" do - IncomingMessageProcessor.process_single(Mail.new { body "body"; from "a@b.c" }, - @message.reply_to_secure_id, -1) - Message.count.should == @previous_message_count + 1 - # new_message = Message.find_by_id(@message.id + 1) - new_message = Message.find(:first, :order => 'created_at DESC') - new_message.subject.should match(/Reply Failed/) - new_message.body.should match(/unknown mailbox/) + check_new_message(:unknown) end it "should send a bounce reply when user is not found" do @message.user = nil @message.save! - IncomingMessageProcessor.process_single(Mail.new { body "body"; from "a@b.c" }, + IncomingMessageProcessor.process_single(simple_mail_from_user, @message.reply_to_secure_id, @message.id) - Message.count.should == @previous_message_count + 1 - # new_message = Message.find_by_id(@message.id + 1) - new_message = Message.find(:first, :order => 'created_at DESC') - new_message.subject.should match(/Reply Failed/) - new_message.body.should match(/unknown mailbox/) + check_new_message(:unknown) end it "should send a bounce reply when context is not found" do @message.context = nil @message.save! - IncomingMessageProcessor.process_single(Mail.new { body "body"; from "a@b.c" }, + IncomingMessageProcessor.process_single(simple_mail_from_user, @message.reply_to_secure_id, @message.id) - Message.count.should == @previous_message_count + 1 - # new_message = Message.find_by_id(@message.id + 1) - new_message = Message.find(:first, :order => 'created_at DESC') - new_message.subject.should match(/Reply Failed/) - new_message.body.should match(/unknown mailbox/) + check_new_message(:unknown) + end + + it "should send a bounce reply directly when sent a bogus message id" do + expect { + IncomingMessageProcessor.process_single(simple_mail_from_user, + @message.reply_to_secure_id, -1) + }.to change { ActionMailer::Base.deliveries.size }.by(1) + @previous_message_count.should eql @previous_message_count + end + + it "should send a bounce reply directly if no communication channel is found" do + expect { + IncomingMessageProcessor.process_single(Mail.new(:body => "body", :from => "bogus_email@example.com"), + "deadbeef", @message.id) + }.to change { ActionMailer::Base.deliveries.size }.by(1) + @previous_message_count.should eql @previous_message_count end end it "should send a bounce reply when reply_from raises ReplyToLockedTopicError" do DiscussionTopic.reply_from_result = IncomingMessageProcessor::ReplyToLockedTopicError - test_mail = Mail.new { body "reply body"; from "test@example.a" } - IncomingMessageProcessor.process_single(test_mail, @message.reply_to_secure_id, @message.id) - Message.count.should == @previous_message_count + 1 - new_message = Message.find(:first, :order => 'created_at DESC') - new_message.body.should match(/topic is locked/) + IncomingMessageProcessor.process_single(simple_mail_from_user, + @message.reply_to_secure_id, @message.id) + check_new_message(:locked) end it "should process emails from mailman" do