diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 85ef806c9d9..481f87704d4 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -107,13 +107,10 @@ class UserNotifications < ActionMailer::Base url: @post.url, username: username, add_unsubscribe_link: true, + allow_reply_by_email: opts[:allow_reply_by_email], template: "user_notifications.user_#{notification_type}" } - if opts[:allow_reply_by_email] && SiteSetting.reply_by_email_enabled? - email_opts[:allow_reply_by_email] = true - end - # If we have a display name, change the from address if username.present? email_opts[:from_alias] = I18n.t(:via, username: username, site_name: SiteSetting.title) diff --git a/app/models/email_log.rb b/app/models/email_log.rb index 9e6e05613c6..4c3b9de8201 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -3,13 +3,6 @@ class EmailLog < ActiveRecord::Base validates_presence_of :email_type validates_presence_of :to_address - before_create do - # We only generate a reply - if SiteSetting.reply_by_email_enabled? - self.reply_key = SecureRandom.hex(16) - end - end - 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? diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index b74499d29c4..7aed31e817e 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -35,6 +35,10 @@ module Email body end + def allow_reply_by_email? + SiteSetting.reply_by_email_enabled? && @opts[:allow_reply_by_email] + end + def template_args @template_args ||= { site_name: SiteSetting.title, base_url: Discourse.base_url, @@ -60,6 +64,9 @@ module Email if @opts[:add_unsubscribe_link] result['List-Unsubscribe'] = "<#{template_args[:user_preferences_url]}>" if @opts[:add_unsubscribe_link] end + + result['Discourse-Reply-Key'] = SecureRandom.hex(16) if allow_reply_by_email? + result end diff --git a/lib/email/sender.rb b/lib/email/sender.rb index d0ffc31a9aa..5f23dba9ce3 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -42,7 +42,13 @@ module Email to_address = @message.to to_address = to_address.first if to_address.is_a?(Array) - EmailLog.create!(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)) + + 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 end diff --git a/spec/components/email/message_builder_spec.rb b/spec/components/email/message_builder_spec.rb index 973cca81e87..a392fcb7afe 100644 --- a/spec/components/email/message_builder_spec.rb +++ b/spec/components/email/message_builder_spec.rb @@ -25,6 +25,42 @@ describe Email::MessageBuilder do expect(builder.build_args[:charset]).to eq("UTF-8") end + context "reply by email" do + + context "without allow_reply_by_email" do + it "does not have a Discourse-Reply-Key" do + expect(builder.header_args['Discourse-Reply-Key']).to be_blank + end + end + + context "with allow_reply_by_email" do + let(:reply_by_email_builder) { Email::MessageBuilder.new(to_address, allow_reply_by_email: true) } + let(:reply_key) { reply_by_email_builder.header_args['Discourse-Reply-Key'] } + + context "With the SiteSetting enabled" do + before do + SiteSetting.expects(:reply_by_email_enabled?).returns(true) + end + + it "has a Discourse-Reply-Key" do + expect(reply_key).to be_present + expect(reply_key.size).to eq(32) + end + end + + context "With the SiteSetting disabled" do + before do + SiteSetting.expects(:reply_by_email_enabled?).returns(false) + end + + it "has no Discourse-Reply-Key" do + expect(reply_key).to be_blank + end + end + end + + end + context "unsubscribe link" do context "with add_unsubscribe_link false" do diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index c2a693c26ed..f557a584428 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -22,6 +22,8 @@ describe Email::Sender do context 'with a valid message' do + let(:reply_key) { "abcd" * 8 } + let(:message) do message = Mail::Message.new to: 'eviltrout@test.domain', body: '**hello**' @@ -37,49 +39,33 @@ describe Email::Sender do end context 'email logs' do + let(:email_log) { EmailLog.last } - before do - email_sender.send - @email_log = EmailLog.last - end - - it 'creates an email log' do - @email_log.should be_present - end - - it 'has the correct type' do - @email_log.email_type.should == 'valid_type' - end - - it 'has the correct to_address' do - @email_log.to_address.should == 'eviltrout@test.domain' - end - - it 'has no user_id' do - @email_log.user_id.should be_blank - end - - + When { email_sender.send } + Then { expect(email_log).to be_present } + Then { expect(email_log.email_type).to eq('valid_type') } + Then { expect(email_log.to_address).to eq('eviltrout@test.domain') } + Then { expect(email_log.reply_key).to be_blank } + Then { expect(email_log.user_id).to be_blank } end + context "email log with a reply key" do + before do + message.header['Discourse-Reply-Key'] = reply_key + end + + let(:email_log) { EmailLog.last } + When { email_sender.send } + Then { expect(email_log.reply_key).to eq(reply_key) } + end + + context 'email parts' do - before { email_sender.send } - - it 'makes the message multipart' do - message.should be_multipart - end - - it 'sets the correct content type for the plain text part' do - expect(message.text_part.content_type).to eq 'text/plain; charset=UTF-8' - end - - it 'sets the correct content type for the html part' do - expect(message.html_part.content_type).to eq 'text/html; charset=UTF-8' - end - - it 'converts the html part to html' do - expect(message.html_part.body.to_s).to match("

