-
Notifications
You must be signed in to change notification settings - Fork 640
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 back support for Slack attachments #148
Conversation
I should add, this relies on slackapi/node-slack-sdk#15 being merged before it will work... |
customMessage: (data) => | ||
channel = @client.getChannelGroupOrDMByName data.message.envelope.room | ||
|
||
msg = [] |
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.
Should this be an object instead of an array? Below msg.attachments
is set, which actually does work as "expected" even though it's an array (because arrays are objects) but it is pretty unusual.
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.
API aren't explicitly clear on the format, but everything suggest that an object is needed, so I think @evansolomon is right.
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.
Yep! Thanks for pointing this out, it's fixed in 471e71d
I'm not entirely clear on what |
if data.icon_url? | ||
msg.icon_url = data.icon_url | ||
else if data.icon_emoji? | ||
msg.icon_emoji = data.icon_emoji |
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.
You don't seem to actually be adding data.username
to the msg
here. Surely that's necessary or it won't know what name to post as.
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.
Unless it's getting the user name from the authentication token, which might make sense, but explicitly setting the username makes even more sense, IMO.
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.
as_user
causes the message to get the name from the authentication token.
This is because behind the scenes as_user
causes the message to be sent as a standard message instead of a bot_message, which means the message is correctly attributed (among many other things).
@paulhammond this is really good news, that attachments support is enabled back in the main library. My only concern is relying on an undocumented feature. |
would love to see it merged into master |
A general feedback, based on how we used slack attachments in hubot in the old version is that we changed the adapter (related note is that our internal scripts often reply with an object that will return different representation for different adapters) |
Add back support for Slack attachments
Does this mean that Either way, this is great news 😺 |
@inkel @paulhammond is there a way to set fallback? When I have notifications set the message is "NO FALLBACK DEFINED" no matter what I add to the payload. |
@scott2449 could you paste a code snippet of the payload you're using? I could use that to check how am I doing it in |
I tried what you said in the docs and set fallback as such, maybe it's not carried to the PR:
|
also tried setting message |
NM I had it outside of content |
👍 |
This pull request reimplements slack attachment support in Hubot.
It does so using an as-yet undocumented Slack API, where chat.postMessage will now accept an
as_user
boolean argument. Even though this API has been deployed to our production servers, it should be considered provisional and the details may change. We'll be documenting it on api.slack.com and confirm here when we're confident that the design is right.I believe this is compatible with both scripts that used hubot-slack v2's attachment support, and scripts that use @inkel's hubot-slack-attachment script. But, I don't have many test cases, so I may have missed something.
I'm looking for feedback on whether this approach makes sense, and bug reports where the attachment support doesn't work with existing scripts.