Merge pull request #236 from TK95/fix-member_joined/left_channel-even…

…t-not-firing-for-own-bot

Fixed the issue with member joined\left channel event not firing for own bot

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,13 +261,180 @@ 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 {
   matches?: RegExpExecArray;
 }
 
 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 = {},

src/middleware/builtin.ts

@@ -118,8 +118,8 @@ export function matchCallbackId(
  * Middleware that checks for matches given constraints
  */
 export function matchConstraints(
-    constraints: ActionConstraints,
-  ): Middleware<SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs> {
+  constraints: ActionConstraints,
+): Middleware<SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs> {
   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)
@@ -275,7 +275,14 @@ export function ignoreSelf(): Middleware<AnyMiddlewareArgs> {
       }
 
       // 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 eventsWhichShouldBeKept = [
+        'member_joined_channel',
+        'member_left_channel',
+      ];
+      const isEventShouldBeKept = eventsWhichShouldBeKept.includes(args.event.type);
+
+      if (botUserId !== undefined && args.event.user === botUserId && !isEventShouldBeKept) {
         return;
       }
     }