DEV: Add created_via column to IncomingEmail (#11751)

This should make it easier to track down how the incoming email was created, which is one of four locations:

The POP3 poller (which picks up reply via email replies)
The admin email controller #handle_mail (which is where hosted mail is sent)
The IMAP sync tool
The group SMTP mailer, which sends emails when replying to IMAP topics, pre-emptively creating IncomingEmail records to avoid double syncing
This commit is contained in:
Martin Brennan 2021-01-20 13:22:41 +10:00 committed by GitHub
parent 2e39939fe9
commit fb184fed06
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 44 additions and 10 deletions

View File

@ -147,7 +147,7 @@ class Admin::EmailController < Admin::AdminController
retry_count = 0 retry_count = 0
begin begin
Jobs.enqueue(:process_email, mail: params[:email], retry_on_rate_limit: true) Jobs.enqueue(:process_email, mail: params[:email], retry_on_rate_limit: true, source: :handle_mail)
rescue JSON::GeneratorError => e rescue JSON::GeneratorError => e
if retry_count == 0 if retry_count == 0
params[:email] = params[:email].force_encoding('iso-8859-1').encode("UTF-8") params[:email] = params[:email].force_encoding('iso-8859-1').encode("UTF-8")

View File

@ -41,7 +41,8 @@ module Jobs
message_id: message.message_id, message_id: message.message_id,
to_addresses: message.to, to_addresses: message.to,
cc_addresses: message.cc, cc_addresses: message.cc,
from_address: message.from from_address: message.from,
created_via: IncomingEmail.created_via_types[:group_smtp]
) )
end end
end end

View File

@ -6,7 +6,11 @@ module Jobs
sidekiq_options retry: 3 sidekiq_options retry: 3
def execute(args) def execute(args)
Email::Processor.process!(args[:mail], retry_on_rate_limit: args[:retry_on_rate_limit] || false) Email::Processor.process!(
args[:mail],
retry_on_rate_limit: args[:retry_on_rate_limit] || false,
source: args[:source]&.to_sym
)
end end
sidekiq_retries_exhausted do |msg| sidekiq_retries_exhausted do |msg|

View File

@ -20,7 +20,7 @@ module Jobs
end end
def process_popmail(mail_string) def process_popmail(mail_string)
Email::Processor.process!(mail_string) Email::Processor.process!(mail_string, source: :pop3_poll)
end end
POLL_MAILBOX_TIMEOUT_ERROR_KEY = "poll_mailbox_timeout_error_key" POLL_MAILBOX_TIMEOUT_ERROR_KEY = "poll_mailbox_timeout_error_key"

View File

@ -29,6 +29,15 @@ class IncomingEmail < ActiveRecord::Base
SQL SQL
end end
def self.created_via_types
@types ||= Enum.new(
handle_mail: 1,
pop3_poll: 2,
imap: 3,
group_smtp: 4
)
end
def to_addresses_split def to_addresses_split
self.to_addresses&.split(";") || [] self.to_addresses&.split(";") || []
end end
@ -83,6 +92,7 @@ end
# imap_uid :integer # imap_uid :integer
# imap_sync :boolean # imap_sync :boolean
# imap_group_id :bigint # imap_group_id :bigint
# created_via :integer
# #
# Indexes # Indexes
# #

View File

@ -0,0 +1,7 @@
# frozen_string_literal: true
class AddCreatedViaToIncomingEmail < ActiveRecord::Migration[6.0]
def change
add_column :incoming_emails, :created_via, :integer, null: true
end
end

View File

@ -19,7 +19,7 @@ module Email
@receiver = Email::Receiver.new(@mail, @opts) @receiver = Email::Receiver.new(@mail, @opts)
@receiver.process! @receiver.process!
rescue RateLimiter::LimitExceeded rescue RateLimiter::LimitExceeded
@opts[:retry_on_rate_limit] ? Jobs.enqueue(:process_email, mail: @mail) : raise @opts[:retry_on_rate_limit] ? Jobs.enqueue(:process_email, mail: @mail, source: @opts[:source]) : raise
rescue => e rescue => e
return handle_bounce(e) if @receiver.is_bounce? return handle_bounce(e) if @receiver.is_bounce?

