Slack Bot #2632

mxstbr · 2018-03-21T11:36:36Z

Status

WIP
Ready for review
Needs testing

Deploy after merge (delete what needn't be deployed)

iris (api)
hyperion (frontend)

Release notes for users (delete if codebase-only change)

Slack bot! You can now get notified in your Slack team whenever a new thread is posted in your community on Spectrum

mxstbr · 2018-03-21T11:44:54Z

This works now!! 🎉

Authenticate community with Slack team
Copy token from slackImports table
Send POST request to Slack with:

curl \
  -X POST \
  -H 'Content-Type: application/json' \
  -H 'Authorization: Bearer {token}' \
  -d '{"channel":"#general","text": "Hello World again!"}' \
  https://slack.com/api/chat.postMessage

This is what it'll look like:

Note: This will show "Spectrum" with our logo, I just need admin access to the Space Program Slack team @brianlovin to edit the actual apps permission setup—for now I created a separate one in my own test workspace as I couldn't change them in the actual one.

I've updated the OP with the two main leftover questions.

@brianlovin I'll need your help on the settings UI for this—basically users need to be able to say "On {action} (either new message or new thread atm) send message to {channel}" (where {channel} is just a text input and we show an error if that channel doesn't exist) I don't know where to fit that with the existing invite stuff to be completely honest.

There's also the existing connections that won't have the right permissions, I'm trying to figure out how we check that and get us the right permissions for those, that'll also need a UI state to maybe connect again or something? Don't know yet.

brianlovin · 2018-03-21T15:13:20Z

Woo woo!!!!

Stoked to see this. And I think @uberbryn is the person to make you an admin of the space program slack team.

Here are my thoughts from reading your last comment:

we should nail down what some of the core user flows are and build around that
we should consider keeping slack imports and slack bot tokens separate, since they serve different purposes, and we will have the problem of mixed read/write permission tokens now with the old slackimports code.

Here are my thoughts on the above:

I think a key flow will be that an owner says "When a new thread happens in {channel a} on Spectrum, I want it to post in {channel b} on my Slack team"
I'm having a hard time thinking of a good use case for wanting all new messages to ping a Slack team; do you have something in mind?
It seems likely that community owners will want multiple channels to go to multiple channels. For example we'd have the feature-requests channel on spectrum ping our 'product' slack channel, and our hugs-n-bugs channel on spectrum ping our 'support team' slack channel.

If those assumptions hold, we might end up with something like this:

on communitySettings record we store a slack token with read/write auth
on channelSettings we store preferences for slack cross posting, maybe like:

{
  // ...channelSettings
  slackBotSettings: {
    isEnabled: boolean,
    channels: Array<string>,
    eventTypes: Array<string>
  }
}

where channels is an array of strings that map to channels they have on their slack team. We should start with only supporting one, but make it an array so that in the future we could allow them to cross post into multiple slack channels. This might be edge case?

where eventTypes maps to events that happen on our end. We should start with supporting 'new thread published' to keep things easy to start, but in the future could add anything like 'moderation event X', or 'user requested to join Y' etc etc

mxstbr · 2018-03-21T15:30:03Z

we should consider keeping slack imports and slack bot tokens separate, since they serve different purposes, and we will have the problem of mixed read/write permission tokens now with the old slackimports code.

That seems like a really unnecessary abstraction? Users would have to click Connect Slack Team twice for essentially the same thing, and we'd have to duplicate all the routing logic on the backend just to get two tokens when a single one can do all the things too?

I'm having a hard time thinking of a good use case for wanting all new messages to ping a Slack team; do you have something in mind?

Syndicating all content to Slack I guess. We should make this happen and see what people do with it imo.

If those assumptions hold, we might end up with something like this:

That data model makes sense to me!

brianlovin · 2018-03-21T15:33:33Z

That seems like a really unnecessary abstraction? Users would have to click Connect Slack Team twice for essentially the same thing, and we'd have to duplicate all the routing logic on the backend just to get two tokens when a single one can do all the things too?

Fair; only case I could think would be people needed different slack teams for importing (eg. import their community) and a team for cross posting (e.g. internal team slack). Guess we'll find this out soon enough...

Syndicating all content to Slack I guess. We should make this happen and see what people do with it imo.

Fair enough, if we're handling all kinds of event types it makes sense to me to add it and see what happens.

brianlovin · 2018-03-21T15:44:45Z

Was just thinking on this a bit more, and I think we will want two Slack auth flows in many cases.

Imagine a community first sets up the slack bot. We should definitely not grab member info to queue for invites - we only need the token. So if they do this first, then if they ever choose to import a slack team they'd need to re-auth so we can get the members data with their permission.

The opposite flow (import => bot) seems fine to re-use the token, although I don't know how we'll sort out which tokens need extra permissions at this point. We can dig in though.

mxstbr · 2018-03-21T16:24:08Z

That seems so hard to do, let's not over-engineer and make it single-connect for v1?

brianlovin · 2018-03-21T16:28:03Z

I do not think that we should gather member information if the user is not expecting us to do so. If anything were to happen there it would be pretty bad and hurt the trust we've been building.

mxstbr · 2018-03-21T16:53:58Z

I do not think that we should gather member information if the user is not expecting us to do so.

"Connect a Slack team—To invite the members of your Slack team and send new threads to a channel connect it below!"

How is that not expected? I feel like we're way overthinking this for the MVP, let's just send new threads to a channel and be done with it?

brianlovin · 2018-03-21T18:21:34Z

How is that not expected? I feel like we're way overthinking this for the MVP, let's just send new threads to a channel and be done with it?

It just needs to be useful at the start, and I don't think that having one action to set up both is right. I think that inviting a Slack team is a pretty big decision, and people (especially internal teams, or communities thinking of migrating) will be very intentional about deciding when to pull that trigger.

Where sending new thread notifications to a channel is super low pressure, and is actually a huge opportunity for us to build that trust and help communities eventually build up to the decision of doing a full import.

I don't want to overthink this for an MVP, but I also don't want to ship something that people are scared to use because they think we're about to swallow their whole team's data.

It should be two sections for importing a team versus managing outbound bot pings. We can figure out how to connect those with a single 'connect with slack' button, but I don't feel that the first connection should get all their member info until they explicitly want to import.

mxstbr · 2018-03-21T19:01:53Z

I think that inviting a Slack team is a pretty big decision, and people (especially internal teams, or communities thinking of migrating) will be very intentional about deciding when to pull that trigger.

I 100% agree, that's why once you connect a Slack team you'll get two buttons: One to invite and one to add a new message sending thingy, right?

Let me figure out tomorrow how hard it would be to add more permissions later, if it's easy let's do it your way, if not let's figure out what else to do.

superbryntendo · 2018-03-21T19:04:03Z

I think most slack team invites will be internal teams fwiw. I think it won’t be a huge deal to make that decision

…

On Mar 21, 2018 at 12:01 PM, <Max Stoiber ***@***.***)> wrote: > > > I think that inviting a Slack team is a pretty big decision, and people (especially internal teams, or communities thinking of migrating) will be very intentional about deciding when to pull that trigger. > > I 100% agree, that's why once you connect a Slack team you'll get two buttons: One to invite and one to add a new message sending thingy, right? Let me figure out tomorrow how hard it would be to add more permissions later, if it's easy let's do it your way, if not let's figure out what else to do. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (#2632 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AEstaCNjZK5mjvEKSsibCdH54dd7rOa-ks5tgqOjgaJpZM4SzaBk).

brianlovin · 2018-03-21T19:17:57Z

Disagree based on the imports we've seen so far though. but maybe.

Anyways, synced on video chat and Max and I were talking about the same thing: we request the permissions up front, but let the import itself be explicit so that we never store member data until the user is ready.

mxstbr · 2018-03-22T07:46:42Z

To ask for an additional permission, a user must first interact with your application one of two ways: 1. Invoke a slash command, if your app offers one 2. Interact with a message button or message menu as part of the interactive message framework.

