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

Add endpoints for calls.* apis and Type: call in blockkit #1190

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

winston-stripe
Copy link
Contributor

Implement the API methods for the Calls API in Slack https://api.slack.com/apis/calls

Implemented methods

  • calls.add - Indicate a new call has been started
  • calls.end - Indicate to slack that the call has ended
  • calls.info - Get information about an ongoing slack call object
  • calls.update - update call information
  • calls.participants.add
  • calls.participants.remove

Additionally, I've added the minimal version of Block{Type: "call", CallID: string} which slack recommends/requires be posted back to the channel https://api.slack.com/apis/calls#post_to_channel.

All implemented functionality is publicly documented. There appear to be additional attributes on the type: call block, however those appear to be internal values for slack's rendering, so I have left them out. See this gist for specific responses https://gist.github.com/winston-stripe/0cac608bd63b42d73a352be53577f7fd

Pull Request Guidelines

These are recommendations for pull requests.
They are strictly guidelines to help manage expectations.

PR preparation

Run make pr-prep from the root of the repository to run formatting, linting and tests.

Should this be an issue instead
  • is it a convenience method? (no new functionality, streamlines some use case)
  • exposes a previously private type, const, method, etc.
  • is it application specific (caching, retry logic, rate limiting, etc)
  • is it performance related.
API changes

Since API changes have to be maintained they undergo a more detailed review and are more likely to require changes.

  • no tests, if you're adding to the API include at least a single test of the happy case.
  • If you can accomplish your goal without changing the API, then do so.
  • dependency changes. updates are okay. adding/removing need justification.
Examples of API changes that do not meet guidelines:
  • in library cache for users. caches are use case specific.
  • Convenience methods for Sending Messages, update, post, ephemeral, etc. consider opening an issue instead.

@winston-stripe
Copy link
Contributor Author

Oddly, I have no idea why the lint step is failing. Downloaded and ran golangci-lint run locally and everything ran without a hitch. Maybe some weird bad caching or some unexpected actions interaction? Fwiw, when looking through their docs, I did notice that

v3.0.0+ requires explicit setup-go installation step prior to using this action: uses: actions/setup-go@v3

Still no idea otherwise esp since master is passing, i'm fully based off of master, and all failing files are other ones I didn't touch here 😕.

@winston-stripe
Copy link
Contributor Author

Thanks @kanata2 for fixing the repo's linting check in #1194! rebased off master and everything is passing now

@winston-stripe
Copy link
Contributor Author

bump: @kanata2 is it possible to get a 👀 on this?

@lorenzoaiello lorenzoaiello self-assigned this Jul 14, 2024
Copy link
Contributor

@lorenzoaiello lorenzoaiello left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome addition @winston-stripe ! I've left just a few minor items for you to review. I'm excited to get this merged and see what folks build with this.

calls.go Outdated
}

// EndCallContext ends a Call.
func (api *Client) EndCallContext(ctx context.Context, callID string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the ability to pass the duration optional argument when ending the call?

calls.go Outdated
return response.Err()
}

// CallAddUsers adds users to a Call.
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you feel about renaming these functions to use Participant instead of User to more closely align to Slack's official terminology and API methods?

@lorenzoaiello lorenzoaiello added feedback given When a review has been conducted and awaiting the response from the comitter(s) and removed needs review labels Jul 15, 2024
@lorenzoaiello lorenzoaiello removed their assignment Jul 29, 2024
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale label Sep 16, 2024
@winston-stripe
Copy link
Contributor Author

ack, finally have time to look back into this. Hopefully get it cleaned up again today. Thanks for the feedback @lorenzoaiello!

@winston-stripe
Copy link
Contributor Author

@lorenzoaiello I updated the params + s/users/participants/ for the Go side of the API to make it a bit more clear & consistent. LMK your thoughts :)

@github-actions github-actions bot removed the stale label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feedback given When a review has been conducted and awaiting the response from the comitter(s) Web API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants