FEATURE: Add more columns to outbound EmailLog (#13449)

This adds the following columns to EmailLog:

* cc_addresses
* cc_user_ids
* topic_id
* raw

This is to bring the EmailLog table closer in parity to
IncomingEmail so it can be better utilized for Group SMTP
and IMAP mailing.

The raw column contains the full content of the outbound email,
but _only_ if the new hidden site setting
enable_raw_outbound_email_logging is enabled. Most sites do not
need it, and it's mostly required for IMAP and SMTP sending.

In the next pull request, there will be a migration to backfill
topic_id on the EmailLog table, at which point we can remove the
topic fallback method on EmailLog.
This commit is contained in:
Martin Brennan 2021-06-22 08:32:01 +10:00 committed by GitHub
parent c3e4389b81
commit 5222247746
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 124 additions and 11 deletions

View File

@ -18,8 +18,6 @@ class EmailLog < ActiveRecord::Base
belongs_to :post
belongs_to :smtp_group, class_name: 'Group'
has_one :topic, through: :post
validates :email_type, :to_address, presence: true
scope :bounced, -> { where(bounced: true) }
@ -40,6 +38,10 @@ class EmailLog < ActiveRecord::Base
User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present?
end
def topic
@topic ||= self.topic_id.present? ? Topic.find_by(id: self.topic_id) : self.post&.topic
end
def self.unique_email_per_post(post, user)
return yield unless post && user
@ -85,6 +87,14 @@ class EmailLog < ActiveRecord::Base
super&.delete('-')
end
def cc_users
return [] if !self.cc_user_ids
@cc_users ||= User.where(id: self.cc_user_ids)
end
def cc_addresses_split
@cc_addresses_split ||= self.cc_addresses&.split(";") || []
end
end
# == Schema Information
@ -102,14 +112,18 @@ end
# bounced :boolean default(FALSE), not null
# message_id :string
# smtp_group_id :integer
# cc_addresses :text
# cc_user_ids :integer is an Array
# raw :text
# topic_id :integer
#
# Indexes
#
# 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)
# 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_topic_id (topic_id) WHERE (topic_id IS NOT NULL)
# index_email_logs_on_user_id (user_id)
#

View File

@ -0,0 +1,21 @@
# frozen_string_literal: true
class AddColumnsToEmailLogToMatchIncomingForSmtpImap < ActiveRecord::Migration[6.1]
def up
add_column :email_logs, :cc_addresses, :text, null: true
add_column :email_logs, :cc_user_ids, :integer, array: true, null: true
add_column :email_logs, :raw, :text, null: true
add_column :email_logs, :topic_id, :integer, null: true
add_index :email_logs, :topic_id, where: 'topic_id IS NOT NULL'
end
def down
remove_column :email_logs, :cc_addresses if column_exists?(:email_logs, :cc_addresses)
remove_column :email_logs, :cc_user_ids if column_exists?(:email_logs, :cc_user_ids)
remove_column :email_logs, :raw if column_exists?(:email_logs, :raw)
remove_column :email_logs, :topic_id if column_exists?(:email_logs, :topic_id)
remove_index :email_logs, :topic_id if index_exists?(:email_logs, [:topic_id])
end
end

View File

@ -92,6 +92,11 @@ module Email
user_id: user_id
)
if cc_addresses.any?
email_log.cc_addresses = cc_addresses.join(";")
email_log.cc_user_ids = User.with_email(cc_addresses).pluck(:id)
end
host = Email::Sender.host_for(Discourse.base_url)
post_id = header_value('X-Discourse-Post-Id')
@ -201,6 +206,7 @@ module Email
end
email_log.post_id = post_id if post_id.present?
email_log.topic_id = topic_id if topic_id.present?
# Remove headers we don't need anymore
@message.header['X-Discourse-Topic-Id'] = nil if topic_id.present?
@ -243,7 +249,16 @@ module Email
# Log when a message is being sent from a group SMTP address, so we
# can debug deliverability issues.
email_log.smtp_group_id = smtp_group_id
if smtp_group_id
email_log.smtp_group_id = smtp_group_id
# Store contents of all outgoing emails using group SMTP
# for greater visibility and debugging. If the size of this
# gets out of hand, we should look into a group-level setting
# to enable this; size should be kept in check by regular purging
# of EmailLog though.
email_log.raw = Email::Cleaner.new(@message).execute
end
DiscourseEvent.trigger(:before_email_send, @message, @email_type)
@ -270,6 +285,12 @@ module Email
end
end
def cc_addresses
@cc_addresses ||= begin
@message.try(:cc) || []
end
end
def self.host_for(base_url)
host = "localhost"
if base_url.present?

