diff options
-rw-r--r-- | actionmailer/CHANGELOG.md | 2 | ||||
-rw-r--r-- | actionmailer/lib/action_mailer.rb | 1 | ||||
-rw-r--r-- | actionmailer/lib/action_mailer/base.rb | 2 | ||||
-rw-r--r-- | actionmailer/lib/action_mailer/delivery_job.rb | 13 | ||||
-rw-r--r-- | actionmailer/lib/action_mailer/mail_delivery_job.rb | 38 | ||||
-rw-r--r-- | actionmailer/lib/action_mailer/message_delivery.rb | 10 | ||||
-rw-r--r-- | actionmailer/lib/action_mailer/parameterized.rb | 18 | ||||
-rw-r--r-- | actionmailer/lib/action_mailer/test_helper.rb | 4 | ||||
-rw-r--r-- | actionmailer/test/legacy_delivery_job_test.rb | 86 | ||||
-rw-r--r-- | actionmailer/test/message_delivery_test.rb | 16 | ||||
-rw-r--r-- | actionmailer/test/parameterized_test.rb | 10 | ||||
-rw-r--r-- | actionmailer/test/test_helper_test.rb | 2 | ||||
-rw-r--r-- | actionpack/lib/action_controller/log_subscriber.rb | 2 | ||||
-rw-r--r-- | activerecord/test/cases/serialized_attribute_test.rb | 2 | ||||
-rw-r--r-- | guides/rails_guides.rb | 2 | ||||
-rw-r--r-- | guides/rails_guides/generator.rb | 4 | ||||
-rw-r--r-- | guides/source/testing.md | 50 |
17 files changed, 234 insertions, 28 deletions
diff --git a/actionmailer/CHANGELOG.md b/actionmailer/CHANGELOG.md index 9d2a1928c5..2de83e611b 100644 --- a/actionmailer/CHANGELOG.md +++ b/actionmailer/CHANGELOG.md @@ -1,4 +1,4 @@ -* Deliver parameterized mail with `ActionMailer::DeliveryJob` and remove `ActionMailer::Parameterized::DeliveryJob`. +* Add `MailDeliveryJob` for delivering both regular and parameterized mail. Deprecate using `DeliveryJob` and `Parameterized::DeliveryJob`. *Gannon McGibbon* diff --git a/actionmailer/lib/action_mailer.rb b/actionmailer/lib/action_mailer.rb index 69eae65d60..48f16180be 100644 --- a/actionmailer/lib/action_mailer.rb +++ b/actionmailer/lib/action_mailer.rb @@ -52,6 +52,7 @@ module ActionMailer autoload :TestHelper autoload :MessageDelivery autoload :DeliveryJob + autoload :MailDeliveryJob def self.eager_load! super diff --git a/actionmailer/lib/action_mailer/base.rb b/actionmailer/lib/action_mailer/base.rb index 5d41e5ff2a..8ddc90b9df 100644 --- a/actionmailer/lib/action_mailer/base.rb +++ b/actionmailer/lib/action_mailer/base.rb @@ -461,7 +461,7 @@ module ActionMailer helper ActionMailer::MailHelper - class_attribute :delivery_job, default: ::ActionMailer::DeliveryJob + class_attribute :delivery_job, default: ::ActionMailer::MailDeliveryJob class_attribute :default_params, default: { mime_version: "1.0", charset: "UTF-8", diff --git a/actionmailer/lib/action_mailer/delivery_job.rb b/actionmailer/lib/action_mailer/delivery_job.rb index 8af2097819..f228006920 100644 --- a/actionmailer/lib/action_mailer/delivery_job.rb +++ b/actionmailer/lib/action_mailer/delivery_job.rb @@ -12,9 +12,16 @@ module ActionMailer rescue_from StandardError, with: :handle_exception_with_mailer_class - def perform(mailer, mail_method, delivery_method, params, *args) #:nodoc: - mailer_class = params ? mailer.constantize.with(params) : mailer.constantize - mailer_class.public_send(mail_method, *args).send(delivery_method) + before_perform do + ActiveSupport::Deprecation.warn <<~MSG.squish + Sending mail with DeliveryJob and Parameterized::DeliveryJob + is deprecated and will be removed in Rails 6.1. + Please use MailDeliveryJob instead. + MSG + end + + def perform(mailer, mail_method, delivery_method, *args) #:nodoc: + mailer.constantize.public_send(mail_method, *args).send(delivery_method) end private diff --git a/actionmailer/lib/action_mailer/mail_delivery_job.rb b/actionmailer/lib/action_mailer/mail_delivery_job.rb new file mode 100644 index 0000000000..93778edfce --- /dev/null +++ b/actionmailer/lib/action_mailer/mail_delivery_job.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require "active_job" + +module ActionMailer + # The <tt>ActionMailer::NewDeliveryJob</tt> class is used when you + # want to send emails outside of the request-response cycle. It supports + # sending either parameterized or normal mail. + # + # Exceptions are rescued and handled by the mailer class. + class MailDeliveryJob < ActiveJob::Base # :nodoc: + queue_as { ActionMailer::Base.deliver_later_queue_name } + + rescue_from StandardError, with: :handle_exception_with_mailer_class + + def perform(mailer, mail_method, delivery_method, args:, params: nil) #:nodoc: + mailer_class = params ? mailer.constantize.with(params) : mailer.constantize + mailer_class.public_send(mail_method, *args).send(delivery_method) + end + + private + # "Deserialize" the mailer class name by hand in case another argument + # (like a Global ID reference) raised DeserializationError. + def mailer_class + if mailer = Array(@serialized_arguments).first || Array(arguments).first + mailer.constantize + end + end + + def handle_exception_with_mailer_class(exception) + if klass = mailer_class + klass.handle_exception exception + else + raise exception + end + end + end +end diff --git a/actionmailer/lib/action_mailer/message_delivery.rb b/actionmailer/lib/action_mailer/message_delivery.rb index 6b02076422..1e5cab6d47 100644 --- a/actionmailer/lib/action_mailer/message_delivery.rb +++ b/actionmailer/lib/action_mailer/message_delivery.rb @@ -135,10 +135,18 @@ module ActionMailer "#deliver_later, 2. only touch the message *within your mailer " \ "method*, or 3. use a custom Active Job instead of #deliver_later." else - args = @mailer_class.name, @action.to_s, delivery_method.to_s, nil, *@args job = @mailer_class.delivery_job + args = arguments_for(job, delivery_method) job.set(options).perform_later(*args) end end + + def arguments_for(delivery_job, delivery_method) + if delivery_job <= MailDeliveryJob + [@mailer_class.name, @action.to_s, delivery_method.to_s, args: @args] + else + [@mailer_class.name, @action.to_s, delivery_method.to_s, *@args] + end + end end end diff --git a/actionmailer/lib/action_mailer/parameterized.rb b/actionmailer/lib/action_mailer/parameterized.rb index 0b0afd6ad4..999435919e 100644 --- a/actionmailer/lib/action_mailer/parameterized.rb +++ b/actionmailer/lib/action_mailer/parameterized.rb @@ -121,6 +121,12 @@ module ActionMailer end end + class DeliveryJob < ActionMailer::DeliveryJob # :nodoc: + def perform(mailer, mail_method, delivery_method, params, *args) + mailer.constantize.with(params).public_send(mail_method, *args).send(delivery_method) + end + end + class MessageDelivery < ActionMailer::MessageDelivery # :nodoc: def initialize(mailer_class, action, params, *args) super(mailer_class, action, *args) @@ -139,11 +145,19 @@ module ActionMailer if processed? super else - args = @mailer_class.name, @action.to_s, delivery_method.to_s, @params, *@args - job = @mailer_class.delivery_job + job = @mailer_class.delivery_job + args = arguments_for(job, delivery_method) job.set(options).perform_later(*args) end end + + def arguments_for(delivery_job, delivery_method) + if delivery_job <= MailDeliveryJob + [@mailer_class.name, @action.to_s, delivery_method.to_s, params: @params, args: @args] + else + [@mailer_class.name, @action.to_s, delivery_method.to_s, @params, *@args] + end + end end end end diff --git a/actionmailer/lib/action_mailer/test_helper.rb b/actionmailer/lib/action_mailer/test_helper.rb index 3eb61c2363..e222301dff 100644 --- a/actionmailer/lib/action_mailer/test_helper.rb +++ b/actionmailer/lib/action_mailer/test_helper.rb @@ -125,9 +125,9 @@ module ActionMailer # end def assert_enqueued_email_with(mailer, method, args: nil, queue: "mailers", &block) args = if args.is_a?(Hash) - [mailer.to_s, method.to_s, "deliver_now", args] + [mailer.to_s, method.to_s, "deliver_now", params: args, args: []] else - [mailer.to_s, method.to_s, "deliver_now", nil, *args] + [mailer.to_s, method.to_s, "deliver_now", args: Array(args)] end assert_enqueued_with(job: mailer.delivery_job, args: args, queue: queue, &block) end diff --git a/actionmailer/test/legacy_delivery_job_test.rb b/actionmailer/test/legacy_delivery_job_test.rb new file mode 100644 index 0000000000..112c842beb --- /dev/null +++ b/actionmailer/test/legacy_delivery_job_test.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require "abstract_unit" +require "active_job" +require "mailers/params_mailer" +require "mailers/delayed_mailer" + +class LegacyDeliveryJobTest < ActiveSupport::TestCase + include ActiveJob::TestHelper + + class LegacyDeliveryJob < ActionMailer::DeliveryJob + end + + class LegacyParmeterizedDeliveryJob < ActionMailer::Parameterized::DeliveryJob + end + + setup do + @previous_logger = ActiveJob::Base.logger + ActiveJob::Base.logger = Logger.new(nil) + + @previous_delivery_method = ActionMailer::Base.delivery_method + ActionMailer::Base.delivery_method = :test + + @previous_deliver_later_queue_name = ActionMailer::Base.deliver_later_queue_name + ActionMailer::Base.deliver_later_queue_name = :test_queue + end + + teardown do + ActiveJob::Base.logger = @previous_logger + ParamsMailer.deliveries.clear + + ActionMailer::Base.delivery_method = @previous_delivery_method + ActionMailer::Base.deliver_later_queue_name = @previous_deliver_later_queue_name + end + + test "should send parameterized mail correctly" do + mail = ParamsMailer.with(inviter: "david@basecamp.com", invitee: "jason@basecamp.com").invitation + args = [ + "ParamsMailer", + "invitation", + "deliver_now", + { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" }, + ] + + with_delivery_job(LegacyParmeterizedDeliveryJob) do + assert_deprecated do + assert_performed_with(job: LegacyParmeterizedDeliveryJob, args: args) do + mail.deliver_later + end + end + end + end + + test "should send mail correctly" do + mail = DelayedMailer.test_message(1, 2, 3) + args = [ + "DelayedMailer", + "test_message", + "deliver_now", + 1, + 2, + 3, + ] + + with_delivery_job(LegacyDeliveryJob) do + assert_deprecated do + assert_performed_with(job: LegacyDeliveryJob, args: args) do + mail.deliver_later + end + end + end + end + + private + + def with_delivery_job(job) + old_params_delivery_job = ParamsMailer.delivery_job + old_regular_delivery_job = DelayedMailer.delivery_job + ParamsMailer.delivery_job = job + DelayedMailer.delivery_job = job + yield + ensure + ParamsMailer.delivery_job = old_params_delivery_job + DelayedMailer.delivery_job = old_regular_delivery_job + end +end diff --git a/actionmailer/test/message_delivery_test.rb b/actionmailer/test/message_delivery_test.rb index 8d5c473faf..46260f6414 100644 --- a/actionmailer/test/message_delivery_test.rb +++ b/actionmailer/test/message_delivery_test.rb @@ -64,20 +64,20 @@ class MessageDeliveryTest < ActiveSupport::TestCase end test "should enqueue the email with :deliver_now delivery method" do - assert_performed_with(job: ActionMailer::DeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3]) do + assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]]) do @mail.deliver_later end end test "should enqueue the email with :deliver_now! delivery method" do - assert_performed_with(job: ActionMailer::DeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now!", nil, 1, 2, 3]) do + assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now!", args: [1, 2, 3]]) do @mail.deliver_later! end end test "should enqueue a delivery with a delay" do travel_to Time.new(2004, 11, 24, 01, 04, 44) do - assert_performed_with(job: ActionMailer::DeliveryJob, at: Time.current + 10.minutes, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3]) do + assert_performed_with(job: ActionMailer::MailDeliveryJob, at: Time.current + 10.minutes, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]]) do @mail.deliver_later wait: 10.minutes end end @@ -85,13 +85,13 @@ class MessageDeliveryTest < ActiveSupport::TestCase test "should enqueue a delivery at a specific time" do later_time = Time.current + 1.hour - assert_performed_with(job: ActionMailer::DeliveryJob, at: later_time, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3]) do + assert_performed_with(job: ActionMailer::MailDeliveryJob, at: later_time, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]]) do @mail.deliver_later wait_until: later_time end end test "should enqueue the job on the correct queue" do - assert_performed_with(job: ActionMailer::DeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3], queue: "test_queue") do + assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]], queue: "test_queue") do @mail.deliver_later end end @@ -100,17 +100,17 @@ class MessageDeliveryTest < ActiveSupport::TestCase old_delivery_job = DelayedMailer.delivery_job DelayedMailer.delivery_job = DummyJob - assert_performed_with(job: DummyJob, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3]) do + assert_performed_with(job: DummyJob, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]]) do @mail.deliver_later end DelayedMailer.delivery_job = old_delivery_job end - class DummyJob < ActionMailer::DeliveryJob; end + class DummyJob < ActionMailer::MailDeliveryJob; end test "can override the queue when enqueuing mail" do - assert_performed_with(job: ActionMailer::DeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3], queue: "another_queue") do + assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]], queue: "another_queue") do @mail.deliver_later(queue: :another_queue) end end diff --git a/actionmailer/test/parameterized_test.rb b/actionmailer/test/parameterized_test.rb index 3369808515..7dd6156744 100644 --- a/actionmailer/test/parameterized_test.rb +++ b/actionmailer/test/parameterized_test.rb @@ -7,7 +7,7 @@ require "mailers/params_mailer" class ParameterizedTest < ActiveSupport::TestCase include ActiveJob::TestHelper - class DummyDeliveryJob < ActionMailer::DeliveryJob + class DummyDeliveryJob < ActionMailer::MailDeliveryJob end setup do @@ -42,9 +42,10 @@ class ParameterizedTest < ActiveSupport::TestCase "ParamsMailer", "invitation", "deliver_now", - { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" }, + params: { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" }, + args: [], ] - assert_performed_with(job: ActionMailer::DeliveryJob, args: args) do + assert_performed_with(job: ActionMailer::MailDeliveryJob, args: args) do @mail.deliver_later end end @@ -68,7 +69,8 @@ class ParameterizedTest < ActiveSupport::TestCase "ParamsMailer", "invitation", "deliver_now", - { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" }, + params: { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" }, + args: [], ] with_delivery_job DummyDeliveryJob do diff --git a/actionmailer/test/test_helper_test.rb b/actionmailer/test/test_helper_test.rb index 4e21ab0a69..60e2389aa8 100644 --- a/actionmailer/test/test_helper_test.rb +++ b/actionmailer/test/test_helper_test.rb @@ -24,7 +24,7 @@ class TestHelperMailer < ActionMailer::Base end end -class CustomDeliveryJob < ActionMailer::DeliveryJob +class CustomDeliveryJob < ActionMailer::MailDeliveryJob end class CustomDeliveryMailer < TestHelperMailer diff --git a/actionpack/lib/action_controller/log_subscriber.rb b/actionpack/lib/action_controller/log_subscriber.rb index afbd38e7fe..d8b04d8ddb 100644 --- a/actionpack/lib/action_controller/log_subscriber.rb +++ b/actionpack/lib/action_controller/log_subscriber.rb @@ -56,7 +56,7 @@ module ActionController def unpermitted_parameters(event) debug do unpermitted_keys = event.payload[:keys] - "Unpermitted parameter#{'s' if unpermitted_keys.size > 1}: #{unpermitted_keys.map { |e| ":#{e}" }.join(", ")}" + color("Unpermitted parameter#{'s' if unpermitted_keys.size > 1}: #{unpermitted_keys.map { |e| ":#{e}" }.join(", ")}", RED) end end diff --git a/activerecord/test/cases/serialized_attribute_test.rb b/activerecord/test/cases/serialized_attribute_test.rb index 08ab4dc600..f6cd4f85ee 100644 --- a/activerecord/test/cases/serialized_attribute_test.rb +++ b/activerecord/test/cases/serialized_attribute_test.rb @@ -371,7 +371,7 @@ class SerializedAttributeTest < ActiveRecord::TestCase end def test_serialized_attribute_works_under_concurrent_initial_access - model = Topic.dup + model = ::Topic.dup topic = model.last topic.update group: "1" diff --git a/guides/rails_guides.rb b/guides/rails_guides.rb index 28a7c94ca9..a72acdbd06 100644 --- a/guides/rails_guides.rb +++ b/guides/rails_guides.rb @@ -26,5 +26,5 @@ RailsGuides::Generator.new( only: env_value["ONLY"], kindle: env_flag["KINDLE"], language: env_value["GUIDES_LANGUAGE"], - direction: env_value["DIRECTION"].to_sym + direction: env_value["DIRECTION"] ).generate diff --git a/guides/rails_guides/generator.rb b/guides/rails_guides/generator.rb index 1f0a2063f2..48e90510e1 100644 --- a/guides/rails_guides/generator.rb +++ b/guides/rails_guides/generator.rb @@ -17,7 +17,7 @@ module RailsGuides class Generator GUIDES_RE = /\.(?:erb|md)\z/ - def initialize(edge:, version:, all:, only:, kindle:, language:, direction: :ltr) + def initialize(edge:, version:, all:, only:, kindle:, language:, direction: "ltr") @edge = edge @version = version @all = all @@ -118,7 +118,7 @@ module RailsGuides def copy_assets FileUtils.cp_r(Dir.glob("#{@guides_dir}/assets/*"), @output_dir) - if @direction == :rtl + if @direction == "rtl" overwrite_css_with_right_to_left_direction end end diff --git a/guides/source/testing.md b/guides/source/testing.md index 02b60a395b..9541598b26 100644 --- a/guides/source/testing.md +++ b/guides/source/testing.md @@ -1397,6 +1397,56 @@ class ProfileControllerTest < ActionDispatch::IntegrationTest end ``` +#### Using Separate Files + +If you find your helpers are cluttering `test_helper.rb`, you can extract them into separate files. One good place to store them is `lib/test`. + +```ruby +# lib/test/multiple_assertions.rb +module MultipleAssertions + def assert_multiple_of_fourty_two(number) + assert (number % 42 == 0), 'expected #{number} to be a multiple of 42' + end +end +``` + +These helpers can then be explicitly required as needed and included as needed + +```ruby +require 'test_helper' +require 'test/multiple_assertions' + +class NumberTest < ActiveSupport::TestCase + include MultipleAssertions + + test '420 is a multiple of fourty two' do + assert_multiple_of_fourty_two 420 + end +end +``` + +or they can continue to be included directly into the relevant parent classes + +```ruby +# test/test_helper.rb +require 'test/sign_in_helper' + +class ActionDispatch::IntegrationTest + include SignInHelper +end +``` + +#### Eagerly Requiring Helpers + +You may find it convenient to eagerly require helpers in `test_helper.rb` so your test files have implicit access to them. This can be accomplished using globbing, as follows + +```ruby +# test/test_helper.rb +Dir[Rails.root.join('lib', 'test', '**', '*.rb')].each { |file| require file } +``` + +This has the downside of increasing the boot-up time, as opposed to manually requiring only the necessary files in your individual tests. + Testing Routes -------------- |