From 7c2b3cc128b1e8edafc113b4c53d91cd0d8307c0 Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 7 Aug 2015 15:06:36 +0100
Subject: [PATCH 1/9] Refactor batch email jobs to allow multiple arguments

To support custom email messages we need to pass the email object into
run_later_tonight so it needs to be able to take multiple args and then
pass them down to the perform method.

Also tidied up the specs to eliminate a Rails monkey patch.
---
 .../admin/debate_outcomes_controller.rb       |   2 +-
 .../admin/government_response_controller.rb   |   2 +-
 app/controllers/admin/petitions_controller.rb |   2 +-
 .../admin/schedule_debate_controller.rb       |   2 +-
 .../email_all_petition_signatories.rb         |  97 ++++----
 app/jobs/concerns/email_delivery.rb           |  74 +++---
 app/jobs/deliver_debate_outcome_email_job.rb  |   1 -
 .../deliver_debate_scheduled_email_job.rb     |   1 -
 .../deliver_threshold_response_email_job.rb   |   1 -
 app/jobs/email_debate_outcomes_job.rb         |   9 +-
 app/jobs/email_debate_scheduled_job.rb        |   9 +-
 app/jobs/email_threshold_response_job.rb      |   9 +-
 .../deliver_debate_outcome_email_job_spec.rb  |   2 +-
 ...deliver_debate_scheduled_email_job_spec.rb |   2 +-
 ...liver_threshold_response_email_job_spec.rb |   2 +-
 spec/jobs/shared_examples.rb                  | 219 +++++++++++-------
 16 files changed, 239 insertions(+), 195 deletions(-)

diff --git a/app/controllers/admin/debate_outcomes_controller.rb b/app/controllers/admin/debate_outcomes_controller.rb
index 665358b8e..60ed7edc7 100644
--- a/app/controllers/admin/debate_outcomes_controller.rb
+++ b/app/controllers/admin/debate_outcomes_controller.rb
@@ -9,7 +9,7 @@ def show
 
   def update
     if @debate_outcome.update(debate_outcome_params)
-      EmailDebateOutcomesJob.run_later_tonight(@petition)
+      EmailDebateOutcomesJob.run_later_tonight(petition: @petition)
       redirect_to [:admin, @petition], notice: 'Email will be sent overnight'
     else
       render 'admin/petitions/show'
diff --git a/app/controllers/admin/government_response_controller.rb b/app/controllers/admin/government_response_controller.rb
index 191f4272c..2db43016b 100644
--- a/app/controllers/admin/government_response_controller.rb
+++ b/app/controllers/admin/government_response_controller.rb
@@ -9,7 +9,7 @@ def show
 
   def update
     if @government_response.update_attributes(government_response_params)
-      EmailThresholdResponseJob.run_later_tonight(@petition)
+      EmailThresholdResponseJob.run_later_tonight(petition: @petition)
       redirect_to [:admin, @petition], notice: 'Email will be sent overnight'
     else
       render 'admin/petitions/show'
diff --git a/app/controllers/admin/petitions_controller.rb b/app/controllers/admin/petitions_controller.rb
index fab31b7ad..2ed3f74ce 100644
--- a/app/controllers/admin/petitions_controller.rb
+++ b/app/controllers/admin/petitions_controller.rb
@@ -20,7 +20,7 @@ def edit_scheduled_debate_date
   def update_scheduled_debate_date
     fetch_petition_for_scheduled_debate_date
     if @petition.update(update_scheduled_debate_date_params)
-      EmailDebateScheduledJob.run_later_tonight(@petition)
+      EmailDebateScheduledJob.run_later_tonight(petition: @petition)
       redirect_to admin_petition_url(@petition), notice: "Email will be sent overnight"
     else
       render :edit_scheduled_debate_date
diff --git a/app/controllers/admin/schedule_debate_controller.rb b/app/controllers/admin/schedule_debate_controller.rb
index e1dfa2894..6a731b33d 100644
--- a/app/controllers/admin/schedule_debate_controller.rb
+++ b/app/controllers/admin/schedule_debate_controller.rb
@@ -8,7 +8,7 @@ def show
 
   def update
     if @petition.update_attributes(params_for_update)
-      EmailDebateScheduledJob.run_later_tonight(@petition)
+      EmailDebateScheduledJob.run_later_tonight(petition: @petition)
       redirect_to [:admin, @petition], notice: "Email will be sent overnight"
     else
       render 'admin/petitions/show'
diff --git a/app/jobs/concerns/email_all_petition_signatories.rb b/app/jobs/concerns/email_all_petition_signatories.rb
index 0c59b360e..a66ddd98f 100644
--- a/app/jobs/concerns/email_all_petition_signatories.rb
+++ b/app/jobs/concerns/email_all_petition_signatories.rb
@@ -1,75 +1,83 @@
 module EmailAllPetitionSignatories
-  extend ActiveSupport::Concern
+  # Concern to add shared functionality to ActiveJob classes
+  # that are responsible for enqueuing send email jobs
 
-  #
-  # Concern to add shared functionality to ActiveJob classes that are responsible
-  # for enqueuing send email jobs
-  #
+  extend ActiveSupport::Concern
 
   included do
-    queue_as :default
+    class_attribute :email_delivery_job_class
+    class_attribute :timestamp_name
 
-    def self.run_later_tonight(petition)
-      requested_at = Time.current
+    attr_reader :petition, :requested_at
 
-      petition.set_email_requested_at_for(new.timestamp_name, to: requested_at)
+    queue_as :default
+  end
+
+  module ClassMethods
+    def run_later_tonight(**args)
+      petition, @requested_at = args[:petition], nil
 
-      set(wait_until: later_tonight).
-        perform_later(petition, requested_at.getutc.iso8601(6))
+      petition.set_email_requested_at_for(timestamp_name, to: requested_at)
+      set(wait_until: later_tonight).perform_later(**args.merge(requested_at: requested_at_iso8601))
     end
 
-    def self.later_tonight
-      1.day.from_now.at_midnight + rand(240).minutes + rand(60).seconds
+    private
+
+    def requested_at
+      @requested_at ||= Time.current
     end
-    private_class_method :later_tonight
 
-  end
+    def requested_at_iso8601
+      requested_at.getutc.iso8601(6)
+    end
 
+    def later_tonight
+      midnight + random_interval
+    end
 
+    def midnight
+      requested_at.end_of_day
+    end
 
-  def perform(petition, requested_at_string)
-    @petition = petition
-    @requested_at = requested_at_string.in_time_zone
-    do_work!
+    def random_interval
+      rand(240).minutes + rand(60).seconds
+    end
   end
 
-  def timestamp_name
-    raise NotImplementedError.new "Including classes must implement #timestamp_name method"
+  def perform(**args)
+    @petition, @requested_at = args[:petition], args[:requested_at]
+
+    # If the petition has been updated since the job
+    # was queued then don't send the emails.
+    unless petition_has_been_updated?
+      enqueue_send_email_jobs
+    end
   end
 
   private
 
-  attr_reader :petition, :requested_at
-
-  def do_work!
-    return if petition_has_been_updated?
-
-    logger.info("Starting #{self.class.name} for petition '#{petition.action}' with email requested at: #{petition_timestamp}")
-    enqueue_send_email_jobs
-    logger.info("Finished #{self.class.name} for petition '#{petition.action}'")
-
-  end
-
-  #
   # Batches the signataries to send emails to in groups of 1000
   # and enqueues a job to do the actual sending
-  #
   def enqueue_send_email_jobs
     signatures_to_email.find_each do |signature|
-      email_delivery_job_class.perform_later(
-        signature:                    signature,
-        timestamp_name:               timestamp_name,
-        petition:                     petition,
-        requested_at_as_string:       requested_at.getutc.iso8601(6)
-      )
+      email_delivery_job_class.perform_later(**mailer_arguments(signature))
     end
   end
 
+  def mailer_arguments(signature)
+    {
+      signature:      signature,
+      timestamp_name: timestamp_name,
+      petition:       petition,
+      requested_at:   requested_at
+    }
+  end
+
   # admins can ask to send the email multiple times and each time they
   # ask we enqueues a new job to send out emails with a new timestamp
   # we want to execute only the latest job enqueued
   def petition_has_been_updated?
-    (petition_timestamp - requested_at).abs > 1
+    (petition_timestamp - requested_at.in_time_zone).abs > 1
   end
 
   def petition_timestamp
@@ -79,11 +87,4 @@ def petition_timestamp
   def signatures_to_email
     petition.signatures_to_email_for(timestamp_name)
   end
-
-  # The job class that handles the actual email sending for this job type
-  def email_delivery_job_class
-    raise NotImplementedError.new "Including classes must implement #email_delivery_job_class method"
-  end
 end
-
-
diff --git a/app/jobs/concerns/email_delivery.rb b/app/jobs/concerns/email_delivery.rb
index 67e758a01..016459383 100644
--- a/app/jobs/concerns/email_delivery.rb
+++ b/app/jobs/concerns/email_delivery.rb
@@ -1,25 +1,44 @@
 module EmailDelivery
-  extend ActiveSupport::Concern
-
-  #
   # Send a single email to a recipient informing them about a petition that they have signed
   # Implemented as a custom job rather than using action mailers #deliver_later so we can do
   # extra checking before sending the email
-  #
+
+  extend ActiveSupport::Concern
+
+  PERMANENT_FAILURES = [
+    Net::SMTPFatalError,
+    Net::SMTPSyntaxError
+  ]
+
+  TEMPORARY_FAILURES = [
+    Net::SMTPAuthenticationError,
+    Net::OpenTimeout,
+    Net::SMTPServerBusy,
+    Errno::ECONNRESET,
+    Errno::ECONNREFUSED,
+    Errno::ETIMEDOUT,
+    Timeout::Error
+  ]
 
   included do
+    attr_reader :signature, :timestamp_name, :petition, :requested_at
     queue_as :default
-  end
 
-  def perform(signature:, timestamp_name:, petition:,
-    requested_at_as_string:, mailer: PetitionMailer.name, logger: nil)
+    rescue_from *PERMANENT_FAILURES do |exception|
+      log_exception(exception)
+    end
+
+    rescue_from *TEMPORARY_FAILURES do |exception|
+      log_exception(exception)
+      retry_job
+    end
+  end
 
-    @mailer = mailer.constantize
-    @signature = signature
-    @petition = petition
-    @requested_at = requested_at_as_string.in_time_zone
-    @timestamp_name = timestamp_name
-    @logger = logger
+  def perform(**args)
+    @signature = args[:signature]
+    @petition = args[:petition]
+    @requested_at = args[:requested_at].in_time_zone
+    @timestamp_name = args[:timestamp_name]
 
     if can_send_email?
       send_email
