ActionMailer::MessageDelivery respects current I18n.locale

[xijo]

Fixes #20774

When #deliver_now is called all translations within the
generated email will be looked up for the current I18n
locale.

I18n.locale = ‘de’
mail.deliver_now # Generates german email, correct

In #enqueue_delivery the locale was not considered and
the resulting job uses the default locale.

I18n.locale = ‘de’
mail.deliver_later # Generate english email, incorrect

In order to achieve a consistent behaviour the current locale
is now always passed to ActionMailer::DeliveryJob.

[kaspth]

Can you add a changelog entry for Action Mailer at the top of the file? Otherwise LGTM 😄

[xijo]

@kaspth I added the changelog entry, thanks for the review. 👍

[kaspth]

Great, now you just have to squash your commits down to one 😄

[xijo]

@kaspth Right, done. Thanks for the reminder! :)

[kaspth]

Thanks ❤️

[matthewd]

@kaspth it seems undesirable (and impractical) to require everyone to drain their AJ queue prior to upgrade... thoughts?

[xijo]

@matthewd You're right, it'd be a breaking change if there are still mails on the queue.

One solution would be to implement a new job class that expects the locale parameter and leave the old one untouched. This way it can still send out the remaining jobs on the queue. It could then safely be removed later.

@kaspth What do you think?

I'd make a second PR if both of you agree that this is the better solution.

[kaspth]

@matthewd nice catch, I missed that.

@xijo can we extend the existing class to be more resilient i.e. don't break if the locale isn't there?

[xijo]

@kaspth I don't see how. Can't use default parameters because of the splat, can't move it into the optional parameters, because it would still be missing for those jobs that were stored before the update.

But maybe this job is the wrong point to implement it in anyway. I wonder if this behaviour shouldn't be part of ActiveJob::Base itself, since all enqueued jobs lose their current locale atm. What do you think?

[kaspth]

@xijo I've reverted this for now, until we figure out a better upgrade path. Sorry for merging too soon, I'm still new at this.
Though I'm confident we'll figure this out ❤️

[xijo]

@kaspth Thanks for reverting. 👍
I created a new PR #20800 to fix this issue inside of ActiveJob itself. Would you take a look at it?

@matthewd Thanks again for pointing the compatibility issue out. I think in #20800 I found a good way to work around it and I'd love to hear your opinion as well.