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

Optionally allow a user ID to be passed into users.identity for WTA #432

Merged
merged 2 commits into from
Dec 20, 2017

Conversation

bertrandom
Copy link
Contributor

Summary

users.identity can now be passed a user ID when used with Workspace Token Apps. This allows an optional user ID to be passed into the method, while supporting the old function signature where this parameter used to be a callback.

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Dec 13, 2017

Codecov Report

Merging #432 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #432      +/-   ##
==========================================
+ Coverage   89.01%   89.03%   +0.01%     
==========================================
  Files          44       44              
  Lines        1247     1249       +2     
  Branches      187      189       +2     
==========================================
+ Hits         1110     1112       +2     
  Misses        137      137
Impacted Files Coverage Δ
lib/clients/incoming-webhook/client.js 100% <0%> (ø) ⬆️

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 ec04e5d...574d242. Read the comment docs.

@aoberoi
Copy link
Contributor

aoberoi commented Dec 14, 2017

@bertrandom this is great! i have a couple questions for ya.

  1. the user argument isn't documented on the reference. will it be? if so, when? if not, then i'm not too keen on adding "hidden" arguments into the SDK's API. if those arguments change, we will break users, and hidden arguments are subject to change with no announcement.

  2. we've had a number of issues trying to "upgrade" a facet from having only explicit position arguments to later accepting an object for optional arguments. i'd like to future proof where we can. this would be API gymnastics, but how do you feel about an implementation that supports all these call sites?

  • web.users.identity()
  • web.users.identity(callback)
  • web.users.identity(user)
  • web.users.identity(user, callback)
  • web.users.identity(user, options)
  • web.users.identity(user, options, callback)

@aoberoi
Copy link
Contributor

aoberoi commented Dec 14, 2017

Update: i'm dumb.

user would be an optional argument, not a required one (which we translate into positional arguments in the facet).

we actually want to support these call sites then:

  • web.users.identity()
  • web.users.identity(callback)
  • web.users.identity(options)
  • web.users.identity(options, callback)

@bertrandom
Copy link
Contributor Author

@aoberoi Let me try to get that parameter unhidden before we change this. And you make a good point, using options instead of user will help us future-proof if we add more params later, I'll update the PR.

@bertrandom
Copy link
Contributor Author

@aoberoi Okay, I modified this to pass an options object, would you mind reviewing it one last time and then merging it in when you get a chance? Thanks.

@aoberoi
Copy link
Contributor

aoberoi commented Dec 20, 2017

thanks so much @bertrandom!

@aoberoi aoberoi merged commit 2b7f417 into slackapi:master Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants