Fixed the issue with member joined\left channel event not firing for own bot #236
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…own bot. Added tests for ignoreSelf middleware
TK95
commented
Aug 12, 2019
| subtype: 'bot_message', | ||
| bot_id: fakeBotUserId, | ||
| }, | ||
| } as any; |
There was a problem hiding this comment.
Unfortunately, I wasn't able to provide the correct type here :(
If someone could help, it would be great!
Codecov Report
@@ Coverage Diff @@
## master #236 +/- ##
==========================================
+ Coverage 59.95% 61.94% +1.98%
==========================================
Files 7 7
Lines 492 494 +2
Branches 136 136
==========================================
+ Hits 295 306 +11
+ Misses 169 163 -6
+ Partials 28 25 -3
Continue to review full report at Codecov.
|
…iring-for-own-bot
ikenami
reviewed
Sep 25, 2019
src/middleware/builtin.ts
Outdated
| @@ -231,7 +231,11 @@ 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 eventsWhichShouldNotBeFilteredOut = ['member_joined_channel', 'member_left_channel']; | |||
There was a problem hiding this comment.
Suggested change
| const eventsWhichShouldNotBeFilteredOut = ['member_joined_channel', 'member_left_channel']; | |
| const eventsWhichShouldBeKept = [ | |
| 'member_joined_channel', | |
| 'member_left_channel', | |
| ]; |
ikenami
reviewed
Sep 25, 2019
src/middleware/builtin.ts
Outdated
| 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); |
There was a problem hiding this comment.
Suggested change
| const isEventShouldBeSkipped = !eventsWhichShouldNotBeFilteredOut.includes(args.event.type); | |
| const isEventShouldBeSkipped = !eventsWhichShouldBeKept.includes(args.event.type); |
I didn't like the double negative 🤔 this might be more readable...
shaydewael
approved these changes
Oct 1, 2019
…iring-for-own-bot
seratch
approved these changes
Oct 2, 2019
added 3 commits
October 3, 2019 10:11
…iring-for-own-bot
…iring-for-own-bot
…iring-for-own-bot
aoberoi
reviewed
Oct 4, 2019
| assert(fakeNext.notCalled); | ||
| }); | ||
|
|
||
| it('should filter an event out, because it matches our own app and shouldn\'t be retained', async () => { |
There was a problem hiding this comment.
is this the same test as above?
|
i just want to say this PR is super awesome! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes issue #225 related to
member_joined_channel(and alsomember_left_channel) events which are not fired for own bot.Plus, I covered with unit tests
ignoreSelf()middleware from https://github.com/slackapi/bolt/blob/522e70b381cf3d18a88b7ca271dcfb4f0ce1be9b/src/middleware/builtin.ts#L205Requirements (place an
xin each[ ])