From b3820fbb31d16687014ffac65773b19fdcffe888 Mon Sep 17 00:00:00 2001
From: Gabor Javorszky <gabor@javorszky.co.uk>
Date: Sat, 20 Sep 2014 19:14:16 +0100
Subject: [PATCH 1/2] More robust deprecation check code
Closes #4082
* reformatted code to allow for traversal
* deeper config items should be denoted like this: `object.object.object.property`.
* added tests for testing the deprecation warnings
---
core/server/config/index.js | 30 ++++++++++----
core/test/unit/config_spec.js | 74 ++++++++++++++++++++++++++++++++++-
2 files changed, 95 insertions(+), 9 deletions(-)
diff --git a/core/server/config/index.js b/core/server/config/index.js
index 231b2426b1..d1e98135c2 100644
--- a/core/server/config/index.js
+++ b/core/server/config/index.js
@@ -322,17 +322,31 @@ ConfigManager.prototype.checkDeprecated = function () {
var deprecatedItems = ['updateCheck'],
self = this;
- _.each(deprecatedItems, function (item) {
- if (self.hasOwnProperty(item)) {
- var errorText = 'The configuration property [' + item.toString().bold + '] has been deprecated.',
- explinationText = 'This will be removed in a future version, please update your config.js file.',
- helpText = 'Please check http://support.ghost.org/config for the most up-to-date example.';
-
- errors.logWarn(errorText, explinationText, helpText);
- }
+ _.each(deprecatedItems, function (property) {
+ self.displayDeprecated(self, property.split('.'), []);
});
};
+ConfigManager.prototype.displayDeprecated = function (item, properties, address) {
+ var self = this,
+ property = properties.shift(),
+ errorText,
+ explanationText,
+ helpText;
+
+ address.push(property);
+
+ if (item.hasOwnProperty(property)) {
+ if (properties.length) {
+ return self.displayDeprecated(item[property], properties, address);
+ }
+ errorText = 'The configuration property [' + address.join('.').bold + '] has been deprecated.';
+ explanationText = 'This will be removed in a future version, please update your config.js file.';
+ helpText = 'Please check http://support.ghost.org/config for the most up-to-date example.';
+ errors.logWarn(errorText, explanationText, helpText);
+ }
+};
+
if (testingEnvs.indexOf(process.env.NODE_ENV) > -1) {
defaultConfig = require('../../../config.example')[process.env.NODE_ENV];
}
diff --git a/core/test/unit/config_spec.js b/core/test/unit/config_spec.js
index 64389c7058..4baf69a9c8 100644
--- a/core/test/unit/config_spec.js
+++ b/core/test/unit/config_spec.js
@@ -12,7 +12,9 @@ var should = require('should'),
// Thing we are testing
defaultConfig = require('../../../config.example')[process.env.NODE_ENV],
- config = rewire('../../server/config');
+ config = rewire('../../server/config'),
+ // storing current environment
+ currentEnv = process.env.NODE_ENV;
// To stop jshint complaining
should.equal(true, true);
@@ -677,4 +679,74 @@ describe('Config', function () {
}).catch(done);
});
});
+
+ describe('Deprecated', 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();
+ config = rewire('../../server/config');
+ });
+
+ it('doesn\'t display warning when deprecated options not set', function () {
+ config.checkDeprecated();
+ logStub.calledOnce.should.be.false;
+
+ // Future tests: This is important here!
+ resetEnvironment();
+ });
+
+ it('displays warning when updateCheck exists and is truthy', function () {
+ config.set({
+ updateCheck: 'foo'
+ });
+ // Run the test code
+ 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;
+
+ // Future tests: This is important here!
+ resetEnvironment();
+ });
+
+ it('displays warning when updateCheck exists and is falsy', function () {
+ config.set({
+ updateCheck: undefined
+ });
+ // Run the test code
+ 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;
+
+ // Future tests: This is important here!
+ resetEnvironment();
+ });
+ });
});
From 1f5a378b4c7beccd380ee5a9ed09661ad9cda2af Mon Sep 17 00:00:00 2001
From: Gabor Javorszky <gabor@javorszky.co.uk>
Date: Sat, 20 Sep 2014 22:11:30 +0100
Subject: [PATCH 2/2] Deprecated mail.fromaddress, mail.from is Title
<email@address>
Closes #4018
* cleaned up `mail_spec.js`
* deprecated `mail.fromaddress`
* implemented 'Blog title <email@address.com>' format with fallbacks
* added tests to deprecation and from address, made existing ones more robust
* moved domain intuit into its own module: `GhostMailer.getDomain()`
---
core/server/config/index.js | 2 +-
core/server/errors/index.js | 1 +
core/server/mail.js | 29 +++++---
core/test/unit/config_spec.js | 85 +++++++++++++++++------
core/test/unit/error_handling_spec.js | 75 ++++++++++++++++++++-
core/test/unit/mail_spec.js | 97 ++++++++++++++++-----------
6 files changed, 218 insertions(+), 71 deletions(-)
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 <static@example.com>'
+ }
+ });
+ mailer.from().should.equal('Blog Title <static@example.com>');
});
- it('should fall back to ghost@[blog.url] as from address', function () {
+ it('should fall back to [blog.title] <ghost@[blog.url]> 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 <ghost@default.com>');
// 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 <ghost@default.com>');
// 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 <ghost@default.com>');
+ });
+
+ it('should use mail.from if both from and fromaddress are present', function () {
+ // Standard domain
+ config.set({mail: {from: 'bar <from@default.com>', fromaddress: 'Qux <fa@default.com>'}});
+ mailer.from().should.equal('bar <from@default.com>');
+ });
+
+ 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 <from@default.com>');
+
+ // only from set
+ config.set({mail: {from: 'from@default.com', fromaddress: null}, theme: {title: 'Test'}});
+ mailer.from().should.equal('Test <from@default.com>');
+
+ // only fromaddress set
+ config.set({mail: {from: null, fromaddress: 'fa@default.com'}, theme: {title: 'Test'}});
+ mailer.from().should.equal('Test <fa@default.com>');
+ });
+
+ it('should ignore theme title if from address is Title <email@address.com> format', function () {
+ // from and fromaddress are both set
+ config.set({mail: {from: 'R2D2 <from@default.com>', fromaddress: 'C3PO <fa@default.com>'}, theme: {title: 'Test'}});
+ mailer.from().should.equal('R2D2 <from@default.com>');
+
+ // only from set
+ config.set({mail: {from: 'R2D2 <from@default.com>', fromaddress: null}, theme: {title: 'Test'}});
+ mailer.from().should.equal('R2D2 <from@default.com>');
+
+ // only fromaddress set
+ config.set({mail: {from: null, fromaddress: 'C3PO <fa@default.com>'}, theme: {title: 'Test'}});
+ mailer.from().should.equal('C3PO <fa@default.com>');
});
});