Skip to content

Conversation

@cjpitch23
Copy link
Contributor

@cjpitch23 cjpitch23 commented Jun 29, 2021

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

  • 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.

  • I have read the CONTRIBUTING doc
  • 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...

Added bcc and cc to 'to' for MIME
@thetutlage
Copy link
Member

Why all recipients should be added to the to field?

@thetutlage
Copy link
Member

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

@cjpitch23
Copy link
Contributor Author

cjpitch23 commented Jun 30, 2021

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

Nope. And thanks for the fix

@thetutlage thetutlage merged commit ad97849 into adonisjs:develop Jun 30, 2021
@cjpitch23
Copy link
Contributor Author

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

@memsbdm memsbdm mentioned this pull request Mar 30, 2024
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.

2 participants