From 5441fc56e66b6ffebfa38f7719f6097dc0dc71c0 Mon Sep 17 00:00:00 2001 From: ansonK Date: Mon, 27 Jul 2015 16:25:27 +0100 Subject: [PATCH 1/7] Refactor the jobs that send email. Break each email response type into 2 jobs * a parent job that will load the recipients and enqueue a child job for each * a child job that will perform a single email delivery Doing this removes the memory leak seen in previous version when delivering emails, and gives greater control over how much work is left to do as each job now does 1 thing and will eliminate the need for long running mail delivery jobs Both the parent and child jobs will check that the petition they are delivering information for and will exit if the petition has been updated since the jobs were enqueued. --- app/jobs/email_debate_outcomes_job.rb | 11 +- app/jobs/email_debate_scheduled_job.rb | 11 +- app/jobs/email_delivery_jobs/base.rb | 69 +++++++ .../email_delivery_jobs/debate_outcome.rb | 9 + .../email_delivery_jobs/debate_scheduled.rb | 9 + .../email_delivery_jobs/threshold_response.rb | 9 + app/jobs/email_petition_signatories.rb | 92 ---------- app/jobs/email_petition_signatories_job.rb | 80 +++++++++ app/lib/job_logger.rb | 37 ++++ ...tition_action_government_response.html.erb | 2 +- config/application.rb | 3 + config/initializers/job_logger.rb | 5 + spec/jobs/email_debate_outcomes_job_spec.rb | 26 +-- spec/jobs/email_debate_scheduled_job_spec.rb | 26 +-- .../debate_outcome_spec.rb | 25 +++ .../debate_scheduled_spec.rb | 25 +++ .../threshold_response_spec.rb | 25 +++ spec/jobs/email_petition_signatories_spec.rb | 170 +++--------------- .../jobs/email_threshold_response_job_spec.rb | 25 +-- spec/jobs/shared_examples.rb | 130 ++++++++++++++ spec/support/active_job.rb | 5 + 21 files changed, 472 insertions(+), 322 deletions(-) create mode 100644 app/jobs/email_delivery_jobs/base.rb create mode 100644 app/jobs/email_delivery_jobs/debate_outcome.rb create mode 100644 app/jobs/email_delivery_jobs/debate_scheduled.rb create mode 100644 app/jobs/email_delivery_jobs/threshold_response.rb delete mode 100644 app/jobs/email_petition_signatories.rb create mode 100644 app/jobs/email_petition_signatories_job.rb create mode 100644 app/lib/job_logger.rb create mode 100644 config/initializers/job_logger.rb create mode 100644 spec/jobs/email_delivery_jobs/debate_outcome_spec.rb create mode 100644 spec/jobs/email_delivery_jobs/debate_scheduled_spec.rb create mode 100644 spec/jobs/email_delivery_jobs/threshold_response_spec.rb create mode 100644 spec/jobs/shared_examples.rb diff --git a/app/jobs/email_debate_outcomes_job.rb b/app/jobs/email_debate_outcomes_job.rb index d8ab07bdf..689fcd652 100644 --- a/app/jobs/email_debate_outcomes_job.rb +++ b/app/jobs/email_debate_outcomes_job.rb @@ -1,19 +1,14 @@ -class EmailDebateOutcomesJob < EmailPetitionSignatories::Job +class EmailDebateOutcomesJob < EmailPetitionSignatoriesJob def self.run_later_tonight(petition) petition.set_email_requested_at_for('debate_outcome', to: Time.current) super(petition, petition.get_email_requested_at_for('debate_outcome')) end - def perform(petition, requested_at_string, mailer = PetitionMailer.name, logger = nil) - @mailer = mailer.constantize - worker(petition, requested_at_string, logger).do_work! + def email_delivery_job_class + EmailDeliveryJobs::DebateOutcome end def timestamp_name 'debate_outcome' end - - def create_email(petition, signature) - @mailer.notify_signer_of_debate_outcome(petition, signature) - end end diff --git a/app/jobs/email_debate_scheduled_job.rb b/app/jobs/email_debate_scheduled_job.rb index 94342a94d..fda6c9461 100644 --- a/app/jobs/email_debate_scheduled_job.rb +++ b/app/jobs/email_debate_scheduled_job.rb @@ -1,19 +1,14 @@ -class EmailDebateScheduledJob < EmailPetitionSignatories::Job +class EmailDebateScheduledJob < EmailPetitionSignatoriesJob def self.run_later_tonight(petition) petition.set_email_requested_at_for('debate_scheduled', to: Time.current) super(petition, petition.get_email_requested_at_for('debate_scheduled')) end - def perform(petition, requested_at_string, mailer = PetitionMailer.name, threshold_logger = nil) - @mailer = mailer.constantize - worker(petition, requested_at_string, threshold_logger).do_work! + def email_delivery_job_class + EmailDeliveryJobs::DebateScheduled end def timestamp_name 'debate_scheduled' end - - def create_email(petition, signature) - @mailer.notify_signer_of_debate_scheduled(petition, signature) - end end diff --git a/app/jobs/email_delivery_jobs/base.rb b/app/jobs/email_delivery_jobs/base.rb new file mode 100644 index 000000000..93d3bceef --- /dev/null +++ b/app/jobs/email_delivery_jobs/base.rb @@ -0,0 +1,69 @@ +module EmailDeliveryJobs + class Base < ActiveJob::Base + + # + # 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 + # + + queue_as :default + + def perform(signature:, timestamp_name:, petition:, + requested_at_as_string:, mailer: PetitionMailer.name, logger: nil) + + @mailer = mailer.constantize + @signature = signature + @petition = petition + @requested_at = requested_at_as_string.in_time_zone + @timestamp_name = timestamp_name + @logger = logger + + if petition_has_not_been_updated? + send_email + record_email_sent + end + end + + private + + attr_reader :mailer, :signature, :timestamp_name, :petition, :requested_at + + def send_email + create_email.deliver_now + + 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 + end + + def create_email + raise NotImplementedError.new "Extending classes must implement create_email" + end + + def record_email_sent + signature.set_email_sent_at_for timestamp_name, to: petition_timestamp + end + + def petition_timestamp + petition.get_email_requested_at_for(timestamp_name) + end + + # 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.to_i == requested_at.to_i + end + end +end diff --git a/app/jobs/email_delivery_jobs/debate_outcome.rb b/app/jobs/email_delivery_jobs/debate_outcome.rb new file mode 100644 index 000000000..956282f24 --- /dev/null +++ b/app/jobs/email_delivery_jobs/debate_outcome.rb @@ -0,0 +1,9 @@ +module EmailDeliveryJobs + class DebateOutcome < Base + + def create_email + mailer.notify_signer_of_debate_outcome signature.petition, signature + end + + end +end diff --git a/app/jobs/email_delivery_jobs/debate_scheduled.rb b/app/jobs/email_delivery_jobs/debate_scheduled.rb new file mode 100644 index 000000000..9781e4019 --- /dev/null +++ b/app/jobs/email_delivery_jobs/debate_scheduled.rb @@ -0,0 +1,9 @@ +module EmailDeliveryJobs + class DebateScheduled < Base + + def create_email + mailer.notify_signer_of_debate_scheduled signature.petition, signature + end + + end +end diff --git a/app/jobs/email_delivery_jobs/threshold_response.rb b/app/jobs/email_delivery_jobs/threshold_response.rb new file mode 100644 index 000000000..9e1fa7a97 --- /dev/null +++ b/app/jobs/email_delivery_jobs/threshold_response.rb @@ -0,0 +1,9 @@ +module EmailDeliveryJobs + class ThresholdResponse < Base + + def create_email + mailer.notify_signer_of_threshold_response signature.petition, signature + end + + end +end diff --git a/app/jobs/email_petition_signatories.rb b/app/jobs/email_petition_signatories.rb deleted file mode 100644 index deef3d307..000000000 --- a/app/jobs/email_petition_signatories.rb +++ /dev/null @@ -1,92 +0,0 @@ -class EmailPetitionSignatories - class PleaseRetry < StandardError; end - - class Job < ActiveJob::Base - queue_as :default - - def self.run_later_tonight(petition, requested_at, *extra_args) - set(wait_until: later_tonight). - perform_later(petition, requested_at.getutc.iso8601, *extra_args) - end - - def self.later_tonight - 1.day.from_now.at_midnight + rand(240).minutes + rand(60).seconds - end - private_class_method :later_tonight - - def worker(petition, requested_at_string, logger) - Worker.new(self, petition, requested_at_string, logger) - end - end - - class Worker - def initialize(job, petition, requested_at_string, logger = nil) - @job = job - @petition = petition - @requested_at = requested_at_string.in_time_zone - @logger = logger || construct_logger - end - - def do_work! - return unless newest_request? - - logger.info("Starting job for petition '#{petition.action}' with email requested at: #{petition_timestamp}") - email_signees - logger.info("Finished job for petition '#{petition.action}'") - - assert_all_signees_notified - end - - private - - attr_reader :job, :petition, :requested_at, :logger - - delegate :timestamp_name, :create_email, to: :job - - def petition_timestamp - petition.get_email_requested_at_for(timestamp_name) - end - - def signatures_to_email - petition.signatures_to_email_for(timestamp_name) - end - - def send_email_to(signature) - create_email(petition, signature).deliver_later - signature.set_email_sent_at_for(timestamp_name, to: petition_timestamp) - 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 newest_request? - # NOTE: to_i comparison is used to cater for precision differences - # between DB timestamp (petition_timestamp precise to fractional - # seconds) and job timestamp (requested_at - only seconds precise) - petition_timestamp.to_i == requested_at.to_i - end - - def email_signees - signatures_to_email.find_each do |signature| - begin - send_email_to(signature) - rescue Errno::ECONNREFUSED, Errno::ETIMEDOUT, Net::SMTPError => e - # try this one again later - logger.info("#{e.class.name} while sending email for #{job.class.name} to: #{signature.email}") - end - end - end - - def assert_all_signees_notified - return if signatures_to_email.count == 0 - - logger.info("Raising error to force a retry of email send of '#{petition.action}'") - raise PleaseRetry - end - - def construct_logger - logfilename = "#{job.class.name}_for_petition_id_#{petition.id}.log" - AuditLogger.new(Rails.root.join('log', logfilename), "Email #{job.class.name} error") - end - end -end diff --git a/app/jobs/email_petition_signatories_job.rb b/app/jobs/email_petition_signatories_job.rb new file mode 100644 index 000000000..e18674895 --- /dev/null +++ b/app/jobs/email_petition_signatories_job.rb @@ -0,0 +1,80 @@ +class EmailPetitionSignatoriesJob < ActiveJob::Base + queue_as :default + + def self.run_later_tonight(petition, requested_at, *extra_args) + set(wait_until: later_tonight). + perform_later(petition, requested_at.getutc.iso8601, *extra_args) + end + + def self.later_tonight + 1.day.from_now.at_midnight + rand(240).minutes + rand(60).seconds + end + private_class_method :later_tonight + + + def perform(petition, requested_at_string) + @petition = petition + @requested_at = requested_at_string.in_time_zone + do_work! + 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}'") + + # TODO: removed check_all_assignees recieved email + # as email is not sent from here. + # Do we need to check that at all here? + 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 + ) + end + 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? + # NOTE: to_i comparison is used to cater for precision differences + # between DB timestamp (petition_timestamp precise to fractional + # seconds) and job timestamp (requested_at - only seconds precise) + petition_timestamp.to_i != requested_at.to_i + end + + def petition_timestamp + petition.get_email_requested_at_for(timestamp_name) + end + + 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 "Extending classes must implement email_delivery_job_class" + end + + def timestamp_name + raise NotImplementedError.new "Extending classes must implement timestamp_name" + end +end + + diff --git a/app/lib/job_logger.rb b/app/lib/job_logger.rb new file mode 100644 index 000000000..e3c59b6ce --- /dev/null +++ b/app/lib/job_logger.rb @@ -0,0 +1,37 @@ +class JobLogger + + # + # Custom logger for ActiveJob classes that adds the Job class name into the log event + # to make filtering / tracking easier + # + + def initialize(job_class_name, logger = nil) + @job_class_name = job_class_name + @logger = logger + end + + def debug(msg) + logger.debug message: msg, job_class: job_class_name + end + + def info(msg) + logger.info message: msg, job_class: job_class_name + end + + def warn(msg) + logger.warn message: msg, job_class: job_class_name + end + + def error(msg) + logger.error message: msg, job_class: job_class_name + end + + private + + attr_reader :job_class_name + + def logger + @logger ||= Rails.logger + end + +end 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 4fd7a8667..f85ea1339 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-max-length': 200, class: 'form-control' %> + <%= f.text_area :summary, rows: 3, cols: 70, tabindex: increment, data: { maxlength: 200 }, class: 'form-control' %>

200 characters max

<% end %> diff --git a/config/application.rb b/config/application.rb index 2203ea18c..7ec8b14a2 100644 --- a/config/application.rb +++ b/config/application.rb @@ -44,5 +44,8 @@ class Application < Rails::Application ) config.action_dispatch.default_headers.merge!('X-UA-Compatible' => 'IE=edge') + + # Load the job concerns + config.eager_load_paths += %W(#{config.root}/app/jobs/**/*) end end diff --git a/config/initializers/job_logger.rb b/config/initializers/job_logger.rb new file mode 100644 index 000000000..c742d2407 --- /dev/null +++ b/config/initializers/job_logger.rb @@ -0,0 +1,5 @@ +class ActiveJob::Base + def logger + @logger || JobLogger.new(self.class.name) + end +end diff --git a/spec/jobs/email_debate_outcomes_job_spec.rb b/spec/jobs/email_debate_outcomes_job_spec.rb index 011ddede3..686b49af8 100644 --- a/spec/jobs/email_debate_outcomes_job_spec.rb +++ b/spec/jobs/email_debate_outcomes_job_spec.rb @@ -1,37 +1,15 @@ require 'rails_helper' +require_relative 'shared_examples' RSpec.describe EmailDebateOutcomesJob, type: :job do let(:email_requested_at) { Time.current } let(:petition) { FactoryGirl.create(:open_petition) } let(:signature) { FactoryGirl.create(:validated_signature, :petition => petition) } - let(:mailer) { double.as_null_object } - let(:logger) { double.as_null_object } - before do petition.set_email_requested_at_for('debate_outcome', to: email_requested_at) allow(petition).to receive_message_chain(:signatures_to_email_for, :find_each).and_yield(signature) - allow(petition).to receive_message_chain(:signatures_to_email_for, :count => 0) - end - - def perform_job(requested_at = email_requested_at) - described_class.perform_now(petition, requested_at.getutc.iso8601, mailer, logger) - end - - it "sends the notify_signer_of_debate_outcome emails to each signatory of a petition" do - expect(mailer).to receive(:notify_signer_of_debate_outcome).with(petition, signature).and_return(mailer) - perform_job - end - - it "marks the signature with the 'debate_outcome' last emailing time" do - expect(signature).to receive(:set_email_sent_at_for).with('debate_outcome', to: email_requested_at) - perform_job end - it 'uses a EmailPetitionSignatories::Worker to do the work' do - worker = double - expect(worker).to receive(:do_work!) - expect(EmailPetitionSignatories::Worker).to receive(:new).with(instance_of(described_class), petition, email_requested_at.getutc.iso8601, anything).and_return worker - perform_job - end + it_behaves_like "job to enqueue signatory mailing jobs" end diff --git a/spec/jobs/email_debate_scheduled_job_spec.rb b/spec/jobs/email_debate_scheduled_job_spec.rb index 2c593bdc3..525227394 100644 --- a/spec/jobs/email_debate_scheduled_job_spec.rb +++ b/spec/jobs/email_debate_scheduled_job_spec.rb @@ -1,37 +1,15 @@ require 'rails_helper' +require_relative 'shared_examples' RSpec.describe EmailDebateScheduledJob, type: :job do let(:email_requested_at) { Time.current } let(:petition) { FactoryGirl.create(:open_petition) } let(:signature) { FactoryGirl.create(:validated_signature, :petition => petition) } - let(:mailer) { double.as_null_object } - let(:logger) { double.as_null_object } - before do petition.set_email_requested_at_for('debate_scheduled', to: email_requested_at) allow(petition).to receive_message_chain(:signatures_to_email_for, :find_each).and_yield(signature) - allow(petition).to receive_message_chain(:signatures_to_email_for, :count => 0) - end - - def perform_job(requested_at = email_requested_at) - described_class.perform_now(petition, requested_at.getutc.iso8601, mailer, logger) - end - - it "sends the notify_signer_of_debate_scheduled emails to each signatory of a petition" do - expect(mailer).to receive(:notify_signer_of_debate_scheduled).with(petition, signature).and_return(mailer) - perform_job - end - - it "marks the signature with the 'debate_scheduled' last emailing time" do - expect(signature).to receive(:set_email_sent_at_for).with('debate_scheduled', to: email_requested_at) - perform_job end - it 'uses a EmailPetitionSignatories::Worker to do the work' do - worker = double - expect(worker).to receive(:do_work!) - expect(EmailPetitionSignatories::Worker).to receive(:new).with(instance_of(described_class), petition, email_requested_at.getutc.iso8601, anything).and_return worker - perform_job - end + it_behaves_like "job to enqueue signatory mailing jobs" end diff --git a/spec/jobs/email_delivery_jobs/debate_outcome_spec.rb b/spec/jobs/email_delivery_jobs/debate_outcome_spec.rb new file mode 100644 index 000000000..69713761c --- /dev/null +++ b/spec/jobs/email_delivery_jobs/debate_outcome_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' +require_relative '../shared_examples' + +RSpec.describe EmailDeliveryJobs::DebateOutcome, type: :job do + let(:email_requested_at) { Time.current } + let(:petition) { FactoryGirl.create(:debated_petition) } + let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) } + let(:timestamp_name) { 'debate_outcome' } + + before do + petition.set_email_requested_at_for(timestamp_name, to: email_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_as_string: email_requested_at.getutc.iso8601 + ) + end +end diff --git a/spec/jobs/email_delivery_jobs/debate_scheduled_spec.rb b/spec/jobs/email_delivery_jobs/debate_scheduled_spec.rb new file mode 100644 index 000000000..951166ee0 --- /dev/null +++ b/spec/jobs/email_delivery_jobs/debate_scheduled_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' +require_relative '../shared_examples' + +RSpec.describe EmailDeliveryJobs::DebateScheduled, type: :job do + let(:email_requested_at) { Time.current } + let(:petition) { FactoryGirl.create(:awaiting_debate_petition) } + let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) } + let(:timestamp_name) { 'debate_scheduled' } + + before do + petition.set_email_requested_at_for(timestamp_name, to: email_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_as_string: email_requested_at.getutc.iso8601 + ) + end +end diff --git a/spec/jobs/email_delivery_jobs/threshold_response_spec.rb b/spec/jobs/email_delivery_jobs/threshold_response_spec.rb new file mode 100644 index 000000000..ead2a1ac0 --- /dev/null +++ b/spec/jobs/email_delivery_jobs/threshold_response_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' +require_relative '../shared_examples' + +RSpec.describe EmailDeliveryJobs::ThresholdResponse, type: :job do + let(:email_requested_at) { Time.current } + let(:petition) { FactoryGirl.create(:responded_petition) } + let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) } + let(:timestamp_name) { 'government_response' } + + before do + petition.set_email_requested_at_for(timestamp_name, to: email_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_as_string: email_requested_at.getutc.iso8601 + ) + end +end diff --git a/spec/jobs/email_petition_signatories_spec.rb b/spec/jobs/email_petition_signatories_spec.rb index a595fa3ef..9475416fc 100644 --- a/spec/jobs/email_petition_signatories_spec.rb +++ b/spec/jobs/email_petition_signatories_spec.rb @@ -1,158 +1,44 @@ -require 'net/smtp' require 'rails_helper' -RSpec.describe EmailPetitionSignatories, type: :job do - describe EmailPetitionSignatories::Job do - context '.run_later_tonight' do - let(:petition) { FactoryGirl.create(:open_petition) } - let(:requested_at) { Time.current } - before do - ActiveJob::Base.queue_adapter.enqueued_jobs = [] - ActiveJob::Base.queue_adapter.performed_jobs = [] - end - - def global_id_job_arg_for(object) - { "_aj_globalid" => object.to_global_id.to_s } - end - def timestamp_job_arg_for(timestamp) - timestamp.getutc.iso8601 - end - - it 'queues up a job' do - described_class.run_later_tonight(petition, requested_at) - expect(enqueued_jobs.size).to eq 1 - expect(enqueued_jobs.first[:job]).to eq described_class - end - - it 'sets the job to run between midnight and 4am tomorrow' do - described_class.run_later_tonight(petition, requested_at) - 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 } - end - - it 'queues up the job to run with the petition and timestamp supplied as args' do - described_class.run_later_tonight(petition, requested_at) - 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 - - it 'adds any extra params provided as job args after the petition and timestamp' do - described_class.run_later_tonight(petition, requested_at, 'cheese', 1, petition.creator_signature) - queued_args = enqueued_jobs.first[:args] - expect(queued_args[2]).to eq 'cheese' - expect(queued_args[3]).to eq 1 - expect(queued_args[4]).to eq global_id_job_arg_for(petition.creator_signature) - end - end - end - - describe EmailPetitionSignatories::Worker do - let(:email_requested_at) { Time.current } +RSpec.describe EmailPetitionSignatoriesJob, type: :job do + describe '.run_later_tonight' do let(:petition) { FactoryGirl.create(:open_petition) } - let!(:signature) { FactoryGirl.create(:validated_signature, :petition => petition) } - - let(:timestamp_name) { 'government_response' } - let(:job) { double(timestamp_name: timestamp_name).as_null_object } - let(:logger) { double.as_null_object } - let(:email) { double.as_null_object } + let(:requested_at) { Time.current } - before { petition.set_email_requested_at_for(timestamp_name, to: email_requested_at) } - - def do_work(requested_at = email_requested_at) - requested_at = requested_at.getutc.iso8601 - described_class.new(job, petition, requested_at, logger).do_work! + def global_id_job_arg_for(object) + { "_aj_globalid" => object.to_global_id.to_s } end - it "asks the job to send an email to each signatory of a petition" do - expect(job).to receive(:create_email).with(petition, signature).and_return email - do_work + def timestamp_job_arg_for(timestamp) + timestamp.getutc.iso8601 end - it "doesn't run unless it's the latest request according to the jobs timestamp name" do - expect(job).not_to receive(:create_email) - requested_at = email_requested_at - 1000 - do_work(requested_at) + it 'queues up a job' do + described_class.run_later_tonight(petition, requested_at) + expect(enqueued_jobs.size).to eq 1 + expect(enqueued_jobs.first[:job]).to eq described_class end - it "marks the signature using the timestamp name and the time the job was requested " do - do_work - signature.reload - expect(signature.get_email_sent_at_for(timestamp_name)).to be_usec_precise_with email_requested_at + it 'sets the job to run between midnight and 4am tomorrow' do + described_class.run_later_tonight(petition, requested_at) + 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 } end - context "email sending fails" do - shared_examples_for 'catching errors during individual email sending' do - before do - allow(job).to receive(:create_email).and_return email - allow(job).to receive(:create_email).with(petition, signature).and_raise(exception_class) - end - - it "captures the error and raises a retry error" do - expect { do_work }.to raise_error(EmailPetitionSignatories::PleaseRetry) - end - - it 'logs the email sending error as information' do - expect(logger).to receive(:info).with(/#{Regexp.escape(exception_class.name)}/) - suppress(EmailPetitionSignatories::PleaseRetry) { do_work } - end - - it "does not mark the signature" do - suppress(EmailPetitionSignatories::PleaseRetry) { do_work } - signature.reload - expect(signature.get_email_sent_at_for(timestamp_name)).not_to be_usec_precise_with email_requested_at - end - - it 'continues to process other signatures after the one that errored' do - other_signature = FactoryGirl.create(:validated_signature, :petition => petition) - suppress(EmailPetitionSignatories::PleaseRetry) { do_work } - other_signature.reload - expect(other_signature.get_email_sent_at_for(timestamp_name)).to be_usec_precise_with email_requested_at - end - end - - context "with connection refused" do - let(:exception_class) { Errno::ECONNREFUSED } - - it_behaves_like 'catching errors during individual email sending' - end - - context "with an SMTPError" do - let(:exception_class) { Net::SMTPFatalError } - - it_behaves_like 'catching errors during individual email sending' - end - - context 'with a timeout' do - let(:exception_class) { Errno::ETIMEDOUT } - - it_behaves_like 'catching errors during individual email sending' - end - - context "for some other reason" do - before do - allow(job).to receive(:create_email).and_return email - allow(job).to receive(:create_email).with(petition, signature).and_raise(ActiveRecord::RecordNotSaved, 'uh oh!') - end - - it "raises the error" do - expect { do_work }.to raise_error(ActiveRecord::RecordNotSaved) - end - - it "does not mark the signature" do - suppress(ActiveRecord::RecordNotSaved) { do_work } - signature.reload - expect(signature.get_email_sent_at_for(timestamp_name)).not_to be_usec_precise_with email_requested_at - end + it 'queues up the job to run with the petition and timestamp supplied as args' do + described_class.run_later_tonight(petition, requested_at) + 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 - it 'does not process other signatures after the one that errored' do - other_signature = FactoryGirl.create(:validated_signature, :petition => petition) - suppress(ActiveRecord::RecordNotSaved) { do_work } - other_signature.reload - expect(other_signature.get_email_sent_at_for(timestamp_name)).not_to be_usec_precise_with email_requested_at - end - end + it 'adds any extra params provided as job args after the petition and timestamp' do + described_class.run_later_tonight(petition, requested_at, 'cheese', 1, petition.creator_signature) + queued_args = enqueued_jobs.first[:args] + expect(queued_args[2]).to eq 'cheese' + expect(queued_args[3]).to eq 1 + expect(queued_args[4]).to eq global_id_job_arg_for(petition.creator_signature) end end end diff --git a/spec/jobs/email_threshold_response_job_spec.rb b/spec/jobs/email_threshold_response_job_spec.rb index 85aa47dea..b1f432d20 100644 --- a/spec/jobs/email_threshold_response_job_spec.rb +++ b/spec/jobs/email_threshold_response_job_spec.rb @@ -1,37 +1,16 @@ require 'rails_helper' +require_relative 'shared_examples' RSpec.describe EmailThresholdResponseJob, type: :job do let(:email_requested_at) { Time.current } let(:petition) { FactoryGirl.create(:open_petition) } let(:signature) { FactoryGirl.create(:validated_signature, :petition => petition) } - let(:mailer) { double.as_null_object } - let(:logger) { double.as_null_object } - before do petition.set_email_requested_at_for('government_response', to: email_requested_at) allow(petition).to receive_message_chain(:signatures_to_email_for, :find_each).and_yield(signature) - allow(petition).to receive_message_chain(:signatures_to_email_for, :count => 0) - end - - def perform_job(requested_at = email_requested_at) - described_class.perform_now(petition, requested_at.getutc.iso8601, mailer, logger) end - it "sends the notify_signer_of_threshold_response emails to each signatory of a petition" do - expect(mailer).to receive(:notify_signer_of_threshold_response).with(petition, signature).and_return(mailer) - perform_job - end + it_behaves_like "job to enqueue signatory mailing jobs" - it "marks the signature with the 'government_response' last emailing time" do - expect(signature).to receive(:set_email_sent_at_for).with('government_response', to: email_requested_at) - perform_job - end - - it 'uses a EmailPetitionSignatories::Worker to do the work' do - worker = double - expect(worker).to receive(:do_work!) - expect(EmailPetitionSignatories::Worker).to receive(:new).with(instance_of(described_class), petition, email_requested_at.getutc.iso8601, anything).and_return worker - perform_job - end end diff --git a/spec/jobs/shared_examples.rb b/spec/jobs/shared_examples.rb new file mode 100644 index 000000000..61cf17f63 --- /dev/null +++ b/spec/jobs/shared_examples.rb @@ -0,0 +1,130 @@ +RSpec.shared_examples_for "job to enqueue signatory mailing jobs" do + def do_work(requested_at = email_requested_at) + @requested_at = requested_at.getutc.iso8601 + subject.perform(petition, requested_at) + end + + context "when the petition has not been updated" do + it "enqueues a job to send an email to each signatory" do + do_work + expect(enqueued_jobs.size).to eq(1) + end + + it "the job is the correct type for the type of notification" do + do_work + job = enqueued_jobs.first + expect(job[:job]).to eq(subject.email_delivery_job_class) + end + + it "the job has the expected arguments" do + do_work + 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) + 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 ) + end + + it "does not enqueue any jobs to send emails" do + do_work + expect(enqueued_jobs).to be_empty + end + end +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 + subject.perform( + signature: signature, + timestamp_name: timestamp_name, + petition: petition, + requested_at_as_string: @requested_at_as_string, + logger: logger + ) + end + + let(:logger) { double("Logger").as_null_object } + + context "when the petition has not been updated" do + it "uses the correct mailer to generate the email" do + expect(subject).to receive(:create_email).and_call_original + perform_job + 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 + end + + it "does sends the email" do + expect { perform_job }.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) + 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 + + it "captures the error and reraises it" do + expect { perform_job }.to raise_error(exception_class) + end + + 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 } + 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 + end + end + + context "with connection refused" do + let(:exception_class) { Errno::ECONNREFUSED } + + it_behaves_like 'catching errors during individual email sending' + end + + context "with an SMTPError" do + let(:exception_class) { Net::SMTPFatalError } + + it_behaves_like 'catching errors during individual email sending' + end + + context 'with a timeout' do + let(:exception_class) { Errno::ETIMEDOUT } + + it_behaves_like 'catching errors during individual email sending' + end + end + end + + context "when the petition has been updated" do + before do + petition.set_email_requested_at_for(timestamp_name, to: Time.current + 5.minutes ) + end + + it "does not send any email" do + expect { perform_job }.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) + end + end +end diff --git a/spec/support/active_job.rb b/spec/support/active_job.rb index 8b02156a7..d59d69daa 100644 --- a/spec/support/active_job.rb +++ b/spec/support/active_job.rb @@ -1,3 +1,8 @@ RSpec.configure do |config| config.include(ActiveJob::TestHelper) + + config.before(:each) do + ActiveJob::Base.queue_adapter.enqueued_jobs = [] + ActiveJob::Base.queue_adapter.performed_jobs = [] + end end From e76cdb2525ba72063b3ac0ecd41c6fabeb041b5d Mon Sep 17 00:00:00 2001 From: ansonK Date: Tue, 28 Jul 2015 17:12:14 +0100 Subject: [PATCH 2/7] make deliver email jobs idempotent by checking whether the email has already been delivered --- app/jobs/email_delivery_jobs/base.rb | 15 ++++++++++++++- spec/jobs/shared_examples.rb | 14 ++++++++++++++ spec/support/email.rb | 4 ++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/app/jobs/email_delivery_jobs/base.rb b/app/jobs/email_delivery_jobs/base.rb index 93d3bceef..ed32b4e7b 100644 --- a/app/jobs/email_delivery_jobs/base.rb +++ b/app/jobs/email_delivery_jobs/base.rb @@ -19,7 +19,7 @@ def perform(signature:, timestamp_name:, petition:, @timestamp_name = timestamp_name @logger = logger - if petition_has_not_been_updated? + if can_send_email? send_email record_email_sent end @@ -29,6 +29,10 @@ def perform(signature:, timestamp_name:, petition:, attr_reader :mailer, :signature, :timestamp_name, :petition, :requested_at + def can_send_email? + petition_has_not_been_updated? && email_not_previously_sent? + end + def send_email create_email.deliver_now @@ -65,5 +69,14 @@ def petition_timestamp def petition_has_not_been_updated? petition_timestamp.to_i == requested_at.to_i 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? + end end end diff --git a/spec/jobs/shared_examples.rb b/spec/jobs/shared_examples.rb index 61cf17f63..3a25383ec 100644 --- a/spec/jobs/shared_examples.rb +++ b/spec/jobs/shared_examples.rb @@ -72,6 +72,20 @@ def perform_job expect { perform_job }.to change(EmailSentReceipt, :count).by(1) end + context "an email has already been sent for the petition to this signatory" do + before do + signature.set_email_sent_at_for timestamp_name, to: petition.get_email_requested_at_for(timestamp_name) + end + + it "does not send any email" do + expect { perform_job }.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) + end + end + context "email sending fails" do shared_examples_for 'catching errors during individual email sending' do before do diff --git a/spec/support/email.rb b/spec/support/email.rb index 95435e163..3ccea23a2 100644 --- a/spec/support/email.rb +++ b/spec/support/email.rb @@ -1,4 +1,8 @@ RSpec.configure do |config| config.include(EmailSpec::Helpers) config.include(EmailSpec::Matchers) + + config.before(:each) do + ActionMailer::Base.deliveries.clear + end end From f15b709d5d5b40660c20cf9033557e1a76d06eb3 Mon Sep 17 00:00:00 2001 From: ansonK Date: Wed, 29 Jul 2015 08:36:43 +0100 Subject: [PATCH 3/7] rails eager loads everything under /app so no need to manually do it --- config/application.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/config/application.rb b/config/application.rb index 7ec8b14a2..2203ea18c 100644 --- a/config/application.rb +++ b/config/application.rb @@ -44,8 +44,5 @@ class Application < Rails::Application ) config.action_dispatch.default_headers.merge!('X-UA-Compatible' => 'IE=edge') - - # Load the job concerns - config.eager_load_paths += %W(#{config.root}/app/jobs/**/*) end end From f86e681e717268eb3b3f21f87c41ece7be56f0e2 Mon Sep 17 00:00:00 2001 From: ansonK Date: Wed, 29 Jul 2015 08:41:24 +0100 Subject: [PATCH 4/7] memoize custom logger used for all active jobs --- config/initializers/job_logger.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/job_logger.rb b/config/initializers/job_logger.rb index c742d2407..383ab3f49 100644 --- a/config/initializers/job_logger.rb +++ b/config/initializers/job_logger.rb @@ -1,5 +1,5 @@ class ActiveJob::Base def logger - @logger || JobLogger.new(self.class.name) + @logger ||= JobLogger.new(self.class.name) end end From ffdbcedf43f9d699a4c49acd7db212104580371f Mon Sep 17 00:00:00 2001 From: ansonK Date: Wed, 29 Jul 2015 09:04:04 +0100 Subject: [PATCH 5/7] spec helpers already clear out test jobs between specs --- spec/support/active_job.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/spec/support/active_job.rb b/spec/support/active_job.rb index d59d69daa..8b02156a7 100644 --- a/spec/support/active_job.rb +++ b/spec/support/active_job.rb @@ -1,8 +1,3 @@ RSpec.configure do |config| config.include(ActiveJob::TestHelper) - - config.before(:each) do - ActiveJob::Base.queue_adapter.enqueued_jobs = [] - ActiveJob::Base.queue_adapter.performed_jobs = [] - end end From 60bb75dc69b47d0c86145326b4480ed0e29f2ff3 Mon Sep 17 00:00:00 2001 From: ansonK Date: Wed, 29 Jul 2015 10:51:22 +0100 Subject: [PATCH 6/7] consolidate .run_later_tonight method for queuing email jobs to one place, not several that do the same thing --- app/jobs/email_debate_outcomes_job.rb | 5 --- app/jobs/email_debate_scheduled_job.rb | 5 --- app/jobs/email_petition_signatories_job.rb | 15 ++++--- spec/jobs/email_petition_signatories_spec.rb | 44 -------------------- spec/jobs/shared_examples.rb | 33 +++++++++++++++ 5 files changed, 42 insertions(+), 60 deletions(-) delete mode 100644 spec/jobs/email_petition_signatories_spec.rb diff --git a/app/jobs/email_debate_outcomes_job.rb b/app/jobs/email_debate_outcomes_job.rb index 689fcd652..3ca09946c 100644 --- a/app/jobs/email_debate_outcomes_job.rb +++ b/app/jobs/email_debate_outcomes_job.rb @@ -1,9 +1,4 @@ class EmailDebateOutcomesJob < EmailPetitionSignatoriesJob - def self.run_later_tonight(petition) - petition.set_email_requested_at_for('debate_outcome', to: Time.current) - super(petition, petition.get_email_requested_at_for('debate_outcome')) - end - def email_delivery_job_class EmailDeliveryJobs::DebateOutcome end diff --git a/app/jobs/email_debate_scheduled_job.rb b/app/jobs/email_debate_scheduled_job.rb index fda6c9461..882f8b5fb 100644 --- a/app/jobs/email_debate_scheduled_job.rb +++ b/app/jobs/email_debate_scheduled_job.rb @@ -1,9 +1,4 @@ class EmailDebateScheduledJob < EmailPetitionSignatoriesJob - def self.run_later_tonight(petition) - petition.set_email_requested_at_for('debate_scheduled', to: Time.current) - super(petition, petition.get_email_requested_at_for('debate_scheduled')) - end - def email_delivery_job_class EmailDeliveryJobs::DebateScheduled end diff --git a/app/jobs/email_petition_signatories_job.rb b/app/jobs/email_petition_signatories_job.rb index e18674895..b05fc0f85 100644 --- a/app/jobs/email_petition_signatories_job.rb +++ b/app/jobs/email_petition_signatories_job.rb @@ -1,9 +1,13 @@ class EmailPetitionSignatoriesJob < ActiveJob::Base queue_as :default - def self.run_later_tonight(petition, requested_at, *extra_args) + def self.run_later_tonight(petition) + requested_at = Time.current + + petition.set_email_requested_at_for(new.timestamp_name, to: requested_at) + set(wait_until: later_tonight). - perform_later(petition, requested_at.getutc.iso8601, *extra_args) + perform_later(petition, requested_at.getutc.iso8601) end def self.later_tonight @@ -11,6 +15,9 @@ def self.later_tonight end private_class_method :later_tonight + def timestamp_name + raise NotImplementedError.new "Extending classes must implement #timestamp_name" + end def perform(petition, requested_at_string) @petition = petition @@ -71,10 +78,6 @@ def signatures_to_email def email_delivery_job_class raise NotImplementedError.new "Extending classes must implement email_delivery_job_class" end - - def timestamp_name - raise NotImplementedError.new "Extending classes must implement timestamp_name" - end end diff --git a/spec/jobs/email_petition_signatories_spec.rb b/spec/jobs/email_petition_signatories_spec.rb deleted file mode 100644 index 9475416fc..000000000 --- a/spec/jobs/email_petition_signatories_spec.rb +++ /dev/null @@ -1,44 +0,0 @@ -require 'rails_helper' - -RSpec.describe EmailPetitionSignatoriesJob, type: :job do - describe '.run_later_tonight' do - let(:petition) { FactoryGirl.create(:open_petition) } - let(:requested_at) { Time.current } - - def global_id_job_arg_for(object) - { "_aj_globalid" => object.to_global_id.to_s } - end - - def timestamp_job_arg_for(timestamp) - timestamp.getutc.iso8601 - end - - it 'queues up a job' do - described_class.run_later_tonight(petition, requested_at) - expect(enqueued_jobs.size).to eq 1 - expect(enqueued_jobs.first[:job]).to eq described_class - end - - it 'sets the job to run between midnight and 4am tomorrow' do - described_class.run_later_tonight(petition, requested_at) - 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 } - end - - it 'queues up the job to run with the petition and timestamp supplied as args' do - described_class.run_later_tonight(petition, requested_at) - 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 - - it 'adds any extra params provided as job args after the petition and timestamp' do - described_class.run_later_tonight(petition, requested_at, 'cheese', 1, petition.creator_signature) - queued_args = enqueued_jobs.first[:args] - expect(queued_args[2]).to eq 'cheese' - expect(queued_args[3]).to eq 1 - expect(queued_args[4]).to eq global_id_job_arg_for(petition.creator_signature) - end - end -end diff --git a/spec/jobs/shared_examples.rb b/spec/jobs/shared_examples.rb index 3a25383ec..c77377733 100644 --- a/spec/jobs/shared_examples.rb +++ b/spec/jobs/shared_examples.rb @@ -4,6 +4,39 @@ def do_work(requested_at = email_requested_at) subject.perform(petition, requested_at) end + describe '.run_later_tonight' do + let(:petition) { FactoryGirl.create(:open_petition) } + let(:requested_at) { Time.current } + + def global_id_job_arg_for(object) + { "_aj_globalid" => object.to_global_id.to_s } + end + + def timestamp_job_arg_for(timestamp) + timestamp.getutc.iso8601 + 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 + 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 } + end + + it 'queues up the job to run with the petition and timestamp supplied as args' 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 + end + context "when the petition has not been updated" do it "enqueues a job to send an email to each signatory" do do_work From 13306926b3ba9b01ab89ddd49f4c98fac059ddaa Mon Sep 17 00:00:00 2001 From: ansonK Date: Wed, 29 Jul 2015 12:11:45 +0100 Subject: [PATCH 7/7] Refactor email sending jobs to extract common functionality into concerns rather than have base classes that cannot be instaniated directly Move all shared functionality for emailing jobs into concerns out of concrete job classes. Use usec accuracy when comparing versions of petitions. --- Gemfile.lock | 3 - .../email_all_petition_signatories.rb} | 52 +++++++----- app/jobs/concerns/email_delivery.rb | 83 +++++++++++++++++++ app/jobs/deliver_debate_outcome_email_job.rb | 8 ++ .../deliver_debate_scheduled_email_job.rb | 8 ++ .../deliver_threshold_response_email_job.rb | 8 ++ app/jobs/email_debate_outcomes_job.rb | 6 +- app/jobs/email_debate_scheduled_job.rb | 6 +- app/jobs/email_delivery_jobs/base.rb | 82 ------------------ .../email_delivery_jobs/debate_outcome.rb | 9 -- .../email_delivery_jobs/debate_scheduled.rb | 9 -- .../email_delivery_jobs/threshold_response.rb | 9 -- app/jobs/email_threshold_response_job.rb | 16 +--- config/application.rb | 3 + ... deliver_debate_outcome_email_job_spec.rb} | 4 +- ...eliver_debate_scheduled_email_job_spec.rb} | 4 +- ...iver_threshold_response_email_job_spec.rb} | 4 +- spec/jobs/shared_examples.rb | 43 ++++++++-- 18 files changed, 193 insertions(+), 164 deletions(-) rename app/jobs/{email_petition_signatories_job.rb => concerns/email_all_petition_signatories.rb} (59%) create mode 100644 app/jobs/concerns/email_delivery.rb create mode 100644 app/jobs/deliver_debate_outcome_email_job.rb create mode 100644 app/jobs/deliver_debate_scheduled_email_job.rb create mode 100644 app/jobs/deliver_threshold_response_email_job.rb delete mode 100644 app/jobs/email_delivery_jobs/base.rb delete mode 100644 app/jobs/email_delivery_jobs/debate_outcome.rb delete mode 100644 app/jobs/email_delivery_jobs/debate_scheduled.rb delete mode 100644 app/jobs/email_delivery_jobs/threshold_response.rb rename spec/jobs/{email_delivery_jobs/debate_outcome_spec.rb => deliver_debate_outcome_email_job_spec.rb} (88%) rename spec/jobs/{email_delivery_jobs/debate_scheduled_spec.rb => deliver_debate_scheduled_email_job_spec.rb} (88%) rename spec/jobs/{email_delivery_jobs/threshold_response_spec.rb => deliver_threshold_response_email_job_spec.rb} (88%) diff --git a/Gemfile.lock b/Gemfile.lock index 6ed5ea682..fd1b422b9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -334,6 +334,3 @@ DEPENDENCIES webmock whenever will_paginate - -BUNDLED WITH - 1.10.4 diff --git a/app/jobs/email_petition_signatories_job.rb b/app/jobs/concerns/email_all_petition_signatories.rb similarity index 59% rename from app/jobs/email_petition_signatories_job.rb rename to app/jobs/concerns/email_all_petition_signatories.rb index b05fc0f85..0c59b360e 100644 --- a/app/jobs/email_petition_signatories_job.rb +++ b/app/jobs/concerns/email_all_petition_signatories.rb @@ -1,30 +1,42 @@ -class EmailPetitionSignatoriesJob < ActiveJob::Base - queue_as :default +module EmailAllPetitionSignatories + extend ActiveSupport::Concern - def self.run_later_tonight(petition) - requested_at = Time.current + # + # Concern to add shared functionality to ActiveJob classes that are responsible + # for enqueuing send email jobs + # - petition.set_email_requested_at_for(new.timestamp_name, to: requested_at) + included do + queue_as :default - set(wait_until: later_tonight). - perform_later(petition, requested_at.getutc.iso8601) - end + def self.run_later_tonight(petition) + requested_at = Time.current - def self.later_tonight - 1.day.from_now.at_midnight + rand(240).minutes + rand(60).seconds - end - private_class_method :later_tonight + petition.set_email_requested_at_for(new.timestamp_name, to: requested_at) + + set(wait_until: later_tonight). + perform_later(petition, requested_at.getutc.iso8601(6)) + end + + def self.later_tonight + 1.day.from_now.at_midnight + rand(240).minutes + rand(60).seconds + end + private_class_method :later_tonight - def timestamp_name - raise NotImplementedError.new "Extending classes must implement #timestamp_name" end + + def perform(petition, requested_at_string) @petition = petition @requested_at = requested_at_string.in_time_zone do_work! end + def timestamp_name + raise NotImplementedError.new "Including classes must implement #timestamp_name method" + end + private attr_reader :petition, :requested_at @@ -36,9 +48,6 @@ def do_work! enqueue_send_email_jobs logger.info("Finished #{self.class.name} for petition '#{petition.action}'") - # TODO: removed check_all_assignees recieved email - # as email is not sent from here. - # Do we need to check that at all here? end # @@ -51,7 +60,7 @@ def enqueue_send_email_jobs signature: signature, timestamp_name: timestamp_name, petition: petition, - requested_at_as_string: requested_at.getutc.iso8601 + requested_at_as_string: requested_at.getutc.iso8601(6) ) end end @@ -60,10 +69,7 @@ def enqueue_send_email_jobs # 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? - # NOTE: to_i comparison is used to cater for precision differences - # between DB timestamp (petition_timestamp precise to fractional - # seconds) and job timestamp (requested_at - only seconds precise) - petition_timestamp.to_i != requested_at.to_i + (petition_timestamp - requested_at).abs > 1 end def petition_timestamp @@ -76,7 +82,7 @@ def signatures_to_email # The job class that handles the actual email sending for this job type def email_delivery_job_class - raise NotImplementedError.new "Extending classes must implement 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 new file mode 100644 index 000000000..67e758a01 --- /dev/null +++ b/app/jobs/concerns/email_delivery.rb @@ -0,0 +1,83 @@ +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 + # + + included do + queue_as :default + end + + def perform(signature:, timestamp_name:, petition:, + requested_at_as_string:, mailer: PetitionMailer.name, logger: nil) + + @mailer = mailer.constantize + @signature = signature + @petition = petition + @requested_at = requested_at_as_string.in_time_zone + @timestamp_name = timestamp_name + @logger = logger + + if can_send_email? + send_email + record_email_sent + end + end + + private + + attr_reader :mailer, :signature, :timestamp_name, :petition, :requested_at + + def can_send_email? + petition_has_not_been_updated? && email_not_previously_sent? + end + + def send_email + create_email.deliver_now + + 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 + end + + def create_email + raise NotImplementedError.new "Including classes must implement #create_email method" + end + + def record_email_sent + signature.set_email_sent_at_for timestamp_name, to: petition_timestamp + end + + def petition_timestamp + petition.get_email_requested_at_for(timestamp_name) + end + + # 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 + 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? + end +end diff --git a/app/jobs/deliver_debate_outcome_email_job.rb b/app/jobs/deliver_debate_outcome_email_job.rb new file mode 100644 index 000000000..22e36f647 --- /dev/null +++ b/app/jobs/deliver_debate_outcome_email_job.rb @@ -0,0 +1,8 @@ +class DeliverDebateOutcomeEmailJob < ActiveJob::Base + include EmailDelivery + + 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 new file mode 100644 index 000000000..8519cde56 --- /dev/null +++ b/app/jobs/deliver_debate_scheduled_email_job.rb @@ -0,0 +1,8 @@ +class DeliverDebateScheduledEmailJob < ActiveJob::Base + include EmailDelivery + + 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 new file mode 100644 index 000000000..2784b6532 --- /dev/null +++ b/app/jobs/deliver_threshold_response_email_job.rb @@ -0,0 +1,8 @@ +class DeliverThresholdResponseEmailJob < ActiveJob::Base + include EmailDelivery + + 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 3ca09946c..116e44064 100644 --- a/app/jobs/email_debate_outcomes_job.rb +++ b/app/jobs/email_debate_outcomes_job.rb @@ -1,6 +1,8 @@ -class EmailDebateOutcomesJob < EmailPetitionSignatoriesJob +class EmailDebateOutcomesJob < ActiveJob::Base + include EmailAllPetitionSignatories + def email_delivery_job_class - EmailDeliveryJobs::DebateOutcome + DeliverDebateOutcomeEmailJob end def timestamp_name diff --git a/app/jobs/email_debate_scheduled_job.rb b/app/jobs/email_debate_scheduled_job.rb index 882f8b5fb..54f9134e7 100644 --- a/app/jobs/email_debate_scheduled_job.rb +++ b/app/jobs/email_debate_scheduled_job.rb @@ -1,6 +1,8 @@ -class EmailDebateScheduledJob < EmailPetitionSignatoriesJob +class EmailDebateScheduledJob < ActiveJob::Base + include EmailAllPetitionSignatories + def email_delivery_job_class - EmailDeliveryJobs::DebateScheduled + DeliverDebateScheduledEmailJob end def timestamp_name diff --git a/app/jobs/email_delivery_jobs/base.rb b/app/jobs/email_delivery_jobs/base.rb deleted file mode 100644 index ed32b4e7b..000000000 --- a/app/jobs/email_delivery_jobs/base.rb +++ /dev/null @@ -1,82 +0,0 @@ -module EmailDeliveryJobs - class Base < ActiveJob::Base - - # - # 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 - # - - queue_as :default - - def perform(signature:, timestamp_name:, petition:, - requested_at_as_string:, mailer: PetitionMailer.name, logger: nil) - - @mailer = mailer.constantize - @signature = signature - @petition = petition - @requested_at = requested_at_as_string.in_time_zone - @timestamp_name = timestamp_name - @logger = logger - - if can_send_email? - send_email - record_email_sent - end - end - - private - - attr_reader :mailer, :signature, :timestamp_name, :petition, :requested_at - - def can_send_email? - petition_has_not_been_updated? && email_not_previously_sent? - end - - def send_email - create_email.deliver_now - - 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 - end - - def create_email - raise NotImplementedError.new "Extending classes must implement create_email" - end - - def record_email_sent - signature.set_email_sent_at_for timestamp_name, to: petition_timestamp - end - - def petition_timestamp - petition.get_email_requested_at_for(timestamp_name) - end - - # 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.to_i == requested_at.to_i - 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? - end - end -end diff --git a/app/jobs/email_delivery_jobs/debate_outcome.rb b/app/jobs/email_delivery_jobs/debate_outcome.rb deleted file mode 100644 index 956282f24..000000000 --- a/app/jobs/email_delivery_jobs/debate_outcome.rb +++ /dev/null @@ -1,9 +0,0 @@ -module EmailDeliveryJobs - class DebateOutcome < Base - - def create_email - mailer.notify_signer_of_debate_outcome signature.petition, signature - end - - end -end diff --git a/app/jobs/email_delivery_jobs/debate_scheduled.rb b/app/jobs/email_delivery_jobs/debate_scheduled.rb deleted file mode 100644 index 9781e4019..000000000 --- a/app/jobs/email_delivery_jobs/debate_scheduled.rb +++ /dev/null @@ -1,9 +0,0 @@ -module EmailDeliveryJobs - class DebateScheduled < Base - - def create_email - mailer.notify_signer_of_debate_scheduled signature.petition, signature - end - - end -end diff --git a/app/jobs/email_delivery_jobs/threshold_response.rb b/app/jobs/email_delivery_jobs/threshold_response.rb deleted file mode 100644 index 9e1fa7a97..000000000 --- a/app/jobs/email_delivery_jobs/threshold_response.rb +++ /dev/null @@ -1,9 +0,0 @@ -module EmailDeliveryJobs - class ThresholdResponse < Base - - def create_email - mailer.notify_signer_of_threshold_response signature.petition, signature - end - - end -end diff --git a/app/jobs/email_threshold_response_job.rb b/app/jobs/email_threshold_response_job.rb index 7f84186a1..5518af88a 100644 --- a/app/jobs/email_threshold_response_job.rb +++ b/app/jobs/email_threshold_response_job.rb @@ -1,19 +1,11 @@ -class EmailThresholdResponseJob < EmailPetitionSignatories::Job - def self.run_later_tonight(petition, requested_at = Time.current) - petition.set_email_requested_at_for('government_response', to: requested_at) - super(petition, petition.get_email_requested_at_for('government_response')) - end +class EmailThresholdResponseJob < ActiveJob::Base + include EmailAllPetitionSignatories - def perform(petition, requested_at_string, mailer = PetitionMailer.name, logger = nil) - @mailer = mailer.constantize - worker(petition, requested_at_string, logger).do_work! + def email_delivery_job_class + DeliverThresholdResponseEmailJob end def timestamp_name 'government_response' end - - def create_email(petition, signature) - @mailer.notify_signer_of_threshold_response(petition, signature) - end end diff --git a/config/application.rb b/config/application.rb index 2203ea18c..9d98d2005 100644 --- a/config/application.rb +++ b/config/application.rb @@ -44,5 +44,8 @@ class Application < Rails::Application ) config.action_dispatch.default_headers.merge!('X-UA-Compatible' => 'IE=edge') + + # Needed as Rails does not eager load app/jobs/concerns by default + config.eager_load_paths += [Rails.root.join('app/jobs/concerns')] end end diff --git a/spec/jobs/email_delivery_jobs/debate_outcome_spec.rb b/spec/jobs/deliver_debate_outcome_email_job_spec.rb similarity index 88% rename from spec/jobs/email_delivery_jobs/debate_outcome_spec.rb rename to spec/jobs/deliver_debate_outcome_email_job_spec.rb index 69713761c..f3a61a413 100644 --- a/spec/jobs/email_delivery_jobs/debate_outcome_spec.rb +++ b/spec/jobs/deliver_debate_outcome_email_job_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' -require_relative '../shared_examples' +require_relative 'shared_examples' -RSpec.describe EmailDeliveryJobs::DebateOutcome, type: :job do +RSpec.describe DeliverDebateOutcomeEmailJob, type: :job do let(:email_requested_at) { Time.current } let(:petition) { FactoryGirl.create(:debated_petition) } let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) } diff --git a/spec/jobs/email_delivery_jobs/debate_scheduled_spec.rb b/spec/jobs/deliver_debate_scheduled_email_job_spec.rb similarity index 88% rename from spec/jobs/email_delivery_jobs/debate_scheduled_spec.rb rename to spec/jobs/deliver_debate_scheduled_email_job_spec.rb index 951166ee0..2d08cb8c0 100644 --- a/spec/jobs/email_delivery_jobs/debate_scheduled_spec.rb +++ b/spec/jobs/deliver_debate_scheduled_email_job_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' -require_relative '../shared_examples' +require_relative 'shared_examples' -RSpec.describe EmailDeliveryJobs::DebateScheduled, type: :job do +RSpec.describe DeliverDebateScheduledEmailJob, type: :job do let(:email_requested_at) { Time.current } let(:petition) { FactoryGirl.create(:awaiting_debate_petition) } let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) } diff --git a/spec/jobs/email_delivery_jobs/threshold_response_spec.rb b/spec/jobs/deliver_threshold_response_email_job_spec.rb similarity index 88% rename from spec/jobs/email_delivery_jobs/threshold_response_spec.rb rename to spec/jobs/deliver_threshold_response_email_job_spec.rb index ead2a1ac0..ce47b2027 100644 --- a/spec/jobs/email_delivery_jobs/threshold_response_spec.rb +++ b/spec/jobs/deliver_threshold_response_email_job_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' -require_relative '../shared_examples' +require_relative 'shared_examples' -RSpec.describe EmailDeliveryJobs::ThresholdResponse, type: :job do +RSpec.describe DeliverThresholdResponseEmailJob, type: :job do let(:email_requested_at) { Time.current } let(:petition) { FactoryGirl.create(:responded_petition) } let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) } diff --git a/spec/jobs/shared_examples.rb b/spec/jobs/shared_examples.rb index c77377733..e8ca56aac 100644 --- a/spec/jobs/shared_examples.rb +++ b/spec/jobs/shared_examples.rb @@ -1,9 +1,36 @@ 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 + @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 + + 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 + end + describe '.run_later_tonight' do let(:petition) { FactoryGirl.create(:open_petition) } let(:requested_at) { Time.current } @@ -13,7 +40,7 @@ def global_id_job_arg_for(object) end def timestamp_job_arg_for(timestamp) - timestamp.getutc.iso8601 + timestamp.getutc.iso8601(6) end it 'queues up a job' do @@ -30,10 +57,12 @@ def timestamp_job_arg_for(timestamp) end it 'queues up the job to run with the petition and timestamp supplied as args' 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) + 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 end end @@ -72,7 +101,7 @@ def timestamp_job_arg_for(timestamp) RSpec.shared_examples_for "a job to send an signatory email" do def perform_job - @requested_at_as_string = email_requested_at.getutc.iso8601 + @requested_at_as_string = email_requested_at.getutc.iso8601(6) subject.perform( signature: signature, timestamp_name: timestamp_name,