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

Mandrill Cc/Bcc handling #31

Merged
merged 4 commits into from
Jul 3, 2014
Merged

Mandrill Cc/Bcc handling #31

merged 4 commits into from
Jul 3, 2014

Conversation

Ninir
Copy link
Contributor

@Ninir Ninir commented Jul 2, 2014

Hi,

This adds the implementation of Cc/Bcc for Mandrill. There were 2 ways to do it, as exposed on the Mandrill API doc:

  • Updating the identity to set a type, which is then parsed by Mandrill.
  • Adding a bcc_address option

I found the first one more maintainable. Not sure how to handle tests in a proper way.

@Ninir
Copy link
Contributor Author

Ninir commented Jul 2, 2014

As @stof worked on the Identity stuff, I think he could get his mind on this.

I would also add that this is an urgent work, so I will be reactive on this. Thanks for your time and patience.

@stof
Copy link
Member

stof commented Jul 3, 2014

I don't see the reason to modify the Identity (thus, adding a method in IdentityInterface is a bC break).

The Mandrill adapter could simply loop over $tos, $ccs and $bccs separately instead of merging them together and then needing to distinguish where the came from.

Thus, your logic is broken: if I set an Identity object in BCC without setting its type, you will still handle it as a To (actually, even for others as you don't set the type when normalizing either) because you use the type from the identity rather than the source where they come from.

@stof
Copy link
Member

stof commented Jul 3, 2014

and MandrillTest.php is testing the formatting of messages. This is the place where you should add a test using Cc and Bcc

@Ninir
Copy link
Contributor Author

Ninir commented Jul 3, 2014

@stof At the moment, I don't have time to do a big work on this... this was a 20minutes work. Could you suggest your way to proceed? I would gladly come with a new version if I could have hints on this. Thanks.

@stof
Copy link
Member

stof commented Jul 3, 2014

@Ninir the way to send the type as To, even for Bcc and Cc. This is why addign a test is important.

and for the implementation, just keep the existing loop processing To recipients, and add 2 new loops after that for Cc and Bcc

@Ninir
Copy link
Contributor Author

Ninir commented Jul 3, 2014

@stof Added some tests to handle cases. Understood what you meant, and it's even lighter compared to what was done previously. Thanks for this.

@@ -5,7 +5,7 @@
use Stampie\Adapter\AdapterInterface;

/**
* Takes a MailerInterface and sends to to Postmark throgh Buzz
* Takes a MailerInterface and sends to Postmark through Buzz
Copy link
Member

Choose a reason for hiding this comment

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

Postmark and Buzz should not be mentionned here actually:

  • we have more HTTP adapters than Buzz (currently Guzzle3)
  • we have more implementations of this interface than just Postmark

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the message to be more generic.

@stof
Copy link
Member

stof commented Jul 3, 2014

👍

@henrikbjorn
Copy link
Contributor

Thanks both :)

henrikbjorn added a commit that referenced this pull request Jul 3, 2014
Mandrill Cc/Bcc handling
@henrikbjorn henrikbjorn merged commit 26e433b into Stampie:master Jul 3, 2014
@Ninir Ninir deleted the bcc branch July 3, 2014 13:46
@stof
Copy link
Member

stof commented Jul 3, 2014

@henrikbjorn it would be great to do a 0.8.0 release with this

@henrikbjorn
Copy link
Contributor

@Ninir
Copy link
Contributor Author

Ninir commented Jul 3, 2014

A big thank @henrikbjorn @stof :)

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.

3 participants