diff --git a/app/controllers/admin/email_controller.rb b/app/controllers/admin/email_controller.rb index 8fb8c6713b7..5ce8f2c170c 100644 --- a/app/controllers/admin/email_controller.rb +++ b/app/controllers/admin/email_controller.rb @@ -18,8 +18,42 @@ class Admin::EmailController < Admin::AdminController end def sent - email_logs = filter_logs(EmailLog, params) - render_serialized(email_logs, EmailLogSerializer) + email_logs = EmailLog.sent + .joins(" + LEFT JOIN post_reply_keys + ON post_reply_keys.post_id = email_logs.post_id + AND post_reply_keys.user_id = email_logs.user_id + ") + + email_logs = filter_logs(email_logs, params) + + if params[:reply_key].present? + email_logs = email_logs.where( + "post_reply_keys.reply_key ILIKE ?", "%#{params[:reply_key]}%" + ) + end + + email_logs = email_logs.to_a + + tuples = email_logs.map do |email_log| + [email_log.post_id, email_log.user_id] + end + + reply_keys = {} + + if tuples.present? + PostReplyKey + .where( + "(post_id,user_id) IN (#{(['(?)'] * tuples.size).join(', ')})", + *tuples + ) + .pluck(:post_id, :user_id, "reply_key::text") + .each do |post_id, user_id, reply_key| + reply_keys[[post_id, user_id]] = reply_key + end + end + + render_serialized(email_logs, EmailLogSerializer, reply_keys: reply_keys) end def skipped @@ -149,7 +183,6 @@ class Admin::EmailController < Admin::AdminController logs = logs.where("users.username ILIKE ?", "%#{params[:user]}%") if params[:user].present? logs = logs.where("#{table_name}.to_address ILIKE ?", "%#{params[:address]}%") if params[:address].present? logs = logs.where("#{table_name}.email_type ILIKE ?", "%#{params[:type]}%") if params[:type].present? - logs = logs.where("#{table_name}.reply_key ILIKE ?", "%#{params[:reply_key]}%") if params[:reply_key].present? logs end diff --git a/app/jobs/scheduled/clean_up_email_logs.rb b/app/jobs/scheduled/clean_up_email_logs.rb index 68505b0aaca..68650829b23 100644 --- a/app/jobs/scheduled/clean_up_email_logs.rb +++ b/app/jobs/scheduled/clean_up_email_logs.rb @@ -8,7 +8,7 @@ module Jobs threshold = SiteSetting.delete_email_logs_after_days.days.ago - EmailLog.where(reply_key: nil) + EmailLog.where("reply_key IS NULL") .where("created_at < ?", threshold) .delete_all diff --git a/app/models/email_log.rb b/app/models/email_log.rb index 911edacc6a3..c8eab2749a2 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -1,7 +1,10 @@ require_dependency 'distributed_mutex' class EmailLog < ActiveRecord::Base - self.ignored_columns = %w{topic_id} + self.ignored_columns = %w{ + topic_id + reply_key + } CRITICAL_EMAIL_TYPES ||= Set.new %w{ account_created @@ -74,10 +77,6 @@ class EmailLog < ActiveRecord::Base super&.delete('-') end - def reply_key - super&.delete('-') - end - end # == Schema Information diff --git a/app/models/post_reply_key.rb b/app/models/post_reply_key.rb new file mode 100644 index 00000000000..18d15ed0c5f --- /dev/null +++ b/app/models/post_reply_key.rb @@ -0,0 +1,18 @@ +class PostReplyKey < ActiveRecord::Base + belongs_to :post + belongs_to :user + + before_validation { self.reply_key ||= self.class.generate_reply_key } + + validates :post_id, presence: true, uniqueness: { scope: :user_id } + validates :user_id, presence: true + validates :reply_key, presence: true + + def reply_key + super&.delete('-') + end + + def self.generate_reply_key + SecureRandom.hex(16) + end +end diff --git a/app/serializers/email_log_serializer.rb b/app/serializers/email_log_serializer.rb index 8f21bb3a391..75c7010298c 100644 --- a/app/serializers/email_log_serializer.rb +++ b/app/serializers/email_log_serializer.rb @@ -5,4 +5,13 @@ class EmailLogSerializer < ApplicationSerializer :bounced has_one :user, serializer: BasicUserSerializer, embed: :objects + + def include_reply_key? + reply_keys = @options[:reply_keys] + reply_keys.present? && reply_keys[[object.post_id, object.user_id]] + end + + def reply_key + @options[:reply_keys][[object.post_id, object.user_id]].delete("-") + end end diff --git a/db/migrate/20180718062728_create_post_reply_keys.rb b/db/migrate/20180718062728_create_post_reply_keys.rb new file mode 100644 index 00000000000..22005d22f92 --- /dev/null +++ b/db/migrate/20180718062728_create_post_reply_keys.rb @@ -0,0 +1,53 @@ +require 'migration/column_dropper' + +class CreatePostReplyKeys < ActiveRecord::Migration[5.2] + def up + create_table :post_reply_keys do |t| + t.integer :user_id, null: false + t.integer :post_id, null: false + t.uuid :reply_key, null: false + t.timestamps null: false + end + + add_index :post_reply_keys, :reply_key, unique: true + + Migration::ColumnDropper.mark_readonly(:email_logs, :reply_key) + + sql = <<~SQL + DELETE FROM email_logs + WHERE id IN ( + SELECT id + FROM ( + SELECT + id, + ROW_NUMBER() OVER(PARTITION BY post_id, user_id ORDER BY id DESC) AS row_num + FROM email_logs + ) t + WHERE t.row_num > 1 + ) + SQL + + execute(sql) + + sql = <<~SQL + INSERT INTO post_reply_keys( + user_id, post_id, reply_key, updated_at, created_at + ) SELECT + user_id, + post_id, + reply_key, + updated_at, + created_at + FROM email_logs + WHERE reply_key IS NOT NULL + SQL + + execute(sql) + + add_index :post_reply_keys, [:user_id, :post_id], unique: true + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index 0eb345b45f0..0d39cd5eff6 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -17,6 +17,8 @@ module Email class MessageBuilder attr_reader :template_args + ALLOW_REPLY_BY_EMAIL_HEADER = 'X-Discourse-Allow-Reply-By-Email'.freeze + def initialize(to, opts = nil) @to = to @opts = opts || {} @@ -147,7 +149,7 @@ module Email result['X-Auto-Response-Suppress'] = 'All' if allow_reply_by_email? - result['X-Discourse-Reply-Key'] = reply_key + result[ALLOW_REPLY_BY_EMAIL_HEADER] = true result['Reply-To'] = reply_by_email_address else result['Reply-To'] = from_value @@ -171,10 +173,6 @@ module Email protected - def reply_key - @reply_key ||= SecureRandom.hex(16) - end - def allow_reply_by_email? SiteSetting.reply_by_email_enabled? && reply_by_email_address.present? && @@ -196,7 +194,6 @@ module Email return nil unless SiteSetting.reply_by_email_address.present? @reply_by_email_address = SiteSetting.reply_by_email_address.dup - @reply_by_email_address.gsub!("%{reply_key}", reply_key) @reply_by_email_address = if private_reply? diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index ae8da1a36f6..f8ab257d2f9 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -548,8 +548,8 @@ module Email if match && match.captures match.captures.each do |c| next if c.blank? - email_log = EmailLog.for(c) - return { type: :reply, obj: email_log } if email_log + post_reply_key = PostReplyKey.find_by(reply_key: c) + return { type: :reply, obj: post_reply_key } if post_reply_key end end nil @@ -580,18 +580,18 @@ module Email skip_validations: user.staged?) when :reply - email_log = destination[:obj] + post_reply_key = destination[:obj] - if email_log.user_id != user.id && !forwarded_reply_key?(email_log, user) - raise ReplyUserNotMatchingError, "email_log.user_id => #{email_log.user_id.inspect}, user.id => #{user.id.inspect}" + if post_reply_key.user_id != user.id && !forwarded_reply_key?(post_reply_key, user) + raise ReplyUserNotMatchingError, "post_reply_key.user_id => #{post_reply_key.user_id.inspect}, user.id => #{user.id.inspect}" end create_reply(user: user, raw: body, elided: elided, hidden_reason_id: hidden_reason_id, - post: email_log.post, - topic: email_log.post.topic, + post: post_reply_key.post, + topic: post_reply_key.post.topic, skip_validations: user.staged?) end end @@ -631,11 +631,11 @@ module Email end end - def forwarded_reply_key?(email_log, user) + def forwarded_reply_key?(post_reply_key, user) incoming_emails = IncomingEmail .joins(:post) - .where('posts.topic_id = ?', email_log.topic.id) - .addressed_to(email_log.reply_key) + .where('posts.topic_id = ?', post_reply_key.post.topic_id) + .addressed_to(post_reply_key.reply_key) .addressed_to_user(user) .pluck(:to_addresses, :cc_addresses) @@ -643,8 +643,8 @@ module Email next unless contains_email_address_of_user?(to_addresses, user) || contains_email_address_of_user?(cc_addresses, user) - return true if contains_reply_by_email_address(to_addresses, email_log.reply_key) || - contains_reply_by_email_address(cc_addresses, email_log.reply_key) + return true if contains_reply_by_email_address(to_addresses, post_reply_key.reply_key) || + contains_reply_by_email_address(cc_addresses, post_reply_key.reply_key) end false diff --git a/lib/email/sender.rb b/lib/email/sender.rb index c503c750a0c..56f427bcaf4 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -31,8 +31,7 @@ module Email return skip(SkippedEmailLog.reason_types[:sender_message_to_blank]) if @message.to.blank? if SiteSetting.disable_emails == "non-staff" - user = User.find_by_email(to_address) - return unless user && user.staff? + return unless User.find_by_email(to_address)&.staff? end if @message.text_part @@ -68,15 +67,20 @@ module Email @message.parts[0].body = @message.parts[0].body.to_s.gsub(/]*)>/, '![](' + url_prefix + '\1)') @message.text_part.content_type = 'text/plain; charset=UTF-8' + user_id = @user&.id # Set up the email log - 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_id + ) host = Email::Sender.host_for(Discourse.base_url) post_id = header_value('X-Discourse-Post-Id') topic_id = header_value('X-Discourse-Topic-Id') - reply_key = header_value('X-Discourse-Reply-Key') + reply_key = set_reply_key(post_id, user_id) # always set a default Message ID from the host @message.header['Message-ID'] = "<#{SecureRandom.uuid}@#{host}>" @@ -160,12 +164,14 @@ module Email end email_log.post_id = post_id if post_id.present? - email_log.reply_key = reply_key if reply_key.present? # Remove headers we don't need anymore @message.header['X-Discourse-Topic-Id'] = nil if topic_id.present? @message.header['X-Discourse-Post-Id'] = nil if post_id.present? - @message.header['X-Discourse-Reply-Key'] = nil if reply_key.present? + + if reply_key.present? + @message.header[Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER] = nil + end # pass the original message_id when using mailjet/mandrill/sparkpost case ActionMailer::Base.smtp_settings[:address] @@ -251,5 +257,19 @@ module Email @message.header[name] = data.to_json end + def set_reply_key(post_id, user_id) + return unless user_id && + post_id && + header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present? + + reply_key = PostReplyKey.find_or_create_by!( + post_id: post_id, + user_id: user_id + ).reply_key + + @message.header['Reply-To'] = + header_value('Reply-To').gsub!("%{reply_key}", reply_key) + end + end end diff --git a/spec/components/email/message_builder_spec.rb b/spec/components/email/message_builder_spec.rb index 9283f87262e..95cb5f4f971 100644 --- a/spec/components/email/message_builder_spec.rb +++ b/spec/components/email/message_builder_spec.rb @@ -9,6 +9,7 @@ describe Email::MessageBuilder do let(:builder) { Email::MessageBuilder.new(to_address, subject: subject, body: body) } let(:build_args) { builder.build_args } let(:header_args) { builder.header_args } + let(:allow_reply_header) { described_class::ALLOW_REPLY_BY_EMAIL_HEADER } it "has the correct to address" do expect(build_args[:to]).to eq(to_address) @@ -44,7 +45,6 @@ describe Email::MessageBuilder do 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['X-Discourse-Reply-Key'] } context "With the SiteSetting enabled" do before do @@ -52,18 +52,22 @@ describe Email::MessageBuilder do SiteSetting.stubs(:reply_by_email_address).returns("r+%{reply_key}@reply.myforum.com") end - it "has a X-Discourse-Reply-Key" do - expect(reply_key).to be_present - expect(reply_key.size).to eq(32) - end - it "returns a Reply-To header with the reply key" do - expect(reply_by_email_builder.header_args['Reply-To']).to eq("\"#{SiteSetting.title}\" ") + expect(reply_by_email_builder.header_args['Reply-To']) + .to eq("\"#{SiteSetting.title}\" ") + + expect(reply_by_email_builder.header_args[allow_reply_header]) + .to eq(true) end it "cleans up the site title" do SiteSetting.stubs(:title).returns(">>>Obnoxious Title: Deal, \"With\" It<<<") - expect(reply_by_email_builder.header_args['Reply-To']).to eq("\"Obnoxious Title Deal With It\" ") + + expect(reply_by_email_builder.header_args['Reply-To']) + .to eq("\"Obnoxious Title Deal With It\" ") + + expect(reply_by_email_builder.header_args[allow_reply_header]) + .to eq(true) end end @@ -72,33 +76,39 @@ describe Email::MessageBuilder do SiteSetting.stubs(:reply_by_email_enabled?).returns(false) end - it "has no X-Discourse-Reply-Key" do - expect(reply_key).to be_blank - end - it "returns a Reply-To header that's the same as From" do - expect(header_args['Reply-To']).to eq(build_args[:from]) + expect(reply_by_email_builder.header_args['Reply-To']) + .to eq(reply_by_email_builder.build_args[:from]) + + expect(reply_by_email_builder.header_args[allow_reply_header]) + .to eq(nil) end end end context "with allow_reply_by_email" do - let(:reply_by_email_builder) { Email::MessageBuilder.new(to_address, allow_reply_by_email: true, private_reply: true, from_alias: "Username") } - let(:reply_key) { reply_by_email_builder.header_args['X-Discourse-Reply-Key'] } + let(:reply_by_email_builder) do + Email::MessageBuilder.new(to_address, + allow_reply_by_email: true, + private_reply: true, + from_alias: "Username" + ) + end context "With the SiteSetting enabled" do before do SiteSetting.stubs(:reply_by_email_enabled?).returns(true) - SiteSetting.stubs(:reply_by_email_address).returns("r+%{reply_key}@reply.myforum.com") - end - it "has a X-Discourse-Reply-Key" do - expect(reply_key).to be_present - expect(reply_key.size).to eq(32) + SiteSetting.stubs(:reply_by_email_address) + .returns("r+%{reply_key}@reply.myforum.com") end it "returns a Reply-To header with the reply key" do - expect(reply_by_email_builder.header_args['Reply-To']).to eq("\"Username\" ") + expect(reply_by_email_builder.header_args['Reply-To']) + .to eq("\"Username\" ") + + expect(reply_by_email_builder.header_args[allow_reply_header]) + .to eq(true) end end @@ -107,12 +117,12 @@ describe Email::MessageBuilder do SiteSetting.stubs(:reply_by_email_enabled?).returns(false) end - it "has no X-Discourse-Reply-Key" do - expect(reply_key).to be_blank - end - it "returns a Reply-To header that's the same as From" do - expect(header_args['Reply-To']).to eq(build_args[:from]) + expect(reply_by_email_builder.header_args['Reply-To']) + .to eq(reply_by_email_builder.build_args[:from]) + + expect(reply_by_email_builder.header_args[allow_reply_header]) + .to eq(nil) end end end diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 1c08e59cb43..60f1fa40e2a 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -63,12 +63,12 @@ describe Email::Receiver do it "doesn't raise an InactiveUserError when the sender is staged" do user = Fabricate(:user, email: "staged@bar.com", active: false, staged: true) + post = Fabricate(:post) - email_log = Fabricate(:email_log, - to_address: 'reply+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa@bar.com', - reply_key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', + post_reply_key = Fabricate(:post_reply_key, user: user, - post: Fabricate(:post) + post: post, + reply_key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' ) expect { process(:staged_sender) }.not_to raise_error @@ -153,7 +153,14 @@ describe Email::Receiver do let(:user) { Fabricate(:user, email: "discourse@bar.com") } let(:topic) { create_topic(category: category, user: user) } let(:post) { create_post(topic: topic, user: user) } - let!(:email_log) { Fabricate(:email_log, reply_key: reply_key, user: user, topic: topic, post: post) } + + let!(:post_reply_key) do + Fabricate(:post_reply_key, + reply_key: reply_key, + user: user, + post: post + ) + end it "uses MD5 of 'mail_string' there is no message_id" do mail_string = email(:missing_message_id) @@ -814,12 +821,15 @@ describe Email::Receiver do context "with a valid reply" do it "returns the destination when the key is valid" do - Fabricate(:email_log, reply_key: '4f97315cc828096c9cb34c6f1a0d6fe8') + post_reply_key = Fabricate(:post_reply_key, + reply_key: '4f97315cc828096c9cb34c6f1a0d6fe8' + ) dest = Email::Receiver.check_address('foo+4f97315cc828096c9cb34c6f1a0d6fe8@bar.com') + expect(dest).to be_present expect(dest[:type]).to eq(:reply) - expect(dest[:obj]).to be_present + expect(dest[:obj]).to eq(post_reply_key) end end end @@ -925,7 +935,10 @@ describe Email::Receiver do let(:user) { Fabricate(:user, email: "discourse@bar.com") } let(:topic) { create_topic(category: category, user: user) } let(:post) { create_post(topic: topic, user: user) } - let!(:email_log) { Fabricate(:email_log, reply_key: reply_key, user: user, topic: topic, post: post) } + + let!(:post_reply_key) do + Fabricate(:post_reply_key, reply_key: reply_key, user: user, post: post) + end context "when the email address isn't matching the one we sent the notification to" do include_examples "no staged users", :reply_user_not_matching, Email::Receiver::ReplyUserNotMatchingError diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index 5fad20d13b2..086afb07134 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -2,6 +2,7 @@ require 'rails_helper' require 'email/sender' describe Email::Sender do + let(:post) { Fabricate(:post) } context "disable_emails is enabled" do let(:user) { Fabricate(:user) } @@ -292,19 +293,19 @@ describe Email::Sender do let(:email_log) { EmailLog.last } it 'should create the right log' do - email_sender.send + expect do + email_sender.send + end.to_not change { PostReplyKey.count } expect(email_log).to be_present expect(email_log.email_type).to eq('valid_type') expect(email_log.to_address).to eq('eviltrout@test.domain') - expect(email_log.reply_key).to be_blank expect(email_log.user_id).to be_blank end end context "email log with a post id and topic id" do - let(:topic) { Fabricate(:topic) } - let(:post) { Fabricate(:post, topic: topic) } + let(:topic) { post.topic } before do message.header['X-Discourse-Post-Id'] = post.id @@ -320,19 +321,6 @@ describe Email::Sender do end end - context "email log with a reply key" do - before do - message.header['X-Discourse-Reply-Key'] = reply_key - end - - let(:email_log) { EmailLog.last } - - it 'should create the right log' do - email_sender.send - expect(email_log.reply_key).to eq(reply_key) - end - end - context 'email parts' do it 'should contain the right message' do email_sender.send @@ -364,6 +352,42 @@ describe Email::Sender do expect(@email_log.user_id).to eq(user.id) end + describe "post reply keys" do + let(:post) { Fabricate(:post) } + + before do + message.header['X-Discourse-Post-Id'] = post.id + message.header['Reply-To'] = "test-%{reply_key}@test.com" + end + + describe 'when allow reply by email header is not present' do + it 'should not create a post reply key' do + expect { email_sender.send }.to_not change { PostReplyKey.count } + end + end + + describe 'when allow reply by email header is present' do + let(:header) { Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER } + + before do + message.header[header] = "test-%{reply_key}@test.com" + end + + it 'should create a post reply key' do + expect { email_sender.send }.to change { PostReplyKey.count }.by(1) + post_reply_key = PostReplyKey.last + + expect(message.header['Reply-To'].value).to eq( + "test-#{post_reply_key.reply_key}@test.com" + ) + + expect(message.header[header]).to eq(nil) + expect(post_reply_key.user_id).to eq(user.id) + expect(post_reply_key.post_id).to eq(post.id) + expect { email_sender.send }.to change { PostReplyKey.count }.by(0) + end + end + end end end diff --git a/spec/fabricators/post_reply_key_fabricator.rb b/spec/fabricators/post_reply_key_fabricator.rb new file mode 100644 index 00000000000..99fef8275ac --- /dev/null +++ b/spec/fabricators/post_reply_key_fabricator.rb @@ -0,0 +1,5 @@ +Fabricator(:post_reply_key) do + user + post + reply_key { PostReplyKey.generate_reply_key } +end diff --git a/spec/jobs/clean_up_email_logs_spec.rb b/spec/jobs/clean_up_email_logs_spec.rb index 177a7414cde..59305c492f9 100644 --- a/spec/jobs/clean_up_email_logs_spec.rb +++ b/spec/jobs/clean_up_email_logs_spec.rb @@ -3,7 +3,6 @@ require 'rails_helper' describe Jobs::CleanUpEmailLogs do before do - Fabricate(:email_log, created_at: 2.years.ago, reply_key: SecureRandom.hex) Fabricate(:email_log, created_at: 2.years.ago) Fabricate(:email_log, created_at: 2.weeks.ago) Fabricate(:email_log, created_at: 2.days.ago) @@ -14,14 +13,14 @@ describe Jobs::CleanUpEmailLogs do it "removes old email logs without a reply_key" do Jobs::CleanUpEmailLogs.new.execute({}) - expect(EmailLog.count).to eq(3) + expect(EmailLog.count).to eq(2) expect(SkippedEmailLog.count).to eq(1) end it "does not remove old email logs when delete_email_logs_after_days is 0" do SiteSetting.delete_email_logs_after_days = 0 Jobs::CleanUpEmailLogs.new.execute({}) - expect(EmailLog.count).to eq(4) + expect(EmailLog.count).to eq(3) expect(SkippedEmailLog.count).to eq(2) end diff --git a/spec/models/email_log_spec.rb b/spec/models/email_log_spec.rb index 8608cc01a19..5a749dedbdc 100644 --- a/spec/models/email_log_spec.rb +++ b/spec/models/email_log_spec.rb @@ -101,20 +101,18 @@ describe EmailLog do end end - %w{reply_key bounce_key}.each do |key| - describe "##{key}" do - it "should format the #{key} correctly" do - hex = SecureRandom.hex - email_log = Fabricate(:email_log, user: user, "#{key}": hex) + describe "#bounce_key" do + it "should format the bounce_key correctly" do + hex = SecureRandom.hex + email_log = Fabricate(:email_log, user: user, bounce_key: hex) - raw_key = EmailLog.where(id: email_log.id) - .pluck("#{key}::text") - .first + raw_key = EmailLog.where(id: email_log.id) + .pluck("bounce_key::text") + .first - expect(raw_key).to_not eq(hex) - expect(raw_key.delete('-')).to eq(hex) - expect(EmailLog.find(email_log.id).send(key)).to eq(hex) - end + expect(raw_key).to_not eq(hex) + expect(raw_key.delete('-')).to eq(hex) + expect(EmailLog.find(email_log.id).bounce_key).to eq(hex) end end end diff --git a/spec/models/post_reply_key_spec.rb b/spec/models/post_reply_key_spec.rb new file mode 100644 index 00000000000..631e6cd6a2e --- /dev/null +++ b/spec/models/post_reply_key_spec.rb @@ -0,0 +1,20 @@ +require 'rails_helper' + +RSpec.describe PostReplyKey do + describe "#reply_key" do + it "should format the reply_key correctly" do + hex = SecureRandom.hex + post_reply_key = Fabricate(:post_reply_key, + reply_key: hex + ) + + raw_key = PostReplyKey.where(id: post_reply_key.id) + .pluck("reply_key::text") + .first + + expect(raw_key).to_not eq(hex) + expect(raw_key.delete('-')).to eq(hex) + expect(PostReplyKey.find(post_reply_key.id).reply_key).to eq(hex) + end + end +end diff --git a/spec/requests/admin/email_controller_spec.rb b/spec/requests/admin/email_controller_spec.rb index 681dcdcc5da..e59f3cc125a 100644 --- a/spec/requests/admin/email_controller_spec.rb +++ b/spec/requests/admin/email_controller_spec.rb @@ -2,6 +2,7 @@ require 'rails_helper' describe Admin::EmailController do let(:admin) { Fabricate(:admin) } + let(:email_log) { Fabricate(:email_log) } before do sign_in(admin) @@ -32,9 +33,30 @@ describe Admin::EmailController do end describe '#sent' do - it "succeeds" do + let(:post) { Fabricate(:post) } + let(:email_log) { Fabricate(:email_log, post: post) } + + let(:post_reply_key) do + Fabricate(:post_reply_key, post: post, user: email_log.user) + end + + it "should return the right response" do + email_log get "/admin/email/sent.json" + expect(response.status).to eq(200) + log = JSON.parse(response.body).first + expect(log["id"]).to eq(email_log.id) + expect(log["reply_key"]).to eq(nil) + + post_reply_key + + get "/admin/email/sent.json" + + expect(response.status).to eq(200) + log = JSON.parse(response.body).first + expect(log["id"]).to eq(email_log.id) + expect(log["reply_key"]).to eq(post_reply_key.reply_key) end end