Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

add email service with nodemailer #146

Merged

Conversation

eolculnamo2
Copy link
Contributor

@eolculnamo2 eolculnamo2 commented Oct 26, 2019

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the master branch of Chapter.

Creates Email Service using nodemailer that will be used later for our email tickets.

Dependencies Added:

  1. nodemailer
  2. chai
  3. sinon
  4. sinon-chai.

package.json Outdated Show resolved Hide resolved
server/services/MailerService.ts Show resolved Hide resolved
@timmyichen timmyichen mentioned this pull request Oct 28, 2019
5 tasks
@vaibhavsingh97 vaibhavsingh97 self-requested a review October 28, 2019 13:43
Copy link
Member

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor changes

Note to self: Have to raise another issue, as according to @QuincyLarson comment, we have to support popular options like Amazon SES, SendGrid, and Mandrill.

@@ -80,6 +80,9 @@ Install dependencies:
npm install
```

Setup Environment Variables:
Copy and paste env.sample into your .env file
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly don't know much at all about Docker -- Do you mean that we should use environment variables in Docker Compose for default variables that can be overwritten by an .env file?

@allella
Copy link
Contributor

allella commented Nov 14, 2019

@vaibhavsingh97 @eolculnamo2 would we need to enable all the transports, by default, to support the common options?

Looks like SMTP is the default and then SES and sendmail (local relay) need to be enabled.

So, would we be installing everything even if people only will use one transport?

@jacobbogers
Copy link

Hi, I can look into this

@jacobbogers
Copy link

jacobbogers commented Nov 16, 2019

Hi @ALL, I need to dive into node-mailer ( I am only familiar with mailgun-js ), for me to make any sensible decision about design.. I will take this opportunity to learn node-mailer very well,

@vaibhavsingh97
Copy link
Member

would we need to enable all the transports, by default, to support the common options?

Nope, by default only SMTP will be enabled, we will just provide options to enable all other providers.

Would we be installing everything even if people only will use one transport?

Nope, We will give options to users, if they want to use other services.

@allella
Copy link
Contributor

allella commented Nov 17, 2019

Nope, by default only SMTP will be enabled, we will just provide options to enable all other providers

How would installing new mailer packages work for the app? Would the administrator need to run a command on the server, or is there a way to install new packages directly from the application admin UI and/or configuration files?

@jacobbogers
Copy link

jacobbogers commented Nov 17, 2019

@allella nodemailer uses plugins for non SMTP transports (has some build in), to avoid manually installing them, they need to already exist, its best to view these diff plugins as a "preset" (needs its own config page to fill in params) to be selected, I am looking into that as well to put in MVP, it seems to be very trivial to add in the various plugins for node-mailer, still investigating...

@vaibhavsingh97
Copy link
Member

@eolculnamo2 Can you please resolve conflicts so that we can merge 😄

@eolculnamo2
Copy link
Contributor Author

Will do :)

@vaibhavsingh97 vaibhavsingh97 self-requested a review November 18, 2019 17:19
@vaibhavsingh97
Copy link
Member

Also, Can you please resolve @ScottBrenner comment, also as you had added mocha and chai for test frameworks, so can you please add the same in the readme here and here.

Copy link
Member

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

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

LGTM

@allella
Copy link
Contributor

allella commented Feb 23, 2020

We had more current conversation in today's meeting on using nodemailer and SMTP with free / cheap tiers of 3rd party transactional mailing services.

@allella allella added the Email label Aug 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants