From 7fca7fb7ff09c9b53f82a7ee03bc4dfea384942d Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 15 Jun 2021 11:29:46 +1000 Subject: [PATCH] DEV: Add SMTP group ID to EmailLog (#13381) Adds a new `smtp_group_id` column to `EmailLog` which is filled in if the mail `from_address` matches a group's `email_username`. This is for easier debugging, so we know which emails have been sent via group SMTP. --- app/mailers/user_notifications.rb | 3 -- app/models/email_log.rb | 36 ++++++++++--------- ...14232334_add_smtp_group_id_to_email_log.rb | 7 ++++ lib/email/sender.rb | 7 ++++ spec/components/email/sender_spec.rb | 30 ++++++++++++++++ spec/fabricators/group_fabricator.rb | 9 +++++ 6 files changed, 73 insertions(+), 19 deletions(-) create mode 100644 db/migrate/20210614232334_add_smtp_group_id_to_email_log.rb diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index acd10a0451c..a63c7b14477 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -546,9 +546,6 @@ class UserNotifications < ActionMailer::Base group.smtp_server, group.smtp_port, group.smtp_ssl, group.smtp_ssl ) - # TODO (martin): Remove this once testing is over and this is more stable. - Rails.logger.warn("Using SMTP settings from group #{group.name} (#{group.id}) to send user notification for topic #{post.topic.id} and user #{user.id} (#{user.email})") - delivery_method_options = { address: group.smtp_server, port: port, diff --git a/app/models/email_log.rb b/app/models/email_log.rb index 33c8ea12e73..0770698b753 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -16,6 +16,8 @@ class EmailLog < ActiveRecord::Base belongs_to :user belongs_to :post + belongs_to :smtp_group, class_name: 'Group' + has_one :topic, through: :post validates :email_type, :to_address, presence: true @@ -89,23 +91,25 @@ end # # Table name: email_logs # -# id :integer not null, primary key -# to_address :string not null -# email_type :string not null -# user_id :integer -# created_at :datetime not null -# updated_at :datetime not null -# post_id :integer -# bounce_key :uuid -# bounced :boolean default(FALSE), not null -# message_id :string +# id :integer not null, primary key +# to_address :string not null +# email_type :string not null +# user_id :integer +# created_at :datetime not null +# updated_at :datetime not null +# post_id :integer +# bounce_key :uuid +# bounced :boolean default(FALSE), not null +# message_id :string +# smtp_group_id :integer # # Indexes # -# index_email_logs_on_bounce_key (bounce_key) UNIQUE WHERE (bounce_key IS NOT NULL) -# index_email_logs_on_bounced (bounced) -# index_email_logs_on_created_at (created_at) -# index_email_logs_on_message_id (message_id) -# index_email_logs_on_post_id (post_id) -# index_email_logs_on_user_id (user_id) +# idx_email_logs_on_smtp_group_id (smtp_group_id) +# index_email_logs_on_bounce_key (bounce_key) UNIQUE WHERE (bounce_key IS NOT NULL) +# index_email_logs_on_bounced (bounced) +# index_email_logs_on_created_at (created_at) +# index_email_logs_on_message_id (message_id) +# index_email_logs_on_post_id (post_id) +# index_email_logs_on_user_id (user_id) # diff --git a/db/migrate/20210614232334_add_smtp_group_id_to_email_log.rb b/db/migrate/20210614232334_add_smtp_group_id_to_email_log.rb new file mode 100644 index 00000000000..bd177fdbb92 --- /dev/null +++ b/db/migrate/20210614232334_add_smtp_group_id_to_email_log.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddSmtpGroupIdToEmailLog < ActiveRecord::Migration[6.1] + def change + add_column :email_logs, :smtp_group_id, :integer, null: true, index: true + end +end diff --git a/lib/email/sender.rb b/lib/email/sender.rb index ffd9707d328..6b634203fc6 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -97,6 +97,7 @@ module Email post_id = header_value('X-Discourse-Post-Id') topic_id = header_value('X-Discourse-Topic-Id') reply_key = set_reply_key(post_id, user_id) + from_address = @message.from&.first # always set a default Message ID from the host @message.header['Message-ID'] = "<#{SecureRandom.uuid}@#{host}>" @@ -228,6 +229,12 @@ module Email email_log.message_id = @message.message_id + # Log when a message is being sent from a group SMTP address, so we + # can debug deliverability issues. + if from_address && smtp_group_id = Group.where(email_username: from_address, smtp_enabled: true).pluck_first(:id) + email_log.smtp_group_id = smtp_group_id + end + DiscourseEvent.trigger(:before_email_send, @message, @email_type) begin diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index bacf426dbd9..603dd76bd33 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -337,6 +337,36 @@ describe Email::Sender do expect(email_log.to_address).to eq('eviltrout@test.domain') expect(email_log.user_id).to be_blank end + + context 'when the email is sent using group SMTP credentials' 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 + ) + end + let(:group) { Fabricate(:smtp_group) } + + before do + SiteSetting.enable_smtp = true + end + + it 'adds the group id to the email log' do + TopicAllowedGroup.create(topic: post.topic, group: group) + + email_sender.send + + expect(email_log).to be_present + expect(email_log.email_type).to eq('valid_type') + expect(email_log.to_address).to eq(post.user.email) + expect(email_log.user_id).to be_blank + expect(email_log.smtp_group_id).to eq(group.id) + end + end end context "email log with a post id and topic id" do diff --git a/spec/fabricators/group_fabricator.rb b/spec/fabricators/group_fabricator.rb index da0ac73d7f4..de9993d3628 100644 --- a/spec/fabricators/group_fabricator.rb +++ b/spec/fabricators/group_fabricator.rb @@ -24,3 +24,12 @@ Fabricator(:imap_group, from: :group) do email_username "discourseteam@ponyexpress.com" email_password "test" end + +Fabricator(:smtp_group, from: :group) do + smtp_server "smtp.ponyexpress.com" + smtp_port 587 + smtp_ssl true + smtp_enabled true + email_username "discourseteam@ponyexpress.com" + email_password "test" +end