-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Expose optional State parameter returned from Add to Slack button #95
Expose optional State parameter returned from Add to Slack button #95
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't want state in the model. Otherwise it's close.
@@ -60,6 +61,8 @@ class TeamsEndpoint < Grape::API | |||
) | |||
end | |||
|
|||
team.state = params[:state] if params[:state] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be if params.key?(:state)
, because a blank state is different from absence of state.
@@ -60,6 +61,8 @@ class TeamsEndpoint < Grape::API | |||
) | |||
end | |||
|
|||
team.state = params[:state] if params[:state] | |||
|
|||
Service.instance.create!(team) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want to change create!
to take options = {}
in which you pass in whatever was in params
for future compatibility.
@@ -10,6 +10,7 @@ module TeamPresenter | |||
property :team_id, type: String, desc: 'Slack team ID.' | |||
property :name, type: String, desc: 'Team name.' | |||
property :domain, type: String, desc: 'Team domain.' | |||
property :state, type: String, desc: 'State parameter returned from Add to Slack response.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
State is unlikely to be used other than to verify something once, lets not add it to the model.
@@ -60,7 +61,9 @@ class TeamsEndpoint < Grape::API | |||
) | |||
end | |||
|
|||
Service.instance.create!(team) | |||
state = params.key?(:state) ? { state: params[:state] } : {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather that state
this is options
, let's just rename.
But just use slice
, so
options = params.slice(:state)
Service.instance.create!(team, options)
Add another integration spec in https://github.com/slack-ruby/slack-ruby-bot-server/blob/master/spec/integration/teams_spec.rb#L12 for state as well, please. |
Merged, thank you. Any interest to help co-maintain this gem? No obligations other than whatever time permitting. Maybe you can make the next release? If yes, email me your rubygems email/username to dblock at dblock dot org. |
I would be absolutely honored to. Sending the email now |
The Add to Slack button also allows for an optional
state
parameter that will be returned on completion of the request. Thecreating
andcreated
callbacks include an options hash where this value can be accessed (to check for forgery attacks for instance).