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 Slack integration. #1505

Merged
merged 6 commits into from Mar 27, 2017

Conversation

Projects
None yet
4 participants
@toolness
Contributor

toolness commented Mar 20, 2017

This adds Slack integration via their incoming webhooks API, which we're allowed to use on our TTS Slack. Right now it just notifies its default channel when a price list has been submitted:

price-list-slack-notification

Clicking on the words "price list" will take the user to the "unreviewed price list change" admin page.

We are intentionally not exposing too many details about the price list in the Slack message because we don't want to accidentally include PII or anything else like that.

This PR also adds a manage.py sendtestslack command which sends a test message:

test-msg

The "CALC" link takes the user to the homepage of the CALC deployment.

The name of the slackbot is set to the default Site object's name field. Absolute URLs, as with emails, are generated via the default Site object too.

To do:

  • Add tests for the sendtestslack command.
  • Add tests for slackbot.bot.
@toolness

This comment has been minimized.

Contributor

toolness commented Mar 20, 2017

@jseppi does this approach seem reasonable to you? if so I will go ahead and add the remaining tests.

@jseppi

This comment has been minimized.

Contributor

jseppi commented Mar 20, 2017

The approach looks good to me! I'm not familiar with Python signals, so this looks like a neat way to learn about them.

I think we need to check on any ATO impacts this might have, however.

@toolness

This comment has been minimized.

Contributor

toolness commented Mar 22, 2017

Ok, I talked to Alex Bisker about this yesterday and she said she'd get back to me today--she doesn't think it should be a big deal though, particularly since we're still in the ATO process so it shouldn't be hard to add to the SSP if absolutely necessary.

Btw, the signals thing is actually a Django thing, not a Python thing (Flask has a very similar architecture though, as do lots of other libraries).

toolness added some commits Mar 23, 2017

@toolness toolness changed the title from [WIP] Add Slack integration. to Add Slack integration. Mar 23, 2017

@toolness

This comment has been minimized.

Contributor

toolness commented Mar 23, 2017

Finished adding all tests, so this is good to go once we have ATO stuff sorted out. 🤞

@toolness

This comment has been minimized.

Contributor

toolness commented Mar 23, 2017

Ok, I've further loosened our mypy config because if we don't, there are some compelling use cases we're missing out on.

In the case of this PR, for instance, I wanted to make sure all call sites of slackbot.bot.sendmsg() were valid. If the call signature of sendmsg changed but the call sites didn't, we'd actually never know from the unit tests alone, because the unit tests for every call site mocks out sendmsg.

However, adding type annotations to all the call sites would alert us to this problem: try changing the call signature of sendmsg by adding a required int parameter and see what manage.py ultratest mypy does.

This kind of annotation is only easy, however, if we change mypy's defaults to be more lax about modules that don't have type annotations. Otherwise we start having to make type stubs for all kinds of stuff in django and other third-party libraries and it just becomes a lot more work than it's worth.

@toolness

This comment has been minimized.

Contributor

toolness commented Mar 23, 2017

@jseppi even if we don't get the ATO mod for this in the very near future, do you mind if we merge it but just leave it disabled in prod? It seemed like Aidan and all the other folks I talked to didn't think of it as an actual security concern, so it seems like we should be able to get the paperwork resolved eventually, and it'd be nice to not have to keep this PR in-sync with develop until that happens (also, it'd be nice to get the global changes to the mypy config into develop but I guess I can split that out into a separate PR if needed).

@afeld

This comment has been minimized.

afeld commented Mar 23, 2017

do you mind if we merge it but just leave it disabled in prod?

Yeah, effectively feature-flagging it in the near term is a good solution.

@jseppi

jseppi approved these changes Mar 27, 2017

Code looks great! I have a few questions about how the Slack integration works, but I'll read up on that if you don't want to fill me in.

I assume the "feature-flagging" to not have this enabled until we are ATO-cleared is to just leave SLACKBOT_WEBHOOK_URL undefined?

signals seem sweet! We should use those for the email sending behavior post-SubmittedPriceList-review and post-bulk-upload-processing

name = 'slackbot'
def ready(self):
from . import signals # NOQA

This comment has been minimized.

@jseppi

jseppi Mar 27, 2017

Contributor

Why does this have to be done within ready?

This comment has been minimized.

@jseppi

jseppi Mar 27, 2017

Contributor

Oh, that's what the signals docs say to do:

In practice, signal handlers are usually defined in a signals submodule of the application they relate to. Signal receivers are connected in the ready() method of your application configuration class. If you’re using the receiver() decorator, simply import the signals submodule inside ready().

'text': text,
'username': Site.objects.get_current().name,
'icon_url': absolutify_url(staticfiles_storage.url(
'frontend/images/price-callout/mule.png')),

This comment has been minimized.

@jseppi

jseppi Mar 27, 2017

Contributor

That mule looks kind of weirdly stretched in your sample screenshots :P

