Refactored trigger module to be testable

refs https://github.com/TryGhost/Toolbox/issues/283

- Current trigger module handling webhook paypload delivery isn't testable! It sucks to add features to it without assurance things still work
- Apart from expanding the test suite this changeset also needs live testing - setting up webhooks etc.

core/server/services/webhooks/listen.js

@@ -1,7 +1,9 @@
 const _ = require('lodash');
 const limitService = require('../../services/limits');
 const logging = require('@tryghost/logging');
-const trigger = require('./trigger');
+const WebhookTrigger = require('./trigger');
+const models = require('../../models');
+const payload = require('./payload');
 
 // The webhook system is fundamentally built on top of our model event system
 const events = require('../../lib/common/events');
@@ -55,6 +57,7 @@ const listen = async () => {
         }
     }
 
+    const webhookTrigger = new WebhookTrigger({models, payload});
     _.each(WEBHOOKS, (event) => {
         events.on(event, (model, options) => {
             // CASE: avoid triggering webhooks when importing
@@ -62,7 +65,7 @@ const listen = async () => {
                 return;
             }
 
-            trigger(event, model);
+            webhookTrigger.trigger(event, model);
         });
     });
 };

core/server/services/webhooks/trigger.js

@@ -1,19 +1,22 @@
-const _ = require('lodash');
 const debug = require('@tryghost/debug')('services:webhooks:trigger');
 const logging = require('@tryghost/logging');
 const request = require('@tryghost/request');
-const models = require('../../models');
+const ghostVersion = require('@tryghost/version');
-const payload = require('./payload');
+
+class WebhookTrigger {
+    constructor({models, payload}){
+        this.models = models;
+        this.payload = payload;
+    }
 
-const webhooks = {
     getAll(event) {
-        return models
+        return this.models
             .Webhook
             .findAllByEvent(event, {context: {internal: true}});
-    },
+    }
 
     update(webhook, data) {
-        models
+        this.models
             .Webhook
             .edit({
                 last_triggered_at: Date.now(),
@@ -23,36 +26,34 @@ const webhooks = {
             .catch(() => {
                 logging.warn(`Unable to update "last_triggered" for webhook: ${webhook.id}`);
             });
-    },
+    }
 
     destroy(webhook) {
-        return models
+        return this.models
             .Webhook
             .destroy({id: webhook.id}, {context: {internal: true}})
             .catch(() => {
                 logging.warn(`Unable to destroy webhook ${webhook.id}.`);
             });
     }
-};
 
-const response = {
     onSuccess(webhook) {
         return (res) => {
-            webhooks.update(webhook, {
+            this.update(webhook, {
                 statusCode: res.statusCode
             });
         };
-    },
+    }
 
     onError(webhook) {
         return (err) => {
             if (err.statusCode === 410) {
                 logging.info(`Webhook destroyed (410 response) for "${webhook.get('event')}" with url "${webhook.get('target_url')}".`);
 
-                return webhooks.destroy(webhook);
+                return this.destroy(webhook);
             }
 
-            webhooks.update(webhook, {
+            this.update(webhook, {
                 statusCode: err.statusCode,
                 error: `Request failed: ${err.code || 'unknown'}`
             });
@@ -60,34 +61,39 @@ const response = {
             logging.warn(`Request to ${webhook.get('target_url') || null} failed because of: ${err.code || ''}.`);
         };
     }
-};
 
-module.exports = (event, model) => {
+    async trigger(event, model) {
-    webhooks.getAll(event)
+        const response = {
-        .then((hooks) => {
+            onSuccess: this.onSuccess.bind(this),
-            debug(`${hooks.models.length} webhooks found for ${event}.`);
+            onError: this.onError.bind(this)
+        };
 
-            _.each(hooks.models, (webhook) => {
+        const hooks = await this.getAll(event);
-                payload(webhook.get('event'), model)
-                    .then((hookPayload) => {
-                        const reqPayload = JSON.stringify(hookPayload);
-                        const url = webhook.get('target_url');
-                        const opts = {
-                            body: reqPayload,
-                            headers: {
-                                'Content-Length': Buffer.byteLength(reqPayload),
-                                'Content-Type': 'application/json'
-                            },
-                            timeout: 2 * 1000,
-                            retry: 5
-                        };
 
-                        logging.info(`Triggering webhook for "${webhook.get('event')}" with url "${url}"`);
+        debug(`${hooks.models.length} webhooks found for ${event}.`);
 
-                        request(url, opts)
+        hooks.models.forEach(async (webhook) => {
-                            .then(response.onSuccess(webhook))
+            const hookPayload = await this.payload(webhook.get('event'), model);
-                            .catch(response.onError(webhook));
+
-                    });
+            const reqPayload = JSON.stringify(hookPayload);
-            });
+            const url = webhook.get('target_url');
+            const opts = {
+                body: reqPayload,
+                headers: {
+                    'Content-Length': Buffer.byteLength(reqPayload),
+                    'Content-Type': 'application/json'
+                },
+                timeout: 2 * 1000,
+                retry: 5
+            };
+
+            logging.info(`Triggering webhook for "${webhook.get('event')}" with url "${url}"`);
+
+            request(url, opts)
+                .then(response.onSuccess(webhook))
+                .catch(response.onError(webhook));
         });
-};
+    }
+}
+
+module.exports = WebhookTrigger;

test/e2e-api/shared/version.test.js

@@ -1,7 +1,7 @@
 const {agentProvider, fixtureManager, matchers, mockManager} = require('../../utils/e2e-framework');
 const {anyErrorId, stringMatching, anyObjectId, anyLocationFor, anyISODateTime, anyEtag} = matchers;
 
-describe.only('API Versioning', function () {
+describe('API Versioning', function () {
     describe('Admin API', function () {
         let agentAdminAPI;
 

test/unit/server/services/webhooks/trigger.test.js

@@ -0,0 +1,40 @@
+const should = require('should');
+const sinon = require('sinon');
+
+const WebhookTrigger = require('../../../../../core/server/services/webhooks/trigger');
+
+describe('Webhook Service', function () {
+    const models = {
+        Webhook: {
+            edit: () => sinon.stub().resolves(null),
+            destroy: () => sinon.stub().resolves(null),
+            findAllByEvent: () => sinon.stub().resolves(null),
+            getByEventAndTarget: () => sinon.stub().resolves(null),
+            add: () => sinon.stub().resolves(null)
+        }
+    };
+
+    const payload = sinon.stub().resolves(null);
+
+    afterEach(function () {
+        sinon.restore();
+    });
+
+    describe('trigger', function () {
+        it('Does not trigger payload handler when event and model that has no hooks registered', async function () {
+            sinon.stub(models.Webhook, 'findAllByEvent')
+                .withArgs('post.added', {context: {internal: true}})
+                .resolves({models: []});
+
+            const webhookTrigger = new WebhookTrigger({
+                models,
+                payload
+            });
+
+            await webhookTrigger.trigger('post.added');
+
+            should.equal(models.Webhook.findAllByEvent.called, true);
+            should.equal(payload.called, false);
+        });
+    });
+});