diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb index ba7afa98e66..8092dc38aa4 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -10,6 +10,7 @@ module Jobs group = Group.find_by(id: args[:group_id]) post = Post.find_by(id: args[:post_id]) email = args[:email] + cc_addresses = args[:cc_emails] # There is a rare race condition causing the Imap::Sync class to create # an incoming email and associated post/topic, which then kicks off @@ -27,11 +28,17 @@ module Jobs 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) + message = GroupSmtpMailer.send_mail(group, email, post, cc_addresses) + + # The EmailLog record created by the sender will have the raw email + # stored, the group smtp ID, and any cc addresses recorded for later + # cross referencing. Email::Sender.new(message, :group_smtp, recipient_user).send # Create an incoming email record to avoid importing again from IMAP - # server. + # server. While this may not be technically required if IMAP is not + # currently enabled for the group, it will help a lot with the initial + # sync if it is turned on at a later date. IncomingEmail.create!( user_id: post.user_id, topic_id: post.topic_id, diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb index 44edec917c3..574facc1578 100644 --- a/app/mailers/group_smtp_mailer.rb +++ b/app/mailers/group_smtp_mailer.rb @@ -5,13 +5,10 @@ require_dependency 'email/message_builder' class GroupSmtpMailer < ActionMailer::Base include Email::BuildEmailHelper - def send_mail(from_group, to_address, post) + def send_mail(from_group, to_address, post, cc_addresses = nil) raise 'SMTP is disabled' if !SiteSetting.enable_smtp - incoming_email = IncomingEmail.joins(:post) - .where('imap_uid IS NOT NULL') - .where(topic_id: post.topic_id, posts: { post_number: 1 }) - .limit(1).first + op_incoming_email = post.topic.first_post.incoming_email context_posts = Post .where(topic_id: post.topic_id) @@ -38,29 +35,32 @@ class GroupSmtpMailer < ActionMailer::Base user_name = post.user.name unless post.user.name.blank? end - build_email(to_address, + group_name = from_group.full_name.presence || from_group.name + build_email( + to_address, message: post.raw, url: post.url(without_slug: SiteSetting.private_email?), post_id: post.id, topic_id: post.topic_id, context: context(context_posts), username: post.user.username, - group_name: from_group.name, + group_name: group_name, allow_reply_by_email: true, only_reply_by_email: true, - use_from_address_for_reply_to: SiteSetting.enable_imap && from_group.imap_enabled?, + use_from_address_for_reply_to: SiteSetting.enable_smtp && from_group.smtp_enabled?, private_reply: post.topic.private_message?, participants: participants(post), include_respond_instructions: true, template: 'user_notifications.user_posted_pm', use_topic_title_subject: true, - topic_title: incoming_email&.subject || post.topic.title, + topic_title: op_incoming_email&.subject || post.topic.title, add_re_to_subject: true, locale: SiteSetting.default_locale, delivery_method_options: delivery_options, from: from_group.email_username, - from_alias: I18n.t('email_from', user_name: user_name, site_name: Email.site_title), - html_override: html_override(post, context_posts: context_posts) + from_alias: I18n.t('email_from', user_name: group_name, site_name: Email.site_title), + html_override: html_override(post, context_posts: context_posts), + cc: cc_addresses ) end diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 31336ce6c5f..993d46fa25e 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -327,7 +327,6 @@ class UserNotifications < ActionMailer::Base opts[:show_category_in_subject] = false opts[:show_tags_in_subject] = false opts[:show_group_in_subject] = true if SiteSetting.group_in_subject - opts[:use_group_smtp_if_configured] = true # We use the 'user_posted' event when you are emailed a post in a PM. opts[:notification_type] = 'posted' @@ -461,7 +460,6 @@ class UserNotifications < ActionMailer::Base notification_type: notification_type, use_invite_template: opts[:use_invite_template], use_topic_title_subject: use_topic_title_subject, - use_group_smtp_if_configured: opts[:use_group_smtp_if_configured], user: user } @@ -487,13 +485,6 @@ class UserNotifications < ActionMailer::Base group_name = opts[:group_name] locale = user_locale(user) - # this gets set in MessageBuilder if it is nil here, we just want to be - # able to override it if the group has SMTP enabled - from_address = nil - delivery_method_options = nil - use_from_address_for_reply_to = false - using_group_smtp = false - template = +"user_notifications.user_#{notification_type}" if post.topic.private_message? template << "_pm" @@ -531,41 +522,6 @@ class UserNotifications < ActionMailer::Base group = post.topic.allowed_groups&.first - # If the group has IMAP enabled, then this will be handled by - # the Jobs::GroupSmtpEmail which is enqueued from the PostAlerter - # - # use_group_smtp_if_configured is used to ensure that no notifications - # expect for specific ones that we bless (such as user_private_message) - # accidentally get sent with the group SMTP settings. - if group.present? && - group.smtp_enabled && - !group.imap_enabled && - SiteSetting.enable_smtp && - opts[:use_group_smtp_if_configured] - - port, enable_tls, enable_starttls_auto = EmailSettingsValidator.provider_specific_ssl_overrides( - group.smtp_server, group.smtp_port, group.smtp_ssl, group.smtp_ssl - ) - - delivery_method_options = { - address: group.smtp_server, - port: port, - domain: group.email_username_domain, - user_name: group.email_username, - password: group.email_password, - authentication: GlobalSetting.smtp_authentication, - enable_starttls_auto: enable_starttls_auto - } - - # We want from to be the same as the group's email_username, so if - # someone emails support@discourse.org they will get a reply from - # support@discourse.org and be able to email the SMTP email, which - # will forward the email back into Discourse and process/link it correctly. - use_from_address_for_reply_to = true - from_address = group.email_username - using_group_smtp = true - end - if post.topic.private_message? subject_pm = if opts[:show_group_in_subject] && group.present? @@ -692,7 +648,7 @@ class UserNotifications < ActionMailer::Base context: context, username: username, group_name: group_name, - add_unsubscribe_link: !user.staged && !using_group_smtp, + add_unsubscribe_link: !user.staged, mailing_list_mode: user.user_option.mailing_list_mode, unsubscribe_url: post.unsubscribe_url(user), allow_reply_by_email: allow_reply_by_email, @@ -710,10 +666,7 @@ class UserNotifications < ActionMailer::Base site_description: SiteSetting.site_description, site_title: SiteSetting.title, site_title_url_encoded: UrlHelper.encode_component(SiteSetting.title), - locale: locale, - delivery_method_options: delivery_method_options, - use_from_address_for_reply_to: use_from_address_for_reply_to, - from: from_address + locale: locale } unless translation_override_exists diff --git a/app/models/email_log.rb b/app/models/email_log.rb index 2e93daaede6..c089746ade2 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -28,7 +28,8 @@ class EmailLog < ActiveRecord::Base SELECT 1 FROM user_emails WHERE user_emails.user_id = :user_id AND - email_logs.to_address = user_emails.email + (email_logs.to_address = user_emails.email OR + email_logs.cc_addresses ILIKE '%' || user_emails.email || '%') ) SQL end diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 297a97257dd..da1f445af19 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -582,20 +582,16 @@ class PostAlerter warn_if_not_sidekiq - # Users who interacted with the post by _directly_ emailing the group - # via the group's email_username which is configured via SMTP/IMAP. - # - # This excludes people who replied via email to a user_private_message - # notification email which will have a PostReplyKey. These people should - # not be emailed again by the user_private_message notifications below. - # - # This also excludes people who emailed the group by one of its incoming_email - # addresses, e.g. somegroup+support@discoursemail.com, which is part of the - # normal group email flow and has nothing to do with SMTP/IMAP. - emails_to_skip_send = notify_group_direct_emailers(post) + # To simplify things and to avoid IMAP double sync issues, and to cut down + # on emails sent via SMTP, any topic_allowed_users (except those who are + # not_allowed?) for a group that has SMTP enabled will have their notification + # email combined into one and sent via a single group SMTP email with CC addresses. + emails_to_skip_send = email_using_group_smtp_if_configured(post) - # Users that aren't part of any mentioned groups and who did not email - # the group directly at the group's email_username. + # We create notifications for all directly_targeted_users and email those + # who do _not_ have their email addresses in the emails_to_skip_send array + # (which will include all topic allowed users' email addresses if group SMTP + # is enabled). users = directly_targeted_users(post).reject { |u| notified.include?(u) } DiscourseEvent.trigger(:before_create_notifications_for_users, users, post) users.each do |user| @@ -605,7 +601,8 @@ class PostAlerter end end - # Users that are part of all mentioned groups. + # Users that are part of all mentioned groups. Emails sent by this notification + # flow will not be sent via group SMTP if it is enabled. users = indirectly_targeted_users(post).reject { |u| notified.include?(u) } DiscourseEvent.trigger(:before_create_notifications_for_users, users, post) users.each do |user| @@ -620,27 +617,54 @@ class PostAlerter end def group_notifying_via_smtp(post) - return nil if !SiteSetting.enable_smtp || !SiteSetting.enable_imap || post.post_type != Post.types[:regular] - post.topic.allowed_groups.where(smtp_enabled: true, imap_enabled: true).first + return nil if !SiteSetting.enable_smtp || post.post_type != Post.types[:regular] + post.topic.allowed_groups.where(smtp_enabled: true).first end - def notify_group_direct_emailers(post) - email_addresses = [] + def email_using_group_smtp_if_configured(post) emails_to_skip_send = [] group = group_notifying_via_smtp(post) - return emails_to_skip_send if group.blank? - # If the post already has an incoming email, it has been set in the - # Email::Receiver or via the GroupSmtpEmail job, and thus it was created - # via the IMAP/SMTP flow, so there is no need to notify those involved - # in the email chain again. - if post.incoming_email.blank? - email_addresses = post.topic.incoming_email_addresses(group: group) - if email_addresses.any? - Jobs.enqueue(:group_smtp_email, group_id: group.id, post_id: post.id, email: email_addresses) - end + to_address = nil + cc_addresses = [] + + # We need to use topic_allowed_users here instead of directly_targeted_users + # because we want to make sure the to_address goes to the OP of the topic. + topic_allowed_users_by_age = post.topic.topic_allowed_users.includes(:user).order(:created_at).reject do |tau| + not_allowed?(tau.user, post) end + return emails_to_skip_send if topic_allowed_users_by_age.empty? + + # This should usually be the OP of the topic, unless they are the one + # replying by email (they are excluded by not_allowed? then) + to_address = topic_allowed_users_by_age.first.user.email + cc_addresses = topic_allowed_users_by_age[1..-1].map { |tau| tau.user.email } + email_addresses = [to_address, cc_addresses].flatten + + # If any of these email addresses were cc address on the + # incoming email for the target post, do not send them emails (they + # already have been notified by the CC on the email) + if post.incoming_email.present? + cc_addresses = cc_addresses - post.incoming_email.cc_addresses_split + + # If the to address is one of the recently added CC addresses, then we + # need to bail early, because otherwise we are sending a notification + # email to the user who was just added by CC. In this case the OP probably + # replied and CC'd some people, and they are the only other topic users. + return if post.incoming_email.cc_addresses_split.include?(to_address) + end + + # Send a single email using group SMTP settings to cut down on the + # number of emails sent via SMTP, also to replicate how support systems + # and group inboxes generally work in other systems. + Jobs.enqueue( + :group_smtp_email, + group_id: group.id, + post_id: post.id, + email: to_address, + cc_emails: cc_addresses + ) # Add the group's email_username into the array, because it is used for # skip_send_email_to in the case of user private message notifications @@ -648,7 +672,7 @@ class PostAlerter # will make another email for IMAP to pick up in the group's mailbox) emails_to_skip_send = email_addresses.dup if email_addresses.any? emails_to_skip_send << group.email_username - emails_to_skip_send + emails_to_skip_send.uniq end def notify_post_users(post, notified, group_ids: nil, include_topic_watchers: true, include_category_watchers: true, include_tag_watchers: true, new_record: false) diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index fa42814bb24..187bfc364d4 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -140,7 +140,8 @@ module Email subject: subject, body: body, charset: 'UTF-8', - from: from_value + from: from_value, + cc: @opts[:cc] } args[:delivery_method_options] = @opts[:delivery_method_options] if @opts[:delivery_method_options] @@ -161,11 +162,24 @@ module Email # please, don't send us automatic responses... result['X-Auto-Response-Suppress'] = 'All' - 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 + if !allow_reply_by_email? + # This will end up being the notification_email, which is a + # noreply address. result['Reply-To'] = from_value + else + + # The only reason we use from address for reply to is for group + # SMTP emails, where the person will be replying to the group's + # email_username. + if !@opts[:use_from_address_for_reply_to] + result[ALLOW_REPLY_BY_EMAIL_HEADER] = true + result['Reply-To'] = reply_by_email_address + else + # No point in adding a reply-to header if it is going to be identical + # to the from address/alias. If the from option is not present, then + # the default reply-to address is used. + result['Reply-To'] = from_value if from_value != alias_email(@opts[:from]) + end end result.merge(MessageBuilder.custom_headers(SiteSetting.email_custom_headers)) diff --git a/lib/email/sender.rb b/lib/email/sender.rb index 75a3a978640..aafd834bfd0 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -429,6 +429,8 @@ module Email end def set_reply_key(post_id, user_id) + # ALLOW_REPLY_BY_EMAIL_HEADER is only added if we are _not_ sending + # via group SMTP and if reply by email site settings are configured return if !user_id || !post_id || !header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present? # use safe variant here cause we tend to see concurrency issue diff --git a/spec/components/email/message_builder_spec.rb b/spec/components/email/message_builder_spec.rb index 9043652b7c1..75fe28347fa 100644 --- a/spec/components/email/message_builder_spec.rb +++ b/spec/components/email/message_builder_spec.rb @@ -152,12 +152,15 @@ describe Email::MessageBuilder do context "header args" do + let(:additional_opts) { {} } let(:message_with_header_args) do Email::MessageBuilder.new( to_address, + { body: 'hello world', topic_id: 1234, post_id: 4567, + }.merge(additional_opts) ) end @@ -169,6 +172,42 @@ describe Email::MessageBuilder do expect(message_with_header_args.header_args['X-Discourse-Topic-Id']).to eq('1234') end + it "uses the default reply-to header" do + expect(message_with_header_args.header_args['Reply-To']).to eq("\"Discourse\" <#{SiteSetting.notification_email}>") + end + + context "when allow_reply_by_email is enabled " do + let(:additional_opts) { { allow_reply_by_email: true } } + + it "uses the reply by email address if that is enabled" do + SiteSetting.manual_polling_enabled = true + SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com" + SiteSetting.reply_by_email_enabled = true + expect(message_with_header_args.header_args['Reply-To']).to eq("\"Discourse\" ") + end + end + + context "when allow_reply_by_email is enabled and use_from_address_for_reply_to is enabled but no from address is specified" do + let(:additional_opts) { { allow_reply_by_email: true, use_from_address_for_reply_to: true } } + + it "uses the notification_email address, the default reply-to header" do + SiteSetting.manual_polling_enabled = true + SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com" + SiteSetting.reply_by_email_enabled = true + expect(message_with_header_args.header_args['Reply-To']).to eq("\"Discourse\" <#{SiteSetting.notification_email}>") + end + end + + context "when allow_reply_by_email is enabled and use_from_address_for_reply_to is enabled and from is specified" do + let(:additional_opts) { { allow_reply_by_email: true, use_from_address_for_reply_to: true, from: "team@test.com" } } + + it "removes the reply-to header because it is identical to the from header" do + SiteSetting.manual_polling_enabled = true + SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com" + SiteSetting.reply_by_email_enabled = true + expect(message_with_header_args.header_args['Reply-To']).to eq(nil) + end + end end context "unsubscribe link" do @@ -337,7 +376,5 @@ describe Email::MessageBuilder do SiteSetting.stubs(:email_site_title).returns("::>>>Best \"Forum\", EU: Award Winning<<<") expect(build_args[:from]).to eq("\"Best Forum EU Award Winning\" <#{SiteSetting.notification_email}>") end - end - end diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index fbdbacb7c3c..34c1a2e36a5 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -343,11 +343,10 @@ describe Email::Sender do let(:reply) { Fabricate(:post, topic: post.topic, reply_to_user: post.user, reply_to_post_number: post.post_number) } let(:notification) { Fabricate(:posted_notification, user: post.user, post: reply) } let(:message) do - UserNotifications.user_private_message( - post.user, - post: reply, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash + GroupSmtpMailer.send_mail( + group, + post.user.email, + post ) end let(:group) { Fabricate(:smtp_group) } diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index a3456c510b0..ffba33863d4 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -4,63 +4,91 @@ require 'rails_helper' RSpec.describe Jobs::GroupSmtpEmail do fab!(:post) do - topic = Fabricate(:topic) + topic = Fabricate(:topic, title: "Help I need support") Fabricate(:post, topic: topic) Fabricate(:post, topic: topic) end - fab!(:group) { Fabricate(:imap_group) } + fab!(:group) { Fabricate(:smtp_group, name: "support-group", full_name: "Support 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, - email: "test@test.com" + email: "test@test.com", + cc_emails: ["otherguy@test.com", "cormac@lit.com"] } end before do - SiteSetting.reply_by_email_address = "test+%{reply_key}@incoming.com" - SiteSetting.manual_polling_enabled = true - SiteSetting.reply_by_email_enabled = true SiteSetting.enable_smtp = true + SiteSetting.manual_polling_enabled = true + SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com" + SiteSetting.reply_by_email_enabled = true end 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, recipient_user).returns(stub(send: nil)) + GroupSmtpMailer.expects(:send_mail).with(group, "test@test.com", post, ["otherguy@test.com", "cormac@lit.com"]).returns(message) subject.execute(args) end - it "creates an IncomingEmail record to avoid double processing via IMAP" do + it "creates an EmailLog record with the correct details" 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}/#{post.id}@test.localhost") - expect(incoming.created_via).to eq(IncomingEmail.created_via_types[:group_smtp]) + email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) + expect(email_log).not_to eq(nil) + expect(email_log.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 + it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do subject.execute(args) - incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id) + incoming_email = IncomingEmail.find_by(post_id: post.id, topic_id: post.topic_id, user_id: post.user.id) + expect(incoming_email).not_to eq(nil) + expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") + expect(incoming_email.created_via).to eq(IncomingEmail.created_via_types[:group_smtp]) + expect(incoming_email.to_addresses).to eq("test@test.com") + expect(incoming_email.cc_addresses).to eq("otherguy@test.com;cormac@lit.com") + expect(incoming_email.subject).to eq("Re: Help I need support") + end + + it "does not create a post reply key, it always replies to the group email_username" do + subject.execute(args) + email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) 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 ") + expect(post_reply_key).to eq(nil) + expect(email_log.raw).not_to include("Reply-To: Support Group via Discourse <#{group.email_username}") + expect(email_log.raw).to include("From: Support Group via Discourse <#{group.email_username}") end - it "has the from_address and the to_addresses and subject filled in correctly" do + it "falls back to the group name if full name is blank" do + group.update(full_name: "") 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") + email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) + expect(email_log.raw).to include("From: support-group via Discourse <#{group.email_username}") + end + + it "has the group_smtp_id and the to_address filled in correctly" do + subject.execute(args) + email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) + expect(email_log.to_address).to eq("test@test.com") + expect(email_log.smtp_group_id).to eq(group.id) + end + + context "when there are cc_addresses" do + let!(:cormac_user) { Fabricate(:user, email: "cormac@lit.com") } + + it "has the cc_addresses and cc_user_ids filled in correctly" do + subject.execute(args) + email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) + expect(email_log.cc_addresses).to eq("otherguy@test.com;cormac@lit.com") + expect(email_log.cc_user_ids).to eq([cormac_user.id]) + end 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 }) + expect { subject.execute(args) }.not_to(change { EmailLog.count }) end end end diff --git a/spec/mailers/group_smtp_mailer_spec.rb b/spec/mailers/group_smtp_mailer_spec.rb index 0eedddab14e..afca00f11e5 100644 --- a/spec/mailers/group_smtp_mailer_spec.rb +++ b/spec/mailers/group_smtp_mailer_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' require 'email/receiver' describe GroupSmtpMailer do - let(:group) { + let(:group) do Fabricate(:group, name: 'Testers', title: 'Tester', @@ -19,15 +19,15 @@ describe GroupSmtpMailer do email_username: 'bugs@gmail.com', email_password: 'super$secret$password' ) - } + end - let(:user) { + let(:user) do user = Fabricate(:user) group.add_owner(user) user - } + end - let(:email) { + let(:email) do <<~EOF Delivered-To: bugs@gmail.com MIME-Version: 1.0 @@ -42,17 +42,18 @@ describe GroupSmtpMailer do How are you doing? EOF - } + end - let(:receiver) { - receiver = Email::Receiver.new(email, - destinations: [group], - uid_validity: 1, - uid: 10000 - ) - receiver.process! - receiver - } + let(:receiver) do + receiver = Email::Receiver.new( + email, + destinations: [group], + uid_validity: 1, + uid: 10000 + ) + receiver.process! + receiver + end let(:raw) { 'hello, how are you doing?' } @@ -60,6 +61,9 @@ describe GroupSmtpMailer do SiteSetting.enable_smtp = true SiteSetting.enable_imap = true Jobs.run_immediately! + SiteSetting.manual_polling_enabled = true + SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com" + SiteSetting.reply_by_email_enabled = true end it 'sends an email as reply' do @@ -72,11 +76,21 @@ describe GroupSmtpMailer do 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.reply_to).to eq(nil) expect(sent_mail.subject).to eq('Re: Hello from John') expect(sent_mail.to_s).to include(raw) end + it "uses the OP incoming email subject for the subject over topic title" do + receiver.incoming_email.topic.update(title: "blah") + post = PostCreator.create(user, + topic_id: receiver.incoming_email.topic.id, + raw: raw + ) + sent_mail = ActionMailer::Base.deliveries[0] + expect(sent_mail.subject).to eq('Re: Hello from John') + end + context "when the site has a reply by email address configured" do before do SiteSetting.manual_polling_enabled = true @@ -84,7 +98,7 @@ describe GroupSmtpMailer do SiteSetting.reply_by_email_enabled = true end - it 'uses the correct IMAP/SMTP reply to address' do + it 'uses the correct IMAP/SMTP reply to address and does not create a post reply key' do post = PostCreator.create(user, topic_id: receiver.incoming_email.topic.id, raw: raw @@ -92,8 +106,11 @@ describe GroupSmtpMailer do expect(ActionMailer::Base.deliveries.size).to eq(1) + expect(PostReplyKey.find_by(user_id: user.id, post_id: post.id)).to eq(nil) + sent_mail = ActionMailer::Base.deliveries[0] - expect(sent_mail.reply_to).to contain_exactly('bugs@gmail.com') + expect(sent_mail.reply_to).to eq(nil) + expect(sent_mail.from).to contain_exactly('bugs@gmail.com') end context "when IMAP is disabled for the group" do @@ -101,6 +118,21 @@ describe GroupSmtpMailer do group.update(imap_enabled: false) end + it "does send the email" do + post = PostCreator.create(user, + topic_id: receiver.incoming_email.topic.id, + raw: raw + ) + + expect(ActionMailer::Base.deliveries.size).to eq(1) + end + end + + context "when SMTP is disabled for the group" do + before do + group.update(smtp_enabled: false) + end + it "does not send the email" do post = PostCreator.create(user, topic_id: receiver.incoming_email.topic.id, diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 344cc55a8ff..3edb1f3b331 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -611,84 +611,6 @@ describe UserNotifications do expect(mail.body).to include("[group1 (2)](http://test.localhost/groups/group1), [group2 (1)](http://test.localhost/groups/group2), [one](http://test.localhost/u/one), [two](http://test.localhost/u/two)") end - context "when group smtp is configured and SiteSetting.enable_smtp" do - let!(:group1) do - Fabricate( - :group, - name: "group1", - smtp_enabled: true, - smtp_port: 587, - smtp_ssl: true, - smtp_server: "smtp.test.com", - email_username: "user@test.com", - email_password: "password" - ) - end - - before do - SiteSetting.enable_smtp = true - topic.allowed_groups = [group1] - end - - it "uses the from address, which is the group's email_username, for reply-to" do - mail = UserNotifications.user_private_message( - user, - post: response, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash - ) - - expect(mail.from).to eq([group1.email_username]) - expect(mail.reply_to).to eq([group1.email_username]) - end - - it "uses the SMTP settings from the group for delivery" do - mail = UserNotifications.user_private_message( - user, - post: response, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash - ) - - delivery_method = mail.delivery_method.settings - expect(delivery_method[:port]).to eq(group1.smtp_port) - expect(delivery_method[:address]).to eq(group1.smtp_server) - expect(delivery_method[:domain]).to eq("test.com") - expect(delivery_method[:password]).to eq("password") - expect(delivery_method[:user_name]).to eq("user@test.com") - end - - context "when imap is configured for the group" do - before do - group1.update( - imap_server: "imap.test.com", - imap_port: 993, - imap_ssl: true, - imap_enabled: true, - imap_mailbox_name: "All Mail" - ) - end - - it "does not use group SMTP settings for delivery, this is handled by Jobs::GroupSmtpEmail" do - mail = UserNotifications.user_private_message( - user, - post: response, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash - ) - - expect(mail.from).to eq([SiteSetting.notification_email]) - expect(mail.reply_to).to eq([SiteSetting.notification_email]) - delivery_method = mail.delivery_method.settings - expect(delivery_method[:port]).not_to eq(group1.smtp_port) - expect(delivery_method[:address]).not_to eq(group1.smtp_server) - expect(delivery_method[:domain]).not_to eq("test.com") - expect(delivery_method[:password]).not_to eq("password") - expect(delivery_method[:user_name]).not_to eq("user@test.com") - end - end - end - context "when SiteSetting.group_name_in_subject is true" do before do SiteSetting.group_in_subject = true diff --git a/spec/models/email_log_spec.rb b/spec/models/email_log_spec.rb index 0e6206bb1fe..60064f4b86d 100644 --- a/spec/models/email_log_spec.rb +++ b/spec/models/email_log_spec.rb @@ -138,4 +138,27 @@ describe EmailLog do end end end + + describe ".addressed_to_user scope" do + let(:user) { Fabricate(:user, email: "test@test.com") } + before do + Fabricate(:email_log, to_address: "john@smith.com") + Fabricate(:email_log, cc_addresses: "jane@jones.com;elle@someplace.org") + user.reload + end + + it "returns email logs where the to address matches" do + user.user_emails.first.update!(email: "john@smith.com") + expect(EmailLog.addressed_to_user(user).count).to eq(1) + end + + it "returns email logs where a cc address matches" do + user.user_emails.first.update!(email: "elle@someplace.org") + expect(EmailLog.addressed_to_user(user).count).to eq(1) + end + + it "returns nothing if no emails match" do + expect(EmailLog.addressed_to_user(user).count).to eq(0) + end + end end diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index ea7cd26f0a7..c9d4773a879 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -1295,7 +1295,7 @@ describe PostAlerter do context "SMTP (group_smtp_email)" do before do SiteSetting.enable_smtp = true - SiteSetting.enable_imap = true + SiteSetting.email_in = true Jobs.run_immediately! end @@ -1315,99 +1315,257 @@ describe PostAlerter do ) end - fab!(:topic) do - Fabricate( - :private_message_topic, - topic_allowed_groups: [ - Fabricate.build(:topic_allowed_group, group: group) - ] - ) - end - def create_post_with_incoming - Fabricate( - :post, - topic: topic, - incoming_email: - Fabricate( - :incoming_email, - topic: topic, - from_address: "foo@discourse.org", - to_addresses: group.email_username, - cc_addresses: "bar@discourse.org" - ) - ) + raw_mail = <<~MAIL + From: Foo + To: discourse@example.com + Cc: bar@discourse.org, jim@othersite.com + Subject: Full email group username flow + Date: Fri, 15 Jan 2021 00:12:43 +0100 + Message-ID: + Mime-Version: 1.0 + Content-Type: text/plain + Content-Transfer-Encoding: 7bit + + This is the first email. + MAIL + + Email::Receiver.new(raw_mail, {}).process! end - it "does not send a group smtp email when the post already has an incoming email" do - post = create_post_with_incoming - - expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) - end - - it "sends a group smtp email when the post does not have an incoming email" do - create_post_with_incoming + it "sends a group smtp email because SMTP is enabled for the site and the group" do + incoming_email_post = create_post_with_incoming + topic = incoming_email_post.topic post = Fabricate(:post, topic: topic) expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(1) email = ActionMailer::Base.deliveries.last expect(email.from).to include(group.email_username) - expect(email.to).to contain_exactly("foo@discourse.org", "bar@discourse.org") + expect(email.to).to contain_exactly(topic.reload.topic_allowed_users.order(:created_at).first.user.email) + expect(email.cc).to match_array(["bar@discourse.org", "jim@othersite.com"]) expect(email.subject).to eq("Re: #{topic.title}") end - it "does not send a group smtp email if imap is not enabled for the group" do - group.update!(imap_enabled: false) - create_post_with_incoming - post = Fabricate(:post, topic: topic) - expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) - end - - it "does not send a group smtp email if SiteSetting.enable_imap is false" do - SiteSetting.enable_imap = false - create_post_with_incoming + it "does not send a group smtp email if smtp is not enabled for the group" do + group.update!(smtp_enabled: false) + incoming_email_post = create_post_with_incoming + topic = incoming_email_post.topic post = Fabricate(:post, topic: topic) expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) end it "does not send a group smtp email if SiteSetting.enable_smtp is false" do SiteSetting.enable_smtp = false - create_post_with_incoming + incoming_email_post = create_post_with_incoming + topic = incoming_email_post.topic post = Fabricate(:post, topic: topic) expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) end it "does not send group smtp emails for a whisper" do - create_post_with_incoming + incoming_email_post = create_post_with_incoming + topic = incoming_email_post.topic post = Fabricate(:post, topic: topic, post_type: Post.types[:whisper]) expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) end - it "does not send a notification email to the group when the post does not have an incoming email" do - PostAlerter.any_instance.expects(:create_notification).with(kind_of(User), Notification.types[:private_message], kind_of(Post), skip_send_email_to: ["discourse@example.com"]).at_least_once - post = create_post_with_incoming - staged_group_user = Fabricate(:staged, email: "discourse@example.com") - Fabricate(:topic_user, user: staged_group_user, topic: post.topic) - topic.allowed_users << staged_group_user - topic.save - expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) + it "skips sending a notification email to the group and all other email addresses that are _not_ members of the group, + sends a group_smtp_email instead" do + NotificationEmailer.enable + + incoming_email_post = create_post_with_incoming + topic = incoming_email_post.topic + + group_user1 = Fabricate(:group_user, group: group) + group_user2 = Fabricate(:group_user, group: group) + TopicUser.create(user: group_user1.user, notification_level: TopicUser.notification_levels[:watching], topic: topic) + post = Fabricate(:post, topic: topic.reload) + + # Sends an email for: + # + # 1. the group user that is watching the post (but does not send this email with group SMTO) + # 2. the group smtp email to notify all topic_users not in the group + expect { PostAlerter.new.after_save_post(post, true) }.to change { + ActionMailer::Base.deliveries.size + }.by(2).and change { Notification.count }.by(2) + + # The group smtp email + email = ActionMailer::Base.deliveries.first + expect(email.from).to eq([group.email_username]) + expect(email.to).to contain_exactly("foo@discourse.org") + expect(email.cc).to match_array(["bar@discourse.org", "jim@othersite.com"]) + expect(email.subject).to eq("Re: #{topic.title}") + + # The watching group user notification email + email = ActionMailer::Base.deliveries.last + expect(email.from).to eq([SiteSetting.notification_email]) + expect(email.to).to contain_exactly(group_user1.user.email) + expect(email.cc).to eq(nil) + expect(email.subject).to eq("[Discourse] [PM] #{topic.title}") end - it "skips sending a notification email to the group and all other incoming email addresses" do + it "skips sending a notification email to the cc address that was added on the same post with an incoming email" do + NotificationEmailer.enable - create_post_with_incoming - PostAlerter.any_instance.expects(:create_notification).with(kind_of(User), Notification.types[:private_message], kind_of(Post), skip_send_email_to: ["foo@discourse.org", "bar@discourse.org", "discourse@example.com"]).at_least_once + incoming_email_post = create_post_with_incoming + topic = incoming_email_post.topic post = Fabricate(:post, topic: topic.reload) - staged_group_user = Fabricate(:staged, email: "discourse@example.com") - Fabricate(:topic_user, user: staged_group_user, topic: post.topic) - topic.allowed_users << staged_group_user - topic.save - - expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(1) + expect { PostAlerter.new.after_save_post(post, true) }.to change { + ActionMailer::Base.deliveries.size + }.by(1).and change { Notification.count }.by(1) email = ActionMailer::Base.deliveries.last + + # the reply post from someone who was emailed + reply_raw_mail = <<~MAIL + From: Bar + To: discourse@example.com + Cc: someothernewcc@baz.com, finalnewcc@doom.com + Subject: #{email.subject} + Date: Fri, 16 Jan 2021 00:12:43 +0100 + Message-ID: + In-Reply-To: #{email.message_id} + Mime-Version: 1.0 + Content-Type: text/plain + Content-Transfer-Encoding: 7bit + + Hey here is my reply! + MAIL + + reply_post_from_email = nil + expect { + reply_post_from_email = Email::Receiver.new(reply_raw_mail, {}).process! + }.to change { + User.count # the two new cc addresses have users created + }.by(2).and change { + TopicAllowedUser.where(topic: topic).count # and they are added as topic allowed users + }.by(2).and change { + # but they are not sent emails because they were cc'd on an email, only jim@othersite.com + # is emailed because he is a topic allowed user cc'd on the _original_ email and he is not + # the one creating the post, and foo@discourse.org, who is the OP of the topic + ActionMailer::Base.deliveries.size + }.by(1).and change { + Notification.count # and they are still sent their normal discourse notification + }.by(2) + + email = ActionMailer::Base.deliveries.last + + expect(email.to).to eq(["foo@discourse.org"]) + expect(email.cc).to eq(["jim@othersite.com"]) expect(email.from).to eq([group.email_username]) - expect(email.to).to contain_exactly("foo@discourse.org", "bar@discourse.org") expect(email.subject).to eq("Re: #{topic.title}") end + + it "handles the OP of the topic replying by email and sends a group email to the other topic allowed users successfully" do + NotificationEmailer.enable + + incoming_email_post = create_post_with_incoming + topic = incoming_email_post.topic + post = Fabricate(:post, topic: topic.reload) + expect { PostAlerter.new.after_save_post(post, true) }.to change { + ActionMailer::Base.deliveries.size + }.by(1).and change { Notification.count }.by(1) + email = ActionMailer::Base.deliveries.last + + # the reply post from someone who was emailed + reply_raw_mail = <<~MAIL + From: Foo + To: discourse@example.com + Cc: someothernewcc@baz.com, finalnewcc@doom.com + Subject: #{email.subject} + Date: Fri, 16 Jan 2021 00:12:43 +0100 + Message-ID: + In-Reply-To: #{email.message_id} + Mime-Version: 1.0 + Content-Type: text/plain + Content-Transfer-Encoding: 7bit + + I am ~~Commander Shepherd~~ the OP and I approve of this message. + MAIL + + reply_post_from_email = nil + expect { + reply_post_from_email = Email::Receiver.new(reply_raw_mail, {}).process! + }.to change { + User.count # the two new cc addresses have users created + }.by(2).and change { + TopicAllowedUser.where(topic: topic).count # and they are added as topic allowed users + }.by(2).and change { + # but they are not sent emails because they were cc'd on an email, only jim@othersite.com + # is emailed because he is a topic allowed user cc'd on the _original_ email and he is not + # the one creating the post + ActionMailer::Base.deliveries.size + }.by(1).and change { + Notification.count # and they are still sent their normal discourse notification + }.by(2) + + email = ActionMailer::Base.deliveries.last + + expect(email.to).to eq(["bar@discourse.org"]) + expect(email.cc).to eq(["jim@othersite.com"]) + expect(email.from).to eq([group.email_username]) + expect(email.subject).to eq("Re: #{topic.title}") + end + + it "handles the OP of the topic replying by email and cc'ing new people, and does not send a group SMTP email to those newly cc'd users" do + NotificationEmailer.enable + + # this is a special case where we are not CC'ing on the original email, + # only on the follow up email + raw_mail = <<~MAIL + From: Foo + To: discourse@example.com + Subject: Full email group username flow + Date: Fri, 14 Jan 2021 00:12:43 +0100 + Message-ID: + Mime-Version: 1.0 + Content-Type: text/plain + Content-Transfer-Encoding: 7bit + + This is the first email. + MAIL + + incoming_email_post = Email::Receiver.new(raw_mail, {}).process! + topic = incoming_email_post.topic + post = Fabricate(:post, topic: topic.reload) + expect { PostAlerter.new.after_save_post(post, true) }.to change { + ActionMailer::Base.deliveries.size + }.by(1).and change { Notification.count }.by(1) + email = ActionMailer::Base.deliveries.last + + # the reply post from the OP, cc'ing new people in + reply_raw_mail = <<~MAIL + From: Foo + To: discourse@example.com + Cc: someothernewcc@baz.com, finalnewcc@doom.com + Subject: #{email.subject} + Date: Fri, 16 Jan 2021 00:12:43 +0100 + Message-ID: <3849cu9843yncr9834yr9348x934@discourse.org.mail> + In-Reply-To: #{email.message_id} + Mime-Version: 1.0 + Content-Type: text/plain + Content-Transfer-Encoding: 7bit + + I am inviting my mates to this email party. + MAIL + + reply_post_from_email = nil + expect { + reply_post_from_email = Email::Receiver.new(reply_raw_mail, {}).process! + }.to change { + User.count # the two new cc addresses have users created + }.by(2).and change { + TopicAllowedUser.where(topic: topic).count # and they are added as topic allowed users + }.by(2).and change { + # but they are not sent emails because they were cc'd on an email. + # no group smtp message is sent because the OP is not sent an email, + # they made this post. + ActionMailer::Base.deliveries.size + }.by(0).and change { + Notification.count # and they are still sent their normal discourse notification + }.by(2) + + last_email = ActionMailer::Base.deliveries.last + expect(email).to eq(last_email) + end end end