Skip to content

Conversation

pirj
Copy link
Member

@pirj pirj commented Jan 28, 2022

I've cherry-picked #2566 to main.
Locally,on Ruby 3.1 it passed for 6.0 (to fix failing sub-builds) and for 7.0, too.

@pirj pirj self-assigned this Jan 28, 2022
@pirj
Copy link
Member Author

pirj commented Jan 29, 2022

Without fixes from #2552, 7.0 builds expectedly fail.

What fails on 6.1 and 6.0 are those 2 scenarios.
It seems it needs more specs for various combinations depending on self.delivery_job = ActionMailer::MailDeliveryJob setting like even the same Rails version can have a different global or even a mailer-local setting for a job that affects serialization, e.g. mentioned here, and in our own specs:

    class UnifiedMailer < ActionMailer::Base
      self.delivery_job = ActionMailer::MailDeliveryJob
# vs
      self.delivery_job = ActionMailer::DeliveryJob

I can hack on it next week.

My impression is that we won't be able to make enuque_mail to match with both with({'foo' => 'bar'}, 1, 2) and with(params: {'foo' => 'bar'}, args: [1, 2]), so this will be a breaking change. I admit recklessly merging #2546 without noticing this change of behaviour.

@JonRowe JonRowe force-pushed the fix-ruby-31-enqueue_mail-matcher branch from 423ea20 to 6fd11ab Compare March 7, 2022 11:04
@JonRowe JonRowe force-pushed the fix-ruby-31-enqueue_mail-matcher branch from 5174152 to b22a08b Compare March 14, 2022 10:13
@JonRowe JonRowe merged commit 5fe75e7 into main Mar 14, 2022
@JonRowe JonRowe deleted the fix-ruby-31-enqueue_mail-matcher branch March 14, 2022 10:43
@JonRowe
Copy link
Member

JonRowe commented Mar 14, 2022

@pirj I've merged this because I'm reconcilling the main branch with the released 5.1.1, my intent is to ressurrect the discussion around the changed behaviour in another branch on top of this

csutter added a commit to DFE-Digital/teaching-vacancies that referenced this pull request May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants