[refactor] Migrate from Sidekiq workers to ActiveJob jobs (#153)

* [refactor] Migrate from Sidekiq workers to ActiveJob jobs

Fixes #26

* Clean up mailer exception handling

Guardfile

@@ -5,7 +5,7 @@ guard :minitest, all_on_start: false, all_after_pass: false, spring: "bin/rails
   watch(%r{^app/mailers/(.+)\.rb$})                              { |m| "test/controllers/#{m[1]}_test.rb" }
   watch(%r{^app/controllers/(.+)\.rb$})                          { |m| "test/controllers/#{m[1]}_test.rb" }
   watch(%r{^app/views/(.+)\/.+$})                                { |m| "test/controllers/#{m[1]}_controller_test.rb" }
-  watch(%r{^app/workers/(.+)\.rb$})                              { |m| "test/workers/#{m[1]}_test.rb" }
+  watch(%r{^app/jobs/(.+)\.rb$})                                 { |m| "test/jobs/#{m[1]}_test.rb" }
   watch(%r{^app/views/layouts/.+$})                              { "test/controllers" }
   watch(%r{^app/views/.+\.rb$})                                  { "test/integration" }
   watch(%r{^test/.+_test.rb$})

app/controllers/manage/messages_controller.rb

@@ -52,7 +52,7 @@ def deliver
       return redirect_to manage_messages_path
     end
     @message.update_attribute(:queued_at, Time.now)
-    BulkMessageWorker.perform_async(@message.id)
+    BulkMessageJob.perform_later(@message)
     flash[:notice] = "Message queued for delivery"
     redirect_to manage_message_path(@message)
   end

app/workers/bulk_message_worker.rb → app/jobs/bulk_message_job.rb

@@ -1,16 +1,15 @@
-class BulkMessageWorker
-  include Sidekiq::Worker
+class BulkMessageJob < ApplicationJob
+  queue_as :default
 
-  def perform(message_id)
-    message = Message.find(message_id)
-    return unless message.present? && message.status == "queued"
+  def perform(message)
+    return unless message.status == "queued"
 
     message.update_attribute(:started_at, Time.now)
 
     recipients = self.class.build_recipients(message.recipients)
 
     recipients.each do |recipient|
-      Mailer.delay.bulk_message_email(message.id, recipient)
+      Mailer.bulk_message_email(message.id, recipient).deliver_later
     end
 
     message.update_attribute(:delivered_at, Time.now)

app/mailers/mailer.rb

@@ -23,24 +23,18 @@ def incomplete_reminder_email(user_id)
     Message.queue_for_trigger("user.24hr_incomplete_application", @user.id)
   end
 
+  rescue_from SparkPostRails::DeliveryException do |e|
+    error_codes_to_not_retry = [
+      "1902" # Generation rejection
+    ]
+    raise e unless e.blank? || error_codes_to_not_retry.include?(e.service_code)
+  end
+
   private
 
   def pretty_email(name, email)
     return email if name.blank?
 
     "\"#{name}\" <#{email}>"
   end
-
-  def mail_questionnaire(subject, sparkpost_data = {})
-    mail(
-      to: pretty_email(@questionnaire.full_name, @questionnaire.user.email),
-      subject: subject,
-      sparkpost_data: sparkpost_data
-    )
-  rescue SparkPostRails::DeliveryException => e
-    error_code_to_not_retry = [
-      "1902" # Generation rejection
-    ]
-    raise e unless e.blank? || error_code_to_not_retry.include?(e.service_code)
-  end
 end

app/models/message.rb

@@ -171,7 +171,7 @@ def self.for_trigger(trigger)
   end
 
   def self.queue_for_trigger(trigger, user_id)
-    for_trigger(trigger).map { |message| Mailer.delay.bulk_message_email(message.id, user_id) }
+    for_trigger(trigger).map { |message| Mailer.bulk_message_email(message.id, user_id).deliver_later }
   end
 
   def self.bulk

app/models/user.rb

@@ -32,7 +32,7 @@ def send_devise_notification(notification, *args)
 
   def queue_reminder_email
     return if reminder_sent_at
-    Mailer.delay_for(1.day).incomplete_reminder_email(id)
+    Mailer.incomplete_reminder_email(id).deliver_later(wait: 1.day)
     update_attribute(:reminder_sent_at, Time.now)
   end
 

app/views/manage/messages/show.html.haml

@@ -1,6 +1,6 @@
 :ruby
   begin
-    recipient_count = pluralize(BulkMessageWorker.build_recipients(@message.recipients).count, "recipient")
+    recipient_count = pluralize(BulkMessageJob.build_recipients(@message.recipients).count, "recipient")
   rescue => recipient_error
   end
 

config/environments/development.rb

@@ -31,7 +31,7 @@
   config.active_storage.service = :local
 
   # Don't care if the mailer can't send.
-  config.action_mailer.raise_delivery_errors = false
+  config.action_mailer.raise_delivery_errors = true
 
   # MailCatcher
   config.action_mailer.delivery_method = :smtp

config/environments/production.rb

@@ -67,7 +67,7 @@
 
   # Ignore bad email addresses and do not raise email delivery errors.
   # Set this to true and configure the email server for immediate delivery to raise delivery errors.
-  # config.action_mailer.raise_delivery_errors = false
+  config.action_mailer.raise_delivery_errors = true
 
   # Enable locale fallbacks for I18n (makes lookups for any locale fall back to
   # the I18n.default_locale when a translation cannot be found).

config/initializers/devise.rb

@@ -13,7 +13,7 @@
   # Configure the e-mail address which will be shown in Devise::Mailer,
   # note that it will be overwritten if you use your own mailer class
   # with default "from" parameter.
-  config.mailer_sender = -> { HackathonConfig['email_from'] }
+  config.mailer_sender = ->(_) { HackathonConfig['email_from'] }
 
   # Configure the class responsible to send e-mails.
   # config.mailer = 'Devise::Mailer'

config/initializers/sidekiq.rb

@@ -1,11 +1,9 @@
 if Rails.env.production? || Rails.env.staging?
   Sidekiq.configure_server do |config|
-    config.redis = { url: ENV['REDIS_URL'] }
+    config.redis = { url: ENV["REDIS_URL"] }
   end
 
   Sidekiq.configure_client do |config|
-    config.redis = { url: ENV['REDIS_URL'] }
+    config.redis = { url: ENV["REDIS_URL"] }
   end
 end
-
-Sidekiq::Extensions.enable_delay!

config/sidekiq.yml

@@ -0,0 +1,4 @@
+---
+:queues:
+  - default
+  - mailers

test/controllers/mailer_test.rb

@@ -1,8 +1,6 @@
-require 'test_helper'
+require "test_helper"
 
 class MailerTest < ActionMailer::TestCase
-  setup { ActionMailer::Base.deliveries.clear }
-
   context "upon trigger of a bulk message" do
     setup do
       @message = create(:message, subject: "Example Subject", body: "Hello World!")
@@ -13,20 +11,20 @@ class MailerTest < ActionMailer::TestCase
       email = Mailer.bulk_message_email(@message.id, @user.id).deliver_now
 
       assert_equal ["test@example.com"], email.to
-      assert_equal "Example Subject",    email.subject
-      assert_match /Hello World/,        email.encoded
+      assert_equal "Example Subject", email.subject
+      assert_match /Hello World/, email.encoded
     end
   end
 
   context "upon scheduled incomplete reminder email" do
     setup do
       @user = create(:user, email: "test@example.com")
-      @message = create(:message, subject: "Incomplete Application", type: 'automated', trigger: 'user.24hr_incomplete_application')
+      @message = create(:message, subject: "Incomplete Application", type: "automated", trigger: "user.24hr_incomplete_application")
     end
 
     should "queue reminder bulk message" do
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 1 do
-        Mailer.incomplete_reminder_email(@user.id).deliver_now
+      assert_difference "enqueued_jobs.size", 1 do
+        Mailer.incomplete_reminder_email(@user.id).deliver_later
       end
     end
   end
@@ -35,7 +33,7 @@ class MailerTest < ActionMailer::TestCase
     setup do
       @user = create(:user, email: "test@example.com")
       @message = create(:message, subject: "Example Subject", body: "Hello World!")
-      HackathonConfig['email_from'] = 'This is a test <test@custom.example.com>'
+      HackathonConfig["email_from"] = "This is a test <test@custom.example.com>"
     end
 
     should "use customized email_from" do

test/controllers/manage/bus_lists_controller_test.rb

@@ -1,6 +1,8 @@
-require 'test_helper'
+require "test_helper"
 
 class Manage::BusListsControllerTest < ActionController::TestCase
+  include ActiveJob::TestHelper
+
   setup do
     @bus_list = create(:bus_list)
   end
@@ -44,16 +46,16 @@ class Manage::BusListsControllerTest < ActionController::TestCase
 
     should "not allow access to manage_bus_lists#toggle_bus_captain" do
       questionnaire = create(:questionnaire)
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 0 do
-        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: '1' }
+      assert_difference "enqueued_jobs.size", 0 do
+        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: "1" }
       end
       assert_equal false, questionnaire.reload.is_bus_captain
       assert_response :redirect
       assert_redirected_to new_user_session_path
     end
 
     should "not allow access to manage_bus_lists#send_update_email" do
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 0 do
+      assert_difference "enqueued_jobs.size", 0 do
         patch :send_update_email, params: { id: @bus_list }
       end
       assert_response :redirect
