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

expect { }.to have_enqueued_mail().with() doesn't work with ActionMailer::MailDeliveryJob #2184

Closed
simmerz opened this issue Oct 9, 2019 · 35 comments

Comments

@simmerz
Copy link

simmerz commented Oct 9, 2019

What Ruby, Rails and RSpec versions are you using?

Ruby version: 2.5.1
Rails version: 6.0.0
RSpec version: branch 4-0-dev

Observed behaviour

Given an email enqueued with:

SomeMailer.with(object: foo, option: { amended: true }).some_method.deliver_later

I would expect the following to pass:

expect { some_action_which_enqueues_mail }.to have_enqueued_mail(SomeMailer, :some_method).with(object: foo, option: { amended: true })

It doesn't pass when setting the default delivery job to ActionMailer::MailDeliveryJob but does when using the legacy ActionMailer::Parameterized::DeliveryJob (ie. config.load_defaults 5.2 and Rails.application.config.action_mailer.delivery_job = 'ActionMailer::MailDeliveryJob' commented out in config/initializers/new_framework_defaults_6_0.rb

The listed enqueued mails shows the mail has been enqueued, but I cannot seem to access it with my test criteria.

@benoittgt
Copy link
Member

Hello @simmerz

Thanks for your issue. Could you provide a reproduction script or new Rails project?

Thanks

@simmerz
Copy link
Author

simmerz commented Oct 17, 2019

Hi @benoittgt - here's a blank rails project recreating the issue: https://github.com/simmerz/rspec_mailer_bug

@JonRowe
Copy link
Member

JonRowe commented Oct 17, 2019

@benoittgt I'm pretty certain this has to do with the class used, its just not detecting the mail jobs used in Rails 6.

@simmerz
Copy link
Author

simmerz commented Oct 17, 2019

@JonRowe without the with() it passes. Just not without, and it's specific to the new MailDeliveryJob class. The old ones work fine, but everything is being deprecated to be removed in Rails 6.1

@JonRowe
Copy link
Member

JonRowe commented Oct 17, 2019

@simmerz Interesting, then its probably the format of the arguments used, as thats how it changes when you use with https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/matchers/have_enqueued_mail.rb#L68

@simmerz
Copy link
Author

simmerz commented Oct 17, 2019

Failure output:

Failures:

  1) MailerTestService is expected to enqueues MailtestMailer.mailtest
     Failure/Error:
       expect { described_class.new.save }
         .to have_enqueued_mail(MailtestMailer, :mailtest)
         .with(option: { foo: 'bar' })
     
       expected to enqueue MailtestMailer.mailtest exactly 1 time with [{:option=>{:foo=>"bar"}}], but enqueued 0
       Queued deliveries:
         MailtestMailer.mailtest with [{"params"=>{"option"=>{"foo"=>"bar", "_aj_symbol_keys"=>["foo"]}, "_aj_symbol_keys"=>["option"]}, "args"=>[], "_aj_symbol_keys"=>["params", "args"]}]
     # ./spec/services/mailer_test_service_spec.rb:7:in `block (2 levels) in <top (required)>'

@JonRowe
Copy link
Member

JonRowe commented Oct 17, 2019

There we go then, it does some new mangling of arguments, previously they were just verbatim.

@simmerz
Copy link
Author

simmerz commented Oct 17, 2019

So that's something I should be fixing in my code, or should be handled in the with() expectation?

@JonRowe
Copy link
Member

JonRowe commented Oct 17, 2019

Something that needs to be added to the params building code in the matcher when Rails 6 is being used with this new mailer class

@JonRowe
Copy link
Member

JonRowe commented Oct 17, 2019

You could work around it for now with a_hash_including("params" => a_hash_including("option" => "bar"))

@JonRowe
Copy link
Member

JonRowe commented Oct 17, 2019

I don't have time to work on this myself right now, do you feel like tackling a PR or @benoittgt ?

@simmerz
Copy link
Author

simmerz commented Oct 17, 2019

Like you've described, if I use the following, the test passes:

.with(params: { option: { foo: 'bar' } }, args: [])

We need to handle someone creating a mail with params eg SomeMailer.with(params).mailer_method as well as with args eg SomeMailer.mailer_method(args)

I'm not sure what the best way to tackle those would be. Any pointers appreciated and I'd be happy to submit a PR

@JonRowe
Copy link
Member

JonRowe commented Oct 18, 2019

The tactic we've used before is to use the rails param parsing code itself to create the params we check against the queued ones.

@mkamensky
Copy link
Contributor

I just ran into the same issue (I think) and fixed it by changing

base_mailer_args + @mail_args
into:

base_mailer_args + [args: @mail_args]

I'm not sure this is the right fix, and that it works in the other configuration, but it worked for me.

@mkamensky
Copy link
Contributor

Also, this requires changing mail_job_message as follows:

        def mail_job_message(job)
          mailer_method = job[:args][0..1].join('.')

          msg_parts = []
          msg_parts << "with #{::ActiveJob::Arguments.deserialize(job[:args][3]['args'])}" if job[:args][3]
          msg_parts << "on queue #{job[:queue]}" if job[:queue] && job[:queue] != 'mailers'
          msg_parts << "at #{Time.at(job[:at])}" if job[:at]

          "#{mailer_method} #{msg_parts.join(', ')}".strip
        end

@simmerz
Copy link
Author

simmerz commented Nov 8, 2019

@mkamensky have you put it into a PR? If you fancy doing that, I'll test my codebase against your branch, to at least get some more validation on the method.

@mkamensky
Copy link
Contributor

@mkamensky have you put it into a PR? If you fancy doing that, I'll test my codebase against your branch, to at least get some more validation on the method.

Actually, I just tried running the specs, which failed, and also reread the comments here, and realised I probably don't understand how one is supposed to use this function. The example in the docs shows the following should pass:

expect {
  MyMailer.welcome(user).deliver_later
}.to have_enqueued_mail(MyMailer, :welcome).with(user)

As far as I can tell, this does pass with my modification, and does not with the current code. On the other hand, the tests expect the following to pass:

expect {                                                                 
         UnifiedMailer.with('foo' => 'bar').test_email.deliver_later            
       }.to have_enqueued_mail(UnifiedMailer, :test_email).with(                
         a_hash_including(:params => { 'foo' => 'bar' })                        
       )

and with my modification it does not. I don't know what UnifiedMailer is. At any rate, it seems clear that my modification is at most correct in some cases, so I don't think a PR is in order. But you can grab it from my repo, and at any rate, it is literally just what I wrote.

@JonRowe
Copy link
Member

JonRowe commented Nov 9, 2019

I'd imagine its because you tried replacing the method / args for the new Rails 6 style, but thats not the only style of args thats in use.

UnifiedMailer is a test class using ::ActionMailer::MailDeliveryJob

@mkamensky
Copy link
Contributor

Yes, that's what I thought, but I'm not familiar enough with the code to give a real solution here

@ybiquitous
Copy link

Hi, I try the following workaround and it works on my project:

Ruby version: 2.6.5
Rails version: 5.2.4.1

RSpec::Rails::Matchers::HaveEnqueuedMail.define_method(:mailer_job) do
  ActionMailer::Parameterized::DeliveryJob
end

It seems that PR #2125 has resolved it but not released yet.

@JonRowe
Copy link
Member

JonRowe commented Jan 10, 2020

@ybiquitous it should have been released as 4.0.0.beta3

@ybiquitous
Copy link

@JonRowe Oh, thank you! I'll try it.

@ybiquitous
Copy link

4.0.0.beta3 works fine on our project. Thank you so much! @JonRowe

@JonRowe
Copy link
Member

JonRowe commented Jan 10, 2020

@simmerz is this still an issue for you?

@pirj
Copy link
Member

pirj commented Feb 4, 2020

@simmerz Please feel free to reopen if you are still experiencing the issue.

@pirj pirj closed this as completed Feb 4, 2020
@simmerz
Copy link
Author

simmerz commented Mar 4, 2020

@pirj @JonRowe I still seem to be seeing an issue here with the unified mailer, though this could be me misunderstanding its use.

The following test passes:

      it 'sends joining instructions if there is a person' do
        booking.students.first.update person: person
        payment.charge
        expect { payment.save }
          .to have_enqueued_mail(BookingMailer, :joining_instructions)
          .with(params: { student: booking.students.first }, args: [])
      end

However, if i I strip out the args: [] from the expectation, it fails. I'd have expected, based on the docs, that I wouldn't need args, nor the params hash here?

@pirj
Copy link
Member

pirj commented Mar 5, 2020

@simmerz Do you mind setting a breakpoint here and see if there's something suspicious when you call have_enqueued_mail without args: [] in with?

@simmerz
Copy link
Author

simmerz commented Mar 5, 2020

Not at all:

[1] pry(#<RSpec::Rails::Matchers::HaveEnqueuedMail>)> job
=> {:job=>ActionMailer::MailDeliveryJob,
 :args=>
  ["BookingMailer",
   "joining_instructions",
   "deliver_now",
   {"params"=>{"student"=>{"_aj_globalid"=>"gid://myapp/Student/6781"}, "_aj_symbol_keys"=>["student"]}, "args"=>[], "_aj_symbol_keys"=>["params", "args"]}],
 :queue=>"mailers"}

[3] pry(#<RSpec::Rails::Matchers::HaveEnqueuedMail>)> @args
=> ["BookingMailer",
 "joining_instructions",
 "deliver_now",
 {:params=>
   {:student=>
     #<Student:0x00007fd3e46ab598
      id: 6781,
      person_id: 5426,
      booking_id: 2962,
      aasm_state: nil,
      created_at: Thu, 05 Mar 2020 11:41:13 GMT +00:00,
      updated_at: Thu, 05 Mar 2020 11:41:13 GMT +00:00,
      instructions_sent_at: nil,
      event_id: 6474,
      course_id: 7553,
      course_delivery_id: 7108>}}]

The error I see when running RSpec is (ignore the object IDs - they're from different runs):

  1) StudentsController POST #resend_instructions queues an email to the student
     Failure/Error:
       expect { req }.to have_enqueued_mail(BookingMailer, :joining_instructions)
         .with(params: { student: student })
     
       expected to enqueue BookingMailer.joining_instructions exactly 1 time with [{:params=>{:student=>#<Student id: 6797, person_id: 5442, booking_id: 2970, aasm_state: nil, created_at: "2020-03-05 12:03:17", updated_at: "2020-03-05 12:03:17", instructions_sent_at: nil, event_id: 6490, course_id: 7577, course_delivery_id: 7124>}}], but enqueued 0
       Queued deliveries:
         BookingMailer.joining_instructions with [{"params"=>{"student"=>{"_aj_globalid"=>"gid://myapp/Student/6795"}, "_aj_symbol_keys"=>["student"]}, "args"=>[], "_aj_symbol_keys"=>["params", "args"]}]

@pirj
Copy link
Member

pirj commented Mar 5, 2020

How does the part in payment.save related to enqueuing joining_instructions look like?
How does joining_instructions declaration look like in the mailer?
Trying to understand where those "args" => [] are coming from.

@simmerz
Copy link
Author

simmerz commented Mar 5, 2020

Both are of the form:

# payment.save -> via callbacks on the Student model
BookingMailer.with(student: self).joining_instructions.deliver_later
# Direct call from a controller (the resend instruction test above)
BookingMailer.with(student: @student).joining_instructions.deliver_later

@pirj
Copy link
Member

pirj commented Mar 5, 2020

Your code seems to be correct according to the guide, but I have not yet worked with parameterized mailers using a with notation before. In the ActionMailer API docs they use a classic notation.

# classic
NotifierMailer.welcome(User.first).deliver_later
# parameterized
UserMailer.with(user: @user).welcome_email.deliver_later

Can you please apply those changes:

- BookingMailer.with(student: self).joining_instructions.deliver_later
+ BookingMailer.joining_instructions(student: self).deliver_later
- BookingMailer.with(student: @student).joining_instructions.deliver_later
+ BookingMailer.joining_instructions(student: @student).deliver_later

and see if the following works:

expect { payment.save }
          .to have_enqueued_mail(BookingMailer, :joining_instructions)
          .with(student: booking.students.first)

(notice I've also dropped params).

If this passes as expected (and those two notations of sending mail have the same behavior), please file a new ticket.

@simmerz
Copy link
Author

simmerz commented Mar 12, 2020

@pirj so using your notation above, the test fails:

  1) StudentsController POST #resend_instructions queues an email to the student
     Failure/Error:
       expect { req }.to have_enqueued_mail(BookingMailer, :joining_instructions)
         .with(student: student)
     
       expected to enqueue BookingMailer.joining_instructions exactly 1 time with [{:student=>#<Student id: 7245, person_id: 5814, booking_id: 3155, aasm_state: nil, created_at: "2020-03-12 22:16:56", updated_at: "2020-03-12 22:16:56", instructions_sent_at: nil, event_id: 6945, course_id: 8099, course_delivery_id: 7591>}], but enqueued 0
       Queued deliveries:
         BookingMailer.joining_instructions with [{"args"=>[{"student"=>{"_aj_globalid"=>"gid://myapp/Student/7245"}, "_aj_symbol_keys"=>["student"]}], "_aj_symbol_keys"=>["args"]}]

The controller does the following:

BookingMailer.joining_instructions(student: @student).deliver_later

@JonRowe
Copy link
Member

JonRowe commented Mar 12, 2020

I mean thats an issue with job serialisation not matching the test arguments, are the new matchers not serialising arguments?

@pirj
Copy link
Member

pirj commented Mar 17, 2020

We don't have specs that would cover ActiveJob serialization, I'll double-check this @simmerz
Thanks from bringing this up.

slorek added a commit to OfficeForProductSafetyAndStandards/product-safety-database that referenced this issue Sep 29, 2020
slorek added a commit to OfficeForProductSafetyAndStandards/product-safety-database that referenced this issue Oct 1, 2020
slorek added a commit to OfficeForProductSafetyAndStandards/product-safety-database that referenced this issue Oct 2, 2020
slorek added a commit to OfficeForProductSafetyAndStandards/product-safety-database that referenced this issue Oct 5, 2020
* Upgrade Ruby version to 2.7.1, and necessary related CloudFoundry buildpack changes

* Fix deprecation warnings in Rspec suite

* Install Rails 6 and run update script

* Resolve conflict with multiple root paths defined

* Fix ActiveStorage queue names

* Fix for rspec-rails have_enqueued_mail matcher not working with ActionMailer::MailDeliveryJob as of Rails 6

See rspec/rspec-rails#2184

* Rubocop fixes

* Fix for change to ActiveStorage attach interface

* Fix preloading not working on decorated objects

* Fix attachment blobs being destroyed before audit activity generated

* Fix issue with updating test results comparing changes by reference to the modified object

* Fix for test queue adapter being used by default in request specs

* Set database adapter explicitly in production environment to avoid errors

* Remove redundant framework defaults causing loader errors

* Fix incorrect class name causing loader errors

* Ensure all forms are submitted correctly rather than with AJAX

* Bump govuk-design-system-rails to resolve deprecated usage warning

* Fix for change to save behavior when model has an attached ActiveStorage::Blob
scruti pushed a commit to OfficeForProductSafetyAndStandards/product-safety-database that referenced this issue Oct 8, 2020
* Upgrade Ruby version to 2.7.1, and necessary related CloudFoundry buildpack changes

* Fix deprecation warnings in Rspec suite

* Install Rails 6 and run update script

* Resolve conflict with multiple root paths defined

* Fix ActiveStorage queue names

* Fix for rspec-rails have_enqueued_mail matcher not working with ActionMailer::MailDeliveryJob as of Rails 6

See rspec/rspec-rails#2184

* Rubocop fixes

* Fix for change to ActiveStorage attach interface

* Fix preloading not working on decorated objects

* Fix attachment blobs being destroyed before audit activity generated

* Fix issue with updating test results comparing changes by reference to the modified object

* Fix for test queue adapter being used by default in request specs

* Set database adapter explicitly in production environment to avoid errors

* Remove redundant framework defaults causing loader errors

* Fix incorrect class name causing loader errors

* Ensure all forms are submitted correctly rather than with AJAX

* Bump govuk-design-system-rails to resolve deprecated usage warning

* Fix for change to save behavior when model has an attached ActiveStorage::Blob
@pirj
Copy link
Member

pirj commented Oct 21, 2022

For the future researchers, this have been fixed in various PRs (e.g. #2566) and released in 5.1.

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

No branches or pull requests

6 participants