From ed1cceb7f8ec92ab2f2339233fb307e5c86084e9 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 14:58:22 +1000 Subject: [PATCH 01/39] Add extended Jest matchers --- jest.config.js | 2 +- jest.plugins.js | 15 ++++++++++ package.json | 1 + tests/app.js | 4 +-- tests/e2e-tests.js | 4 +-- tests/integration-tests.js | 6 ++-- yarn.lock | 59 +++++++++++++++++++++++++++++++++++++- 7 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 jest.plugins.js diff --git a/jest.config.js b/jest.config.js index 00a4743..4b65497 100644 --- a/jest.config.js +++ b/jest.config.js @@ -8,7 +8,7 @@ const config = { collectCoverage: true, - setupTestFrameworkScriptFile: 'jest-chain', + setupTestFrameworkScriptFile: './jest.plugins', testEnvironment: 'node', testMatch: [ diff --git a/jest.plugins.js b/jest.plugins.js new file mode 100644 index 0000000..db38561 --- /dev/null +++ b/jest.plugins.js @@ -0,0 +1,15 @@ +/** + * Defines plugins for use with Jest. + * + * @see https://jestjs.io/docs/en/configuration.html#setuptestframeworkscriptfile-string + */ + +'use strict'; + +// Allows assertions to be chained together, to reduce repetition. +// @see https://github.com/mattphillips/jest-chain#usage +require( 'jest-chain' ); + +// Adds a bunch of additional matchers to Jest. +// @see https://github.com/jest-community/jest-extended#api +require( 'jest-extended' ); diff --git a/package.json b/package.json index 1cea340..100ec48 100644 --- a/package.json +++ b/package.json @@ -39,6 +39,7 @@ "eslint-plugin-jest": "^21.18.0", "jest": "^23.4.2", "jest-chain": "^1.0.3", + "jest-extended": "^0.8.1", "object-assign-deep": "^0.4.0" } } diff --git a/tests/app.js b/tests/app.js index 5fdfb37..8642495 100644 --- a/tests/app.js +++ b/tests/app.js @@ -259,7 +259,7 @@ describe( 'validateToken', () => { }); it( 'returns true for a token that DOES match', () => { - expect( app.validateToken( 'something', 'something' ) ).toBe( true ); + expect( app.validateToken( 'something', 'something' ) ).toBeTrue(); }); }); // ValidateToken. @@ -321,7 +321,7 @@ describe( 'handlePost', () => { mockExpress.request.body.challenge = Math.random().toString(); const result = app.handlePost( mockExpress.request, mockExpress.response ); expect( receivedResponse ).toBe( mockExpress.request.body.challenge ); - expect( result ).toBe( false ); + expect( result ).toBeFalse(); }); diff --git a/tests/e2e-tests.js b/tests/e2e-tests.js index 6e781e5..f01b0cd 100644 --- a/tests/e2e-tests.js +++ b/tests/e2e-tests.js @@ -158,7 +158,7 @@ describe( 'The database', () => { }; runner( '<@' + user + '>++', options, ( result ) => { - expect( result ).toBe( false ); + expect( result ).toBeFalse(); done(); }); }); @@ -436,7 +436,7 @@ describe( 'Slack messaging', () => { } expect( slackClientMock.chat.postMessage ).toHaveBeenCalledTimes( 1 ); - expect( messageFoundInCollection ).toBe( true ); + expect( messageFoundInCollection ).toBeTrue(); done(); diff --git a/tests/integration-tests.js b/tests/integration-tests.js index 700eb08..312f575 100644 --- a/tests/integration-tests.js +++ b/tests/integration-tests.js @@ -169,7 +169,7 @@ describe( 'The Express server', () => { mockExpress.response.send.mockClear(); mockExpress.request.headers['x-slack-retry-num'] = 1; const result = app.handlePost( mockExpress.request, mockExpress.response ); - expect( result ).toBe( false ); + expect( result ).toBeFalse(); expect( mockExpress.response.send ).toHaveBeenCalledTimes( 1 ); }); @@ -194,7 +194,7 @@ describe( 'The database', () => { const dbClient = await postgres.connect(); const query = await dbClient.query( tableExistsQuery ); await dbClient.release(); - expect( query.rows[0].exists ).toBe( false ); + expect( query.rows[0].exists ).toBeFalse(); }); it( 'does not yet have the case-insensitive extension', async() => { @@ -220,7 +220,7 @@ describe( 'The database', () => { listener.close(); const query = await dbClient.query( tableExistsQuery ); await dbClient.release(); - expect( query.rows[0].exists ).toBe( true ); + expect( query.rows[0].exists ).toBeTrue(); done(); }); }); diff --git a/yarn.lock b/yarn.lock index fe783c3..d47c9c8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1338,6 +1338,17 @@ expand-range@^1.8.1: dependencies: fill-range "^2.1.0" +expect@^22.1.0: + version "22.4.3" + resolved "https://registry.yarnpkg.com/expect/-/expect-22.4.3.tgz#d5a29d0a0e1fb2153557caef2674d4547e914674" + dependencies: + ansi-styles "^3.2.0" + jest-diff "^22.4.3" + jest-get-type "^22.4.3" + jest-matcher-utils "^22.4.3" + jest-message-util "^22.4.3" + jest-regex-util "^22.4.3" + expect@^23.4.0: version "23.4.0" resolved "https://registry.yarnpkg.com/expect/-/expect-23.4.0.tgz#6da4ecc99c1471253e7288338983ad1ebadb60c3" @@ -2345,6 +2356,15 @@ jest-config@^23.4.2: jest-validate "^23.4.0" pretty-format "^23.2.0" +jest-diff@^22.4.3: + version "22.4.3" + resolved "https://registry.yarnpkg.com/jest-diff/-/jest-diff-22.4.3.tgz#e18cc3feff0aeef159d02310f2686d4065378030" + dependencies: + chalk "^2.0.1" + diff "^3.2.0" + jest-get-type "^22.4.3" + pretty-format "^22.4.3" + jest-diff@^23.2.0: version "23.2.0" resolved "https://registry.yarnpkg.com/jest-diff/-/jest-diff-23.2.0.tgz#9f2cf4b51e12c791550200abc16b47130af1062a" @@ -2382,7 +2402,15 @@ jest-environment-node@^23.4.0: jest-mock "^23.2.0" jest-util "^23.4.0" -jest-get-type@^22.1.0: +jest-extended@^0.8.1: + version "0.8.1" + resolved "https://registry.yarnpkg.com/jest-extended/-/jest-extended-0.8.1.tgz#cde6b202dbed5c455e5dd383b365d0c9ee3420eb" + dependencies: + expect "^22.1.0" + jest-get-type "^22.4.3" + jest-matcher-utils "^22.0.0" + +jest-get-type@^22.1.0, jest-get-type@^22.4.3: version "22.4.3" resolved "https://registry.yarnpkg.com/jest-get-type/-/jest-get-type-22.4.3.tgz#e3a8504d8479342dd4420236b322869f18900ce4" @@ -2421,6 +2449,14 @@ jest-leak-detector@^23.2.0: dependencies: pretty-format "^23.2.0" +jest-matcher-utils@^22.0.0, jest-matcher-utils@^22.4.3: + version "22.4.3" + resolved "https://registry.yarnpkg.com/jest-matcher-utils/-/jest-matcher-utils-22.4.3.tgz#4632fe428ebc73ebc194d3c7b65d37b161f710ff" + dependencies: + chalk "^2.0.1" + jest-get-type "^22.4.3" + pretty-format "^22.4.3" + jest-matcher-utils@^23.2.0: version "23.2.0" resolved "https://registry.yarnpkg.com/jest-matcher-utils/-/jest-matcher-utils-23.2.0.tgz#4d4981f23213e939e3cedf23dc34c747b5ae1913" @@ -2429,6 +2465,16 @@ jest-matcher-utils@^23.2.0: jest-get-type "^22.1.0" pretty-format "^23.2.0" +jest-message-util@^22.4.3: + version "22.4.3" + resolved "https://registry.yarnpkg.com/jest-message-util/-/jest-message-util-22.4.3.tgz#cf3d38aafe4befddbfc455e57d65d5239e399eb7" + dependencies: + "@babel/code-frame" "^7.0.0-beta.35" + chalk "^2.0.1" + micromatch "^2.3.11" + slash "^1.0.0" + stack-utils "^1.0.1" + jest-message-util@^23.4.0: version "23.4.0" resolved "https://registry.yarnpkg.com/jest-message-util/-/jest-message-util-23.4.0.tgz#17610c50942349508d01a3d1e0bda2c079086a9f" @@ -2443,6 +2489,10 @@ jest-mock@^23.2.0: version "23.2.0" resolved "https://registry.yarnpkg.com/jest-mock/-/jest-mock-23.2.0.tgz#ad1c60f29e8719d47c26e1138098b6d18b261134" +jest-regex-util@^22.4.3: + version "22.4.3" + resolved "https://registry.yarnpkg.com/jest-regex-util/-/jest-regex-util-22.4.3.tgz#a826eb191cdf22502198c5401a1fc04de9cef5af" + jest-regex-util@^23.3.0: version "23.3.0" resolved "https://registry.yarnpkg.com/jest-regex-util/-/jest-regex-util-23.3.0.tgz#5f86729547c2785c4002ceaa8f849fe8ca471bc5" @@ -3431,6 +3481,13 @@ preserve@^0.2.0: version "0.2.0" resolved "https://registry.yarnpkg.com/preserve/-/preserve-0.2.0.tgz#815ed1f6ebc65926f865b310c0713bcb3315ce4b" +pretty-format@^22.4.3: + version "22.4.3" + resolved "https://registry.yarnpkg.com/pretty-format/-/pretty-format-22.4.3.tgz#f873d780839a9c02e9664c8a082e9ee79eaac16f" + dependencies: + ansi-regex "^3.0.0" + ansi-styles "^3.2.0" + pretty-format@^23.2.0: version "23.2.0" resolved "https://registry.yarnpkg.com/pretty-format/-/pretty-format-23.2.0.tgz#3b0aaa63c018a53583373c1cb3a5d96cc5e83017" From c72f4711c9a56a4ccc2041d1ae5c152c1278dda9 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 15:54:32 +1000 Subject: [PATCH 02/39] Completely restructure the app yet again, to better support more events --- index.js | 12 +- package.json | 1 + src/app.js | 230 ++----------------------------------- src/events.js | 153 ++++++++++++++++++++++++ src/helpers.js | 46 ++++++++ src/messages.js | 6 +- src/operations.js | 29 ++++- src/points.js | 67 +++++++++++ src/send.js | 57 +++++++++ tests/_config.js | 5 +- tests/app.js | 190 +----------------------------- tests/events.js | 114 ++++++++++++++++++ tests/helpers.js | 127 ++++++++++++++++++++ tests/integration-tests.js | 27 +++-- tests/operations.js | 26 +++++ tests/send.js | 35 ++++++ yarn.lock | 4 + 17 files changed, 694 insertions(+), 435 deletions(-) create mode 100644 src/events.js create mode 100644 src/helpers.js create mode 100644 src/points.js create mode 100644 src/send.js create mode 100644 tests/events.js create mode 100644 tests/helpers.js create mode 100644 tests/operations.js create mode 100644 tests/send.js diff --git a/index.js b/index.js index d641da8..58ed196 100644 --- a/index.js +++ b/index.js @@ -9,10 +9,12 @@ 'use strict'; -const express = require( 'express' ), - slack = require( '@slack/client' ), - bodyParser = require( 'body-parser' ), - app = require( './src/app' ); +const app = require( './src/app' ), + send = require( './src/send' ); + +const slack = require( '@slack/client' ), + express = require( 'express' ), + bodyParser = require( 'body-parser' ); /* eslint-disable no-process-env, no-magic-numbers */ const PORT = process.env.PORT || 80; // Let Heroku set the port. @@ -33,7 +35,7 @@ const bootstrap = ( options = {}) => { // Allow alternative implementations of both Express and Slack to be passed in. const server = options.express || express(); - app.setSlackClient( options.slack || new slack.WebClient( SLACK_OAUTH_ACCESS_TOKEN ) ); + send.setSlackClient( options.slack || new slack.WebClient( SLACK_OAUTH_ACCESS_TOKEN ) ); server.use( bodyParser.json() ); server.enable( 'trust proxy' ); diff --git a/package.json b/package.json index 100ec48..b8dce02 100644 --- a/package.json +++ b/package.json @@ -29,6 +29,7 @@ "@slack/client": "^4.3.1", "body-parser": "^1.18.3", "express": "^4.16.3", + "lodash.camelcase": "^4.3.0", "pg": "^7.4.3" }, "devDependencies": { diff --git a/src/app.js b/src/app.js index 3a20811..decbf36 100644 --- a/src/app.js +++ b/src/app.js @@ -12,221 +12,13 @@ 'use strict'; -const pg = require( 'pg' ), - { getRandomMessage } = require( './messages' ), - operations = require( './operations' ); +const events = require( './events' ); -let slack; - -// Get environment variables. -/* eslint-disable no-process-env */ -const SLACK_VERIFICATION_TOKEN = process.env.SLACK_VERIFICATION_TOKEN, - DATABASE_URL = process.env.DATABASE_URL, - DATABASE_USE_SSL = 'false' === process.env.DATABASE_USE_SSL ? false : true; -/* eslint-enable no-process-env */ +// eslint-disable-next-line no-process-env +const SLACK_VERIFICATION_TOKEN = process.env.SLACK_VERIFICATION_TOKEN; const HTTP_403 = 403, - HTTP_500 = 500, - scoresTableName = 'scores', - postgresPoolConfig = { - connectionString: DATABASE_URL, - ssl: DATABASE_USE_SSL - }; - -const postgres = new pg.Pool( postgresPoolConfig ); - -/** - * Injects the Slack client to be used for all outgoing messages. - * - * @param {WebClient} client An instance of Slack's WebClient as documented at - * https://slackapi.github.io/node-slack-sdk/web_api and - * implemented at - * https://github.com/slackapi/node-slack-sdk/blob/master/src/WebClient.ts - * @returns {void} - */ -const setSlackClient = ( client ) => { - slack = client; -}; - -/** - * Determines whether or not incoming events from Slack can be handled by this app. - * - * @param {object} event A hash of a Slack event. See the documentation at - * https://api.slack.com/events-api#events_dispatched_as_json and - * https://api.slack.com/events/message for details. - * @returns {bool} Whether or not the app can handle the provided event. - */ -const isValidEvent = ( event ) => { - - // If the event has no type, something has gone wrong. - if ( 'undefined' === typeof event.type ) { - console.warn( 'Event data missing' ); - return false; - } - - // We only support the 'message' event. - if ( 'message' !== event.type ) { - console.warn( 'Invalid event received: ' + event.type ); - return false; - } - - // If the event has a subtype, we don't support it. - if ( 'undefined' !== typeof event.subtype ) { - console.warn( 'Unsupported event subtype: ' + event.subtype ); - return false; - } - - // If there's no text with the message, there's not a lot we can do. - if ( 'undefined' === typeof event.text || ! event.text.trim() ) { - console.warn( 'Message text missing' ); - return false; - } - - return true; - -}; // IsValidEvent. - -/** - * Gets the user or 'thing' that is being spoken about, and the 'operation' being done on it. - * We take the operation down to one character, and also support — due to iOS' replacement of --. - * - * @param {string} text The message text sent through in the event. - * @returns {object} An object containing both the 'item' being referred to - either a Slack user - * ID (eg. 'U12345678') or the name of a 'thing' (eg. 'NameOfThing'); and the - * 'operation' being done on it - expressed as a valid mathematical operation - * (i.e. + or -). - */ -const extractEventData = ( ( text ) => { - const data = text.match( /@([A-Za-z0-9]+?)>?\s*([-+]{2}|—{1})/ ); - - if ( ! data ) { - return false; - } - - return { - item: data[1], - operation: data[2].substring( 0, 1 ).replace( '—', '-' ) - }; -}); - -/** - * Sends a message back to the relevant Slack channel with a response. - * - * @param {string} item The Slack user ID (if user) or name (if thing) of the item being - * operated on. - * @param {string} operation The mathematical operation performed on the item's score. - * @param {int} score The item's score after potentially being updated by the operation. - * @param {object} event A hash of a Slack event. See the documentation at - * https://api.slack.com/events-api#events_dispatched_as_json and - * https://api.slack.com/events/message for details. - * @return {Promise} A Promise to send a Slack message back to the requesting channel. - */ -const respondToUser = ( item, operation, score, event ) => { - - const itemMaybeLinked = item.match( /U[A-Z0-9]{8}/ ) ? '<@' + item + '>' : item; - const operationName = operation.replace( '+', operations.PLUS ).replace( '-', operations.MINUS ); - const message = getRandomMessage( operationName, itemMaybeLinked, score ); - - return new Promise( ( resolve, reject ) => { - slack.chat.postMessage({ - channel: event.channel, - text: message - }).then( ( data ) => { - - if ( ! data.ok ) { - console.error( 'Error occurred posting response.' ); - return reject(); - } - - console.log( item + ' now on ' + score ); - resolve(); - - }); - - }); // Return new Promise. -}; // RespondToUser. - -/** - * Updates the score of an item in the database. If the item doesn't yet exist, it will be inserted - * into the database with an assumed initial score of 0. - * - * This function also sets up the database if it is not already ready, including creating the - * scores table and activating the Postgres case-insensitive extension. - * - * @param {string} item The Slack user ID (if user) or name (if thing) of the item being - * operated on. - * @param {string} operation The mathematical operation performed on the item's score. - * @return {int} The item's new score after the update has been applied. - */ -const updateScore = async( item, operation ) => { - - // Connect to the DB, and create a table if it's not yet there. - // We also set up the citext extension, so that we can easily be case insensitive. - const dbClient = await postgres.connect(); - await dbClient.query( '\ - CREATE EXTENSION IF NOT EXISTS citext; \ - CREATE TABLE IF NOT EXISTS ' + scoresTableName + ' (item CITEXT PRIMARY KEY, score INTEGER); \ - ' ); - - // Atomically record the action. - // TODO: Fix potential SQL injection issues here, even though we know the input should be safe. - await dbClient.query( '\ - INSERT INTO ' + scoresTableName + ' VALUES (\'' + item + '\', ' + operation + '1) \ - ON CONFLICT (item) DO UPDATE SET score = ' + scoresTableName + '.score ' + operation + ' 1; \ - ' ); - - // Get the new value. - // TODO: Fix potential SQL injection issues here, even though we know the input should be safe. - const dbSelect = await dbClient.query( '\ - SELECT score FROM ' + scoresTableName + ' WHERE item = \'' + item + '\'; \ - ' ); - - dbClient.release(); - return dbSelect.rows[0].score; - -}; // UpdateScore. - -/** - * Handles events sent from Slack. - * - * @param {object} event A hash of a Slack event. See the documentation at - * https://api.slack.com/events-api#events_dispatched_as_json and - * https://api.slack.com/events/message for details. - * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise to send a - * Slack message back to the requesting channel. - */ -const handleEvent = async( event ) => { - - const { item, operation } = extractEventData( event.text ); - - if ( ! item || ! operation ) { - return false; - } - - // If the user is trying to ++ themselves... - if ( item === event.user && '+' === operation ) { - - const message = getRandomMessage( operations.SELF, '<@' + event.user + '>', 0 ); - - slack.chat.postMessage({ - channel: event.channel, - text: message - }).then( ( data ) => { - console.log( - data.ok ? - item + ' tried to alter their own score.' : - 'Error occurred posting response to user altering their own score.' - ); - }); - - return false; - - } // If self ++. - - const score = await updateScore( item, operation ); - return respondToUser( item, operation, score, event ); - -}; // HandleEvent. + HTTP_500 = 500; /** * Simple logging of requests. @@ -297,7 +89,7 @@ const handleGet = ( request, response ) => { * @param {express.req} request An Express request. See https://expressjs.com/en/4x/api.html#req. * @param {express.res} response An Express response. See https://expressjs.com/en/4x/api.html#res. * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise as returned - * by `handleEvent()`. + * by `events.handleEvent()`. */ const handlePost = ( request, response ) => { logRequest( request ); @@ -329,20 +121,12 @@ const handlePost = ( request, response ) => { return false; } - // Handle the event now, if it's valid. - if ( isValidEvent( request.body.event ) ) { - return handleEvent( request.body.event ); - } + // Handle the event now. If the event is invalid, this will return false. + return events.handleEvent( request.body.event ); }; // HandlePost. module.exports = { - setSlackClient, - isValidEvent, - extractEventData, - respondToUser, - updateScore, - handleEvent, logRequest, validateToken, handleGet, diff --git a/src/events.js b/src/events.js new file mode 100644 index 0000000..08a5bc9 --- /dev/null +++ b/src/events.js @@ -0,0 +1,153 @@ +/** + * Handles incoming events, using Slack's Events API. See also send.js, which handles outgoing + * messages sent back to Slack. + * + * @see https://api.slack.com/events-api + */ + +'use strict'; + +const send = require( './send' ), + points = require( './points' ), + helpers = require( './helpers' ), + messages = require( './messages' ), + operations = require( './operations' ); + +const camelCase = require( 'lodash.camelcase' ); + +/** + * Handles an attempt by a user to 'self plus' themselves, which includes both logging the attempt + * and letting the user know it wasn't successful. + * + * @param {object} user The ID of the user (Uxxxxxxxx) who tried to self plus. + * @param {object} channel The ID of the channel (Cxxxxxxxx for public channels or Gxxxxxxxx for + * private channels - aka groups) that the message was sent from. + * @return {Promise} A Promise to send a Slack message back to the requesting channel. + */ +const handleSelfPlus = ( user, channel ) => { + console.log( user + ' tried to alter their own score.' ); + const message = messages.getRandomMessage( operations.operations.SELF, '<@' + user + '>', 0 ); + return send.sendMessage( message, channel ); +}; + +/** + * Handles a plus or minus against a user, and then notifies the channel of the new score. + * + * @param {string} item The Slack user ID (if user) or name (if thing) of the item being + * operated on. + * @param {string} operation The mathematical operation performed on the item's score. + * @param {object} channel The ID of the channel (Cxxxxxxxx for public channels or Gxxxxxxxx for + * private channels - aka groups) that the message was sent from. + * @return {Promise} A Promise to send a Slack message back to the requesting channel after the + * points have been updated. + */ +const handlePlusMinus = async( item, operation, channel ) => { + const score = await points.updateScore( item, operation ), + itemMaybeLinked = helpers.maybeLinkItem( item ), + operationName = operations.getOperationName( operation ), + message = messages.getRandomMessage( operationName, itemMaybeLinked, score ); + + return send.sendMessage( message, channel ); +}; + +const handlers = { + + /** + * Handles standard incoming 'message' events sent from Slack. + * + * Assumes basic validation has been done before receiving the event. See handleEvent(). + * + * @param {object} event A hash of a validated Slack 'message' event. See the documentation at + * https://api.slack.com/events-api#events_dispatched_as_json and + * https://api.slack.com/events/message for details. + * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise to send a + * Slack message back to the requesting channel. + */ + message: ( event ) => { + + // Extract the relevant data from the message text. + const { item, operation } = helpers.extractEventData( event.text ); + + console.log( item ); + console.log( operation ); + + if ( ! item || ! operation ) { + return false; + } + + // Bail if the user is trying to ++ themselves... + if ( item === event.user && '+' === operation ) { + handleSelfPlus( event.user, event.channel ); + return false; + } + + // Otherwise, let's go! + return handlePlusMinus( item, operation, event.channel ); + + }, // Message event. + + /** + * Handles 'app_mention' events sent from Slack. + * + * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at + * https://api.slack.com/events-api#events_dispatched_as_json and + * https://api.slack.com/events/app_mention for details. + * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise to send a + * Slack message back to the requesting channel. + */ + appMention: async( event ) => { + + // TODO: Handle this event. + console.log( event ); + + } // AppMention event. + +}; // Handlers. + +/** + * Determines whether or not incoming events from Slack can be handled by this app, and if so, + * passes the event off to its handler function. + * + * @param {object} event A hash of a Slack event. See the documentation at + * https://api.slack.com/events-api#events_dispatched_as_json and + * https://api.slack.com/events/message for details. + * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise as returned + * by the event's handler function. + */ +const handleEvent = ( event ) => { + + // If the event has no type, something has gone wrong. + if ( 'undefined' === typeof event.type ) { + console.warn( 'Event data missing' ); + return false; + } + + // If the event has a subtype, we don't support it. + if ( 'undefined' !== typeof event.subtype ) { + console.warn( 'Unsupported event subtype: ' + event.subtype ); + return false; + } + + // If there's no text with the event, there's not a lot we can do. + if ( 'undefined' === typeof event.text || ! event.text.trim() ) { + console.warn( 'Event text missing' ); + return false; + } + + // Providing we have a handler for the event, let's handle it! + const eventName = camelCase( event.type ); + if ( handlers[ eventName ] instanceof Function ) { + return handlers[ eventName ] ( event ); + } + + console.warn( 'Invalid event received: ' + event.type ); + return false; + +}; // HandleEvent. + +module.exports = { + handleSelfPlus, + handlePlusMinus, + handlers, + handleEvent +}; diff --git a/src/helpers.js b/src/helpers.js new file mode 100644 index 0000000..a33c856 --- /dev/null +++ b/src/helpers.js @@ -0,0 +1,46 @@ +/** + * Contains assorted helper functions. + */ + +'use strict'; + +/** + * Gets the user or 'thing' that is being spoken about, and the 'operation' being done on it. + * We take the operation down to one character, and also support — due to iOS' replacement of --. + * + * @param {string} text The message text sent through in the event. + * @returns {object} An object containing both the 'item' being referred to - either a Slack user + * ID (eg. 'U12345678') or the name of a 'thing' (eg. 'NameOfThing'); and the + * 'operation' being done on it - expressed as a valid mathematical operation + * (i.e. + or -). + */ +const extractEventData = ( ( text ) => { + const data = text.match( /@([A-Za-z0-9]+?)>?\s*([-+]{2}|—{1})/ ); + + if ( ! data ) { + return false; + } + + return { + item: data[1], + operation: data[2].substring( 0, 1 ).replace( '—', '-' ) + }; + +}); + +/** + * Takes an item and returns it maybe linked using Slack's 'mrkdwn' format (their own custom + * version of markdown). + * + * @param {string} item A raw 'item' - either a Slack user ID, or the name of a 'thing'. + * @return {string} The item linked with Slack mrkdwn + * @see https://api.slack.com/docs/message-formatting#linking_to_channels_and_users + */ +const maybeLinkItem = ( item ) => { + return item.match( /U[A-Z0-9]{8}/ ) ? '<@' + item + '>' : item; +}; + +module.exports = { + extractEventData, + maybeLinkItem +}; diff --git a/src/messages.js b/src/messages.js index 3621077..45a948d 100644 --- a/src/messages.js +++ b/src/messages.js @@ -9,7 +9,7 @@ 'use strict'; -const operations = require( './operations' ); +const operations = require( './operations' ).operations; const messages = {}; messages[ operations.PLUS ] = [ @@ -147,6 +147,6 @@ const getRandomMessage = ( operation, item, score ) => { }; // GetRandomMessage. module.exports = { - getRandomMessage, - messages + messages, + getRandomMessage }; diff --git a/src/operations.js b/src/operations.js index a78ee02..6805aa1 100644 --- a/src/operations.js +++ b/src/operations.js @@ -1,11 +1,36 @@ /** - * Provides constants for operations. + * Provides constants and supporting functions for operations. */ 'use strict'; -module.exports = { +const operations = { PLUS: 'plus', MINUS: 'minus', SELF: 'selfPlus' }; + +/** + * Given a mathematical operation, returns the name of that operation. + * + * @param {string} operation A mathematical operation such as '+' or '-'. + * @return {string} The name of the operation. + */ +const getOperationName = ( operation ) => { + let operationName = ''; + + /* eslint-disable max-statements-per-line */ + switch ( operation ) { + case '+': operationName = operations.PLUS; break; + case '-': operationName = operations.MINUS; break; + } + /* eslint-enable max-statements-per-line */ + + return operationName ? operationName : false; + +}; + +module.exports = { + operations, + getOperationName +}; diff --git a/src/points.js b/src/points.js new file mode 100644 index 0000000..d3c214a --- /dev/null +++ b/src/points.js @@ -0,0 +1,67 @@ +/** + * All the stuff that handles the giving, taking away, or otherwise querying of points. + */ + +'use strict'; + +const pg = require( 'pg' ); + +/* eslint-disable no-process-env */ +const DATABASE_URL = process.env.DATABASE_URL, + DATABASE_USE_SSL = 'false' === process.env.DATABASE_USE_SSL ? false : true; +/* eslint-enable no-process-env */ + +const scoresTableName = 'scores', + postgresPoolConfig = { + connectionString: DATABASE_URL, + ssl: DATABASE_USE_SSL + }; + +const postgres = new pg.Pool( postgresPoolConfig ); + +/** + * Updates the score of an item in the database. If the item doesn't yet exist, it will be inserted + * into the database with an assumed initial score of 0. + * + * This function also sets up the database if it is not already ready, including creating the + * scores table and activating the Postgres case-insensitive extension. + * + * @param {string} item The Slack user ID (if user) or name (if thing) of the item being + * operated on. + * @param {string} operation The mathematical operation performed on the item's score. + * @return {int} The item's new score after the update has been applied. + */ +const updateScore = async( item, operation ) => { + + // Connect to the DB, and create a table if it's not yet there. + // We also set up the citext extension, so that we can easily be case insensitive. + const dbClient = await postgres.connect(); + await dbClient.query( '\ + CREATE EXTENSION IF NOT EXISTS citext; \ + CREATE TABLE IF NOT EXISTS ' + scoresTableName + ' (item CITEXT PRIMARY KEY, score INTEGER); \ + ' ); + + // Atomically record the action. + // TODO: Fix potential SQL injection issues here, even though we know the input should be safe. + await dbClient.query( '\ + INSERT INTO ' + scoresTableName + ' VALUES (\'' + item + '\', ' + operation + '1) \ + ON CONFLICT (item) DO UPDATE SET score = ' + scoresTableName + '.score ' + operation + ' 1; \ + ' ); + + // Get the new value. + // TODO: Fix potential SQL injection issues here, even though we know the input should be safe. + const dbSelect = await dbClient.query( '\ + SELECT score FROM ' + scoresTableName + ' WHERE item = \'' + item + '\'; \ + ' ); + + dbClient.release(); + const score = dbSelect.rows[0].score; + + console.log( item + ' now on ' + score ); + return score; + +}; // UpdateScore. + +module.exports = { + updateScore +}; diff --git a/src/send.js b/src/send.js new file mode 100644 index 0000000..84d3745 --- /dev/null +++ b/src/send.js @@ -0,0 +1,57 @@ +/** + * Handles sending of messages - i.e. outgoing messages - back to Slack, via Slack's Web API. See + * also ./events.js, which handles incoming messages from subscribed events. + * + * @see https://api.slack.com/web + */ + +'use strict'; + +let slack; + +/** + * Injects the Slack client to be used for all outgoing messages. + * + * @param {WebClient} client An instance of Slack's WebClient as documented at + * https://slackapi.github.io/node-slack-sdk/web_api and + * implemented at + * https://github.com/slackapi/node-slack-sdk/blob/master/src/WebClient.ts + * @returns {void} + */ +const setSlackClient = ( client ) => { + slack = client; +}; + +/** + * Sends a message to a Slack channel. + * + * @param {string} message The message text to send. + * @param {string} channel The ID of the channel to send the message to. + * @return {Promise} A Promise to send the message to Slack. + */ +const sendMessage = ( text, channel ) => { + + const payload = { + channel, + text + }; + + return new Promise( ( resolve, reject ) => { + slack.chat.postMessage( payload ).then( ( data ) => { + + if ( ! data.ok ) { + console.error( 'Error occurred posting response.' ); + return reject(); + } + + resolve(); + + }); + + }); // Return new Promise. +}; // SendMessage. + +module.exports = { + setSlackClient, + sendMessage +}; diff --git a/tests/_config.js b/tests/_config.js index ec939ff..0d8f410 100644 --- a/tests/_config.js +++ b/tests/_config.js @@ -23,7 +23,10 @@ module.exports = { host: 'localhost', method: 'POST', port: PORT, - headers: { 'Content-Type': 'application/json' } + headers: { + 'Content-Type': 'application/json', + 'User-Agent': 'Test Runner' + } } }; diff --git a/tests/app.js b/tests/app.js index 8642495..f2f8dfd 100644 --- a/tests/app.js +++ b/tests/app.js @@ -9,10 +9,7 @@ 'use strict'; -const app = require( '../src/app' ), - slackClientMock = require( './mocks/slack' ); - -app.setSlackClient( slackClientMock ); +const app = require( '../src/app' ); // Catch all console output during tests. console.error = jest.fn(); @@ -20,191 +17,6 @@ console.info = jest.fn(); console.log = jest.fn(); console.warn = jest.fn(); -describe( 'isValidEvent', () => { - - it( 'reports an event with message and text as valid', () => { - const event = { - type: 'message', - text: 'Hello' - }; - - expect( app.isValidEvent( event ) ).toBe( true ); - }); - - it( 'reports an event with missing type as invalid', () => { - const event = { text: 'Hello' }; - expect( app.isValidEvent( event ) ).toBe( false ); - }); - - it( 'reports an event without type \'message\' as invalid', () => { - const event = { - type: 'random', - text: 'Hello' - }; - - expect( app.isValidEvent( event ) ).toBe( false ); - }); - - it( 'reports an event with a subtype as invalid', () => { - const event = { - type: 'message', - subtype: 'random', - text: 'Hello' - }; - - expect( app.isValidEvent( event ) ).toBe( false ); - }); - - it( 'reports an event without text as invalid', () => { - const event = { type: 'message' }; - expect( app.isValidEvent( event ) ).toBe( false ); - }); - - it( 'reports an event with only a space for text as invalid', () => { - const event = { - type: 'message', - text: ' ' - }; - - expect( app.isValidEvent( event ) ).toBe( false ); - }); - -}); // IsValidEvent. - -describe( 'extractEventData', () => { - - it( 'drops message without an @ symbol', () => { - expect( app.extractEventData( 'Hello++' ) ).toBe( false ); - }); - - it( 'drops messages without a valid operation', () => { - expect( app.extractEventData( '@Hello' ) ).toBe( false ); - }); - - it( 'drops messages without a valid user/item', () => { - expect( app.extractEventData( '@++' ) ).toBe( false ); - }); - - it( 'extracts a \'thing\' and operation from the start of a message', () => { - expect( app.extractEventData( '@SomethingRandom++ that was awesome' ) ).toEqual({ - item: 'SomethingRandom', - operation: '+' - }); - }); - - it( 'extracts a user and operation from the start of a message', () => { - expect( app.extractEventData( '<@U87654321>++ that was awesome' ) ).toEqual({ - item: 'U87654321', - operation: '+' - }); - }); - - it( 'extracts data in the middle of a message', () => { - expect( app.extractEventData( 'Hey @SomethingRandom++ that was awesome' ) ).toEqual({ - item: 'SomethingRandom', - operation: '+' - }); - }); - - it( 'extracts data at the end of a message', () => { - expect( app.extractEventData( 'Awesome work @SomethingRandom++' ) ).toEqual({ - item: 'SomethingRandom', - operation: '+' - }); - }); - - const itemsToMatch = [ - { - supplied: '<@U1234567890>', - expected: 'U1234567890' - }, - { - supplied: '@SomethingRandom', - expected: 'SomethingRandom' - }, - { - supplied: '@SomethingRandom123', - expected: 'SomethingRandom123' - } - ]; - - const operationsToMatch = [ - { - supplied: '++', - expected: '+' - }, - { - supplied: '--', - expected: '-' - }, - { - supplied: '—', // Emdash, which iOS replaces -- with. - expected: '-' - } - ]; - - const operationsNotToMatch = [ - '+', - '-' - ]; - - for ( const item of itemsToMatch ) { - - for ( const operation of operationsToMatch ) { - for ( let iterator = 0; 1 >= iterator; iterator++ ) { - - const space = 1 === iterator ? ' ' : '', - messageText = item.supplied + space + operation.supplied, - testName = ( - 'matches ' + messageText + ' as ' + item.expected + ' and ' + operation.expected - ); - - it( testName, () => { - const result = app.extractEventData( messageText ); - expect( result ).toEqual({ - item: item.expected, - operation: operation.expected - }); - }); - - } // For iterator. - } // For operationsToMatch. - - for ( const operation of operationsNotToMatch ) { - const messageText = item.supplied + operation; - it( 'does NOT match ' + messageText, () => { - expect( app.extractEventData( messageText ) ).toBe( false ); - }); - } - - } // For itemsToMatch. -}); // ExtractEventData. - -/** - * These functions are both suitably covered in the end-to-end tests, and are probably difficult to - * test as individual units. - */ -describe( 'respondToUser', () => {}); -describe( 'updateScore', () => {}); - -describe( 'handleEvent', () => { - - it( 'drops a user trying to ++ themselves', () => { - const event = { - type: 'message', - text: '<@U12345678>++', - user: 'U12345678' - }; - - expect.hasAssertions(); - - return app.handleEvent( event ).then( ( data ) => { - expect( data ).toBe( false ); - }); - }); - -}); - describe( 'logRequest', () => { it( 'logs request data to stdout', () => { diff --git a/tests/events.js b/tests/events.js new file mode 100644 index 0000000..d865810 --- /dev/null +++ b/tests/events.js @@ -0,0 +1,114 @@ +/** + * Unit tests on the event handlers in events.js. + * + * TODO: These tests are currently generating an unhandled Promise (probably from the Slack client) + * and need to be updated. + * + * @see https://jestjs.io/docs/en/expect + * @author Tim Malone + */ + +/* global jest */ + +'use strict'; + +const events = require( '../src/events' ); +const handlers = events.handlers; + +// Catch all console output during tests. +console.error = jest.fn(); +console.info = jest.fn(); +console.log = jest.fn(); +console.warn = jest.fn(); + +describe( 'handleSelfPlus', () => { + + // TODO: + +}); + +describe( 'handlePlusMinus', () => { + + // TODO: + +}); + +describe( 'handlers.message', () => { + + it( 'drops a user trying to ++ themselves', () => { + const event = { + type: 'message', + text: '<@U12345678>++', + user: 'U12345678' + }; + + expect( handlers.message( event ) ).toBeFalse(); + }); + +}); // HandleMessageEvent. + +describe( 'handlers.appMention', () => { + + // TODO: + +}); + +describe( 'handleEvent', () => { + + const validEvents = [ + 'message', + 'app_mention' + ]; + + for ( const eventName of validEvents ) { + + it( 'returns a Promise for a \'' + eventName + '\' event with text', () => { + const event = { + type: eventName, + text: '@Hello++' + }; + + expect( events.handleEvent( event ) instanceof Promise ).toBeTrue(); + }); + + it( 'reports a \'' + eventName + '\' event without text as invalid', () => { + const event = { type: eventName }; + expect( events.handleEvent( event ) ).toBeFalse(); + }); + + it( 'reports a \'' + eventName + '\' event with only a space for text as invalid', () => { + const event = { + type: eventName, + text: ' ' + }; + + expect( events.handleEvent( event ) ).toBeFalse(); + }); + + } // For validEvents. + + it( 'reports an event with missing type as invalid', () => { + const event = { text: 'Hello' }; + expect( events.handleEvent( event ) ).toBeFalse(); + }); + + it( 'reports an event with some random type as invalid', () => { + const event = { + type: 'random', + text: 'Hello' + }; + + expect( events.handleEvent( event ) ).toBeFalse(); + }); + + it( 'reports an event with a subtype as invalid', () => { + const event = { + type: 'message', + subtype: 'random', + text: 'Hello' + }; + + expect( events.handleEvent( event ) ).toBeFalse(); + }); + +}); // HandleEvent. diff --git a/tests/helpers.js b/tests/helpers.js new file mode 100644 index 0000000..e8eba89 --- /dev/null +++ b/tests/helpers.js @@ -0,0 +1,127 @@ +/** + * Unit tests on the helpers in helpers.js. + * + * @see https://jestjs.io/docs/en/expect + * @author Tim Malone + */ + +/* global jest */ + +'use strict'; + +const helpers = require( '../src/helpers' ); + +describe( 'extractEventData', () => { + + it( 'drops message without an @ symbol', () => { + expect( helpers.extractEventData( 'Hello++' ) ).toBeFalse(); + }); + + it( 'drops messages without a valid operation', () => { + expect( helpers.extractEventData( '@Hello' ) ).toBeFalse(); + }); + + it( 'drops messages without a valid user/item', () => { + expect( helpers.extractEventData( '@++' ) ).toBeFalse(); + }); + + it( 'extracts a \'thing\' and operation from the start of a message', () => { + expect( helpers.extractEventData( '@SomethingRandom++ that was awesome' ) ).toEqual({ + item: 'SomethingRandom', + operation: '+' + }); + }); + + it( 'extracts a user and operation from the start of a message', () => { + expect( helpers.extractEventData( '<@U87654321>++ that was awesome' ) ).toEqual({ + item: 'U87654321', + operation: '+' + }); + }); + + it( 'extracts data in the middle of a message', () => { + expect( helpers.extractEventData( 'Hey @SomethingRandom++ that was awesome' ) ).toEqual({ + item: 'SomethingRandom', + operation: '+' + }); + }); + + it( 'extracts data at the end of a message', () => { + expect( helpers.extractEventData( 'Awesome work @SomethingRandom++' ) ).toEqual({ + item: 'SomethingRandom', + operation: '+' + }); + }); + + const itemsToMatch = [ + { + supplied: '<@U1234567890>', + expected: 'U1234567890' + }, + { + supplied: '@SomethingRandom', + expected: 'SomethingRandom' + }, + { + supplied: '@SomethingRandom123', + expected: 'SomethingRandom123' + } + ]; + + const operationsToMatch = [ + { + supplied: '++', + expected: '+' + }, + { + supplied: '--', + expected: '-' + }, + { + supplied: '—', // Emdash, which iOS replaces -- with. + expected: '-' + } + ]; + + const operationsNotToMatch = [ + '+', + '-' + ]; + + for ( const item of itemsToMatch ) { + + for ( const operation of operationsToMatch ) { + for ( let iterator = 0; 1 >= iterator; iterator++ ) { + + const space = 1 === iterator ? ' ' : '', + messageText = item.supplied + space + operation.supplied, + testName = ( + 'matches ' + messageText + ' as ' + item.expected + ' and ' + operation.expected + ); + + it( testName, () => { + const result = helpers.extractEventData( messageText ); + expect( result ).toEqual({ + item: item.expected, + operation: operation.expected + }); + }); + + } // For iterator. + } // For operationsToMatch. + + for ( const operation of operationsNotToMatch ) { + const messageText = item.supplied + operation; + it( 'does NOT match ' + messageText, () => { + expect( helpers.extractEventData( messageText ) ).toBeFalse(); + }); + } + + } // For itemsToMatch. +}); // ExtractEventData. + +describe( 'maybeLinkItem', () => { + + // TODO: + +}); // MaybeLinkItem. diff --git a/tests/integration-tests.js b/tests/integration-tests.js index 312f575..987e00d 100644 --- a/tests/integration-tests.js +++ b/tests/integration-tests.js @@ -15,15 +15,18 @@ * Environment Configuration. ****************************************************************/ -const http = require( 'http' ), - pg = require( 'pg' ), - app = require( '../src/app' ), - config = require( './_config' ), +const app = require( '../src/app' ); +const pathToListener = '../'; + +const pg = require( 'pg' ), + http = require( 'http' ); + +const config = require( './_config' ), runner = require( './_runner' ), slackClientMock = require( './mocks/slack' ); -const originalProcessEnv = process.env; -const postgres = new pg.Pool( config.postgresPoolConfig ); +const originalProcessEnv = process.env, + postgres = new pg.Pool( config.postgresPoolConfig ); /**************************************************************** * Jest Setup. @@ -58,7 +61,7 @@ describe( 'The Express server', () => { it( 'returns HTTP 200 for GET operations', ( done ) => { expect.hasAssertions(); - const listener = require( '../' )(); + const listener = require( pathToListener )(); listener.on( 'listening', () => { http.get( 'http://localhost:' + config.PORT, ( response ) => { @@ -73,7 +76,7 @@ describe( 'The Express server', () => { it( 'correctly returns the Slack event challenge value', ( done ) => { expect.assertions( 2 ); - const listener = require( '../' )(); + const listener = require( pathToListener )(); const requestBody = { challenge: Math.random().toString() }; listener.on( 'listening', () => { @@ -100,7 +103,7 @@ describe( 'The Express server', () => { expect.hasAssertions(); delete process.env.SLACK_VERIFICATION_TOKEN; - const listener = require( '../' )(); + const listener = require( pathToListener )(); listener.on( 'listening', () => { http.request( config.defaultRequestOptions, ( response ) => { @@ -116,7 +119,7 @@ describe( 'The Express server', () => { expect.hasAssertions(); process.env.SLACK_VERIFICATION_TOKEN = 'xxxxxxxxxxxxxxxxxxxxxxxx'; - const listener = require( '../' )(); + const listener = require( pathToListener )(); listener.on( 'listening', () => { http.request( config.defaultRequestOptions, ( response ) => { @@ -131,7 +134,7 @@ describe( 'The Express server', () => { it( 'returns HTTP 403 when verification token is incorrect', ( done ) => { expect.hasAssertions(); - const listener = require( '../' )(); + const listener = require( pathToListener )(); const body = { token: 'something_is_not_right' }; listener.on( 'listening', () => { @@ -213,7 +216,7 @@ describe( 'The database', () => { */ const doFirstRequest = ( done ) => { expect.hasAssertions(); - const listener = require( '../' )({ slack: slackClientMock }); + const listener = require( pathToListener )({ slack: slackClientMock }); listener.on( 'listening', () => { runner( '@something++', async( dbClient ) => { diff --git a/tests/operations.js b/tests/operations.js new file mode 100644 index 0000000..3c41b3b --- /dev/null +++ b/tests/operations.js @@ -0,0 +1,26 @@ +/** + * Unit tests on operations.js. + * + * @see https://jestjs.io/docs/en/expect + * @author Tim Malone + */ + +/* global jest */ + +'use strict'; + +const operations = require( '../src/operations' ); + +it( 'exports constants for operations', () => { + expect( operations.operations ) + .toBeObject() + .toHaveProperty( 'PLUS' ) + .toHaveProperty( 'MINUS' ) + .toHaveProperty( 'SELF' ); +}); + +describe( 'getOperationName', () => { + + // TODO: + +}); diff --git a/tests/send.js b/tests/send.js new file mode 100644 index 0000000..d8d6b3e --- /dev/null +++ b/tests/send.js @@ -0,0 +1,35 @@ +/** + * Unit tests on the code in send.js. + * + * @see https://jestjs.io/docs/en/expect + * @author Tim Malone + */ + +/* global jest */ + +'use strict'; + +const send = require( '../src/send' ); +const slackClientMock = require( './mocks/slack' ); + +send.setSlackClient( slackClientMock ); + +// Catch all console output during tests. +console.error = jest.fn(); +console.info = jest.fn(); +console.log = jest.fn(); +console.warn = jest.fn(); + +describe( 'setSlackClient', () => { + + it( 'accepts a single parameter (that is later used as the Slack API client)', () => { + expect( send.setSlackClient ).toHaveLength( 1 ); + }); + +}); + +describe( 'sendMessage', () => { + + // TODO: + +}); diff --git a/yarn.lock b/yarn.lock index d47c9c8..6d7722c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2793,6 +2793,10 @@ locate-path@^2.0.0: p-locate "^2.0.0" path-exists "^3.0.0" +lodash.camelcase@^4.3.0: + version "4.3.0" + resolved "https://registry.yarnpkg.com/lodash.camelcase/-/lodash.camelcase-4.3.0.tgz#b28aa6288a2b9fc651035c7711f65ab6190331a6" + lodash.sortby@^4.7.0: version "4.7.0" resolved "https://registry.yarnpkg.com/lodash.sortby/-/lodash.sortby-4.7.0.tgz#edd14c824e2cc9c1e0b0a1b42bb5210516a42438" From 5fa0b77a22a567a9d343da9aca0bb8bcf4e1c941 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 15:56:36 +1000 Subject: [PATCH 03/39] Add simple leaderboard functionality via app_mention event --- README.md | 29 ++++++++++++++++++++++++++--- src/events.js | 3 --- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index cd5fc01..3ee0ee2 100644 --- a/README.md +++ b/README.md @@ -68,16 +68,39 @@ Completely open source, so do with it what you like. Or if you don't want to mak Via *Event Subscriptions* in the left menu. After switching on, enter your new Heroku app address - eg. `https://my-plusplus.herokuapp.com` - as the request URL. - Scroll down and, under *Subscribe to Bot Events*, add the `message.channels` and `message.groups` events, then click *Save Changes*. + Scroll down and, under *Subscribe to Bot Events*, select the relevant events for the features you want the app to support: + + * Select `message.channels` to support all general features in _public_ channels it is invited to + * Select `message.groups` to support all general features in _private_ channels it is invited to + * Select `app_mention` to support extended features such as leaderboards + + Finally, click *Save Changes*. If you wish, you can come back to this screen later and add or change the events the app handles. 1. **Invite your new bot to any channel in your Slack team.** 1. **Think of someone who's been awesome lately and send `@Someone++`!** -## Detailed Instructions +### More Information Further instructions, such as hosting elsewhere, upgrading, etc. are coming soon. +## Usage + +**Working PlusPlus++** will listen out for messages, in channels it has been invited to, for valid commands. Commands are accepted anywhere in a message - at the beginning, middle, or end - and are currently limited to one command per message (if multiple commands are sent, only the first one found will be handled). + +Currently supported general commands are: + +* `@Someone++`: Adds points to a user or a thing +* `@Someone--`: Subtracts points from a user or a thing + +Currently supported extended commands are: + +* `@WorkingPlusPlus leaderboard`: Displays the leaderboard for your Slack workspace + +If you set a different name for your bot when adding the app to your Slack workspace, use that name instead. + +ℹ️ _Extended commands are supported if you've subscribed to the `app_mentions` event in your Slack app settings. See **Step 6** in the installation instructions above for further details._ + ## Contributing Your contributions are welcome! [Create an issue](https://github.com/tdmalone/working-plusplus/issues/new) if there's something you'd like to see or [send a pull request](https://github.com/tdmalone/working-plusplus/compare) if you can implement it yourself. @@ -94,7 +117,7 @@ Although it works, it's very basic. Potential enhancements include: * Move to the newer, more secure method of calculating signatures for incoming Slack hooks * A way to look up someone's karma without necessarily `++`'ing or `--`'ing them (eg. `@username==`) * Support for posting back messages within threads, rather than automatically jumping back out to the channel -* Support for detecting multiple actions within one message +* Support for detecting multiple commands within one message * Natural language processing to figure out positive and negative sentiment automatically * Option to deduct karma instead of adding karma when someone tries to give themselves karma * Option to deduct karma automatically for swearing (with customisable word list?) diff --git a/src/events.js b/src/events.js index 08a5bc9..3b152ed 100644 --- a/src/events.js +++ b/src/events.js @@ -68,9 +68,6 @@ const handlers = { // Extract the relevant data from the message text. const { item, operation } = helpers.extractEventData( event.text ); - console.log( item ); - console.log( operation ); - if ( ! item || ! operation ) { return false; } From ad58d511e5daea562778c3101d10fe5624b5ad6f Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 16:39:12 +1000 Subject: [PATCH 04/39] Complete tests for send.js --- tests/mocks/slack.js | 21 ++++++++++++------- tests/send.js | 49 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/tests/mocks/slack.js b/tests/mocks/slack.js index bb852f7..e2b9059 100644 --- a/tests/mocks/slack.js +++ b/tests/mocks/slack.js @@ -8,12 +8,19 @@ /* eslint-disable no-empty-function */ -module.exports = { - chat: { - postMessage: ( payload ) => { // eslint-disable-line no-unused-vars - return new Promise( ( resolve ) => { - resolve({ ok: true }); - }); - } +const options = { + shouldPostMessageSucceed: true +}; + +const chat = { + postMessage: ( payload ) => { // eslint-disable-line no-unused-vars + return new Promise( ( resolve ) => { + resolve({ ok: options.shouldPostMessageSucceed }); + }); } }; + +module.exports = { + options, + chat +}; diff --git a/tests/send.js b/tests/send.js index d8d6b3e..d3b282e 100644 --- a/tests/send.js +++ b/tests/send.js @@ -10,9 +10,7 @@ 'use strict'; const send = require( '../src/send' ); -const slackClientMock = require( './mocks/slack' ); - -send.setSlackClient( slackClientMock ); +const pathToMock = './mocks/slack'; // Catch all console output during tests. console.error = jest.fn(); @@ -20,6 +18,11 @@ console.info = jest.fn(); console.log = jest.fn(); console.warn = jest.fn(); +// Clear module cache due to us sometimes messing with the mock. +beforeEach( () => { + jest.resetModules(); +}); + describe( 'setSlackClient', () => { it( 'accepts a single parameter (that is later used as the Slack API client)', () => { @@ -30,6 +33,44 @@ describe( 'setSlackClient', () => { describe( 'sendMessage', () => { - // TODO: + const payload = { + text: 'Hello there', + channel: 'C12345678' + }; + + it( 'sends the provided message text to the provided channel via the Slack Web API', () => { + expect.assertions( 1 ); + const slackClientMock = require( pathToMock ); + send.setSlackClient( slackClientMock ); + + // Re-mock the client so we can listen to it. + slackClientMock.chat.postMessage = jest.fn(); + + send.sendMessage( payload.text, payload.channel ).catch( () => { + expect( slackClientMock.chat.postMessage ).toHaveBeenCalledWith( payload ); + }); + }); + + it( 'returns a Promise and resolves it if the message succeeds', () => { + expect.assertions( 1 ); + const slackClientMock = require( pathToMock ); + send.setSlackClient( slackClientMock ); + + slackClientMock.options.shouldPostMessageSucceed = true; + return send.sendMessage( payload.text, payload.channel ).then( ( data ) => { + expect( data ).toBeNil(); + }); + }); + + it( 'returns a Promise and rejects it if the message fails', () => { + expect.assertions( 1 ); + const slackClientMock = require( pathToMock ); + send.setSlackClient( slackClientMock ); + + slackClientMock.options.shouldPostMessageSucceed = false; + return send.sendMessage( payload.text, payload.channel ).catch( ( error ) => { + expect( error ).toBeNil(); + }); + }); }); From e65e7b951ff3bac301cd17c287e1c6a12f8e1ad4 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 16:41:42 +1000 Subject: [PATCH 05/39] Complete tests for operations.js --- tests/operations.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/operations.js b/tests/operations.js index 3c41b3b..0d6a037 100644 --- a/tests/operations.js +++ b/tests/operations.js @@ -21,6 +21,16 @@ it( 'exports constants for operations', () => { describe( 'getOperationName', () => { - // TODO: + it( 'returns \'plus\' when given +', () => { + expect( operations.getOperationName( '+' ) ).toBe( 'plus' ); + }); + + it( 'returns \'minus\' when given -', () => { + expect( operations.getOperationName( '-' ) ).toBe( 'minus' ); + }); + + it( 'returns false when given an invalid operation', () => { + expect( operations.getOperationName( 'some invalid operation' ) ).toBeFalse(); + }); }); From a2769f8c19e22dab93b40bec279bc96c96255d5e Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 17:13:20 +1000 Subject: [PATCH 06/39] Complete tests for helpers.js --- tests/helpers.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/helpers.js b/tests/helpers.js index e8eba89..d478e34 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -122,6 +122,14 @@ describe( 'extractEventData', () => { describe( 'maybeLinkItem', () => { - // TODO: + it( 'returns an item as-is if it is not a Slack user ID', () => { + const item = 'something'; + expect( helpers.maybeLinkItem( item ) ).toBe( item ); + }); + + it( 'returns an item linked with Slack mrkdown if it looks like a Slack user ID', () => { + const item = 'U12345678'; + expect( helpers.maybeLinkItem( item ) ).toBe( '<@' + item + '>' ); + }); }); // MaybeLinkItem. From 5721a936ee76b5f566fb644455bbc4bac5dd1151 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 18:55:43 +1000 Subject: [PATCH 07/39] Minor: move item linking into messages.js and allow score to be optional --- src/events.js | 5 ++--- src/messages.js | 12 +++++++----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/events.js b/src/events.js index 3b152ed..4a5b0a2 100644 --- a/src/events.js +++ b/src/events.js @@ -26,7 +26,7 @@ const camelCase = require( 'lodash.camelcase' ); */ const handleSelfPlus = ( user, channel ) => { console.log( user + ' tried to alter their own score.' ); - const message = messages.getRandomMessage( operations.operations.SELF, '<@' + user + '>', 0 ); + const message = messages.getRandomMessage( operations.operations.SELF, user ); return send.sendMessage( message, channel ); }; @@ -43,9 +43,8 @@ const handleSelfPlus = ( user, channel ) => { */ const handlePlusMinus = async( item, operation, channel ) => { const score = await points.updateScore( item, operation ), - itemMaybeLinked = helpers.maybeLinkItem( item ), operationName = operations.getOperationName( operation ), - message = messages.getRandomMessage( operationName, itemMaybeLinked, score ); + message = messages.getRandomMessage( operationName, item, score ); return send.sendMessage( message, channel ); }; diff --git a/src/messages.js b/src/messages.js index 45a948d..6331dab 100644 --- a/src/messages.js +++ b/src/messages.js @@ -9,7 +9,9 @@ 'use strict'; -const operations = require( './operations' ).operations; +const helpers = require( './helpers' ), + operations = require( './operations' ).operations; + const messages = {}; messages[ operations.PLUS ] = [ @@ -81,12 +83,12 @@ messages[ operations.SELF ] = [ * * @param {string} operation The name of the operation to retrieve potential messages for. * See operations.js. - * @param {string} item The subject of the message, either "<@user>" or "object". - * @param {integer} score The item's current score. + * @param {string} item The subject of the message, eg. 'U12345678' or 'SomeRandomThing'. + * @param {integer} score The item's current score. Defaults to 0 if not supplied. * * @returns {string} A random message from the chosen pool. */ -const getRandomMessage = ( operation, item, score ) => { +const getRandomMessage = ( operation, item, score = 0 ) => { const messageSets = messages[ operation ]; @@ -137,7 +139,7 @@ const getRandomMessage = ( operation, item, score ) => { const random = Math.floor( Math.random() * max ); const message = chosenSet[ random ]; - const formattedMessage = format.replace( '', item ) + const formattedMessage = format.replace( '', helpers.maybeLinkItem( item ) ) .replace( '', score ) .replace( '', plural ) .replace( '', message ); From f6f3e4a1e15fd9de4b754abe93bfce9079f9436b Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 18:57:41 +1000 Subject: [PATCH 08/39] Minor cleanups for messages.js --- src/messages.js | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/messages.js b/src/messages.js index 6331dab..e36b677 100644 --- a/src/messages.js +++ b/src/messages.js @@ -91,12 +91,7 @@ messages[ operations.SELF ] = [ const getRandomMessage = ( operation, item, score = 0 ) => { const messageSets = messages[ operation ]; - - let setRandom, - set, - totalProbability = 0, - chosenSet = null, - format = ''; + let format = ''; switch ( operation ) { case operations.MINUS: @@ -112,32 +107,33 @@ const getRandomMessage = ( operation, item, score = 0 ) => { throw 'Invalid operation: ' + operation; } - for ( set of messageSets ) { + let totalProbability = 0; + for ( const set of messageSets ) { totalProbability += set.probability; } - setRandom = Math.floor( Math.random() * totalProbability ); + let chosenSet = null, + setRandom = Math.floor( Math.random() * totalProbability ); - for ( set of messageSets ) { + for ( const set of messageSets ) { setRandom -= set.probability; if ( 0 > setRandom ) { chosenSet = set.set; - break; } } if ( null === chosenSet ) { throw ( - 'Could not find set for ' + operation + ' ran out of sets with ' + setRandom + ' remaining' + 'Could not find set for ' + operation + ' (ran out of sets with ' + setRandom + ' remaining)' ); } - const plural = 1 === Math.abs( score ) ? '' : 's'; - const max = chosenSet.length - 1; - const random = Math.floor( Math.random() * max ); - const message = chosenSet[ random ]; + const plural = 1 === Math.abs( score ) ? '' : 's', + max = chosenSet.length - 1, + random = Math.floor( Math.random() * max ), + message = chosenSet[ random ]; const formattedMessage = format.replace( '', helpers.maybeLinkItem( item ) ) .replace( '', score ) From 3db8ae82eefe855216b59e84d2c17ac3bd9c17c7 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 18:58:15 +1000 Subject: [PATCH 09/39] Complete tests for events.js --- package.json | 2 +- tests/events.js | 131 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 127 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index b8dce02..d4cf0a4 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,7 @@ "lint": "eslint --color --ignore-pattern '/coverage/' --ignore-pattern '!.*.js' \"**/*.js\"", "fix": "yarn lint --fix", "test": "jest --forceExit --runInBand", - "unit-tests": "SKIP_INTEGRATION_TESTS=true SKIP_E2E_TESTS=true jest", + "unit-tests": "SKIP_INTEGRATION_TESTS=true SKIP_E2E_TESTS=true jest --forceExit", "integration-tests": "SKIP_E2E_TESTS=true jest --forceExit integration-tests", "e2e-tests": "SKIP_INTEGRATION_TESTS=true jest --forceExit e2e-tests", "report-coverage": "codecov && cat ./coverage/lcov.info | codacy-coverage" diff --git a/tests/events.js b/tests/events.js index d865810..daccdb9 100644 --- a/tests/events.js +++ b/tests/events.js @@ -1,9 +1,6 @@ /** * Unit tests on the event handlers in events.js. * - * TODO: These tests are currently generating an unhandled Promise (probably from the Slack client) - * and need to be updated. - * * @see https://jestjs.io/docs/en/expect * @author Tim Malone */ @@ -15,21 +12,145 @@ const events = require( '../src/events' ); const handlers = events.handlers; +const send = require( '../src/send' ), + points = require( '../src/points' ), + messages = require( '../src/messages' ); + +const slackClientMock = require( './mocks/slack' ); +send.setSlackClient( slackClientMock ); + +send.sendMessage = jest.fn(); +points.updateScore = jest.fn(); +messages.getRandomMessage = jest.fn(); + // Catch all console output during tests. console.error = jest.fn(); console.info = jest.fn(); console.log = jest.fn(); console.warn = jest.fn(); +// Clear module cache + mock counts due to us sometimes messing with mocks. +beforeEach( () => { + jest.resetModules(); + messages.getRandomMessage.mockClear(); + send.sendMessage.mockClear(); +}); + describe( 'handleSelfPlus', () => { - // TODO: + const user = 'U12345678', + channel = 'C12345678'; + + it( 'logs an attempt by a user to increment their own score', () => { + events.handleSelfPlus( user, channel ); + expect( console.log ).toHaveBeenCalledTimes( 1 ); + }); + + it( 'gets a message from the \'self plus\' collection', () => { + events.handleSelfPlus( user, channel ); + + expect( messages.getRandomMessage ) + .toHaveBeenCalledTimes( 1 ) + .toHaveBeenCalledWith( 'selfPlus', user ); + }); + + it( 'sends a message back to the user and channel that called it', () => { + const send = require( '../src/send' ), + events = require( '../src/events' ); + + send.sendMessage = jest.fn(); + send.setSlackClient( slackClientMock ); + + events.handleSelfPlus( user, channel ); + + expect( send.sendMessage ) + .toHaveBeenCalledTimes( 1 ) + .toHaveBeenCalledWith( expect.stringContaining( user ), channel ); + }); }); describe( 'handlePlusMinus', () => { - // TODO: + const item = 'SomeRandomThing', + channel = 'C12345678', + score = 5; + + /** @returns {integer} Returns a fake score. */ + const updateScoreMock = () => { + return score; + }; + + it( 'calls the score updater to update an item\'s score', () => { + const send = require( '../src/send' ), + points = require( '../src/points' ), + events = require( '../src/events' ); + + send.setSlackClient( slackClientMock ); + points.updateScore = jest.fn(); + + events.handlePlusMinus( item, '+', channel ); + + expect( points.updateScore ) + .toHaveBeenCalledTimes( 1 ) + .toHaveBeenCalledWith( item, '+' ); + }); + + it( 'gets a message from the \'plus\' collection', () => { + expect.hasAssertions(); + + const send = require( '../src/send' ), + points = require( '../src/points' ), + events = require( '../src/events' ), + messages = require( '../src/messages' ); + + send.setSlackClient( slackClientMock ); + points.updateScore = jest.fn( updateScoreMock ); + messages.getRandomMessage = jest.fn(); + + return events.handlePlusMinus( item, '+', channel ).then( () => { + expect( messages.getRandomMessage ) + .toHaveBeenCalledTimes( 1 ) + .toHaveBeenCalledWith( 'plus', item, score ); + }); + }); + + it( 'gets a message from the \'minus\' collection', () => { + expect.hasAssertions(); + + const send = require( '../src/send' ), + points = require( '../src/points' ), + events = require( '../src/events' ), + messages = require( '../src/messages' ); + + send.setSlackClient( slackClientMock ); + points.updateScore = jest.fn( updateScoreMock ); + messages.getRandomMessage = jest.fn(); + + return events.handlePlusMinus( item, '-', channel ).then( () => { + expect( messages.getRandomMessage ) + .toHaveBeenCalledTimes( 1 ) + .toHaveBeenCalledWith( 'minus', item, score ); + }); + }); + + it( 'sends a message back to the channel that called it', () => { + expect.hasAssertions(); + + const send = require( '../src/send' ), + points = require( '../src/points' ), + events = require( '../src/events' ); + + send.setSlackClient( slackClientMock ); + points.updateScore = jest.fn(); + send.sendMessage = jest.fn(); + + return events.handlePlusMinus( item, '+', channel ).then( () => { + expect( send.sendMessage ) + .toHaveBeenCalledTimes( 1 ) + .toHaveBeenCalledWith( expect.stringContaining( item ), channel ); + }); + }); }); From 8c37d25c1209a871cdfebb9ea2b4a3d2a1d7cc72 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 18:58:50 +1000 Subject: [PATCH 10/39] Hotfix: ensure message tests send through an item now, due to 5721a93 --- tests/messages.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/messages.js b/tests/messages.js index 2e233d5..87b4905 100644 --- a/tests/messages.js +++ b/tests/messages.js @@ -21,12 +21,12 @@ const operations = [ for ( const operation of operations ) { it( 'returns a message for the ' + operation + ' operation', () => { - expect( typeof messages.getRandomMessage( operation ) ).toBe( 'string' ); + expect( typeof messages.getRandomMessage( operation, 'RandomThing' ) ).toBe( 'string' ); }); } it( 'throws an error for an invalid operation', () => { expect( () => { - messages.getRandomMessage( 'INVALID_OPERATION' ); + messages.getRandomMessage( 'INVALID_OPERATION', 'RandomThing' ); }).toThrow(); }); From 84a59d0d4f968d51e5d9f483a56af5e7e762e8cd Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 21:33:50 +1000 Subject: [PATCH 11/39] Minor: shorten some tests by using it.each for near-duplicates --- src/send.js | 2 +- tests/.eslintrc.js | 3 +- tests/e2e-tests.js | 281 ++++++++++++++------------------------------- tests/events.js | 58 ++++------ tests/helpers.js | 2 +- 5 files changed, 113 insertions(+), 233 deletions(-) diff --git a/src/send.js b/src/send.js index 84d3745..480bafa 100644 --- a/src/send.js +++ b/src/send.js @@ -25,7 +25,7 @@ const setSlackClient = ( client ) => { /** * Sends a message to a Slack channel. * - * @param {string} message The message text to send. + * @param {string} text The message text to send. * @param {string} channel The ID of the channel to send the message to. * @return {Promise} A Promise to send the message to Slack. */ diff --git a/tests/.eslintrc.js b/tests/.eslintrc.js index e91e547..46a5120 100644 --- a/tests/.eslintrc.js +++ b/tests/.eslintrc.js @@ -12,6 +12,7 @@ module.exports = { 'no-loop-func': 'off', 'no-magic-numbers': 'off', 'no-process-env': 'off', - 'max-nested-callbacks': [ 'error', { max: 5 } ] + 'max-nested-callbacks': [ 'error', { max: 5 } ], + 'max-params': [ 'error', { max: 5 } ] } }; diff --git a/tests/e2e-tests.js b/tests/e2e-tests.js index f01b0cd..06846de 100644 --- a/tests/e2e-tests.js +++ b/tests/e2e-tests.js @@ -61,37 +61,41 @@ afterAll( () => { describe( 'The database', () => { - it( 'stores a ++ for a new \'thing\' (ThingA) and returns a score of 1', ( done ) => { - expect.hasAssertions(); - runner( '@ThingA++', { itemToCheck: 'ThingA' }, ( result ) => { - expect( result ).toBe( 1 ); - done(); - }); - }); + const thingTable = [ + [ '++', 'new ', 'ThingA', 1 ], + [ '++', 'existing', 'ThingA', 2 ], + [ '--', 'new ', 'ThingB', -1 ], + [ '--', 'existing', 'ThingB', -2 ] + ]; - it( 'stores a -- for a new \'thing\' (ThingB) and returns a score of -1', ( done ) => { - expect.hasAssertions(); - runner( '@ThingB--', { itemToCheck: 'ThingB' }, ( result ) => { - expect( result ).toBe( -1 ); - done(); - }); - }); + const userTable = [ + [ '++', 'new ', 'U00000100', 1 ], + [ '++', 'existing', 'U00000100', 2 ], + [ '--', 'new ', 'U00000200', -1 ], + [ '--', 'existing', 'U00000200', -2 ] + ]; - it( 'stores a ++ for an existing \'thing\' (ThingA) and returns a score of 2', ( done ) => { - expect.hasAssertions(); - runner( '@ThingA++', { itemToCheck: 'ThingA' }, ( result ) => { - expect( result ).toBe( 2 ); - done(); - }); - }); + it.each( thingTable )( + 'stores a %s for a %s thing (%s) and returns a score of %d', + ( operation, description, thing, score, done ) => { + expect.hasAssertions(); + runner( '@' + thing + operation, { itemToCheck: thing }, ( result ) => { + expect( result ).toBe( score ); + done(); + }); + } + ); - it( 'stores a -- for an existing \'thing\' (ThingB) and returns a score of -2', ( done ) => { - expect.hasAssertions(); - runner( '@ThingB--', { itemToCheck: 'ThingB' }, ( result ) => { - expect( result ).toBe( -2 ); - done(); - }); - }); + it.each( userTable )( + 'stores a %s for a %s user (%s) and returns a score of %d', + ( operation, description, user, score, done ) => { + expect.hasAssertions(); + runner( '<@' + user + '>' + operation, { itemToCheck: user }, ( result ) => { + expect( result ).toBe( score ); + done(); + }); + } + ); it( 'stores a ++ for ThInGa in a different case and returns a score of 3', ( done ) => { expect.hasAssertions(); @@ -101,46 +105,14 @@ describe( 'The database', () => { }); }); - it( 'stores a ++ for a new user (100) and returns a score of 1', ( done ) => { - expect.hasAssertions(); - runner( '<@U00000100>++', { itemToCheck: 'U00000100' }, ( result ) => { - expect( result ).toBe( 1 ); - done(); - }); - }); - - it( 'stores a -- for a new user (200) and returns a score of -1', ( done ) => { - expect.hasAssertions(); - runner( '<@U00000200>--', { itemToCheck: 'U00000200' }, ( result ) => { - expect( result ).toBe( -1 ); - done(); - }); - }); - - it( 'stores a ++ for an existing user (100) and returns a score of 2', ( done ) => { - expect.hasAssertions(); - runner( '<@U00000100>++', { itemToCheck: 'U00000100' }, ( result ) => { - expect( result ).toBe( 2 ); - done(); - }); - }); - - it( 'stores a -- for an existing user (200) and returns a score of -2', ( done ) => { - expect.hasAssertions(); - runner( '<@U00000200>--', { itemToCheck: 'U00000200' }, ( result ) => { - expect( result ).toBe( -2 ); - done(); - }); - }); - it( 'refuses a self ++ for an existing user (100) and still returns a score of 2', ( done ) => { expect.hasAssertions(); - const user = 'U00000100', - options = { - itemToCheck: user, - extraBody: { event: { user } } - }; + const user = 'U00000100'; + const options = { + itemToCheck: user, + extraBody: { event: { user } } + }; runner( '<@' + user + '>++', options, ( result ) => { expect( result ).toBe( 2 ); @@ -253,119 +225,60 @@ describe( 'Slack messaging', () => { }); }); - it( 'contains a user\'s link (user 100) and a score of 4 after another ++', ( done ) => { - expect.hasAssertions(); - const user = 'U00000100'; - - slackClientMock.chat.postMessage.mockClear(); - runner( '<@' + user + '>++', () => { - - expect( slackClientMock.chat.postMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( - expect.objectContaining({ text: expect.stringContaining( '<@' + user + '>' ) }) - ) - .toHaveBeenCalledWith( - expect.objectContaining({ text: expect.stringMatching( /\s4\b/ ) }) - ); - - done(); - - }); - }); - - it( 'contains a user\'s link (user 200) and a score of -4 after another --', ( done ) => { - expect.hasAssertions(); - const user = 'U00000200'; - - slackClientMock.chat.postMessage.mockClear(); - runner( '<@' + user + '>--', () => { - - expect( slackClientMock.chat.postMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( - expect.objectContaining({ text: expect.stringContaining( '<@' + user + '>' ) }) - ) - .toHaveBeenCalledWith( - expect.objectContaining({ text: expect.stringMatching( /\s-4\b/ ) }) - ); - - done(); - - }); - }); - - it( 'contains the singular \'point\' after a ++ for new ThingC (i.e. score 1)', ( done ) => { - expect.hasAssertions(); - const thing = 'ThingC'; - - slackClientMock.chat.postMessage.mockClear(); - runner( '@' + thing + '++', () => { - - expect( slackClientMock.chat.postMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( - expect.objectContaining({ text: expect.stringMatching( /\spoint\b/ ) }) - ); - - done(); - - }); - }); + const userTable = [ + [ 'U00000100', 4, '++' ], + [ 'U00000200', -4, '--' ] + ]; - it( 'contains the plural \'points\' after another ++ for ThingC (i.e. score 2)', ( done ) => { - expect.hasAssertions(); - const thing = 'ThingC'; + it.each( userTable )( + 'contains a user\'s link (%s) and a score of %d after another %s', + ( user, score, operation, done ) => { + expect.hasAssertions(); + const scoreRegExp = new RegExp( '\\s' + score + '\\b' ); - slackClientMock.chat.postMessage.mockClear(); - runner( '@' + thing + '++', () => { + slackClientMock.chat.postMessage.mockClear(); + runner( '<@' + user + '>' + operation, () => { - expect( slackClientMock.chat.postMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( - expect.objectContaining({ text: expect.stringMatching( /\spoints\b/ ) }) - ); + expect( slackClientMock.chat.postMessage ) + .toHaveBeenCalledTimes( 1 ) + .toHaveBeenCalledWith( + expect.objectContaining({ text: expect.stringContaining( '<@' + user + '>' ) }) + ) + .toHaveBeenCalledWith( + expect.objectContaining({ text: expect.stringMatching( scoreRegExp ) }) + ); - done(); + done(); + }); }); - }); - - it( 'contains the singular \'point\' after a -- for new ThingD (i.e. score -1)', ( done ) => { - expect.hasAssertions(); - const thing = 'ThingD'; - - slackClientMock.chat.postMessage.mockClear(); - runner( '@' + thing + '--', () => { - expect( slackClientMock.chat.postMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( - expect.objectContaining({ text: expect.stringMatching( /\spoint\b/ ) }) - ); - - done(); - - }); - }); + const thingTable = [ + [ 'point', '++', 'ThingC', 1 ], + [ 'points', '++', 'ThingC', 2 ], + [ 'point', '--', 'ThingD', 1 ], + [ 'points', '--', 'ThingD', 2 ] + ]; - it( 'contains the plural \'points\' after another -- for ThingD (i.e. score -2)', ( done ) => { - expect.hasAssertions(); - const thing = 'ThingD'; + it.each( thingTable )( + 'contains \'%s\' after a %s for %s (i.e. score %d)', + ( word, operation, thing, sampleScore, done ) => { + expect.hasAssertions(); + const wordRegExp = new RegExp( '\\s' + word + '\\b' ); - slackClientMock.chat.postMessage.mockClear(); - runner( '@' + thing + '--', () => { + slackClientMock.chat.postMessage.mockClear(); + runner( '@' + thing + operation, () => { - expect( slackClientMock.chat.postMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( - expect.objectContaining({ text: expect.stringMatching( /\spoints\b/ ) }) - ); + expect( slackClientMock.chat.postMessage ) + .toHaveBeenCalledTimes( 1 ) + .toHaveBeenCalledWith( + expect.objectContaining({ text: expect.stringMatching( wordRegExp ) }) + ); - done(); + done(); + }); }); - }); it( 'contains the plural \'points\' for a score of 0 (ThingE++ then --)', ( done ) => { expect.hasAssertions(); @@ -386,42 +299,26 @@ describe( 'Slack messaging', () => { }); }); - const operations = [ - { - name: 'plus', - operation: '++', - extraData: {} - }, - { - name: 'minus', - operation: '--', - extraData: {} - }, - { - name: 'selfPlus', - operation: '++', - extraData: { event: { user: 'U12345678' } } - } + const operationsTable = [ + [ 'plus', '++', {} ], + [ 'minus', '--', {} ], + [ 'selfPlus', '++', { event: { user: 'U12345678' } } ] ]; - for ( const operation of operations ) { - - const testName = ( - 'sends a message from the ' + operation.name + ' collection for a ' + operation.operation - ); - - it( testName, ( done ) => { + it.each( operationsTable )( + 'sends a message from the %s collection for a %s', + ( name, operation, extraData, done ) => { expect.hasAssertions(); slackClientMock.chat.postMessage.mockClear(); - const messageText = '<@U12345678>' + operation.operation, - options = { extraBody: operation.extraData }; + const messageText = '<@U12345678>' + operation, + options = { extraBody: extraData }; runner( messageText, options, async() => { const postMessageCall = slackClientMock.chat.postMessage.mock.calls[0], payload = postMessageCall[0], - collection = messages.messages[ operation.name ]; + collection = messages.messages[ name ]; let messageFoundInCollection = false; @@ -443,8 +340,6 @@ describe( 'Slack messaging', () => { }); }); - } // For operation. - it( 'sends messages back to the channel they were sent from', ( done ) => { expect.hasAssertions(); diff --git a/tests/events.js b/tests/events.js index daccdb9..3c989aa 100644 --- a/tests/events.js +++ b/tests/events.js @@ -96,43 +96,27 @@ describe( 'handlePlusMinus', () => { .toHaveBeenCalledWith( item, '+' ); }); - it( 'gets a message from the \'plus\' collection', () => { - expect.hasAssertions(); - - const send = require( '../src/send' ), - points = require( '../src/points' ), - events = require( '../src/events' ), - messages = require( '../src/messages' ); - - send.setSlackClient( slackClientMock ); - points.updateScore = jest.fn( updateScoreMock ); - messages.getRandomMessage = jest.fn(); - - return events.handlePlusMinus( item, '+', channel ).then( () => { - expect( messages.getRandomMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( 'plus', item, score ); - }); - }); - - it( 'gets a message from the \'minus\' collection', () => { - expect.hasAssertions(); - - const send = require( '../src/send' ), - points = require( '../src/points' ), - events = require( '../src/events' ), - messages = require( '../src/messages' ); - - send.setSlackClient( slackClientMock ); - points.updateScore = jest.fn( updateScoreMock ); - messages.getRandomMessage = jest.fn(); - - return events.handlePlusMinus( item, '-', channel ).then( () => { - expect( messages.getRandomMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( 'minus', item, score ); - }); - }); + it.each([ [ 'plus', '+' ], [ 'minus', '-' ] ])( + 'gets a message from the \'%s\' collection', + ( operationName, operation ) => { + expect.hasAssertions(); + + const send = require( '../src/send' ), + points = require( '../src/points' ), + events = require( '../src/events' ), + messages = require( '../src/messages' ); + + send.setSlackClient( slackClientMock ); + points.updateScore = jest.fn( updateScoreMock ); + messages.getRandomMessage = jest.fn(); + + return events.handlePlusMinus( item, operation, channel ).then( () => { + expect( messages.getRandomMessage ) + .toHaveBeenCalledTimes( 1 ) + .toHaveBeenCalledWith( operationName, item, score ); + }); + } + ); it( 'sends a message back to the channel that called it', () => { expect.hasAssertions(); diff --git a/tests/helpers.js b/tests/helpers.js index d478e34..e6ab008 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -127,7 +127,7 @@ describe( 'maybeLinkItem', () => { expect( helpers.maybeLinkItem( item ) ).toBe( item ); }); - it( 'returns an item linked with Slack mrkdown if it looks like a Slack user ID', () => { + it( 'returns an item linked with Slack mrkdwn if it looks like a Slack user ID', () => { const item = 'U12345678'; expect( helpers.maybeLinkItem( item ) ).toBe( '<@' + item + '>' ); }); From 26c1783b375f3889e16e26e79245bc03311894ed Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 21:39:26 +1000 Subject: [PATCH 12/39] Minor additional test docs --- tests/app.js | 1 + tests/e2e-tests.js | 1 + tests/events.js | 1 + tests/helpers.js | 1 + tests/integration-tests.js | 1 + tests/messages.js | 1 + tests/operations.js | 1 + tests/send.js | 1 + 8 files changed, 8 insertions(+) diff --git a/tests/app.js b/tests/app.js index f2f8dfd..8ba1f28 100644 --- a/tests/app.js +++ b/tests/app.js @@ -2,6 +2,7 @@ * Unit tests on the main app.js file. * * @see https://jestjs.io/docs/en/expect + * @see https://github.com/jest-community/jest-extended#api * @author Tim Malone */ diff --git a/tests/e2e-tests.js b/tests/e2e-tests.js index 06846de..b5b7eaf 100644 --- a/tests/e2e-tests.js +++ b/tests/e2e-tests.js @@ -2,6 +2,7 @@ * End-to-end tests. * * @see https://jestjs.io/docs/en/expect + * @see https://github.com/jest-community/jest-extended#api * @author Tim Malone */ diff --git a/tests/events.js b/tests/events.js index 3c989aa..5fc17a7 100644 --- a/tests/events.js +++ b/tests/events.js @@ -2,6 +2,7 @@ * Unit tests on the event handlers in events.js. * * @see https://jestjs.io/docs/en/expect + * @see https://github.com/jest-community/jest-extended#api * @author Tim Malone */ diff --git a/tests/helpers.js b/tests/helpers.js index e6ab008..ec3e724 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -2,6 +2,7 @@ * Unit tests on the helpers in helpers.js. * * @see https://jestjs.io/docs/en/expect + * @see https://github.com/jest-community/jest-extended#api * @author Tim Malone */ diff --git a/tests/integration-tests.js b/tests/integration-tests.js index 987e00d..2ce4dcf 100644 --- a/tests/integration-tests.js +++ b/tests/integration-tests.js @@ -2,6 +2,7 @@ * Integration tests. * * @see https://jestjs.io/docs/en/expect + * @see https://github.com/jest-community/jest-extended#api * @see https://jestjs.io/docs/en/asynchronous.html * @see https://jestjs.io/docs/en/mock-functions#mocking-modules * @author Tim Malone diff --git a/tests/messages.js b/tests/messages.js index 87b4905..8b8964a 100644 --- a/tests/messages.js +++ b/tests/messages.js @@ -4,6 +4,7 @@ * TODO: Expand tests. * * @see https://jestjs.io/docs/en/expect + * @see https://github.com/jest-community/jest-extended#api * @author Tim Malone */ diff --git a/tests/operations.js b/tests/operations.js index 0d6a037..234b6d6 100644 --- a/tests/operations.js +++ b/tests/operations.js @@ -2,6 +2,7 @@ * Unit tests on operations.js. * * @see https://jestjs.io/docs/en/expect + * @see https://github.com/jest-community/jest-extended#api * @author Tim Malone */ diff --git a/tests/send.js b/tests/send.js index d3b282e..a360b55 100644 --- a/tests/send.js +++ b/tests/send.js @@ -2,6 +2,7 @@ * Unit tests on the code in send.js. * * @see https://jestjs.io/docs/en/expect + * @see https://github.com/jest-community/jest-extended#api * @author Tim Malone */ From 005b69c6b297abf5c1014fd2dedc7db9e54b2900 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 22:00:30 +1000 Subject: [PATCH 13/39] Hotfix: prevent +- or -+ from being accepted and interpreted as ++ and -- respectively --- src/helpers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helpers.js b/src/helpers.js index a33c856..2d96e6b 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -15,7 +15,7 @@ * (i.e. + or -). */ const extractEventData = ( ( text ) => { - const data = text.match( /@([A-Za-z0-9]+?)>?\s*([-+]{2}|—{1})/ ); + const data = text.match( /@([A-Za-z0-9]+?)>?\s*(\+{2}|-{2}|—{1})/ ); if ( ! data ) { return false; From 861e63f178b09b811726357ff6494083036f1d3c Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 23:07:13 +1000 Subject: [PATCH 14/39] Minor: additional message event handler tests --- tests/events.js | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/tests/events.js b/tests/events.js index 5fc17a7..8b2a8ce 100644 --- a/tests/events.js +++ b/tests/events.js @@ -141,9 +141,29 @@ describe( 'handlePlusMinus', () => { describe( 'handlers.message', () => { - it( 'drops a user trying to ++ themselves', () => { + const eventType = 'message'; + + it( 'returns false if a valid item cannot be extracted', () => { const event = { - type: 'message', + type: eventType, + text: '@Invalid#Item++' + }; + + expect( handlers.message( event ) ).toBeFalse(); + }); + + it( 'returns false if a valid operation cannot be extracted', () => { + const event = { + type: eventType, + text: '<@U12345678>+-+' // Invalid operation. + }; + + expect( handlers.message( event ) ).toBeFalse(); + }); + + it( 'returns false if a user trying to ++ themselves', () => { + const event = { + type: eventType, text: '<@U12345678>++', user: 'U12345678' }; From 5d1265b86e0d738774e501830f764ed6d92bf8f8 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 23:07:21 +1000 Subject: [PATCH 15/39] Minor: README link fix --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3ee0ee2..95ef111 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [![Build Status](https://travis-ci.com/tdmalone/working-plusplus.svg?branch=master)](https://travis-ci.com/tdmalone/working-plusplus) [![Codacy Badge](https://api.codacy.com/project/badge/Grade/d0d9b6c1d1c4430e9fad61bb60b5dc4e)](https://www.codacy.com/project/tdmalone/working-plusplus/dashboard) -[![Codacy Badge](https://api.codacy.com/project/badge/Coverage/d0d9b6c1d1c4430e9fad61bb60b5dc4e)](https://www.codacy.com/project/tdmalone/working-plusplus/files) +[![Codacy Badge](https://api.codacy.com/project/badge/Coverage/d0d9b6c1d1c4430e9fad61bb60b5dc4e)](https://www.codacy.com/app/tdmalone/working-plusplus/files) Like [plusplus.chat](https://plusplus.chat/), except this one actually works - because you can host it yourself! 😉 From 6a14267d09e8bb90cccac32fc0d3eea5d91e3f8f Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 23:09:55 +1000 Subject: [PATCH 16/39] Minor: rename extractEventData to extractPlusMinusEventData --- src/events.js | 2 +- src/helpers.js | 4 ++-- tests/helpers.js | 22 +++++++++++----------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/events.js b/src/events.js index 4a5b0a2..1aff59e 100644 --- a/src/events.js +++ b/src/events.js @@ -65,7 +65,7 @@ const handlers = { message: ( event ) => { // Extract the relevant data from the message text. - const { item, operation } = helpers.extractEventData( event.text ); + const { item, operation } = helpers.extractPlusMinusEventData( event.text ); if ( ! item || ! operation ) { return false; diff --git a/src/helpers.js b/src/helpers.js index 2d96e6b..1201e71 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -14,7 +14,7 @@ * 'operation' being done on it - expressed as a valid mathematical operation * (i.e. + or -). */ -const extractEventData = ( ( text ) => { +const extractPlusMinusEventData = ( ( text ) => { const data = text.match( /@([A-Za-z0-9]+?)>?\s*(\+{2}|-{2}|—{1})/ ); if ( ! data ) { @@ -41,6 +41,6 @@ const maybeLinkItem = ( item ) => { }; module.exports = { - extractEventData, + extractPlusMinusEventData, maybeLinkItem }; diff --git a/tests/helpers.js b/tests/helpers.js index ec3e724..b180a48 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -12,43 +12,43 @@ const helpers = require( '../src/helpers' ); -describe( 'extractEventData', () => { +describe( 'extractPlusMinusEventData', () => { it( 'drops message without an @ symbol', () => { - expect( helpers.extractEventData( 'Hello++' ) ).toBeFalse(); + expect( helpers.extractPlusMinusEventData( 'Hello++' ) ).toBeFalse(); }); it( 'drops messages without a valid operation', () => { - expect( helpers.extractEventData( '@Hello' ) ).toBeFalse(); + expect( helpers.extractPlusMinusEventData( '@Hello' ) ).toBeFalse(); }); it( 'drops messages without a valid user/item', () => { - expect( helpers.extractEventData( '@++' ) ).toBeFalse(); + expect( helpers.extractPlusMinusEventData( '@++' ) ).toBeFalse(); }); it( 'extracts a \'thing\' and operation from the start of a message', () => { - expect( helpers.extractEventData( '@SomethingRandom++ that was awesome' ) ).toEqual({ + expect( helpers.extractPlusMinusEventData( '@SomethingRandom++ that was awesome' ) ).toEqual({ item: 'SomethingRandom', operation: '+' }); }); it( 'extracts a user and operation from the start of a message', () => { - expect( helpers.extractEventData( '<@U87654321>++ that was awesome' ) ).toEqual({ + expect( helpers.extractPlusMinusEventData( '<@U87654321>++ that was awesome' ) ).toEqual({ item: 'U87654321', operation: '+' }); }); it( 'extracts data in the middle of a message', () => { - expect( helpers.extractEventData( 'Hey @SomethingRandom++ that was awesome' ) ).toEqual({ + expect( helpers.extractPlusMinusEventData( 'Hey @SomethingRandom++ that was awesome' ) ).toEqual({ item: 'SomethingRandom', operation: '+' }); }); it( 'extracts data at the end of a message', () => { - expect( helpers.extractEventData( 'Awesome work @SomethingRandom++' ) ).toEqual({ + expect( helpers.extractPlusMinusEventData( 'Awesome work @SomethingRandom++' ) ).toEqual({ item: 'SomethingRandom', operation: '+' }); @@ -101,7 +101,7 @@ describe( 'extractEventData', () => { ); it( testName, () => { - const result = helpers.extractEventData( messageText ); + const result = helpers.extractPlusMinusEventData( messageText ); expect( result ).toEqual({ item: item.expected, operation: operation.expected @@ -114,12 +114,12 @@ describe( 'extractEventData', () => { for ( const operation of operationsNotToMatch ) { const messageText = item.supplied + operation; it( 'does NOT match ' + messageText, () => { - expect( helpers.extractEventData( messageText ) ).toBeFalse(); + expect( helpers.extractPlusMinusEventData( messageText ) ).toBeFalse(); }); } } // For itemsToMatch. -}); // ExtractEventData. +}); // ExtractPlusMinusEventData. describe( 'maybeLinkItem', () => { From a0c1004f98375fd6c3017724ee99e898386cde7e Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 12 Aug 2018 23:55:26 +1000 Subject: [PATCH 17/39] Prepare event handling for new leaderboard command --- src/events.js | 28 ++++++++++++----- src/helpers.js | 34 +++++++++++++++++++-- src/leaderboard.js | 15 ++++++++++ tests/events.js | 75 ++++++++++++++++++++++++++++++---------------- tests/helpers.js | 45 ++++++++++++++++++++++++++++ 5 files changed, 161 insertions(+), 36 deletions(-) create mode 100644 src/leaderboard.js diff --git a/src/events.js b/src/events.js index 1aff59e..d48684c 100644 --- a/src/events.js +++ b/src/events.js @@ -11,7 +11,8 @@ const send = require( './send' ), points = require( './points' ), helpers = require( './helpers' ), messages = require( './messages' ), - operations = require( './operations' ); + operations = require( './operations' ), + leaderboard = require( './leaderboard' ); const camelCase = require( 'lodash.camelcase' ); @@ -83,21 +84,32 @@ const handlers = { }, // Message event. /** - * Handles 'app_mention' events sent from Slack. + * Handles 'app_mention' events sent from Slack, primarily by looking for known app commands, and + * then handing the command off for processing. * * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at * https://api.slack.com/events-api#events_dispatched_as_json and * https://api.slack.com/events/app_mention for details. - * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise to send a - * Slack message back to the requesting channel. + * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise - usually + * to send a Slack message back to the requesting channel - which will be + * handled by the command's own handler. */ - appMention: async( event ) => { + appMention: ( event ) => { - // TODO: Handle this event. - console.log( event ); + const appCommandHandlers = { + leaderboard: leaderboard.handler + }; - } // AppMention event. + const validCommands = Object.keys( appCommandHandlers ), + appCommand = helpers.extractCommand( event.text, validCommands ); + + if ( appCommand ) { + return appCommandHandlers[appCommand]( event ); + } + + return false; + } // AppMention event. }; // Handlers. /** diff --git a/src/helpers.js b/src/helpers.js index 1201e71..822737e 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -1,9 +1,38 @@ /** * Contains assorted helper functions. + * + * @author Tim Malone */ 'use strict'; +/** + * Given a message and a list of commands, extracts the first command mentioned in the message. + * + * TODO: May need to ensure that commands are whole words, so a smaller command doesn't get + * detected inside a larger one. + * + * @param {string} message The message text to search. + * @param {array} commands The commands to look for. + * @return {string|Boolean} Either the first command found, or false if no commands were found. + */ +const extractCommand = ( message, commands ) => { + + let firstLocation = Number.MAX_SAFE_INTEGER, + firstCommand; + + for ( const command of commands ) { + const location = message.indexOf( command ); + if ( -1 !== location && location < firstLocation ) { + firstLocation = location; + firstCommand = command; + } + } + + return firstCommand ? firstCommand : false; + +}; + /** * Gets the user or 'thing' that is being spoken about, and the 'operation' being done on it. * We take the operation down to one character, and also support — due to iOS' replacement of --. @@ -14,7 +43,7 @@ * 'operation' being done on it - expressed as a valid mathematical operation * (i.e. + or -). */ -const extractPlusMinusEventData = ( ( text ) => { +const extractPlusMinusEventData = ( text ) => { const data = text.match( /@([A-Za-z0-9]+?)>?\s*(\+{2}|-{2}|—{1})/ ); if ( ! data ) { @@ -26,7 +55,7 @@ const extractPlusMinusEventData = ( ( text ) => { operation: data[2].substring( 0, 1 ).replace( '—', '-' ) }; -}); +}; /** * Takes an item and returns it maybe linked using Slack's 'mrkdwn' format (their own custom @@ -41,6 +70,7 @@ const maybeLinkItem = ( item ) => { }; module.exports = { + extractCommand, extractPlusMinusEventData, maybeLinkItem }; diff --git a/src/leaderboard.js b/src/leaderboard.js new file mode 100644 index 0000000..fa129db --- /dev/null +++ b/src/leaderboard.js @@ -0,0 +1,15 @@ +/** + * Contains logic for returning the leaderboard. + */ + +'use strict'; + +const handler = () => { + return new Promise( ( resolve, reject ) => { + + }); +}; + +module.exports = { + handler +}; diff --git a/tests/events.js b/tests/events.js index 8b2a8ce..316bf73 100644 --- a/tests/events.js +++ b/tests/events.js @@ -175,43 +175,66 @@ describe( 'handlers.message', () => { describe( 'handlers.appMention', () => { - // TODO: + const eventType = 'app_mention'; -}); + const appCommandTable = [ + [ 'leaderboard', 'leaderboard.js' ] + ]; + + it.each( appCommandTable )( 'calls the app command handler for %s', ( command, handlerFile ) => { + const event = { + type: eventType, + text: '<@U00000000> ' + command + }; + + const events = require( '../src/events' ), + commandHandler = require( '../src/' + handlerFile ); + + commandHandler.handler = jest.fn(); + events.handlers.appMention( event ); + expect( commandHandler.handler ).toHaveBeenCalledTimes( 1 ); + }); + + it( 'returns false if a supported command cannot be found', () => { + const event = { + type: eventType, + text: '<@U00000000> some_command_that_should_not_exist' + }; + + expect( handlers.appMention( event ) ).toBeFalse(); + }); + +}); // Handlers.appMention. describe( 'handleEvent', () => { const validEvents = [ - 'message', - 'app_mention' + [ 'message', '@Hello++' ], + [ 'app_mention', '<@U12345678> can haz leaderboard' ] ]; - for ( const eventName of validEvents ) { - - it( 'returns a Promise for a \'' + eventName + '\' event with text', () => { - const event = { - type: eventName, - text: '@Hello++' - }; - - expect( events.handleEvent( event ) instanceof Promise ).toBeTrue(); - }); + it.each( validEvents )( 'returns a Promise for a \'%s\' event with text', ( type, text ) => { + const event = { + type, + text + }; - it( 'reports a \'' + eventName + '\' event without text as invalid', () => { - const event = { type: eventName }; - expect( events.handleEvent( event ) ).toBeFalse(); - }); + expect( events.handleEvent( event ) instanceof Promise ).toBeTrue(); + }); - it( 'reports a \'' + eventName + '\' event with only a space for text as invalid', () => { - const event = { - type: eventName, - text: ' ' - }; + it.each( validEvents )( 'reports a \'%s\' event without text as invalid', ( type ) => { + const event = { type }; + expect( events.handleEvent( event ) ).toBeFalse(); + }); - expect( events.handleEvent( event ) ).toBeFalse(); - }); + it.each( validEvents )( 'reports a \'%s\' event with a space for text as invalid', ( type ) => { + const event = { + type, + text: ' ' + }; - } // For validEvents. + expect( events.handleEvent( event ) ).toBeFalse(); + }); it( 'reports an event with missing type as invalid', () => { const event = { text: 'Hello' }; diff --git a/tests/helpers.js b/tests/helpers.js index b180a48..db125f2 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -12,6 +12,51 @@ const helpers = require( '../src/helpers' ); +describe( 'extractCommand', () => { + + const commands = [ + 'test-command', + 'something-else', + 'another-command' + ]; + + it( 'returns a valid command from a message containing only that command', () => { + const message = '<@U12345678> test-command'; + expect( helpers.extractCommand( message, commands ) ).toEqual( 'test-command' ); + }); + + it( 'returns a valid command from the start of a message', () => { + const message = '<@U12345678> test-command would be great'; + expect( helpers.extractCommand( message, commands ) ).toEqual( 'test-command' ); + }); + + it( 'returns a valid command from the middle of a message', () => { + const message = '<@U12345678> can I have a test-command please'; + expect( helpers.extractCommand( message, commands ) ).toEqual( 'test-command' ); + }); + + it( 'returns a valid command from the end of a message', () => { + const message = '<@U12345678> I would love to see a test-command'; + expect( helpers.extractCommand( message, commands ) ).toEqual( 'test-command' ); + }); + + it( 'returns the first valid command in a message with multiple', () => { + const message = '<@U12345678> looking for something-else rather than a test-command'; + expect( helpers.extractCommand( message, commands ) ).toEqual( 'something-else' ); + }); + + it( 'returns the first valid command in a message with multiple (with order switched)', () => { + const message = '<@U12345678> looking for a test-command rather than something-else'; + expect( helpers.extractCommand( message, commands ) ).toEqual( 'test-command' ); + }); + + it( 'returns false if it cannot find a valid command in a message', () => { + const message = '<@U12345678> there is nothing actionable here'; + expect( helpers.extractCommand( message, commands ) ).toBeFalse(); + }); + +}); + describe( 'extractPlusMinusEventData', () => { it( 'drops message without an @ symbol', () => { From 72e2013f7cba3b52b69fcfe496dd7a738dc4eeed Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Mon, 13 Aug 2018 09:36:59 +1000 Subject: [PATCH 18/39] Add isUser & isPlural functions --- src/helpers.js | 24 +++++++++++++++++++++++- src/messages.js | 2 +- tests/helpers.js | 32 +++++++++++++++++++++++++++++++- 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/src/helpers.js b/src/helpers.js index 822737e..aad8557 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -57,6 +57,26 @@ const extractPlusMinusEventData = ( text ) => { }; +/** + * Determines whether or not a number should be referred to as a plural - eg. anything but 1 or -1. + * + * @param {integer} number The number in question. + * @returns {Boolean} Whether or not the number is a plural. + */ +const isPlural = ( number ) => { + return 1 !== Math.abs( number ); +}; + +/** + * Determines whether or not a string represents a Slack user ID - eg. U12345678. + * + * @param {string} item The string in question. + * @returns {Boolean} Whether or not the string is a Slack user ID. + */ +const isUser = ( item ) => { + return item.match( /U[A-Z0-9]{8}/ ) ? true : false; +}; + /** * Takes an item and returns it maybe linked using Slack's 'mrkdwn' format (their own custom * version of markdown). @@ -66,11 +86,13 @@ const extractPlusMinusEventData = ( text ) => { * @see https://api.slack.com/docs/message-formatting#linking_to_channels_and_users */ const maybeLinkItem = ( item ) => { - return item.match( /U[A-Z0-9]{8}/ ) ? '<@' + item + '>' : item; + return isUser( item ) ? '<@' + item + '>' : item; }; module.exports = { extractCommand, extractPlusMinusEventData, + isPlural, + isUser, maybeLinkItem }; diff --git a/src/messages.js b/src/messages.js index e36b677..8d27724 100644 --- a/src/messages.js +++ b/src/messages.js @@ -130,7 +130,7 @@ const getRandomMessage = ( operation, item, score = 0 ) => { ); } - const plural = 1 === Math.abs( score ) ? '' : 's', + const plural = helpers.isPlural( score ) ? 's' : '', max = chosenSet.length - 1, random = Math.floor( Math.random() * max ), message = chosenSet[ random ]; diff --git a/tests/helpers.js b/tests/helpers.js index db125f2..4d04298 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -86,7 +86,7 @@ describe( 'extractPlusMinusEventData', () => { }); it( 'extracts data in the middle of a message', () => { - expect( helpers.extractPlusMinusEventData( 'Hey @SomethingRandom++ that was awesome' ) ).toEqual({ + expect( helpers.extractPlusMinusEventData( 'Hey @SomethingRandom++ you\'re great' ) ).toEqual({ item: 'SomethingRandom', operation: '+' }); @@ -166,6 +166,36 @@ describe( 'extractPlusMinusEventData', () => { } // For itemsToMatch. }); // ExtractPlusMinusEventData. +describe( 'isPlural', () => { + + const table = [ + [ true, -11 ], + [ true, -2 ], + [ false, -1 ], + [ true, 0 ], + [ false, 1 ], + [ true, 2 ], + [ true, 11 ] + ]; + + it.each( table )( 'returns %p for %d', ( result, number ) => { + expect( helpers.isPlural( number ) ).toBe( result ); + }); + +}); + +describe( 'isUser', () => { + + it( 'returns true for a Slack user ID', () => { + expect( helpers.isUser( 'U00000000' ) ).toBeTrue(); + }); + + it( 'returns false for something other than a Slack user ID', () => { + expect( helpers.isUser( 'SomethingRandom' ) ).toBeFalse(); + }); + +}); + describe( 'maybeLinkItem', () => { it( 'returns an item as-is if it is not a Slack user ID', () => { From 283a9023751c974cab0b6a1326378a6ed8d52aea Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Mon, 13 Aug 2018 18:51:59 +1000 Subject: [PATCH 19/39] Add the ability to send rich messages to Slack --- src/send.js | 15 ++++++++++++--- tests/send.js | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/src/send.js b/src/send.js index 480bafa..1bcee95 100644 --- a/src/send.js +++ b/src/send.js @@ -25,17 +25,26 @@ const setSlackClient = ( client ) => { /** * Sends a message to a Slack channel. * - * @param {string} text The message text to send. - * @param {string} channel The ID of the channel to send the message to. + * @param {string|Object} text Either message text to send, or a Slack message payload. See the + * docs at https://api.slack.com/methods/chat.postMessage and + * https://api.slack.com/docs/message-formatting. + * @param {string} channel The ID of the channel to send the message to. Can alternatively + * be provided as part of the payload in the previous argument. * @return {Promise} A Promise to send the message to Slack. */ const sendMessage = ( text, channel ) => { - const payload = { + let payload = { channel, text }; + // If 'text' was provided as an object instead, merge it into the payload. + if ( 'object' === typeof text ) { + delete payload.text; + payload = Object.assign( payload, text ); + } + return new Promise( ( resolve, reject ) => { slack.chat.postMessage( payload ).then( ( data ) => { diff --git a/tests/send.js b/tests/send.js index a360b55..e47d443 100644 --- a/tests/send.js +++ b/tests/send.js @@ -39,7 +39,7 @@ describe( 'sendMessage', () => { channel: 'C12345678' }; - it( 'sends the provided message text to the provided channel via the Slack Web API', () => { + it( 'sends message text to a channel when provided as two arguments', () => { expect.assertions( 1 ); const slackClientMock = require( pathToMock ); send.setSlackClient( slackClientMock ); @@ -47,7 +47,54 @@ describe( 'sendMessage', () => { // Re-mock the client so we can listen to it. slackClientMock.chat.postMessage = jest.fn(); - send.sendMessage( payload.text, payload.channel ).catch( () => { + return send.sendMessage( payload.text, payload.channel ).catch( () => { + expect( slackClientMock.chat.postMessage ).toHaveBeenCalledWith( payload ); + }); + }); + + it( 'sends a message to a channel with a full payload as one argument', () => { + expect.assertions( 1 ); + const slackClientMock = require( pathToMock ); + send.setSlackClient( slackClientMock ); + + // Re-mock the client so we can listen to it. + slackClientMock.chat.postMessage = jest.fn(); + + const payload = { + text: 'Hello there', + channel: 'C12345678', + attachments: [ + { + 'text': 'Attachment text' + } + ] + }; + + return send.sendMessage( payload ).catch( () => { + expect( slackClientMock.chat.postMessage ).toHaveBeenCalledWith( payload ); + }); + }); + + it( 'sends a message to a channel when payload and channel are passed separately', () => { + expect.assertions( 1 ); + const slackClientMock = require( pathToMock ); + send.setSlackClient( slackClientMock ); + + // Re-mock the client so we can listen to it. + slackClientMock.chat.postMessage = jest.fn(); + + const channel = 'C12345678'; + const payload = { + text: 'Hello there', + attachments: [ + { + 'text': 'Attachment text' + } + ] + }; + + return send.sendMessage( payload, channel ).catch( () => { + payload.channel = channel; expect( slackClientMock.chat.postMessage ).toHaveBeenCalledWith( payload ); }); }); From a1a443274c9531da4643888258b1512b25a57fbd Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Mon, 13 Aug 2018 19:15:54 +1000 Subject: [PATCH 20/39] Add function to retrieve top scores from the DB --- src/points.js | 32 +++++++++++++++++++++++++ tests/integration-tests.js | 49 +++++++++++++++++++++++++++++++++----- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/src/points.js b/src/points.js index d3c214a..4d9b132 100644 --- a/src/points.js +++ b/src/points.js @@ -1,5 +1,12 @@ /** * All the stuff that handles the giving, taking away, or otherwise querying of points. + * + * NOTE: As the functions here pretty much deal exclusively with the database, they generally + * aren't unit tested, as that would require anyone who runs the tests to also have a Postgres + * server. Instead, the functions in this file are well covered via the integration and + * end-to-end tests. + * + * @author Tim Malone */ 'use strict'; @@ -19,6 +26,30 @@ const scoresTableName = 'scores', const postgres = new pg.Pool( postgresPoolConfig ); +/** + * Retrieves all scores from the database, ordered from highest to lowest. + * + * TODO: Add further smarts to retrieve only a limited number of scores, to avoid having to query + * everything. Note that this isn't just LIMIT, because we'll need to apply the limit + * separately to both users (/U[A-Z0-9]{8}/) and things (everything else) & return both sets. + * + * @return {array} An array of entries, each an object containing 'item' (string) and 'score' + * (integer) properties. + */ +const retrieveTopScores = async() => { + + const query = 'SELECT * FROM ' + scoresTableName + ' ORDER BY score DESC'; + + const dbClient = await postgres.connect(), + result = await dbClient.query( query ), + scores = result.rows; + + dbClient.release(); + + return scores; + +}; + /** * Updates the score of an item in the database. If the item doesn't yet exist, it will be inserted * into the database with an assumed initial score of 0. @@ -63,5 +94,6 @@ const updateScore = async( item, operation ) => { }; // UpdateScore. module.exports = { + retrieveTopScores, updateScore }; diff --git a/tests/integration-tests.js b/tests/integration-tests.js index 2ce4dcf..7ebc84b 100644 --- a/tests/integration-tests.js +++ b/tests/integration-tests.js @@ -16,7 +16,9 @@ * Environment Configuration. ****************************************************************/ -const app = require( '../src/app' ); +const app = require( '../src/app' ), + points = require( '../src/points' ); + const pathToListener = '../'; const pg = require( 'pg' ), @@ -186,6 +188,11 @@ describe( 'The Express server', () => { describe( 'The database', () => { + const defaultUser = 'U00000000', + defaultItem = 'something', + defaultUserPlus = '<@' + defaultUser + '>++', + defaultItemPlus = '@' + defaultItem + '++'; + const tableExistsQuery = 'SELECT EXISTS ( ' + 'SELECT 1 FROM information_schema.tables ' + 'WHERE table_name = \'' + config.scoresTableName + '\'' + @@ -212,15 +219,17 @@ describe( 'The database', () => { /** * Provides a 'first request' and a test that it successfully creates the database table. * - * @param {callable} done A callback to use for alerting Jest that the test is complete. + * @param {callable} done A callback to use for alerting Jest that the test is complete. + * @param {string} message Optionally the message to use for the request, if you need to differ + * from the default. * @return {void} */ - const doFirstRequest = ( done ) => { + const doRequest = ( done, message = defaultItemPlus ) => { expect.hasAssertions(); const listener = require( pathToListener )({ slack: slackClientMock }); listener.on( 'listening', () => { - runner( '@something++', async( dbClient ) => { + runner( message, async( dbClient ) => { listener.close(); const query = await dbClient.query( tableExistsQuery ); await dbClient.release(); @@ -230,7 +239,7 @@ describe( 'The database', () => { }); }; - it( 'creates the ' + config.scoresTableName + ' table on the first request', doFirstRequest ); + it( 'creates the ' + config.scoresTableName + ' table on the first request', doRequest ); it( 'also creates the case-insensitive extension on the first request', async() => { expect.hasAssertions(); @@ -240,6 +249,34 @@ describe( 'The database', () => { expect( query.rowCount ).toBe( 1 ); }); - it( 'does not cause errors on subsequent requests', doFirstRequest ); + it( 'does not cause errors on subsequent requests', doRequest ); + + it( 'returns a list of top scores in the correct order', ( done ) => { + expect.hasAssertions(); + + const expectedScores = [ + { + item: defaultUser, + score: 3 + }, + { + item: defaultItem, + score: 2 + } + ]; + + // Give us a few additional scores so we can check it works. + // TODO: This is messy. Should rewrite this to not use callbacks. + doRequest( () => { + doRequest( () => { + doRequest( async() => { + const topScores = await points.retrieveTopScores(); + expect( topScores ).toEqual( expectedScores ); + done(); + }, defaultUserPlus ); + }, defaultUserPlus ); + }, defaultUserPlus ); + + }); }); // Postgres tests. From dcc7724de3eb07c99fcdc149010268385bb720f2 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Mon, 13 Aug 2018 22:20:30 +1000 Subject: [PATCH 21/39] Simplify integration tests to not actually partially be end-to-end tests --- tests/_runner.js | 2 +- tests/integration-tests.js | 67 ++++++++++++++------------------------ 2 files changed, 26 insertions(+), 43 deletions(-) diff --git a/tests/_runner.js b/tests/_runner.js index 934c212..9821927 100644 --- a/tests/_runner.js +++ b/tests/_runner.js @@ -1,6 +1,6 @@ /** * Custom test runner that makes an HTTP request to the app and facilitates checking the outcome. - * Primarily used for end-to-end tests but also useful for some integration tests. + * Used for end-to-end testing. * * @author Tim Malone */ diff --git a/tests/integration-tests.js b/tests/integration-tests.js index 7ebc84b..21524cc 100644 --- a/tests/integration-tests.js +++ b/tests/integration-tests.js @@ -24,9 +24,7 @@ const pathToListener = '../'; const pg = require( 'pg' ), http = require( 'http' ); -const config = require( './_config' ), - runner = require( './_runner' ), - slackClientMock = require( './mocks/slack' ); +const config = require( './_config' ); const originalProcessEnv = process.env, postgres = new pg.Pool( config.postgresPoolConfig ); @@ -189,9 +187,7 @@ describe( 'The Express server', () => { describe( 'The database', () => { const defaultUser = 'U00000000', - defaultItem = 'something', - defaultUserPlus = '<@' + defaultUser + '>++', - defaultItemPlus = '@' + defaultItem + '++'; + defaultItem = 'something'; const tableExistsQuery = 'SELECT EXISTS ( ' + 'SELECT 1 FROM information_schema.tables ' + @@ -216,30 +212,14 @@ describe( 'The database', () => { expect( query.rowCount ).toBe( 0 ); }); - /** - * Provides a 'first request' and a test that it successfully creates the database table. - * - * @param {callable} done A callback to use for alerting Jest that the test is complete. - * @param {string} message Optionally the message to use for the request, if you need to differ - * from the default. - * @return {void} - */ - const doRequest = ( done, message = defaultItemPlus ) => { + it( 'creates the ' + config.scoresTableName + ' table on the first request', async() => { expect.hasAssertions(); - const listener = require( pathToListener )({ slack: slackClientMock }); - - listener.on( 'listening', () => { - runner( message, async( dbClient ) => { - listener.close(); - const query = await dbClient.query( tableExistsQuery ); - await dbClient.release(); - expect( query.rows[0].exists ).toBeTrue(); - done(); - }); - }); - }; - - it( 'creates the ' + config.scoresTableName + ' table on the first request', doRequest ); + await points.updateScore( defaultItem, '++' ); + const dbClient = await postgres.connect(); + const query = await dbClient.query( tableExistsQuery ); + await dbClient.release(); + expect( query.rows[0].exists ).toBeTrue(); + }); it( 'also creates the case-insensitive extension on the first request', async() => { expect.hasAssertions(); @@ -249,9 +229,16 @@ describe( 'The database', () => { expect( query.rowCount ).toBe( 1 ); }); - it( 'does not cause errors on subsequent requests', doRequest ); + /* eslint-disable jest/expect-expect */ + // TODO: This test really should have an assertion, but I can't figure out how to catch the error + // properly... it's possible that updateScore needs rewriting to catch properly. In the + // meantime, this test *does* actually work like expected. + it( 'does not cause any errors on a second request when everything already exists', async() => { + await points.updateScore( defaultItem, '++' ); + }); + /* eslint-enable jest/expect-expect */ - it( 'returns a list of top scores in the correct order', ( done ) => { + it( 'returns a list of top scores in the correct order', async() => { expect.hasAssertions(); const expectedScores = [ @@ -265,17 +252,13 @@ describe( 'The database', () => { } ]; - // Give us a few additional scores so we can check it works. - // TODO: This is messy. Should rewrite this to not use callbacks. - doRequest( () => { - doRequest( () => { - doRequest( async() => { - const topScores = await points.retrieveTopScores(); - expect( topScores ).toEqual( expectedScores ); - done(); - }, defaultUserPlus ); - }, defaultUserPlus ); - }, defaultUserPlus ); + // Give us a few additional scores so we can check the order works. + await points.updateScore( defaultUser, '++' ); + await points.updateScore( defaultUser, '++' ); + await points.updateScore( defaultUser, '++' ); + + const topScores = await points.retrieveTopScores(); + expect( topScores ).toEqual( expectedScores ); }); From 3730d3e0c69ea79b98d5c3d36ed06947acaa9464 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Mon, 13 Aug 2018 22:21:09 +1000 Subject: [PATCH 22/39] Minor docs --- src/points.js | 4 ++-- tests/.eslintrc.js | 2 ++ yarn.lock | 6 +++++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/points.js b/src/points.js index 4d9b132..abc201b 100644 --- a/src/points.js +++ b/src/points.js @@ -44,7 +44,7 @@ const retrieveTopScores = async() => { result = await dbClient.query( query ), scores = result.rows; - dbClient.release(); + await dbClient.release(); return scores; @@ -85,7 +85,7 @@ const updateScore = async( item, operation ) => { SELECT score FROM ' + scoresTableName + ' WHERE item = \'' + item + '\'; \ ' ); - dbClient.release(); + await dbClient.release(); const score = dbSelect.rows[0].score; console.log( item + ' now on ' + score ); diff --git a/tests/.eslintrc.js b/tests/.eslintrc.js index 46a5120..800d9b9 100644 --- a/tests/.eslintrc.js +++ b/tests/.eslintrc.js @@ -8,6 +8,8 @@ module.exports = { rules: { + 'jest/expect-expect': [ 'error' ], + 'jest/valid-expect-in-promise': [ 'error' ], 'no-empty-function': 'off', 'no-loop-func': 'off', 'no-magic-numbers': 'off', diff --git a/yarn.lock b/yarn.lock index 6d7722c..7c80ec7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1129,7 +1129,11 @@ eslint-config-wordpress@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/eslint-config-wordpress/-/eslint-config-wordpress-2.0.0.tgz#5201206c6964d648315232edf6dfbd2e925e4cd6" -eslint-plugin-jest@^21.18.0, eslint-plugin-jest@^21.2.0: +eslint-plugin-jest@^21.18.0: + version "21.21.0" + resolved "https://registry.yarnpkg.com/eslint-plugin-jest/-/eslint-plugin-jest-21.21.0.tgz#f0afd138c4acb5f0cd7698318fb49c7d49f3bf45" + +eslint-plugin-jest@^21.2.0: version "21.18.0" resolved "https://registry.yarnpkg.com/eslint-plugin-jest/-/eslint-plugin-jest-21.18.0.tgz#d7305969a9c1902f895468791d968fcf08b5c0b7" From ebdb00692a972162b17c00bd5e48cf28c62682f9 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Mon, 13 Aug 2018 22:24:58 +1000 Subject: [PATCH 23/39] WIP: Leaderboard functionality (working, no tests, no 'full leaderboard') --- index.js | 3 ++ src/app.js | 16 +++++++-- src/events.js | 8 ++--- src/leaderboard.js | 82 ++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 100 insertions(+), 9 deletions(-) diff --git a/index.js b/index.js index 58ed196..bd5e60c 100644 --- a/index.js +++ b/index.js @@ -42,6 +42,9 @@ const bootstrap = ( options = {}) => { server.get( '/', app.handleGet ); server.post( '/', app.handlePost ); + // Additional routes. + server.get( '/leaderboard', app.handleGet ); + return server.listen( PORT, () => { console.log( 'Listening on port ' + PORT + '.' ); }); diff --git a/src/app.js b/src/app.js index decbf36..ea2e73e 100644 --- a/src/app.js +++ b/src/app.js @@ -80,7 +80,19 @@ const validateToken = ( suppliedToken, serverToken ) => { */ const handleGet = ( request, response ) => { logRequest( request ); - response.send( 'It works! However, this app only accepts POST requests for now.' ); + + switch ( request.path.replace( /\/$/, '' ) ) { + + case '/leaderboard': + response.send( 'Full leaderboard coming soon.' ); + break; + + default: + response.send( 'It works! However, this app only accepts POST requests for now.' ); + break; + + } + }; /** @@ -122,7 +134,7 @@ const handlePost = ( request, response ) => { } // Handle the event now. If the event is invalid, this will return false. - return events.handleEvent( request.body.event ); + return events.handleEvent( request.body.event, request ); }; // HandlePost. diff --git a/src/events.js b/src/events.js index d48684c..4656f4d 100644 --- a/src/events.js +++ b/src/events.js @@ -94,7 +94,7 @@ const handlers = { * to send a Slack message back to the requesting channel - which will be * handled by the command's own handler. */ - appMention: ( event ) => { + appMention: ( event, request ) => { const appCommandHandlers = { leaderboard: leaderboard.handler @@ -104,7 +104,7 @@ const handlers = { appCommand = helpers.extractCommand( event.text, validCommands ); if ( appCommand ) { - return appCommandHandlers[appCommand]( event ); + return appCommandHandlers[appCommand]( event, request ); } return false; @@ -122,7 +122,7 @@ const handlers = { * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise as returned * by the event's handler function. */ -const handleEvent = ( event ) => { +const handleEvent = ( event, request ) => { // If the event has no type, something has gone wrong. if ( 'undefined' === typeof event.type ) { @@ -145,7 +145,7 @@ const handleEvent = ( event ) => { // Providing we have a handler for the event, let's handle it! const eventName = camelCase( event.type ); if ( handlers[ eventName ] instanceof Function ) { - return handlers[ eventName ] ( event ); + return handlers[ eventName ] ( event, request ); } console.warn( 'Invalid event received: ' + event.type ); diff --git a/src/leaderboard.js b/src/leaderboard.js index fa129db..3481b11 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -4,10 +4,86 @@ 'use strict'; -const handler = () => { - return new Promise( ( resolve, reject ) => { +const send = require( './send' ), + points = require( './points' ), + helpers = require( './helpers' ); - }); +const crypto = require( 'crypto' ); + +/** + * Retrieves and sends the current leaderboard to the requesting Slack channel. + * + * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at + * https://api.slack.com/events-api#events_dispatched_as_json and + * https://api.slack.com/events/app_mention for details. + * @returns {Promise} A Promise to send the Slack message. + */ +const handler = async( event, request ) => { + + const limit = 5; + + const topScores = await points.retrieveTopScores(); + const users = [], + things = []; + + let lastUserScore, lastUserRank, + lastThingScore, lastThingRank; + + for ( const topScore of topScores ) { + const item = helpers.maybeLinkItem( topScore.item ), + itemTitleCase = item.substring( 0, 1 ).toUpperCase() + item.substring( 1 ), + plural = helpers.isPlural( topScore.score ) ? 's' : '', + message = itemTitleCase + ' [' + topScore.score + ' point' + plural + ']'; + + if ( helpers.isUser( topScore.item ) ) { + const rank = topScore.score === lastUserScore ? lastUserRank : users.length + 1; + users.push( rank + '. ' + message + ( users.length ? '' : ' :muscle:' ) ); + lastUserRank = rank; + lastUserScore = topScore.score; + } else { + const rank = topScore.score === lastThingScore ? lastThingRank : things.length + 1; + things.push( rank + '. @' + message + ( things.length ? '' : ' :tada:' ) ); + lastThingRank = rank; + lastThingScore = topScore.score; + } + } + + const ts = Math.floor( Date.now() / 1000 ), + secret1 = process.env.SLACK_VERIFICATION_TOKEN, + secret2 = process.env.DATABASE_URL, + token = crypto.createHmac( 'sha256', secret1 ).update( ts + secret2 ).digest( 'hex' ), + url = 'https://' + request.headers.host + '/leaderboard?token=' + token + '&ts=' + ts; + + const messageText = ( + 'Here you go. ' + + // TODO: Enable the below once ready. + //+ 'Or see the <' + url + '|whole list>.' + ); + + const message = { + attachments: [ + { + text: messageText, + color: 'good', + fields: [ + { + title: 'Users', + value: users.slice( 0, limit ).join( '\n' ), + short: true + }, + { + title: 'Things', + value: things.slice( 0, limit ).join( '\n' ), + short: true + } + ] + } + ] + }; + + console.log( 'Sending the leaderboard.' ); + return send.sendMessage( message, event.channel ); }; module.exports = { From 6d79e8c7a85f2c7cf8670dc117348b53a3867225 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Thu, 16 Aug 2018 23:05:59 +1000 Subject: [PATCH 24/39] Leaderboard: extract URL + token building functionality + add tests --- src/events.js | 14 ++++---- src/helpers.js | 71 +++++++++++++++++++++++++++++++++++++++ src/leaderboard.js | 40 +++++++++++++--------- tests/helpers.js | 79 ++++++++++++++++++++++++++++++++++++++++++++ tests/leaderboard.js | 64 +++++++++++++++++++++++++++++++++++ 5 files changed, 246 insertions(+), 22 deletions(-) create mode 100644 tests/leaderboard.js diff --git a/src/events.js b/src/events.js index 4656f4d..2444c37 100644 --- a/src/events.js +++ b/src/events.js @@ -87,9 +87,10 @@ const handlers = { * Handles 'app_mention' events sent from Slack, primarily by looking for known app commands, and * then handing the command off for processing. * - * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at - * https://api.slack.com/events-api#events_dispatched_as_json and - * https://api.slack.com/events/app_mention for details. + * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at + * https://api.slack.com/events-api#events_dispatched_as_json and + * https://api.slack.com/events/app_mention for details. + * @param {object} request The incoming Express request object for this event. * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise - usually * to send a Slack message back to the requesting channel - which will be * handled by the command's own handler. @@ -116,9 +117,10 @@ const handlers = { * Determines whether or not incoming events from Slack can be handled by this app, and if so, * passes the event off to its handler function. * - * @param {object} event A hash of a Slack event. See the documentation at - * https://api.slack.com/events-api#events_dispatched_as_json and - * https://api.slack.com/events/message for details. + * @param {object} event A hash of a Slack event. See the documentation at + * https://api.slack.com/events-api#events_dispatched_as_json and + * https://api.slack.com/events/message for details. + * @param {object} request The incoming Express request object for this event. * @return {bool|Promise} Either `false` if the event cannot be handled, or a Promise as returned * by the event's handler function. */ diff --git a/src/helpers.js b/src/helpers.js index aad8557..efd41ee 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -6,6 +6,17 @@ 'use strict'; +const crypto = require( 'crypto' ); + +/* eslint-disable no-process-env */ +const envSecret1 = process.env.SLACK_VERIFICATION_TOKEN, + envSecret2 = process.env.DATABASE_URL; +/* eslint-enable no-process-env */ + +const ONE_DAY = 60 * 60 * 24, // eslint-disable-line no-magic-numbers + TOKEN_TTL = ONE_DAY, + MILLISECONDS_TO_SECONDS = 1000; + /** * Given a message and a list of commands, extracts the first command mentioned in the message. * @@ -57,6 +68,33 @@ const extractPlusMinusEventData = ( text ) => { }; +/** + * Generates a time-based token based on secrets from the environment. + * + * @param {string} ts A timestamp to hash into the token. + * @returns {string} A token, that can be re-checked later using the same timestamp. + */ +const getTimeBasedToken = ( ts ) => { + + if ( ! ts ) { + throw Error( 'Timestamp not provided when getting time-based token.' ); + } + + return crypto + .createHmac( 'sha256', envSecret1 ) + .update( ts + envSecret2 ) + .digest( 'hex' ); +}; + +/** + * Returns the current time as a standard Unix epoch timestamp. + * + * @returns {integer} The current Unix timestamp. + */ +const getTimestamp = () => { + return Math.floor( Date.now() / MILLISECONDS_TO_SECONDS ); +}; + /** * Determines whether or not a number should be referred to as a plural - eg. anything but 1 or -1. * @@ -67,6 +105,36 @@ const isPlural = ( number ) => { return 1 !== Math.abs( number ); }; +/** + * Validates a time-based token to ensure it is both still valid, and that it can be successfully + * re-hashed using the expected secrets. + * + * @param {string} token The token to validate. + * @param {integer} ts The timestamp the token was supplied with. + * @returns {boolean} Whether or not the token is valid. + */ +const isTimeBasedTokenStillValid = ( token, ts ) => { + const now = getTimestamp(); + + // Don't support tokens too far from the past. + if ( now > ts + TOKEN_TTL ) { + return false; + } + + // Don't support tokens from the future. + if ( now < ts ) { + return false; + } + + const hash = getTimeBasedToken( ts ); + + if ( hash !== token ) { + return false; + } + + return true; +}; + /** * Determines whether or not a string represents a Slack user ID - eg. U12345678. * @@ -92,7 +160,10 @@ const maybeLinkItem = ( item ) => { module.exports = { extractCommand, extractPlusMinusEventData, + getTimeBasedToken, + getTimestamp, isPlural, + isTimeBasedTokenStillValid, isUser, maybeLinkItem }; diff --git a/src/leaderboard.js b/src/leaderboard.js index 3481b11..4b90159 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -8,27 +8,42 @@ const send = require( './send' ), points = require( './points' ), helpers = require( './helpers' ); -const crypto = require( 'crypto' ); +/** + * Gets the URL for the full leaderboard, including a token to ensure that it is only viewed by + * someone who has access to this Slack team. + * + * @param {string} hostname The hostname this app is being served on. + * @returns {string} The leaderboard URL, which will be picked up in ../index.js when called. + */ +const getLeaderboardUrl = ( hostname ) => { + const ts = helpers.getTimestamp(), + token = helpers.getTimeBasedToken( ts ), + url = 'https://' + hostname + '/leaderboard?token=' + token + '&ts=' + ts; + + return url; +}; /** * Retrieves and sends the current leaderboard to the requesting Slack channel. * - * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at - * https://api.slack.com/events-api#events_dispatched_as_json and - * https://api.slack.com/events/app_mention for details. + * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at + * https://api.slack.com/events-api#events_dispatched_as_json and + * https://api.slack.com/events/app_mention for details. + * @param {object} request The Express request object that resulted in this handler being run. * @returns {Promise} A Promise to send the Slack message. */ const handler = async( event, request ) => { const limit = 5; - const topScores = await points.retrieveTopScores(); - const users = [], + const topScores = await points.retrieveTopScores(), + users = [], things = []; let lastUserScore, lastUserRank, lastThingScore, lastThingRank; + // Process the top scores, including applying their ranks. for ( const topScore of topScores ) { const item = helpers.maybeLinkItem( topScore.item ), itemTitleCase = item.substring( 0, 1 ).toUpperCase() + item.substring( 1 ), @@ -48,17 +63,9 @@ const handler = async( event, request ) => { } } - const ts = Math.floor( Date.now() / 1000 ), - secret1 = process.env.SLACK_VERIFICATION_TOKEN, - secret2 = process.env.DATABASE_URL, - token = crypto.createHmac( 'sha256', secret1 ).update( ts + secret2 ).digest( 'hex' ), - url = 'https://' + request.headers.host + '/leaderboard?token=' + token + '&ts=' + ts; - const messageText = ( - 'Here you go. ' - - // TODO: Enable the below once ready. - //+ 'Or see the <' + url + '|whole list>.' + 'Here you go. ' + + 'Or see the <' + getLeaderboardUrl( request.headers.host ) + '|whole list>.' ); const message = { @@ -87,5 +94,6 @@ const handler = async( event, request ) => { }; module.exports = { + getLeaderboardUrl, handler }; diff --git a/tests/helpers.js b/tests/helpers.js index 4d04298..e649ac5 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -12,6 +12,8 @@ const helpers = require( '../src/helpers' ); +const MILLISECONDS_TO_SECONDS = 1000; + describe( 'extractCommand', () => { const commands = [ @@ -166,6 +168,41 @@ describe( 'extractPlusMinusEventData', () => { } // For itemsToMatch. }); // ExtractPlusMinusEventData. +describe( 'getTimeBasedToken', () => { + + it( 'returns a string', () => { + expect( helpers.getTimeBasedToken( helpers.getTimestamp() ) ).toBeString(); + }); + + it( 'throws if a timestamp is not provided', () => { + expect( () => { + helpers.getTimeBasedToken(); + }).toThrow(); + }); + + it( 'provides a different token if called with a different timestamp', () => { + const token1 = helpers.getTimeBasedToken( 123456789 ); + const token2 = helpers.getTimeBasedToken( 123123123 ); + expect( token1 ).not.toEqual( token2 ); + }); + +}); + +describe( 'getTimestamp', () => { + + it( 'returns an integer', () => { + expect( helpers.getTimestamp() ) + .toBeNumber() + .not.toBeString(); + }); + + it( 'returns the current unix epoch', () => { + const now = Math.floor( Date.now() / MILLISECONDS_TO_SECONDS ); + expect( helpers.getTimestamp() ).toBeWithin( now - 5, now + 1 ); + }); + +}); + describe( 'isPlural', () => { const table = [ @@ -184,6 +221,48 @@ describe( 'isPlural', () => { }); +describe( 'isTimeBasedTokenStillValid', () => { + + it( 'returns true for a token created just now', () => { + const now = helpers.getTimestamp(), + token = helpers.getTimeBasedToken( now ); + + expect( helpers.isTimeBasedTokenStillValid( token, now ) ).toBeTrue(); + }); + + it( 'returns true for a token created an hour ago', () => { + const now = helpers.getTimestamp(), + oneHourAgo = now - 60 * 60, + token = helpers.getTimeBasedToken( oneHourAgo ); + + expect( helpers.isTimeBasedTokenStillValid( token, oneHourAgo ) ).toBeTrue(); + }); + + it( 'returns false for a token created with a different timestamp', () => { + const now = helpers.getTimestamp(), + token = helpers.getTimeBasedToken( now - 1 ); + + expect( helpers.isTimeBasedTokenStillValid( token, now ) ).toBeFalse(); + }); + + it( 'returns false for a token created in the future', () => { + const now = helpers.getTimestamp(), + theFuture = now + 10, + token = helpers.getTimeBasedToken( theFuture ); + + expect( helpers.isTimeBasedTokenStillValid( token, theFuture ) ).toBeFalse(); + }); + + it( 'returns false for a token created two days ago', () => { + const now = helpers.getTimestamp(), + twoDaysAgo = now - 60 * 60 * 24 * 2, + token = helpers.getTimeBasedToken( twoDaysAgo ); + + expect( helpers.isTimeBasedTokenStillValid( token, twoDaysAgo ) ).toBeFalse(); + }); + +}); + describe( 'isUser', () => { it( 'returns true for a Slack user ID', () => { diff --git a/tests/leaderboard.js b/tests/leaderboard.js new file mode 100644 index 0000000..05d9fd8 --- /dev/null +++ b/tests/leaderboard.js @@ -0,0 +1,64 @@ +/** + * Unit tests on the code in leaderboard.js. + * + * @see https://jestjs.io/docs/en/expect + * @see https://github.com/jest-community/jest-extended#api + * @author Tim Malone + */ + +/* global jest */ + +'use strict'; + +const leaderboard = require( '../src/leaderboard' ), + helpers = require( '../src/helpers' ); + +describe( 'getLeaderboardUrl', () => { + + const MILLISECONDS_TO_SECONDS = 1000; + + const leaderboardUrl = leaderboard.getLeaderboardUrl( 'example.com' ), + parsedUrl = new URL( leaderboardUrl ), + token = parsedUrl.searchParams.get( 'token' ), + ts = parsedUrl.searchParams.get( 'ts' ), + now = Math.floor( Date.now() / MILLISECONDS_TO_SECONDS ); + + it( 'returns a well-formed URL that doesn\'t need fixing', () => { + expect( leaderboardUrl ).toBe( parsedUrl.href ); + }); + + it( 'returns a URL with a hostname', () => { + expect( parsedUrl.hostname ).toBeString(); + }); + + it( 'returns a URL with a path', () => { + expect( parsedUrl.pathname ).toBeString(); + }); + + it( 'returns an HTTPS URL', () => { + expect( parsedUrl.protocol ).toBe( 'https:' ); + }); + + it( 'includes a token parameter for authorization', () => { + expect( token ).toBeString(); + }); + + it( 'includes a ts (timestamp) parameter for authorization', () => { + expect( ts ).toBeString(); + }); + + it( 'has a ts (timestamp) parameter that was just created', () => { + expect( ts ).toBeWithin( now - 5, now + 1 ); + }); + + it( 'has a token that can be validated using the timestamp', () => { + expect( helpers.isTimeBasedTokenStillValid( token, now ) ).toBeTrue(); + }); + + it( 'has a token that fails to validate with a different timestamp', () => { + expect( helpers.isTimeBasedTokenStillValid( token, now - 1 ) ).toBeFalse(); + }); + +}); // GetLeaderboardUrl. + +// TODO: Add tests for handler(). From 7f44bc0f5df8f0941a1c56d4362d286f47b3d610 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sat, 18 Aug 2018 23:31:05 +1000 Subject: [PATCH 25/39] Leaderboard: split ranking into separate function --- src/leaderboard.js | 120 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 92 insertions(+), 28 deletions(-) diff --git a/src/leaderboard.js b/src/leaderboard.js index 4b90159..7736400 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -8,6 +8,91 @@ const send = require( './send' ), points = require( './points' ), helpers = require( './helpers' ); +const fs = require( 'fs' ); + +let leaderboardHtml; + +/** + * Ranks items by their scores, returning them in a human readable list complete with emoji for the + * winner. Items which draw will be given the same rank, and the next rank will then be skipped. + * + * For example, 2 users on 54 would draw 1st. The next user on 52 would be 3rd, and the final on 34 + * would be 4th. + * + * @param {array} topScores An array of score objects, usually pre-retrieved by + * points.retrieveTopScores(). These *must* be in 'top score' order (i.e. + * descending order), otherwise ranking will not function correctly. Score + * objects contain 'item' and 'score' properties. + * @param {string} itemType The type of item to rank. Accepts 'users' or 'things'. Only one type + * can be ranked at a time. + * @param {string} format The format to return the results in. 'slack' returns or 'object'. + * + * @returns {array|object} Depending on the value of 'format', an array, in rank order, of either + * human-readable Slack strings or objects containing 'rank', 'item' and + * 'score' values. + */ +const rankItems = ( topScores, itemType = 'users', format = 'slack' ) => { + + let lastScore, lastRank, output; + const items = []; + + for ( const score of topScores ) { + + const isUser = helpers.isUser( score.item ) ? true : false; + + // Skip if this item is not the item type we're ranking. + if ( isUser && 'users' !== itemType || ! isUser && 'users' === itemType ) { + continue; + } + + // TODO: If 'slack' !== format, we're gonna need to get the user's name from the Slack API. + + const item = 'slack' === format ? helpers.maybeLinkItem( score.item ) : score.item, + itemTitleCase = item.substring( 0, 1 ).toUpperCase() + item.substring( 1 ), + plural = helpers.isPlural( score.score ) ? 's' : ''; + + // For the Slack format, *users* are already linked with an @. In all other cases we need to + // insert an @ for items too. + const prefix = isUser && 'slack' === format ? '' : '@'; + + // Determine the rank by keeping it the same as the last user if the score is the same, or + // otherwise setting it to the same as the item count (and adding 1 to deal with 0-base count). + const rank = score.score === lastScore ? lastRank : items.length + 1; + + switch ( format ) { + case 'slack': + + output = ( + rank + '. ' + prefix + itemTitleCase + ' [' + score.score + ' point' + plural + ']' + ); + + // If this is the first item, it's the winner! + if ( ! items.length ) { + output += ' ' + ( isUser ? ':muscle:' : ':tada:' ); + } + + break; + + case 'html': + output = { + rank, + item: prefix + itemTitleCase, + score: score.score + ' point' + plural + }; + break; + } + + items.push( output ); + + lastRank = rank; + lastScore = score.score; + + } // For scores. + + return items; + +}; // RankItems. + /** * Gets the URL for the full leaderboard, including a token to ensure that it is only viewed by * someone who has access to this Slack team. @@ -36,32 +121,9 @@ const handler = async( event, request ) => { const limit = 5; - const topScores = await points.retrieveTopScores(), - users = [], - things = []; - - let lastUserScore, lastUserRank, - lastThingScore, lastThingRank; - - // Process the top scores, including applying their ranks. - for ( const topScore of topScores ) { - const item = helpers.maybeLinkItem( topScore.item ), - itemTitleCase = item.substring( 0, 1 ).toUpperCase() + item.substring( 1 ), - plural = helpers.isPlural( topScore.score ) ? 's' : '', - message = itemTitleCase + ' [' + topScore.score + ' point' + plural + ']'; - - if ( helpers.isUser( topScore.item ) ) { - const rank = topScore.score === lastUserScore ? lastUserRank : users.length + 1; - users.push( rank + '. ' + message + ( users.length ? '' : ' :muscle:' ) ); - lastUserRank = rank; - lastUserScore = topScore.score; - } else { - const rank = topScore.score === lastThingScore ? lastThingRank : things.length + 1; - things.push( rank + '. @' + message + ( things.length ? '' : ' :tada:' ) ); - lastThingRank = rank; - lastThingScore = topScore.score; - } - } + const scores = await points.retrieveTopScores(), + users = rankItems( scores, 'users' ), + things = rankItems( scores, 'things' ); const messageText = ( 'Here you go. ' + @@ -72,7 +134,7 @@ const handler = async( event, request ) => { attachments: [ { text: messageText, - color: 'good', + color: 'good', // Slack's 'green' colour. fields: [ { title: 'Users', @@ -91,9 +153,11 @@ const handler = async( event, request ) => { console.log( 'Sending the leaderboard.' ); return send.sendMessage( message, event.channel ); -}; + +}; // Handler. module.exports = { getLeaderboardUrl, + rankItems, handler }; From f3bde31e6605d5f95ad2dcc6d1bf1d6d524a64d9 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sat, 18 Aug 2018 23:33:00 +1000 Subject: [PATCH 26/39] WIP: Templates and structure for full web-viewable leaderboard --- index.js | 15 ++++++- package.json | 1 + src/app.js | 23 ++++++++--- src/assets/main.css | 85 +++++++++++++++++++++++++++++++++++++++ src/helpers.js | 39 +++++++++++++++++- src/html/footer.html | 3 ++ src/html/header.html | 17 ++++++++ src/html/leaderboard.html | 25 ++++++++++++ src/leaderboard.js | 28 +++++++++++++ yarn.lock | 4 ++ 10 files changed, 231 insertions(+), 9 deletions(-) create mode 100644 src/assets/main.css create mode 100644 src/html/footer.html create mode 100644 src/html/header.html create mode 100644 src/html/leaderboard.html diff --git a/index.js b/index.js index bd5e60c..88d3578 100644 --- a/index.js +++ b/index.js @@ -12,7 +12,9 @@ const app = require( './src/app' ), send = require( './src/send' ); -const slack = require( '@slack/client' ), +const fs = require( 'fs' ), + mime = require( 'mime' ), + slack = require( '@slack/client' ), express = require( 'express' ), bodyParser = require( 'body-parser' ); @@ -42,6 +44,15 @@ const bootstrap = ( options = {}) => { server.get( '/', app.handleGet ); server.post( '/', app.handlePost ); + // Static assets. + server.get( '/assets/*', ( request, response ) => { + const path = 'src/' + request._parsedUrl.path, + type = mime.getType( path ); + + response.setHeader( 'Content-Type', type ); + response.send( fs.readFileSync( path ) ); + }); + // Additional routes. server.get( '/leaderboard', app.handleGet ); @@ -49,7 +60,7 @@ const bootstrap = ( options = {}) => { console.log( 'Listening on port ' + PORT + '.' ); }); -}; +}; // Bootstrap. // If module was called directly, bootstrap now. if ( require.main === module ) { diff --git a/package.json b/package.json index d4cf0a4..d6f3a76 100644 --- a/package.json +++ b/package.json @@ -30,6 +30,7 @@ "body-parser": "^1.18.3", "express": "^4.16.3", "lodash.camelcase": "^4.3.0", + "mime": "^2.3.1", "pg": "^7.4.3" }, "devDependencies": { diff --git a/src/app.js b/src/app.js index ea2e73e..94ec593 100644 --- a/src/app.js +++ b/src/app.js @@ -12,7 +12,9 @@ 'use strict'; -const events = require( './events' ); +const events = require( './events' ), + helpers = require( './helpers' ), + leaderboard = require( './leaderboard' ); // eslint-disable-next-line no-process-env const SLACK_VERIFICATION_TOKEN = process.env.SLACK_VERIFICATION_TOKEN; @@ -72,28 +74,39 @@ const validateToken = ( suppliedToken, serverToken ) => { }; // ValidateToken. /** - * Handles GET requests to the app. + * Handles GET requests to the app. At the moment this only really consists of an authenticated + * view of the full leaderboard. * * @param {express.req} request An Express request. See https://expressjs.com/en/4x/api.html#req. * @param {express.res} response An Express response. See https://expressjs.com/en/4x/api.html#res. * @return {void} */ -const handleGet = ( request, response ) => { +const handleGet = async( request, response ) => { logRequest( request ); switch ( request.path.replace( /\/$/, '' ) ) { + // Full leaderboard. This will only work when a valid, non-expired token and timestamp are + // provided - the full link can be retrieved by requesting the leaderboard within Slack. + // TODO: This should probably be split out into a separate function of sorts, like handlePost. case '/leaderboard': - response.send( 'Full leaderboard coming soon.' ); + if ( helpers.isTimeBasedTokenStillValid( request.query.token, request.query.ts ) ) { + response.send( await leaderboard.getFull() ); + } else { + response + .status( HTTP_403 ) + .send( 'Sorry, this link is no longer valid. Please request a new link in Slack.' ); + } break; + // A simple default GET response is sometimes useful for troubleshooting. default: response.send( 'It works! However, this app only accepts POST requests for now.' ); break; } -}; +}; // HandleGet. /** * Handles POST requests to the app. diff --git a/src/assets/main.css b/src/assets/main.css new file mode 100644 index 0000000..1118c9f --- /dev/null +++ b/src/assets/main.css @@ -0,0 +1,85 @@ +/** + * + * + */ + +* { + transition: all .5s; +} + +body { + font-family: Arial, Helvetica, sans-serif; + font-size: 16px; + background-color: #7DB83C; +} + +.wrapper { + width: 90%; + max-width: 600px; + margin: 3em auto; +} + +.site-title, +h1, +h2 { + text-align: center; +} + +h1 { + margin: .5em 0 1.5em; +} + +.leaderboard { + margin-bottom: 3em; + padding: 5% 10% 10%; + border-radius: 100% 50%; + background-color: #fff; +} + +@media only screen and (min-width: 600px) { + + .leaderboard { + margin-bottom: 0; + width: 50%; + clear: left; + opacity: .8; + transform: scale(.9); + } + + .leaderboard:hover { + opacity: 1; + transform: scale(1); + } + + .leaderboard:nth-child(odd) { + float: left; + } + + .leaderboard:nth-child(even) { + float: right; + } + +} + +ol { + list-style-type: none; +} + +li { + line-height: 1.5em; +} + +.rank:after { + content: '.'; + display: inline; +} + +.item { + font-weight: bold; +} + +.score { + margin-left: .25em; + font-size: .95em; + color: #aaa; +} diff --git a/src/helpers.js b/src/helpers.js index efd41ee..599e04b 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -6,7 +6,11 @@ 'use strict'; -const crypto = require( 'crypto' ); +const fs = require( 'fs' ), + crypto = require( 'crypto' ), + handlebars = require( 'handlebars' ); + +let header, footer, stylesheet; /* eslint-disable no-process-env */ const envSecret1 = process.env.SLACK_VERIFICATION_TOKEN, @@ -157,6 +161,36 @@ const maybeLinkItem = ( item ) => { return isUser( item ) ? '<@' + item + '>' : item; }; +/** + * Renders HTML for the browser, using Handlebars. Includes a standard header and footer. + * + * @param {string} template The Handlebars-compatible template to render; that is, valid HTML with + * variable interpolations as required. + * @param {object} context The Handlebars-compatible context to render; that is, an object with + * values for the variables referenced in the template. Also include + * standard variables referenced in the header and footer, such as + * 'title'. See the contents of ./html/ for more details. + * @returns {string} HTML ready to be rendered in the browser. + * @see https://handlebarsjs.com/ + */ +const render = ( template = '', context = {}) => { + + if ( ! header || ! footer || ! stylesheet ) { + header = fs.readFileSync( 'src/html/header.html', 'utf8' ); + footer = fs.readFileSync( 'src/html/footer.html', 'utf8' ); + stylesheet = fs.readFileSync( 'src/assets/main.css', 'utf8' ); + } + + const defaults = { + site_title: 'PlusPlus++ That Works', + stylesheet + }; + + const output = header + template + footer; + return handlebars.compile( output )( Object.assign( defaults, context ) ); + +}; // Render. + module.exports = { extractCommand, extractPlusMinusEventData, @@ -165,5 +199,6 @@ module.exports = { isPlural, isTimeBasedTokenStillValid, isUser, - maybeLinkItem + maybeLinkItem, + render }; diff --git a/src/html/footer.html b/src/html/footer.html new file mode 100644 index 0000000..8b03368 --- /dev/null +++ b/src/html/footer.html @@ -0,0 +1,3 @@ + + + diff --git a/src/html/header.html b/src/html/header.html new file mode 100644 index 0000000..63b3084 --- /dev/null +++ b/src/html/header.html @@ -0,0 +1,17 @@ + + + + {{title}} - {{site_title}} + + + + +
+ +
+ {{site_title}} +
+ +

+ {{title}} +

diff --git a/src/html/leaderboard.html b/src/html/leaderboard.html new file mode 100644 index 0000000..a8c2628 --- /dev/null +++ b/src/html/leaderboard.html @@ -0,0 +1,25 @@ +
+

Users

+
    + {{#each users}} +
  1. + {{this.rank}} + {{this.item}} + {{this.score}} +
    2. + {{/each}} +
+
+ +
+

Things

+
    + {{#each things}} +
  1. + {{this.rank}} + {{this.item}} + {{this.score}} +
    2. + {{/each}} +
+
diff --git a/src/leaderboard.js b/src/leaderboard.js index 7736400..54d23f5 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -1,5 +1,7 @@ /** * Contains logic for returning the leaderboard. + * + * @author Tim Malone */ 'use strict'; @@ -108,6 +110,31 @@ const getLeaderboardUrl = ( hostname ) => { return url; }; +/** + * Returns HTML for the full leaderboard. + * + * TODO: This should be split out into separate frontend generating functions for the boilerplate + * structure and stuff. + * + * @returns {string} HTML for the browser. + */ +const getFull = async() => { + + if ( ! leaderboardHtml ) { + leaderboardHtml = fs.readFileSync( 'src/html/leaderboard.html', 'utf8' ); + } + + const scores = await points.retrieveTopScores(), + users = rankItems( scores, 'users', 'html' ), + things = rankItems( scores, 'things', 'html' ); + + return helpers.render( leaderboardHtml, { + users, + things, + title: 'Leaderboard' + }); +}; + /** * Retrieves and sends the current leaderboard to the requesting Slack channel. * @@ -157,6 +184,7 @@ const handler = async( event, request ) => { }; // Handler. module.exports = { + getFull, getLeaderboardUrl, rankItems, handler diff --git a/yarn.lock b/yarn.lock index 7c80ec7..ef8dc39 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2940,6 +2940,10 @@ mime@1.4.1: version "1.4.1" resolved "https://registry.yarnpkg.com/mime/-/mime-1.4.1.tgz#121f9ebc49e3766f311a76e1fa1c8003c4b03aa6" +mime@^2.3.1: + version "2.3.1" + resolved "https://registry.yarnpkg.com/mime/-/mime-2.3.1.tgz#b1621c54d63b97c47d3cfe7f7215f7d64517c369" + mimic-fn@^1.0.0: version "1.2.0" resolved "https://registry.yarnpkg.com/mimic-fn/-/mimic-fn-1.2.0.tgz#820c86a39334640e99516928bd03fca88057d022" From 248ec50ff98612982c33dfba0fc04c0580cbbee9 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 12:31:43 +1000 Subject: [PATCH 27/39] WIP: Minor tweaks and fixes for web leaderboard --- package.json | 1 + src/assets/main.css | 82 +++++++++++++++++++++++++-------------- src/html/leaderboard.html | 1 + src/leaderboard.js | 6 +-- yarn.lock | 2 +- 5 files changed, 58 insertions(+), 34 deletions(-) diff --git a/package.json b/package.json index d6f3a76..16b7341 100644 --- a/package.json +++ b/package.json @@ -29,6 +29,7 @@ "@slack/client": "^4.3.1", "body-parser": "^1.18.3", "express": "^4.16.3", + "handlebars": "^4.0.11", "lodash.camelcase": "^4.3.0", "mime": "^2.3.1", "pg": "^7.4.3" diff --git a/src/assets/main.css b/src/assets/main.css index 1118c9f..6bb969f 100644 --- a/src/assets/main.css +++ b/src/assets/main.css @@ -9,16 +9,22 @@ body { font-family: Arial, Helvetica, sans-serif; - font-size: 16px; + font-size: 14px; background-color: #7DB83C; } .wrapper { width: 90%; - max-width: 600px; + max-width: 900px; margin: 3em auto; } +.wrapper:after { + display: table; + clear: both; + content: ''; +} + .site-title, h1, h2 { @@ -26,41 +32,17 @@ h2 { } h1 { - margin: .5em 0 1.5em; + margin: .5em 0 1em; } .leaderboard { + box-sizing: border-box; margin-bottom: 3em; - padding: 5% 10% 10%; - border-radius: 100% 50%; + padding: 10px 25px 25px; + border-radius: 200px 100px; background-color: #fff; } -@media only screen and (min-width: 600px) { - - .leaderboard { - margin-bottom: 0; - width: 50%; - clear: left; - opacity: .8; - transform: scale(.9); - } - - .leaderboard:hover { - opacity: 1; - transform: scale(1); - } - - .leaderboard:nth-child(odd) { - float: left; - } - - .leaderboard:nth-child(even) { - float: right; - } - -} - ol { list-style-type: none; } @@ -83,3 +65,43 @@ li { font-size: .95em; color: #aaa; } + +@media only screen and (min-width: 430px) { + + body { + font-size: 16px; + } + + .leaderboard { + padding: 20px 50px 50px; + } + +} /* min-width: 430 */ + +@media only screen and (min-width: 800px) { + + .leaderboard { + margin-bottom: 0; + width: 50%; + opacity: .8; + transform: scale(.9); + } + + .leaderboard:hover { + opacity: 1; + transform: scale(1); + } + + .leaderboard:nth-child(odd) { + float: left; + clear: left; + transform-origin: top left; + } + + .leaderboard:nth-child(even) { + float: right; + transform-origin: top right; + border-radius: 100px 200px; + } + +} /* min-width: 800 */ diff --git a/src/html/leaderboard.html b/src/html/leaderboard.html index a8c2628..27de702 100644 --- a/src/html/leaderboard.html +++ b/src/html/leaderboard.html @@ -1,3 +1,4 @@ +

Users

    diff --git a/src/leaderboard.js b/src/leaderboard.js index 54d23f5..ccf5cdc 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -75,7 +75,7 @@ const rankItems = ( topScores, itemType = 'users', format = 'slack' ) => { break; - case 'html': + case 'object': output = { rank, item: prefix + itemTitleCase, @@ -125,8 +125,8 @@ const getFull = async() => { } const scores = await points.retrieveTopScores(), - users = rankItems( scores, 'users', 'html' ), - things = rankItems( scores, 'things', 'html' ); + users = rankItems( scores, 'users', 'object' ), + things = rankItems( scores, 'things', 'object' ); return helpers.render( leaderboardHtml, { users, diff --git a/yarn.lock b/yarn.lock index ef8dc39..ed604c9 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1730,7 +1730,7 @@ growly@^1.3.0: version "1.3.0" resolved "https://registry.yarnpkg.com/growly/-/growly-1.3.0.tgz#f10748cbe76af964b7c96c93c6bcc28af120c081" -handlebars@^4.0.3: +handlebars@^4.0.11, handlebars@^4.0.3: version "4.0.11" resolved "https://registry.yarnpkg.com/handlebars/-/handlebars-4.0.11.tgz#630a35dfe0294bc281edae6ffc5d329fc7982dcc" dependencies: From accabd283439dbf6eebdc95475dd90ea01ca9afc Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 12:32:21 +1000 Subject: [PATCH 28/39] Hotfix: ensure token on verification is an integer to avoid concatenation --- src/helpers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helpers.js b/src/helpers.js index 599e04b..c08dd39 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -121,7 +121,7 @@ const isTimeBasedTokenStillValid = ( token, ts ) => { const now = getTimestamp(); // Don't support tokens too far from the past. - if ( now > ts + TOKEN_TTL ) { + if ( now > parseInt( ts ) + TOKEN_TTL ) { return false; } From 3911340485fbd5f40c61666e841e9a90fd0f6741 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 13:23:13 +1000 Subject: [PATCH 29/39] Leaderboard: complete web-viewable leaderboard with user name retrieval --- src/leaderboard.js | 29 ++++++++++++------------ src/send.js | 56 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 15 deletions(-) diff --git a/src/leaderboard.js b/src/leaderboard.js index ccf5cdc..4a8492f 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -33,13 +33,14 @@ let leaderboardHtml; * human-readable Slack strings or objects containing 'rank', 'item' and * 'score' values. */ -const rankItems = ( topScores, itemType = 'users', format = 'slack' ) => { +const rankItems = async( topScores, itemType = 'users', format = 'slack' ) => { let lastScore, lastRank, output; const items = []; for ( const score of topScores ) { + let item = score.item; const isUser = helpers.isUser( score.item ) ? true : false; // Skip if this item is not the item type we're ranking. @@ -47,16 +48,16 @@ const rankItems = ( topScores, itemType = 'users', format = 'slack' ) => { continue; } - // TODO: If 'slack' !== format, we're gonna need to get the user's name from the Slack API. + // For users, we need to link the item (for Slack) or get their real name (for other formats). + if ( isUser ) { + item = ( + 'slack' === format ? helpers.maybeLinkItem( item ) : await send.getUserName( item ) + ); + } - const item = 'slack' === format ? helpers.maybeLinkItem( score.item ) : score.item, - itemTitleCase = item.substring( 0, 1 ).toUpperCase() + item.substring( 1 ), + const itemTitleCase = item.substring( 0, 1 ).toUpperCase() + item.substring( 1 ), plural = helpers.isPlural( score.score ) ? 's' : ''; - // For the Slack format, *users* are already linked with an @. In all other cases we need to - // insert an @ for items too. - const prefix = isUser && 'slack' === format ? '' : '@'; - // Determine the rank by keeping it the same as the last user if the score is the same, or // otherwise setting it to the same as the item count (and adding 1 to deal with 0-base count). const rank = score.score === lastScore ? lastRank : items.length + 1; @@ -65,7 +66,7 @@ const rankItems = ( topScores, itemType = 'users', format = 'slack' ) => { case 'slack': output = ( - rank + '. ' + prefix + itemTitleCase + ' [' + score.score + ' point' + plural + ']' + rank + '. ' + itemTitleCase + ' [' + score.score + ' point' + plural + ']' ); // If this is the first item, it's the winner! @@ -78,7 +79,7 @@ const rankItems = ( topScores, itemType = 'users', format = 'slack' ) => { case 'object': output = { rank, - item: prefix + itemTitleCase, + item: itemTitleCase, score: score.score + ' point' + plural }; break; @@ -125,8 +126,8 @@ const getFull = async() => { } const scores = await points.retrieveTopScores(), - users = rankItems( scores, 'users', 'object' ), - things = rankItems( scores, 'things', 'object' ); + users = await rankItems( scores, 'users', 'object' ), + things = await rankItems( scores, 'things', 'object' ); return helpers.render( leaderboardHtml, { users, @@ -149,8 +150,8 @@ const handler = async( event, request ) => { const limit = 5; const scores = await points.retrieveTopScores(), - users = rankItems( scores, 'users' ), - things = rankItems( scores, 'things' ); + users = await rankItems( scores, 'users' ), + things = await rankItems( scores, 'things' ); const messageText = ( 'Here you go. ' + diff --git a/src/send.js b/src/send.js index 1bcee95..68b0360 100644 --- a/src/send.js +++ b/src/send.js @@ -2,12 +2,15 @@ * Handles sending of messages - i.e. outgoing messages - back to Slack, via Slack's Web API. See * also ./events.js, which handles incoming messages from subscribed events. * + * TODO: This file should probably be renamed to 'slack.js' so it can handle all other requests to + * the Slack APIs rather than just sending. + * * @see https://api.slack.com/web */ 'use strict'; -let slack; +let slack, users; /** * Injects the Slack client to be used for all outgoing messages. @@ -22,6 +25,55 @@ const setSlackClient = ( client ) => { slack = client; }; +/** + * Retrieves a list of all users in the linked Slack team. Caches it in memory. + * + * @returns {object} A collection of Slack user objects, indexed by the user IDs (Uxxxxxxxx). + */ +const getUserList = async() => { + + if ( users ) { + return users; + } + + console.log( 'Retrieving user list from Slack...' ); + + users = {}; + const userList = await slack.users.list(); + + if ( ! userList.ok ) { + throw Error( 'Error occurred retrieving user list from Slack.' ); + } + + for ( const user of userList.members ) { + users[ user.id ] = user; + } + + return users; + +}; // GetUserList. + +/** + * Given a Slack user ID, returns the user's real name or optionally, the user's username. If the + * user *does not* have a real name set, their username is returned regardless. + * + * @param {string} userId A Slack user ID in the format Uxxxxxxxx. + * @param {bool} username Whether the username should always be returned instead of the real name. + * @returns {string} The user's real name, as per their Slack profile. + */ +const getUserName = async( userId, username = false ) => { + + const users = await getUserList(), + user = users[ userId ]; + + if ( 'undefined' === typeof user ) { + return '(unknown)'; + } + + return username || ! user.profile.real_name ? user.name : user.profile.real_name; + +}; + /** * Sends a message to a Slack channel. * @@ -62,5 +114,7 @@ const sendMessage = ( text, channel ) => { module.exports = { setSlackClient, + getUserList, + getUserName, sendMessage }; From 8bb6e41da5cbd22cb6ca982c23cca3f4fd8702f0 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 13:23:28 +1000 Subject: [PATCH 30/39] Minor tweaks and cleanups --- src/helpers.js | 2 +- src/leaderboard.js | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/helpers.js b/src/helpers.js index c08dd39..b6b758f 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -46,7 +46,7 @@ const extractCommand = ( message, commands ) => { return firstCommand ? firstCommand : false; -}; +}; // ExtractCommand. /** * Gets the user or 'thing' that is being spoken about, and the 'operation' being done on it. diff --git a/src/leaderboard.js b/src/leaderboard.js index 4a8492f..fc715e0 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -129,16 +129,21 @@ const getFull = async() => { users = await rankItems( scores, 'users', 'object' ), things = await rankItems( scores, 'things', 'object' ); - return helpers.render( leaderboardHtml, { + const data = { users, things, title: 'Leaderboard' - }); + }; + + return helpers.render( leaderboardHtml, data ); + }; /** * Retrieves and sends the current leaderboard to the requesting Slack channel. * + * TODO: Rename this method to something like getPartial() or getForSlack(). + * * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at * https://api.slack.com/events-api#events_dispatched_as_json and * https://api.slack.com/events/app_mention for details. From b75994b2d92994f74b83400542885334750f3ae8 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 14:49:04 +1000 Subject: [PATCH 31/39] Rename 'send' functions to 'slack' because they now do more than just sending --- index.js | 8 +++---- src/events.js | 6 +++--- src/leaderboard.js | 4 ++-- src/{send.js => slack.js} | 2 +- tests/events.js | 32 ++++++++++++++-------------- tests/{send.js => slack.js} | 42 +++++++++++++++++++++++++------------ 6 files changed, 55 insertions(+), 39 deletions(-) rename src/{send.js => slack.js} (98%) rename tests/{send.js => slack.js} (76%) diff --git a/index.js b/index.js index 88d3578..47eccfe 100644 --- a/index.js +++ b/index.js @@ -10,13 +10,13 @@ 'use strict'; const app = require( './src/app' ), - send = require( './src/send' ); + slack = require( './src/slack' ); const fs = require( 'fs' ), mime = require( 'mime' ), - slack = require( '@slack/client' ), express = require( 'express' ), - bodyParser = require( 'body-parser' ); + bodyParser = require( 'body-parser' ), + slackClient = require( '@slack/client' ); /* eslint-disable no-process-env, no-magic-numbers */ const PORT = process.env.PORT || 80; // Let Heroku set the port. @@ -37,7 +37,7 @@ const bootstrap = ( options = {}) => { // Allow alternative implementations of both Express and Slack to be passed in. const server = options.express || express(); - send.setSlackClient( options.slack || new slack.WebClient( SLACK_OAUTH_ACCESS_TOKEN ) ); + slack.setSlackClient( options.slack || new slackClient.WebClient( SLACK_OAUTH_ACCESS_TOKEN ) ); server.use( bodyParser.json() ); server.enable( 'trust proxy' ); diff --git a/src/events.js b/src/events.js index 2444c37..564cb06 100644 --- a/src/events.js +++ b/src/events.js @@ -7,7 +7,7 @@ 'use strict'; -const send = require( './send' ), +const slack = require( './slack' ), points = require( './points' ), helpers = require( './helpers' ), messages = require( './messages' ), @@ -28,7 +28,7 @@ const camelCase = require( 'lodash.camelcase' ); const handleSelfPlus = ( user, channel ) => { console.log( user + ' tried to alter their own score.' ); const message = messages.getRandomMessage( operations.operations.SELF, user ); - return send.sendMessage( message, channel ); + return slack.sendMessage( message, channel ); }; /** @@ -47,7 +47,7 @@ const handlePlusMinus = async( item, operation, channel ) => { operationName = operations.getOperationName( operation ), message = messages.getRandomMessage( operationName, item, score ); - return send.sendMessage( message, channel ); + return slack.sendMessage( message, channel ); }; const handlers = { diff --git a/src/leaderboard.js b/src/leaderboard.js index fc715e0..80596ce 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -6,7 +6,7 @@ 'use strict'; -const send = require( './send' ), +const slack = require( './slack' ), points = require( './points' ), helpers = require( './helpers' ); @@ -51,7 +51,7 @@ const rankItems = async( topScores, itemType = 'users', format = 'slack' ) => { // For users, we need to link the item (for Slack) or get their real name (for other formats). if ( isUser ) { item = ( - 'slack' === format ? helpers.maybeLinkItem( item ) : await send.getUserName( item ) + 'slack' === format ? helpers.maybeLinkItem( item ) : await slack.getUserName( item ) ); } diff --git a/src/send.js b/src/slack.js similarity index 98% rename from src/send.js rename to src/slack.js index 68b0360..0f3802a 100644 --- a/src/send.js +++ b/src/slack.js @@ -36,7 +36,7 @@ const getUserList = async() => { return users; } - console.log( 'Retrieving user list from Slack...' ); + console.log( 'Retrieving user list from Slack.' ); users = {}; const userList = await slack.users.list(); diff --git a/tests/events.js b/tests/events.js index 316bf73..5ebb79b 100644 --- a/tests/events.js +++ b/tests/events.js @@ -13,14 +13,14 @@ const events = require( '../src/events' ); const handlers = events.handlers; -const send = require( '../src/send' ), +const slack = require( '../src/slack' ), points = require( '../src/points' ), messages = require( '../src/messages' ); const slackClientMock = require( './mocks/slack' ); -send.setSlackClient( slackClientMock ); +slack.setSlackClient( slackClientMock ); -send.sendMessage = jest.fn(); +slack.sendMessage = jest.fn(); points.updateScore = jest.fn(); messages.getRandomMessage = jest.fn(); @@ -34,7 +34,7 @@ console.warn = jest.fn(); beforeEach( () => { jest.resetModules(); messages.getRandomMessage.mockClear(); - send.sendMessage.mockClear(); + slack.sendMessage.mockClear(); }); describe( 'handleSelfPlus', () => { @@ -56,15 +56,15 @@ describe( 'handleSelfPlus', () => { }); it( 'sends a message back to the user and channel that called it', () => { - const send = require( '../src/send' ), + const slack = require( '../src/slack' ), events = require( '../src/events' ); - send.sendMessage = jest.fn(); - send.setSlackClient( slackClientMock ); + slack.sendMessage = jest.fn(); + slack.setSlackClient( slackClientMock ); events.handleSelfPlus( user, channel ); - expect( send.sendMessage ) + expect( slack.sendMessage ) .toHaveBeenCalledTimes( 1 ) .toHaveBeenCalledWith( expect.stringContaining( user ), channel ); }); @@ -83,11 +83,11 @@ describe( 'handlePlusMinus', () => { }; it( 'calls the score updater to update an item\'s score', () => { - const send = require( '../src/send' ), + const slack = require( '../src/slack' ), points = require( '../src/points' ), events = require( '../src/events' ); - send.setSlackClient( slackClientMock ); + slack.setSlackClient( slackClientMock ); points.updateScore = jest.fn(); events.handlePlusMinus( item, '+', channel ); @@ -102,12 +102,12 @@ describe( 'handlePlusMinus', () => { ( operationName, operation ) => { expect.hasAssertions(); - const send = require( '../src/send' ), + const slack = require( '../src/slack' ), points = require( '../src/points' ), events = require( '../src/events' ), messages = require( '../src/messages' ); - send.setSlackClient( slackClientMock ); + slack.setSlackClient( slackClientMock ); points.updateScore = jest.fn( updateScoreMock ); messages.getRandomMessage = jest.fn(); @@ -122,16 +122,16 @@ describe( 'handlePlusMinus', () => { it( 'sends a message back to the channel that called it', () => { expect.hasAssertions(); - const send = require( '../src/send' ), + const slack = require( '../src/slack' ), points = require( '../src/points' ), events = require( '../src/events' ); - send.setSlackClient( slackClientMock ); + slack.setSlackClient( slackClientMock ); points.updateScore = jest.fn(); - send.sendMessage = jest.fn(); + slack.sendMessage = jest.fn(); return events.handlePlusMinus( item, '+', channel ).then( () => { - expect( send.sendMessage ) + expect( slack.sendMessage ) .toHaveBeenCalledTimes( 1 ) .toHaveBeenCalledWith( expect.stringContaining( item ), channel ); }); diff --git a/tests/send.js b/tests/slack.js similarity index 76% rename from tests/send.js rename to tests/slack.js index e47d443..be2e692 100644 --- a/tests/send.js +++ b/tests/slack.js @@ -10,7 +10,7 @@ 'use strict'; -const send = require( '../src/send' ); +const slack = require( '../src/slack' ); const pathToMock = './mocks/slack'; // Catch all console output during tests. @@ -27,11 +27,27 @@ beforeEach( () => { describe( 'setSlackClient', () => { it( 'accepts a single parameter (that is later used as the Slack API client)', () => { - expect( send.setSlackClient ).toHaveLength( 1 ); + expect( slack.setSlackClient ).toHaveLength( 1 ); }); }); +describe( 'getUserList', () => { + + it( '', () => { + + }); + +}); // GetUserList. + +describe( 'getUserName', () => { + + it( '', () => { + + }); + +}); // GetUserName. + describe( 'sendMessage', () => { const payload = { @@ -42,12 +58,12 @@ describe( 'sendMessage', () => { it( 'sends message text to a channel when provided as two arguments', () => { expect.assertions( 1 ); const slackClientMock = require( pathToMock ); - send.setSlackClient( slackClientMock ); + slack.setSlackClient( slackClientMock ); // Re-mock the client so we can listen to it. slackClientMock.chat.postMessage = jest.fn(); - return send.sendMessage( payload.text, payload.channel ).catch( () => { + return slack.sendMessage( payload.text, payload.channel ).catch( () => { expect( slackClientMock.chat.postMessage ).toHaveBeenCalledWith( payload ); }); }); @@ -55,7 +71,7 @@ describe( 'sendMessage', () => { it( 'sends a message to a channel with a full payload as one argument', () => { expect.assertions( 1 ); const slackClientMock = require( pathToMock ); - send.setSlackClient( slackClientMock ); + slack.setSlackClient( slackClientMock ); // Re-mock the client so we can listen to it. slackClientMock.chat.postMessage = jest.fn(); @@ -70,7 +86,7 @@ describe( 'sendMessage', () => { ] }; - return send.sendMessage( payload ).catch( () => { + return slack.sendMessage( payload ).catch( () => { expect( slackClientMock.chat.postMessage ).toHaveBeenCalledWith( payload ); }); }); @@ -78,7 +94,7 @@ describe( 'sendMessage', () => { it( 'sends a message to a channel when payload and channel are passed separately', () => { expect.assertions( 1 ); const slackClientMock = require( pathToMock ); - send.setSlackClient( slackClientMock ); + slack.setSlackClient( slackClientMock ); // Re-mock the client so we can listen to it. slackClientMock.chat.postMessage = jest.fn(); @@ -93,7 +109,7 @@ describe( 'sendMessage', () => { ] }; - return send.sendMessage( payload, channel ).catch( () => { + return slack.sendMessage( payload, channel ).catch( () => { payload.channel = channel; expect( slackClientMock.chat.postMessage ).toHaveBeenCalledWith( payload ); }); @@ -102,10 +118,10 @@ describe( 'sendMessage', () => { it( 'returns a Promise and resolves it if the message succeeds', () => { expect.assertions( 1 ); const slackClientMock = require( pathToMock ); - send.setSlackClient( slackClientMock ); + slack.setSlackClient( slackClientMock ); slackClientMock.options.shouldPostMessageSucceed = true; - return send.sendMessage( payload.text, payload.channel ).then( ( data ) => { + return slack.sendMessage( payload.text, payload.channel ).then( ( data ) => { expect( data ).toBeNil(); }); }); @@ -113,12 +129,12 @@ describe( 'sendMessage', () => { it( 'returns a Promise and rejects it if the message fails', () => { expect.assertions( 1 ); const slackClientMock = require( pathToMock ); - send.setSlackClient( slackClientMock ); + slack.setSlackClient( slackClientMock ); slackClientMock.options.shouldPostMessageSucceed = false; - return send.sendMessage( payload.text, payload.channel ).catch( ( error ) => { + return slack.sendMessage( payload.text, payload.channel ).catch( ( error ) => { expect( error ).toBeNil(); }); }); -}); +}); // SendMessage. From 46c91d679f1499e8dfdb158acc4933b5c51e803b Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 14:51:14 +1000 Subject: [PATCH 32/39] Rename leaderboard retrieval functions + simplify HTML rendering by only requiring template filename to be passed --- src/app.js | 2 +- src/helpers.js | 40 ++++++++++------- src/leaderboard.js | 110 ++++++++++++++++++++++----------------------- 3 files changed, 81 insertions(+), 71 deletions(-) diff --git a/src/app.js b/src/app.js index 94ec593..2fd18b1 100644 --- a/src/app.js +++ b/src/app.js @@ -91,7 +91,7 @@ const handleGet = async( request, response ) => { // TODO: This should probably be split out into a separate function of sorts, like handlePost. case '/leaderboard': if ( helpers.isTimeBasedTokenStillValid( request.query.token, request.query.ts ) ) { - response.send( await leaderboard.getFull() ); + response.send( await leaderboard.getForWeb() ); } else { response .status( HTTP_403 ) diff --git a/src/helpers.js b/src/helpers.js index b6b758f..f788ce7 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -10,7 +10,7 @@ const fs = require( 'fs' ), crypto = require( 'crypto' ), handlebars = require( 'handlebars' ); -let header, footer, stylesheet; +const templates = {}; /* eslint-disable no-process-env */ const envSecret1 = process.env.SLACK_VERIFICATION_TOKEN, @@ -164,29 +164,39 @@ const maybeLinkItem = ( item ) => { /** * Renders HTML for the browser, using Handlebars. Includes a standard header and footer. * - * @param {string} template The Handlebars-compatible template to render; that is, valid HTML with - * variable interpolations as required. - * @param {object} context The Handlebars-compatible context to render; that is, an object with - * values for the variables referenced in the template. Also include - * standard variables referenced in the header and footer, such as - * 'title'. See the contents of ./html/ for more details. + * @param {string} templatePath Path to the Handlebars-compatible template file to render; that is, + * a file containing valid HTML, with variable interpolations as + * required. The path should be relative to the app's entry-point + * (which is usually an index.js in the root of the repository). + * @param {object} context The Handlebars-compatible context to render; that is, an object with + * values for the variables referenced in the template. Also include + * standard variables referenced in the header and footer, such as + * 'title'. See the contents of ./html/ for more details. Some + * variables may have defaults provided, which can be overridden. * @returns {string} HTML ready to be rendered in the browser. * @see https://handlebarsjs.com/ */ -const render = ( template = '', context = {}) => { +const render = ( templatePath = '', context = {}) => { - if ( ! header || ! footer || ! stylesheet ) { - header = fs.readFileSync( 'src/html/header.html', 'utf8' ); - footer = fs.readFileSync( 'src/html/footer.html', 'utf8' ); - stylesheet = fs.readFileSync( 'src/assets/main.css', 'utf8' ); + // Retrieve the header and footer HTML, if we don't already have it in memory. + if ( ! templates.header ) templates.header = fs.readFileSync( 'src/html/header.html', 'utf8' ); + if ( ! templates.footer ) templates.footer = fs.readFileSync( 'src/html/footer.html', 'utf8' ); + + // Retrieve the requested template HTML if it is not already in memory. + if ( ! templates[ templatePath ]) { + console.log( 'Retrieving template ' + templatePath + '.' ); + templates[ templatePath ] = fs.readFileSync( templatePath, 'utf8' ); } + /* eslint-disable camelcase */ // Handlebars templates commonly use snake_case instead. const defaults = { - site_title: 'PlusPlus++ That Works', - stylesheet + + // TODO: Get this from bot's name settings in the Slack users list. + site_title: 'PlusPlus++ That Works' }; + /* eslint-enable camelcase */ - const output = header + template + footer; + const output = templates.header + templates[ templatePath ] + templates.footer; return handlebars.compile( output )( Object.assign( defaults, context ) ); }; // Render. diff --git a/src/leaderboard.js b/src/leaderboard.js index 80596ce..97550f1 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -10,9 +10,20 @@ const slack = require( './slack' ), points = require( './points' ), helpers = require( './helpers' ); -const fs = require( 'fs' ); +/** + * Gets the URL for the full leaderboard, including a token to ensure that it is only viewed by + * someone who has access to this Slack team. + * + * @param {string} hostname The hostname this app is being served on. + * @returns {string} The leaderboard URL, which will be picked up in ../index.js when called. + */ +const getLeaderboardUrl = ( hostname ) => { + const ts = helpers.getTimestamp(), + token = helpers.getTimeBasedToken( ts ), + url = 'https://' + hostname + '/leaderboard?token=' + token + '&ts=' + ts; -let leaderboardHtml; + return url; +}; /** * Ranks items by their scores, returning them in a human readable list complete with emoji for the @@ -29,9 +40,9 @@ let leaderboardHtml; * can be ranked at a time. * @param {string} format The format to return the results in. 'slack' returns or 'object'. * - * @returns {array|object} Depending on the value of 'format', an array, in rank order, of either - * human-readable Slack strings or objects containing 'rank', 'item' and - * 'score' values. + * @returns {array} An array, in rank order, of either of either human-readable Slack strings (if + * format is 'slack') or objects containing 'rank', 'item' and 'score' values (if + * format is 'object'). */ const rankItems = async( topScores, itemType = 'users', format = 'slack' ) => { @@ -97,52 +108,8 @@ const rankItems = async( topScores, itemType = 'users', format = 'slack' ) => { }; // RankItems. /** - * Gets the URL for the full leaderboard, including a token to ensure that it is only viewed by - * someone who has access to this Slack team. - * - * @param {string} hostname The hostname this app is being served on. - * @returns {string} The leaderboard URL, which will be picked up in ../index.js when called. - */ -const getLeaderboardUrl = ( hostname ) => { - const ts = helpers.getTimestamp(), - token = helpers.getTimeBasedToken( ts ), - url = 'https://' + hostname + '/leaderboard?token=' + token + '&ts=' + ts; - - return url; -}; - -/** - * Returns HTML for the full leaderboard. - * - * TODO: This should be split out into separate frontend generating functions for the boilerplate - * structure and stuff. - * - * @returns {string} HTML for the browser. - */ -const getFull = async() => { - - if ( ! leaderboardHtml ) { - leaderboardHtml = fs.readFileSync( 'src/html/leaderboard.html', 'utf8' ); - } - - const scores = await points.retrieveTopScores(), - users = await rankItems( scores, 'users', 'object' ), - things = await rankItems( scores, 'things', 'object' ); - - const data = { - users, - things, - title: 'Leaderboard' - }; - - return helpers.render( leaderboardHtml, data ); - -}; - -/** - * Retrieves and sends the current leaderboard to the requesting Slack channel. - * - * TODO: Rename this method to something like getPartial() or getForSlack(). + * Retrieves and sends the current partial leaderboard (top scores only) to the requesting Slack + * channel. * * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at * https://api.slack.com/events-api#events_dispatched_as_json and @@ -150,7 +117,7 @@ const getFull = async() => { * @param {object} request The Express request object that resulted in this handler being run. * @returns {Promise} A Promise to send the Slack message. */ -const handler = async( event, request ) => { +const getForSlack = async( event, request ) => { const limit = 5; @@ -185,13 +152,46 @@ const handler = async( event, request ) => { }; console.log( 'Sending the leaderboard.' ); - return send.sendMessage( message, event.channel ); + return slack.sendMessage( message, event.channel ); + +}; // GetForSlack. + +/** + * Retrieves and returns HTML for the full leaderboard, for displaying on the web. + * + * @returns {string} HTML for the browser. + */ +const getForWeb = async() => { + + const scores = await points.retrieveTopScores(), + users = await rankItems( scores, 'users', 'object' ), + things = await rankItems( scores, 'things', 'object' ); + + const data = { + users, + things, + title: 'Leaderboard' + }; + + return helpers.render( 'src/html/leaderboard.html', data ); -}; // Handler. +}; // GetForWeb. + +/** + * The default handler for this command when invoked over Slack. + * + * @param {*} event See the documentation for getForSlack. + * @param {*} request See the documentation for getForSlack. + * @returns {*} See the documentation for getForSlack. + */ +const handler = async( event, request ) => { + return getForSlack( event, request ); +}; module.exports = { - getFull, getLeaderboardUrl, rankItems, + getForSlack, + getForWeb, handler }; From 10df072bc5d51f1057b02ce0c12a2cccb8f873de Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 14:52:23 +1000 Subject: [PATCH 33/39] Minor cleanups, incl docs --- CONTRIBUTING.md | 2 -- README.md | 3 ++- src/app.js | 2 -- src/assets/main.css | 3 ++- src/messages.js | 4 ++-- tests/events.js | 2 ++ tests/helpers.js | 2 +- tests/messages.js | 18 ++++++++++-------- 8 files changed, 19 insertions(+), 17 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9dbfd27..959c3b1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,8 +53,6 @@ You can run just a subset of tests: - Integration tests with `yarn integration-tests` - End-to-end tests with `yarn e2e-tests` -It is normal to see _some_ errors while running the integration and end-to-end tests, but keep an eye on the exit code of the process to determine if it is successful (run `echo $?` immediately after running `yarn test` - you're looking for an exit code of `0` for a pass). - You can modify the default testing behaviour by adjusting the relevant `scripts` in [`package.json`](package.json) or in some cases by passing additional [Jest configuration parameters](https://jestjs.io/docs/en/configuration.html) at the end of the test commands above. If you come across annoying *stylistic* linting rules, feel free to [change them](https://eslint.org/docs/rules/) in [`.eslintrc.js`](.eslintrc.js) as part of your pull request, providing they don't cause an adverse effect on existing code. diff --git a/README.md b/README.md index 95ef111..95170a3 100644 --- a/README.md +++ b/README.md @@ -112,7 +112,6 @@ For full details on contributing, including getting a local environment set up, Although it works, it's very basic. Potential enhancements include: * A way to retrieve the current version/git hash from Slack, for sanity-checking of deployments -* Leaderboard functionality (either, or both, via a full leaderboard on the web - with some sort of token or oauth - and a shorter leaderboard via a command in Slack) * The ability to customise the messages the bot sends back at runtime (eg. via environment variables) * Move to the newer, more secure method of calculating signatures for incoming Slack hooks * A way to look up someone's karma without necessarily `++`'ing or `--`'ing them (eg. `@username==`) @@ -124,6 +123,8 @@ Although it works, it's very basic. Potential enhancements include: * Record and make accessible how many karma points someone has _given_ * Set up a Dockerfile to make local development easier (i.e. to not require Node, Yarn or Postgres) * Improve error handling +* The ability to customise some of the leaderboard web functionality, such as colours and perhaps imagery as well +* Additional linting tools for CSS and HTML ## License diff --git a/src/app.js b/src/app.js index 2fd18b1..cc227f7 100644 --- a/src/app.js +++ b/src/app.js @@ -41,8 +41,6 @@ const logRequest = ( request ) => { * WARNING: When checking the return value of this function, ensure you use strict equality so that * an error response is not misinterpreted as truthy. * - * TODO: Move to calculating the signature instead (newer, more secure method). - * * @param {string} suppliedToken The token supplied in the request. * @param {string} serverToken The token to validate against. * @return {object|bool} If invalid, an error object containing an 'error' with HTTP status code diff --git a/src/assets/main.css b/src/assets/main.css index 6bb969f..78f840e 100644 --- a/src/assets/main.css +++ b/src/assets/main.css @@ -1,6 +1,7 @@ /** + * Main stylesheet for web-based pages for Working PlusPlus++. * - * + * @author Tim Malone */ * { diff --git a/src/messages.js b/src/messages.js index 8d27724..c0245ff 100644 --- a/src/messages.js +++ b/src/messages.js @@ -104,7 +104,7 @@ const getRandomMessage = ( operation, item, score = 0 ) => { break; default: - throw 'Invalid operation: ' + operation; + throw Error ( 'Invalid operation: ' + operation ); } let totalProbability = 0; @@ -125,7 +125,7 @@ const getRandomMessage = ( operation, item, score = 0 ) => { } if ( null === chosenSet ) { - throw ( + throw Error( 'Could not find set for ' + operation + ' (ran out of sets with ' + setRandom + ' remaining)' ); } diff --git a/tests/events.js b/tests/events.js index 5ebb79b..4a836fd 100644 --- a/tests/events.js +++ b/tests/events.js @@ -182,6 +182,7 @@ describe( 'handlers.appMention', () => { ]; it.each( appCommandTable )( 'calls the app command handler for %s', ( command, handlerFile ) => { + const event = { type: eventType, text: '<@U00000000> ' + command @@ -193,6 +194,7 @@ describe( 'handlers.appMention', () => { commandHandler.handler = jest.fn(); events.handlers.appMention( event ); expect( commandHandler.handler ).toHaveBeenCalledTimes( 1 ); + }); it( 'returns false if a supported command cannot be found', () => { diff --git a/tests/helpers.js b/tests/helpers.js index e649ac5..bc50cb3 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -261,7 +261,7 @@ describe( 'isTimeBasedTokenStillValid', () => { expect( helpers.isTimeBasedTokenStillValid( token, twoDaysAgo ) ).toBeFalse(); }); -}); +}); // IsTimeBasedTokenStillValid. describe( 'isUser', () => { diff --git a/tests/messages.js b/tests/messages.js index 8b8964a..663d773 100644 --- a/tests/messages.js +++ b/tests/messages.js @@ -20,14 +20,16 @@ const operations = [ 'selfPlus' ]; -for ( const operation of operations ) { - it( 'returns a message for the ' + operation + ' operation', () => { +describe( 'getRandomMessage', () => { + + it.each( operations )( 'returns a message for the %s operation', ( operation ) => { expect( typeof messages.getRandomMessage( operation, 'RandomThing' ) ).toBe( 'string' ); }); -} -it( 'throws an error for an invalid operation', () => { - expect( () => { - messages.getRandomMessage( 'INVALID_OPERATION', 'RandomThing' ); - }).toThrow(); -}); + it( 'throws an error for an invalid operation', () => { + expect( () => { + messages.getRandomMessage( 'INVALID_OPERATION', 'RandomThing' ); + }).toThrow(); + }); + +}); // GetRandomMessage. From d02c636a5c4e1a79f00b61fb933c00d7b032efc1 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 14:52:48 +1000 Subject: [PATCH 34/39] Hotfix: prevent missing request data in events test --- tests/events.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/events.js b/tests/events.js index 4a836fd..469416d 100644 --- a/tests/events.js +++ b/tests/events.js @@ -215,13 +215,17 @@ describe( 'handleEvent', () => { [ 'app_mention', '<@U12345678> can haz leaderboard' ] ]; + const request = { + headers: { host: 'test.local' } + }; + it.each( validEvents )( 'returns a Promise for a \'%s\' event with text', ( type, text ) => { const event = { type, text }; - expect( events.handleEvent( event ) instanceof Promise ).toBeTrue(); + expect( events.handleEvent( event, request ) instanceof Promise ).toBeTrue(); }); it.each( validEvents )( 'reports a \'%s\' event without text as invalid', ( type ) => { From 46ce97688dc5bb7c317ae7ae36a186bfdb0cdd42 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 14:54:49 +1000 Subject: [PATCH 35/39] Minor: remove empty tests that weren't supposed to make it in yet --- tests/slack.js | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/tests/slack.js b/tests/slack.js index be2e692..472a317 100644 --- a/tests/slack.js +++ b/tests/slack.js @@ -32,22 +32,6 @@ describe( 'setSlackClient', () => { }); -describe( 'getUserList', () => { - - it( '', () => { - - }); - -}); // GetUserList. - -describe( 'getUserName', () => { - - it( '', () => { - - }); - -}); // GetUserName. - describe( 'sendMessage', () => { const payload = { From 9ad31b0124a3b294833547591331def368b06759 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 15:06:19 +1000 Subject: [PATCH 36/39] Update CHANGELOG after merge of feature/leaderboard --- CHANGELOG.md | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb7010f..2863f0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,20 @@ # Changelog -All notable changes to this project will be documented in this file. +All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added +- Leaderboard functionality when `@WorkingPlusPlus leaderboard` is sent (requires additional app permissions - see step 6 of the installation instructions), including a link to a full web-based leaderboard with a time-based token to protect your team's data +- Contributors: a raft of new helper functions including `isUser`, `isPlural` and many more +- Contributors: additional tests to cover all previously added functionality (leaderboard functionality is not covered yet) + +### Changed +- Contributors: another set of significant structural re-organisations to set the app up for handling more 'direct commands' in future + +### Fixed +- Prevented +- or -+ from being interpreted as valid commands ([`005b69c`](https://github.com/tdmalone/working-plusplus/commit/005b69c6b297abf5c1014fd2dedc7db9e54b2900)) + ## [0.0.3] - 2018-08-11 ### Added @@ -26,7 +37,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a - Contributors: quite a bit of reorganising to make it easier both to grow and to test the app ### Fixed -- Scores of -1 are now referred to as -1 point, rather than -1 points +- Scores of -1 are now referred to as -1 point, rather than -1 points ([`d7d92be`](https://github.com/tdmalone/working-plusplus/commit/d7d92be0cd31aed26afcac1d189d17381330f418)) ## [0.0.1] - 2018-08-06 From 0811a8494906ab49fcfc28b001549d268528dd7e Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 15:49:02 +1000 Subject: [PATCH 37/39] Add 'help' and 'thankyou' commands' --- CHANGELOG.md | 1 + README.md | 1 + src/events.js | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2863f0d..a1298ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a ### Added - Leaderboard functionality when `@WorkingPlusPlus leaderboard` is sent (requires additional app permissions - see step 6 of the installation instructions), including a link to a full web-based leaderboard with a time-based token to protect your team's data +- Help message when `@WorkingPlusPlus help` is sent (requires the same additional app permissions as above) - Contributors: a raft of new helper functions including `isUser`, `isPlural` and many more - Contributors: additional tests to cover all previously added functionality (leaderboard functionality is not covered yet) diff --git a/README.md b/README.md index 95170a3..bd2a07f 100644 --- a/README.md +++ b/README.md @@ -96,6 +96,7 @@ Currently supported general commands are: Currently supported extended commands are: * `@WorkingPlusPlus leaderboard`: Displays the leaderboard for your Slack workspace +* `@WorkingPlusPlus help`: Displays a help message showing these commands If you set a different name for your bot when adding the app to your Slack workspace, use that name instead. diff --git a/src/events.js b/src/events.js index 564cb06..94623ff 100644 --- a/src/events.js +++ b/src/events.js @@ -50,6 +50,62 @@ const handlePlusMinus = async( item, operation, channel ) => { return slack.sendMessage( message, channel ); }; +/** + * Sends a random thank you message to the requesting channel. + * + * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at + * https://api.slack.com/events-api#events_dispatched_as_json and + * https://api.slack.com/events/app_mention for details. + * @returns {Promise} A Promise to send the Slack message. + */ +const sayThankyou = ( event ) => { + + const thankyouMessages = [ + 'Don\'t mention it!', + 'You\'re welcome.', + 'Pleasure!', + 'No thank YOU!', + ( + '++ for taking the time to say thanks!\n...' + + 'just kidding, I can\'t `++` you. But it\'s the thought that counts, right??' + ) + ]; + + const randomKey = Math.floor( Math.random() * thankyouMessages.length ), + message = '<@' + event.user + '> ' + thankyouMessages[ randomKey ]; + + return slack.sendMessage( message, event.channel ); + +}; // SayThankyou. + +/** + * Sends a help message, explaining the bot's commands, to the requesting channel. + * + * @param {object} event A hash of a validated Slack 'app_mention' event. See the docs at + * https://api.slack.com/events-api#events_dispatched_as_json and + * https://api.slack.com/events/app_mention for details. + * @returns {Promise} A Promise to send the Slack message. + */ +const sendHelp = ( event ) => { + + const botUserId = event.text.match( /<@(U[A-Z0-9]{8})>/ )[1]; + + const message = ( + 'Sure, here\'s what I can do:\n\n' + + '• `@Someone++`: Add points to a user or a thing\n' + + '• `@Someone--`: Subtract points from a user or a thing\n' + + '• `<@' + botUserId + '> leaderboard`: Display the leaderboard\n' + + '• `<@' + botUserId + '> help`: Display this message\n\n' + + 'You\'ll need to invite me to a channel before I can recognise ' + + '`++` and `--` commands in it.\n\n' + + 'If you\'re a developer, you can teach me new things! ' + + 'See to get started.' + ); + + return slack.sendMessage( message, event.channel ); + +}; // SendHelp. + const handlers = { /** @@ -98,7 +154,11 @@ const handlers = { appMention: ( event, request ) => { const appCommandHandlers = { - leaderboard: leaderboard.handler + leaderboard: leaderboard.handler, + help: sendHelp, + thx: sayThankyou, + thanks: sayThankyou, + thankyou: sayThankyou }; const validCommands = Object.keys( appCommandHandlers ), @@ -133,6 +193,10 @@ const handleEvent = ( event, request ) => { } // If the event has a subtype, we don't support it. + // TODO: We could look at this in the future, in particular, the bot_message subtype, which would + // allow us to react to messages sent by other bots. However, we'd have to be careful to + // filter appropriately, because otherwise we'll also react to messages from ourself. + // Because the 'help' output contains commands in it, that could look interesting! if ( 'undefined' !== typeof event.subtype ) { console.warn( 'Unsupported event subtype: ' + event.subtype ); return false; @@ -158,6 +222,8 @@ const handleEvent = ( event, request ) => { module.exports = { handleSelfPlus, handlePlusMinus, + sayThankyou, + sendHelp, handlers, handleEvent }; From 57106673fbeeeef9307ef96bc2eb8851cf1ac876 Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 15:51:04 +1000 Subject: [PATCH 38/39] Add simple default message on receiving an unknown command --- src/events.js | 7 ++++++- tests/events.js | 9 --------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/events.js b/src/events.js index 94623ff..736da4b 100644 --- a/src/events.js +++ b/src/events.js @@ -168,7 +168,12 @@ const handlers = { return appCommandHandlers[appCommand]( event, request ); } - return false; + const defaultMessage = ( + 'Sorry, I\'m not quite sure what you\'re asking me. I\'m not very smart - there\'s only a ' + + 'few things I\'ve been trained to do. Send me `help` for more details.' + ); + + return slack.sendMessage( defaultMessage, event.channel ); } // AppMention event. }; // Handlers. diff --git a/tests/events.js b/tests/events.js index 469416d..8c3b64a 100644 --- a/tests/events.js +++ b/tests/events.js @@ -197,15 +197,6 @@ describe( 'handlers.appMention', () => { }); - it( 'returns false if a supported command cannot be found', () => { - const event = { - type: eventType, - text: '<@U00000000> some_command_that_should_not_exist' - }; - - expect( handlers.appMention( event ) ).toBeFalse(); - }); - }); // Handlers.appMention. describe( 'handleEvent', () => { From 94d43ba435c8ecc1d353c2e2d271af831681bd3d Mon Sep 17 00:00:00 2001 From: Tim Malone Date: Sun, 19 Aug 2018 16:20:26 +1000 Subject: [PATCH 39/39] Use the Slack bot's configured name for the web template title --- src/app.js | 2 +- src/events.js | 6 +++--- src/helpers.js | 30 ++++++++++++++++++++++++++---- src/leaderboard.js | 29 ++++++++++++++++++++--------- tests/leaderboard.js | 7 ++++++- 5 files changed, 56 insertions(+), 18 deletions(-) diff --git a/src/app.js b/src/app.js index cc227f7..da0d288 100644 --- a/src/app.js +++ b/src/app.js @@ -89,7 +89,7 @@ const handleGet = async( request, response ) => { // TODO: This should probably be split out into a separate function of sorts, like handlePost. case '/leaderboard': if ( helpers.isTimeBasedTokenStillValid( request.query.token, request.query.ts ) ) { - response.send( await leaderboard.getForWeb() ); + response.send( await leaderboard.getForWeb( request ) ); } else { response .status( HTTP_403 ) diff --git a/src/events.js b/src/events.js index 736da4b..1611268 100644 --- a/src/events.js +++ b/src/events.js @@ -88,14 +88,14 @@ const sayThankyou = ( event ) => { */ const sendHelp = ( event ) => { - const botUserId = event.text.match( /<@(U[A-Z0-9]{8})>/ )[1]; + const botUserID = helpers.extractUserID( event.text ); const message = ( 'Sure, here\'s what I can do:\n\n' + '• `@Someone++`: Add points to a user or a thing\n' + '• `@Someone--`: Subtract points from a user or a thing\n' + - '• `<@' + botUserId + '> leaderboard`: Display the leaderboard\n' + - '• `<@' + botUserId + '> help`: Display this message\n\n' + + '• `<@' + botUserID + '> leaderboard`: Display the leaderboard\n' + + '• `<@' + botUserID + '> help`: Display this message\n\n' + 'You\'ll need to invite me to a channel before I can recognise ' + '`++` and `--` commands in it.\n\n' + 'If you\'re a developer, you can teach me new things! ' + diff --git a/src/helpers.js b/src/helpers.js index f788ce7..b5dfeed 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -6,6 +6,8 @@ 'use strict'; +const slack = require( './slack' ); + const fs = require( 'fs' ), crypto = require( 'crypto' ), handlebars = require( 'handlebars' ); @@ -70,6 +72,19 @@ const extractPlusMinusEventData = ( text ) => { operation: data[2].substring( 0, 1 ).replace( '—', '-' ) }; +}; // ExtractPlusMinusEventData. + +/** + * Extracts a valid Slack user ID from a string of text. + * + * @param {string} text The string in question. + * @returns {string} The first matched Slack user ID in the string, or an empty string if a match + * could not be found. + * @see ::isUser + */ +const extractUserID = ( text ) => { + const match = text.match( /U[A-Z0-9]{8}/ ); + return match ? match[0] : ''; }; /** @@ -144,6 +159,7 @@ const isTimeBasedTokenStillValid = ( token, ts ) => { * * @param {string} item The string in question. * @returns {Boolean} Whether or not the string is a Slack user ID. + * @see ::extractUserID() */ const isUser = ( item ) => { return item.match( /U[A-Z0-9]{8}/ ) ? true : false; @@ -173,10 +189,13 @@ const maybeLinkItem = ( item ) => { * standard variables referenced in the header and footer, such as * 'title'. See the contents of ./html/ for more details. Some * variables may have defaults provided, which can be overridden. + * @param {object} request Optional. The Express request object that resulted in this + * rendering job being run. Can be used to provide additional context + * to the templates. * @returns {string} HTML ready to be rendered in the browser. * @see https://handlebarsjs.com/ */ -const render = ( templatePath = '', context = {}) => { +const render = async( templatePath, context = {}, request = {}) => { // Retrieve the header and footer HTML, if we don't already have it in memory. if ( ! templates.header ) templates.header = fs.readFileSync( 'src/html/header.html', 'utf8' ); @@ -190,9 +209,11 @@ const render = ( templatePath = '', context = {}) => { /* eslint-disable camelcase */ // Handlebars templates commonly use snake_case instead. const defaults = { - - // TODO: Get this from bot's name settings in the Slack users list. - site_title: 'PlusPlus++ That Works' + site_title: ( + request.query.botUser ? + await slack.getUserName( request.query.botUser ) : + 'Working PlusPlus++' + ) }; /* eslint-enable camelcase */ @@ -204,6 +225,7 @@ const render = ( templatePath = '', context = {}) => { module.exports = { extractCommand, extractPlusMinusEventData, + extractUserID, getTimeBasedToken, getTimestamp, isPlural, diff --git a/src/leaderboard.js b/src/leaderboard.js index 97550f1..0389360 100644 --- a/src/leaderboard.js +++ b/src/leaderboard.js @@ -10,20 +10,30 @@ const slack = require( './slack' ), points = require( './points' ), helpers = require( './helpers' ); +const querystring = require( 'querystring' ); + /** * Gets the URL for the full leaderboard, including a token to ensure that it is only viewed by * someone who has access to this Slack team. * - * @param {string} hostname The hostname this app is being served on. + * @param {object} request The Express request object that resulted in this handler being run. * @returns {string} The leaderboard URL, which will be picked up in ../index.js when called. */ -const getLeaderboardUrl = ( hostname ) => { - const ts = helpers.getTimestamp(), - token = helpers.getTimeBasedToken( ts ), - url = 'https://' + hostname + '/leaderboard?token=' + token + '&ts=' + ts; +const getLeaderboardUrl = ( request ) => { + + const hostname = request.headers.host, + ts = helpers.getTimestamp(); + + const params = { + token: helpers.getTimeBasedToken( ts ), + ts, + botUser: helpers.extractUserID( request.body.event.text ) + }; + const url = 'https://' + hostname + '/leaderboard?' + querystring.stringify( params ); return url; -}; + +}; // GetLeaderboardUrl. /** * Ranks items by their scores, returning them in a human readable list complete with emoji for the @@ -127,7 +137,7 @@ const getForSlack = async( event, request ) => { const messageText = ( 'Here you go. ' + - 'Or see the <' + getLeaderboardUrl( request.headers.host ) + '|whole list>.' + 'Or see the <' + getLeaderboardUrl( request ) + '|whole list>.' ); const message = { @@ -159,9 +169,10 @@ const getForSlack = async( event, request ) => { /** * Retrieves and returns HTML for the full leaderboard, for displaying on the web. * + * @param {object} request The Express request object that resulted in this handler being run. * @returns {string} HTML for the browser. */ -const getForWeb = async() => { +const getForWeb = async( request ) => { const scores = await points.retrieveTopScores(), users = await rankItems( scores, 'users', 'object' ), @@ -173,7 +184,7 @@ const getForWeb = async() => { title: 'Leaderboard' }; - return helpers.render( 'src/html/leaderboard.html', data ); + return helpers.render( 'src/html/leaderboard.html', data, request ); }; // GetForWeb. diff --git a/tests/leaderboard.js b/tests/leaderboard.js index 05d9fd8..763c629 100644 --- a/tests/leaderboard.js +++ b/tests/leaderboard.js @@ -17,7 +17,12 @@ describe( 'getLeaderboardUrl', () => { const MILLISECONDS_TO_SECONDS = 1000; - const leaderboardUrl = leaderboard.getLeaderboardUrl( 'example.com' ), + const request = { + headers: { host: 'test.local' }, + body: { event: { text: '<@U00000000> test' } } + }; + + const leaderboardUrl = leaderboard.getLeaderboardUrl( request ), parsedUrl = new URL( leaderboardUrl ), token = parsedUrl.searchParams.get( 'token' ), ts = parsedUrl.searchParams.get( 'ts' ),