From 8acdc18bc8099dd9107bce3306adb15c20071bb3 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 13 Jun 2013 18:11:10 -0400 Subject: [PATCH] First stab at polling support for POP3S / reply by email --- app/mailers/user_notifications.rb | 2 + app/models/email_log.rb | 8 ++ app/models/site_setting.rb | 8 +- config/clock.rb | 1 + config/locales/server.en.yml | 7 ++ ...0130617181804_add_post_id_to_email_logs.rb | 6 ++ lib/email/incoming_message.rb | 19 ----- lib/email/message_builder.rb | 3 + lib/email/receiver.rb | 47 +++++++++--- lib/email/sender.rb | 21 +++++- lib/jobs/poll_mailbox.rb | 34 +++++++++ spec/components/email/incoming_email_spec.rb | 16 ---- spec/components/email/message_builder_spec.rb | 17 +++++ spec/components/email/receiver_spec.rb | 75 +++++++++++++++++-- spec/components/email/sender_spec.rb | 12 +++ spec/components/jobs/poll_mailbox_spec.rb | 24 ++++++ spec/fixtures/emails/valid_reply.txt | 40 ++++++++++ spec/mailers/user_notifications_spec.rb | 8 ++ spec/models/email_log_spec.rb | 2 - 19 files changed, 293 insertions(+), 57 deletions(-) create mode 100644 db/migrate/20130617181804_add_post_id_to_email_logs.rb delete mode 100644 lib/email/incoming_message.rb create mode 100644 lib/jobs/poll_mailbox.rb delete mode 100644 spec/components/email/incoming_email_spec.rb create mode 100644 spec/components/jobs/poll_mailbox_spec.rb create mode 100644 spec/fixtures/emails/valid_reply.txt diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 481f87704d4..45327cf0cfb 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -105,6 +105,8 @@ class UserNotifications < ActionMailer::Base topic_title: @notification.data_hash[:topic_title], message: @post.raw, url: @post.url, + post_id: @post.id, + topic_id: @post.topic_id, username: username, add_unsubscribe_link: true, allow_reply_by_email: opts[:allow_reply_by_email], diff --git a/app/models/email_log.rb b/app/models/email_log.rb index de79f27db70..9b912fdb6ed 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -3,6 +3,9 @@ class EmailLog < ActiveRecord::Base validates_presence_of :email_type validates_presence_of :to_address + belongs_to :post + belongs_to :topic + after_create do # Update last_emailed_at if the user_id is present User.update_all("last_emailed_at = CURRENT_TIMESTAMP", id: user_id) if user_id.present? @@ -11,6 +14,11 @@ class EmailLog < ActiveRecord::Base def self.count_per_day(sinceDaysAgo = 30) where('created_at > ?', sinceDaysAgo.days.ago).group('date(created_at)').order('date(created_at)').count end + + def self.for(reply_key) + EmailLog.where(reply_key: reply_key).first + end + end # == Schema Information diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb index 4e126b8b37c..feaa3ff1e2b 100644 --- a/app/models/site_setting.rb +++ b/app/models/site_setting.rb @@ -191,7 +191,13 @@ class SiteSetting < ActiveRecord::Base # Reply by Email Settings setting(:reply_by_email_enabled, false) - setting(:reply_by_email_address, nil) + setting(:reply_by_email_address, '') + + setting(:pop3s_polling_enabled, false) + setting(:pop3s_polling_host, '') + setting(:pop3s_polling_port, 995) + setting(:pop3s_polling_username, '') + setting(:pop3s_polling_password, '') # Entropy checks setting(:title_min_entropy, 10) diff --git a/config/clock.rb b/config/clock.rb index bc0c4962e75..ad0edf10016 100644 --- a/config/clock.rb +++ b/config/clock.rb @@ -16,4 +16,5 @@ module Clockwork every(10.minutes, 'periodical_updates') every(1.day, 'version_check') every(1.minute, 'clockwork_heartbeat') + every(1.minute, 'email_poll' end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index e3bd66becc6..e863c445dff 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -618,6 +618,13 @@ en: reply_by_email_enabled: "Whether this forum supports reply by email" reply_by_email_address: "Template for reply by email address in form, for example: %{reply_key}@reply.myforum.com" + pop3s_polling_enabled: "Whether to poll via POP3S for reply by email" + pop3s_polling_port: "The port to poll a POP3S account on" + pop3s_polling_host: "The host to poll for email via POP3S" + pop3s_polling_username: "The username for the POP3S account to poll for email" + pop3s_polling_password: "The password for the POP3S account to poll for email" + + notification_types: mentioned: "%{display_username} mentioned you in %{link}" liked: "%{display_username} liked your post in %{link}" diff --git a/db/migrate/20130617181804_add_post_id_to_email_logs.rb b/db/migrate/20130617181804_add_post_id_to_email_logs.rb new file mode 100644 index 00000000000..793b85dec89 --- /dev/null +++ b/db/migrate/20130617181804_add_post_id_to_email_logs.rb @@ -0,0 +1,6 @@ +class AddPostIdToEmailLogs < ActiveRecord::Migration + def change + add_column :email_logs, :post_id, :integer, null: true + add_column :email_logs, :topic_id, :integer, null: true + end +end diff --git a/lib/email/incoming_message.rb b/lib/email/incoming_message.rb deleted file mode 100644 index b3dfc965f02..00000000000 --- a/lib/email/incoming_message.rb +++ /dev/null @@ -1,19 +0,0 @@ -module Email - - class IncomingMessage - - attr_reader :reply_key, - :body_plain - - def initialize(reply_key, body) - @reply_key = reply_key - @body = body - end - - def reply - @reply ||= EmailReplyParser.read(@body).visible_text - end - - end - -end \ No newline at end of file diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index a97e55535f4..3a0bdcbf66d 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -55,6 +55,9 @@ module Email result['List-Unsubscribe'] = "<#{template_args[:user_preferences_url]}>" if @opts[:add_unsubscribe_link] end + result['Discourse-Post-Id'] = @opts[:post_id].to_s if @opts[:post_id] + result['Discourse-Topic-Id'] = @opts[:topic_id].to_s if @opts[:topic_id] + if allow_reply_by_email? result['Discourse-Reply-Key'] = reply_key result['Reply-To'] = reply_by_email_address diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index bb1c6781e25..8efad423575 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -1,29 +1,58 @@ # # Handles an incoming message # -require_dependency 'email/incoming_message' module Email class Receiver def self.results - @results ||= Enum.new(:unprocessable) + @results ||= Enum.new(:unprocessable, :missing, :processed) end - def initialize(incoming_message) - @incoming_message = incoming_message + attr_reader :body, :reply_key, :email_log + + def initialize(raw) + @raw = raw end def process + return Email::Receiver.results[:unprocessable] if @raw.blank? - if @incoming_message.blank? || @incoming_message.reply_key.blank? - return Email::Receiver.results[:unprocessable] + message = Mail::Message.new(@raw) + return Email::Receiver.results[:unprocessable] if message.body.blank? + + @body = EmailReplyParser.read(message.body.to_s).visible_text + return Email::Receiver.results[:unprocessable] if @body.blank? + + @reply_key = message.to.first + + # Extract the `reply_key` from the format the site has specified + tokens = SiteSetting.reply_by_email_address.split("%{reply_key}") + tokens.each do |t| + @reply_key.gsub!(t, "") if t.present? end - log = EmailLog.where(reply_key: @incoming_message.reply_key).first - return Email::Receiver.results[:unprocessable] if log.blank? + # Look up the email log for the reply key + @email_log = EmailLog.for(reply_key) + return Email::Receiver.results[:missing] if @email_log.blank? - nil + create_reply + + Email::Receiver.results[:processed] + end + + private + + + def create_reply + + # Try to post the body as a reply + creator = PostCreator.new(email_log.user, + raw: @body, + topic_id: @email_log.topic_id, + reply_to_post_number: @email_log.post.post_number) + + creator.create end end diff --git a/lib/email/sender.rb b/lib/email/sender.rb index 5f23dba9ce3..0c086869581 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -42,14 +42,29 @@ module Email to_address = @message.to to_address = to_address.first if to_address.is_a?(Array) - email_log = EmailLog.new(email_type: @email_type, to_address: to_address, user_id: @user.try(:id)) + email_log = EmailLog.new(email_type: @email_type, + to_address: to_address, + user_id: @user.try(:id)) + + email_log.post_id = @messager + add_header_to_log('Discourse-Reply-Key', email_log, :reply_key) + add_header_to_log('Discourse-Post-Id', email_log, :post_id) + add_header_to_log('Discourse-Topic-Id', email_log, :topic_id) - reply_key = @message.header['Discourse-Reply-Key'].to_s - email_log.reply_key = reply_key if reply_key.present? email_log.save! email_log end + private + + def add_header_to_log(name, email_log, email_log_field) + header = @message.header[name] + return unless header + + val = header.value + email_log[email_log_field] = val if val.present? + end + end end \ No newline at end of file diff --git a/lib/jobs/poll_mailbox.rb b/lib/jobs/poll_mailbox.rb new file mode 100644 index 00000000000..49f2c1fc7d2 --- /dev/null +++ b/lib/jobs/poll_mailbox.rb @@ -0,0 +1,34 @@ +# +# Connects to a mailbox and checks for replies +# +require 'net/pop' +require_dependency 'email/receiver' + +module Jobs + class PollMailbox < Jobs::Base + + sidekiq_options retry: false + + def execute(args) + if SiteSetting.pop3s_polling_enabled? + poll_pop3s + end + end + + def poll_pop3s + Net::POP3.enable_ssl(OpenSSL::SSL::VERIFY_NONE) + Net::POP3.start(SiteSetting.pop3s_polling_host, + SiteSetting.pop3s_polling_port, + SiteSetting.pop3s_polling_username, + SiteSetting.pop3s_polling_password) do |pop| + unless pop.mails.empty? + pop.each do |mail| + Email::Receiver.new(mail.pop).process + mail.delete + end + end + end + end + + end +end diff --git a/spec/components/email/incoming_email_spec.rb b/spec/components/email/incoming_email_spec.rb deleted file mode 100644 index e128813aaea..00000000000 --- a/spec/components/email/incoming_email_spec.rb +++ /dev/null @@ -1,16 +0,0 @@ -require 'spec_helper' -require 'email/receiver' - -describe Email::IncomingMessage do - - let(:message) { Email::IncomingMessage.new("asdf", "hello\n\n> how are you?") } - - it "returns the reply_key" do - expect(message.reply_key).to eq("asdf") - end - - it "extracts the reply" do - expect(message.reply).to eq("hello") - end - -end diff --git a/spec/components/email/message_builder_spec.rb b/spec/components/email/message_builder_spec.rb index dd83c393a40..f3ae0437902 100644 --- a/spec/components/email/message_builder_spec.rb +++ b/spec/components/email/message_builder_spec.rb @@ -75,6 +75,23 @@ describe Email::MessageBuilder do end + context "header args" do + + let(:message_with_header_args) { Email::MessageBuilder.new(to_address, + body: 'hello world', + topic_id: 1234, + post_id: 4567) } + + it "passes through a post_id" do + expect(message_with_header_args.header_args['Discourse-Post-Id']).to eq('4567') + end + + it "passes through a topic_id" do + expect(message_with_header_args.header_args['Discourse-Topic-Id']).to eq('1234') + end + + end + context "unsubscribe link" do context "with add_unsubscribe_link false" do diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 19525ab9236..05bac6dd68e 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -3,17 +3,78 @@ require 'email/receiver' describe Email::Receiver do + before do + SiteSetting.stubs(:reply_by_email_address).returns("reply+%{reply_key}@appmail.adventuretime.ooo") + end - describe 'invalid key' do - let(:incoming) { Email::IncomingMessage.new('asdf', 'hello') } - - it "returns unprocessable for nil message" do - expect(Email::Receiver.new(nil).process).to eq(Email::Receiver.results[:unprocessable]) + describe 'invalid emails' do + it "returns unprocessable if the message is blank" do + expect(Email::Receiver.new("").process).to eq(Email::Receiver.results[:unprocessable]) end - it "returns unprocessable for a made up key" do - expect(Email::Receiver.new(incoming).process).to eq(Email::Receiver.results[:unprocessable]) + it "returns unprocessable if the message is not an email" do + expect(Email::Receiver.new("asdf" * 30).process).to eq(Email::Receiver.results[:unprocessable]) end end + describe "with a valid email" do + let(:reply_key) { "59d8df8370b7e95c5a49fbf86aeb2c93" } + let(:valid_reply) { File.read("#{Rails.root}/spec/fixtures/emails/valid_reply.txt") } + let(:receiver) { Email::Receiver.new(valid_reply) } + let(:post) { Fabricate.build(:post) } + let(:user) { Fabricate.build(:user) } + let(:email_log) { EmailLog.new(reply_key: reply_key, post_id: 1234, topic_id: 4567, user_id: 6677, post: post, user: user ) } + let(:reply_body) { +"I could not disagree more. I am obviously biased but adventure time is the +greatest show ever created. Everyone should watch it. + +- Jake out" } + + describe "email with non-existant email log" do + + before do + EmailLog.expects(:for).returns(nil) + end + + let!(:result) { receiver.process } + + it "returns missing" do + expect(result).to eq(Email::Receiver.results[:missing]) + end + + end + + describe "with an email log" do + + before do + EmailLog.expects(:for).with(reply_key).returns(email_log) + + creator = mock + PostCreator.expects(:new).with(instance_of(User), has_entry(raw: reply_body)).returns(creator) + creator.expects(:create) + end + + let!(:result) { receiver.process } + + it "returns a processed result" do + expect(result).to eq(Email::Receiver.results[:processed]) + end + + it "extracts the body" do + expect(receiver.body).to eq(reply_body) + end + + it "looks up the email log" do + expect(receiver.email_log).to eq(email_log) + end + + it "extracts the key" do + expect(receiver.reply_key).to eq(reply_key) + end + + end + + end + + end diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index f557a584428..2b904b87e75 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -49,6 +49,18 @@ describe Email::Sender do Then { expect(email_log.user_id).to be_blank } end + context "email log with a post id and topic id" do + before do + message.header['Discourse-Post-Id'] = 3344 + message.header['Discourse-Topic-Id'] = 5577 + end + + let(:email_log) { EmailLog.last } + When { email_sender.send } + Then { expect(email_log.post_id).to eq(3344) } + Then { expect(email_log.topic_id).to eq(5577) } + end + context "email log with a reply key" do before do message.header['Discourse-Reply-Key'] = reply_key diff --git a/spec/components/jobs/poll_mailbox_spec.rb b/spec/components/jobs/poll_mailbox_spec.rb new file mode 100644 index 00000000000..79c5499c8cc --- /dev/null +++ b/spec/components/jobs/poll_mailbox_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' +require 'jobs/process_post' + +describe Jobs::PollMailbox do + + + let(:poller) { Jobs::PollMailbox.new } + + it "does no polling if pop3s_polling_enabled is false" do + SiteSetting.expects(:pop3s_polling_enabled?).returns(false) + poller.expects(:poll_pop3s).never + poller.execute({}) + end + + describe "with pop3s_polling_enabled" do + + it "calls poll_pop3s" do + SiteSetting.expects(:pop3s_polling_enabled?).returns(true) + poller.expects(:poll_pop3s).once + poller.execute({}) + end + end + +end diff --git a/spec/fixtures/emails/valid_reply.txt b/spec/fixtures/emails/valid_reply.txt new file mode 100644 index 00000000000..1e696389954 --- /dev/null +++ b/spec/fixtures/emails/valid_reply.txt @@ -0,0 +1,40 @@ +Return-Path: +Received: from iceking.adventuretime.ooo ([unix socket]) by iceking (Cyrus v2.2.13-Debian-2.2.13-19+squeeze3) with LMTPA; Thu, 13 Jun 2013 17:03:50 -0400 +Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by iceking.adventuretime.ooo (8.14.3/8.14.3/Debian-9.4) with ESMTP id r5DL3nFJ016967 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for ; Thu, 13 Jun 2013 17:03:50 -0400 +Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for ; Thu, 13 Jun 2013 14:03:48 -0700 +Received: by 10.0.0.1 with HTTP; Thu, 13 Jun 2013 14:03:48 -0700 +Date: Thu, 13 Jun 2013 17:03:48 -0400 +From: Jake the Dog +To: reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo +Message-ID: +Subject: re: [Discourse Meta] eviltrout posted in 'Adventure Time Sux' +Mime-Version: 1.0 +Content-Type: text/plain; + charset=ISO-8859-1 +Content-Transfer-Encoding: 7bit +X-Sieve: CMU Sieve 2.2 +X-Received: by 10.0.0.1 with SMTP id n7mr11234144ipb.85.1371157428600; Thu, + 13 Jun 2013 14:03:48 -0700 (PDT) +X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1 + +I could not disagree more. I am obviously biased but adventure time is the +greatest show ever created. Everyone should watch it. + +- Jake out + + +On Sun, Jun 9, 2013 at 1:39 PM, eviltrout via Discourse Meta + wrote: +> +> +> +> eviltrout posted in 'Adventure Time Sux' on Discourse Meta: +> +> --- +> hey guys everyone knows adventure time sucks! +> +> --- +> Please visit this link to respond: http://localhost:3000/t/adventure-time-sux/1234/3 +> +> To unsubscribe from these emails, visit your [user preferences](http://localhost:3000/user_preferences). +> \ No newline at end of file diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index ea1a25943bb..bc88c41d123 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -98,6 +98,14 @@ describe UserNotifications do expects_build_with(has_key(:add_unsubscribe_link)) end + it "has an post_id" do + expects_build_with(has_key(:post_id)) + end + + it "has an topic_id" do + expects_build_with(has_key(:topic_id)) + end + it "has a from alias" do expects_build_with(has_entry(:from_alias, "#{username} via #{SiteSetting.title}")) end diff --git a/spec/models/email_log_spec.rb b/spec/models/email_log_spec.rb index 51cf7d4077b..1262fb73d1a 100644 --- a/spec/models/email_log_spec.rb +++ b/spec/models/email_log_spec.rb @@ -3,11 +3,9 @@ require 'spec_helper' describe EmailLog do it { should belong_to :user } - it { should validate_presence_of :to_address } it { should validate_presence_of :email_type } - context 'after_create' do context 'with user' do