Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

Allow checked mailer recipients to also include bcc and cc emails #5

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

andyklimczak
Copy link
Contributor

@andyklimczak andyklimczak commented Nov 5, 2018

We've run into a few instances where we'd like to check the bcc and cc emails

  • Change checked recipients from only the mailer's .to recipient, to also check other recipients (.bcc, .cc)
  • Add new helper methods have_received_bcc_email and have_received_cc_email

@@ -17,7 +17,7 @@ module ActionMailerMatchers

def matching_mail_for(email, body, email_subject)
deliveries.detect do |mail|
has_expected_email = mail.to.include?(email)
has_expected_email = mail.destinations.include?(email)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about having more explicit expectations around this? I'm imagining something like this. Would this result in any false positives where we meant to send someone as the main to but instead sent it as bcc?

expect(user).to have_received_email(subject: "Subject")
expect(user).to have_received_cc_email(subject: "Subject")
expect(user).to have_received_bcc_email(subject: "Subject")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that approach, it's specifies the user email and the method that the email should be sent. I'll code it up

Copy link
Contributor

Choose a reason for hiding this comment

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

The updates look great!

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding a small update to the usage section explaining that there are two additional methods? https://github.com/contently/action_mailer_matchers#usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

@andyklimczak andyklimczak force-pushed the destinations branch 2 times, most recently from 09aa68e to 8ce60f3 Compare November 19, 2018 15:50
@andyklimczak
Copy link
Contributor Author

Added examples to the README and squashed ⛄️

Copy link
Contributor

@aaackerman aaackerman left a comment

Choose a reason for hiding this comment

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

Awesome addition, thank you!!! 🚢 I'm not sure if we have to do anything to cut a new version in rubygems?

@andyklimczak
Copy link
Contributor Author

I had one more change that I messed up with rebasing that I just force pushed (the expected var is only defined inside of matcher, so it must be passed to be accessed outside)
I set my Gemfile to look for this gem locally using this branch (using gem "action_mailer_matchers", path: "/tmp/action_mailer_matchers"), and triple verified that the new methods work in our test suite

I think we'll have to follow the steps in this guide to publish a new version. Did you originally push the gem to rubygems?

I don't have write permissions on this repo too, so I can't merge this PR

- Change checked recipients from only the mailer's `.to` recipient, to all recipients (`.bcc`, `.cc`)
- Add new helper methods `have_received_bcc_email` and `have_received_cc_email`
@andyklimczak
Copy link
Contributor Author

One more force push to correct commit message

@aaackerman
Copy link
Contributor

Thanks! I tested locally too, will merge and cut a new version.

@aaackerman aaackerman merged commit 5dc2d8f into trilogy-group:master Nov 19, 2018
@andyklimczak andyklimczak deleted the destinations branch November 19, 2018 21:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants