Skip to content
This repository has been archived by the owner on Oct 24, 2019. It is now read-only.

handle bad slack usernames more bettah #78

Merged
merged 2 commits into from
Sep 20, 2017
Merged

handle bad slack usernames more bettah #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c14e73
handle bad slack usernames more bettah and don't halt and catch fire
Sep 14, 2017
25c479b
add more tests for postAlert logic
Sep 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions incoming/function.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ module.exports.fn = function(event, context, callback) {
message.number = res.number;
message.requestId = requestId;
console.log(`${requestId} issue ${res.number} created for ${message.body.github.title}`);
slack.alertToSlack(message, user.slack, client, (err, status) => {
slack.alertToSlack(message, user.slack, client, slackChannel, (err, status) => {
if (err) return callback(err);
return callback(null, status);
});
Expand Down Expand Up @@ -79,7 +79,7 @@ module.exports.fn = function(event, context, callback) {
let q = queue(1);
message.users.forEach((user) => {
user = checkUser(user);
q.defer(slack.alertToSlack, message, user.slack, client);
q.defer(slack.alertToSlack, message, user.slack, client, slackChannel);
});
q.awaitAll(function(err, status) {
if (err) return callback(err);
Expand Down
75 changes: 55 additions & 20 deletions lib/slack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
'use strict';

const encode = require('./utils').encode;
const slack = {};

module.exports.alertToSlack = alertToSlack;
module.exports.ingestSNS = ingestSNS;
module.exports.postAlert = postAlert;

function alertToSlack(snsMessage, destination, client, callback) {
slack.alertToSlack = function(snsMessage, destination, client, slackChannel, callback) {
if (!snsMessage.number) {
console.log(`${snsMessage.requestId} No GitHub issue number found in message body`);
return callback('No GitHub issue number found in message body');
Expand All @@ -15,31 +12,44 @@ function alertToSlack(snsMessage, destination, client, callback) {
if (err) return callback(err);
console.log(`${snsMessage.requestId} generated Slack callback_id ${res} for issue ${snsMessage.number} user ${destination}`);
snsMessage.callback_id = res; // eslint-disable-line camelcase
ingestSNS(snsMessage, (err, message, prompt) => {
slack.ingestSNS(snsMessage, (err, message, prompt) => {
if (err) return callback(err);
postAlert(destination, message, client, (err, res) => {
if (err) return callback(err);
slack.postAlert(destination, message, client, slackChannel, snsMessage.requestId, (err, res) => {
console.log('message');
if (err && err == 'badSlack') {
return callback(null, res);
} else if (err) {
return callback(err);
}
const status = {
alert: res.ok,
destination: destination,
message: res.message.text,
url: snsMessage.url
};
if (prompt) {
postAlert(destination, prompt, client, (err, res) => {
console.log('prompt');
slack.postAlert(destination, prompt, client, slackChannel, snsMessage.requestId, (err, res) => {
if (err && err == 'badSlack') {
return callback(null, res);
};
if (err) return callback(err);
status.message = `${status.message}, Prompt: ${res.message.text}`;
console.log(`${snsMessage.requestId} sent Slack message for issue ${snsMessage.number} to ${destination}`);
return callback(null, status);
});
} else {
console.log(`${snsMessage.requestId} sent Slack message for issue ${snsMessage.number} to ${destination}`);
return callback(null, status);
}
console.log(`${snsMessage.requestId} sent Slack message for issue ${snsMessage.number} sent to ${destination}`);
return callback(null, status);
});
});
});
}
}
};

function ingestSNS(snsMessage, callback) {
slack.ingestSNS = function(snsMessage, callback) {
console.log('ingestSNS called');
try {
let message = {
text: snsMessage.body.slack.message,
Expand Down Expand Up @@ -83,17 +93,42 @@ function ingestSNS(snsMessage, callback) {
return callback(null, message, prompt);
} else return callback(null, message, null);
} catch (err) {
return callback(`${snsMessage.requestId} sns message parsing error`);
return callback(`${snsMessage.requestId} sns message parsing error: ${err}`);
}
}
};

function postAlert(destination, alert, client, callback) {
slack.postAlert = function(destination, alert, client, slackChannel, requestId, callback) {
let options;
if (!alert.text) return callback(`${alert.requestId} missing Slack message body`);

if (!alert.text) return callback(`${requestId} missing Slack message body`);
if (destination.indexOf('@') > -1) options = { 'as_user': true, attachments: alert.attachments };
if (destination.indexOf('#') > -1) options = { attachments: alert.attachments };
client.chat.postMessage(destination, alert.text, options, function(err, res) {
if (err) return callback(res);
return callback(null, res);
if (err) {
console.log(`${requestId} Error sending message to slack destination: ${destination}`);
let postFailure = {
text: `Error sending message to \`${destination}\` for requestId ${requestId}`,
attachments: [
{
title: 'Slack error message',
text: JSON.stringify(err)
}
]
};
client.chat.postMessage(slackChannel, postFailure.text, { attachments: postFailure.attachments}, function(err, res) {
if (err) {
console.log(`${requestId} Error sending message to slack channel ${slackChannel} for slack destination failure ${destination}`);
}
// return a custom error so we can skip generating a second error notification
return callback('badSlack', res);
});
} else {
return callback(null, res);
}
});
}
};

// module.exports.alertToSlack = alertToSlack;
// module.exports.ingestSNS = ingestSNS;
// module.exports.postAlert = postAlert;
module.exports = slack;
58 changes: 50 additions & 8 deletions test/fixtures/slack.fixtures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ module.exports.sns = {
body: {},
requestId: 123
},
malformedError: '123 sns message parsing error',
malformedError: '123 sns message parsing error: TypeError: Cannot read property \'slack\' of undefined',
malformedMessageError: '123 sns message parsing error: TypeError: Cannot read property \'message\' of undefined',
malformedNoIssueError: 'No GitHub issue number found in message body',
success: {
type: 'self_service',
Expand Down Expand Up @@ -186,7 +187,7 @@ module.exports.slack = {
statusPrompt: {
alert: true,
destination: '@testUser',
message: 'testSlackMessage, Prompt: testSlackMessage',
message: 'testSlackMessage, Prompt: testSlackPrompt',
url: 'https://github.com/testOwner/testRepo/issues/7'
},
statusBroadcast: [
Expand Down Expand Up @@ -252,12 +253,53 @@ module.exports.clients = {
slackAPIUrl:'test-url',
chat: {
postMessage: function(username, message, options, callback) {
return callback('error', {
ok: false,
error: 'no_text',
scopes: [ 'identify', 'bot:basic' ],
acceptedScopes: [ 'chat:write:user', 'client' ]
});
if (username == '@testUser') {
return callback('error', {
ok: false,
error: 'no_text',
scopes: [ 'identify', 'bot:basic' ],
acceptedScopes: [ 'chat:write:user', 'client' ]
});
} else {
return callback(null, {
ok: true,
channel: 'D6G0UU7MW',
ts: '1501777340.256863',
message: {
type: 'message',
user: 'U6GHXJQ1Z',
text: 'testSlackMessage',
'bot_id': 'B6G0UU6HW',
attachments: [ [Object] ],
ts: '1501777340.256863'
},
scopes: [ 'identify', 'bot:basic' ],
acceptedScopes: [ 'chat:write:user', 'client' ]
});
}
}
}
},
everythingErrors: {
_token:'test-token',
slackAPIUrl:'test-url',
chat: {
postMessage: function(username, message, options, callback) {
if (username == '@testUser') {
return callback('error', {
ok: false,
error: 'no_text',
scopes: [ 'identify', 'bot:basic' ],
acceptedScopes: [ 'chat:write:user', 'client' ]
});
} else {
return callback('error', {
ok: false,
error: 'channel_not_found',
scopes: [ 'identify', 'bot:basic' ],
acceptedScopes: [ 'chat:write:bot', 'post' ]
});
}
}
}
},
Expand Down
97 changes: 59 additions & 38 deletions test/lib/slack.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const test = require('tape');

test('[slack] [ingestSNS] SNS parsing error', (t) => {
file.ingestSNS(fixtures.sns.malformed, (err) => {
t.equal(err, fixtures.sns.malformedError, '-- should pass through error message');
t.equal(err, fixtures.sns.malformedMessageError, '-- should pass through error message');
t.end();
});
});
Expand Down Expand Up @@ -41,30 +41,32 @@ test('[slack] [ingestSNS] broadcast success', (t) => {
});

test('[slack] [postAlert] missing message body', (t) => {
file.postAlert(fixtures.slack.username, {requestId: 123}, fixtures.clients.empty, (err, res) => {
file.postAlert(fixtures.slack.username, {requestId: 123}, fixtures.clients.empty, 'test-channel', 123, (err, res) => {
t.equal(err, fixtures.slack.missingMessageError, '-- should pass through error message');
t.end();
});
});

test('[slack] [postAlert] cannot locate destination (channel)', (t) => {
file.postAlert(fixtures.slack.channel, fixtures.slack.message, fixtures.clients.noChannel, (err, res) => {
t.equal(err.ok, false, '-- ok should be false');
t.deepEqual(err, fixtures.slack.noChannel, '-- should pass through error response object');
file.postAlert(fixtures.slack.channel, fixtures.slack.message, fixtures.clients.noChannel, 'test-channel', 123, (err, res) => {
t.ok(err, 'badSlack', '-- passes custom error on Slack failure');
t.equal(res.ok, false, '-- ok should be false');
t.deepEqual(res, fixtures.slack.noChannel, '-- should pass through response object');
t.end();
});
});

test('[slack] [postAlert] error', (t) => {
file.postAlert(fixtures.slack.username, fixtures.slack.message, fixtures.clients.error, (err, res) => {
t.equal(err.ok, false, '-- ok should be false');
t.deepEqual(err, fixtures.slack.error, '-- should pass through error response object');
file.postAlert(fixtures.slack.username, fixtures.slack.message, fixtures.clients.error, 'test-channel', 123, (err, res) => {
t.ok(err, 'badSlack', '-- passes custom error on Slack failure');
t.equal(res.ok, true, '-- ok should be true');
t.deepEqual(res, fixtures.slack.success, '-- should pass successful response object');
t.end();
});
});

test('[slack] [postAlert] channel success', (t) => {
file.postAlert(fixtures.slack.channel, fixtures.slack.message, fixtures.clients.success, (err, res) => {
file.postAlert(fixtures.slack.channel, fixtures.slack.message, fixtures.clients.success, 'test-channel', 123, (err, res) => {
t.ifError(err, '-- should not error');
t.equal(res.ok, true, '-- should be true');
t.deepEqual(res, fixtures.slack.success, '-- should pass through response object');
Expand All @@ -73,60 +75,65 @@ test('[slack] [postAlert] channel success', (t) => {
});

test('[slack] [postAlert] username success', (t) => {
file.postAlert(fixtures.slack.username, fixtures.slack.message, fixtures.clients.success, (err, res) => {
file.postAlert(fixtures.slack.username, fixtures.slack.message, fixtures.clients.success, 'test-channel', 123, (err, res) => {
t.ifError(err, '-- should not error');
t.equal(res.ok, true, '-- should be true');
t.deepEqual(res, fixtures.slack.success, '-- should pass through response object');
t.end();
});
});

test('[slack] [postAlert] username fails, channel post success', (t) => {
file.postAlert(fixtures.slack.username, fixtures.slack.message, fixtures.clients.error, 'test-channel', 123, (err, res) => {
t.equal(err, 'badSlack', '-- should return custom error');
t.equal(res.ok, true, '-- should be true');
t.deepEqual(res, fixtures.slack.success, '-- should pass through response object');
t.end();
});
});

test('[slack] [postAlert] username fails, channel post fails', (t) => {
file.postAlert(fixtures.slack.username, fixtures.slack.message, fixtures.clients.noChannel, 'test-channel', 123, (err, res) => {
t.equal(err, 'badSlack', '-- should return custom error');
t.equal(res.ok, false, '-- should be true');
t.deepEqual(res, fixtures.slack.noChannel, '-- should pass through response object');
t.end();
});
});

test('[slack] [alertToSlack] ingestSNS error', (t) => {
const stub = sinon.stub(file, 'ingestSNS').returns(fixtures.sns.malformedError);
file.alertToSlack({number: 7, requestId: 123}, fixtures.slack.username, fixtures.clients.empty, (err, status) => {
const stub = sinon.stub(file, 'ingestSNS').yields(fixtures.sns.malformedError);
file.alertToSlack({number: 7, requestId: 123}, fixtures.slack.username, fixtures.clients.empty, 'test-channel', (err, status) => {
t.equal(err, fixtures.sns.malformedError, '-- should pass through error message');
t.end();
});
file.ingestSNS.restore();
});

test('[slack] [alertToSlack] encode error', (t) => {
file.alertToSlack({requestId: 123}, fixtures.slack.username, fixtures.clients.empty, (err, status) => {
file.alertToSlack({requestId: 123}, fixtures.slack.username, fixtures.clients.empty, 'test-channel', (err, status) => {
t.equal(err, fixtures.sns.malformedNoIssueError, '-- should pass through error message');
t.end();
});
});

test('[slack] [alertToSlack] postAlert message error', (t) => {
const stub0 = sinon.stub(file, 'ingestSNS').returns(null, fixtures.slack.message);
const stub1 = sinon.stub(file, 'postAlert').returns(fixtures.slack.error);
file.alertToSlack(fixtures.sns.success, fixtures.slack.username, fixtures.clients.error, (err, status) => {
t.equal(err.ok, false, '-- ok should be false');
t.deepEqual(err, fixtures.slack.error, '-- should pass through error response object');
const stub0 = sinon.stub(file, 'ingestSNS').yields(null, fixtures.slack.message);
const stub1 = sinon.stub(file, 'postAlert').yields('badSlack');
file.alertToSlack(fixtures.sns.success, fixtures.slack.username, fixtures.clients.error, 'test-channel', (err, status) => {
t.error(err, '-- does not error on bad Slack message');
t.equal(status, undefined, '-- bad Slack message generates no response object');
t.end();
});
file.ingestSNS.restore();
file.postAlert.restore();
});

test('[slack] [alertToSlack] postAlert message success, prompt error', (t) => {
const stub0 = sinon.stub(file, 'ingestSNS').returns(null, fixtures.slack.message, fixtures.slack.prompt);
const stub1 = sinon.stub(file, 'postAlert');
stub1.withArgs(fixtures.slack.username, fixtures.slack.message, fixtures.clients.success).returns(null, fixtures.slack.success);
stub1.withArgs(fixtures.slack.username, fixtures.slack.message, fixtures.clients.error).returns(fixtures.slack.error);
file.alertToSlack(fixtures.sns.success, fixtures.slack.username, fixtures.clients.error, (err, status) => {
t.equal(err.ok, false, '-- ok should be false');
t.deepEqual(err, fixtures.slack.error, '-- should pass through error response object');
t.end();
});
file.ingestSNS.restore();
file.postAlert.restore();
});

test('[slack] [alertToSlack] postAlert message, no prompt', (t) => {
const stub0 = sinon.stub(file, 'ingestSNS').returns(null, fixtures.slack.message);
const stub1 = sinon.stub(file, 'postAlert').returns(null, fixtures.slack.success);
file.alertToSlack(fixtures.sns.successNoPrompt, fixtures.slack.username, fixtures.clients.success, (err, status) => {
const stub0 = sinon.stub(file, 'ingestSNS').yields(null, fixtures.slack.message);
const stub1 = sinon.stub(file, 'postAlert').yields(null, fixtures.slack.success);
file.alertToSlack(fixtures.sns.successNoPrompt, fixtures.slack.username, fixtures.clients.success, 'test-channel', (err, status) => {
t.ifError(err, '-- should not error');
t.equal(status.alert, true, '-- should be true');
t.deepEqual(status, fixtures.slack.status, '-- should pass through status object');
Expand All @@ -139,11 +146,11 @@ test('[slack] [alertToSlack] postAlert message, no prompt', (t) => {
// NOTE: Make a second pass at this test case, can be improved

test('[slack] [alertToSlack] postAlert message and prompt success', (t) => {
const stub0 = sinon.stub(file, 'ingestSNS').returns(null, fixtures.slack.message, fixtures.slack.prompt);
const stub0 = sinon.stub(file, 'ingestSNS').yields(null, fixtures.slack.message, fixtures.slack.prompt);
const stub1 = sinon.stub(file, 'postAlert');
stub1.withArgs(fixtures.slack.username, fixtures.slack.message, fixtures.clients.success).returns(null, fixtures.slack.success);
stub1.withArgs(fixtures.slack.username, fixtures.slack.prompt, fixtures.clients.success).returns(null, fixtures.slack.successPrompt);
file.alertToSlack(fixtures.sns.success, fixtures.slack.username, fixtures.clients.success, (err, status) => {
stub1.withArgs(fixtures.slack.username, fixtures.slack.message, fixtures.clients.success).yields(null, fixtures.slack.success);
stub1.withArgs(fixtures.slack.username, fixtures.slack.prompt, fixtures.clients.success).yields(null, fixtures.slack.successPrompt);
file.alertToSlack(fixtures.sns.success, fixtures.slack.username, fixtures.clients.success, 'test-channel', (err, status) => {
t.ifError(err, '-- should not error');
t.equal(status.alert, true, '-- should be true');
t.deepEqual(status, fixtures.slack.statusPrompt, '-- should pass through status object');
Expand All @@ -152,3 +159,17 @@ test('[slack] [alertToSlack] postAlert message and prompt success', (t) => {
file.ingestSNS.restore();
file.postAlert.restore();
});

test('[slack] [alertToSlack] postAlert message success, prompt error', (t) => {
const ingestSnsStub = sinon.stub(file, 'ingestSNS').yields(null, fixtures.slack.message, fixtures.slack.prompt);
const postAlertStub = sinon.stub(file, 'postAlert');
postAlertStub.withArgs(fixtures.slack.username, fixtures.slack.message, fixtures.clients.success, 'test-channel', 123).yields(null, fixtures.slack.success);
postAlertStub.withArgs(fixtures.slack.username, fixtures.slack.prompt, fixtures.clients.success,'test-channel', 123).yields('badSlack');
file.alertToSlack(fixtures.sns.success, fixtures.slack.username, fixtures.clients.success, 'test-channel', (err, status) => {
t.ifError(err, '-- should not error');
t.equal(status, undefined, '-- does not pass status object when badSlacking');
t.end();
});
file.ingestSNS.restore();
file.postAlert.restore();
});