@@ -112,16 +114,16 @@ class Manage::BusListsControllerTest < ActionController::TestCase
 
     should "not allow access to manage_bus_lists#toggle_bus_captain" do
       questionnaire = create(:questionnaire)
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 0 do
-        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: '1' }
+      assert_difference "enqueued_jobs.size", 0 do
+        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: "1" }
       end
       assert_equal false, questionnaire.reload.is_bus_captain
       assert_response :redirect
       assert_redirected_to root_path
     end
 
     should "not allow access to manage_bus_lists#send_update_email" do
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 0 do
+      assert_difference "enqueued_jobs.size", 0 do
         patch :send_update_email, params: { id: @bus_list }
       end
       assert_response :redirect
@@ -178,16 +180,16 @@ class Manage::BusListsControllerTest < ActionController::TestCase
 
     should "not allow access to manage_bus_lists#toggle_bus_captain" do
       questionnaire = create(:questionnaire)
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 0 do
-        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: '1' }
+      assert_difference "enqueued_jobs.size", 0 do
+        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: "1" }
       end
       assert_equal false, questionnaire.reload.is_bus_captain
       assert_response :redirect
       assert_redirected_to manage_bus_lists_path
     end
 
     should "not allow access to manage_bus_lists#send_update_email" do
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 0 do
+      assert_difference "enqueued_jobs.size", 0 do
         patch :send_update_email, params: { id: @bus_list }
       end
       assert_response :redirect
