lunny/gitea

Archived

Fix RocketChat webhook #9908

Merged

lunny merged 5 commits from rocketchat into master

2020-01-21 20:29:25 +00:00

jolheiser commented

2020-01-21 05:13:32 +00:00

Contributor

Resolves #9899

~~I'm fairly certain this should fix rocket chat webhooks for push actions.~~
~~I do not have a rocket chat instance to confirm on, however, so this PR should probably not be accepted until someone can confirm.~~

Turns out rocket chat has a free cloud trial, so I can confirm it works.
I also just removed attachments altogether for repo create/delete event, as it was always blank. 🤷‍♂️

(Repo event) With blank attachment

(Repo event) Without

(Push event) Rocket Chat

(Repo events and push event) Slack

Resolves #9899 ~~I'm fairly certain this should fix rocket chat webhooks for push actions.~~ ~~I do not have a rocket chat instance to confirm on, however, so this PR should probably not be accepted until someone can confirm.~~ Turns out rocket chat has a free cloud trial, so I can confirm it works. I also just removed attachments altogether for repo create/delete event, as it was always blank. :man_shrugging: (Repo event) With blank attachment ![Screenshot from 2020-01-20 23-43-13](https://user-images.githubusercontent.com/42128690/72778459-b09bcb00-3bde-11ea-840c-696156a2d180.png) (Repo event) Without ![Screenshot from 2020-01-20 23-43-19](https://user-images.githubusercontent.com/42128690/72778460-b09bcb00-3bde-11ea-9818-33719aa63d43.png) (Push event) Rocket Chat ![Screenshot from 2020-01-20 23-44-38](https://user-images.githubusercontent.com/42128690/72778575-f193df80-3bde-11ea-863a-96d2323ff9fa.png) (Repo events and push event) Slack ![Screenshot from 2020-01-20 23-40-48](https://user-images.githubusercontent.com/42128690/72778398-78948800-3bde-11ea-96db-cc83107a7c4b.png)

guillep2k (Migrated from github.com) reviewed 2020-01-21 05:21:09 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

guillep2k (Migrated from github.com) commented

2020-01-21 05:20:05 +00:00

An URL for a title? I expected some plain text (https://api.slack.com/reference/messaging/attachments).

An URL for a title? I expected some plain text (https://api.slack.com/reference/messaging/attachments).

jolheiser reviewed 2020-01-21 05:23:03 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

jolheiser commented

2020-01-21 05:23:03 +00:00

Author

Contributor

Do you have a suggestion for that text? Other payloads have issue titles or some such, but I didn't know of a good one for this, unless I just re-use x new commits pushed or something.

Do you have a suggestion for that text? Other payloads have issue titles or some such, but I didn't know of a good one for this, unless I just re-use `x new commits pushed` or something.

guillep2k (Migrated from github.com) reviewed 2020-01-21 05:58:48 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

guillep2k (Migrated from github.com) commented

2020-01-21 05:58:48 +00:00

No idea, but if the attachment links to the repo itself (which is weird, maybe?), perhaps the repo name should be used?

I'm under the impression that it should better to remove the property from the payload when empty rather than sending improper data. ?

No idea, but if the attachment links to the repo itself (which is weird, maybe?), perhaps the repo name should be used? I'm under the impression that it should better to remove the property from the payload when empty rather than sending improper data. ?

jolheiser reviewed 2020-01-21 06:05:58 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

jolheiser commented

2020-01-21 06:05:58 +00:00

Author

Contributor

I'd agree with not sending anything, but that seems to be what caused the bug for whatever reason.

It could be repo name, but even that is sort of dupe data. Better than a URL, though? ?

I'd agree with not sending anything, but that seems to be what caused the bug for whatever reason. It could be repo name, but even that is sort of dupe data. Better than a URL, though? ?

guillep2k (Migrated from github.com) reviewed 2020-01-21 06:06:23 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

guillep2k (Migrated from github.com) commented

2020-01-21 06:06:23 +00:00

I'm not a webhook user; not even a Slack user, so please take my opinion with a grain of salt. From what I read from the docs, I think we're using the attachment section to produce a "Read more" action. It looks like it should not have a link at all, and if any title, it should be.... "More details" or something like that (but it would be redundant because I bet Slack already gives you the "Read more").

I'm not a webhook user; not even a Slack user, so please take my opinion with a grain of salt. From what I [read](https://api.slack.com/reference/messaging/attachments) from the [docs](https://api.slack.com/messaging/composing/layouts#when-to-use-attachments), I think we're using the attachment section to produce a "Read more" action. It looks like it should not have a link at all, and if any title, it should be.... "More details" or something like that (but it would be redundant because I bet Slack already gives you the "Read more").

jolheiser reviewed 2020-01-21 06:09:08 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

jolheiser commented

2020-01-21 06:09:07 +00:00

Author

Contributor

Bars on my screenshots above, "see more" works in RC but possibly not Slack, since RC allows you to hide it but Slack appears to show all of it no matter what

Bars on my screenshots above, "see more" works in RC but possibly not Slack, since RC allows you to hide it but Slack appears to show all of it no matter what

jolheiser reviewed 2020-01-21 06:10:03 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

jolheiser commented

2020-01-21 06:10:02 +00:00

Author

Contributor

But I am also not a Slack or RC user, I just have some workspaces for testing webhooks lately.

But I am also not a Slack or RC user, I just have some workspaces for testing webhooks lately.

guillep2k (Migrated from github.com) reviewed 2020-01-21 06:22:17 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

guillep2k (Migrated from github.com) commented

2020-01-21 06:22:17 +00:00

https://api.slack.com/messaging/composing/layouts#when-to-use-attachments

Any content displayed within attachments may be wrapped, truncated, or hidden behind a "show more" style option by Slack clients.

So I guess it depends on the client.

https://api.slack.com/messaging/composing/layouts#when-to-use-attachments > > Any content displayed within attachments may be wrapped, truncated, or hidden behind a "show more" style option by Slack clients. So I guess it depends on the client.

zeripath approved these changes 2020-01-21 08:02:17 +00:00

zeripath left a comment

Contributor

One query, but otherwise lgtm.

One query, but otherwise lgtm.

modules/webhook/slack.go

zeripath commented

2020-01-21 08:01:53 +00:00

Contributor

Is this right?

Is this right?

jolheiser reviewed 2020-01-21 13:50:35 +00:00

modules/webhook/slack.go

jolheiser commented

2020-01-21 13:50:34 +00:00

Author

Contributor

@zeripath Yeah, if you look at the images all this used to do was add an attachment with no actual content, just a title with the repo URL (which is already part of repoLink)
It also broke the hook for delete events because title was only ever given a value here.

@zeripath Yeah, if you look at the images all this used to do was add an attachment with no actual content, just a title with the repo URL (which is already part of `repoLink`) It also broke the hook for delete events because title was only ever given a value here.

jolheiser reviewed 2020-01-21 16:31:32 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

jolheiser commented

2020-01-21 16:31:32 +00:00

Author

Contributor

For the record I am not against changing this, I just really can't think of a good title.
An empty title would break the webhook (at least for RC, not sure about Slack or mattermost)

For the record I am not against changing this, I just really can't think of a good title. An empty title would break the webhook (at least for RC, not sure about Slack or mattermost)

bkraul (Migrated from github.com) reviewed 2020-01-21 16:44:18 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

bkraul (Migrated from github.com) commented

2020-01-21 16:44:18 +00:00

Why not open it up as an option with a default? Maybe more trouble than it's worth?

Why not open it up as an option with a default? Maybe more trouble than it's worth?

jolheiser reviewed 2020-01-21 16:46:09 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

jolheiser commented

2020-01-21 16:46:09 +00:00

Author

Contributor

Since it only applies to this payload, I don't know that it would be worth it.
Other payloads have something meaningful for this title.

Since it only applies to this payload, I don't know that it would be worth it. Other payloads have something meaningful for this title.

6543 (Migrated from github.com) approved these changes 2020-01-21 17:19:06 +00:00

lafriks (Migrated from github.com) approved these changes 2020-01-21 17:25:59 +00:00

guillep2k (Migrated from github.com) reviewed 2020-01-21 19:16:04 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

guillep2k (Migrated from github.com) commented

2020-01-21 19:16:03 +00:00

What about "details"? The problem is of course l10n....
Anyway, I'm not in the least against leaving it like this if nobody complains. ?

What about "details"? The problem is of course l10n.... Anyway, I'm not in the least against leaving it like this if nobody complains. ?

jolheiser reviewed 2020-01-21 19:20:46 +00:00

modules/webhook/slack.go

						
				@ -232,8 +232,10 @@ func getSlackPushPayload(p *api.PushPayload, slack *SlackMeta) (*SlackPayload, e

						Username: slack.Username,

jolheiser commented

2020-01-21 19:20:45 +00:00

Author

Contributor

Agreed. Maybe it can be updated in future PR.

Agreed. Maybe it can be updated in future PR.

This repo is archived. You cannot comment on pull requests.

No reviewers

No Label

good first issue

hacktoberfest-accepted

kind/deployment

kind/deprecated

kind/enhancement

kind/moderation

kind/regression

kind/translation

kind/upstream-related

performance/bigrepo

performance/cpu

performance/memory

performance/speed

priority/critical

priority/medium

proposal/rejected

reviewed/confirmed

reviewed/duplicate

reviewed/invalid

reviewed/not-a-bug

reviewed/wontfix

status/needs-feedback

theme/authentication

theme/backup-restore

theme/federation

theme/migration

theme/timetracker

No Milestone

No project

No Assignees

3 Participants

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: lunny/gitea#9908

No description provided.