ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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 <jenkins@instructure.com>
Reviewed-by: Mark Ericksen <marke@instructure.com>
Reviewed-by: Zach Pendleton <zachp@instructure.com>
Reviewed-by: Marc LeGendre <marc@instructure.com>
This commit is contained in:
Jon Willesen 2012-10-23 17:09:25 -06:00 committed by Zach Pendleton
parent 7d15f4696c
commit 55593a12da
2 changed files with 105 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
lib/incoming_message_processor.rb
View File

@ -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)

94
spec/lib/incoming_message_processor_spec.rb
View File

@ -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