Implement slash command to list channels#30
Conversation
| Utils::ParsesCliStyleCommandArgs.new.call(text: params[:text]).subcommand | ||
| end | ||
|
|
||
| def command_params |
There was a problem hiding this comment.
The intent for this method is for arguments passed to a slash command to feel like typical rails params hash.
| render plain: "Nope.", status: :unauthorized | ||
| end | ||
|
|
||
| def subcommand |
There was a problem hiding this comment.
This returns the subcommand for the given request. For /doubleup list this would be "list"
| @@ -0,0 +1,18 @@ | |||
| module Chatops | |||
| class SlackSlashCommandListController < ::ApplicationChatopsController | |||
| def handle | |||
There was a problem hiding this comment.
This should be treated as a minimal first pass to be iterated on. Slack does not turn channels into links when the channel name is used. Instead we need to use the channel name to look up the channel ID and use that, like "<##{grouping.channel_id}>. I'm opting to leave this out of the current PR since we're planning to move to a DB backed model where we can cache the channel id.
| @@ -0,0 +1,6 @@ | |||
| module Matchmaking | |||
| def config | |||
There was a problem hiding this comment.
Despite my growing distaste for Rails' config.x., that's not why I added this. 😂 I'd prefer to stub our own methods instead of stubbing something from the framework so this lets us to allow(Matchmaking).to receive(:config).and_return({}) which is a bit more tidy. It adds a layer of indirection but I don't think it's too bad.
| @@ -1,8 +1,20 @@ | |||
| class SlackSlashSubcommandConstraint | |||
There was a problem hiding this comment.
Constraint allows us to route different subcommands to different controllers instead of building up some big case statement based on subcommands. There are no specific tests for this but it should be covered by the Request specs that make sure the post works.
| @@ -0,0 +1,95 @@ | |||
| require "rails_helper" | |||
|
|
|||
| class CommandParamsChatopsController < ApplicationChatopsController | |||
There was a problem hiding this comment.
Testing slash command params method in something like isolation.
|
|
||
| slack_signing_secret = Slack::Events.config.signing_secret | ||
| timestamp = Time.zone.now.to_i | ||
| request_body = "token=#{slack_signing_secret}&team_id=T02PF6RHYSY&team_domain=testdouble-hq&channel_id=C02NYBB3VPH&channel_name=some-channel&user_id=U02PRHH0XEV&user_name=cliff.pruitt&command=%2Fdoubleup&text=list&api_app_id=A02PD0DUE03&is_enterprise_install=false&response_url=https%3A%2F%2Fhooks.slack.com%2Fcommands%2FT02PF6RHYSY%2F2823421496992%2F0WC0HfWeGJpHetxmF8yUmawo&trigger_id=2801968477828.2797229610916.883001cf5008d6d02fd58a3cf70f449e" |
There was a problem hiding this comment.
This is gonna be tech debt. It's a pain to edit these when a param changes, but Hash#to_param does sorting that changes the request body so the HMAC signature isn't the same. I'd like to add a test helper in to manage this stuff but kinda just want to prioritize getting some features out.
| scenario "responds with default message when no channels configured" do | ||
| allow(Matchmaking).to receive(:config).and_return({}) | ||
|
|
||
| signed = signed_request_body(params: { |
There was a problem hiding this comment.
Proposing this as a simple helper to manage the params in tests (see private method in this file below). signed_request_body returns an object that gives us the params (body) as a string instead of a hash, a signature, and the timestamp used to produce the signature. These can then be used to make the test request.
If this patterns works then we can extract this into a test helper method and update the other tests.
cpruitt
changed the title
cpruitt
force-pushed
the
implement-slash-list-command
branch
from
2abb496 to
269011a
Compare
|
This is way old. Closing. |
1 participant
This PR adds a basic handler for
/doubleup list. The command will return a list of all active channels by default, and all channels if using the--allflag. (Technically you could use--all,-all,allor-------------------------allsince we just strip-s). Each item in the list privides the name, schedule, and channel to join.Channel names are not links. This is because Slack formatting requires using channel ID's not names. I think this is kind of annoying, but I don't control slack. I don't address channel ID lookup in this PR so for now the channel names are just plain text. We can more easily address that when we move configs to the DB. When that happens we can store the channel ID.
There are inline comments with some thoughts on the code changes.
Scope of Review
I'm still optimizing for getting features done. I don't want to go all "Wild West" but I'd suggest that since we don't have a ton of bandwidth to devote to this we focus mainly on blocking issues or things that will lead us down bad paths that will be hard to back out of.
command_params[:something])