@@ -29,7 +48,13 @@ def perform(signature:, timestamp_name:, petition:,
 
   private
 
-  attr_reader :mailer, :signature, :timestamp_name, :petition, :requested_at
+  def log_exception(exception)
+    logger.info(log_message(exception))
+  end
+
+  def log_message(exception)
+    "#{exception.class.name} while sending email for #{self.class.name} to: #{signature.email} for #{petition.action}"
+  end
 
   def can_send_email?
     petition_has_not_been_updated? && email_not_previously_sent?
@@ -37,21 +62,10 @@ def can_send_email?
 
   def send_email
     create_email.deliver_now
+  end
 
-    rescue Errno::ECONNREFUSED, Errno::ETIMEDOUT, Net::SMTPError, Timeout::Error => e
-      # log that the send failed
-      logger.info("#{e.class.name} while sending email for #{self.class.name} to: #{signature.email} for #{signature.petition.action}")
-
-      #
-      # TODO: check the error and if it is a n AWS SES rate error:
-      # 454 Throttling failure: Maximum sending rate exceeded
-      # 454 Throttling failure: Daily message quota exceeded
-      #
-      # Then reschedule the send for a day later rather than keep failing
-      #
-
-      # reraise to rerun the job later via the job retry mechanism
-      raise e
+  def mailer
+    PetitionMailer
   end
 
   def create_email
@@ -69,13 +83,11 @@ def petition_timestamp
   # We do not want to send the email if the petition has been updated
   # As email sending is enqueued straight after a petition has been updated
   def petition_has_not_been_updated?
-    (petition_timestamp - requested_at).abs < 1
+    (petition_timestamp - requested_at.in_time_zone).abs < 1
   end
 
-  #
   # Have we already sent an email for this petition version?
   # If we have then the timestamp for the signature will match the timestamp for the petition
-  #
   def email_not_previously_sent?
     # check that the signature is still in the list of signatures
     petition.signatures_to_email_for(timestamp_name).where(id: signature.id).exists?
diff --git a/app/jobs/deliver_debate_outcome_email_job.rb b/app/jobs/deliver_debate_outcome_email_job.rb
index 22e36f647..9ac9e6680 100644
--- a/app/jobs/deliver_debate_outcome_email_job.rb
+++ b/app/jobs/deliver_debate_outcome_email_job.rb
@@ -4,5 +4,4 @@ class DeliverDebateOutcomeEmailJob < ActiveJob::Base
   def create_email
     mailer.notify_signer_of_debate_outcome signature.petition, signature
   end
-
 end
diff --git a/app/jobs/deliver_debate_scheduled_email_job.rb b/app/jobs/deliver_debate_scheduled_email_job.rb
index 8519cde56..43da91e34 100644
--- a/app/jobs/deliver_debate_scheduled_email_job.rb
+++ b/app/jobs/deliver_debate_scheduled_email_job.rb
@@ -4,5 +4,4 @@ class DeliverDebateScheduledEmailJob < ActiveJob::Base
   def create_email
     mailer.notify_signer_of_debate_scheduled signature.petition, signature
   end
-
 end
diff --git a/app/jobs/deliver_threshold_response_email_job.rb b/app/jobs/deliver_threshold_response_email_job.rb
index 2784b6532..d1d511d06 100644
--- a/app/jobs/deliver_threshold_response_email_job.rb
+++ b/app/jobs/deliver_threshold_response_email_job.rb
@@ -4,5 +4,4 @@ class DeliverThresholdResponseEmailJob < ActiveJob::Base
   def create_email
     mailer.notify_signer_of_threshold_response signature.petition, signature
   end
-
 end
diff --git a/app/jobs/email_debate_outcomes_job.rb b/app/jobs/email_debate_outcomes_job.rb
index 116e44064..a00f9c723 100644
--- a/app/jobs/email_debate_outcomes_job.rb
+++ b/app/jobs/email_debate_outcomes_job.rb
@@ -1,11 +1,6 @@
 class EmailDebateOutcomesJob < ActiveJob::Base
   include EmailAllPetitionSignatories
 
-  def email_delivery_job_class
-    DeliverDebateOutcomeEmailJob
-  end
-
-  def timestamp_name
-    'debate_outcome'
-  end
+  self.email_delivery_job_class = DeliverDebateOutcomeEmailJob
+  self.timestamp_name = 'debate_outcome'
 end
diff --git a/app/jobs/email_debate_scheduled_job.rb b/app/jobs/email_debate_scheduled_job.rb
index 54f9134e7..7413ade40 100644
--- a/app/jobs/email_debate_scheduled_job.rb
+++ b/app/jobs/email_debate_scheduled_job.rb
@@ -1,11 +1,6 @@
 class EmailDebateScheduledJob < ActiveJob::Base
   include EmailAllPetitionSignatories
 
-  def email_delivery_job_class
-    DeliverDebateScheduledEmailJob
-  end
-
-  def timestamp_name
-    'debate_scheduled'
-  end
+  self.email_delivery_job_class = DeliverDebateScheduledEmailJob
+  self.timestamp_name = 'debate_scheduled'
 end
diff --git a/app/jobs/email_threshold_response_job.rb b/app/jobs/email_threshold_response_job.rb
index 5518af88a..a2b781989 100644
--- a/app/jobs/email_threshold_response_job.rb
+++ b/app/jobs/email_threshold_response_job.rb
@@ -1,11 +1,6 @@
 class EmailThresholdResponseJob < ActiveJob::Base
   include EmailAllPetitionSignatories
 
-  def email_delivery_job_class
-    DeliverThresholdResponseEmailJob
-  end
-
-  def timestamp_name
-    'government_response'
-  end
+  self.email_delivery_job_class = DeliverThresholdResponseEmailJob
+  self.timestamp_name = 'government_response'
 end
diff --git a/spec/jobs/deliver_debate_outcome_email_job_spec.rb b/spec/jobs/deliver_debate_outcome_email_job_spec.rb
index f3a61a413..95e22da3f 100644
--- a/spec/jobs/deliver_debate_outcome_email_job_spec.rb
+++ b/spec/jobs/deliver_debate_outcome_email_job_spec.rb
@@ -19,7 +19,7 @@
       signature: signature,
       timestamp_name: timestamp_name,
       petition: petition,
-      requested_at_as_string: email_requested_at.getutc.iso8601
+      requested_at: email_requested_at.getutc.iso8601(6)
     )
   end
 end
diff --git a/spec/jobs/deliver_debate_scheduled_email_job_spec.rb b/spec/jobs/deliver_debate_scheduled_email_job_spec.rb
index 2d08cb8c0..a0867bbcb 100644
--- a/spec/jobs/deliver_debate_scheduled_email_job_spec.rb
+++ b/spec/jobs/deliver_debate_scheduled_email_job_spec.rb
@@ -19,7 +19,7 @@
       signature: signature,
       timestamp_name: timestamp_name,
       petition: petition,
-      requested_at_as_string: email_requested_at.getutc.iso8601
+      requested_at: email_requested_at.getutc.iso8601(6)
     )
   end
 end
diff --git a/spec/jobs/deliver_threshold_response_email_job_spec.rb b/spec/jobs/deliver_threshold_response_email_job_spec.rb
index ce47b2027..c83f727cc 100644
--- a/spec/jobs/deliver_threshold_response_email_job_spec.rb
+++ b/spec/jobs/deliver_threshold_response_email_job_spec.rb
@@ -19,7 +19,7 @@
       signature: signature,
       timestamp_name: timestamp_name,
       petition: petition,
-      requested_at_as_string: email_requested_at.getutc.iso8601
+      requested_at: email_requested_at.getutc.iso8601
     )
   end
 end
diff --git a/spec/jobs/shared_examples.rb b/spec/jobs/shared_examples.rb
index e8ca56aac..138d371e4 100644
--- a/spec/jobs/shared_examples.rb
+++ b/spec/jobs/shared_examples.rb
@@ -1,68 +1,50 @@
 RSpec.shared_examples_for "job to enqueue signatory mailing jobs" do
   include ActiveSupport::Testing::TimeHelpers
 
-  def do_work(requested_at = email_requested_at)
-    @requested_at = requested_at.getutc.iso8601(6)
-    subject.perform(petition, requested_at)
-  end
-
-  #
-  # Annoyingly Rails TimeHelper sets usec to 0 for all stubbed times
-  # But we need usec accuracy so the #travel_to method is copied here with the usec kept
-  #
-  # See https://github.com/rails/rails/blob/master/activesupport/lib/active_support/testing/time_helpers.rb#L95-L113
-  #
-  def travel_to(date_or_time)
-    if date_or_time.is_a?(Date) && !date_or_time.is_a?(DateTime)
-      now = date_or_time.midnight.to_time
-    else
-      now = date_or_time.to_time
-    end
+  let(:requested_at) { Time.current.change(usec: 0) }
+  let(:requested_at_as_string) { requested_at.getutc.iso8601(6) }
 
-    simple_stubs.stub_object(Time, :now, now)
-    simple_stubs.stub_object(Date, :today, now.to_date)
-
-    if block_given?
-      begin
-        yield
-      ensure
-        travel_back
-      end
-    end
+  def do_work
+    subject.perform(petition: petition, requested_at: requested_at_as_string)
   end
 
   describe '.run_later_tonight' do
     let(:petition) { FactoryGirl.create(:open_petition) }
-    let(:requested_at) { Time.current }
+    let(:petition_gid) { { "_aj_globalid" => petition.to_global_id.to_s } }
 
-    def global_id_job_arg_for(object)
-      { "_aj_globalid" => object.to_global_id.to_s }
+    let(:queue_size) { enqueued_jobs.size }
+    let(:enqueued_job) { enqueued_jobs.first }
+    let(:job_class) { enqueued_job[:job] }
+    let(:queued_at) { enqueued_job[:at] }
+    let(:job_arguments) { enqueued_job[:args][0] }
+
+    let(:window_duration) { 240.minutes + 60.seconds }
+    let(:window_start) { requested_at.end_of_day.to_f }
+    let(:window_end) { window_start + window_duration.to_f }
+    let(:execution_window) { window_start..window_end }
+
+    around do |example|
+      travel_to requested_at do
+        example.run
+      end
     end
 
-    def timestamp_job_arg_for(timestamp)
-      timestamp.getutc.iso8601(6)
+    before do
+      described_class.run_later_tonight(petition: petition)
     end
 
     it 'queues up a job' do
-      described_class.run_later_tonight(petition)
-      expect(enqueued_jobs.size).to eq 1
-      expect(enqueued_jobs.first[:job]).to eq described_class
+      expect(queue_size).to eq 1
+      expect(job_class).to eq described_class
     end
 
     it 'sets the job to run between midnight and 4am tomorrow' do
-      described_class.run_later_tonight(petition)
-      queued_at = enqueued_jobs.first[:at]
-      expect(queued_at).to satisfy { |at| at >= (1.day.from_now.midnight.to_i) }
-      expect(queued_at).to satisfy { |at| at <= (1.day.from_now.midnight + 4.hours).to_i }
+      expect(queued_at).to be_in(execution_window)
     end
 
     it 'queues up the job to run with the petition and timestamp supplied as args' do
-      travel_to requested_at do
-        described_class.run_later_tonight(petition)
-        queued_args = enqueued_jobs.first[:args]
-        expect(queued_args[0]).to eq global_id_job_arg_for(petition)
-        expect(queued_args[1]).to eq timestamp_job_arg_for(requested_at)
-      end
+      expect(job_arguments['petition']).to eq petition_gid
+      expect(job_arguments['requested_at']).to eq requested_at_as_string
     end
   end
 
@@ -83,13 +65,13 @@ def timestamp_job_arg_for(timestamp)
       args = enqueued_jobs.first[:args].first
 
       expect(args["timestamp_name"]).to eq(subject.timestamp_name)
-      expect(args["requested_at_as_string"]).to eq(@requested_at)
+      expect(args["requested_at"]).to eq(requested_at_as_string)
     end
   end
 
   context "when the petition has been updated" do
     before do
-      petition.set_email_requested_at_for(subject.timestamp_name, to: Time.current + 5.minutes )
+      petition.set_email_requested_at_for(subject.timestamp_name, to: requested_at + 5.minutes )
     end
 
     it "does not enqueue any jobs to send emails" do
@@ -100,38 +82,42 @@ def timestamp_job_arg_for(timestamp)
 end
 
 RSpec.shared_examples_for "a job to send an signatory email" do
