Merge pull request #6104 from tgxworld/split_up_reply_key_from_email_logs

PERF: Move `EmailLog#reply_key` into new `post_reply_keys` table.

app/controllers/admin/email_controller.rb

@@ -18,8 +18,42 @@ class Admin::EmailController < Admin::AdminController
   end
 
   def sent
-    email_logs = filter_logs(EmailLog, params)
+    email_logs = EmailLog.sent
-    render_serialized(email_logs, EmailLogSerializer)
+      .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
 

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
 

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

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

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

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

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?

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)
+          post_reply_key = PostReplyKey.find_by(reply_key: c)
-          return { type: :reply, obj: email_log } if email_log
+          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)
+        if post_reply_key.user_id != user.id && !forwarded_reply_key?(post_reply_key, user)
-          raise ReplyUserNotMatchingError, "email_log.user_id => #{email_log.user_id.inspect}, user.id => #{user.id.inspect}"
+          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,
+                     post: post_reply_key.post,
-                     topic: email_log.post.topic,
+                     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)
+        .where('posts.topic_id = ?', post_reply_key.post.topic_id)
-        .addressed_to(email_log.reply_key)
+        .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) ||
+        return true if contains_reply_by_email_address(to_addresses, post_reply_key.reply_key) ||
-          contains_reply_by_email_address(cc_addresses, email_log.reply_key)
+          contains_reply_by_email_address(cc_addresses, post_reply_key.reply_key)
       end
 
       false

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.find_by_email(to_address)&.staff?
-        return unless user && user.staff?
       end
 
       if @message.text_part
@@ -68,15 +67,20 @@ module Email
       @message.parts[0].body = @message.parts[0].body.to_s.gsub(/<img src="(\/uploads\/default\/[^"]+)"([^>]*)>/, '![](' + 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

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}\" <r+#{reply_key}@reply.myforum.com>")
+          expect(reply_by_email_builder.header_args['Reply-To'])
+            .to eq("\"#{SiteSetting.title}\" <r+%{reply_key}@reply.myforum.com>")
+
+          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\" <r+#{reply_key}@reply.myforum.com>")
+
+          expect(reply_by_email_builder.header_args['Reply-To'])
+            .to eq("\"Obnoxious Title Deal With It\" <r+%{reply_key}@reply.myforum.com>")
+
+          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_by_email_builder) do
-      let(:reply_key) { reply_by_email_builder.header_args['X-Discourse-Reply-Key'] }
+        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
+          SiteSetting.stubs(:reply_by_email_address)
-          expect(reply_key).to be_present
+            .returns("r+%{reply_key}@reply.myforum.com")
-          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("\"Username\" <r+#{reply_key}@reply.myforum.com>")
+          expect(reply_by_email_builder.header_args['Reply-To'])
+            .to eq("\"Username\" <r+%{reply_key}@reply.myforum.com>")
+
+          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

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,
+    post_reply_key = Fabricate(:post_reply_key,
-      to_address: 'reply+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa@bar.com',
-      reply_key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa',
       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

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(:topic) { post.topic }
-      let(:post) { Fabricate(:post, topic: 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

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

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
 

spec/models/email_log_spec.rb

@@ -101,20 +101,18 @@ describe EmailLog do
     end
   end
 
-  %w{reply_key bounce_key}.each do |key|
+  describe "#bounce_key" do
-    describe "##{key}" do
+    it "should format the bounce_key correctly" do
-      it "should format the #{key} correctly" do
+      hex = SecureRandom.hex
-        hex = SecureRandom.hex
+      email_log = Fabricate(:email_log, user: user, bounce_key: hex)
-        email_log = Fabricate(:email_log, user: user, "#{key}": hex)
 
-        raw_key = EmailLog.where(id: email_log.id)
+      raw_key = EmailLog.where(id: email_log.id)
-          .pluck("#{key}::text")
+        .pluck("bounce_key::text")
-          .first
+        .first
 
-        expect(raw_key).to_not eq(hex)
+      expect(raw_key).to_not eq(hex)
-        expect(raw_key.delete('-')).to eq(hex)
+      expect(raw_key.delete('-')).to eq(hex)
-        expect(EmailLog.find(email_log.id).send(key)).to eq(hex)
+      expect(EmailLog.find(email_log.id).bounce_key).to eq(hex)
-      end
     end
   end
 end

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

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