Fixed issue with Mailgun driver

cjpitch23 commented 3 years ago

Added bcc and cc to 'to' for MIME

Proposed changes

Added bcc and cc recipients to the to key on the form for MIME to be able to process and send out emails.

Types of changes

What types of changes does your code introduce?

Put an x in the boxes that apply

[x] Bugfix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

[x] I have read the CONTRIBUTING doc
[x] Lint and unit tests pass locally with my changes
[ ] I have added tests that prove my fix is effective or that my feature works.
[ ] I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

thetutlage commented 3 years ago

Why all recipients should be added to the to field?

thetutlage commented 3 years ago

Also, please add appropriate tests. Your change is a black box right now.

cjpitch23 commented 3 years ago

Why all recipients should be added to the to field?

I was having an issue where recipients in the bcc field weren't being sent the email. I looked at the Mailgun API and the messages.mime route requires that bcc and cc recipients be added to the to parameter.

https://documentation.mailgun.com/en/latest/api-sending.html#sending

Maybe I am misunderstanding it, but whenever I changed the code, I didn't have any more issues.

Also, please add appropriate tests. Your change is a black box right now.

I will add the tests.

thetutlage commented 3 years ago

Yup, its always nice to share the source of your information. Otherwise I have no idea why the change was being made.

Following is the relevant source for the change

Screenshot 2021-06-30 at 9 10 17 AM

cjpitch23 commented 3 years ago

Yup, its always nice to share the source of your information. Otherwise I have no idea why the change was being made.

Haha Yeah I suppose a screen shot of the information would have been the more direct approach than just linking the documentation. I'm just happy I helped out a little bit.

cjpitch23 commented 3 years ago

I ran the code through the linter and corrected the semi colon issue. I ran the tests and they all cleared. Since the bugfix is covered by send email using mailgun driver test, it seems to be excessive to add another test just to test the recipients in the mailgun driver..

cjpitch23 commented 3 years ago

These tests are failing because there is no .env with the api keys for the different mail drivers, right? Am I suppose to do something to fix that?

thetutlage commented 3 years ago

Nope. And thanks for the fix

cjpitch23 commented 3 years ago

I'm glad I could contribute a little bit. Thank you guys, Adonis is awesome.

adonisjs / mail