@@ -230,14 +232,14 @@ class Manage::BusListsControllerTest < ActionController::TestCase
     end
 
     should "render markdown in manage_bus_lists#show" do
-      @bus_list.update_attribute(:notes, '### This is a title')
+      @bus_list.update_attribute(:notes, "### This is a title")
       get :show, params: { id: @bus_list }
       assert_response :success
       assert_select "fieldset h3", "This is a title"
     end
 
     should "render html in manage_bus_lists#show" do
-      @bus_list.update_attribute(:notes, '<h3>This is a title</h3>')
+      @bus_list.update_attribute(:notes, "<h3>This is a title</h3>")
       get :show, params: { id: @bus_list }
       assert_response :success
       assert_select "fieldset h3", "This is a title"
@@ -255,34 +257,34 @@ class Manage::BusListsControllerTest < ActionController::TestCase
 
     should "make questionnaire a bus captain" do
       questionnaire = create(:questionnaire)
-      patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: '1' }
+      patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: "1" }
       assert_equal true, questionnaire.reload.is_bus_captain
       assert_response :redirect
       assert_redirected_to manage_bus_list_path(@bus_list)
     end
 
     should "send message to notify bus captain" do
       questionnaire = create(:questionnaire)
-      create(:message, type: 'automated', trigger: 'bus_list.new_captain_confirmation')
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 1 do
-        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: '1' }
+      create(:message, type: "automated", trigger: "bus_list.new_captain_confirmation")
+      assert_difference "enqueued_jobs.size", 1 do
+        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: "1" }
       end
     end
 
     should "remove questionnaire from being a bus captain" do
       questionnaire = create(:questionnaire)
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 0 do
-        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: '0' }
+      assert_difference "enqueued_jobs.size", 0 do
+        patch :toggle_bus_captain, params: { id: @bus_list, questionnaire_id: questionnaire.id, bus_captain: "0" }
       end
       assert_equal false, questionnaire.reload.is_bus_captain
       assert_response :redirect
       assert_redirected_to manage_bus_list_path(@bus_list)
     end
 
     should "send email upon manage_bus_lists#send_update_email" do
