diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb index 4035a1f0702..7f54d27ddde 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -10,10 +10,11 @@ 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}") message = GroupSmtpMailer.send_mail(group, email, post) - Email::Sender.new(message, :group_smtp, post.user).send + Email::Sender.new(message, :group_smtp, recipient_user).send # Create an incoming email record to avoid importing again from IMAP # server. diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb index ac203661bbc..58fcbc4a1b8 100644 --- a/app/mailers/group_smtp_mailer.rb +++ b/app/mailers/group_smtp_mailer.rb @@ -48,6 +48,7 @@ class GroupSmtpMailer < ActionMailer::Base group_name: from_group.name, allow_reply_by_email: true, only_reply_by_email: true, + use_from_address_for_reply_to: from_group.imap_enabled?, private_reply: post.topic.private_message?, participants: participants(post), include_respond_instructions: true, diff --git a/app/models/group.rb b/app/models/group.rb index 610e570194c..c673afed46a 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -817,6 +817,11 @@ class Group < ActiveRecord::Base } end + def imap_enabled? + return false if !SiteSetting.enable_imap + imap_config.values.compact.length == imap_config.keys.length + end + def email_username_regex user, domain = email_username.split('@') if user.present? && domain.present? diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index efb0abf01f3..463e9c08684 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -153,7 +153,7 @@ module Email # please, don't send us automatic responses... result['X-Auto-Response-Suppress'] = 'All' - if allow_reply_by_email? + if allow_reply_by_email? && !@opts[:use_from_address_for_reply_to] result[ALLOW_REPLY_BY_EMAIL_HEADER] = true result['Reply-To'] = reply_by_email_address else diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index af01d4a2bcd..606cd4e087b 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -5,6 +5,7 @@ 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") } let(:args) do { group_id: group.id, @@ -23,7 +24,7 @@ RSpec.describe Jobs::GroupSmtpEmail do it "sends an email using the GroupSmtpMailer and Email::Sender" do message = Mail::Message.new(body: "hello", to: "myemail@example.invalid") GroupSmtpMailer.expects(:send_mail).with(group, "test@test.com", post).returns(message) - Email::Sender.expects(:new).with(message, :group_smtp, post.user).returns(stub(send: nil)) + Email::Sender.expects(:new).with(message, :group_smtp, recipient_user).returns(stub(send: nil)) subject.execute(args) end @@ -37,7 +38,7 @@ RSpec.describe Jobs::GroupSmtpEmail do it "creates a PostReplyKey and correctly uses it for the email reply_key substitution" do subject.execute(args) incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id) - post_reply_key = PostReplyKey.where(user_id: post.user_id, post_id: post.id).first + post_reply_key = PostReplyKey.where(user_id: recipient_user, post_id: post.id).first expect(post_reply_key).not_to eq(nil) expect(incoming.raw).to include("Reply-To: Discourse ") end diff --git a/spec/mailers/group_smtp_mailer_spec.rb b/spec/mailers/group_smtp_mailer_spec.rb index 6ecf9bbb102..4be00abf5d1 100644 --- a/spec/mailers/group_smtp_mailer_spec.rb +++ b/spec/mailers/group_smtp_mailer_spec.rb @@ -5,18 +5,18 @@ require 'email/receiver' describe GroupSmtpMailer do let(:group) { Fabricate(:group, - name: 'Testers', - title: 'Tester', - full_name: 'Testers Group', - smtp_server: 'smtp.gmail.com', - smtp_port: 587, - smtp_ssl: true, - imap_server: 'imap.gmail.com', - imap_port: 993, - imap_ssl: true, - email_username: 'bugs@gmail.com', - email_password: 'super$secret$password' - ) + name: 'Testers', + title: 'Tester', + full_name: 'Testers Group', + smtp_server: 'smtp.gmail.com', + smtp_port: 587, + smtp_ssl: true, + imap_server: 'imap.gmail.com', + imap_port: 993, + imap_ssl: true, + email_username: 'bugs@gmail.com', + email_password: 'super$secret$password' + ) } let(:user) { @@ -44,33 +44,75 @@ describe GroupSmtpMailer do let(:receiver) { receiver = Email::Receiver.new(email, - destinations: [group], - uid_validity: 1, - uid: 10000 - ) - receiver.process! - receiver + destinations: [group], + uid_validity: 1, + uid: 10000 + ) + receiver.process! + receiver } let(:raw) { 'hello, how are you doing?' } before do SiteSetting.enable_smtp = true + SiteSetting.enable_imap = true Jobs.run_immediately! end it 'sends an email as reply' do post = PostCreator.create(user, - topic_id: receiver.incoming_email.topic.id, - raw: raw - ) + topic_id: receiver.incoming_email.topic.id, + raw: raw + ) expect(ActionMailer::Base.deliveries.size).to eq(1) sent_mail = ActionMailer::Base.deliveries[0] expect(sent_mail.to).to contain_exactly('john@doe.com') + expect(sent_mail.reply_to).to contain_exactly('bugs@gmail.com') expect(sent_mail.subject).to eq('Re: Hello from John') expect(sent_mail.to_s).to include(raw) end + context "when the site has a reply by email address configured" do + before do + SiteSetting.manual_polling_enabled = true + SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com" + SiteSetting.reply_by_email_enabled = true + end + + it 'uses the correct IMAP/SMTP reply to address' do + post = PostCreator.create(user, + topic_id: receiver.incoming_email.topic.id, + raw: raw + ) + + expect(ActionMailer::Base.deliveries.size).to eq(1) + + sent_mail = ActionMailer::Base.deliveries[0] + expect(sent_mail.reply_to).to contain_exactly('bugs@gmail.com') + end + + context "when IMAP is disabled for the group" do + before do + group.update( + imap_server: nil + ) + end + + it "uses the reply key based reply to address" do + post = PostCreator.create(user, + topic_id: receiver.incoming_email.topic.id, + raw: raw + ) + + expect(ActionMailer::Base.deliveries.size).to eq(1) + + sent_mail = ActionMailer::Base.deliveries[0] + post_reply_key = PostReplyKey.last + expect(sent_mail.reply_to).to contain_exactly("test+#{post_reply_key.reply_key}@test.com") + end + end + end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 0e501d5360a..9dce1fa07f5 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -973,7 +973,7 @@ describe Group do end end - describe "#imap_mailboxes" do + describe "IMAP" do let(:group) { Fabricate(:group) } def mock_imap @@ -1010,39 +1010,55 @@ describe Group do Discourse.redis.del("group_imap_mailboxes_#{group.id}") end - it "returns an empty array if group imap is not configured" do - expect(group.imap_mailboxes).to eq([]) + describe "#imap_enabled?" do + it "returns true if imap is configured and enabled for the site" do + mock_imap + configure_imap + enable_imap + expect(group.imap_enabled?).to eq(true) + end + + it "returns false if imap is configured and not enabled for the site" do + configure_imap + expect(group.imap_enabled?).to eq(false) + end end - it "returns an empty array and does not contact IMAP server if group imap is configured but the setting is disabled" do - configure_imap - Imap::Providers::Detector.expects(:init_with_detected_provider).never - expect(group.imap_mailboxes).to eq([]) - end + describe "#imap_mailboxes" do + it "returns an empty array if group imap is not configured" do + expect(group.imap_mailboxes).to eq([]) + end - it "logs the imap error if one occurs" do - configure_imap - mock_imap - SiteSetting.enable_imap = true - @mocked_imap_provider.stubs(:connect!).raises(Net::IMAP::NoResponseError) - group.imap_mailboxes - expect(group.reload.imap_last_error).not_to eq(nil) - end + it "returns an empty array and does not contact IMAP server if group imap is configured but the setting is disabled" do + configure_imap + Imap::Providers::Detector.expects(:init_with_detected_provider).never + expect(group.imap_mailboxes).to eq([]) + end - it "returns a list of mailboxes from the IMAP provider" do - configure_imap - mock_imap - enable_imap - expect(group.imap_mailboxes).to eq(["Inbox"]) - end + it "logs the imap error if one occurs" do + configure_imap + mock_imap + SiteSetting.enable_imap = true + @mocked_imap_provider.stubs(:connect!).raises(Net::IMAP::NoResponseError) + group.imap_mailboxes + expect(group.reload.imap_last_error).not_to eq(nil) + end - it "caches the login and mailbox fetch" do - configure_imap - mock_imap - enable_imap - group.imap_mailboxes - Imap::Providers::Detector.expects(:init_with_detected_provider).never - group.imap_mailboxes + it "returns a list of mailboxes from the IMAP provider" do + configure_imap + mock_imap + enable_imap + expect(group.imap_mailboxes).to eq(["Inbox"]) + end + + it "caches the login and mailbox fetch" do + configure_imap + mock_imap + enable_imap + group.imap_mailboxes + Imap::Providers::Detector.expects(:init_with_detected_provider).never + group.imap_mailboxes + end end end