Well damn, as far as I can tell this basically means anybody who connected a Slack team before this PR will need to do it again. 😕

mxstbr · 2018-03-22T07:57:14Z

There's two different flows here:

First-time connection

A community admin goes to /settings and clicks on "Connect to Slack"
Authorizes usage with their Slack team

Previously connected Slack teams

A community admin goes to /settings
We ping https://api.slack.com/methods/apps.permissions.info to see if we have the chat:write:bot permission
- If not, we show a "Reconnect to Slack" button
- Authorizes usage with their Slack team again

At the end, both of them see two buttons: One to "Invite their Slack team members to the Spectrum community" and one to "Send new threads in their community to a channel in Slack".

I'm thinking about where to put the API call to check the permissions. Maybe we just add a flag in a migration to all existing connections that says "They don't have permissions, ask again", then we don't even have to do that API call?

mxstbr · 2018-03-22T08:42:16Z

Turns out, we actually import members on connect right now from athena. Good news is that it'll be easy to refactor since that's just a job we can trigger at any time!

mxstbr · 2018-03-22T09:02:34Z

Doing a bunch of UI tweaks in the settings pane right now to make it fit with no longer only being there to invite folks, it'd be great to get a hand with those.

This is a first rough WIP:

Note: The separate "Invite members" step happens automatically right now, to see it you have to turn off athena. Once you turn athena on, it'll import the members automatically and the state will change on the next refresh—this'll be way different once that is a manual action, haven't built that just yet.

Obviously missing is the second section to enable webhooks, no clue how to make that happen just yet!

- Split into two components, one for members inviting and one general - Add intermediate state for importing members (WIP)

mxstbr · 2018-03-22T09:13:43Z

I really want to e2e test this UI but the existing community settings tests are in #2354 ugh

brianlovin · 2018-03-22T14:17:33Z

I really want to e2e test this UI but the existing community settings tests are in #2354 ugh

😵 oh no! Hopefully we can get it on alpha today, I don't see why not :)

brianlovin · 2018-03-22T14:18:42Z

Happy to help with the UI and wiring up the new channelSettings stuff

mxstbr · 2018-03-22T15:34:11Z

Please jump in! Would love any help!

Instead of immediately loading the entire members data from Slack when folks connect their team, we now only do it once they click "Import"! :tada: :tada: :tada: This works really nicely, see https://i.imgur.com/QLX75wF.gif

mxstbr · 2018-03-23T10:28:25Z

The latest commit now means we only import member data once folks click "Import members"!!! 🎉 Look at it:

brianlovin · 2018-05-01T18:16:31Z

@mxstbr we lost the rule to ignore console logs in migrations :)

mxstbr · 2018-05-01T18:19:23Z

We never had that rule in the first place, console.logs aren't allowed anywhere—only flow 😉

/cc @orta that patch with the rate limiting fixed Peril for large PRs! 🎉

brianlovin · 2018-05-01T18:25:11Z

We had an ignore directory rule there. But no worries, will just switch to error

…dering

brianlovin · 2018-05-02T03:56:04Z

Writing tests for this is a bit tricky as it would require handling 3rd party oauth (not possibly with cypress afaik) or mocking out entire request flows to and from slack api. I'm honestly not sure how to go about this properly - in the meantime I at least added checks around who should have access to querying slack settings on communities and channels.

So @mxstbr this is ready for your code review. I also refactored the permission logic today to be separated from the context constructor; lmk if you're happy with how all that looks!

mxstbr · 2018-05-02T07:51:08Z

I also refactored the permission logic today to be separated from the context constructor

Wait hold on, why? I thought we were happy with that?

mxstbr · 2018-05-02T08:02:41Z

api/types/ChannelSlackSettings.js

