From fb184fed06ac13ea7ea6b0bc0e70e59825ad997f Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 20 Jan 2021 13:22:41 +1000 Subject: [PATCH] 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 --- app/controllers/admin/email_controller.rb | 2 +- app/jobs/regular/group_smtp_email.rb | 3 ++- app/jobs/regular/process_email.rb | 6 +++++- app/jobs/scheduled/poll_mailbox.rb | 2 +- app/models/incoming_email.rb | 10 ++++++++++ ...0210119005647_add_created_via_to_incoming_email.rb | 7 +++++++ lib/email/processor.rb | 2 +- lib/email/receiver.rb | 3 ++- lib/imap/sync.rb | 3 ++- spec/components/email/receiver_spec.rb | 11 +++++++++-- spec/components/imap/sync_spec.rb | 2 ++ spec/jobs/process_email_spec.rb | 2 +- spec/jobs/regular/group_smtp_email_spec.rb | 1 + 13 files changed, 44 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20210119005647_add_created_via_to_incoming_email.rb diff --git a/app/controllers/admin/email_controller.rb b/app/controllers/admin/email_controller.rb index db89c25ebcd..f4662fa724e 100644 --- a/app/controllers/admin/email_controller.rb +++ b/app/controllers/admin/email_controller.rb @@ -147,7 +147,7 @@ class Admin::EmailController < Admin::AdminController retry_count = 0 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 if retry_count == 0 params[:email] = params[:email].force_encoding('iso-8859-1').encode("UTF-8") diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb index 2e31d5d548b..ba7afa98e66 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -41,7 +41,8 @@ module Jobs message_id: message.message_id, to_addresses: message.to, cc_addresses: message.cc, - from_address: message.from + from_address: message.from, + created_via: IncomingEmail.created_via_types[:group_smtp] ) end end diff --git a/app/jobs/regular/process_email.rb b/app/jobs/regular/process_email.rb index 1a6f9d1db80..2ac10eddd15 100644 --- a/app/jobs/regular/process_email.rb +++ b/app/jobs/regular/process_email.rb @@ -6,7 +6,11 @@ module Jobs sidekiq_options retry: 3 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 sidekiq_retries_exhausted do |msg| diff --git a/app/jobs/scheduled/poll_mailbox.rb b/app/jobs/scheduled/poll_mailbox.rb index 4a9ee8bf100..47d1cd597db 100644 --- a/app/jobs/scheduled/poll_mailbox.rb +++ b/app/jobs/scheduled/poll_mailbox.rb @@ -20,7 +20,7 @@ module Jobs end def process_popmail(mail_string) - Email::Processor.process!(mail_string) + Email::Processor.process!(mail_string, source: :pop3_poll) end POLL_MAILBOX_TIMEOUT_ERROR_KEY = "poll_mailbox_timeout_error_key" diff --git a/app/models/incoming_email.rb b/app/models/incoming_email.rb index b54c5098121..cd902a6d542 100644 --- a/app/models/incoming_email.rb +++ b/app/models/incoming_email.rb @@ -29,6 +29,15 @@ class IncomingEmail < ActiveRecord::Base SQL 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 self.to_addresses&.split(";") || [] end @@ -83,6 +92,7 @@ end # imap_uid :integer # imap_sync :boolean # imap_group_id :bigint +# created_via :integer # # Indexes # diff --git a/db/migrate/20210119005647_add_created_via_to_incoming_email.rb b/db/migrate/20210119005647_add_created_via_to_incoming_email.rb new file mode 100644 index 00000000000..34ff21dae94 --- /dev/null +++ b/db/migrate/20210119005647_add_created_via_to_incoming_email.rb @@ -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 diff --git a/lib/email/processor.rb b/lib/email/processor.rb index 6fd8602a89a..d0a266eaaf4 100644 --- a/lib/email/processor.rb +++ b/lib/email/processor.rb @@ -19,7 +19,7 @@ module Email @receiver = Email::Receiver.new(@mail, @opts) @receiver.process! 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 return handle_bounce(e) if @receiver.is_bounce? diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 79441914930..2b9ecfe5140 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -152,7 +152,8 @@ module Email imap_uid_validity: @opts[:imap_uid_validity], imap_uid: @opts[:imap_uid], 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 diff --git a/lib/imap/sync.rb b/lib/imap/sync.rb index 348af0f3eac..7877edbd937 100644 --- a/lib/imap/sync.rb +++ b/lib/imap/sync.rb @@ -254,7 +254,8 @@ module Imap destinations: [@group], imap_uid_validity: @status[:uid_validity], imap_uid: email['UID'], - imap_group_id: @group.id + imap_group_id: @group.id, + source: :imap ) receiver.process! diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 62e2158fa5b..49d77aca474 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -11,8 +11,8 @@ describe Email::Receiver do SiteSetting.alternative_reply_by_email_addresses = "alt+%{reply_key}@bar.com" end - def process(email_name) - Email::Receiver.new(email(email_name)).process! + def process(email_name, opts = {}) + Email::Receiver.new(email(email_name), opts).process! end 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 ;)") 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 SiteSetting.always_show_trimmed_content = true expect { process(:gmail_html_reply) }.to change { topic.posts.count } diff --git a/spec/components/imap/sync_spec.rb b/spec/components/imap/sync_spec.rb index f6ebf44dbe6..da0ed74d250 100644 --- a/spec/components/imap/sync_spec.rb +++ b/spec/components/imap/sync_spec.rb @@ -78,6 +78,8 @@ describe Imap::Sync do .and change { Post.where(post_type: Post.types[:regular]).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_last_uid).to eq(100) diff --git a/spec/jobs/process_email_spec.rb b/spec/jobs/process_email_spec.rb index 54cd12ef703..0a98d1255a0 100644 --- a/spec/jobs/process_email_spec.rb +++ b/spec/jobs/process_email_spec.rb @@ -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?" } 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) end diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index a67f56dbf3d..a3456c510b0 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -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) 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]) end it "creates a PostReplyKey and correctly uses it for the email reply_key substitution" do