-  def perform_job
-    @requested_at_as_string = email_requested_at.getutc.iso8601(6)
-    subject.perform(
+  let(:requested_at) { Time.current.change(usec: 0) }
+  let(:requested_at_as_string) { requested_at.getutc.iso8601(6) }
+
+  let :arguments do
+    {
       signature: signature,
       timestamp_name: timestamp_name,
       petition: petition,
-      requested_at_as_string: @requested_at_as_string,
-      logger: logger
-    )
+      requested_at: requested_at_as_string
+    }
   end
 
-  let(:logger) { double("Logger").as_null_object }
+  let :job do
+    described_class.new(arguments)
+  end
 
   context "when the petition has not been updated" do
+    let(:mail_object) { double(:mail_object) }
+
     it "uses the correct mailer to generate the email" do
-      expect(subject).to receive(:create_email).and_call_original
-      perform_job
+      expect(job).to receive(:create_email).and_call_original
+      job.perform_now
     end
 
-    it "delivers the rendered email supplied by #create_email" do
-      rendered_email = double("Rendered email")
-      expect(rendered_email).to receive(:deliver_now)
-      allow(subject).to receive(:create_email).and_return rendered_email
-      perform_job
+    it "delivers the email returned by #create_email" do
+      expect(job).to receive(:create_email).and_return(mail_object)
+      expect(mail_object).to receive(:deliver_now)
+      job.perform_now
     end
 
     it "does sends the email" do
-      expect { perform_job }.to change(ActionMailer::Base.deliveries, :size).by(1)
+      expect { job.perform_now }.to change(ActionMailer::Base.deliveries, :size).by(1)
     end
 
     it "records the email being sent" do
-      expect { perform_job }.to change(EmailSentReceipt, :count).by(1)
+      expect { job.perform_now }.to change(EmailSentReceipt, :count).by(1)
     end
 
     context "an email has already been sent for the petition to this signatory" do
@@ -140,52 +126,115 @@ def perform_job
       end
 
       it "does not send any email" do
-        expect { perform_job }.not_to change(ActionMailer::Base.deliveries, :size)
+        expect { job.perform_now }.not_to change(ActionMailer::Base.deliveries, :size)
       end
 
       it "does not record any email being sent" do
-        expect { perform_job }.not_to change(signature.email_sent_receipt.reload, :updated_at)
+        expect { job.perform_now }.not_to change(signature.email_sent_receipt.reload, :updated_at)
       end
     end
 
     context "email sending fails" do
-      shared_examples_for 'catching errors during individual email sending' do
-        before do
-          allow(subject).to receive(:create_email).and_raise(exception_class)
-        end
+      shared_examples_for "catching errors during individual email sending" do
+        let(:logger) { job.logger }
 
-        it "captures the error and reraises it" do
-          expect { perform_job }.to raise_error(exception_class)
+        it "captures the error and doesn't re-raise it" do
+          job.perform_now
         end
 