-      create(:questionnaire, acc_status: 'rsvp_confirmed', bus_list_id: @bus_list.id)
-      create(:message, type: 'automated', trigger: 'bus_list.notes_update')
-      assert_difference 'Sidekiq::Extensions::DelayedMailer.jobs.size', 1 do
+      create(:questionnaire, acc_status: "rsvp_confirmed", bus_list_id: @bus_list.id)
+      create(:message, type: "automated", trigger: "bus_list.notes_update")
+      assert_difference "enqueued_jobs.size", 1 do
         patch :send_update_email, params: { id: @bus_list }
       end
       assert_response :redirect
@@ -291,7 +293,7 @@ class Manage::BusListsControllerTest < ActionController::TestCase
 
     context "#destroy" do
       should "destroy bus_list" do
-        assert_difference('BusList.count', -1) do
+        assert_difference("BusList.count", -1) do
           patch :destroy, params: { id: @bus_list }
         end
         assert_redirected_to manage_bus_lists_path

test/controllers/manage/messages_controller_test.rb

@@ -1,6 +1,8 @@
 require 'test_helper'
 
 class Manage::MessagesControllerTest < ActionController::TestCase
+  include ActiveJob::TestHelper
+
   setup do
     @message = create(:message)
   end
@@ -54,7 +56,7 @@ class Manage::MessagesControllerTest < ActionController::TestCase
     end
 
     should "not deliver message" do
-      assert_difference('BulkMessageWorker.jobs.size', 0) do
+      assert_difference("enqueued_jobs.size", 0) do
         patch :deliver, params: { id: @message }
       end
       assert_response :redirect
@@ -138,7 +140,7 @@ class Manage::MessagesControllerTest < ActionController::TestCase
     end
 
     should "not deliver message" do
-      assert_difference('BulkMessageWorker.jobs.size', 0) do
+      assert_difference("enqueued_jobs.size", 0) do
         patch :deliver, params: { id: @message }
       end
       assert_response :redirect
@@ -219,7 +221,7 @@ class Manage::MessagesControllerTest < ActionController::TestCase
     end
 
     should "not deliver message" do
-      assert_difference('BulkMessageWorker.jobs.size', 0) do
+      assert_difference("enqueued_jobs.size", 0) do
         patch :deliver, params: { id: @message }
       end
       assert_response :redirect
@@ -291,16 +293,16 @@ class Manage::MessagesControllerTest < ActionController::TestCase
     end
 
     should "deliver a bulk message" do
-      assert_difference('BulkMessageWorker.jobs.size', 1) do
+      assert_difference("enqueued_jobs.size", 1) do
         patch :deliver, params: { id: @message }
       end
       assert_match /queued/, flash[:notice]
       assert_redirected_to manage_message_path(assigns(:message))
     end
 
     should "not deliver an automated message" do
-      @message.update_attribute(:type, 'automated')
-      assert_difference('BulkMessageWorker.jobs.size', 0) do
+      @message.update_attribute(:type, "automated")
+      assert_difference("enqueued_jobs.size", 0) do
         patch :deliver, params: { id: @message }
       end
       assert_match /cannot be manually delivered/, flash[:error]