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

PLT-3658 Support for Slack attachments in outgoing webhook response (#7774) #8102

Merged
merged 1 commit into from Jan 26, 2018

Conversation

yeoji
Copy link
Contributor

@yeoji yeoji commented Jan 13, 2018

Summary

This PR adds support for Slack attachments for outgoing webhook responses.
It also adds support for mentions with <@userid> and announcement tokens (eg. <!here>) in the outgoing webhook responses as implemented in #7615

Ticket Link

The GitHub issue opened for this ticket is #7774

app/webhook.go Outdated
@@ -122,7 +122,15 @@ func (a *App) TriggerWebhook(payload *model.OutgoingWebhookPayload, hook *model.
webhookResp.Props = make(model.StringInterface)
}
webhookResp.Props["webhook_display_name"] = hook.DisplayName
if _, err := a.CreateWebhookPost(hook.CreatorId, channel, *webhookResp.Text, webhookResp.Username, webhookResp.IconURL, webhookResp.Props, webhookResp.Type, postRootId); err != nil {

text := a.ProcessSlackText(*webhookResp.Text)
Copy link
Contributor Author

@yeoji yeoji Jan 13, 2018

Choose a reason for hiding this comment

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

I noticed that the text in the webhook response isn't being processed as a Slack text, so I added this here. It parses tokens like <!here>, <!all>, <@ userid>. If it is not relevant or needed, I can remove it.

@jasonblais jasonblais added the 1: PM Review Requires review by a product manager label Jan 13, 2018
@jasonblais jasonblais self-assigned this Jan 13, 2018
@jasonblais
Copy link
Contributor

@ccbrown @jwilander Wondering if either of you have suggestion on the best way to test this?

@yeoji Huge thanks for this feature! Much needed for fully compatible outgoing webhooks

@mattermod
Copy link
Contributor

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@jasonblais jasonblais added the Setup Old Test Server Triggers the creation of a test server label Jan 13, 2018
@mattermod
Copy link
Contributor

Spinmint test server created at: http://i-0b670d23f180099b9.spinmint.com

Test Account 1: Email: test@test.com | Password: passwd

Test Account 2: Email: test2@test.com | Password: passwd

Instance ID: i-0b670d23f180099b9

@ccbrown
Copy link
Contributor

ccbrown commented Jan 15, 2018

@jasonblais For manual/spinmint testing, you might be able to just upload a sample response to S3 or somewhere and use that for the hook. (I would go ahead and just do this for you, but I'm off today and away from the computer.) But of course, the best thing would be to get some unit tests for this. :P

Copy link
Contributor

@ccbrown ccbrown left a comment

Choose a reason for hiding this comment

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

Assuming someone tests this, lgtm. I'm tempted to hit the "request changes" button for a unit test, but I'll let @jwilander be the decider on that.

@yeoji
Copy link
Contributor Author

yeoji commented Jan 16, 2018

If it helps, I used Mockbin. The official service isn't working, but there's one hosted on https://requestloggerbin.herokuapp.com/bin/create

@jasonblais
Copy link
Contributor

@ccbrown @jwilander Would either of you be able to help with the testing?

@jwilander
Copy link
Member

@jasonblais I looked at it

@yeoji it looks like if the response only contains an attachment then it doesn't work, otherwise it seems to work well. Would you be able to update it so that if a response has only an attachment, it still works?

@yeoji
Copy link
Contributor Author

yeoji commented Jan 24, 2018

Hi @jwilander, I have updated the PR, it should now work when the response does not contain text

@jwilander jwilander added 2: Dev Review Requires review by a developer and removed 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Jan 24, 2018
@mattermod
Copy link
Contributor

Spinmint test server destroyed

@jwilander
Copy link
Member

@yeoji if you give this a rebase I can merge it in

@jwilander jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a developer labels Jan 24, 2018
@yeoji
Copy link
Contributor Author

yeoji commented Jan 24, 2018

@jwilander Done :)

@jwilander jwilander merged commit 1c7f257 into mattermost:master Jan 26, 2018
@jwilander
Copy link
Member

Thanks @yeoji !

@yeoji yeoji deleted the PLT-3658 branch January 29, 2018 20:44
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Needed Requires documentation labels Feb 1, 2018
@amyblais amyblais self-assigned this Feb 6, 2018
@amyblais amyblais added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Feb 7, 2018
@lindalumitchell lindalumitchell added the Tests/Done Required release tests have been written label Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Done Required documentation has been written Tests/Done Required release tests have been written
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants