xmlrpc + slack init() -> listen() & fixup tests

no issue

- changes xmlrcp & slack `init` function to be `listen`
- update the code to use `listen` instead of `init`
- changes the tests to make sure that event listeners are not wired up
- adds 100% test coverage

Since we added slack event listeners, the xmlrpc event tests have been throwing an error:
 > Unhandled rejection Error
See: http://puu.sh/phvjZ.png

This is because both xmlrpc & slack are listening to `post.published` events.
xmlrpc didn't require any extra stubbing, but the slack listener did
By turning the listeners off after the tests, we reset the environment to not impact the next event test

We probably need to do more work like this to improve the systems around event handling and
make them more robust

core/server/data/slack/index.js

@@ -9,7 +9,6 @@ var https           = require('https'),
     schema          = require('../schema').checks,
     options,
     req,
-    slack = {},
     slackData = {};
 
 function getSlackSettings() {
@@ -17,7 +16,7 @@ function getSlackSettings() {
         var slackSetting = response.settings[0].value;
 
         try {
-            slackSetting = JSON.parse(slackSetting) || slackSetting;
+            slackSetting = JSON.parse(slackSetting);
         } catch (e) {
             return Promise.reject(e);
         }
@@ -83,24 +82,29 @@ function ping(post) {
             options.headers = {'Content-type': 'application/json'};
 
             // with all the data we have, we're doing the request now
-            slack._makeRequest(options, slackData);
+            makeRequest(options, slackData);
         } else {
             return;
         }
     });
 }
 
