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

Fix parameter matching with mail delivery job and ActionMailer::MailDeliveryJob #2516

Closed

Conversation

fabn
Copy link
Contributor

@fabn fabn commented Sep 10, 2021

This should fix #2351.

Maybe it's a breaking change for Rails 6 users, I don't know but it will allow users to easily upgrade to rails 6 without changing tons of specs.

Let me know if it's ok, it's my first PR in rspec.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please feel free to locally disable Metrics/ClassLength for this class.

lib/rspec/rails/matchers/have_enqueued_mail.rb Outdated Show resolved Hide resolved
lib/rspec/rails/matchers/have_enqueued_mail.rb Outdated Show resolved Hide resolved
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! Thank you for the contribution.

@FriedSock
Copy link

Thank you @fabn! Can we merge this now? I'd love to have this change.

@fabn
Copy link
Contributor Author

fabn commented Oct 25, 2021

Thank you @fabn! Can we merge this now? I'd love to have this change.

I'm not a maintainer, I don't know when this will be merged. @pirj?

@pirj
Copy link
Member

pirj commented Oct 25, 2021

@mico @dmitry Does it fix your cases, too?

@pirj
Copy link
Member

pirj commented Oct 25, 2021

Please accept my apologies, I completely forgot about this PR @fabn

@fabn
Copy link
Contributor Author

fabn commented Nov 22, 2021

Should it be like that? Rails API docs suggest using a symbol for the key:

Copy pasted from existing examples, I can change them

@fabn
Copy link
Contributor Author

fabn commented Nov 22, 2021

@pirj Pushed a new commit to have different examples for each syntax

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for moving this forward.
A couple of small fixes/clarifications and it's good to go.

features/matchers/have_enqueued_mail_matcher.feature Outdated Show resolved Hide resolved
features/matchers/have_enqueued_mail_matcher.feature Outdated Show resolved Hide resolved
features/matchers/have_enqueued_mail_matcher.feature Outdated Show resolved Hide resolved
@pirj
Copy link
Member

pirj commented Nov 23, 2021

@mico @dmitry Ping. Does this PR fix your cases?

@pirj
Copy link
Member

pirj commented Nov 23, 2021

@fabn Thanks a lot, looks flawless to me.

@JonRowe Any objections to merging this?

@Haegin
Copy link

Haegin commented Dec 17, 2021

I've just run in to this when upgrading to Rails 6 myself - is there anything I can do to help get this over the line?

@pirj
Copy link
Member

pirj commented Dec 17, 2021 via email

@pirj
Copy link
Member

pirj commented Dec 17, 2021

@Haegin Please accept my apologies, I replied without looking.
I suggest you to use this branch in your Gemfile while the PR is not merged.

@dmitry
Copy link

dmitry commented Dec 17, 2021

@pirj on the next week going to check this branch in the project.

@dmitry
Copy link

dmitry commented Dec 20, 2021

Just tested and seems it works in the following cases:

expect {
          expect {
            request!
          }.to have_enqueued_mail(LeadMailer, :user_created)
                 .with(
                   a_hash_including(
                     params: {
                       user: instance_of(User)
                     }
                   )
                 )
        }.to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
               .with(
                 args: [
                   instance_of(User),
                   instance_of(String),
                   {}
                 ]
               )

Now it looks like this:

expect {
          expect {
            request!
          }.to have_enqueued_mail(LeadMailer, :user_created)
                 .with(
                   user: instance_of(User)
                 )
        }.to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
               .with(
                 instance_of(User),
                 instance_of(String),
                 {}
               )

Means it still works and I have to write less code.

👍 for the feature! 🚀

@pirj
Copy link
Member

pirj commented Dec 20, 2021

I had to make this change to make this change pass on both Rails 7 and Rails 6.0. Kindly appreciate your feedback.

@pirj
Copy link
Member

pirj commented Jan 10, 2022

Fixed in #2546. Thanks for your contribuion, @fabn !

@pirj pirj closed this Jan 10, 2022
@fabn fabn deleted the fix-parameter-matching-with-mail-delivery-job branch January 10, 2022 21:28
@fabn
Copy link
Contributor Author

fabn commented Jan 10, 2022

You're welcome

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

Successfully merging this pull request may close these issues.

have_enqueued_mail with options doesn't match
5 participants