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

#999 Adds Slack Connect API event types #1008

Merged
merged 18 commits into from
Aug 10, 2021

Conversation

srajiang
Copy link
Member

@srajiang srajiang commented Jul 16, 2021

Summary

Fixes #999 by adding event types for the Slack Connect APIs.

For reviewers: I wanted to take note of the fact that as of writing, the event type signatures listed on the api documentation site for the above are incorrect and need to be fixed (it's been noted internally).

So I based these types on 1) internal examples of event payloads 2) technical spec and 3) confirmation from BE-engineer maintaining these endpoints. If reviewers think that it'd be best to merge these changes only after public docs are corrected, we can also hold off on merging.

Reference types:

Requirements (place an x in each [ ])

@srajiang srajiang added the enhancement M-T: A feature request for new functionality label Jul 16, 2021
@srajiang srajiang added this to the 3.6.0 milestone Jul 16, 2021
@srajiang srajiang requested a review from stevengill July 16, 2021 01:42
@srajiang srajiang self-assigned this Jul 16, 2021
@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #1008 (84b2678) into main (4fff0f7) will not change coverage.
The diff coverage is n/a.

❗ Current head 84b2678 differs from pull request most recent head b5c01b4. Consider uploading reports for the commit b5c01b4 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1008   +/-   ##
=======================================
  Coverage   68.89%   68.89%           
=======================================
  Files          13       13           
  Lines        1212     1212           
  Branches      357      357           
=======================================
  Hits          835      835           
  Misses        304      304           
  Partials       73       73           

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 5202107...b5c01b4. Read the comment docs.

@srajiang srajiang requested a review from filmaj August 3, 2021 21:01
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

If reviewers think that it'd be best to merge these changes only after public docs are corrected, we can also hold off on merging.

This seems prudent to me - being able to reference something public in the PR that you based the work off of draws a nice line from the source to your changes. That said, I'm new on the team, so I am not familiar with how things are done 'round these parts yet, and I defer to what the rest of the team usually does 😄

@srajiang
Copy link
Member Author

srajiang commented Aug 3, 2021

@stevengill @filmaj's line of thinking I am following and liking, but defer to you on this in terms of waiting to merge these vs. prodding internally to get these corrected before finalizing these changes!

@srajiang
Copy link
Member Author

srajiang commented Aug 9, 2021

@stevengill - Types have been updated in api.slack.com, and I've updated this PR with the changes. Should be finally ready for review!

Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

LGTM!

@srajiang srajiang merged commit a2324c6 into slackapi:main Aug 10, 2021
@srajiang srajiang deleted the sj-add-slack-connect-event-types branch August 10, 2021 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new slack connect events
3 participants