hello

") - end + When { email_sender.send } + Then { expect(message).to be_multipart } + Then { expect(message.text_part.content_type).to eq('text/plain; charset=UTF-8') } + Then { expect(message.html_part.content_type).to eq('text/html; charset=UTF-8') } + Then { expect(message.html_part.body.to_s).to match("

hello

") } end end diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 31afe29879c..ea1a25943bb 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -50,27 +50,14 @@ describe UserNotifications do shared_examples "supports reply by email" do context "reply_by_email" do it "should have allow_reply_by_email set when that feature is enabled" do - SiteSetting.stubs(:reply_by_email_enabled?).returns(true) expects_build_with(has_entry(:allow_reply_by_email, true)) end - - it "should have not allow_reply_by_email set when that feature is disabled" do - SiteSetting.stubs(:reply_by_email_enabled?).returns(false) - expects_build_with(Not(has_entry(:allow_reply_by_email, true))) - end end end shared_examples "no reply by email" do context "reply_by_email" do - - it "doesn't support reply by email, even when that option is enabled" do - SiteSetting.stubs(:reply_by_email_enabled?).returns(true) - expects_build_with(Not(has_entry(:allow_reply_by_email, true))) - end - - it "should have not allow_reply_by_email set when that feature is disabled" do - SiteSetting.stubs(:reply_by_email_enabled?).returns(false) + it "doesn't support reply by email" do expects_build_with(Not(has_entry(:allow_reply_by_email, true))) end end @@ -131,7 +118,6 @@ describe UserNotifications do end end - describe "user quoted" do include_examples "notification email building" do let(:notification_type) { :quoted } diff --git a/spec/models/email_log_spec.rb b/spec/models/email_log_spec.rb index 7267f8eca01..51cf7d4077b 100644 --- a/spec/models/email_log_spec.rb +++ b/spec/models/email_log_spec.rb @@ -10,40 +10,6 @@ describe EmailLog do context 'after_create' do - context "reply_key" do - - context "with reply by email enabled" do - before do - SiteSetting.expects(:reply_by_email_enabled).returns(true) - end - - context 'generates a reply key' do - let(:reply_key) { Fabricate(:email_log).reply_key } - - it "has a reply key" do - expect(reply_key).to be_present - expect(reply_key.size).to eq(32) - end - end - end - - context "with reply by email disabled" do - before do - SiteSetting.expects(:reply_by_email_enabled).returns(false) - end - - context 'generates a reply key' do - let(:reply_key) { Fabricate(:email_log).reply_key } - - it "has no reply key" do - expect(reply_key).to be_blank - end - end - end - - end - - context 'with user' do let(:user) { Fabricate(:user) }