From 213ae4be595e5251f8f69c33e1d7f6bda4ce6e5e Mon Sep 17 00:00:00 2001 From: Ostapenko Denys Date: Mon, 12 Aug 2019 11:34:15 +0300 Subject: [PATCH 1/2] Fixed the issue with member joined\left channel event not firing for own bot. Added tests for ignoreSelf middleware --- src/middleware/builtin.spec.ts | 169 ++++++++++++++++++++++++++++++++- src/middleware/builtin.ts | 10 +- 2 files changed, 175 insertions(+), 4 deletions(-) diff --git a/src/middleware/builtin.spec.ts b/src/middleware/builtin.spec.ts index 5f4549bbd..d9a25595e 100644 --- a/src/middleware/builtin.spec.ts +++ b/src/middleware/builtin.spec.ts @@ -5,7 +5,7 @@ import sinon from 'sinon'; import { ErrorCode } from '../errors'; import { Override, delay, wrapToResolveOnFirstCall } from '../test-helpers'; import rewiremock from 'rewiremock'; -import { SlackEventMiddlewareArgs, NextMiddleware, Context, MessageEvent } from '../types'; +import { SlackEventMiddlewareArgs, NextMiddleware, Context, MessageEvent, ContextMissingPropertyError, SlackCommandMiddlewareArgs } from '../types'; describe('matchMessage()', () => { function initializeTestCase(pattern: string | RegExp): Mocha.AsyncFunc { @@ -261,6 +261,167 @@ describe('directMention()', () => { }); }); +describe('ignoreSelf()', () => { + it('should handle context missing error', async () => { + // Arrange + const fakeNext = sinon.fake(); + const fakeBotUserId = undefined; + const fakeArgs = { + next: fakeNext, + context: { botUserId: fakeBotUserId }, + } as unknown as MemberJoinedOrLeftChannelMiddlewareArgs; + + const { ignoreSelf: getIgnoreSelfMiddleware, contextMissingPropertyError } = await importBuiltin(); + + // Act + const middleware = getIgnoreSelfMiddleware(); + middleware(fakeArgs); + await delay(); + + // Assert + const expectedError = contextMissingPropertyError( + 'botId', + 'Cannot ignore events from the app without a bot ID. Ensure authorize callback returns a botId.', + ); + + const contextMissingError: ContextMissingPropertyError = fakeNext.getCall(0).lastArg; + + assert.equal(contextMissingError.code, expectedError.code); + assert.equal(contextMissingError.missingProperty, expectedError.missingProperty); + }); + + it('should immediately call next(), because incoming middleware args don\'t contain event', async () => { + // Arrange + const fakeNext = sinon.fake(); + const fakeBotUserId = 'BUSER1'; + const fakeArgs = { + next: fakeNext, + context: { botUserId: fakeBotUserId }, + command: { + command: '/fakeCommand', + }, + } as unknown as CommandMiddlewareArgs; + + const { ignoreSelf: getIgnoreSelfMiddleware } = await importBuiltin(); + + // Act + const middleware = getIgnoreSelfMiddleware(); + middleware(fakeArgs); + await delay(); + + // Assert + assert(fakeNext.calledOnce); + }); + + it('should look for an event identified as a bot message from the same bot ID as this app and skip it', async () => { + // Arrange + const fakeNext = sinon.fake(); + const fakeBotUserId = 'BUSER1'; + // TODO: Fix typings here + const fakeArgs = { + next: fakeNext, + context: { botUserId: fakeBotUserId, botId: fakeBotUserId }, + event: { + type: 'message', + subtype: 'bot_message', + bot_id: fakeBotUserId, + }, + message: { + type: 'message', + subtype: 'bot_message', + bot_id: fakeBotUserId, + }, + } as any; + + const { ignoreSelf: getIgnoreSelfMiddleware } = await importBuiltin(); + + // Act + const middleware = getIgnoreSelfMiddleware(); + middleware(fakeArgs); + await delay(); + + // Assert + assert(fakeNext.notCalled); + }); + + it('should filter an event out, because it matches our own app and shouldn\'t be retained', async () => { + // Arrange + const fakeNext = sinon.fake(); + const fakeBotUserId = 'BUSER1'; + const fakeArgs = { + next: fakeNext, + context: { botUserId: fakeBotUserId, botId: fakeBotUserId }, + event: { + type: 'tokens_revoked', + user: fakeBotUserId, + }, + } as unknown as TokensRevokedMiddlewareArgs; + + const { ignoreSelf: getIgnoreSelfMiddleware } = await importBuiltin(); + + // Act + const middleware = getIgnoreSelfMiddleware(); + middleware(fakeArgs); + await delay(); + + // Assert + assert(fakeNext.notCalled); + }); + + it('should filter an event out, because it matches our own app and shouldn\'t be retained', async () => { + // Arrange + const fakeNext = sinon.fake(); + const fakeBotUserId = 'BUSER1'; + const fakeArgs = { + next: fakeNext, + context: { botUserId: fakeBotUserId, botId: fakeBotUserId }, + event: { + type: 'tokens_revoked', + user: fakeBotUserId, + }, + } as unknown as TokensRevokedMiddlewareArgs; + + const { ignoreSelf: getIgnoreSelfMiddleware } = await importBuiltin(); + + // Act + const middleware = getIgnoreSelfMiddleware(); + middleware(fakeArgs); + await delay(); + + // Assert + assert(fakeNext.notCalled); + }); + + it('shouldn\'t filter an event out, because it should be retained', async () => { + // Arrange + const fakeNext = sinon.fake(); + const fakeBotUserId = 'BUSER1'; + const eventsWhichShouldNotBeFilteredOut = ['member_joined_channel', 'member_left_channel']; + + const listOfFakeArgs = eventsWhichShouldNotBeFilteredOut.map((eventType) => { + return { + next: fakeNext, + context: { botUserId: fakeBotUserId, botId: fakeBotUserId }, + event: { + type: eventType, + user: fakeBotUserId, + }, + } as unknown as MemberJoinedOrLeftChannelMiddlewareArgs; + }); + + const { ignoreSelf: getIgnoreSelfMiddleware } = await importBuiltin(); + + // Act + const middleware = getIgnoreSelfMiddleware(); + listOfFakeArgs.forEach(middleware); + + await delay(); + + // Assert + assert.equal(fakeNext.callCount, listOfFakeArgs.length); + }); +}); + /* Testing Harness */ interface DummyContext { @@ -268,6 +429,12 @@ interface DummyContext { } type MessageMiddlewareArgs = SlackEventMiddlewareArgs<'message'> & { next: NextMiddleware, context: Context }; +type TokensRevokedMiddlewareArgs = SlackEventMiddlewareArgs<'tokens_revoked'> & { next: NextMiddleware, context: Context }; + +type MemberJoinedOrLeftChannelMiddlewareArgs = SlackEventMiddlewareArgs<'member_joined_channel' | 'member_left_channel'> + & { next: NextMiddleware, context: Context }; + +type CommandMiddlewareArgs = SlackCommandMiddlewareArgs & { next: NextMiddleware; context: Context }; async function importBuiltin( overrides: Override = {}, diff --git a/src/middleware/builtin.ts b/src/middleware/builtin.ts index 0a6d90f3c..b0feef84e 100644 --- a/src/middleware/builtin.ts +++ b/src/middleware/builtin.ts @@ -74,8 +74,8 @@ export const onlyEvents: Middleware * Middleware that checks for matches given constraints */ export function matchConstraints( - constraints: ActionConstraints, - ): Middleware { + constraints: ActionConstraints, +): Middleware { return ({ payload, body, next, context }) => { // TODO: is putting matches in an array actually helpful? there's no way to know which of the regexps contributed // which matches (and in which order) @@ -231,7 +231,11 @@ export function ignoreSelf(): Middleware { } // Its an Events API event that isn't of type message, but the user ID might match our own app. Filter these out. - if (botUserId !== undefined && args.event.user === botUserId) { + // However, some events still must be fired, because they can make sense. + const eventsWhichShouldNotBeFilteredOut = ['member_joined_channel', 'member_left_channel']; + const isEventShouldBeSkipped = !eventsWhichShouldNotBeFilteredOut.includes(args.event.type); + + if (botUserId !== undefined && args.event.user === botUserId && isEventShouldBeSkipped) { return; } } From c91d671e5ca0a2e2bc34998026511752fbf9e4ca Mon Sep 17 00:00:00 2001 From: Ostapenko Denys Date: Wed, 25 Sep 2019 12:13:48 +0300 Subject: [PATCH 2/2] Refactored 'ignoreSelf' middleware to make it more readable --- src/middleware/builtin.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/middleware/builtin.ts b/src/middleware/builtin.ts index b0feef84e..cc9aa58ae 100644 --- a/src/middleware/builtin.ts +++ b/src/middleware/builtin.ts @@ -232,10 +232,13 @@ export function ignoreSelf(): Middleware { // Its an Events API event that isn't of type message, but the user ID might match our own app. Filter these out. // However, some events still must be fired, because they can make sense. - const eventsWhichShouldNotBeFilteredOut = ['member_joined_channel', 'member_left_channel']; - const isEventShouldBeSkipped = !eventsWhichShouldNotBeFilteredOut.includes(args.event.type); + const eventsWhichShouldBeKept = [ + 'member_joined_channel', + 'member_left_channel', + ]; + const isEventShouldBeKept = eventsWhichShouldBeKept.includes(args.event.type); - if (botUserId !== undefined && args.event.user === botUserId && isEventShouldBeSkipped) { + if (botUserId !== undefined && args.event.user === botUserId && !isEventShouldBeKept) { return; } }