From 21587137b9df05a3f2e11227fafb1b8ecdd37b2c Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Tue, 3 Sep 2024 14:31:01 -0700 Subject: [PATCH 1/5] Truncate commit message during Discord webhook push events --- services/webhook/discord.go | 10 ++++++++-- services/webhook/discord_test.go | 15 +++++++++++++++ services/webhook/general_test.go | 10 +++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/services/webhook/discord.go b/services/webhook/discord.go index f27b39dac3716..5eb96e7b488ba 100644 --- a/services/webhook/discord.go +++ b/services/webhook/discord.go @@ -154,8 +154,14 @@ func (d discordConvertor) Push(p *api.PushPayload) (DiscordPayload, error) { var text string // for each commit, generate attachment text for i, commit := range p.Commits { - text += fmt.Sprintf("[%s](%s) %s - %s", commit.ID[:7], commit.URL, - strings.TrimRight(commit.Message, "\r\n"), commit.Author.Name) + // limit the commit message display to just the summary, otherwise it would be hard to read + message := strings.Split(commit.Message, "\n")[0] + + // a limit of 50 is set because GitHub does the same + if len(message) > 50 { + message = fmt.Sprintf("%.47s...", message) + } + text += fmt.Sprintf("[%s](%s) %s - %s", commit.ID[:7], commit.URL, message, commit.Author.Name) // add linebreak to each commit but the last if i < len(p.Commits)-1 { text += "\n" diff --git a/services/webhook/discord_test.go b/services/webhook/discord_test.go index c04b95383bf13..073350c01f14f 100644 --- a/services/webhook/discord_test.go +++ b/services/webhook/discord_test.go @@ -80,6 +80,21 @@ func TestDiscordPayload(t *testing.T) { assert.Equal(t, p.Sender.AvatarURL, pl.Embeds[0].Author.IconURL) }) + t.Run("PushWithLongCommitMessage", func(t *testing.T) { + p := pushTestPayloadMultilineCommitMessage() + + pl, err := dc.Push(p) + require.NoError(t, err) + + assert.Len(t, pl.Embeds, 1) + assert.Equal(t, "[test/repo:test] 2 new commits", pl.Embeds[0].Title) + assert.Equal(t, "[2020558](http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778) This is a commit summary that is longer than 50... - user1\n[2020558](http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778) This is a commit summary that is longer than 50... - user1", pl.Embeds[0].Description) + assert.Equal(t, "http://localhost:3000/test/repo/src/test", pl.Embeds[0].URL) + assert.Equal(t, p.Sender.UserName, pl.Embeds[0].Author.Name) + assert.Equal(t, setting.AppURL+p.Sender.UserName, pl.Embeds[0].Author.URL) + assert.Equal(t, p.Sender.AvatarURL, pl.Embeds[0].Author.IconURL) + }) + t.Run("Issue", func(t *testing.T) { p := issueTestPayload() diff --git a/services/webhook/general_test.go b/services/webhook/general_test.go index fc6e7140e7be0..0d9551ab8c1b7 100644 --- a/services/webhook/general_test.go +++ b/services/webhook/general_test.go @@ -64,9 +64,17 @@ func forkTestPayload() *api.ForkPayload { } func pushTestPayload() *api.PushPayload { + return pushTestPayloadWithCommitMessage("commit message") +} + +func pushTestPayloadMultilineCommitMessage() *api.PushPayload { + return pushTestPayloadWithCommitMessage("This is a commit summary that is longer than 50 characters\n\nThis is the message body.") +} + +func pushTestPayloadWithCommitMessage(message string) *api.PushPayload { commit := &api.PayloadCommit{ ID: "2020558fe2e34debb818a514715839cabd25e778", - Message: "commit message", + Message: message, URL: "http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778", Author: &api.PayloadUser{ Name: "user1", From af276c5b5490fe688edd06e6846b02e34e427bf5 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Wed, 4 Sep 2024 08:44:54 -0700 Subject: [PATCH 2/5] Trim away CR character if it exists This is in-line with what our other webhook implementations do. --- services/webhook/discord.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/webhook/discord.go b/services/webhook/discord.go index 5eb96e7b488ba..8c50dcf968951 100644 --- a/services/webhook/discord.go +++ b/services/webhook/discord.go @@ -155,7 +155,7 @@ func (d discordConvertor) Push(p *api.PushPayload) (DiscordPayload, error) { // for each commit, generate attachment text for i, commit := range p.Commits { // limit the commit message display to just the summary, otherwise it would be hard to read - message := strings.Split(commit.Message, "\n")[0] + message := strings.TrimRight(strings.Split(commit.Message, "\n")[0], "\r") // a limit of 50 is set because GitHub does the same if len(message) > 50 { From 8f4bd8eccd61f2311ddbbde62aa7d3a1c4c4523b Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Wed, 4 Sep 2024 12:08:21 -0700 Subject: [PATCH 3/5] Rename test fixture function --- services/webhook/discord_test.go | 2 +- services/webhook/general_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/services/webhook/discord_test.go b/services/webhook/discord_test.go index 073350c01f14f..201f82580d988 100644 --- a/services/webhook/discord_test.go +++ b/services/webhook/discord_test.go @@ -81,7 +81,7 @@ func TestDiscordPayload(t *testing.T) { }) t.Run("PushWithLongCommitMessage", func(t *testing.T) { - p := pushTestPayloadMultilineCommitMessage() + p := pushTestMultilineCommitMessagePayload() pl, err := dc.Push(p) require.NoError(t, err) diff --git a/services/webhook/general_test.go b/services/webhook/general_test.go index 0d9551ab8c1b7..5fa81050aa29e 100644 --- a/services/webhook/general_test.go +++ b/services/webhook/general_test.go @@ -67,7 +67,7 @@ func pushTestPayload() *api.PushPayload { return pushTestPayloadWithCommitMessage("commit message") } -func pushTestPayloadMultilineCommitMessage() *api.PushPayload { +func pushTestMultilineCommitMessagePayload() *api.PushPayload { return pushTestPayloadWithCommitMessage("This is a commit summary that is longer than 50 characters\n\nThis is the message body.") } From 3cc4fd65647d96abee1acae3650064174e023e39 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Thu, 12 Sep 2024 13:28:08 -0700 Subject: [PATCH 4/5] Consider unicode chars in impl and tests --- services/webhook/discord.go | 3 ++- services/webhook/discord_test.go | 3 +-- services/webhook/general_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/services/webhook/discord.go b/services/webhook/discord.go index 8c50dcf968951..ab7faaf870595 100644 --- a/services/webhook/discord.go +++ b/services/webhook/discord.go @@ -11,6 +11,7 @@ import ( "net/url" "strconv" "strings" + "unicode/utf8" webhook_model "code.gitea.io/gitea/models/webhook" "code.gitea.io/gitea/modules/git" @@ -158,7 +159,7 @@ func (d discordConvertor) Push(p *api.PushPayload) (DiscordPayload, error) { message := strings.TrimRight(strings.Split(commit.Message, "\n")[0], "\r") // a limit of 50 is set because GitHub does the same - if len(message) > 50 { + if utf8.RuneCountInString(message) > 50 { message = fmt.Sprintf("%.47s...", message) } text += fmt.Sprintf("[%s](%s) %s - %s", commit.ID[:7], commit.URL, message, commit.Author.Name) diff --git a/services/webhook/discord_test.go b/services/webhook/discord_test.go index 201f82580d988..fbb4b24ef12dd 100644 --- a/services/webhook/discord_test.go +++ b/services/webhook/discord_test.go @@ -88,8 +88,7 @@ func TestDiscordPayload(t *testing.T) { assert.Len(t, pl.Embeds, 1) assert.Equal(t, "[test/repo:test] 2 new commits", pl.Embeds[0].Title) - assert.Equal(t, "[2020558](http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778) This is a commit summary that is longer than 50... - user1\n[2020558](http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778) This is a commit summary that is longer than 50... - user1", pl.Embeds[0].Description) - assert.Equal(t, "http://localhost:3000/test/repo/src/test", pl.Embeds[0].URL) + assert.Equal(t, "[2020558](http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778) This is a commit summary ⚠️⚠️⚠️⚠️ containing 你好... - user1\n[2020558](http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778) This is a commit summary ⚠️⚠️⚠️⚠️ containing 你好... - user1", pl.Embeds[0].Description) assert.Equal(t, p.Sender.UserName, pl.Embeds[0].Author.Name) assert.Equal(t, setting.AppURL+p.Sender.UserName, pl.Embeds[0].Author.URL) assert.Equal(t, p.Sender.AvatarURL, pl.Embeds[0].Author.IconURL) diff --git a/services/webhook/general_test.go b/services/webhook/general_test.go index 5fa81050aa29e..d6a77c9442560 100644 --- a/services/webhook/general_test.go +++ b/services/webhook/general_test.go @@ -68,7 +68,7 @@ func pushTestPayload() *api.PushPayload { } func pushTestMultilineCommitMessagePayload() *api.PushPayload { - return pushTestPayloadWithCommitMessage("This is a commit summary that is longer than 50 characters\n\nThis is the message body.") + return pushTestPayloadWithCommitMessage("This is a commit summary ⚠️⚠️⚠️⚠️ containing 你好 ⚠️⚠️️\n\nThis is the message body.") } func pushTestPayloadWithCommitMessage(message string) *api.PushPayload { From 0983b27d0dac9cbb179184eaf5e584b6ae7cd684 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Tue, 17 Sep 2024 16:02:12 -0700 Subject: [PATCH 5/5] Create at most just 1 substring when splitting --- services/webhook/discord.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/webhook/discord.go b/services/webhook/discord.go index ab7faaf870595..59e87a7e1fb67 100644 --- a/services/webhook/discord.go +++ b/services/webhook/discord.go @@ -156,7 +156,7 @@ func (d discordConvertor) Push(p *api.PushPayload) (DiscordPayload, error) { // for each commit, generate attachment text for i, commit := range p.Commits { // limit the commit message display to just the summary, otherwise it would be hard to read - message := strings.TrimRight(strings.Split(commit.Message, "\n")[0], "\r") + message := strings.TrimRight(strings.SplitN(commit.Message, "\n", 1)[0], "\r") // a limit of 50 is set because GitHub does the same if utf8.RuneCountInString(message) > 50 {