I put an icon in the app registration over at https://api.slack.com/apps/A4L7B3B52, maybe it will pull that one if icon_url is not specified?

screen shot 2017-03-27 at 10 18 50 am

This comment has been minimized.

@toolness

toolness Mar 27, 2017

Contributor

Ah nice! Sure, we can plop that into our static files being served and just provide it as the icon_url.

This comment has been minimized.

@jseppi

jseppi Mar 27, 2017

Contributor

Here's the source for the donkey: https://thenounproject.com/term/donkey/28242/

payload = {
'text': text,
'username': Site.objects.get_current().name,

This comment has been minimized.

@jseppi

jseppi Mar 27, 2017

Contributor

The slack webhook docs seem to say that username cannot be overridden for Slack apps:

You cannot override the default username, icon, or channel for incoming webhooks attached to Slack apps.

This comment has been minimized.

@jseppi

jseppi Mar 27, 2017

Contributor

Though I am a bit confused by the difference between a Slack "app" and just an "incoming webhook" (https://gsa-tts.slack.com/apps/A0F7XDUAZ-incoming-webhooks)

This comment has been minimized.

@toolness

toolness Mar 27, 2017

Contributor

Heh, I am a bit confused about it too, but I get the impression that apps might be useful for things that require lots of different kinds of integration with Slack--e.g. possibly via webhooks, but possibly also via other mechanisms like slack's real-time API and/or its REST API--while incoming webhooks are a super lightweight way to just inject data into a slack channel.

I had thought at first that apps were the only way to integrate with slack, but later found out about the incoming webhook thing and that seemed like all we needed.

@jseppi

Actually can you confirm that providing username in the webhook API call actually works? The docs are giving me pause that it would:

Keep in mind that incoming webhooks packaged as Slack apps cannot override the default channel, username, or icon associated with the webhook.

(from: https://api.slack.com/incoming-webhooks#share_your_incoming_webhook_as_a_slack_app)

@toolness

This comment has been minimized.

Contributor

toolness commented Mar 27, 2017

Hmm, that says "packaged as Slack apps" though--I'm not sure if ours will be packaged as Slack apps? I think that requires us to actually create a Slack app, whereas the way 18f's hooks work is by just telling the admins the incoming webhook name and then they add it to 18f's list of incoming webhooks here: https://gsa-tts.slack.com/apps/A0F7XDUAZ-incoming-webhooks

I think another way of distinguishing between non-appy "incoming webhooks" and apps that have "incoming webhook" functionality is that the latter can be shared and easily added to multiple Slacks across the whole Slack ecosystem, not just TTS's Slack. We might prefer that option if we added functionality to CALC that allowed, say, a CO or Data Administrator to ping them on their Slack of choice whenever an event occurred, rather than emailing them. And the UX from their end would probably be super simple, kind of like authorizing a third-party app to use your GitHub account--i.e. it wouldn't require them to manually fill out the webhook form on their Slack's admin screen and paste a weird URL into it or something.

I think it's understandable that such "webhooks packaged as apps" not be able to override the default username or icon because that could easily lead to spoofing--e.g. imagine a CO installing the CALC app to notify them on Slack, but then the CALC app suddenly starts posting messages to it masquerading as GitHub or something. It's less of a concern with non-appy webhooks I think, because it's admins who install webhooks, and it's assumed that the webhooks are scripts that they or other trusted people wrote.

Heck I dunno, I could be totally wrong.

Anyhow, though, I tested this integration out by making my own personal Slack and configuring my CALC dev instance to use a webhook I configured on it. I can give you access to the Slack and go over it if you want.

@jseppi

This comment has been minimized.

Contributor

jseppi commented Mar 27, 2017

Ok cool, I was pretty sure you had tested that it actually worked but wanted to make sure. Does that mean the app you registered (https://api.slack.com/apps/A4L7B3B52) is actually going to be unused in favor just setting up webhooks?

@jseppi

jseppi approved these changes Mar 27, 2017

@toolness

This comment has been minimized.

Contributor

toolness commented Mar 27, 2017

Oh sorry, I didn't realize you were looking at that--that must have been confusing! Yeah, before I knew about non-appy incoming webhooks, I thought I'd have to create an app, so I made that... but then once I found out about the non-appy way to do it, I forgot to delete the app. I've done that now.

@toolness toolness merged commit 8328573 into develop Mar 27, 2017

4 checks passed

codeclimate no new or fixed issues
Details
codeclimate/coverage 95.55% (+0.1%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@toolness toolness deleted the slackbot branch Mar 27, 2017

@NoahKunin NoahKunin removed the in progress label Mar 27, 2017

@toolness toolness referenced this pull request Mar 29, 2017

Merged

Remember attempted price list upload submissions #1491

7 of 7 tasks complete

@jseppi jseppi referenced this pull request Apr 14, 2017

Merged

Tag and release v2.7.0 #1545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment