Skip to content
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

Update slack notification formatting #34

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

MetcalfeTom
Copy link

Happy holidays! 🎅🏼
I thought the slack message formatting looked a little sad, so I updated it. I used the block kit builder to update the messages.

I also updated the slack_sender to use usernames instead of user_ids (perhaps I used this incorrectly, but posting the user_id never tagged the right person)

The reports look like this now:

Screenshot 2019-12-26 at 7 11 25 pm

@VictorSanh
Copy link
Contributor

@MetcalfeTom I've allocated some time to review your PR this week.
Something I am thinking about in the background is refactoring: a few of these platforms (Slack, Discord, MSFT Teams, etc.) use the same communication protocol (via web hook), so I'm wondering if I can do some refactoring here (it should be possible), and if so, whether this formatting update would be compatible with other platforms than Slack.

@MetcalfeTom
Copy link
Author

Cool

Teams supports a similar formatting feature to blocks called cards. Discord does not have any kind of fancy formats. In all three cases the JSON payloads would still differ

dump["blocks"] = _starting_message(
func_name, host_name, notification, start_time
)
dump["text"] = notification
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need the notification here too? It seems to me that it's already contained in the block field.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, removing it from here means that the message text will not display in desktop notifications

func_name, host_name, notification, start_time
)
dump["text"] = notification
dump["icon_emoji"] = ":clapper:"
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something on the block builder web demo: I couldn't display this emoji (error with this line it seems).

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately I found some bad news about this on the slack webhook docs

You cannot override the default channel (chosen by the user who installed your app), username, or icon when you're using Incoming Webhooks to post messages. Instead, these values will always inherit from the associated Slack app configuration.

I tried some workarounds for this, but it seems that updating the name/emoji are only possible using the new api, which would change the configuration of this sender.

As a result, I removed dump and the channel parameter from this file entirely

Copy link
Contributor

Choose a reason for hiding this comment

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

I have difficulties understanding: where is your message sent then (on which channel)? is it encoded in the URL in an identifiable way?
As far as possible, I think it's nice to have an identifiable bot sending messages (instead of yourself to yourself), something that the current version is handling well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

@MetcalfeTom MetcalfeTom Mar 17, 2020

Choose a reason for hiding this comment

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

Since knockknock only uses the webhook sender for slack, the bot user is set when initialising the app, and the channel is selected specifically for each webhook (as below).

Screenshot 2020-03-17 at 10 23 22 pm

Messages sent to this webhook are still posted as the bot, but only to one channel, and with the same username and profile image every time. This functionality looks like it has been deprecated for webhooks, and is only possible with the new API.

I can update the slack sender to use the new API if you wish, but it will bring it further away from the refactoring you mentioned earlier

Copy link
Author

Choose a reason for hiding this comment

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

@VictorSanh How would you like to proceed? I believe the choices are:

  1. Forgo the profile image formatting
  2. Update this sender with a bot user account, and use chat.postMessage to specify channel and profile image (and potentially more functionality). This would also mean updating the documentation for the new slack token setup.

@VictorSanh
Copy link
Contributor

@MetcalfeTom it looks great!
I added a few question/comments/concerns.
For now, I'll suggest we push this formatting update only for Slack as it does not generalize to all platforms using webhook as a communication protocol.

@MetcalfeTom
Copy link
Author

@VictorSanh Thanks for taking the time to review! 🙌

I followed up on your comments and added your suggestions. Not sure you'd construct the refactor between this, discord and teams, but following that PR I would gladly take a stab at updating the Teams format to use cards too 🚀

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