Migrate email sent receipts to signatures

Doing a join between two tables with tens of millions of rows means that
iterating over the signatures that need emailing can take considerable
time for a large petition (e.g. 16 hours for 4.1m signatures). If we move
the timestamp columns to the signatures table there will be an initial
slow query but each successive batch of 1000 records for find_each will
be much quicker (approx. 3ms).

Before deploying we need to ensure that all existing bulk email jobs have
completed. After deployment we then need to run the migration task to
copy the timestamps from email_sent_receipts table - the table itself
will be deleted in a later deploy.

app/models/signature.rb

@@ -16,6 +16,13 @@ class Signature < ActiveRecord::Base
     VALIDATED_STATE, INVALIDATED_STATE
   ]
 
+  TIMESTAMPS = {
+    'government_response' => :government_response_email_at,
+    'debate_scheduled'    => :debate_scheduled_email_at,
+    'debate_outcome'      => :debate_outcome_email_at,
+    'petition_email'      => :petition_email_at
+  }
+
   # = Relationships =
   belongs_to :petition
   belongs_to :invalidation
@@ -49,27 +56,14 @@ class Signature < ActiveRecord::Base
   scope :for_email, ->(email) { where(email: email.downcase) }
   scope :for_name, ->(name) { where("lower(name) = ?", name.downcase) }
 
-  def self.need_emailing_for(name, since:)
-    receipts_table = EmailSentReceipt.arel_table
-    validated.
-      notify_by_email.
-      joins(arel_join_onto_sent_receipts).
-      where(
-        receipts_table['id'].eq(nil).or(
-          receipts_table[name].eq(nil).or(
-            receipts_table[name].lt(since)
-          )
-        )
-      )
+  def self.for_timestamp(timestamp, since:)
+    column = arel_table[column_name_for(timestamp)]
+    where(column.eq(nil).or(column.lt(since)))
   end
 
-  def self.arel_join_onto_sent_receipts
-    receipts = EmailSentReceipt.arel_table
-    sigs = self.arel_table
-    join_on = sigs.create_on(sigs[:id].eq(receipts[:signature_id]))
-    sigs.create_join(receipts, join_on, Arel::Nodes::OuterJoin)
+  def self.need_emailing_for(timestamp, since:)
+    validated.notify_by_email.for_timestamp(timestamp, since: since)
   end
-  private_class_method :arel_join_onto_sent_receipts
 
   def self.petition_ids_with_invalid_signature_counts
     validated.joins(:petition).
@@ -78,6 +72,12 @@ def self.petition_ids_with_invalid_signature_counts
       pluck(:petition_id)
   end
 
