diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb index 7f54d27ddde..2e31d5d548b 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -10,9 +10,23 @@ module Jobs group = Group.find_by(id: args[:group_id]) post = Post.find_by(id: args[:post_id]) email = args[:email] - recipient_user = ::UserEmail.find_by(email: email, primary: true)&.user - Rails.logger.debug("[IMAP] Sending email for group #{group.name} and post #{post.id}") + # There is a rare race condition causing the Imap::Sync class to create + # an incoming email and associated post/topic, which then kicks off + # the PostAlerter to notify others in the PM about a reply in the topic, + # but for the OP which is not necessary (because the person emailing the + # IMAP inbox already knows about the OP) + # + # Basically, we should never be sending this notification for the first + # post in a topic. + if post.is_first_post? + ImapSyncLog.warn("Aborting SMTP email for post #{post.id} in topic #{post.topic_id} to #{email}, the post is the OP and should not send an email.", group) + return + end + + ImapSyncLog.debug("Sending SMTP email for post #{post.id} in topic #{post.topic_id} to #{email}.", group) + + recipient_user = ::UserEmail.find_by(email: email, primary: true)&.user message = GroupSmtpMailer.send_mail(group, email, post) Email::Sender.new(message, :group_smtp, recipient_user).send @@ -23,7 +37,11 @@ module Jobs topic_id: post.topic_id, post_id: post.id, raw: message.to_s, - message_id: message.message_id + subject: message.subject, + message_id: message.message_id, + to_addresses: message.to, + cc_addresses: message.cc, + from_address: message.from ) end end diff --git a/app/models/imap_sync_log.rb b/app/models/imap_sync_log.rb index 46284a18d85..4d3809bebe9 100644 --- a/app/models/imap_sync_log.rb +++ b/app/models/imap_sync_log.rb @@ -12,7 +12,11 @@ class ImapSyncLog < ActiveRecord::Base def self.log(message, level, group_id = nil) now = Time.now.strftime("%Y-%m-%d %H:%M:%S.%L") new_log = create(message: message, level: ImapSyncLog.levels[level], group_id: group_id) - Rails.logger.send(level, "#{level[0].upcase}, [#{now}] [IMAP] (group_id #{group_id}) #{message}") + if ENV["DEBUG_IMAP"] + Rails.logger.send(:warn, "#{level[0].upcase}, [#{now}] [IMAP] (group_id #{group_id}) #{message}") + else + Rails.logger.send(level, "#{level[0].upcase}, [#{now}] [IMAP] (group_id #{group_id}) #{message}") + end new_log end diff --git a/app/models/incoming_email.rb b/app/models/incoming_email.rb index c18739ddcfa..08eb42a7477 100644 --- a/app/models/incoming_email.rb +++ b/app/models/incoming_email.rb @@ -28,6 +28,27 @@ class IncomingEmail < ActiveRecord::Base ) SQL end + + def to_addresses=(to) + if to&.is_a?(Array) + to = to.map(&:downcase).join(";") + end + super(to) + end + + def cc_addresses=(cc) + if cc&.is_a?(Array) + cc = cc.map(&:downcase).join(";") + end + super(cc) + end + + def from_address=(from) + if from&.is_a?(Array) + from = from.first + end + super(from) + end end # == Schema Information diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index e1aa8ea17df..50fbb219ca7 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -147,8 +147,8 @@ module Email raw: Email::Cleaner.new(@raw_email).execute, subject: subject, from_address: @from_email, - to_addresses: @mail.to&.map(&:downcase)&.join(";"), - cc_addresses: @mail.cc&.map(&:downcase)&.join(";"), + to_addresses: @mail.to, + cc_addresses: @mail.cc, imap_uid_validity: @opts[:imap_uid_validity], imap_uid: @opts[:imap_uid], imap_group_id: @opts[:imap_group_id], diff --git a/lib/email/sender.rb b/lib/email/sender.rb index b656302a692..ffd9707d328 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -173,7 +173,7 @@ module Email end end - if reply_key.present? && @message.header['Reply-To'] =~ /\<([^\>]+)\>/ + if reply_key.present? && @message.header['Reply-To'].to_s =~ /\<([^\>]+)\>/ email = Regexp.last_match[1] @message.header['List-Post'] = "" end diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index 606cd4e087b..a67f56dbf3d 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -3,13 +3,18 @@ require 'rails_helper' RSpec.describe Jobs::GroupSmtpEmail do - let(:post) { Fabricate(:post) } - let(:group) { Fabricate(:imap_group) } - let!(:recipient_user) { Fabricate(:user, email: "test@test.com") } + fab!(:post) do + topic = Fabricate(:topic) + Fabricate(:post, topic: topic) + Fabricate(:post, topic: topic) + end + fab!(:group) { Fabricate(:imap_group) } + fab!(:recipient_user) { Fabricate(:user, email: "test@test.com") } + let(:post_id) { post.id } let(:args) do { group_id: group.id, - post_id: post.id, + post_id: post_id, email: "test@test.com" } end @@ -32,7 +37,7 @@ RSpec.describe Jobs::GroupSmtpEmail do subject.execute(args) incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id) expect(incoming).not_to eq(nil) - expect(incoming.message_id).to eq("topic/#{post.topic_id}@test.localhost") + expect(incoming.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") end it "creates a PostReplyKey and correctly uses it for the email reply_key substitution" do @@ -42,4 +47,19 @@ RSpec.describe Jobs::GroupSmtpEmail do expect(post_reply_key).not_to eq(nil) expect(incoming.raw).to include("Reply-To: Discourse ") end + + it "has the from_address and the to_addresses and subject filled in correctly" do + subject.execute(args) + incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id) + expect(incoming.to_addresses).to eq("test@test.com") + expect(incoming.subject).to include("Re: This is a test topic") + expect(incoming.from_address).to eq("discourseteam@ponyexpress.com") + end + + context "when the post in the argument is the OP" do + let(:post_id) { post.topic.posts.first.id } + it "aborts and does not send a group SMTP email; the OP is the one that sent the email in the first place" do + expect { subject.execute(args) }.not_to(change { IncomingEmail.count }) + end + end end