diff --git a/.travis-postgres10.sh b/.travis-postgres10.sh deleted file mode 100755 index bca31aa..0000000 --- a/.travis-postgres10.sh +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/env bash -# -# Installs PostgreSQL 10 on Travis CI, as the default build images don't support it yet. -# -# @see https://github.com/travis-ci/travis-ci/issues/8537#issuecomment-354020356 - -set -euxo pipefail - -echo "Installing Postgres 10..." -sudo service postgresql stop -sudo apt-get remove --quiet 'postgresql-*' -sudo apt-get update --quiet -sudo apt-get install --quiet postgresql-10 postgresql-client-10 -sudo cp /etc/postgresql/{9.6,10}/main/pg_hba.conf - -echo "Restarting Postgres 10..." -sudo service postgresql restart - -echo "Adding Travis role..." -sudo psql --command="CREATE ROLE travis SUPERUSER LOGIN CREATEDB;" --username="postgres" diff --git a/.travis.yml b/.travis.yml index e96d681..4b04d29 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,7 +16,11 @@ services: # @see https://github.com/travis-ci/travis-ci/issues/8537#issuecomment-354020356 sudo: required addons: - postgresql: 9.6 + postgresql: "10" + apt: + packages: + - postgresql-10 + - postgresql-client-10 cache: yarn: true @@ -29,9 +33,6 @@ before_install: - curl --location https://yarnpkg.com/install.sh | bash -s -- --version "$( node -e "console.log(require('./package.json').engines.yarn)" )" - export PATH="$HOME/.yarn/bin:$PATH" - # Upgrade to PostgreSQL 10. - - ./.travis-postgres10.sh - install: - yarn --frozen-lockfile diff --git a/src/events.js b/src/events.js index 1611268..fcc487b 100644 --- a/src/events.js +++ b/src/events.js @@ -17,37 +17,36 @@ const slack = require( './slack' ), const camelCase = require( 'lodash.camelcase' ); /** + * Handles a plus or minus against a user, and then notifies the channel of the new score. * 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 ); - return slack.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} mentions Array of parsed objects from a Slack message with objects containing + * the Slack user ID (if user) or name (if thing) of the item being + * operated on and the +/- operation performed on the item's score. + * @param {object} user The ID of the user (Uxxxxxxxx) who sent the message. * @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 ), - operationName = operations.getOperationName( operation ), - message = messages.getRandomMessage( operationName, item, score ); +const handlePlusMinus = async( mentions, user, channel ) => { + + const messageLines = []; + for ( const mention of mentions ) { + + // Handle self plus as an event avoiding incrementing the score + if ( mention.item === user && '+' === mention.operation ) { + console.log( user + ' tried to alter their own score.' ); + messageLines.push( messages.getRandomMessage( operations.operations.SELF, user ) ); + } else { + const score = await points.updateScore( mention.item, mention.operation ), + operationName = operations.getOperationName( mention.operation ); + messageLines.push( messages.getRandomMessage( operationName, mention.item, score ) ); + } + } - return slack.sendMessage( message, channel ); + return slack.sendMessage( messageLines.join( '\n' ), channel ); }; /** @@ -122,20 +121,14 @@ const handlers = { message: ( event ) => { // Extract the relevant data from the message text. - const { item, operation } = helpers.extractPlusMinusEventData( event.text ); - - if ( ! item || ! operation ) { - return false; - } + const mentions = helpers.extractPlusMinusEventData( event.text ); - // Bail if the user is trying to ++ themselves... - if ( item === event.user && '+' === operation ) { - handleSelfPlus( event.user, event.channel ); + if ( ! mentions ) { return false; } // Otherwise, let's go! - return handlePlusMinus( item, operation, event.channel ); + return handlePlusMinus( mentions, event.user, event.channel ); }, // Message event. @@ -225,7 +218,6 @@ const handleEvent = ( event, request ) => { }; // HandleEvent. module.exports = { - handleSelfPlus, handlePlusMinus, sayThankyou, sendHelp, diff --git a/src/helpers.js b/src/helpers.js index b5dfeed..c0ca0c6 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -55,22 +55,32 @@ const extractCommand = ( message, commands ) => { * 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 -). + * @returns {object} An array of objects 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 extractPlusMinusEventData = ( text ) => { - const data = text.match( /@([A-Za-z0-9]+?)>?\s*(\+{2}|-{2}|—{1})/ ); + const matchAll = /@([A-Za-z0-9]+?)>?\s*(\+{2}|-{2}|—{1})/g; + const matchOne = /@([A-Za-z0-9]+?)>?\s*(\+{2}|-{2}|—{1})/; - if ( ! data ) { + const matches = text.match( matchAll ); + + if ( null === matches ) { return false; } - return { - item: data[1], - operation: data[2].substring( 0, 1 ).replace( '—', '-' ) - }; + const data = []; + for ( const match of matches ) { + const parts = match.match( matchOne ); + data.push( + { + item: parts[1], + operation: parts[2].substring( 0, 1 ).replace( '—', '-' ) + } + ); + } + return data; }; // ExtractPlusMinusEventData. diff --git a/src/points.js b/src/points.js index abc201b..e4c4071 100644 --- a/src/points.js +++ b/src/points.js @@ -73,17 +73,21 @@ const updateScore = async( item, operation ) => { ' ); // 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; \ - ' ); + const insertOrUpdateScore = { + text: '\ + INSERT INTO ' + scoresTableName + ' VALUES ($1, $2) \ + ON CONFLICT (item) DO UPDATE SET score = ' + scoresTableName + '.score ' + operation + ' 1; \ + ', + values: [ item, operation + '1' ] + }; + await dbClient.query( insertOrUpdateScore ); // 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 + '\'; \ - ' ); + const getCurrentScore = { + text: 'SELECT score FROM ' + scoresTableName + ' WHERE item = $1;', + values: [ item ] + }; + const dbSelect = await dbClient.query( getCurrentScore ); await dbClient.release(); const score = dbSelect.rows[0].score; diff --git a/tests/events.js b/tests/events.js index 8c3b64a..2eb573b 100644 --- a/tests/events.js +++ b/tests/events.js @@ -37,18 +37,38 @@ beforeEach( () => { slack.sendMessage.mockClear(); }); -describe( 'handleSelfPlus', () => { +describe( 'handlePlusMinus', () => { const user = 'U12345678', - channel = 'C12345678'; + item = 'SomeRandomThing', + item2 = 'SomeOtherThing', + channel = 'C12345678', + score = 5; + + /** @returns {integer} Returns a fake score. */ + const updateScoreMock = () => { + return score; + }; it( 'logs an attempt by a user to increment their own score', () => { - events.handleSelfPlus( user, channel ); + const mentions = [ + { + item: user, + operation: '+' + } + ]; + events.handlePlusMinus( mentions, user, channel ); expect( console.log ).toHaveBeenCalledTimes( 1 ); }); it( 'gets a message from the \'self plus\' collection', () => { - events.handleSelfPlus( user, channel ); + const mentions = [ + { + item: user, + operation: '+' + } + ]; + events.handlePlusMinus( mentions, user, channel ); expect( messages.getRandomMessage ) .toHaveBeenCalledTimes( 1 ) @@ -62,26 +82,19 @@ describe( 'handleSelfPlus', () => { slack.sendMessage = jest.fn(); slack.setSlackClient( slackClientMock ); - events.handleSelfPlus( user, channel ); + const mentions = [ + { + item: user, + operation: '+' + } + ]; + events.handlePlusMinus( mentions, user, channel ); expect( slack.sendMessage ) .toHaveBeenCalledTimes( 1 ) .toHaveBeenCalledWith( expect.stringContaining( user ), channel ); }); -}); - -describe( 'handlePlusMinus', () => { - - 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 slack = require( '../src/slack' ), points = require( '../src/points' ), @@ -90,11 +103,24 @@ describe( 'handlePlusMinus', () => { slack.setSlackClient( slackClientMock ); points.updateScore = jest.fn(); - events.handlePlusMinus( item, '+', channel ); + const operation = '+'; + const mentions = [ + { + item, + operation + }, + { + item: item2, + operation + } + ]; + events.handlePlusMinus( mentions, user, channel ).then( () => { + expect( points.updateScore ) + .toHaveBeenCalledTimes( 2 ) + .toHaveBeenNthCalledWith( 1, item, '+' ) + .toHaveBeenNthCalledWith( 2, item2, '+' ); + }); - expect( points.updateScore ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( item, '+' ); }); it.each([ [ 'plus', '+' ], [ 'minus', '-' ] ])( @@ -111,10 +137,21 @@ describe( 'handlePlusMinus', () => { points.updateScore = jest.fn( updateScoreMock ); messages.getRandomMessage = jest.fn(); - return events.handlePlusMinus( item, operation, channel ).then( () => { + const mentions = [ + { + item, + operation + }, + { + item: item2, + operation + } + ]; + return events.handlePlusMinus( mentions, user, channel ).then( () => { expect( messages.getRandomMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( operationName, item, score ); + .toHaveBeenCalledTimes( 2 ) + .toHaveBeenNthCalledWith( 1, operationName, item, score ) + .toHaveBeenNthCalledWith( 2, operationName, item2, score ); }); } ); @@ -130,7 +167,17 @@ describe( 'handlePlusMinus', () => { points.updateScore = jest.fn(); slack.sendMessage = jest.fn(); - return events.handlePlusMinus( item, '+', channel ).then( () => { + const mentions = [ + { + item, + operation: '+' + }, + { + item: item2, + operation: '+' + } + ]; + return events.handlePlusMinus( mentions, user, channel ).then( () => { expect( slack.sendMessage ) .toHaveBeenCalledTimes( 1 ) .toHaveBeenCalledWith( expect.stringContaining( item ), channel ); @@ -161,16 +208,6 @@ describe( 'handlers.message', () => { expect( handlers.message( event ) ).toBeFalse(); }); - it( 'returns false if a user trying to ++ themselves', () => { - const event = { - type: eventType, - text: '<@U12345678>++', - user: 'U12345678' - }; - - expect( handlers.message( event ) ).toBeFalse(); - }); - }); // HandleMessageEvent. describe( 'handlers.appMention', () => { diff --git a/tests/helpers.js b/tests/helpers.js index bc50cb3..6c27661 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -74,31 +74,53 @@ describe( 'extractPlusMinusEventData', () => { }); it( 'extracts a \'thing\' and operation from the start of a message', () => { - expect( helpers.extractPlusMinusEventData( '@SomethingRandom++ that was awesome' ) ).toEqual({ - item: 'SomethingRandom', - operation: '+' - }); + 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.extractPlusMinusEventData( '<@U87654321>++ that was awesome' ) ).toEqual({ - item: 'U87654321', - operation: '+' - }); + expect( helpers.extractPlusMinusEventData( '<@U87654321>++ that was awesome' ) ).toEqual([ + { + item: 'U87654321', + operation: '+' + } + ]); }); it( 'extracts data in the middle of a message', () => { - expect( helpers.extractPlusMinusEventData( 'Hey @SomethingRandom++ you\'re great' ) ).toEqual({ - item: 'SomethingRandom', - operation: '+' - }); + expect( helpers.extractPlusMinusEventData( 'Hey @SomethingRandom++ you\'re great' ) ).toEqual([ + { + item: 'SomethingRandom', + operation: '+' + } + ]); }); it( 'extracts data at the end of a message', () => { - expect( helpers.extractPlusMinusEventData( 'Awesome work @SomethingRandom++' ) ).toEqual({ - item: 'SomethingRandom', - operation: '+' - }); + expect( helpers.extractPlusMinusEventData( 'Awesome work @SomethingRandom++' ) ).toEqual([ + { + item: 'SomethingRandom', + operation: '+' + } + ]); + }); + + it( 'extracts multiple mentions in one message', () => { + const multiMentions = 'Thing one @SomethingRandom++ and thing two @SomethingElse--'; + expect( helpers.extractPlusMinusEventData( multiMentions ) ).toEqual([ + { + item: 'SomethingRandom', + operation: '+' + }, + { + item: 'SomethingElse', + operation: '-' + } + ]); }); const itemsToMatch = [ @@ -149,10 +171,12 @@ describe( 'extractPlusMinusEventData', () => { it( testName, () => { const result = helpers.extractPlusMinusEventData( messageText ); - expect( result ).toEqual({ - item: item.expected, - operation: operation.expected - }); + expect( result ).toEqual([ + { + item: item.expected, + operation: operation.expected + } + ]); }); } // For iterator. diff --git a/tests/integration-tests.js b/tests/integration-tests.js index 21524cc..52a617a 100644 --- a/tests/integration-tests.js +++ b/tests/integration-tests.js @@ -214,11 +214,12 @@ describe( 'The database', () => { it( 'creates the ' + config.scoresTableName + ' table on the first request', async() => { expect.hasAssertions(); - await points.updateScore( defaultItem, '++' ); + const newScore = await points.updateScore( defaultItem, '+' ); const dbClient = await postgres.connect(); const query = await dbClient.query( tableExistsQuery ); await dbClient.release(); expect( query.rows[0].exists ).toBeTrue(); + expect( newScore ).toBe( 1 ); }); it( 'also creates the case-insensitive extension on the first request', async() => { @@ -229,14 +230,11 @@ describe( 'The database', () => { expect( query.rowCount ).toBe( 1 ); }); - /* 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, '++' ); + expect.hasAssertions(); + const newScore = await points.updateScore( defaultItem, '+' ); + expect( newScore ).toBe( 2 ); }); - /* eslint-enable jest/expect-expect */ it( 'returns a list of top scores in the correct order', async() => { expect.hasAssertions(); @@ -253,9 +251,9 @@ describe( 'The database', () => { ]; // 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, '++' ); + await points.updateScore( defaultUser, '+' ); + await points.updateScore( defaultUser, '+' ); + await points.updateScore( defaultUser, '+' ); const topScores = await points.retrieveTopScores(); expect( topScores ).toEqual( expectedScores );