Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MailDeliveryJob for unified mail delivery #34591

Merged
merged 1 commit into from Dec 5, 2018

Conversation

@gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Dec 2, 2018

Summary

In #34367, we merged a breaking change to actionmailer that changed the parameters of DeliveryJob. This will cause upgrade pains for any app delivering mail in the background. This patch attempts to rectify the situation by reverting and deprecating DeliveryJob and Parameterized::DeliveryJob and introducing NewDeliveryJob to send either kind of mail.

Because web and job workers aren't always deployed to in lockstep, and enqueued jobs can be worked off at any time during a deployment, we have two problems to solve:

  1. Old delivery jobs need to be able to be worked off by a rails 6 job worker.
  2. New delivery jobs have to be able to be worked off by an old rails 5.x job worker.

So, the parameter change to DeliveryJob needs to be reverted and Parameterized::DeliveryJob needs to be brought back to satisfy problem 1. And, the NewDeliveryJob class needs to be backported to 5.x to satisfy problem 2. I'll followup with a backport PR if we can all agree on this course of action.

cc @eileencodes @Edouard-chin

Copy link
Member

@eileencodes eileencodes left a comment

I haven't tried this with our app but I think it looks good. One concern I have is the name NewDeliveryJob isn't going to be a good name in a few years so I think we should change it to something we want to keep long term.

And, the NewDeliveryJob class needs to be backported to 5.x to satisfy problem 2. I'll followup with a backport PR if we can all agree on this course of action.

I think if the code isn't deprecated until 6.1 and the jobs work in 6.0 then we don't need a backport.

@@ -52,6 +52,7 @@ module ActionMailer
autoload :TestHelper
autoload :MessageDelivery
autoload :DeliveryJob
autoload :NewDeliveryJob
Copy link
Member

@eileencodes eileencodes Dec 3, 2018

I'm not sure NewDeliveryJob is the best name. We don't want users to have to change the constant name back after we fully remove DeliveryJob (and after a few years NewDeliveryJob will be a strange name cause it's not new anymore). What about a name that better describes the roll of this job AND has the new code like MailDeliveryJob?

Copy link
Member Author

@gmcgibbon gmcgibbon Dec 3, 2018

My plan was to deprecate NewDeliveryJob and rename to DeliveryJob in 6.2 or 7.0 but that will also cause problems for people that upgrade multiple versions at a time. I'll rename to MailDeliveryJob!

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)
end
Copy link
Member

@eileencodes eileencodes Dec 3, 2018

Sorry missed this on the first round of review - I think params should be nil by default right? Since parameter jobs are a special kind of job?

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
Copy link
Member

@eileencodes eileencodes Dec 3, 2018

Since Parameterized::DeliveryJob isn't released yet can we instead deprecate just DeliveryJob and then recommend the new one? If you need the params key then you should use the new one.

Copy link
Member Author

@gmcgibbon gmcgibbon Dec 3, 2018

I removed Parameterized::DeliveryJob from the warning, but the warning will still happen for parameterized delivery jobs because its a subclass. I assume this is good enough?

Copy link
Member

@eileencodes eileencodes Dec 3, 2018

I was thinking that Parameterized::DeliveryJob has never been released so we can delete the code that supported Parameterized::DeliveryJob and anyone who needs a Parameterized::DeliveryJob would use the new MailDeliveryJob instead.

Copy link
Member Author

@gmcgibbon gmcgibbon Dec 3, 2018

I think it is released in all versions since 5.1.0

Copy link
Member

@eileencodes eileencodes Dec 3, 2018

Ah I see I was thinking of this PR 251b9d4 which is only in master.

@rafaelfranca rafaelfranca added this to the 6.0.0 milestone Dec 3, 2018
@gmcgibbon gmcgibbon force-pushed the new_delivery_job branch from 4f557d3 to a72684c Dec 3, 2018
@gmcgibbon gmcgibbon changed the title Add NewDeliveryJob for unified mail delivery Add MailDeliveryJob for unified mail delivery Dec 3, 2018
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 3, 2018

Don't we need to add back Parameterized::DeliveryJob? Otherwise it will fail if a Rails 6 worker get a job enqueued in Rails 5.2.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 3, 2018

Ah, just saw it.

before_perform do
ActiveSupport::Deprecation.warn <<~MSG.squish
Sending mail with DeliveryJob is deprecated and
will be removed in Rails 6.1. Please use NewDeliveryJob instead.
Copy link
Member

@eileencodes eileencodes Dec 4, 2018

This should say MailDeliveryJob now. Also let's add back in the Paramertized deprecation since I was wrong about when it was added.

Add `MailDeliveryJob` for delivering both regular and parameterized mail.
Deprecate using `DeliveryJob` and `Parameterized::DeliveryJob`.
@gmcgibbon gmcgibbon force-pushed the new_delivery_job branch from a72684c to f5050d9 Dec 4, 2018
@gmcgibbon
Copy link
Member Author

@gmcgibbon gmcgibbon commented Dec 4, 2018

I think if the code isn't deprecated until 6.1 and the jobs work in 6.0 then we don't need a backport.

I'm fairly certain that (depending on the number of job workers you have, and your infrastructure) there can be big delays in waiting for all workers to be updated to the latest deployed version of an app. I believe I've seen an old job worker pickup a job enqueued from a new web worker before. Doesn't this mean we should backport MailDeliveryJob to a 5.x release?

@eileencodes eileencodes merged commit d76d8f7 into rails:master Dec 5, 2018
2 checks passed
@gmcgibbon gmcgibbon deleted the new_delivery_job branch Dec 5, 2018
require "active_job"

module ActionMailer
# The <tt>ActionMailer::NewDeliveryJob</tt> class is used when you
Copy link
Contributor

@mjankowski mjankowski Feb 25, 2019

It looks like this one mention of NewDeliveryJob stuck around even though most others were updated to MailDeliveryJob instead?

Copy link
Member

@rafaelfranca rafaelfranca Feb 25, 2019

Fixed. Thanks

Matthijsy added a commit to csvalpha/amber-api that referenced this issue Nov 25, 2019
github-actions bot added a commit to csvalpha/amber-api that referenced this issue Dec 7, 2019
* Update gemfile and run rails app:upgrade

* Fix optional belongs to in mail alias

* Temp disable bullet

* Fix scope deprecation warnings (rails/rails#35186)

* Fix mail delivery job deprecation (rails/rails#34591)

* Switch to load 6.0
- Fix deprecation warnings about initalizers
- Fix model_name eager loading
- Fix autoimport
- Fix carrierwave

* Cleanup

* Rubocop

* Re-enable bullet

* Fix bullet

* Update lockfile

* Update activity spec and rubocop-performance version

* rubocop

* Typo

* Remove secret token since it is not used since rails 6

* Remove carrierwave uploader

* Sentry.rb

* Add additional test case for accept mail
@mathieujobin
Copy link

@mathieujobin mathieujobin commented Apr 28, 2020

Hey started,

started getting this deprecation warning and I was wondering why...
didn't you want to update the default at https://github.com/rails/rails/blob/master/actionmailer/lib/action_mailer/base.rb#L460 ??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants