MailGun - Send Bulk

[bakura10]

I'll have a look at it when I got home but something looks wrong. To me bulkMessage implies sending multiple messages, but you passs only one message, method name sounds inconsistent to what it is really doing.

Also, there is a lot of duplicate code regarding the send message. Please have a look at the mandrill service which have a similar case (two sends methods)

[nikolajlovenhardt]

@bakura10 The email is working as a template, and then you fill in variables for the specific emails. For example if you want to send out an activation key for a group when something has been activated, and all have to get their individual keys. In the body of the mail you would then be able to write %recipient.key%. Would it be better to name it batch then?

I will fix the problem about duplicate code.

Okay, I will seperate it from the ValidOptions array.

[bakura10]

Ok, so I suppose there is no need to actually having a separate send method. You can fit everything in the send method. Check if the recipient variable is empty, and if not the case, you can populate the variables.

[juriansluiman]

I'd also would extract the recipient variables into a separate property (like a tag). The variables are not really an "option" to the end users of SlmMail. We can have a specific getter/setter for these options. Some options (I'm not sure if these are OK, other suggestions are fine):

setRecipientVariables($recipient, array $variables){ ... }
addRecipientVariable($recipient, $name, $value) { ... }
getRecipientVariables() { ... }
...

[bakura10]

Additionally, please update the doc here to show an example how this works: https://github.com/juriansluiman/SlmMail/blob/master/docs/Mailgun.md#supported-functionalities

[juriansluiman]

@Nikolajpetersen reading the docs of the recipient variable now, I read all recipients are identified by their email address from the "to" field. So, it might be a good idea to check whether $recipient actually is an address in to the "to" address list?

[nikolajlovenhardt]

@juriansluiman Yes, I will add the check. @bakura10 I will add an example.

[juriansluiman]

@Nikolajpetersen I really hope you don't mind we're so nitpicky about the code. It's not meant to discourage you!

[nikolajlovenhardt]

@juriansluiman It's my first PR, so I want to learn as much as possible.

[bakura10]

Looks very good!

@juriansluiman , do you have a mail gun account to try it? (or @Nikolajpetersen , can you carefully try the method in all cases to see if it works as expected) ?

Once that's confirmed, we can merge and I'll tag this as 1.4.0 ;).

[nikolajlovenhardt]

@bakura10 I've tested the send method both with and without recipient variables and confirmed that it throws an exception, when the e-mail is not recognized.

[bakura10]

Ok, can you confirm it still works when everything is valid for both with and without recipient variables ?

Envoyé de mon iPhone

Le 4 avr. 2014 à 13:05, Nikolaj Petersen notifications@github.com a écrit :

@bakura10 I've tested the send method both with and without recipient variables and confirmed that it throws an exception, when the e-mail is not recognized.

— Reply to this email directly or view it on GitHub.

[nikolajlovenhardt]

@bakura10 Yes.

[bakura10]

Nice. I'll merge after eating and tagging :)

Envoyé de mon iPhone

Le 4 avr. 2014 à 13:27, Nikolaj Petersen notifications@github.com a écrit :

@bakura10 Yes.

— Reply to this email directly or view it on GitHub.

[juriansluiman]

@bakura10 please wait a sec, there a few things which haven't been corrected or should be updated due to the latest changes. I go once through the patch again and comment on the latest things. If all of those are resolved, it's OK to merge

[bakura10]

lol, I just merged u_u.

[juriansluiman]

Lol, that was fast ;) Please wait to tag the new version, I'll do a PR to fix the latest things myself then

[bakura10]

Ok :). I'll wait for it then