-function init() {
+function listener(model) {
-    events.on('post.published', function (model) {
+    ping(model.toJSON());
-        slack._ping(model.toJSON());
+}
-    });
+
-    events.on('slack.test', function () {
+function testPing() {
-        slack._ping({
+    ping({
         message: 'Heya! This is a test notification from your Ghost blog :simple_smile:. Seems to work fine!'
     });
-    });
 }
-slack.init = init;
+
-slack._ping = ping;
+function listen() {
-slack._makeRequest = makeRequest;
+    events.on('post.published', listener);
-module.exports = slack;
+    events.on('slack.test', testPing);
+}
+
+// Public API
+module.exports = {
+    listen: listen
+};

core/server/data/xml/xmlrpc.js

@@ -66,23 +66,26 @@ function ping(post) {
 
         req = http.request(options);
         req.write(pingXML);
-        req.on('error', function (error) {
+        req.on('error', function handleError(error) {
                 errors.logError(
                     error,
                     i18n.t('errors.data.xml.xmlrpc.pingUpdateFailed.error'),
                     i18n.t('errors.data.xml.xmlrpc.pingUpdateFailed.help', {url: 'http://support.ghost.org'})
                 );
-        });
+            }
+        );
         req.end();
     });
 }
 
-function init() {
+function listener(model) {
-    events.on('post.published', function (model) {
     ping(model.toJSON());
-    });
+}
+
+function listen() {
+    events.on('post.published', listener);
 }
 
 module.exports = {
-    init: init
+    listen: listen
 };

core/server/index.js

@@ -83,9 +83,9 @@ function init(options) {
             // Initialize sitemaps
             sitemap.init(),
             // Initialize xmrpc ping
-            xmlrpc.init(),
+            xmlrpc.listen(),
             // Initialize slack ping
-            slack.init()
+            slack.listen()
         );
     }).then(function () {
         // Get reference to an express app instance.

core/test/unit/slack_spec.js

@@ -1,17 +1,20 @@
 /*globals describe, beforeEach, afterEach, it*/
-var nock            = require('nock'),
+var _               = require('lodash'),
+    nock            = require('nock'),
     should          = require('should'),
     sinon           = require('sinon'),
+    rewire          = require('rewire'),
     Promise         = require('bluebird'),
     testUtils       = require('../utils'),
     url             = require('url'),
 
 // Stuff we test
-    slack          = require('../../server/data/slack'),
+    slack          = rewire('../../server/data/slack'),
     events         = require('../../server/events'),
     api            = require('../../server/api/settings'),
     config         = require('../../server/config'),
     schema         = require('../../server/data/schema').checks,
+
     sandbox        = sinon.sandbox.create(),
 // Test data
     slackObjNoUrl =
@@ -43,28 +46,56 @@ var nock            = require('nock'),
 should.equal(true, true);
 
 describe('Slack', function () {
-    var testPost;
+    var eventStub;
+
+    beforeEach(function () {
+        eventStub = sandbox.stub(events, 'on');
+    });
 
     afterEach(function () {
         sandbox.restore();
     });
 
-    it('should call ping if post is published', function () {
+    it('listen() should initialise event correctly', function () {
-        // set up
+        slack.listen();
-        var ping = sandbox.stub(slack, '_ping');
+        eventStub.calledTwice.should.be.true();
-        testPost = {
+        eventStub.firstCall.calledWith('post.published', slack.__get__('listener')).should.be.true();
-                toJSON: function () {
+        eventStub.secondCall.calledWith('slack.test', slack.__get__('testPing')).should.be.true();
-                    return testUtils.DataGenerator.Content.posts[2];
-                }
-            };
-        // execute code
-        slack.init();
-        events.emit('post.published', testPost);
-
-        // assertions
-        ping.calledOnce.should.be.true();
     });
 
+    it('listener() calls ping() with toJSONified model', function () {
+        var testPost = _.clone(testUtils.DataGenerator.Content.posts[2]),
+            testModel = {toJSON: function () {return testPost; }},
+            pingStub = sandbox.stub(),
+            resetSlack = slack.__set__('ping', pingStub),
+            listener = slack.__get__('listener');
+
+        listener(testModel);
+
+        pingStub.calledOnce.should.be.true();
+        pingStub.calledWith(testPost).should.be.true();
+
+        // Reset slack ping method
+        resetSlack();
+    });
+
+    it('testPing() calls ping() with default message', function () {
+        var pingStub = sandbox.stub(),
+            resetSlack = slack.__set__('ping', pingStub),
+            testPing = slack.__get__('testPing');
+
+        testPing();
+
+        pingStub.calledOnce.should.be.true();
+        pingStub.calledWith(sinon.match.has('message')).should.be.true();
+
+        // Reset slack ping method
+        resetSlack();
+    });
+
+    describe('makeRequest()', function () {
+        var makeRequest = slack.__get__('makeRequest');
+
         it('should make request to slack correctly', function () {
             // set up
             var reqOptions,
@@ -78,8 +109,9 @@ describe('Slack', function () {
             pingSlack = nock('https://hooks.slack.com/')
                 .post('/services/a-b-c-d', {text:'http://myblog.com/mypost', icon_url: 'http://myblog.com/someImageurl.jpg', username: 'Ghost'})
                 .reply(200);
+
             // execute code
-        slack._makeRequest(reqOptions, {text:'http://myblog.com/mypost', icon_url: 'http://myblog.com/someImageurl.jpg', username: 'Ghost'});
+            makeRequest(reqOptions, {text:'http://myblog.com/mypost', icon_url: 'http://myblog.com/someImageurl.jpg', username: 'Ghost'});
 
             // assertions
             pingSlack.isDone().should.be.true();
@@ -98,41 +130,53 @@ describe('Slack', function () {
             pingSlack = nock('https://hooks.slack.com/')
                 .post('/services/a-b-c-d', {text:'http://myblog.com/mypost', icon_url: 'http://myblog.com/someImageurl.jpg', username: 'Ghost'})
                 .replyWithError(404);
+
             // execute code
-        slack._makeRequest(reqOptions, {text:'http://myblog.com/mypost', icon_url: 'http://myblog.com/someImageurl.jpg', username: 'Ghost'});
+            makeRequest(reqOptions, {text:'http://myblog.com/mypost', icon_url: 'http://myblog.com/someImageurl.jpg', username: 'Ghost'});
 
             // assertions
             pingSlack.isDone().should.be.true();
         });
+    });
 
-    describe('Ping', function () {
+    describe('ping()', function () {
         var makeRequestAssertions,
-            schemaStub,
+            isPostStub,
             urlForStub,
             settingsAPIStub,
             settingsObj,
-            makeRequestStub;
+            slackReset,
+            makeRequestMock,
+            makeRequestSpy,
+            ping =  slack.__get__('ping');
 
         beforeEach(function () {
-            schemaStub = sandbox.stub(schema, 'isPost');
+            isPostStub = sandbox.stub(schema, 'isPost');
             urlForStub = sandbox.stub(config, 'urlFor');
-            urlForStub.onFirstCall().returns('http://myblog.com/post');
+            urlForStub.withArgs('post').returns('http://myblog.com/post');
-            urlForStub.onSecondCall().returns('http://myblog.com/someImageurl.jpg');
+            urlForStub.returns('http://myblog.com/someImageurl.jpg');
             settingsObj = {settings: [], meta: {}};
             settingsAPIStub = sandbox.stub(api, 'read').returns(Promise.resolve(settingsObj));
-            makeRequestStub = sandbox.stub(slack, '_makeRequest', function () {
+
+            makeRequestMock = function () {
                 makeRequestAssertions.apply(this, arguments);
-            });
+            };
+            makeRequestSpy = sandbox.spy(makeRequestMock);
+            slackReset = slack.__set__('makeRequest', makeRequestMock);
         });
 
-        it('makes a request if url is provided', function (done) {
+        afterEach(function () {
+            slackReset();
+        });
+
+        it('makes a request for a post if url is provided', function (done) {
             // set up
-            schemaStub.returns('true');
+            isPostStub.returns(true);
             settingsObj.settings[0] = slackObjWithUrl;
 
             // assertions
             makeRequestAssertions = function (requestOptions, requestData) {
-                    schemaStub.calledOnce.should.be.true();
+                isPostStub.calledOnce.should.be.true();
                 urlForStub.calledTwice.should.be.true();
                 settingsAPIStub.calledOnce.should.be.true();
                 requestOptions.should.have.property('href').and.be.equal('https://hooks.slack.com/services/a-b-c-d');
@@ -143,21 +187,57 @@ describe('Slack', function () {
             };
 
             // execute code
-            slack._ping({}).catch(done);
+            ping({}).catch(done);
+        });
+
+        it('makes a request for a message if url is provided', function (done) {
+            isPostStub.returns(false);
+            settingsObj.settings[0] = slackObjWithUrl;
+
+            // assertions
+            makeRequestAssertions = function (requestOptions, requestData) {
+                isPostStub.calledOnce.should.be.true();
+                urlForStub.calledOnce.should.be.true();
+                settingsAPIStub.calledOnce.should.be.true();
+                requestOptions.should.have.property('href').and.be.equal('https://hooks.slack.com/services/a-b-c-d');
+                requestData.should.have.property('text').and.be.equal('Hi!');
+                requestData.should.have.property('icon_url').and.be.equal('http://myblog.com/someImageurl.jpg');
+                requestData.should.have.property('username').and.be.equal('Ghost');
+                done();
+            };
+
+            ping({message: 'Hi!'}).catch(done);
+        });
+
+        it('does not make a request if post is a page', function (done) {
+            // set up
+            isPostStub.returns(true);
+            settingsObj.settings[0] = slackObjWithUrl;
+
+            // execute code
+            ping({page: true}).then(function (result) {
+                // assertions
+                isPostStub.calledOnce.should.be.true();
+                urlForStub.calledOnce.should.be.true();
+                settingsAPIStub.calledOnce.should.be.true();
+                makeRequestSpy.called.should.be.false();
+                should.not.exist(result);
+                done();
+            }).catch(done);
         });
 
         it('does not make a request if no url is provided', function (done) {
             // set up
-            schemaStub.returns('true');
+            isPostStub.returns(true);
             settingsObj.settings[0] = slackObjNoUrl;
 
             // execute code
-            slack._ping({}).then(function (result) {
+            ping({}).then(function (result) {
                 // assertions
-                schemaStub.calledOnce.should.be.true();
+                isPostStub.calledOnce.should.be.true();
                 urlForStub.calledOnce.should.be.true();
                 settingsAPIStub.calledOnce.should.be.true();
-                makeRequestStub.called.should.be.false();
+                makeRequestSpy.called.should.be.false();
                 should.not.exist(result);
                 done();
             }).catch(done);
@@ -165,20 +245,33 @@ describe('Slack', function () {
 
         it('does not send webhook for \'welcome-to-ghost\' post', function (done) {
             // set up
-            schemaStub.returns('true');
+            isPostStub.returns(true);
             settingsObj.settings[0] = slackObjWithUrl;
 
-            makeRequestAssertions = function () {};
             // execute code
-            slack._ping({slug: 'welcome-to-ghost'}).then(function (result) {
+            ping({slug: 'welcome-to-ghost'}).then(function (result) {
                 // assertions
-                schemaStub.calledOnce.should.be.true();
+                isPostStub.calledOnce.should.be.true();
                 urlForStub.calledOnce.should.be.true();
                 settingsAPIStub.calledOnce.should.be.true();
-                makeRequestStub.called.should.be.false();
+                makeRequestSpy.called.should.be.false();
                 should.not.exist(result);
                 done();
             }).catch(done);
         });
+
+        it('handles broken slack settings', function (done) {
+            settingsObj.settings[0] = '';
+
+            ping({}).then(function () {
+                done('This should not get called');
+            }).catch(function () {
+                isPostStub.calledOnce.should.be.true();
+                urlForStub.calledOnce.should.be.false();
+                settingsAPIStub.calledOnce.should.be.true();
+                makeRequestSpy.called.should.be.false();
+                done();
+            });
+        });
     });
 });

core/test/unit/xmlrpc_spec.js

@@ -1,9 +1,12 @@
 /*globals describe, beforeEach, afterEach, it*/
-var nock            = require('nock'),
+var _               = require('lodash'),
+    nock            = require('nock'),
     should          = require('should'),
     sinon           = require('sinon'),
+    rewire          = require('rewire'),
     testUtils       = require('../utils'),
-    xmlrpc          = require('../../server/data/xml/xmlrpc'),
+    configUtils     = require('../utils/configUtils'),
+    xmlrpc          = rewire('../../server/data/xml/xmlrpc'),
     events          = require('../../server/events'),
     // storing current environment
     currentEnv      = process.env.NODE_ENV;
@@ -12,32 +15,119 @@ var nock            = require('nock'),
 should.equal(true, true);
 
 describe('XMLRPC', function () {
-    var sandbox;
+    var sandbox, eventStub;
 
     beforeEach(function () {
         sandbox = sinon.sandbox.create();
+        eventStub = sandbox.stub(events, 'on');
         // give environment a value that will ping
         process.env.NODE_ENV = 'production';
     });
 
     afterEach(function () {
         sandbox.restore();
+        configUtils.restore();
+        nock.cleanAll();
         // reset the environment
         process.env.NODE_ENV = currentEnv;
     });
 
-    it('should execute two pings', function () {
+    it('listen() should initialise event correctly', function () {
+        xmlrpc.listen();
+        eventStub.calledOnce.should.be.true();
+        eventStub.calledWith('post.published', xmlrpc.__get__('listener')).should.be.true();
+    });
+
+    it('listener() calls ping() with toJSONified model', function () {
+        var testPost = _.clone(testUtils.DataGenerator.Content.posts[2]),
+            testModel = {toJSON: function () {return testPost; }},
+            pingStub = sandbox.stub(),
+            resetXmlRpc = xmlrpc.__set__('ping', pingStub),
+            listener = xmlrpc.__get__('listener');
+
+        listener(testModel);
+
+        pingStub.calledOnce.should.be.true();
+        pingStub.calledWith(testPost).should.be.true();
+
+        // Reset xmlrpc ping method
+        resetXmlRpc();
+    });
+
+    describe('ping()', function () {
+        var ping = xmlrpc.__get__('ping');
+
+        it('with a post should execute two pings', function () {
             var ping1 = nock('http://blogsearch.google.com').post('/ping/RPC2').reply(200),
                 ping2 = nock('http://rpc.pingomatic.com').post('/').reply(200),
-            testPost = {
+                testPost = _.clone(testUtils.DataGenerator.Content.posts[2]);
-                toJSON: function () {
+
-                    return testUtils.DataGenerator.Content.posts[2];
+            ping(testPost);
-                }
-            };
 
-        xmlrpc.init();
-        events.emit('post.published', testPost);
             ping1.isDone().should.be.true();
             ping2.isDone().should.be.true();
         });
+
+        it('with default post should not execute pings', function () {
+            var ping1 = nock('http://blogsearch.google.com').post('/ping/RPC2').reply(200),
+                ping2 = nock('http://rpc.pingomatic.com').post('/').reply(200),
+                testPost = _.clone(testUtils.DataGenerator.Content.posts[2]);
+
+            testPost.slug = 'welcome-to-ghost';
+
+            ping(testPost);
+
+            ping1.isDone().should.be.false();
+            ping2.isDone().should.be.false();
+        });
+
+        it('with a page should not execute pings', function () {
+            var ping1 = nock('http://blogsearch.google.com').post('/ping/RPC2').reply(200),
+                ping2 = nock('http://rpc.pingomatic.com').post('/').reply(200),
+                testPage = _.clone(testUtils.DataGenerator.Content.posts[5]);
+
+            ping(testPage);
+
+            ping1.isDone().should.be.false();
+            ping2.isDone().should.be.false();
+        });
+
+        it('when privacy.useRpcPing is false should not execute pings', function () {
+            var ping1 = nock('http://blogsearch.google.com').post('/ping/RPC2').reply(200),
+                ping2 = nock('http://rpc.pingomatic.com').post('/').reply(200),
+                testPost = _.clone(testUtils.DataGenerator.Content.posts[2]);
+
+            configUtils.set({privacy: {useRpcPing: false}});
+
+            ping(testPost);
+
+            ping1.isDone().should.be.false();
+            ping2.isDone().should.be.false();
+        });
+
+        it('captures errors from requests', function (done) {
+            var ping1 = nock('http://blogsearch.google.com').post('/ping/RPC2').reply(200),
+                ping2 = nock('http://rpc.pingomatic.com').post('/').replyWithError('ping site is down'),
+                testPost = _.clone(testUtils.DataGenerator.Content.posts[2]),
+                errorMock, resetXmlRpc;
+
+            errorMock = {
+                logError: function logError(error) {
+                    should.exist(error);
+                    error.message.should.eql('ping site is down');
+
+                    // Reset xmlrpc handleError method and exit test
+                    resetXmlRpc();
+                    done();
+                }
+            };
+
+            resetXmlRpc = xmlrpc.__set__('errors', errorMock);
+
+            ping(testPost);
+
+            ping1.isDone().should.be.true();
+            ping2.isDone().should.be.true();
+        });
+    });
 });