View File

@ -152,7 +152,8 @@ module Email
imap_uid_validity: @opts[:imap_uid_validity], imap_uid_validity: @opts[:imap_uid_validity],
imap_uid: @opts[:imap_uid], imap_uid: @opts[:imap_uid],
imap_group_id: @opts[:imap_group_id], imap_group_id: @opts[:imap_group_id],
imap_sync: false imap_sync: false,
created_via: @opts[:source].present? ? IncomingEmail.created_via_types[@opts[:source]] : nil
) )
end end

View File

@ -254,7 +254,8 @@ module Imap
destinations: [@group], destinations: [@group],
imap_uid_validity: @status[:uid_validity], imap_uid_validity: @status[:uid_validity],
imap_uid: email['UID'], imap_uid: email['UID'],
imap_group_id: @group.id imap_group_id: @group.id,
source: :imap
) )
receiver.process! receiver.process!

View File

@ -11,8 +11,8 @@ describe Email::Receiver do
SiteSetting.alternative_reply_by_email_addresses = "alt+%{reply_key}@bar.com" SiteSetting.alternative_reply_by_email_addresses = "alt+%{reply_key}@bar.com"
end end
def process(email_name) def process(email_name, opts = {})
Email::Receiver.new(email(email_name)).process! Email::Receiver.new(email(email_name), opts).process!
end end
it "raises an EmptyEmailError when 'mail_string' is blank" do it "raises an EmptyEmailError when 'mail_string' is blank" do
@ -350,6 +350,13 @@ describe Email::Receiver do
expect(topic.posts.last.raw).to eq("This is a **HTML** reply ;)") expect(topic.posts.last.raw).to eq("This is a **HTML** reply ;)")
end end
it "stores the created_via source against the incoming email" do
process(:text_reply, source: :handle_mail)
expect(IncomingEmail.last.created_via).to eq(IncomingEmail.created_via_types[:handle_mail])
process(:text_and_html_reply, source: :imap)
expect(IncomingEmail.last.created_via).to eq(IncomingEmail.created_via_types[:imap])
end
it "automatically elides gmail quotes" do it "automatically elides gmail quotes" do
SiteSetting.always_show_trimmed_content = true SiteSetting.always_show_trimmed_content = true
expect { process(:gmail_html_reply) }.to change { topic.posts.count } expect { process(:gmail_html_reply) }.to change { topic.posts.count }

View File

@ -78,6 +78,8 @@ describe Imap::Sync do
.and change { Post.where(post_type: Post.types[:regular]).count }.by(1) .and change { Post.where(post_type: Post.types[:regular]).count }.by(1)
.and change { IncomingEmail.count }.by(1) .and change { IncomingEmail.count }.by(1)
expect(IncomingEmail.last.created_via).to eq(IncomingEmail.created_via_types[:imap])
expect(group.imap_uid_validity).to eq(1) expect(group.imap_uid_validity).to eq(1)
expect(group.imap_last_uid).to eq(100) expect(group.imap_last_uid).to eq(100)

View File

@ -7,7 +7,7 @@ describe Jobs::ProcessEmail do
let(:mail) { "From: foo@bar.com\nTo: bar@foo.com\nSubject: FOO BAR\n\nFoo foo bar bar?" } let(:mail) { "From: foo@bar.com\nTo: bar@foo.com\nSubject: FOO BAR\n\nFoo foo bar bar?" }
it "process an email without retry" do it "process an email without retry" do
Email::Processor.expects(:process!).with(mail, retry_on_rate_limit: false) Email::Processor.expects(:process!).with(mail, retry_on_rate_limit: false, source: nil)
Jobs::ProcessEmail.new.execute(mail: mail) Jobs::ProcessEmail.new.execute(mail: mail)
end end

View File

@ -38,6 +38,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id) 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).not_to eq(nil)
expect(incoming.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") 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])
end end
it "creates a PostReplyKey and correctly uses it for the email reply_key substitution" do it "creates a PostReplyKey and correctly uses it for the email reply_key substitution" do