Skip to content

add slack notification integration#19

Open
roozbeh wants to merge 3 commits intoIrma-Response:masterfrom
roozbeh:roozbeh/issue-18
Open

add slack notification integration#19
roozbeh wants to merge 3 commits intoIrma-Response:masterfrom
roozbeh:roozbeh/issue-18

Conversation

@roozbeh
Copy link
Copy Markdown

@roozbeh roozbeh commented Sep 9, 2017

To handle problem mentioned: #18

production:
secret_key_base: <%= ENV["SECRET_KEY_BASE"] %>
slack_token: "xoxp-235986621104-238493557106-238649524195-b886dacfa83620d7c79b483114198cf9"
slack_channel: "#shelters"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gonna get way too noisy. We need a different channel for this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I created shelters_notif for this purpose.


res = Net::HTTP.get_response(uri)
if res.is_a?(Net::HTTPSuccess)
puts "Slack response: #{res.body}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no logger in application.rb so does all puts go to heroku logging?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. Log removed.

@tdooner
Copy link
Copy Markdown

tdooner commented Sep 10, 2017

I was chatting with @roozbeh about this in DM, but let me just put some thoughts here. I think this is a good idea, so that a slack channel can be used to remind us that updates are happening in the API.

But, I think it is going to be very noisy at our current velocity, which would mean that everyone would just leave the updates channel, thus undermining the feature.

Coupled with the fact that this is not tested at all nor does it contain any error handling, I'm going to say we should wait to merge this until after the storm passes.


if draft.save
redirect_to draft, notice: 'Your new shelter is pending approval.'
send_slack_notification "New shelter added: https://irma-api.herokuapp.com/drafts/#{draft.id}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some Slack formatting would add a nice touch here. Maybe include draft.record.shelter to include the text from the Shelter field like:

send_slack_notification "New shelter added in <https://irma-api.herokuapp.com/drafts/#{draft.id}|#{draft.record.shelter}>"

You could also make it a call to action, ie "Admins, please review new shelter in https://irma-api.herokuapp.com/drafts/#{draft.id}|#{draft.record.shelter}"


if draft.save
redirect_to draft, notice: 'Your new shelter is pending approval.'
send_slack_notification "New shelter added: https://irma-api.herokuapp.com/drafts/#{draft.id}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use draft_url(draft), that should generate a URL that works in all environments, not just production.

params = { :token => Rails.application.secrets.slack_token, :channel => Rails.application.secrets.slack_channel, :text => msg, :pretty => 1 }
uri.query = URI.encode_www_form(params)

Net::HTTP.get_response(uri)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a slack ruby client you can consider using, instead of making the calls directly.

development:
secret_key_base: d22cd95910c4425ab8adfd2aa968b845775c6ffff23f9cdb337bc6f210199ca97736ed261d06bf69242f6483a8e6c474ab88f02dabb9865f9911ff215da31695

slack_token: "xoxp-235986621104-238493557106-238649524195-b886dacfa83620d7c79b483114198cf9"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a real token? It might be better as an environment variable like SECRET_KEY_BASE below. That would make sense for all the environments. Someone with Heroku access to the app would need to set this up before deploying.

The secret should be rolled as well.

secret_key_base: d22cd95910c4425ab8adfd2aa968b845775c6ffff23f9cdb337bc6f210199ca97736ed261d06bf69242f6483a8e6c474ab88f02dabb9865f9911ff215da31695

slack_token: "xoxp-235986621104-238493557106-238649524195-b886dacfa83620d7c79b483114198cf9"
slack_channel: "#shelters_notif"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be good as a environment variable too, even if it isn't a secret per se. That'd make it easy to test in development.

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.

4 participants