Skip to content

Feature: Slack gig notifications#197

Open
bklang wants to merge 24 commits into
Gig-o-Matic:masterfrom
bklang:feature/slack_gig_notifications
Open

Feature: Slack gig notifications#197
bklang wants to merge 24 commits into
Gig-o-Matic:masterfrom
bklang:feature/slack_gig_notifications

Conversation

@bklang

@bklang bklang commented Oct 15, 2017

Copy link
Copy Markdown
Contributor

WIP

captura de tela 2017-10-21 as 15 52 31

TODO:

  • Allow configuring Slack credentials
  • Allow enrollment of the bot on a per-band basis (OAuth flow)
  • Allow configuring the announcement channel on a per-band basis
  • Fix need to join the announcements channel
  • Announce new gigs
  • Announce edits to gigs
  • Fix bug related to localizing gig date when no member qualifies
  • Add a way to dis-enroll band from Slack notifications

@bklang

bklang commented Oct 15, 2017

Copy link
Copy Markdown
Contributor Author

For the record: I initially tried using the official Slack Python library, slackclient, but it does not work well at all on Google App Engine. slackclient uses the Python requests library under the covers. The reason has to do with GAE limitations around access to sockets, as documented here and here and here.

@aaronoppenheimer

Copy link
Copy Markdown
Contributor

Ultimately, how will this work? Every band will have a slack channel to subscribe to? There's only one gig-o slack channel? I'm not enough of a slack user to know the best way. If you want to talk to the other folks who have requested this, let me know - they likely have opinions.

@bklang

bklang commented Oct 16, 2017

Copy link
Copy Markdown
Contributor Author

The goal is to put messages into each band's own Slack group, and let each band control the room into which announcements are posted. The default will be #general, which is the default channel that comes with every Slack group. Notifications will only be posted to a room, and not be individualized.

As for enrollment, the flow I have in mind is this:

  1. A Band admin goes to a settings page to enable Slack integration.
  2. Clicking the "Add Gig-O-Matic to Slack" takes the user to Slack's website to complete the OAuth flow (essentially: logging into Slack and approving the request to add Gig-O-Matic as a bot)
  3. After granting the requested permission, the user is redirected back to Gig-O-Matic's Slack settings screen. On this step, we capture the security token in Gig-O-Matic that we will use to authenticate messages into the Band's Slack team
  4. On this screen, the user is given a listing of channels, and will choose the channel into which announcements will be sent (this will probably only include public channels at first, due to security settings - we can improve this later if there is sufficient demand)

Possible later enhancements:

  • Sending messages into private channels
  • Per-band-member reminders, with personalized links
  • Buttons in the Slack message enabling responding to the Gig request directly from Slack

* Send just one message per band (instead of one per member)
* Link to the general Gig Info page, not to a specific member’s URL
* Include more gig details: confirmation status, date, time(s)
@bklang

bklang commented Oct 21, 2017

Copy link
Copy Markdown
Contributor Author

Updated screenshot showing improved message format added to description

@aaronoppenheimer

Copy link
Copy Markdown
Contributor

Got it - that's cool!

AppEngine only provides Jinja2 2.6, which does not include several useful new built-in filters, including `urlencode`
Until a band is saved, we don’t have an ID to use for callbacks, so it doesn’t make sense to show the button on the New form
@bklang

bklang commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

@aaronoppenheimer This is almost ready! One question for you: we need to stick a secret key into the app, which represents how the app authenticates itself to Slack. The key should not be in the repo because it is a security token. How would you like to handle this? Should we essentially copy the mechanism you use for setting the CryptoKey? This key is only settable by the site admin, and should not ever (or extremely rarely) change, so perhaps UI is not required. What are your thoughts?

@aaronoppenheimer

Copy link
Copy Markdown
Contributor

@bklang Any reason not to just use the same key?

* Separate announcements functionality out from toenail to go announcements
* Move Slack-specific functionality to goslack
@bklang

bklang commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

@bklang Any reason not to just use the same key?

We can't. The authentication token is provided by Slack as part of the registration process. Gig-O-Matic will have to register itself as a Slack App, for which the token will be generated by Slack. Currently, I'm using a copy of the app registered to one of my own Slack accounts, but obviously for production we can re-register it to an account for Gig-O (or leave it under my account if you prefer - I recall you saying you're not a Slack user).

@aaronoppenheimer

Copy link
Copy Markdown
Contributor

Oh, I get it. Well, we could use the slack API token as the crypto key, which is just a random string now anyway. Or you could just add another input box on the admin crypto page and store it separately.

@bklang

bklang commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

That also won't work :( Slack actually provides two pieces of information that we have to hang on to: a Client ID and a Client Secret.

@aaronoppenheimer

Copy link
Copy Markdown
Contributor

Ha, OK, I give up. I'd just add a couple of fields to the crypto admin page and handle it all in there. It's only going to be touched by me, once, so it doesn't have to be pretty!

@bklang bklang force-pushed the feature/slack_gig_notifications branch from a80921d to eac1278 Compare October 24, 2017 14:38
@bklang

bklang commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

@aaronoppenheimer Alright, this is ready to go! I made the changes we discussed: renamed the "Crypto Admin" page to "Secrets Admin" and added the two fields for Slack Client ID and Secret:

captura de tela 2017-10-24 as 18 12 26

Would you do code review and let me know if you would like to see any changes?

It's also worth noting that a fair amount of code changed in goemail.py, since I tried to share as much of it as possible with the new goslack.py. I've tested as best I can locally, with New Gig, Edit Gig and Reminder emails all being sent. But I can only verify what I can see in the App Engine developer console. Since a lot of people depend on those emails, I'd appreciate someone else giving it a look over to make sure I didn't break anything. The included unit tests are still passing.

@aaronoppenheimer

Copy link
Copy Markdown
Contributor

Hi Brian,

Works and very cool! A couple of things:

There's some alignment problem I see on the edit band page:

image

Also, get an error if there is no admin defined for a band (there's some code to handle this in there but doesn't seem to be working):

Traceback (most recent call last): File "/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine/lib/webapp2-2.5.2/webapp2.py", line 1535, in __call__ rv = self.handle_exception(request, response, e) File "/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine/lib/webapp2-2.5.2/webapp2.py", line 1529, in __call__ rv = self.router.dispatch(request, response) File "/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine/lib/webapp2-2.5.2/webapp2.py", line 1278, in default_dispatcher return route.handler_adapter(request, response) File "/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine/lib/webapp2-2.5.2/webapp2.py", line 1102, in __call__ return handler.dispatch() File "/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine/lib/webapp2-2.5.2/webapp2.py", line 572, in dispatch return self.handle_exception(e, self.app.debug) File "/Applications/GoogleAppEngineLauncher.app/Contents/Resources/GoogleAppEngine-default.bundle/Contents/Resources/google_appengine/lib/webapp2-2.5.2/webapp2.py", line 570, in dispatch return method(*args, **kwargs) File "/Users/aaron/src/go2/goslack.py", line 78, in post the_date_string = goannouncements.format_date_string(the_gig.date, a_member) File "/Users/aaron/src/go2/goannouncements.py", line 50, in format_date_string return "{0} ({1})".format(member.format_date_for_member(the_member, the_date), File "/Users/aaron/src/go2/member.py", line 359, in format_date_for_member if the_user.preferences and the_user.preferences.locale: AttributeError: 'Assoc' object has no attribute 'preferences'

The form was relying on the_band to have real data, which it doesn’t yet
Addresses the reported formatting problem
@aaronoppenheimer

Copy link
Copy Markdown
Contributor

Also, should add a way to turn off slack notifications for a band, too.

@bklang

bklang commented Nov 4, 2017

Copy link
Copy Markdown
Contributor Author

I've pushed a fix for the alignment issue. I've added a TODO for a way to dis-enroll the band from Slack, which will stop notifications.

Looking into the exception that you hit, this one is a little trickier. The code is trying to localize the date string. The assumption is that the first band admin is in the same locale as the rest of the band's members. So we try to find the first admin, and then format the date to his/her locale. If an admin can't be found, then we try to take the first invited member and format it to his/her locale.

Unfortunately, I've found a few ways this can fail:

  • The band has no admin, and no invited members (eg. a band with 100% occasional members)
  • The found member (band admin || first invited member) has no preference set - though this is probably an existing bug in the code, which could be hit when sending email to this person

I'm still working out how to fix the issue. If anything obvious jumps out at you, suggestions welcome!

@aaronoppenheimer

aaronoppenheimer commented Nov 4, 2017

Copy link
Copy Markdown
Contributor

Hi Ben,

Well, a couple of things. For one thing, a band might not have any invited members - a member is only "invited" when they're sent an invitation. If they don't join that way (like, they go to the home page and just register) they're not marked "invited" and even if they were invited through the email system, "is_invited" is set to false once they confirm. So, you could just use any confirmed member - use get_confirmed_assocs_of_band_key instead.

There's some chance that you'll pick someone who has language set for 'english' though everyone else in their band uses 'Italian' - so the slack message will show the date in the English format. Definitely a corner case. We could add a default language for band announcements (I guess that would be for the RSS stream, too) if it turns out to be a problem for anyone.

@bklang

bklang commented Nov 4, 2017

Copy link
Copy Markdown
Contributor Author

Putting the preference on the Band probably makes the most sense, but I'm not sure how to handle a migration of existing data (to set a default locale). I think I'd prefer to address per-Band-locales in a separate PR. Alright with you if I make the logic work as-is?

@bklang bklang force-pushed the feature/slack_gig_notifications branch from 52ecd1e to 4fd7760 Compare November 5, 2017 00:03
@aaronoppenheimer

Copy link
Copy Markdown
Contributor

Yes about the band language preference - I'm sure it doesn't really matter and I'd put it on the bottom of the list!

@aaronoppenheimer

Copy link
Copy Markdown
Contributor

If I cancel out of the slack authorization process I get an error back on the gig-o...

File "/Users/aaron/src/go2/slack_client.py", line 31, in get_oauth_info json_response['error'])) Exception: Unable to retrieve OAuth access token due to 'invalid_code' error

@bklang

bklang commented Nov 5, 2017

Copy link
Copy Markdown
Contributor Author

@aaronoppenheimer Button added to disconnect from Slack:

captura de tela 2017-11-04 as 20 59 07

I think that's all the concerns addressed?

@bklang

bklang commented Nov 5, 2017

Copy link
Copy Markdown
Contributor Author

Ah, just saw your comment about canceling out of the Slack signup process.

It's easy enough to detect (that's where the error message you saw came from), but what's the convention for displaying error messages? I will improve the text of the message, but can you point to an example in the app of how you might display it?

@aaronoppenheimer

Copy link
Copy Markdown
Contributor

Hi Ben - I'm not sure it's an error. If I go to connect Slack but then cancel out, I think it's fine to just return to the edit page as if I hadn't pressed the connect button at all. Y'know?

And - if I make a bunch of changes to the page and then go to the slack connection path, I'll lose all of the changes I made - we lose the form contents and when I get back it's all reset. I wonder if there's a way to manage that? Maybe the "connect slack" button should be on the band info page. There's the link to the calendar feed - maybe band admins see the "connect to slack" option below that. Regular members could just see a message there like "the band is connected to Slack" once the connection is made.

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.

2 participants