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

Fix #25: Implement basic email sender #93

Merged
merged 11 commits into from
Nov 14, 2019
Merged

Conversation

RyanWils
Copy link
Contributor

@RyanWils RyanWils commented Nov 7, 2019

Contributes to issue #25

I created a basic email sender using Nodemailer.
To use the function, private information(email/pass,recipients) must be filled once the .env variables are set up.

refer to issue #82 for the tests.

I have also left a comment at the top of the file on how to use this file.

  1. import the file (const sendEmail = require('./email-sender);)
  2. send email (sendEmail.sendMessage("Put your error message in here");)

@humphd
Copy link
Contributor

humphd commented Nov 7, 2019

@RyanWils you can't share username/password pairs on GitHub like this. Please get rid of this account.

src/email-sender.js Outdated Show resolved Hide resolved
@RyanWils
Copy link
Contributor Author

RyanWils commented Nov 7, 2019

I apologies for putting private information on the previous commit.
I have removed the personal information in the newest commit and also deleted that test email I was using.
It seems like there is an issue open for the .env variables - issue #60
I have left the variables blank for when this issue is completed.

@humphd
Copy link
Contributor

humphd commented Nov 9, 2019

I wonder if we should use something like https://sendgrid.com/ for the mail transport layer. Creating an account in GMail or the like isn't sustainable when it's shared among so many users. There might be other services too, but this is one that came to mind, since we could use it long term for free.

@RyanWils
Copy link
Contributor Author

RyanWils commented Nov 9, 2019

Thank you for the suggestion, it seems like a good choice (40,000 emails for the first 30 days, then 100/day forever. should be plenty). Ya you make a good point, I think its best to switch from Nodemailer

@humphd
Copy link
Contributor

humphd commented Nov 9, 2019

@RyanWils I'm not sure if it's what we want or not, but something for you to look at. I think you could use it with nodemailer, or use their node.js API. Needs research.

@humphd
Copy link
Contributor

humphd commented Nov 9, 2019

https://www.sendinblue.com/ is another one, there are a bunch.

@humphd
Copy link
Contributor

humphd commented Nov 9, 2019

Using one of these services, for devs who want to hack on the email portion of the code, they could sign up for their own API key, put it in their .env and then they could work on that part of the code. In production, we'd use a different one.

@RyanWils
Copy link
Contributor Author

RyanWils commented Nov 9, 2019

Ya that would be for the best. For whichever service I pick, I will make sure to document the set up steps.

@RyanWils
Copy link
Contributor Author

RyanWils commented Nov 10, 2019

With Sendgrid is seems like the Seneca Email servers block from emails coming from the outside. I have tested sending an email using Nodemailer to seneca's email and it works
image

Even sending to a regular hotmail, it says it sends. But you never receive the email.
I have sent dozens of emails and have no idea why it never reaches my inbox even those it says it was properly delivered to the outlook servers.
image

Its seems that delivered, does not actually mean that the email has reached the users inbox which is unreliable.

In my opinion, I think its best to stick with Nodemailer as it seems more liable even though you would have to type your password/username in the .env file.

@RyanWils
Copy link
Contributor Author

I messed up on the titles of my commits for the Travis CI fixes as it references other topics.
I have tried to reword my comments with git rebase -i master, but I cannot as the head is detached.
Anyone have any experience with this?

@c3ho
Copy link
Contributor

c3ho commented Nov 12, 2019

@RyanWils I'm not sure if this may help, but it sounds like you went back to some previous commits and changed from there.

@RyanWils
Copy link
Contributor Author

Hi Calvin thanks for the solution, but sadly that solution did not work as my master is up to date. I created this pull request 6 days ago, so a lot of stuff got merged and even though I pulled into my master and rebased my branch into master it still says its detached.

Screenshots of the issue
image

says this for all the commits
image

src/email-sender.js Outdated Show resolved Hide resolved
src/email-sender.js Outdated Show resolved Hide resolved
src/email-sender.js Outdated Show resolved Hide resolved
service: 'gmail',
auth: {
user: '', // Email Name (.env variables must be used refer to issue#60)
pass: '', // Email Pass (.env variables must be used refer to issue#60)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do SMTP_SERVER, SMTP_USERNAME, SMTP_PASSWORD as environment variables in env.sample, and you can read them from process.env

src/email-sender.js Outdated Show resolved Hide resolved
manekenpix
manekenpix previously approved these changes Nov 14, 2019
Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

Looks great!

humphd
humphd previously requested changes Nov 14, 2019
env.example Show resolved Hide resolved
cindyorangis
cindyorangis previously approved these changes Nov 14, 2019
neilong31
neilong31 previously approved these changes Nov 14, 2019
Copy link
Contributor

@neilong31 neilong31 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for adding params to the send message function, as well as returning a reject or resolve for our transporter.sendMail() function.

src/email-sender.js Outdated Show resolved Hide resolved
src/email-sender.js Outdated Show resolved Hide resolved
@RyanWils RyanWils dismissed humphd’s stale review November 14, 2019 23:32

Hi Dave, I have made the changes

@RyanWils RyanWils merged commit 70c1290 into Seneca-CDOT:master Nov 14, 2019
@RyanWils RyanWils deleted the issue-25 branch November 14, 2019 23:34
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.

7 participants