Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #874: Enable using BoltJS without passing a botId #1087

Merged
merged 3 commits into from
Aug 28, 2021

Conversation

misscoded
Copy link
Contributor

Summary

Fixes #874

Currently, when using an app that only makes use of a user token, the ignoreSelf middleware will throw an error because it expects a botId to always be present.

This PR removes the error thrown (and corresponding test) if a botId is not present in the incoming event's payload.

Requirements (place an x in each [ ])

@misscoded misscoded added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch labels Aug 26, 2021
@misscoded misscoded added this to the 3.7.0 milestone Aug 26, 2021
@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #1087 (3141cce) into main (926b669) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1087      +/-   ##
==========================================
- Coverage   71.01%   70.96%   -0.05%     
==========================================
  Files          13       13              
  Lines        1235     1233       -2     
  Branches      362      361       -1     
==========================================
- Hits          877      875       -2     
  Misses        288      288              
  Partials       70       70              
Impacted Files Coverage Δ
src/middleware/builtin.ts 83.84% <ø> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 926b669...3141cce. Read the comment docs.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Perhaps, you've already verified that App initialized with a user token now works fine but if not, we can manually check the pattern before merging this PR. If we can add some tests for the pattern, that's a great addition! but it's not a must this time.

@@ -291,37 +291,6 @@ describe('directMention()', () => {
});

describe('ignoreSelf()', () => {
it('should handle context missing error', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can safely remove this one. Also, as the other tests cover all the patterns in this test group, we don't need to add any tests this time.

@seratch
Copy link
Member

seratch commented Aug 27, 2021

This one is ready for merge but let's hold off merging until we merge #1024

@misscoded
Copy link
Contributor Author

If we can add some tests for the pattern

@seratch Good call. When pulling in the latest changes, I revisited and added a test case for the pattern of only passing in a botUserId. I believe the existing test cases cover the other possible scenarios, but if there are others that I've overlooked that you think would be useful, let me know -- happy to add them!

@seratch
Copy link
Member

seratch commented Aug 28, 2021

@misscoded Thanks, the test also looks good to me 👍

@seratch seratch merged commit 8326547 into slackapi:main Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable using bolt-js without passing bot_id
4 participants