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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add assert_enqueued_email_with to ActionMailer::TestHelper #30695

Merged
merged 1 commit into from Sep 27, 2017
Merged

Add assert_enqueued_email_with to ActionMailer::TestHelper #30695

merged 1 commit into from Sep 27, 2017

Conversation

@mikker
Copy link
Contributor

@mikker mikker commented Sep 24, 2017

Summary

A proposal to add assert_enqueued_email_with to ActionMailer::TestHelper. We already have assert_enqueued_email(amount) and although that certainly is sufficient for most cases it's sometimes nice to be a little bit more specific in what you're expecting.

I couldn't get the test suite to run without an exception that I couldn't diagnose so I hope this passes CI.
Got the suite to run with bundle exec 馃檮
@kaspth eventually told me about bin/test. Is this documented anywhere? 馃槃

Should probably add a test case for something with arguments as well.
Added a test with arguments. Added support for parameterized emails.

@rails-bot
Copy link

@rails-bot rails-bot commented Sep 24, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @javan (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

Copy link
Member

@kaspth kaspth left a comment

Thanks! Haven't had a need for this myself. So I'll defer merge decision to someone else. 馃槉

@georgeclaghorn you added the assert_enqueued_emails way back when. How does this strike you?

args = args && [mailer.to_s, method.to_s, "deliver_now", *args]

assert_enqueued_with(
job: ActionMailer::DeliveryJob,
Copy link
Member

@kaspth kaspth Sep 25, 2017

Mailers can also be enqueued with ActionMailer::Parameterized::DeliveryJob.

Copy link
Contributor Author

@mikker mikker Sep 26, 2017

Thanks, @kaspth! Suggestions on how to handle both are very welcome.

Copy link
Contributor

@georgeclaghorn georgeclaghorn Sep 26, 2017

You could add a separate helper: assert_enqueued_parameterized_email_with.

Alternatively, this helper could assert that an ActionMailer::Parameterized::DeliveryJob is enqueued if args is Hash-like or responds to to_hash.

@javan
Copy link
Member

@javan javan commented Sep 26, 2017

@rails-bot rails-bot assigned georgeclaghorn and unassigned javan Sep 26, 2017
@mikker
Copy link
Contributor Author

@mikker mikker commented Sep 27, 2017

I took your second suggestion @georgeclaghorn 鈥 thanks for your feedback!

# to be enqueued.
#
# def test_emails_again
# assert_enqueued_email_with ContactMailer, :welcome
Copy link

@walf443 walf443 Sep 27, 2017

assert_enqueued_email_with ContactMailer, :welcome do
you forgot do?

Copy link
Contributor Author

@mikker mikker Sep 27, 2017

馃檮

Copy link
Contributor

@georgeclaghorn georgeclaghorn left a comment

Could you please squash your commits into one?

@@ -93,6 +93,43 @@ def assert_enqueued_emails(number, &block)
assert_enqueued_jobs number, only: [ ActionMailer::DeliveryJob, ActionMailer::Parameterized::DeliveryJob ], &block
end

# Asserts that a specific email has been enqueued, optionally
# also matching arguments.
Copy link
Contributor

@georgeclaghorn georgeclaghorn Sep 27, 2017

鉁傦笍 also

# def test_emails
# ContactMailer.welcome.deliver_later
# assert_enqueued_email_with ContactMailer, :welcome
# end
Copy link
Contributor

@georgeclaghorn georgeclaghorn Sep 27, 2017

Given that the description above alludes to passing args, we should include an example of that.

# end
# end
#
# If a hash is passed as arguments the email is presumed to be Paramererized.
Copy link
Contributor

@georgeclaghorn georgeclaghorn Sep 27, 2017

If `args` is provided as a Hash, a parameterized email is matched.

@mikker
Copy link
Contributor Author

@mikker mikker commented Sep 27, 2017

@georgeclaghorn Done and done. Thanks!

assert_enqueued_email_with TestHelperMailer, :test_parameter_args, args: { all: "good" } do
silence_stream($stdout) do
TestHelperMailer.with(all: "good").test_parameter_args.deliver_later
puts enqueued_jobs.inspect
Copy link
Contributor

@georgeclaghorn georgeclaghorn Sep 27, 2017

Why is this puts needed?

Copy link
Contributor Author

@mikker mikker Sep 27, 2017

oh god forgot that in there. I need to concentrate.

@georgeclaghorn
Copy link
Contributor

@georgeclaghorn georgeclaghorn commented Sep 27, 2017

Last thing: this needs a changelog entry. Mind adding one?

@mikker
Copy link
Contributor Author

@mikker mikker commented Sep 27, 2017

Of course! Like so?

@georgeclaghorn
Copy link
Contributor

@georgeclaghorn georgeclaghorn commented Sep 27, 2017

The entry itself looks good, but new entries should be added to the top of the file.

@mikker
Copy link
Contributor Author

@mikker mikker commented Sep 27, 2017

Does it show that this is my first contribution?

@georgeclaghorn georgeclaghorn merged commit 212cf23 into rails:master Sep 27, 2017
1 of 2 checks passed
@georgeclaghorn
Copy link
Contributor

@georgeclaghorn georgeclaghorn commented Sep 27, 2017

Thanks!

@kaspth
Copy link
Member

@kaspth kaspth commented Sep 28, 2017

@mikker looks like http://guides.rubyonrails.org/contributing_to_ruby_on_rails.html#running-tests could mention bin/test instead of the rake test task 馃槉

tomrossi7
Copy link
Contributor

tomrossi7 commented on db6847d Apr 7, 2018

I can't figure out how to get the syntax to work for asserting that an email has been enqueued properly when the argument is an object.

Should this be the syntax?

  assert_enqueued_email_with ContactMailer, :welcome, args: @contact do
    ContactMailer.welcome(@contat).deliver_later
  end
mikker
Copy link
Contributor

mikker commented on db6847d Apr 8, 2018

@tomrossi7 maybe

  assert_enqueued_email_with ContactMailer, :welcome, args: [@contact] do
    ContactMailer.welcome(@contat).deliver_later
  end

?

tomrossi7
Copy link
Contributor

tomrossi7 commented on db6847d Apr 9, 2018

@mikker here is the real code:

assert_enqueued_email_with DonationMailer, :receipt, args: [Donation.last] do
   Donation.process!
end

No enqueued job found with {:job=>ActionMailer::DeliveryJob, :args=>["DonationMailer", "receipt", "deliver_now", #<Donation id: 1015983123, donor_id: 438625875, created_at: "2018-04-09 13:53:49", updated_at: "2018-04-09 13:53:49">], :queue=>"mailers"}

It looks like it just inspected the object in the args. The actual job looks like this:

{:job=>ActionMailer::DeliveryJob, :args=>["DonationMailer", "receipt", "deliver_now", {"_aj_globalid"=>"gid://donortools2/Donation/1015983177"}], :queue=>"mailers"}

It looks like ActiveJob creates its own version of the object with a _aj_globalid?

mikker
Copy link
Contributor

mikker commented on db6847d Apr 9, 2018

Yes, it definitely does. The arguments no to be serialized to be put into (and taken out of) the database. This is why Rails has GlobalID. Before that you were advised to user contact_id and the like and never a real <Contact> object.

Not sure what to do here. Maybe try creating a GlobalID manually and passing that?

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

Successfully merging this pull request may close these issues.

None yet

7 participants