+  def self.column_name_for(timestamp)
+    TIMESTAMPS.fetch(timestamp)
+  rescue
+    raise ArgumentError, "Unknown petition email timestamp: #{timestamp.inspect}"
+  end
+
   scope :in_days, ->(number_of_days) { validated.where("updated_at > ?", number_of_days.day.ago) }
   scope :matching, ->(signature) { where(email: signature.email,
                                          name: signature.name,
@@ -212,16 +212,12 @@ def store_constituency_id
     save if constituency_id_changed?
   end
 
-  def get_email_sent_at_for(name)
-    email_sent_receipt!.get(name)
-  end
-  def set_email_sent_at_for(name, to: Time.current)
-    email_sent_receipt!.set(name, to)
+  def get_email_sent_at_for(timestamp)
+    self[column_name_for(timestamp)]
   end
 
-  has_one :email_sent_receipt, dependent: :destroy
-  def email_sent_receipt!
-    email_sent_receipt || create_email_sent_receipt
+  def set_email_sent_at_for(timestamp, to: Time.current)
+    update_column(column_name_for(timestamp), to)
   end
 
   def domain
@@ -237,6 +233,10 @@ def rate(window = 5.minutes)
 
   private
 
+  def column_name_for(timestamp)
+    self.class.column_name_for(timestamp)
+  end
+
   def retry_lock
     retried = false
 

db/migrate/20160716164929_add_email_timestamps_to_signatures.rb

@@ -0,0 +1,8 @@
+class AddEmailTimestampsToSignatures < ActiveRecord::Migration
+  def change
+    add_column :signatures, :government_response_email_at, :datetime
+    add_column :signatures, :debate_scheduled_email_at, :datetime
+    add_column :signatures, :debate_outcome_email_at, :datetime
+    add_column :signatures, :petition_email_at, :datetime
+  end
+end

db/structure.sql

@@ -727,7 +727,11 @@ CREATE TABLE signatures (
     seen_signed_confirmation_page boolean DEFAULT false NOT NULL,
     location_code character varying(30),
     invalidated_at timestamp without time zone,
-    invalidation_id integer
+    invalidation_id integer,
+    government_response_email_at timestamp without time zone,
+    debate_scheduled_email_at timestamp without time zone,
+    debate_outcome_email_at timestamp without time zone,
+    petition_email_at timestamp without time zone
 );
 
 
@@ -1817,3 +1821,5 @@ INSERT INTO schema_migrations (version) VALUES ('20160713130452');
 
 INSERT INTO schema_migrations (version) VALUES ('20160715092819');
 
+INSERT INTO schema_migrations (version) VALUES ('20160716164929');
+

lib/tasks/migrations.rake

@@ -254,5 +254,27 @@ namespace :epets do
         petition.touch
       end
     end
+
+    desc "Migrate email sent receipts to signature columns"
+    task :email_sent_receipts_to_signatures => :environment do
+      class EmailSentReceipt < ActiveRecord::Base; end
+
+      EmailSentReceipt.find_each do |receipt|
+        updates = []
+        updates << "government_response_email_at = GREATEST(government_response_email_at, ?)"
+        updates << "debate_scheduled_email_at = GREATEST(debate_scheduled_email_at, ?)"
+        updates << "debate_outcome_email_at = GREATEST(debate_outcome_email_at, ?)"
+        updates << "petition_email_at = GREATEST(petition_email_at, ?)"
+        updates = updates.join(", ")
+
+        params = []
+        params << receipt.government_response
+        params << receipt.debate_scheduled
+        params << receipt.debate_outcome
+        params << receipt.petition_email
+
+        Signature.where(id: receipt.signature_id).update_all([updates] + params)
+      end
+    end
   end
 end

spec/controllers/admin/debate_outcomes_controller_spec.rb

@@ -198,7 +198,7 @@ def do_patch(overrides = {})
                 end
               end
 
-              it "stamps the 'debate_outcome' email sent receipt on each signature when the job runs" do
+              it "stamps the 'debate_outcome' email sent timestamp on each signature when the job runs" do
                 perform_enqueued_jobs do
                   do_patch
                   petition.reload

spec/controllers/admin/government_response_controller_spec.rb

@@ -191,7 +191,7 @@ def do_patch(overrides = {})
               end
             end
 
-            it "stamps the 'government_response' email sent receipt on each signature when the job runs" do
+            it "stamps the 'government_response' email sent timestamp on each signature when the job runs" do
               perform_enqueued_jobs do
                 do_patch
                 petition.reload

spec/controllers/admin/petition_emails_controller_spec.rb

@@ -213,7 +213,7 @@ def do_post(overrides = {})
                 end
               end
 
-              it "stamps the 'petition_email' email sent receipt on each signature when the job runs" do
+              it "stamps the 'petition_email' email sent timestamp on each signature when the job runs" do
                 perform_enqueued_jobs do
                   do_post
                   petition.reload
@@ -587,7 +587,7 @@ def do_patch(overrides = {})
                 end
               end
 
-              it "stamps the 'petition_email' email sent receipt on each signature when the job runs" do
+              it "stamps the 'petition_email' email sent timestamp on each signature when the job runs" do
                 perform_enqueued_jobs do
                   do_patch
                   petition.reload

spec/controllers/admin/schedule_debate_controller_spec.rb

@@ -169,7 +169,7 @@ def do_patch(overrides = {})
                 end
               end
 
-              it "stamps the 'debate_scheduled' email sent receipt on each signature when the job runs" do
+              it "stamps the 'debate_scheduled' email sent timestamp on each signature when the job runs" do
                 perform_enqueued_jobs do
                   do_patch
                   petition.reload

spec/factories.rb

@@ -356,10 +356,6 @@
     association :petition, factory: :open_petition
   end
 
-  factory :email_sent_receipt do
-    association :signature, factory: :validated_signature
-  end
-
   factory :location do
     code { Faker::Address.country_code }
     name { Faker::Address.country }

spec/jobs/shared_examples.rb

@@ -101,7 +101,11 @@ def do_work
     end
 
     it "records the email being sent" do
-      expect { job.perform_now }.to change(EmailSentReceipt, :count).by(1)
+      expect {
+        job.perform_now
+      }.to change {
+        signature.reload.get_email_sent_at_for(timestamp_name)
+      }.from(nil).to(requested_at)
     end
 
     context "an email has already been sent for the petition to this signatory" do
@@ -114,7 +118,11 @@ def do_work
       end
 
       it "does not record any email being sent" do
-        expect { job.perform_now }.not_to change(signature.email_sent_receipt.reload, :updated_at)
+        expect {
+          job.perform_now
+        }.not_to change {
+          signature.reload.get_email_sent_at_for(timestamp_name)
+        }
       end
     end
 
@@ -233,7 +241,11 @@ def do_work
     end
 
     it "does not record any email being sent" do
-      expect { job.perform_now }.not_to change(EmailSentReceipt, :count)
+      expect {
+        job.perform_now
+      }.not_to change {
+        signature.reload.get_email_sent_at_for(timestamp_name)
+      }
     end
   end
 end

spec/models/petition_spec.rb

@@ -1776,24 +1776,22 @@
         expect(petition.signatures_to_email_for('government_response')).not_to include other_signature
       end
 
-      it 'does not return signatures that have a sent receipt newer than the petitions requested receipt' do
+      it 'does not return signatures that have a sent timestamp newer than the petitions requested receipt' do
         other_signature.set_email_sent_at_for('government_response', to: petition_timestamp + 1.day)
         expect(petition.signatures_to_email_for('government_response')).not_to include other_signature
       end
 
-      it 'does not return signatures that have a sent receipt equal to the petitions requested receipt' do
+      it 'does not return signatures that have a sent timestamp equal to the petitions requested receipt' do
         other_signature.set_email_sent_at_for('government_response', to: petition_timestamp)
         expect(petition.signatures_to_email_for('government_response')).not_to include other_signature
       end
 
-      it 'does return signatures that have a sent receipt older than the petitions requested receipt' do
+      it 'does return signatures that have a sent timestamp older than the petitions requested receipt' do
         other_signature.set_email_sent_at_for('government_response', to: petition_timestamp - 1.day)
         expect(petition.signatures_to_email_for('government_response')).to include other_signature
       end
 
-      it 'returns signatures that have no sent receipt, or null for the requested timestamp in their receipt' do
-        other_signature.email_sent_receipt!.destroy && other_signature.reload
-        creator_signature.email_sent_receipt!.update_column('government_response', nil)
+      it 'returns signatures that have no sent timestamp, or null for the requested timestamp in their receipt' do
         expect(petition.signatures_to_email_for('government_response')).to match_array [creator_signature, other_signature]
       end
     end