+  }
+
+	type ChannelSlackSettings {
+		botConnection: BotConnection


This name is confusing: botConnection is not a connection. (as per the Relay spec and how we use it elsewhere) If you want this to be a connection just like all our other connections (which it should be), please make it conformant by adding edges, nodes and page info:

type BotSettings { threadCreated: String } type BotConnectionEdge { node: BotSettings! } type BotConnection { pageInfo: PageInfo! edges: [BotConnectionEdge!] }

You don't have to build the actual pagination for now, but at least this sets us up to do it properly in the future.

mxstbr · 2018-05-02T08:14:38Z

Getting this locally after connecting a Slack team:

Maybe I need some updated secrets or something?

brianlovin · 2018-05-02T15:07:47Z

Wait hold on, why? I thought we were happy with that?

Asked about this on Twitter and you said it looked good! Anyways, the reason is to keep permissions away from the schema and pushed down into business logic. Writing them as resolver wrappers is still a bit more tightly integrated, but it makes for a nice experience.

This name is confusing: botConnection is not a connection.

I don't want it to be a relay connection spec field. I am trying to communicate how the bot is connected to the slack team, so connection came to mind. Open to other naming ideas!

Getting this locally after connecting a Slack team:

Console logs?

mxstbr · 2018-05-02T15:12:50Z

Anyways, the reason is to keep permissions away from the schema and pushed down into business logic.

They weren't in the schema before though, just packed into the GraphQL context so that they're easy to call. @isAuthed was in the schema, and I thought that's what we were talking about moving out. Anyways, not a biggie, I liked the user.canManageChannel(channelId) API more than canManageChannel(user.id, channelId, loaders) one but it doesn't matter too much.

I am trying to communicate how the bot is connected to the slack team, so connection came to mind.

Oh I see, I totally misinterpreted that name. I think we probably shouldn't call it *Connection since that's pretty reserved for the Relay stuff—what about slackSettings.links or just slackSettings.botConfig or something like that?

brianlovin · 2018-05-02T15:14:49Z

Re the error you're seeing after connecting a slack team: for some reason we have 2 spectrum apps on Slack, and the secrets were mismatched. I updated the now-secrets in 1password, can you grab that and try again? Any reason we have 2 apps for this?

brianlovin · 2018-05-02T15:16:42Z

Oh I see, I totally misinterpreted that name. I think we probably shouldn't call it *Connection since that's pretty reserved for the Relay stuff—what about slackSettings.links or just slackSettings.botConfig or something like that?

Will update now.

brianlovin · 2018-05-02T15:36:20Z

Named botLinks - how does that feel to you?

Fix Slack permission scopes

9aecdf8

spectrum-bot added the WIP: Building label Mar 21, 2018

console.log -> console.error

e064237

Refactor importSlack UI settings pane

d2d162d

- Split into two components, one for members inviting and one general - Add intermediate state for importing members (WIP)

mxstbr added 2 commits March 22, 2018 10:18

Fix invitation mutation

e4117e3

Im a dummy

6f24bd5

mxstbr added 2 commits March 23, 2018 10:13

Move slackImportQueue.add from db query

adc243a

Lazily import members from Slack

2c405d7

Instead of immediately loading the entire members data from Slack when folks connect their team, we now only do it once they click "Import"! :tada: :tada: :tada: This works really nicely, see https://i.imgur.com/QLX75wF.gif

Store Slack scope with slackImports record

8510218

withspectrum deleted a comment from spectrum-bot bot May 1, 2018

brianlovin added 2 commits May 1, 2018 08:51

ESLint

a7f631c

Fix e2e tests

c835d03

withspectrum deleted a comment from spectrum-bot bot May 1, 2018

Merge branch 'alpha' of github.com:withspectrum/spectrum into slack-bot

c077ce8

brianlovin added 2 commits May 1, 2018 11:16

Remove console

53b47dc

Remove console

ffc8408

brianlovin added 7 commits May 1, 2018 17:16

Move permission logic out of context construction

901ff5c

Fix resolvers to use new permission checks

fd05e6e

Fix tests context construction

1f7b86d

Fix small bug in caching with channel creation

6b3852e

Simplify type for context user

871560d

Add tests for querying channel slack settings

5743014

Adds tests for querying slacksettings, fixes some client fallback ren…

d8fdedb

…dering

mxstbr commented May 2, 2018

View reviewed changes

brianlovin added 2 commits May 2, 2018 08:35

botConnection => botLinks

cf1632d

Remove console

466b5e9

brianlovin merged commit 0c4c737 into alpha May 2, 2018

brianlovin deleted the slack-bot branch May 2, 2018 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slack Bot #2632

Slack Bot #2632

mxstbr commented Mar 21, 2018 •

edited by brianlovin

Loading

mxstbr commented Mar 21, 2018 •

edited

Loading

brianlovin commented Mar 21, 2018 •

edited

Loading

mxstbr commented Mar 21, 2018

brianlovin commented Mar 21, 2018

brianlovin commented Mar 21, 2018

mxstbr commented Mar 21, 2018

brianlovin commented Mar 21, 2018

mxstbr commented Mar 21, 2018 •

edited

Loading

brianlovin commented Mar 21, 2018

mxstbr commented Mar 21, 2018

superbryntendo commented Mar 21, 2018 via email

brianlovin commented Mar 21, 2018

mxstbr commented Mar 22, 2018

mxstbr commented Mar 22, 2018 •

edited

Loading

mxstbr commented Mar 22, 2018

mxstbr commented Mar 22, 2018 •

edited

Loading

mxstbr commented Mar 22, 2018 •

edited

Loading

brianlovin commented Mar 22, 2018

brianlovin commented Mar 22, 2018

mxstbr commented Mar 22, 2018

mxstbr commented Mar 23, 2018

brianlovin commented May 1, 2018

mxstbr commented May 1, 2018

brianlovin commented May 1, 2018

brianlovin commented May 2, 2018

mxstbr commented May 2, 2018

mxstbr May 2, 2018

mxstbr commented May 2, 2018

brianlovin commented May 2, 2018

mxstbr commented May 2, 2018

brianlovin commented May 2, 2018

brianlovin commented May 2, 2018

brianlovin commented May 2, 2018

Slack Bot #2632

Slack Bot #2632

Conversation

mxstbr commented Mar 21, 2018 • edited by brianlovin Loading

mxstbr commented Mar 21, 2018 • edited Loading

brianlovin commented Mar 21, 2018 • edited Loading

mxstbr commented Mar 21, 2018

brianlovin commented Mar 21, 2018

brianlovin commented Mar 21, 2018

mxstbr commented Mar 21, 2018

brianlovin commented Mar 21, 2018

mxstbr commented Mar 21, 2018 • edited Loading

brianlovin commented Mar 21, 2018

mxstbr commented Mar 21, 2018

superbryntendo commented Mar 21, 2018 via email

brianlovin commented Mar 21, 2018

mxstbr commented Mar 22, 2018

mxstbr commented Mar 22, 2018 • edited Loading

mxstbr commented Mar 22, 2018

mxstbr commented Mar 22, 2018 • edited Loading

mxstbr commented Mar 22, 2018 • edited Loading

brianlovin commented Mar 22, 2018

brianlovin commented Mar 22, 2018

mxstbr commented Mar 22, 2018

mxstbr commented Mar 23, 2018

brianlovin commented May 1, 2018

mxstbr commented May 1, 2018

brianlovin commented May 1, 2018

brianlovin commented May 2, 2018

mxstbr commented May 2, 2018

mxstbr May 2, 2018

Choose a reason for hiding this comment

mxstbr commented May 2, 2018

brianlovin commented May 2, 2018

mxstbr commented May 2, 2018

brianlovin commented May 2, 2018

brianlovin commented May 2, 2018

brianlovin commented May 2, 2018

mxstbr commented Mar 21, 2018 •

edited by brianlovin

Loading

mxstbr commented Mar 21, 2018 •

edited

Loading

brianlovin commented Mar 21, 2018 •

edited

Loading

mxstbr commented Mar 21, 2018 •

edited

Loading

mxstbr commented Mar 22, 2018 •

edited

Loading

mxstbr commented Mar 22, 2018 •

edited

Loading

mxstbr commented Mar 22, 2018 •

edited

Loading