From 11867ab43a7c201313233f7c1606e8c72534eb42 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Tue, 5 Apr 2022 14:14:39 +0100 Subject: [PATCH] Replaced schema.isPost in slack service /w custom fn - This is the only piece of server code that relies on the schema.checks, the rest are all frontend - IMO this code should actually check for the post properties that the slack message needs - OR it should switch based on the event type - either way there's no need to have a shared util for this simple use case - especially becaue it's confusing the use case for it and creating cross-coupling between server and frontend --- core/server/services/slack.js | 14 +++++++++++--- test/unit/server/services/slack.test.js | 14 +------------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/core/server/services/slack.js b/core/server/services/slack.js index 8bdbed5e3e..7653d0c477 100644 --- a/core/server/services/slack.js +++ b/core/server/services/slack.js @@ -6,7 +6,6 @@ const {blogIcon} = require('../lib/image'); const urlUtils = require('../../shared/url-utils'); const urlService = require('./url'); const settingsCache = require('../../shared/settings-cache'); -const schema = require('../data/schema').checks; const moment = require('moment'); // Used to receive post.published model event, but also the slack.test event from the API which iirc this was done to avoid circular deps a long time ago @@ -37,6 +36,15 @@ function getSlackSettings() { }; } +/** + * @TODO: change this function to check for the properties we depend on + * @param {Object} data + * @returns {boolean} + */ +function hasPostProperties(data) { + return Object.prototype.hasOwnProperty.call(data, 'html') && Object.prototype.hasOwnProperty.call(data, 'title') && Object.prototype.hasOwnProperty.call(data, 'slug'); +} + function ping(post) { let message; let title; @@ -47,7 +55,7 @@ function ping(post) { let blogTitle = settingsCache.get('title'); // If this is a post, we want to send the link of the post - if (schema.isPost(post)) { + if (hasPostProperties(post)) { message = urlService.getUrlByResourceId(post.id, {absolute: true}); title = post.title ? post.title : null; author = post.authors ? post.authors[0] : null; @@ -79,7 +87,7 @@ function ping(post) { return; } - if (schema.isPost(post)) { + if (hasPostProperties(post)) { slackData = { // We are handling the case of test notification here by checking // if it is a post or a test message to check webhook working. diff --git a/test/unit/server/services/slack.test.js b/test/unit/server/services/slack.test.js index c487806507..6c28ed73d6 100644 --- a/test/unit/server/services/slack.test.js +++ b/test/unit/server/services/slack.test.js @@ -11,7 +11,6 @@ const events = require('../../../../core/server/lib/common/events'); const logging = require('@tryghost/logging'); const imageLib = require('../../../../core/server/lib/image'); const urlService = require('../../../../core/server/services/url'); -const schema = require('../../../../core/server/data/schema').checks; const settingsCache = require('../../../../core/shared/settings-cache'); // Test data @@ -94,14 +93,12 @@ describe('Slack', function () { }); describe('ping()', function () { - let isPostStub; let settingsCacheStub; let slackReset; let makeRequestStub; const ping = slack.__get__('ping'); beforeEach(function () { - isPostStub = sinon.stub(schema, 'isPost'); sinon.stub(urlService, 'getUrlByResourceId'); settingsCacheStub = sinon.stub(settingsCache, 'get'); @@ -127,7 +124,6 @@ describe('Slack', function () { const post = testUtils.DataGenerator.forKnex.createPost({slug: 'webhook-test'}); urlService.getUrlByResourceId.withArgs(post.id, {absolute: true}).returns('http://myblog.com/' + post.slug + '/'); - isPostStub.returns(true); settingsCacheStub.withArgs('slack_url').returns(slackURL); // execute code @@ -135,7 +131,6 @@ describe('Slack', function () { // assertions makeRequestStub.calledOnce.should.be.true(); - isPostStub.calledTwice.should.be.true(); urlService.getUrlByResourceId.calledOnce.should.be.true(); settingsCacheStub.calledWith('slack_url').should.be.true(); @@ -157,7 +152,6 @@ describe('Slack', function () { let requestUrl; let requestData; - isPostStub.returns(false); settingsCacheStub.withArgs('slack_url').returns(slackURL); // execute code @@ -165,7 +159,6 @@ describe('Slack', function () { // assertions makeRequestStub.calledOnce.should.be.true(); - isPostStub.calledTwice.should.be.true(); urlService.getUrlByResourceId.called.should.be.false(); settingsCacheStub.calledWith('slack_url').should.be.true(); @@ -198,7 +191,6 @@ describe('Slack', function () { it('does not make a request if post is a page', function () { const post = testUtils.DataGenerator.forKnex.createPost({type: 'page'}); - isPostStub.returns(true); settingsCacheStub.withArgs('slack_url').returns(slackURL); // execute code @@ -206,14 +198,12 @@ describe('Slack', function () { // assertions makeRequestStub.calledOnce.should.be.false(); - isPostStub.calledOnce.should.be.true(); urlService.getUrlByResourceId.calledOnce.should.be.true(); settingsCacheStub.calledWith('slack_url').should.be.true(); }); it('does not send webhook for \'welcome\' post', function () { const post = testUtils.DataGenerator.forKnex.createPost({slug: 'welcome'}); - isPostStub.returns(true); settingsCacheStub.withArgs('slack_url').returns(slackURL); // execute code @@ -221,7 +211,6 @@ describe('Slack', function () { // assertions makeRequestStub.calledOnce.should.be.false(); - isPostStub.calledOnce.should.be.true(); urlService.getUrlByResourceId.calledOnce.should.be.true(); settingsCacheStub.calledWith('slack_url').should.be.true(); }); @@ -235,8 +224,7 @@ describe('Slack', function () { // assertions makeRequestStub.calledOnce.should.be.false(); - isPostStub.calledOnce.should.be.true(); - urlService.getUrlByResourceId.called.should.be.false(); + urlService.getUrlByResourceId.called.should.be.true(); settingsCacheStub.calledWith('slack_url').should.be.true(); }); });