-        it 'logs the email sending error as information' do
+        it "logs the email sending error as information" do
           expect(logger).to receive(:info).with(/#{Regexp.escape(exception_class.name)}/)
-          suppress(exception_class) { perform_job }
+          job.perform_now
         end
 
         it "does not mark the signature" do
-          suppress(exception_class) { perform_job }
-          signature.reload
-          expect(signature.get_email_sent_at_for(timestamp_name)).not_to be_usec_precise_with email_requested_at
+          expect {
+            job.perform_now
+          }.not_to change {
+            signature.reload.get_email_sent_at_for(timestamp_name)
+          }
         end
       end
 
-      context "with connection refused" do
-        let(:exception_class) { Errno::ECONNREFUSED }
+      shared_examples_for "retrying the email delivery" do
+        it "retries the job" do
+          expect(job).to receive(:retry_job)
+          job.perform_now
+        end
+      end
+
+      shared_examples_for "not retrying the email delivery" do
+        it "doesn't retry the job" do
+          expect(job).not_to receive(:retry_job)
+          job.perform_now
+        end
+      end
 
-        it_behaves_like 'catching errors during individual email sending'
+      before do
+        expect(job).to receive(:create_email).and_raise(exception_class)
       end
 
-      context "with an SMTPError" do
+      context "with a fatal SMTP error" do
         let(:exception_class) { Net::SMTPFatalError }
 
-        it_behaves_like 'catching errors during individual email sending'
+        it_behaves_like "catching errors during individual email sending"
+        it_behaves_like "not retrying the email delivery"
+      end
+
+      context "with a SMTP syntax error" do
+        let(:exception_class) { Net::SMTPSyntaxError }
+
+        it_behaves_like "catching errors during individual email sending"
+        it_behaves_like "not retrying the email delivery"
+      end
+
+      context "with SMTP authentication error" do
+        let(:exception_class) { Net::SMTPAuthenticationError }
+
+        it_behaves_like "catching errors during individual email sending"
+        it_behaves_like "retrying the email delivery"
+      end
+
+      context "with SMTP connection timeout" do
+        let(:exception_class) { Net::OpenTimeout }
+
+        it_behaves_like "catching errors during individual email sending"
+        it_behaves_like "retrying the email delivery"
+      end
+
+      context "with SMTP server busy" do
+        let(:exception_class) { Net::SMTPServerBusy }
+
+        it_behaves_like "catching errors during individual email sending"
+        it_behaves_like "retrying the email delivery"
+      end
+
+      context "with connection reset" do
+        let(:exception_class) { Errno::ECONNRESET }
+
+        it_behaves_like "catching errors during individual email sending"
+        it_behaves_like "retrying the email delivery"
+      end
+
+      context "with connection refused" do
+        let(:exception_class) { Errno::ECONNREFUSED }
+
+        it_behaves_like "catching errors during individual email sending"
+        it_behaves_like "retrying the email delivery"
       end
 
-      context 'with a timeout' do
+      context "with connection timeout" do
         let(:exception_class) { Errno::ETIMEDOUT }
 
-        it_behaves_like 'catching errors during individual email sending'
+        it_behaves_like "catching errors during individual email sending"
+        it_behaves_like "retrying the email delivery"
+      end
+
+      context "with timeout error" do
+        let(:exception_class) { Timeout::Error }
+
+        it_behaves_like "catching errors during individual email sending"
+        it_behaves_like "retrying the email delivery"
       end
     end
   end
@@ -196,11 +245,11 @@ def perform_job
     end
 
     it "does not send any email" do
-      expect { perform_job }.not_to change(ActionMailer::Base.deliveries, :size)
+      expect { job.perform_now }.not_to change(ActionMailer::Base.deliveries, :size)
     end
 
     it "does not record any email being sent" do
-      expect { perform_job }.not_to change(EmailSentReceipt, :count)
+      expect { job.perform_now }.not_to change(EmailSentReceipt, :count)
     end
   end
 end

From 988cf7e888ba8dbe60fd8277c9b6a4a55212e4ac Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 7 Aug 2015 15:34:11 +0100
Subject: [PATCH 2/9] Change default feedback email address

---
 app/models/site.rb                                     |  6 +++++-
 .../20150805142206_change_default_feedback_email.rb    |  9 +++++++++
 db/structure.sql                                       |  4 +++-
 spec/models/site_spec.rb                               | 10 +++++-----
 4 files changed, 22 insertions(+), 7 deletions(-)
 create mode 100644 db/migrate/20150805142206_change_default_feedback_email.rb

diff --git a/app/models/site.rb b/app/models/site.rb
index aff3ca1b3..13423c060 100644
--- a/app/models/site.rb
+++ b/app/models/site.rb
@@ -150,6 +150,10 @@ def default_host
       ENV.fetch('EPETITIONS_HOST', 'petition.parliament.uk')
     end
 
+    def default_domain(tld_length = 1)
+      ActionDispatch::Http::URL.extract_domain(default_host, tld_length)
+    end
+
     def default_moderate_url
       if ENV.fetch('EPETITIONS_PROTOCOL', 'https') == 'https'
         URI::HTTPS.build(default_moderate_url_components).to_s
@@ -175,7 +179,7 @@ def default_email_from
     end
 
     def default_feedback_email
-      ENV.fetch('EPETITIONS_FEEDBACK', %{"Petitions: UK Government and Parliament" <feedback@#{default_host}>})
+      ENV.fetch('EPETITIONS_FEEDBACK', %{"Petitions: UK Government and Parliament" <petitionscommittee@#{default_domain}>})
     end
 
     def default_username
diff --git a/db/migrate/20150805142206_change_default_feedback_email.rb b/db/migrate/20150805142206_change_default_feedback_email.rb
new file mode 100644
index 000000000..8cbbb3606
--- /dev/null
+++ b/db/migrate/20150805142206_change_default_feedback_email.rb
@@ -0,0 +1,9 @@
+class ChangeDefaultFeedbackEmail < ActiveRecord::Migration
+  def up
+    change_column_default :sites, :feedback_email, '"Petitions: UK Government and Parliament" <petitionscommittee@parliament.uk>'
+  end
+
+  def down
+    change_column_default :sites, :feedback_email, '"Petitions: UK Government and Parliament" <feedback@petition.parliament.uk>'
+  end
+end
diff --git a/db/structure.sql b/db/structure.sql
index a240af9f7..d3cde1d07 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -508,7 +508,7 @@ CREATE TABLE sites (
     last_checked_at timestamp without time zone,
     created_at timestamp without time zone NOT NULL,
     updated_at timestamp without time zone NOT NULL,
-    feedback_email character varying(100) DEFAULT '"Petitions: UK Government and Parliament" <feedback@petition.parliament.uk>'::character varying NOT NULL,
+    feedback_email character varying(100) DEFAULT '"Petitions: UK Government and Parliament" <petitionscommittee@parliament.uk>'::character varying NOT NULL,
     moderate_url character varying(50) DEFAULT 'https://moderate.petition.parliament.uk'::character varying NOT NULL
 );
 
@@ -1157,3 +1157,5 @@ INSERT INTO schema_migrations (version) VALUES ('20150714140659');
 
 INSERT INTO schema_migrations (version) VALUES ('20150730110838');
 
+INSERT INTO schema_migrations (version) VALUES ('20150805142206');
+
diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb
index c695257b1..8cff4c636 100644
--- a/spec/models/site_spec.rb
+++ b/spec/models/site_spec.rb
@@ -18,7 +18,7 @@
     it { is_expected.to have_db_column(:last_checked_at).of_type(:datetime).with_options(null: true, default: nil) }
     it { is_expected.to have_db_column(:created_at).of_type(:datetime).with_options(null: false) }
     it { is_expected.to have_db_column(:updated_at).of_type(:datetime).with_options(null: false) }
-    it { is_expected.to have_db_column(:feedback_email).of_type(:string).with_options(limit: 100, default: '"Petitions: UK Government and Parliament" <feedback@petition.parliament.uk>') }
+    it { is_expected.to have_db_column(:feedback_email).of_type(:string).with_options(limit: 100, default: '"Petitions: UK Government and Parliament" <petitionscommittee@parliament.uk>') }
   end
 
   describe "validations" do
@@ -283,12 +283,12 @@
     end
 
     describe "for feedback_email" do
-      it "defaults to 'feedback@petition.parliament.uk'" do
+      it "defaults to 'petitionscommittee@parliament.uk'" do
         allow(ENV).to receive(:fetch).with("EPETITIONS_PROTOCOL", "https").and_return("https")
         allow(ENV).to receive(:fetch).with("EPETITIONS_HOST", "petition.parliament.uk").and_return("petition.parliament.uk")
         allow(ENV).to receive(:fetch).with("EPETITIONS_PORT", '443').and_return(443)
 
-        expect(defaults[:feedback_email]).to eq(%{"Petitions: UK Government and Parliament" <feedback@petition.parliament.uk>})
+        expect(defaults[:feedback_email]).to eq(%{"Petitions: UK Government and Parliament" <petitionscommittee@parliament.uk>})
       end
 
       it "allows overriding via the url environment variables" do
@@ -296,11 +296,11 @@
         allow(ENV).to receive(:fetch).with("EPETITIONS_HOST", "petition.parliament.uk").and_return("localhost")
         allow(ENV).to receive(:fetch).with("EPETITIONS_PORT", '443').and_return("3000")
 
-        expect(defaults[:feedback_email]).to eq(%{"Petitions: UK Government and Parliament" <feedback@localhost>})
+        expect(defaults[:feedback_email]).to eq(%{"Petitions: UK Government and Parliament" <petitionscommittee@localhost>})
       end
 
       it "allows overriding via the EPETITIONS_FEEDBACK environment variables" do
-        allow(ENV).to receive(:fetch).with("EPETITIONS_FEEDBACK", %{"Petitions: UK Government and Parliament" <feedback@petition.parliament.uk>}).and_return("petitions@downingstreet.gov.uk")
+        allow(ENV).to receive(:fetch).with("EPETITIONS_FEEDBACK", %{"Petitions: UK Government and Parliament" <petitionscommittee@parliament.uk>}).and_return("petitions@downingstreet.gov.uk")
         expect(defaults[:feedback_email]).to eq("petitions@downingstreet.gov.uk")
       end
     end

From bbf4dddb2226dc5ab488995ebb323cf2b535c1ac Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 7 Aug 2015 15:35:12 +0100
Subject: [PATCH 3/9] Add dummy signature model for feedback email address

We want to be able to send a copy to the feedback email address of any
custom emails that are sent but to do that it needs to act like a
signature so create a model with just enough behaviour to satisfy the
mailer and templates.
---
 app/models/feedback_signature.rb       | 23 ++++++++++++++++
 spec/models/feedback_signature_spec.rb | 36 ++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 app/models/feedback_signature.rb
 create mode 100644 spec/models/feedback_signature_spec.rb

diff --git a/app/models/feedback_signature.rb b/app/models/feedback_signature.rb
new file mode 100644
index 000000000..81c621abf
--- /dev/null
+++ b/app/models/feedback_signature.rb
@@ -0,0 +1,23 @@
+FeedbackSignature = Struct.new(:petition) do
+  def name
+    'Petitions team'
+  end
+
+  def email
+    rfc2822.address
+  end
+
+  def unsubscribe_token
+    'ThisIsNotAToken'
+  end
+
+  def to_param
+    '0'
+  end
+
+  private
+
+  def rfc2822
+    @rfc2822 ||= Mail::Address.new(Site.feedback_email)
+  end
+end
diff --git a/spec/models/feedback_signature_spec.rb b/spec/models/feedback_signature_spec.rb
new file mode 100644
index 000000000..08addb53b
--- /dev/null
+++ b/spec/models/feedback_signature_spec.rb
@@ -0,0 +1,36 @@
+require 'rails_helper'
+
+RSpec.describe FeedbackSignature, type: :model do
+  let(:petition) { double(:petition) }
+  let(:signature) { described_class.new(petition) }
+
+  describe "#name" do
+    it "returns 'Petitions team'" do
+      expect(signature.name).to eq("Petitions team")
+    end
+  end
+
+  describe "#email" do
+    it "returns 'petitionscommittee@parliament.uk'" do
+      expect(signature.email).to eq("petitionscommittee@parliament.uk")
+    end
+  end
+
+  describe "#petition" do
+    it "returns the petition passed to new" do
+      expect(signature.petition).to eq(petition)
+    end
+  end
+
+  describe "#unsubscribe_token" do
+    it "returns a dummy token" do
+      expect(signature.unsubscribe_token).to eq("ThisIsNotAToken")
+    end
+  end
+
+  describe "#to_param" do
+    it "returns a dummy id" do
+      expect(signature.to_param).to eq("0")
+    end
+  end
+end

From fe8e54fab95cc43381df46a0662db62e59c59872 Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 7 Aug 2015 15:47:01 +0100
Subject: [PATCH 4/9] Add Petition::Email model and email receipt/sent
 timestamp

To store the subject and message for the email to be delivered later we
need to create a model that will be passed to the job.
---
 app/models/petition.rb                        |  1 +
 app/models/petition/email.rb                  |  9 +++
 .../20150805142254_create_petition_emails.rb  | 14 ++++
 ...52_add_petition_email_to_email_receipts.rb |  6 ++
 db/structure.sql                              | 74 ++++++++++++++++++-
 spec/factories.rb                             |  7 ++
 spec/models/petition/email_spec.rb            | 25 +++++++
 spec/models/petition_spec.rb                  |  2 +
 8 files changed, 136 insertions(+), 2 deletions(-)
 create mode 100644 app/models/petition/email.rb
 create mode 100644 db/migrate/20150805142254_create_petition_emails.rb
 create mode 100644 db/migrate/20150806140552_add_petition_email_to_email_receipts.rb
 create mode 100644 spec/models/petition/email_spec.rb

diff --git a/app/models/petition.rb b/app/models/petition.rb
index fa8a9e5d2..806c406c9 100644
--- a/app/models/petition.rb
+++ b/app/models/petition.rb
@@ -61,6 +61,7 @@ class Petition < ActiveRecord::Base
   has_many :sponsors
   has_many :sponsor_signatures, :through => :sponsors, :source => :signature
   has_many :constituency_petition_journals, :dependent => :destroy
+  has_many :emails, :dependent => :destroy
 
   include Staged::Validations::PetitionDetails
   validates_presence_of :open_at, if: :open?
diff --git a/app/models/petition/email.rb b/app/models/petition/email.rb
new file mode 100644
index 000000000..c65cb8c55
--- /dev/null
+++ b/app/models/petition/email.rb
@@ -0,0 +1,9 @@
+class Petition < ActiveRecord::Base
+  class Email < ActiveRecord::Base
+    belongs_to :petition
+
+    validates :petition, presence: true
+    validates :subject, presence: true, length: { maximum: 100 }
+    validates :body, presence: true, length: { maximum: 5000 }
+  end
+end
diff --git a/db/migrate/20150805142254_create_petition_emails.rb b/db/migrate/20150805142254_create_petition_emails.rb
new file mode 100644
index 000000000..518a1804d
--- /dev/null
+++ b/db/migrate/20150805142254_create_petition_emails.rb
@@ -0,0 +1,14 @@
+class CreatePetitionEmails < ActiveRecord::Migration
+  def change
+    create_table :petition_emails do |t|
+      t.references :petition
+      t.string :subject, null: false
+      t.text :body
+      t.string :sent_by
+      t.timestamps null: false
+    end
+
+    add_index :petition_emails, :petition_id
+    add_foreign_key :petition_emails, :petitions, on_delete: :cascade
+  end
+end
diff --git a/db/migrate/20150806140552_add_petition_email_to_email_receipts.rb b/db/migrate/20150806140552_add_petition_email_to_email_receipts.rb
new file mode 100644
index 000000000..e4d9eeaee
--- /dev/null
+++ b/db/migrate/20150806140552_add_petition_email_to_email_receipts.rb
@@ -0,0 +1,6 @@
+class AddPetitionEmailToEmailReceipts < ActiveRecord::Migration
+  def change
+    add_column :email_requested_receipts, :petition_email, :datetime
+    add_column :email_sent_receipts, :petition_email, :datetime
+  end
+end
diff --git a/db/structure.sql b/db/structure.sql
index d3cde1d07..a45526848 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -230,7 +230,8 @@ CREATE TABLE email_requested_receipts (
     debate_outcome timestamp without time zone,
     created_at timestamp without time zone NOT NULL,
     updated_at timestamp without time zone NOT NULL,
-    debate_scheduled timestamp without time zone
+    debate_scheduled timestamp without time zone,
+    petition_email timestamp without time zone
 );
 
 
@@ -264,7 +265,8 @@ CREATE TABLE email_sent_receipts (
     debate_outcome timestamp without time zone,
     created_at timestamp without time zone NOT NULL,
     updated_at timestamp without time zone NOT NULL,
-    debate_scheduled timestamp without time zone
+    debate_scheduled timestamp without time zone,
+    petition_email timestamp without time zone
 );
 
 
@@ -352,6 +354,40 @@ CREATE SEQUENCE notes_id_seq
 ALTER SEQUENCE notes_id_seq OWNED BY notes.id;
 
 
+--
+-- Name: petition_emails; Type: TABLE; Schema: public; Owner: -; Tablespace: 
+--
+
+CREATE TABLE petition_emails (
+    id integer NOT NULL,
+    petition_id integer,
+    subject character varying NOT NULL,
+    body text,
+    sent_by character varying,
+    created_at timestamp without time zone NOT NULL,
+    updated_at timestamp without time zone NOT NULL
+);
+
+
+--
+-- Name: petition_emails_id_seq; Type: SEQUENCE; Schema: public; Owner: -
+--
+
+CREATE SEQUENCE petition_emails_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+
+--
+-- Name: petition_emails_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
+--
+
+ALTER SEQUENCE petition_emails_id_seq OWNED BY petition_emails.id;
+
+
 --
 -- Name: petitions; Type: TABLE; Schema: public; Owner: -; Tablespace: 
 --
@@ -627,6 +663,13 @@ ALTER TABLE ONLY government_responses ALTER COLUMN id SET DEFAULT nextval('gover
 ALTER TABLE ONLY notes ALTER COLUMN id SET DEFAULT nextval('notes_id_seq'::regclass);
 
 
+--
+-- Name: id; Type: DEFAULT; Schema: public; Owner: -
+--
+
+ALTER TABLE ONLY petition_emails ALTER COLUMN id SET DEFAULT nextval('petition_emails_id_seq'::regclass);
+
+
 --
 -- Name: id; Type: DEFAULT; Schema: public; Owner: -
 --
@@ -734,6 +777,14 @@ ALTER TABLE ONLY notes
     ADD CONSTRAINT notes_pkey PRIMARY KEY (id);
 
 
+--
+-- Name: petition_emails_pkey; Type: CONSTRAINT; Schema: public; Owner: -; Tablespace: 
+--
+
+ALTER TABLE ONLY petition_emails
+    ADD CONSTRAINT petition_emails_pkey PRIMARY KEY (id);
+
+
 --
 -- Name: petitions_pkey; Type: CONSTRAINT; Schema: public; Owner: -; Tablespace: 
 --
@@ -872,6 +923,13 @@ CREATE UNIQUE INDEX index_government_responses_on_petition_id ON government_resp
 CREATE UNIQUE INDEX index_notes_on_petition_id ON notes USING btree (petition_id);
 
 
+--
+-- Name: index_petition_emails_on_petition_id; Type: INDEX; Schema: public; Owner: -; Tablespace: 
+--
+
+CREATE INDEX index_petition_emails_on_petition_id ON petition_emails USING btree (petition_id);
+
+
 --
 -- Name: index_petitions_on_action; Type: INDEX; Schema: public; Owner: -; Tablespace: 
 --
@@ -1041,6 +1099,14 @@ ALTER TABLE ONLY email_requested_receipts
     ADD CONSTRAINT fk_rails_898597541e FOREIGN KEY (petition_id) REFERENCES petitions(id);
 
 
+--
+-- Name: fk_rails_9f55aacb99; Type: FK CONSTRAINT; Schema: public; Owner: -
+--
+
+ALTER TABLE ONLY petition_emails
+    ADD CONSTRAINT fk_rails_9f55aacb99 FOREIGN KEY (petition_id) REFERENCES petitions(id) ON DELETE CASCADE;
+
+
 --
 -- Name: fk_rails_bc381510eb; Type: FK CONSTRAINT; Schema: public; Owner: -
 --
@@ -1159,3 +1225,7 @@ INSERT INTO schema_migrations (version) VALUES ('20150730110838');
 
 INSERT INTO schema_migrations (version) VALUES ('20150805142206');
 
+INSERT INTO schema_migrations (version) VALUES ('20150805142254');
+
+INSERT INTO schema_migrations (version) VALUES ('20150806140552');
+
diff --git a/spec/factories.rb b/spec/factories.rb
index d14be830d..d73ffd1c4 100644
--- a/spec/factories.rb
+++ b/spec/factories.rb
@@ -263,6 +263,13 @@
     details "Petition notes"
   end
 
+  factory :petition_email, class: "Petition::Email" do
+    association :petition, factory: :petition
+    subject "Message Subject"
+    body "Message body"
+    sent_by "Admin User"
+  end
+
   factory :rejection do
     association :petition, factory: :validated_petition
     code "duplicate"
diff --git a/spec/models/petition/email_spec.rb b/spec/models/petition/email_spec.rb
new file mode 100644
index 000000000..bf68cddb9
--- /dev/null
+++ b/spec/models/petition/email_spec.rb
@@ -0,0 +1,25 @@
+require 'rails_helper'
+
+RSpec.describe Petition::Email, type: :model do
+  it "has a valid factory" do
+    expect(FactoryGirl.build(:petition_email)).to be_valid
+  end
+
+  describe "associations" do
+    it { is_expected.to belong_to(:petition) }
+  end
+
+  describe "indexes" do
+    it { is_expected.to have_db_index([:petition_id]) }
+  end
+
+  describe "validations" do
+    subject { FactoryGirl.build(:petition_email) }
+
+    it { is_expected.to validate_presence_of(:petition) }
+    it { is_expected.to validate_presence_of(:subject) }
+    it { is_expected.to validate_length_of(:subject).is_at_most(100) }
+    it { is_expected.to validate_presence_of(:body) }
+    it { is_expected.to validate_length_of(:body).is_at_most(5000) }
+  end
+end
diff --git a/spec/models/petition_spec.rb b/spec/models/petition_spec.rb
index b762a6883..76f9cebd9 100644
--- a/spec/models/petition_spec.rb
+++ b/spec/models/petition_spec.rb
@@ -18,6 +18,8 @@
   describe "associations" do
     it { is_expected.to have_one(:debate_outcome).dependent(:destroy) }
     it { is_expected.to have_one(:government_response).dependent(:destroy) }
+
+    it { is_expected.to have_many(:emails).dependent(:destroy) }
   end
 
   context "validations" do

From 08166da0b2cbf9ac18aaa6fb0cba2e51fbaee249 Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 7 Aug 2015 16:01:41 +0100
Subject: [PATCH 5/9] Add custom email to petitioners

---
 app/mailers/petition_mailer.rb                |  9 +++
 .../petition_mailer/email_signer.html.erb     | 13 ++++
 .../petition_mailer/email_signer.text.erb     | 13 ++++
 config/locales/petitions.en-GB.yml            |  2 +
 spec/mailers/petition_mailer_spec.rb          | 59 +++++++++++++++----
 .../previews/petition_mailer_preview.rb       | 10 ++++
 6 files changed, 93 insertions(+), 13 deletions(-)
 create mode 100644 app/views/petition_mailer/email_signer.html.erb
 create mode 100644 app/views/petition_mailer/email_signer.text.erb

diff --git a/app/mailers/petition_mailer.rb b/app/mailers/petition_mailer.rb
index 3a6ba6ff1..ab89cc717 100644
--- a/app/mailers/petition_mailer.rb
+++ b/app/mailers/petition_mailer.rb
@@ -6,6 +6,11 @@ def email_confirmation_for_signer(signature)
     mail to: @signature.email, subject: subject_for(:email_confirmation_for_signer)
   end
 
+  def email_signer(petition, signature, email)
+    @petition, @signature, @email = petition, signature, email
+    mail to: @signature.email, subject: subject_for(:email_signer)
+  end
+
   def special_resend_of_email_confirmation_for_signer(signature)
     @signature = signature
     mail to: @signature.email, subject: subject_for(:special_resend_of_email_confirmation_for_signer)
@@ -78,6 +83,10 @@ def i18n_options
         options[:formatted_count] = number_to_delimited(@petition.signature_count)
         options[:action] = @petition.action
       end
+
+      if defined?(@email)
+        options[:subject] = @email.subject
+      end
     end
   end
 end
diff --git a/app/views/petition_mailer/email_signer.html.erb b/app/views/petition_mailer/email_signer.html.erb
new file mode 100644
index 000000000..75febf7d3
--- /dev/null
+++ b/app/views/petition_mailer/email_signer.html.erb
@@ -0,0 +1,13 @@
+<p>Hi <%= @signature.name %>,</p>
+
+<%= auto_link(simple_format(h(@email.body))) %>
+
+<p>Click this link to view the petition "<%= @petition.action %>":</p>
+
+<p><%= link_to nil, petition_url(@petition) %></p>
+
+<p>Thanks,<br />
+<%= t("petitions.emails.signoff_prefix") %><br />
+<%= t("petitions.emails.signoff_suffix") %></p>
+
+<p><small>You’re receiving this email because you signed this petition. Unsubscribe: <%= link_to 'unsubscribe', unsubscribe_signature_url(@signature, @signature.unsubscribe_token) %></small></p>
diff --git a/app/views/petition_mailer/email_signer.text.erb b/app/views/petition_mailer/email_signer.text.erb
new file mode 100644
index 000000000..e97c5b3af
--- /dev/null
+++ b/app/views/petition_mailer/email_signer.text.erb
@@ -0,0 +1,13 @@
+Hi <%= @signature.name %>,
+
+<%= @email.body %>
+
+Click this link to view the petition "<%= @petition.action %>":
+
+<%= petition_url(@petition) %>
+
+Thanks,
+<%= t("petitions.emails.signoff_prefix") %>
+<%= t("petitions.emails.signoff_suffix") %>
+
+You’re receiving this email because you signed this petition. Unsubscribe: <%= unsubscribe_signature_url(@signature, @signature.unsubscribe_token) %>
diff --git a/config/locales/petitions.en-GB.yml b/config/locales/petitions.en-GB.yml
index 77994a577..e4ff32cb8 100644
--- a/config/locales/petitions.en-GB.yml
+++ b/config/locales/petitions.en-GB.yml
@@ -76,6 +76,8 @@ en-GB:
     emails:
       subjects:
         # Emails to people who sign petitions
+        email_signer: |-
+          %{subject}
         email_confirmation_for_signer: |-
           Please confirm your email address
         notify_signer_of_debate_outcome: |-
diff --git a/spec/mailers/petition_mailer_spec.rb b/spec/mailers/petition_mailer_spec.rb
index ede9e5ad8..5f4040eb7 100644
--- a/spec/mailers/petition_mailer_spec.rb
+++ b/spec/mailers/petition_mailer_spec.rb
@@ -31,11 +31,11 @@
       expect(mail.bcc).to be_blank
     end
 
-    it 'has an appropriate subject heading' do
+    it "has an appropriate subject heading" do
       expect(mail).to have_subject('We published your petition "Allow organic vegetable vans to use red diesel"')
     end
 
-    it 'is addressed to the creator' do
+    it "is addressed to the creator" do
       expect(mail).to have_body_text("Hi Barry Butler,")
     end
 
@@ -64,11 +64,11 @@
       expect(mail.bcc).to be_blank
     end
 
-    it 'has an appropriate subject heading' do
+    it "has an appropriate subject heading" do
       expect(mail).to have_subject('We published the petition "Allow organic vegetable vans to use red diesel" that you supported')
     end
 
-    it 'is addressed to the sponsor' do
+    it "is addressed to the sponsor" do
       expect(mail).to have_body_text("Hi Laura Palmer,")
     end
 
@@ -90,11 +90,11 @@
       expect(mail.bcc).to be_blank
     end
 
-    it 'has an appropriate subject heading' do
+    it "has an appropriate subject heading" do
       expect(mail).to have_subject("We’re closing your petition early")
     end
 
-    it 'is addressed to the creator' do
+    it "is addressed to the creator" do
       expect(mail).to have_body_text("Hi Barry Butler,")
     end
 
@@ -103,7 +103,7 @@
     end
   end
 
-  describe 'gathering sponsors for petition' do
+  describe "gathering sponsors for petition" do
     subject(:mail) { described_class.gather_sponsors_for_petition(petition) }
 
     it "has the correct subject" do
@@ -137,8 +137,8 @@
     end
   end
 
-  describe 'notifying signature of debate outcome' do
-    let(:signature) { FactoryGirl.create(:validated_signature, petition: petition, name: 'Laura Palmer', email: 'laura@red-room.example.com') }
+  describe "notifying signature of debate outcome" do
+    let(:signature) { FactoryGirl.create(:validated_signature, petition: petition, name: "Laura Palmer", email: "laura@red-room.example.com") }
     before { FactoryGirl.create(:debate_outcome, petition: petition) }
     subject(:mail) { described_class.notify_signer_of_debate_outcome(petition, signature) }
 
@@ -164,14 +164,14 @@
       expect(mail).to have_body_text(%r[Allow organic vegetable vans to use red diesel])
     end
 
-    it 'includes an unsubscribe link' do
+    it "includes an unsubscribe link" do
       expect(mail).to have_body_text(%r[https://petition.parliament.uk/signatures/#{signature.id}/unsubscribe/#{signature.unsubscribe_token}])
     end
   end
 
-  describe 'notifying signature of debate scheduled' do
+  describe "notifying signature of debate scheduled" do
     let(:petition) { FactoryGirl.create(:open_petition, :scheduled_for_debate, action: "Allow organic vegetable vans to use red diesel") }
-    let(:signature) { FactoryGirl.create(:validated_signature, petition: petition, name: 'Laura Palmer', email: 'laura@red-room.example.com') }
+    let(:signature) { FactoryGirl.create(:validated_signature, petition: petition, name: "Laura Palmer", email: "laura@red-room.example.com") }
     subject(:mail) { described_class.notify_signer_of_debate_scheduled(petition, signature) }
 
     it "has the correct subject" do
@@ -192,8 +192,41 @@
       expect(mail).to have_body_text(%r[https://petition.parliament.uk/petitions/#{petition.id}])
     end
 
-    it 'includes an unsubscribe link' do
+    it "includes an unsubscribe link" do
       expect(mail).to have_body_text(%r[https://petition.parliament.uk/signatures/#{signature.id}/unsubscribe/#{signature.unsubscribe_token}])
     end
   end
+
+  describe "emailing a signature" do
+    let(:petition) { FactoryGirl.create(:open_petition, :scheduled_for_debate, action: "Allow organic vegetable vans to use red diesel") }
+    let(:signature) { FactoryGirl.create(:validated_signature, petition: petition, name: "Laura Palmer", email: "laura@red-room.example.com") }
+    let(:email) { FactoryGirl.create(:petition_email, petition: petition, subject: "This is a message from the committee", body: "Message body from the petition committee") }
+    subject(:mail) { described_class.email_signer(petition, signature, email) }
+
+    it "has the correct subject" do
+      expect(mail).to have_subject("This is a message from the committee")
+    end
+
+    it "addresses the signatory by name" do
+      expect(mail).to have_body_text("Hi Laura Palmer,")
+    end
+
+    it "sends it only to the signatory" do
+      expect(mail.to).to eq(%w[laura@red-room.example.com])
+      expect(mail.cc).to be_blank
+      expect(mail.bcc).to be_blank
+    end
+
+    it "includes a link to the petition page" do
+      expect(mail).to have_body_text(%r[https://petition.parliament.uk/petitions/#{petition.id}])
+    end
+
+    it "includes an unsubscribe link" do
+      expect(mail).to have_body_text(%r[https://petition.parliament.uk/signatures/#{signature.id}/unsubscribe/#{signature.unsubscribe_token}])
+    end
+
+    it "includes the message body" do
+      expect(mail).to have_body_text(%r[Message body from the petition committee])
+    end
+  end
 end
diff --git a/spec/mailers/previews/petition_mailer_preview.rb b/spec/mailers/previews/petition_mailer_preview.rb
index 0a0b07b20..afc5a3764 100644
--- a/spec/mailers/previews/petition_mailer_preview.rb
+++ b/spec/mailers/previews/petition_mailer_preview.rb
@@ -3,9 +3,19 @@ class PetitionMailerPreview < ActionMailer::Preview
   def email_confirmation_for_signer
     PetitionMailer.email_confirmation_for_signer(Signature.last)
   end
+
   def gather_sponsors_for_petition
     PetitionMailer.gather_sponsors_for_petition(Petition.last)
   end
+
+  def email_signer
+    email = Petition::Email.last
+    petition = email.petition
+    signature = petition.signatures.validated.last
+
+    PetitionMailer.email_signer(petition, signature, email)
+  end
+
   def notify_signer_of_threshold_response
     petition = Petition.with_response.last
     signature = petition.signatures.validated.last

From d3b3fccaea229d15d670263b87a18be6e51d02ee Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 7 Aug 2015 16:02:51 +0100
Subject: [PATCH 6/9] Fix character counter on response admin

The selector in the character-counter.js is looking for textareas with
a data-max-length attribute and not a data-maxlength attribute.
---
 .../_petition_action_government_response.html.erb               | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/admin/government_response/_petition_action_government_response.html.erb b/app/views/admin/government_response/_petition_action_government_response.html.erb
index f85ea1339..acf651108 100644
--- a/app/views/admin/government_response/_petition_action_government_response.html.erb
+++ b/app/views/admin/government_response/_petition_action_government_response.html.erb
@@ -3,7 +3,7 @@
   <%= form_row :for => [f.object, :summary] do %>
     <%= f.label :summary, 'Summary quote', class: 'form-label' %>
     <%= error_messages_for_field f.object, :summary %>
-    <%= f.text_area :summary, rows: 3, cols: 70, tabindex: increment, data: { maxlength: 200 }, class: 'form-control' %>
+    <%= f.text_area :summary, rows: 3, cols: 70, tabindex: increment, data: { max_length: 200 }, class: 'form-control' %>
     <p class="character-count">200 characters max</p>
   <% end %>
 

From 0afe2cb0252cdef6a2297fe2de9b746186b062e1 Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 7 Aug 2015 16:04:48 +0100
Subject: [PATCH 7/9] Remove bottom margin from character counter element

Because the element is floated to the right then the vertical margin
doesn't collapse leading to excessive white space between form rows.
Removing the margin from the bottom of the character counter fixes
the problem.
---
 app/assets/stylesheets/petitions/admin/views/_shared.scss | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/assets/stylesheets/petitions/admin/views/_shared.scss b/app/assets/stylesheets/petitions/admin/views/_shared.scss
index 37a820c7a..702085e98 100644
--- a/app/assets/stylesheets/petitions/admin/views/_shared.scss
+++ b/app/assets/stylesheets/petitions/admin/views/_shared.scss
@@ -20,6 +20,10 @@
     margin-bottom: $gutter-half;
   }
 
+  .character-count {
+    margin-bottom: 0;
+  }
+
   // Pagination
   .pagination {
     margin: $gutter-half 0;

From a76d28c114d734996afebcc3bedb227ad34471e5 Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 7 Aug 2015 17:10:03 +0100
Subject: [PATCH 8/9] Add jobs for queuing and delivering petition emails

---
 app/jobs/deliver_petition_email_job.rb        | 14 ++++++++
 app/jobs/email_petitioners_job.rb             | 19 +++++++++++
 .../deliver_debate_outcome_email_job_spec.rb  | 22 ++++++++-----
 ...deliver_debate_scheduled_email_job_spec.rb | 22 ++++++++-----
 spec/jobs/deliver_petition_email_job_spec.rb  | 33 +++++++++++++++++++
 ...liver_threshold_response_email_job_spec.rb | 22 ++++++++-----
 spec/jobs/email_debate_outcomes_job_spec.rb   |  1 +
 spec/jobs/email_debate_scheduled_job_spec.rb  |  1 +
 spec/jobs/email_petitioners_job_spec.rb       | 18 ++++++++++
 .../jobs/email_threshold_response_job_spec.rb |  1 +
 spec/jobs/shared_examples.rb                  | 18 ++--------
 11 files changed, 131 insertions(+), 40 deletions(-)
 create mode 100644 app/jobs/deliver_petition_email_job.rb
 create mode 100644 app/jobs/email_petitioners_job.rb
 create mode 100644 spec/jobs/deliver_petition_email_job_spec.rb
 create mode 100644 spec/jobs/email_petitioners_job_spec.rb

diff --git a/app/jobs/deliver_petition_email_job.rb b/app/jobs/deliver_petition_email_job.rb
new file mode 100644
index 000000000..08db193bd
--- /dev/null
+++ b/app/jobs/deliver_petition_email_job.rb
@@ -0,0 +1,14 @@
+class DeliverPetitionEmailJob < ActiveJob::Base
+  include EmailDelivery
+
+  attr_reader :email
+
+  def perform(**args)
+    @email = args[:email]
+    super
+  end
+
+  def create_email
+    mailer.email_signer petition, signature, email
+  end
+end
diff --git a/app/jobs/email_petitioners_job.rb b/app/jobs/email_petitioners_job.rb
new file mode 100644
index 000000000..7a538260e
--- /dev/null
+++ b/app/jobs/email_petitioners_job.rb
@@ -0,0 +1,19 @@
+class EmailPetitionersJob < ActiveJob::Base
+  include EmailAllPetitionSignatories
+
+  self.email_delivery_job_class = DeliverPetitionEmailJob
+  self.timestamp_name = 'petition_email'
+
+  attr_reader :email
+
+  def perform(**args)
+    @email = args[:email]
+    super
+  end
+
+  private
+
+  def mailer_arguments(signature)
+    super.merge(email: email)
+  end
+end
diff --git a/spec/jobs/deliver_debate_outcome_email_job_spec.rb b/spec/jobs/deliver_debate_outcome_email_job_spec.rb
index 95e22da3f..44c150a5b 100644
--- a/spec/jobs/deliver_debate_outcome_email_job_spec.rb
+++ b/spec/jobs/deliver_debate_outcome_email_job_spec.rb
@@ -2,24 +2,30 @@
 require_relative 'shared_examples'
 
 RSpec.describe DeliverDebateOutcomeEmailJob, type: :job do
-  let(:email_requested_at) { Time.current }
+  let(:requested_at) { Time.current.change(usec: 0) }
+  let(:requested_at_as_string) { requested_at.getutc.iso8601(6) }
+
   let(:petition) { FactoryGirl.create(:debated_petition) }
   let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) }
   let(:timestamp_name) { 'debate_outcome' }
 
+  let :arguments do
+    {
+      signature: signature,
+      timestamp_name: timestamp_name,
+      petition: petition,
+      requested_at: requested_at_as_string
+    }
+  end
+
   before do
-    petition.set_email_requested_at_for(timestamp_name, to: email_requested_at)
+    petition.set_email_requested_at_for(timestamp_name, to: requested_at)
   end
 
   it_behaves_like "a job to send an signatory email"
 
   it "uses the correct mailer method to generate the email" do
     expect(subject).to receive_message_chain(:mailer, :notify_signer_of_debate_outcome).with(petition, signature).and_return double.as_null_object
-    subject.perform(
-      signature: signature,
-      timestamp_name: timestamp_name,
-      petition: petition,
-      requested_at: email_requested_at.getutc.iso8601(6)
-    )
+    subject.perform(**arguments)
   end
 end
diff --git a/spec/jobs/deliver_debate_scheduled_email_job_spec.rb b/spec/jobs/deliver_debate_scheduled_email_job_spec.rb
index a0867bbcb..906b3c840 100644
--- a/spec/jobs/deliver_debate_scheduled_email_job_spec.rb
+++ b/spec/jobs/deliver_debate_scheduled_email_job_spec.rb
@@ -2,24 +2,30 @@
 require_relative 'shared_examples'
 
 RSpec.describe DeliverDebateScheduledEmailJob, type: :job do
-  let(:email_requested_at) { Time.current }
+  let(:requested_at) { Time.current.change(usec: 0) }
+  let(:requested_at_as_string) { requested_at.getutc.iso8601(6) }
+
   let(:petition) { FactoryGirl.create(:awaiting_debate_petition) }
   let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) }
   let(:timestamp_name) { 'debate_scheduled' }
 
+  let :arguments do
+    {
+      signature: signature,
+      timestamp_name: timestamp_name,
+      petition: petition,
+      requested_at: requested_at_as_string
+    }
+  end
+
   before do
-    petition.set_email_requested_at_for(timestamp_name, to: email_requested_at)
+    petition.set_email_requested_at_for(timestamp_name, to: requested_at)
   end
 
   it_behaves_like "a job to send an signatory email"
 
   it "uses the correct mailer method to generate the email" do
     expect(subject).to receive_message_chain(:mailer, :notify_signer_of_debate_scheduled).with(petition, signature).and_return double.as_null_object
-    subject.perform(
-      signature: signature,
-      timestamp_name: timestamp_name,
-      petition: petition,
-      requested_at: email_requested_at.getutc.iso8601(6)
-    )
+    subject.perform(**arguments)
   end
 end
diff --git a/spec/jobs/deliver_petition_email_job_spec.rb b/spec/jobs/deliver_petition_email_job_spec.rb
new file mode 100644
index 000000000..f757f12ee
--- /dev/null
+++ b/spec/jobs/deliver_petition_email_job_spec.rb
@@ -0,0 +1,33 @@
+require 'rails_helper'
+require_relative 'shared_examples'
+
+RSpec.describe DeliverPetitionEmailJob, type: :job do
+  let(:requested_at) { Time.current.change(usec: 0) }
+  let(:requested_at_as_string) { requested_at.getutc.iso8601(6) }
+
+  let(:petition) { FactoryGirl.create(:debated_petition) }
+  let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) }
+  let(:email) { FactoryGirl.create(:petition_email, petition: petition) }
+  let(:timestamp_name) { 'petition_email' }
+
+  let :arguments do
+    {
+      signature: signature,
+      timestamp_name: timestamp_name,
+      petition: petition,
+      requested_at: requested_at_as_string,
+      email: email
+    }
+  end
+
+  before do
+    petition.set_email_requested_at_for(timestamp_name, to: requested_at)
+  end
+
+  it_behaves_like "a job to send an signatory email"
+
+  it "uses the correct mailer method to generate the email" do
+    expect(subject).to receive_message_chain(:mailer, :email_signer).with(petition, signature, email).and_return double.as_null_object
+    subject.perform(**arguments)
+  end
+end
diff --git a/spec/jobs/deliver_threshold_response_email_job_spec.rb b/spec/jobs/deliver_threshold_response_email_job_spec.rb
index c83f727cc..3ebb6ab87 100644
--- a/spec/jobs/deliver_threshold_response_email_job_spec.rb
+++ b/spec/jobs/deliver_threshold_response_email_job_spec.rb
@@ -2,24 +2,30 @@
 require_relative 'shared_examples'
 
 RSpec.describe DeliverThresholdResponseEmailJob, type: :job do
-  let(:email_requested_at) { Time.current }
+  let(:requested_at) { Time.current.change(usec: 0) }
+  let(:requested_at_as_string) { requested_at.getutc.iso8601(6) }
+
   let(:petition) { FactoryGirl.create(:responded_petition) }
   let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) }
   let(:timestamp_name) { 'government_response' }
 
+  let :arguments do
+    {
+      signature: signature,
+      timestamp_name: timestamp_name,
+      petition: petition,
+      requested_at: requested_at_as_string
+    }
+  end
+
   before do
-    petition.set_email_requested_at_for(timestamp_name, to: email_requested_at)
+    petition.set_email_requested_at_for(timestamp_name, to: requested_at)
   end
 
   it_behaves_like "a job to send an signatory email"
 
   it "uses the correct mailer method to generate the email" do
     expect(subject).to receive_message_chain(:mailer, :notify_signer_of_threshold_response).with(petition, signature).and_return double.as_null_object
-    subject.perform(
-      signature: signature,
-      timestamp_name: timestamp_name,
-      petition: petition,
-      requested_at: email_requested_at.getutc.iso8601
-    )
+    subject.perform(**arguments)
   end
 end
diff --git a/spec/jobs/email_debate_outcomes_job_spec.rb b/spec/jobs/email_debate_outcomes_job_spec.rb
index 686b49af8..92067c90f 100644
--- a/spec/jobs/email_debate_outcomes_job_spec.rb
+++ b/spec/jobs/email_debate_outcomes_job_spec.rb
@@ -5,6 +5,7 @@
   let(:email_requested_at) { Time.current }
   let(:petition) { FactoryGirl.create(:open_petition) }
   let(:signature) { FactoryGirl.create(:validated_signature, :petition => petition) }
+  let(:arguments) { { petition: petition } }
 
   before do
     petition.set_email_requested_at_for('debate_outcome', to: email_requested_at)
diff --git a/spec/jobs/email_debate_scheduled_job_spec.rb b/spec/jobs/email_debate_scheduled_job_spec.rb
index 525227394..06dba4e0f 100644
--- a/spec/jobs/email_debate_scheduled_job_spec.rb
+++ b/spec/jobs/email_debate_scheduled_job_spec.rb
@@ -5,6 +5,7 @@
   let(:email_requested_at) { Time.current }
   let(:petition) { FactoryGirl.create(:open_petition) }
   let(:signature) { FactoryGirl.create(:validated_signature, :petition => petition) }
+  let(:arguments) { { petition: petition } }
 
   before do
     petition.set_email_requested_at_for('debate_scheduled', to: email_requested_at)
diff --git a/spec/jobs/email_petitioners_job_spec.rb b/spec/jobs/email_petitioners_job_spec.rb
new file mode 100644
index 000000000..280529c12
--- /dev/null
+++ b/spec/jobs/email_petitioners_job_spec.rb
@@ -0,0 +1,18 @@
+require 'rails_helper'
+require_relative 'shared_examples'
+
+RSpec.describe EmailPetitionersJob, type: :job do
+  let(:email_requested_at) { Time.current }
+  let(:petition) { FactoryGirl.create(:open_petition) }
+  let(:signature) { FactoryGirl.create(:validated_signature, :petition => petition) }
+  let(:email) { FactoryGirl.create(:petition_email, petition: petition) }
+  let(:arguments) { { petition: petition, email: email } }
+
+  before do
+    petition.set_email_requested_at_for('petition_email', to: email_requested_at)
+    allow(petition).to receive_message_chain(:signatures_to_email_for, :find_each).and_yield(signature)
+  end
+
+  it_behaves_like "job to enqueue signatory mailing jobs"
+
+end
diff --git a/spec/jobs/email_threshold_response_job_spec.rb b/spec/jobs/email_threshold_response_job_spec.rb
index b1f432d20..2c7bda3c9 100644
--- a/spec/jobs/email_threshold_response_job_spec.rb
+++ b/spec/jobs/email_threshold_response_job_spec.rb
@@ -5,6 +5,7 @@
   let(:email_requested_at) { Time.current }
   let(:petition) { FactoryGirl.create(:open_petition) }
   let(:signature) { FactoryGirl.create(:validated_signature, :petition => petition) }
+  let(:arguments) { { petition: petition } }
 
   before do
     petition.set_email_requested_at_for('government_response', to: email_requested_at)
diff --git a/spec/jobs/shared_examples.rb b/spec/jobs/shared_examples.rb
index 138d371e4..feba74b23 100644
--- a/spec/jobs/shared_examples.rb
+++ b/spec/jobs/shared_examples.rb
@@ -30,7 +30,7 @@ def do_work
     end
 
     before do
-      described_class.run_later_tonight(petition: petition)
+      described_class.run_later_tonight(**arguments)
     end
 
     it 'queues up a job' do
@@ -82,21 +82,7 @@ def do_work
 end
 
 RSpec.shared_examples_for "a job to send an signatory email" do
-  let(:requested_at) { Time.current.change(usec: 0) }
-  let(:requested_at_as_string) { requested_at.getutc.iso8601(6) }
-
-  let :arguments do
-    {
-      signature: signature,
-      timestamp_name: timestamp_name,
-      petition: petition,
-      requested_at: requested_at_as_string
-    }
-  end
-
-  let :job do
-    described_class.new(arguments)
-  end
+  let(:job) { described_class.new(arguments) }
 
   context "when the petition has not been updated" do
     let(:mail_object) { double(:mail_object) }

From f716fce538cc4928a099682c9264ce744b9cd140 Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 7 Aug 2015 17:57:49 +0100
Subject: [PATCH 9/9] Add admin page for emailing petitioners

---
 .../admin/petition_emails_controller.rb       |  38 +++
 ...petition_action_email_petitioners.html.erb |   1 +
 ...petition_action_email_petitioners.html.erb |  20 ++
 .../petitions/_petition_actions.html.erb      |   4 +
 config/routes.rb                              |   1 +
 features/admin/petition_email.feature         |  23 ++
 .../step_definitions/petition_email_steps.rb  |  52 ++++
 features/support/paths.rb                     |   3 +
 .../admin/petition_emails_controller_spec.rb  | 249 ++++++++++++++++++
 9 files changed, 391 insertions(+)
 create mode 100644 app/controllers/admin/petition_emails_controller.rb
 create mode 100644 app/views/admin/admin/_petition_action_email_petitioners.html.erb
 create mode 100644 app/views/admin/petition_emails/_petition_action_email_petitioners.html.erb
 create mode 100644 features/admin/petition_email.feature
 create mode 100644 features/step_definitions/petition_email_steps.rb
 create mode 100644 spec/controllers/admin/petition_emails_controller_spec.rb

diff --git a/app/controllers/admin/petition_emails_controller.rb b/app/controllers/admin/petition_emails_controller.rb
new file mode 100644
index 000000000..925fb21dc
--- /dev/null
+++ b/app/controllers/admin/petition_emails_controller.rb
@@ -0,0 +1,38 @@
+class Admin::PetitionEmailsController < Admin::AdminController
+  respond_to :html
+  before_action :fetch_petition
+  before_action :build_email
+
+  def new
+    render 'admin/petitions/show'
+  end
+
+  def create
+    if @email.update(email_params)
+      EmailPetitionersJob.run_later_tonight(petition: @petition, email: @email)
+      PetitionMailer.email_signer(@petition, feedback_signature, @email).deliver_now
+
+      redirect_to [:admin, @petition], notice: 'Email will be sent overnight'
+    else
+      render 'admin/petitions/show'
+    end
+  end
+
+  private
+
+  def fetch_petition
+    @petition = Petition.moderated.find(params[:petition_id])
+  end
+
+  def build_email
+    @email = @petition.emails.build(sent_by: current_user.pretty_name)
+  end
+
+  def email_params
+    params.require(:petition_email).permit(:subject, :body)
+  end
+
+  def feedback_signature
+    FeedbackSignature.new(@petition)
+  end
+end
diff --git a/app/views/admin/admin/_petition_action_email_petitioners.html.erb b/app/views/admin/admin/_petition_action_email_petitioners.html.erb
new file mode 100644
index 000000000..2d0c129f9
--- /dev/null
+++ b/app/views/admin/admin/_petition_action_email_petitioners.html.erb
@@ -0,0 +1 @@
+<%= link_to 'Email petitioners', new_admin_petition_email_path(@petition), class: 'petition-action-heading' %>
diff --git a/app/views/admin/petition_emails/_petition_action_email_petitioners.html.erb b/app/views/admin/petition_emails/_petition_action_email_petitioners.html.erb
new file mode 100644
index 000000000..15be88214
--- /dev/null
+++ b/app/views/admin/petition_emails/_petition_action_email_petitioners.html.erb
@@ -0,0 +1,20 @@
+<h2 class="petition-action-heading">Email petitioners</h2>
+<%= form_for @email, url: admin_petition_emails_path(petition), method: :post do |f| -%>
+  <%= form_row :for => [f.object, :subject] do %>
+    <%= f.label :subject, 'Subject', class: 'form-label' %>
+    <%= error_messages_for_field f.object, :subject %>
+    <%= f.text_area :subject, rows: 2, cols: 70, tabindex: increment, data: { max_length: 100 }, class: 'form-control' %>
+    <p class="character-count">100 characters max</p>
+  <% end %>
+
+  <%= form_row :for => [f.object, :body] do %>
+    <%= f.label :body, 'Body', class: 'form-label' %>
+    <%= error_messages_for_field f.object, :body %>
+    <%= f.text_area :body, rows: 8, cols: 70, tabindex: increment, data: { max_length: 5000 }, class: 'form-control' %>
+    <p class="character-count">5000 characters max</p>
+  <% end %>
+
+  <%= email_signatures_with_count_submit_button(f, petition) %>
+<% end -%>
+
+<%= javascript_include_tag 'character-counter' %>
diff --git a/app/views/admin/petitions/_petition_actions.html.erb b/app/views/admin/petitions/_petition_actions.html.erb
index 4ecc5af50..7201e6987 100644
--- a/app/views/admin/petitions/_petition_actions.html.erb
+++ b/app/views/admin/petitions/_petition_actions.html.erb
@@ -31,6 +31,10 @@
         </li>
       <% end %>
 
+      <li class="petition-action">
+        <%= render 'petition_action_email_petitioners', petition: @petition %>
+      </li>
+
       <% if current_user.can_take_petitions_down? && @petition.can_be_signed? %>
         <li class="petition-action">
           <%= render 'petition_action_take_down', petition: @petition %>
diff --git a/config/routes.rb b/config/routes.rb
index 2e0c54c91..775e920ff 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -74,6 +74,7 @@
       resources :admin_users
       resources :petitions, :only => [:show, :index] do
         resource 'debate-outcome', only: [:show, :update], as: :debate_outcome, controller: :debate_outcomes
+        resources :emails, only: [:new, :create], controller: :petition_emails
         resource :petition_details, :only => [:show, :update]
         resource :moderation, :only => [:update], controller: :moderation
         resource :notes, :only => [:show, :update]
diff --git a/features/admin/petition_email.feature b/features/admin/petition_email.feature
new file mode 100644
index 000000000..478509c60
--- /dev/null
+++ b/features/admin/petition_email.feature
@@ -0,0 +1,23 @@
+@admin
+Feature: Emailing petitioner supporters
+  In order to keep petition supporters up-to-date on their petition
+  As an admin user
+  I want to send an email to all petition supporters
+
+  Scenario: Sending an email to all petitioners
+    Given an open petition "Ban Badger Baiting" with some signatures
+    And I am logged in as a sysadmin with the email "admin@example.com", first_name "Admin", last_name "User"
+    When I am on the admin all petitions page
+    And I follow "Ban Badger Baiting"
+    And I follow "Email petitioners"
+    Then I should be on the admin email petitioners form page for "Ban Badger Baiting"
+    And the markup should be valid
+    When I press "Email 6 signatures"
+    Then the petition should not have any emails
+    And I should see an error
+    When I fill in the email details
+    And press "Email 6 signatures"
+    Then the petition should have the email details I provided
+    And the petition creator should have been emailed with the update
+    And all the signatories of the petition should have been emailed with the update
+    And the feedback email address should have been emailed a copy
diff --git a/features/step_definitions/petition_email_steps.rb b/features/step_definitions/petition_email_steps.rb
new file mode 100644
index 000000000..459ed5ace
--- /dev/null
+++ b/features/step_definitions/petition_email_steps.rb
@@ -0,0 +1,52 @@
+When(/^I fill in the email details$/) do
+  fill_in "Subject", :with => "Petition email subject"
+  fill_in "Body", :with => "Petition email body"
+end
+
+Then(/^the petition should not have any emails$/) do
+  @petition.reload
+  expect(@petition.emails).to be_empty
+end
+
+Then(/^the petition should have the email details I provided$/) do
+  @petition.reload
+  @email = @petition.emails.last
+  expect(@email.subject).to eq("Petition email subject")
+  expect(@email.body).to match(%r[Petition email body])
+  expect(@email.sent_by).to eq("Admin User")
+end
+
+Then(/^the petition creator should have been emailed with the update$/) do
+  @petition.reload
+  steps %Q(
+    Then "#{@petition.creator_signature.email}" should receive an email
+    When they open the email
+    Then they should see "Petition email body" in the email body
+    When they follow "#{petition_url(@petition)}" in the email
+    Then I should be on the petition page for "#{@petition.action}"
+  )
+end
+
+Then(/^all the signatories of the petition should have been emailed with the update$/) do
+  @petition.reload
+  @petition.signatures.notify_by_email.validated.where.not(id: @petition.creator_signature.id).each do |signatory|
+    steps %Q(
+      Then "#{signatory.email}" should receive an email
+      When they open the email
+      Then they should see "Petition email body" in the email body
+      When they follow "#{petition_url(@petition)}" in the email
+      Then I should be on the petition page for "#{@petition.action}"
+    )
+  end
+end
+
+Then(/^the feedback email address should have been emailed a copy$/) do
+  signatory = FeedbackSignature.new(@petition)
+  steps %Q(
+    Then "#{signatory.email}" should receive an email
+    When they open the email
+    Then they should see "Petition email body" in the email body
+    When they follow "#{petition_url(@petition)}" in the email
+    Then I should be on the petition page for "#{@petition.action}"
+  )
+end
diff --git a/features/support/paths.rb b/features/support/paths.rb
index 96050232e..89f474ce1 100644
--- a/features/support/paths.rb
+++ b/features/support/paths.rb
@@ -92,6 +92,9 @@ def admin_url(admin_page)
     when /^debate outcomes form page for "([^\"]*)"$/
       admin_petition_debate_outcome_url(Petition.find_by(action: $1))
 
+    when /^email petitioners form page for "([^\"]*)"$/
+      new_admin_petition_email_url(Petition.find_by(action: $1))
+
     when /^government response page for "([^\"]*)"$/
       admin_petition_government_response_url(Petition.find_by(action: $1))
 
diff --git a/spec/controllers/admin/petition_emails_controller_spec.rb b/spec/controllers/admin/petition_emails_controller_spec.rb
new file mode 100644
index 000000000..b5a9530dd
--- /dev/null
+++ b/spec/controllers/admin/petition_emails_controller_spec.rb
@@ -0,0 +1,249 @@
+require 'rails_helper'
+
+RSpec.describe Admin::PetitionEmailsController, type: :controller, admin: true do
+
+  let!(:petition) { FactoryGirl.create(:open_petition) }
+
+  describe 'not logged in' do
+    describe 'GET /new' do
+      it 'redirects to the login page' do
+        get :new, petition_id: petition.id
+        expect(response).to redirect_to('https://moderate.petition.parliament.uk/admin/login')
+      end
+    end
+
+    describe 'POST /' do
+      it 'redirects to the login page' do
+        patch :create, petition_id: petition.id
+        expect(response).to redirect_to('https://moderate.petition.parliament.uk/admin/login')
+      end
+    end
+  end
+
+  context 'logged in as moderator user but need to reset password' do
+    let(:user) { FactoryGirl.create(:moderator_user, force_password_reset: true) }
+    before { login_as(user) }
+
+    describe 'GET /new' do
+      it 'redirects to edit profile page' do
+        get :new, petition_id: petition.id
+        expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit")
+      end
+    end
+
+    describe 'POST /' do
+      it 'redirects to edit profile page' do
+        patch :create, petition_id: petition.id
+        expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit")
+      end
+    end
+  end
+
+  describe "logged in as moderator user" do
+    let(:user) { FactoryGirl.create(:moderator_user) }
+    before { login_as(user) }
+
+    describe 'GET /new' do
+      describe 'for an open petition' do
+        it 'fetches the requested petition' do
+          get :new, petition_id: petition.id
+          expect(assigns(:petition)).to eq petition
+        end
+
+        it 'responds successfully and renders the petitions/show template' do
+          get :new, petition_id: petition.id
+          expect(response).to be_success
+          expect(response).to render_template('petitions/show')
+        end
+      end
+
+      shared_examples_for 'trying to view the email petitioners form of a petition in the wrong state' do
+        it 'raises a 404 error' do
+          expect {
+            get :new, petition_id: petition.id
+          }.to raise_error ActiveRecord::RecordNotFound
+        end
+      end
+
+      describe 'for a pending petition' do
+        before { petition.update_column(:state, Petition::PENDING_STATE) }
+        it_behaves_like 'trying to view the email petitioners form of a petition in the wrong state'
+      end
+
+      describe 'for a validated petition' do
+        before { petition.update_column(:state, Petition::VALIDATED_STATE) }
+        it_behaves_like 'trying to view the email petitioners form of a petition in the wrong state'
+      end
+
+      describe 'for a sponsored petition' do
+        before { petition.update_column(:state, Petition::SPONSORED_STATE) }
+        it_behaves_like 'trying to view the email petitioners form of a petition in the wrong state'
+      end
+    end
+
+    describe 'POST /' do
+      let(:petition_email_attributes) do
+        {
+          subject: "Petition email subject",
+          body: "Petition email body"
+        }
+      end
+
+      def do_post(overrides = {})
+        params = { petition_id: petition.id, petition_email: petition_email_attributes }
+        post :create, params.merge(overrides)
+      end
+
+      describe 'for an open petition' do
+        it 'fetches the requested petition' do
+          do_post
+          expect(assigns(:petition)).to eq petition
+        end
+
+        describe 'with valid params' do
+          it 'redirects to the petition show page' do
+            do_post
+            expect(response).to redirect_to "https://moderate.petition.parliament.uk/admin/petitions/#{petition.id}"
+          end
+
+          it 'tells the moderator that their email will be sent overnight' do
+            do_post
+            expect(flash[:notice]).to eq 'Email will be sent overnight'
+          end
+
+          it 'stores the supplied email details in the db' do
+            do_post
+            petition.reload
+            email = petition.emails.last
+            expect(email).to be_present
+            expect(email.subject).to eq "Petition email subject"
+            expect(email.body).to eq "Petition email body"
+            expect(email.sent_by).to eq user.pretty_name
+          end
+
+          context "emails out the petition email" do
+            include ActiveJob::TestHelper
+
+            before do
+              3.times do |i|
+                attributes = {
+                  name: "Laura #{i}",
+                  email: "laura_#{i}@example.com",
+                  notify_by_email: true,
+                  petition: petition
+                }
+                s = FactoryGirl.create(:pending_signature, attributes)
+                s.validate!
+              end
+              2.times do |i|
+                attributes = {
+                  name: "Sarah #{i}",
+                  email: "sarah_#{i}@example.com",
+                  notify_by_email: false,
+                  petition: petition
+                }
+
+                s = FactoryGirl.create(:pending_signature, attributes)
+                s.validate!
+              end
+              2.times do |i|
+                attributes = {
+                  name: "Brian #{i}",
+                  email: "brian_#{i}@example.com",
+                  notify_by_email: true,
+                  petition: petition
+                }
+                FactoryGirl.create(:pending_signature, attributes)
+              end
+              petition.reload
+            end
+
+            it "queues a job to process the emails" do
+              assert_enqueued_jobs 1 do
+                do_post
+              end
+            end
+
+            it "stamps the 'petition_email' email sent receipt on each signature when the job runs" do
+              perform_enqueued_jobs do
+                do_post
+                petition.reload
+                petition_timestamp = petition.get_email_requested_at_for('petition_email')
+                expect(petition_timestamp).not_to be_nil
+                petition.signatures.validated.notify_by_email.each do |signature|
+                  expect(signature.get_email_sent_at_for('petition_email')).to eq(petition_timestamp)
+                end
+              end
+            end
+
+            it "should email out to the validated signees who have opted in when the delayed job runs" do
+              ActionMailer::Base.deliveries.clear
+              perform_enqueued_jobs do
+                do_post
+                expect(ActionMailer::Base.deliveries.length).to eq 5
+                expect(ActionMailer::Base.deliveries.map(&:to)).to eq([
+                  [petition.creator_signature.email],
+                  ['laura_0@example.com'],
+                  ['laura_1@example.com'],
+                  ['laura_2@example.com'],
+                  ['petitionscommittee@parliament.uk']
+                ])
+              end
+            end
+          end
+        end
+
+        describe 'with invalid params' do
+          before { petition_email_attributes.delete(:subject) }
+
+          it 're-renders the petitions/show template' do
+            do_post
+            expect(response).to be_success
+            expect(response).to render_template('petitions/show')
+          end
+
+          it 'leaves the in-memory instance with errors' do
+            do_post
+            expect(assigns(:email)).to be_present
+            expect(assigns(:email).errors).not_to be_empty
+          end
+
+          it 'does not stores the email details in the db' do
+            do_post
+            petition.reload
+            expect(petition.emails).to be_empty
+          end
+        end
+      end
+
+      shared_examples_for 'trying to email supporters of a petition in the wrong state' do
+        it 'raises a 404 error' do
+          expect {
+            do_post
+          }.to raise_error ActiveRecord::RecordNotFound
+        end
+
+        it 'does not stores the supplied email details in the db' do
+          suppress(ActiveRecord::RecordNotFound) { do_post }
+          petition.reload
+          expect(petition.emails).to be_empty
+        end
+      end
+
+      describe 'for a pending petition' do
+        before { petition.update_column(:state, Petition::PENDING_STATE) }
+        it_behaves_like 'trying to email supporters of a petition in the wrong state'
+      end
+
+      describe 'for a validated petition' do
+        before { petition.update_column(:state, Petition::VALIDATED_STATE) }
+        it_behaves_like 'trying to email supporters of a petition in the wrong state'
+      end
+
+      describe 'for a sponsored petition' do
+        before { petition.update_column(:state, Petition::SPONSORED_STATE) }
+        it_behaves_like 'trying to email supporters of a petition in the wrong state'
+      end
+    end
+  end
+end