diff --git a/core/server/config/index.js b/core/server/config/index.js index d1e98135c2..f9ef0ebcec 100644 --- a/core/server/config/index.js +++ b/core/server/config/index.js @@ -319,7 +319,7 @@ ConfigManager.prototype.isPrivacyDisabled = function (privacyFlag) { * Check if any of the currently set config items are deprecated, and issues a warning. */ ConfigManager.prototype.checkDeprecated = function () { - var deprecatedItems = ['updateCheck'], + var deprecatedItems = ['updateCheck', 'mail.fromaddress'], self = this; _.each(deprecatedItems, function (property) { diff --git a/core/server/errors/index.js b/core/server/errors/index.js index 4a8b4ffd71..05179aa8ce 100644 --- a/core/server/errors/index.js +++ b/core/server/errors/index.js @@ -73,6 +73,7 @@ errors = { if ((process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'staging' || process.env.NODE_ENV === 'production')) { + warn = warn || 'no message supplied'; var msgs = ['\nWarning:'.yellow, warn.yellow, '\n']; if (context) { diff --git a/core/server/mail.js b/core/server/mail.js index 4ac91aadc0..51a9a23f37 100644 --- a/core/server/mail.js +++ b/core/server/mail.js @@ -1,6 +1,7 @@ var _ = require('lodash'), Promise = require('bluebird'), nodemailer = require('nodemailer'), + validator = require('validator'), config = require('./config'); function GhostMailer(opts) { @@ -28,22 +29,32 @@ GhostMailer.prototype.createTransport = function () { this.transport = nodemailer.createTransport(config.mail.transport, _.clone(config.mail.options) || {}); }; -GhostMailer.prototype.fromAddress = function () { - var from = config.mail && config.mail.fromaddress, - domain; +GhostMailer.prototype.from = function () { + var from = config.mail && (config.mail.from || config.mail.fromaddress); + // If we don't have a from address at all if (!from) { - // Extract the domain name from url set in config.js - domain = config.url.match(new RegExp('^https?://([^/:?#]+)(?:[/:?#]|$)', 'i')); - domain = domain && domain[1]; - // Default to ghost@[blog.url] - from = 'ghost@' + domain; + from = 'ghost@' + this.getDomain(); + } + + // If we do have a from address, and it's just an email + if (validator.isEmail(from)) { + if (!config.theme.title) { + config.theme.title = 'Ghost at ' + this.getDomain(); + } + from = config.theme.title + ' <' + from + '>'; } return from; }; +// Moved it to its own module +GhostMailer.prototype.getDomain = function () { + var domain = config.url.match(new RegExp('^https?://([^/:?#]+)(?:[/:?#]|$)', 'i')); + return domain && domain[1]; +}; + // Sends an e-mail message enforcing `to` (blog owner) and `from` fields // This assumes that api.settings.read('email') was aready done on the API level GhostMailer.prototype.send = function (message) { @@ -63,7 +74,7 @@ GhostMailer.prototype.send = function (message) { sendMail = Promise.promisify(self.transport.sendMail.bind(self.transport)); message = _.extend(message, { - from: self.fromAddress(), + from: self.from(), to: to, generateTextFromHTML: true }); diff --git a/core/test/unit/config_spec.js b/core/test/unit/config_spec.js index 4baf69a9c8..18fb670b47 100644 --- a/core/test/unit/config_spec.js +++ b/core/test/unit/config_spec.js @@ -12,7 +12,7 @@ var should = require('should'), // Thing we are testing defaultConfig = require('../../../config.example')[process.env.NODE_ENV], - config = rewire('../../server/config'), + config = require('../../server/config'), // storing current environment currentEnv = process.env.NODE_ENV; @@ -680,7 +680,7 @@ describe('Config', function () { }); }); - describe('Deprecated', function () { + describe('Check for deprecation messages:', function () { var logStub, // Can't use afterEach here, because mocha uses console.log to output the checkboxes // which we've just stubbed, so we need to restore it before the test ends to see ticks. @@ -695,7 +695,7 @@ describe('Config', function () { }); afterEach(function () { - // logStub.restore(); + logStub.restore(); config = rewire('../../server/config'); }); @@ -715,14 +715,11 @@ describe('Config', function () { config.checkDeprecated(); logStub.calledOnce.should.be.true; - logStub.calledWith( - '\nWarning:'.yellow, - ('The configuration property [' + 'updateCheck'.bold + '] has been deprecated.').yellow, - '\n', - 'This will be removed in a future version, please update your config.js file.'.white, - '\n', - 'Please check http://support.ghost.org/config for the most up-to-date example.'.green, - '\n').should.be.true; + + logStub.calledWithMatch(null, 'updateCheck').should.be.false; + logStub.calledWithMatch('', 'updateCheck').should.be.true; + logStub.calledWithMatch(sinon.match.string, 'updateCheck').should.be.true; + logStub.calledWithMatch(sinon.match.number, 'updateCheck').should.be.false; // Future tests: This is important here! resetEnvironment(); @@ -736,14 +733,64 @@ describe('Config', function () { config.checkDeprecated(); logStub.calledOnce.should.be.true; - logStub.calledWith( - '\nWarning:'.yellow, - ('The configuration property [' + 'updateCheck'.bold + '] has been deprecated.').yellow, - '\n', - 'This will be removed in a future version, please update your config.js file.'.white, - '\n', - 'Please check http://support.ghost.org/config for the most up-to-date example.'.green, - '\n').should.be.true; + + logStub.calledWithMatch(null, 'updateCheck').should.be.false; + logStub.calledWithMatch('', 'updateCheck').should.be.true; + logStub.calledWithMatch(sinon.match.string, 'updateCheck').should.be.true; + logStub.calledWithMatch(sinon.match.number, 'updateCheck').should.be.false; + + // Future tests: This is important here! + resetEnvironment(); + }); + + it('displays warning when mail.fromaddress exists and is truthy', function () { + config.set({ + mail: { + fromaddress: 'foo' + } + }); + // Run the test code + config.checkDeprecated(); + + logStub.calledOnce.should.be.true; + + logStub.calledWithMatch(null, 'mail.fromaddress').should.be.false; + logStub.calledWithMatch('', 'mail.fromaddress').should.be.true; + logStub.calledWithMatch(sinon.match.string, 'mail.fromaddress').should.be.true; + logStub.calledWithMatch(sinon.match.number, 'mail.fromaddress').should.be.false; + + // Future tests: This is important here! + resetEnvironment(); + }); + + it('displays warning when mail.fromaddress exists and is falsy', function () { + config.set({ + mail: { + fromaddress: undefined + } + }); + // Run the test code + config.checkDeprecated(); + + logStub.calledOnce.should.be.true; + logStub.calledWithMatch(null, 'mail.fromaddress').should.be.false; + logStub.calledWithMatch('', 'mail.fromaddress').should.be.true; + logStub.calledWithMatch(sinon.match.string, 'mail.fromaddress').should.be.true; + logStub.calledWithMatch(sinon.match.number, 'mail.fromaddress').should.be.false; + + // Future tests: This is important here! + resetEnvironment(); + }); + + it('doesn\'t display warning when only part of a deprecated option is set', function () { + config.set({ + mail: { + notfromaddress: 'foo' + } + }); + + config.checkDeprecated(); + logStub.calledOnce.should.be.false; // Future tests: This is important here! resetEnvironment(); diff --git a/core/test/unit/error_handling_spec.js b/core/test/unit/error_handling_spec.js index 4497e3bdac..597e013c2e 100644 --- a/core/test/unit/error_handling_spec.js +++ b/core/test/unit/error_handling_spec.js @@ -50,7 +50,80 @@ describe('Error handling', function () { }); }); - describe('Logging', function () { + describe('Warn Logging', function () { + var logStub, + // Can't use afterEach here, because mocha uses console.log to output the checkboxes + // which we've just stubbed, so we need to restore it before the test ends to see ticks. + resetEnvironment = function () { + logStub.restore(); + process.env.NODE_ENV = currentEnv; + }; + + beforeEach(function () { + logStub = sinon.stub(console, 'log'); + process.env.NODE_ENV = 'development'; + }); + + afterEach(function () { + logStub.restore(); + }); + + it('logs default warn with no message supplied', function () { + errors.logWarn(); + + logStub.calledOnce.should.be.true; + logStub.calledWith( + '\nWarning: no message supplied'.yellow, '\n'); + + // Future tests: This is important here! + resetEnvironment(); + }); + + it('logs warn with only message', function () { + var errorText = 'Error1'; + + errors.logWarn(errorText); + + logStub.calledOnce.should.be.true; + logStub.calledWith(('\nWarning: ' + errorText).yellow, '\n'); + + // Future tests: This is important here! + resetEnvironment(); + }); + + it('logs warn with message and context', function () { + var errorText = 'Error1', + contextText = 'Context1'; + + errors.logWarn(errorText, contextText); + + logStub.calledOnce.should.be.true; + logStub.calledWith( + ('\nWarning: ' + errorText).yellow, '\n', contextText.white, '\n' + ); + + // Future tests: This is important here! + resetEnvironment(); + }); + + it('logs warn with message and context and help', function () { + var errorText = 'Error1', + contextText = 'Context1', + helpText = 'Help1'; + + errors.logWarn(errorText, contextText, helpText); + + logStub.calledOnce.should.be.true; + logStub.calledWith( + ('\nWarning: ' + errorText).yellow, '\n', contextText.white, '\n', helpText.green, '\n' + ); + + // Future tests: This is important here! + resetEnvironment(); + }); + }); + + describe('Error Logging', function () { var logStub; beforeEach(function () { diff --git a/core/test/unit/mail_spec.js b/core/test/unit/mail_spec.js index 41c3d89c28..1103fa8621 100644 --- a/core/test/unit/mail_spec.js +++ b/core/test/unit/mail_spec.js @@ -1,18 +1,13 @@ -/*globals describe, beforeEach, afterEach, it*/ +/*globals describe, afterEach, it*/ /*jshint expr:true*/ var should = require('should'), - sinon = require('sinon'), Promise = require('bluebird'), - _ = require('lodash'), - rewire = require('rewire'), // Stuff we are testing - mailer = rewire('../../server/mail'), - defaultConfig = require('../../../config'), - SMTP, - fakeConfig, - fakeSettings, - sandbox = sinon.sandbox.create(); + mailer = require('../../server/mail'), + config = require('../../server/config'), + + SMTP; // Mock SMTP config SMTP = { @@ -27,26 +22,8 @@ SMTP = { }; describe('Mail', function () { - var overrideConfig = function (newConfig) { - var config = rewire('../../server/config'), - existingConfig = mailer.__get__('config'); - - config.set(_.extend(existingConfig, newConfig)); - }; - - beforeEach(function () { - // Mock config and settings - fakeConfig = _.extend({}, defaultConfig); - fakeSettings = { - url: 'http://test.tryghost.org', - email: 'ghost-test@localhost' - }; - - overrideConfig(fakeConfig); - }); - afterEach(function () { - sandbox.restore(); + config.set({mail: null}); }); it('should attach mail provider to ghost instance', function () { @@ -57,7 +34,7 @@ describe('Mail', function () { }); it('should setup SMTP transport on initialization', function (done) { - overrideConfig({mail: SMTP}); + config.set({mail: SMTP}); mailer.init().then(function () { mailer.should.have.property('transport'); mailer.transport.transportType.should.eql('SMTP'); @@ -67,11 +44,10 @@ describe('Mail', function () { }); it('should fallback to direct if config is empty', function (done) { - overrideConfig({mail: {}}); + config.set({mail: {}}); mailer.init().then(function () { mailer.should.have.property('transport'); mailer.transport.transportType.should.eql('DIRECT'); - done(); }).catch(done); }); @@ -86,27 +62,66 @@ describe('Mail', function () { descriptors.forEach(function (d) { d.isRejected().should.be.true; d.reason().should.be.an.instanceOf(Error); + d.reason().message.should.eql('Email Error: Incomplete message data.'); }); done(); }).catch(done); }); it('should use from address as configured in config.js', function () { - overrideConfig({mail: {fromaddress: 'static@example.com'}}); - mailer.fromAddress().should.equal('static@example.com'); + config.set({ + mail: { + from: 'Blog Title ' + } + }); + mailer.from().should.equal('Blog Title '); }); - it('should fall back to ghost@[blog.url] as from address', function () { + it('should fall back to [blog.title] as from address', function () { // Standard domain - overrideConfig({url: 'http://default.com', mail: {fromaddress: null}}); - mailer.fromAddress().should.equal('ghost@default.com'); + config.set({url: 'http://default.com', mail: {from: null}, theme: {title: 'Test'}}); + mailer.from().should.equal('Test '); // Trailing slash - overrideConfig({url: 'http://default.com/', mail: {}}); - mailer.fromAddress().should.equal('ghost@default.com'); + config.set({url: 'http://default.com/', mail: {from: null}, theme: {title: 'Test'}}); + mailer.from().should.equal('Test '); // Strip Port - overrideConfig({url: 'http://default.com:2368/', mail: {}}); - mailer.fromAddress().should.equal('ghost@default.com'); + config.set({url: 'http://default.com:2368/', mail: {from: null}, theme: {title: 'Test'}}); + mailer.from().should.equal('Test '); + }); + + it('should use mail.from if both from and fromaddress are present', function () { + // Standard domain + config.set({mail: {from: 'bar ', fromaddress: 'Qux '}}); + 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 + config.set({mail: {from: 'from@default.com', fromaddress: 'fa@default.com'}, theme: {title: 'Test'}}); + mailer.from().should.equal('Test '); + + // only from set + config.set({mail: {from: 'from@default.com', fromaddress: null}, theme: {title: 'Test'}}); + mailer.from().should.equal('Test '); + + // only fromaddress set + config.set({mail: {from: null, fromaddress: 'fa@default.com'}, theme: {title: 'Test'}}); + mailer.from().should.equal('Test '); + }); + + it('should ignore theme title if from address is Title format', function () { + // from and fromaddress are both set + config.set({mail: {from: 'R2D2 ', fromaddress: 'C3PO '}, theme: {title: 'Test'}}); + mailer.from().should.equal('R2D2 '); + + // only from set + config.set({mail: {from: 'R2D2 ', fromaddress: null}, theme: {title: 'Test'}}); + mailer.from().should.equal('R2D2 '); + + // only fromaddress set + config.set({mail: {from: null, fromaddress: 'C3PO '}, theme: {title: 'Test'}}); + mailer.from().should.equal('C3PO '); }); });