View File

@ -336,6 +336,7 @@ describe Email::Sender do
expect(email_log.email_type).to eq('valid_type')
expect(email_log.to_address).to eq('eviltrout@test.domain')
expect(email_log.user_id).to be_blank
expect(email_log.raw).to eq(nil)
end
context 'when the email is sent using group SMTP credentials' do
@ -355,7 +356,7 @@ describe Email::Sender do
SiteSetting.enable_smtp = true
end
it 'adds the group id to the email log' do
it 'adds the group id and raw content to the email log' do
TopicAllowedGroup.create(topic: post.topic, group: group)
email_sender.send
@ -365,6 +366,7 @@ describe Email::Sender do
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)
expect(email_log.raw).to include("Hello world")
end
it "does not add any of the mailing list headers" do
@ -393,6 +395,7 @@ describe Email::Sender do
it 'should create the right log' do
email_sender.send
expect(email_log.post_id).to eq(post.id)
expect(email_log.topic_id).to eq(topic.id)
expect(email_log.topic.id).to eq(topic.id)
end
end
@ -687,4 +690,30 @@ describe Email::Sender do
end
end
context "with cc addresses" do
let(:message) do
message = Mail::Message.new to: 'eviltrout@test.domain', body: 'test body', cc: 'someguy@test.com;otherguy@xyz.com'
message.stubs(:deliver_now)
message
end
fab!(:user) { Fabricate(:user) }
let(:email_sender) { Email::Sender.new(message, :valid_type, user) }
it "logs the cc addresses in the email log (but not users if they do not match the emails)" do
email_sender.send
email_log = EmailLog.last
expect(email_log.cc_addresses).to eq("someguy@test.com;otherguy@xyz.com")
expect(email_log.cc_users).to eq([])
end
it "logs the cc users if they match the emails" do
user1 = Fabricate(:user, email: "someguy@test.com")
user2 = Fabricate(:user, email: "otherguy@xyz.com")
email_sender.send
email_log = EmailLog.last
expect(email_log.cc_addresses).to eq("someguy@test.com;otherguy@xyz.com")
expect(email_log.cc_users).to match_array([user1, user2])
end
end
end

View File

@ -110,4 +110,32 @@ describe EmailLog do
expect(EmailLog.find(email_log.id).bounce_key).to eq(hex)
end
end
describe "cc addresses handling" do
let!(:email_log) { Fabricate(:email_log, user: user) }
describe "#cc_addresses_split" do
it "returns empty array if there are no cc addresses" do
expect(email_log.cc_addresses_split).to eq([])
end
it "returns array of cc addresses if there are any" do
email_log.update(cc_addresses: "test@test.com;test@test2.com")
expect(email_log.cc_addresses_split).to eq(["test@test.com", "test@test2.com"])
end
end
describe "#cc_users" do
it "returns empty array if there are no cc users" do
expect(email_log.cc_users).to eq([])
end
it "returns array of users if cc_user_ids is present" do
cc_user = Fabricate(:user, email: "test@test.com")
cc_user2 = Fabricate(:user, email: "test@test2.com")
email_log.update(cc_addresses: "test@test.com;test@test2.com", cc_user_ids: [cc_user.id, cc_user2.id])
expect(email_log.cc_users).to match_array([cc_user, cc_user2])
end
end
end
end