From c52fd1df9f2ee04f91b4cb783ca7a87c9627fb58 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Sun, 7 Feb 2016 17:35:47 -0600 Subject: [PATCH] Refactor mail service Closes #5350 - No longer necessary to initialize via async init(). - Adds a startup-check for mail configuration. - Creates a notification in the admin client if mail transport is "direct" and sending a message fails. --- core/server/api/mail.js | 53 +++++++++----- core/server/index.js | 33 --------- core/server/mail/index.js | 34 +++------ core/server/utils/startup-check.js | 18 +++++ core/test/integration/api/api_mail_spec.js | 22 ++---- core/test/unit/mail_spec.js | 83 ++++++++++++++-------- 6 files changed, 125 insertions(+), 118 deletions(-) diff --git a/core/server/api/mail.js b/core/server/api/mail.js index 01d92f55ba..7ae4bb10ca 100644 --- a/core/server/api/mail.js +++ b/core/server/api/mail.js @@ -1,20 +1,24 @@ // # Mail API // API for sending Mail -var _ = require('lodash').runInContext(), - Promise = require('bluebird'), - pipeline = require('../utils/pipeline'), - config = require('../config'), - errors = require('../errors'), - mailer = require('../mail'), - Models = require('../models'), - utils = require('./utils'), - path = require('path'), - fs = require('fs'), - templatesDir = path.resolve(__dirname, '..', 'mail', 'templates'), - htmlToText = require('html-to-text'), - readFile = Promise.promisify(fs.readFile), - docName = 'mail', - i18n = require('../i18n'), +var _ = require('lodash').runInContext(), + Promise = require('bluebird'), + pipeline = require('../utils/pipeline'), + config = require('../config'), + errors = require('../errors'), + GhostMail = require('../mail'), + Models = require('../models'), + utils = require('./utils'), + notifications = require('./notifications'), + path = require('path'), + fs = require('fs'), + templatesDir = path.resolve(__dirname, '..', 'mail', 'templates'), + htmlToText = require('html-to-text'), + readFile = Promise.promisify(fs.readFile), + docName = 'mail', + i18n = require('../i18n'), + mode = process.env.NODE_ENV, + testing = mode !== 'production' && mode !== 'development', + mailer, mail; _.templateSettings.interpolate = /{{([\s\S]+?)}}/g; @@ -24,10 +28,23 @@ _.templateSettings.interpolate = /{{([\s\S]+?)}}/g; */ function sendMail(object) { - return mailer.send(object.mail[0].message).catch(function (err) { - err = new errors.EmailError(err.message); + if (!(mailer instanceof GhostMail) || testing) { + mailer = new GhostMail(); + } - return Promise.reject(err); + return mailer.send(object.mail[0].message).catch(function (err) { + if (mailer.state.usingDirect) { + notifications.add({notifications: [{ + type: 'warn', + message: [ + i18n.t('warnings.index.unableToSendEmail'), + i18n.t('common.seeLinkForInstructions', + {link: 'http://support.ghost.org/mail'}) + ].join(' ') + }]}, {context: {internal: true}}); + } + + return Promise.reject(new errors.EmailError(err.message)); }); } diff --git a/core/server/index.js b/core/server/index.js index 3afc6abfe0..4bcabe48e9 100644 --- a/core/server/index.js +++ b/core/server/index.js @@ -14,7 +14,6 @@ var express = require('express'), config = require('./config'), errors = require('./errors'), helpers = require('./helpers'), - mailer = require('./mail'), middleware = require('./middleware'), migrations = require('./data/migration'), models = require('./models'), @@ -91,34 +90,6 @@ function builtFilesExist() { return Promise.all(deferreds); } -// This is run after every initialization is done, right before starting server. -// Its main purpose is to move adding notifications here, so none of the submodules -// should need to include api, which previously resulted in circular dependencies. -// This is also a "one central repository" of adding startup notifications in case -// in the future apps will want to hook into here -function initNotifications() { - if (mailer.state && mailer.state.usingDirect) { - api.notifications.add({notifications: [{ - type: 'info', - message: [ - i18n.t('warnings.index.usingDirectMethodToSendEmail'), - i18n.t('common.seeLinkForInstructions', - {link: 'http://support.ghost.org/mail'}) - ].join(' ') - }]}, {context: {internal: true}}); - } - if (mailer.state && mailer.state.emailDisabled) { - api.notifications.add({notifications: [{ - type: 'warn', - message: [ - i18n.t('warnings.index.unableToSendEmail'), - i18n.t('common.seeLinkForInstructions', - {link: 'http://support.ghost.org/mail'}) - ].join(' ') - }]}, {context: {internal: true}}); - } -} - // ## Initialise Ghost // Sets up the express server instances, runs init on a bunch of stuff, configures views, helpers, routes and more // Finally it returns an instance of GhostServer @@ -161,8 +132,6 @@ function init(options) { return Promise.join( // Check for or initialise a dbHash. initDbHashAndFirstRun(), - // Initialize mail - mailer.init(), // Initialize apps apps.init(), // Initialize sitemaps @@ -173,8 +142,6 @@ function init(options) { }).then(function () { var adminHbs = hbs.create(); - // Output necessary notifications on init - initNotifications(); // ##Configuration // return the correct mime type for woff files diff --git a/core/server/mail/index.js b/core/server/mail/index.js index 048d678a59..4d9efb4452 100644 --- a/core/server/mail/index.js +++ b/core/server/mail/index.js @@ -7,31 +7,17 @@ var _ = require('lodash'), config = require('../config'), i18n = require('../i18n'); -function GhostMailer(opts) { - opts = opts || {}; - this.transport = opts.transport || null; +function GhostMailer() { + var transport = config.mail && config.mail.transport || 'direct', + options = config.mail && _.clone(config.mail.options) || {}; + + this.state = {}; + + this.transport = nodemailer.createTransport(transport, options); + + this.state.usingDirect = transport === 'direct'; } -// ## Email transport setup -// *This promise should always resolve to avoid halting Ghost::init*. -GhostMailer.prototype.init = function () { - var self = this; - self.state = {}; - if (config.mail && config.mail.transport) { - this.createTransport(); - return Promise.resolve(); - } - - self.transport = nodemailer.createTransport('direct'); - self.state.usingDirect = true; - - return Promise.resolve(); -}; - -GhostMailer.prototype.createTransport = function () { - this.transport = nodemailer.createTransport(config.mail.transport, _.clone(config.mail.options) || {}); -}; - GhostMailer.prototype.from = function () { var from = config.mail && (config.mail.from || config.mail.fromaddress); @@ -116,4 +102,4 @@ GhostMailer.prototype.send = function (message) { }); }; -module.exports = new GhostMailer(); +module.exports = GhostMailer; diff --git a/core/server/utils/startup-check.js b/core/server/utils/startup-check.js index 9b0774193a..0f7fec0e3d 100644 --- a/core/server/utils/startup-check.js +++ b/core/server/utils/startup-check.js @@ -21,6 +21,7 @@ checks = { this.nodeEnv(); this.packages(); this.contentPath(); + this.mail(); this.sqlite(); }, @@ -209,6 +210,23 @@ checks = { process.exit(exitCodes.SQLITE_DB_NOT_WRITABLE); } + }, + + mail: function checkMail() { + var configFile, + config; + + try { + configFile = require(configFilePath); + config = configFile[mode]; + } catch (e) { + configFilePath = path.join(appRoot, 'config.example.js'); + } + + if (!config.mail || !config.mail.transport) { + console.error('\x1B[31mWARNING: Ghost is attempting to use a direct method to send email. \nIt is recommended that you explicitly configure an email service.\033[0m'); + console.error('\x1B[32mHelp and documentation can be found at http://support.ghost.org/mail.\033[0m\n'); + } } }; diff --git a/core/test/integration/api/api_mail_spec.js b/core/test/integration/api/api_mail_spec.js index 564688e300..13805501da 100644 --- a/core/test/integration/api/api_mail_spec.js +++ b/core/test/integration/api/api_mail_spec.js @@ -3,11 +3,7 @@ var testUtils = require('../../utils'), should = require('should'), config = require('../../../server/config'), - mailer = require('../../../server/mail'), - i18n = require('../../../../core/server/i18n'), - - // Stuff we are testing - MailAPI = require('../../../server/api/mail'), + i18n = require('../../../../core/server/i18n'), // test data mailData = { @@ -27,15 +23,12 @@ describe('Mail API', function () { afterEach(testUtils.teardown); beforeEach(testUtils.setup('perms:mail', 'perms:init')); - should.exist(MailAPI); - it('returns a success', function (done) { config.set({mail: {transport: 'stub'}}); - mailer.init().then(function () { - mailer.transport.transportType.should.eql('STUB'); - return MailAPI.send(mailData, testUtils.context.internal); - }).then(function (response) { + var MailAPI = require('../../../server/api/mail'); + + MailAPI.send(mailData, testUtils.context.internal).then(function (response) { should.exist(response.mail); should.exist(response.mail[0].message); should.exist(response.mail[0].status); @@ -48,10 +41,9 @@ describe('Mail API', function () { it('returns a boo boo', function (done) { config.set({mail: {transport: 'stub', options: {error: 'Stub made a boo boo :('}}}); - mailer.init().then(function () { - mailer.transport.transportType.should.eql('STUB'); - return MailAPI.send(mailData, testUtils.context.internal); - }).then(function () { + var MailAPI = require('../../../server/api/mail'); + + MailAPI.send(mailData, testUtils.context.internal).then(function () { done(new Error('Stub did not error')); }).catch(function (error) { error.message.should.startWith('Error: Stub made a boo boo :('); diff --git a/core/test/unit/mail_spec.js b/core/test/unit/mail_spec.js index 9f8375c5b4..7af3d2bd45 100644 --- a/core/test/unit/mail_spec.js +++ b/core/test/unit/mail_spec.js @@ -4,9 +4,10 @@ var should = require('should'), Promise = require('bluebird'), // Stuff we are testing - mailer = require('../../server/mail'), + GhostMail = require('../../server/mail'), configUtils = require('../utils/configUtils'), i18n = require('../../server/i18n'), + mailer, // Mock SMTP config SMTP = { @@ -40,45 +41,48 @@ i18n.init(); describe('Mail', function () { afterEach(function () { + mailer = null; + configUtils.restore(); }); it('should attach mail provider to ghost instance', function () { + mailer = new GhostMail(); + should.exist(mailer); - mailer.should.have.property('init'); - mailer.should.have.property('transport'); mailer.should.have.property('send').and.be.a.Function(); }); - it('should setup SMTP transport on initialization', function (done) { + it('should setup SMTP transport on initialization', function () { configUtils.set({mail: SMTP}); - mailer.init().then(function () { - mailer.should.have.property('transport'); - mailer.transport.transportType.should.eql('SMTP'); - mailer.transport.sendMail.should.be.a.Function(); - done(); - }).catch(done); + mailer = new GhostMail(); + + mailer.should.have.property('transport'); + mailer.transport.transportType.should.eql('SMTP'); + mailer.transport.sendMail.should.be.a.Function(); }); - it('should fallback to direct if config is empty', function (done) { + it('should fallback to direct if config is empty', function () { configUtils.set({mail: {}}); - mailer.init().then(function () { - mailer.should.have.property('transport'); - mailer.transport.transportType.should.eql('DIRECT'); - done(); - }).catch(done); + + mailer = new GhostMail(); + + mailer.should.have.property('transport'); + mailer.transport.transportType.should.eql('DIRECT'); }); it('sends valid message successfully ', function (done) { configUtils.set({mail: {transport: 'stub'}}); - mailer.init().then(function () { - mailer.transport.transportType.should.eql('STUB'); - return mailer.send(mailDataNoServer); - }).then(function (response) { + mailer = new GhostMail(); + + mailer.transport.transportType.should.eql('STUB'); + + mailer.send(mailDataNoServer).then(function (response) { should.exist(response.message); should.exist(response.envelope); response.envelope.to.should.containEql('joe@example.com'); + done(); }).catch(done); }); @@ -86,10 +90,11 @@ describe('Mail', function () { it('handles failure', function (done) { configUtils.set({mail: {transport: 'stub', options: {error: 'Stub made a boo boo :('}}}); - mailer.init().then(function () { - mailer.transport.transportType.should.eql('STUB'); - return mailer.send(mailDataNoServer); - }).then(function () { + mailer = new GhostMail(); + + mailer.transport.transportType.should.eql('STUB'); + + mailer.send(mailDataNoServer).then(function () { done(new Error('Stub did not error')); }).catch(function (error) { error.message.should.eql('Error: Stub made a boo boo :('); @@ -98,6 +103,8 @@ describe('Mail', function () { }); it('should fail to send messages when given insufficient data', function (done) { + mailer = new GhostMail(); + Promise.settle([ mailer.send(), mailer.send({}), @@ -114,12 +121,14 @@ describe('Mail', function () { }); describe('Direct', function () { - beforeEach(function (done) { + beforeEach(function () { configUtils.set({mail: {}}); - mailer.init().then(function () { - done(); - }); + mailer = new GhostMail(); + }); + + afterEach(function () { + mailer = null; }); it('return correct failure message for domain doesn\'t exist', function (done) { @@ -163,12 +172,18 @@ describe('Mail', function () { from: '"Blog Title" ' } }); + + mailer = new GhostMail(); + mailer.from().should.equal('"Blog Title" '); }); it('should fall back to [blog.title] ', function () { // Standard domain configUtils.set({url: 'http://default.com', mail: {from: null}, theme: {title: 'Test'}}); + + mailer = new GhostMail(); + mailer.from().should.equal('"Test" '); // Trailing slash @@ -183,12 +198,18 @@ describe('Mail', function () { it('should use mail.from if both from and fromaddress are present', function () { // Standard domain configUtils.set({mail: {from: '"bar" ', fromaddress: '"Qux" '}}); + + mailer = new GhostMail(); + mailer.from().should.equal('"bar" '); }); it('should attach blog title if from or fromaddress are only email addresses', function () { // from and fromaddress are both set configUtils.set({mail: {from: 'from@default.com', fromaddress: 'fa@default.com'}, theme: {title: 'Test'}}); + + mailer = new GhostMail(); + mailer.from().should.equal('"Test" '); // only from set @@ -203,6 +224,9 @@ describe('Mail', function () { it('should ignore theme title if from address is Title format', function () { // from and fromaddress are both set configUtils.set({mail: {from: '"R2D2" ', fromaddress: '"C3PO" '}, theme: {title: 'Test'}}); + + mailer = new GhostMail(); + mailer.from().should.equal('"R2D2" '); // only from set @@ -216,6 +240,9 @@ describe('Mail', function () { it('should use default title if not theme title is provided', function () { configUtils.set({url: 'http://default.com:2368/', mail: {from: null}, theme: {title: null}}); + + mailer = new GhostMail(); + mailer.from().should.equal('"Ghost at default.com" '); }); });