-
Notifications
You must be signed in to change notification settings - Fork 233
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
base: master
Are you sure you want to change the base?
Conversation
@MetcalfeTom I've allocated some time to review your PR this week. |
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 |
knockknock/slack_sender.py
Outdated
dump["blocks"] = _starting_message( | ||
func_name, host_name, notification, start_time | ||
) | ||
dump["text"] = notification |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
knockknock/slack_sender.py
Outdated
func_name, host_name, notification, start_time | ||
) | ||
dump["text"] = notification | ||
dump["icon_emoji"] = ":clapper:" |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this (https://api.slack.com/bot-users#installing-bot) related?
There was a problem hiding this comment.
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).
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
There was a problem hiding this comment.
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:
- Forgo the profile image formatting
- 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.
@MetcalfeTom it looks great! |
@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 🚀 |
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 ofuser_id
s (perhaps I used this incorrectly, but posting theuser_id
never tagged the right person)The reports look like this now: