Skip to content

Discord Webhooks should hide multiline commit details #32371

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

Closed
quentinguidee opened this issue Oct 29, 2024 · 1 comment · Fixed by #32432
Closed

Discord Webhooks should hide multiline commit details #32371

quentinguidee opened this issue Oct 29, 2024 · 1 comment · Fixed by #32432

Comments

@quentinguidee
Copy link
Contributor

Description

While #31668 was resolved (thanks!), the messages of Discord webhooks is still hard/harder to read in some cases like this one :

Capture d’écran, le 2024-10-29 à 19 30 32

Now, the message is truncated after 50 characters like GitHub, but I think that only the first line of the message should be displayed. See GitHub for comparison (these are also multiline commits) :

image

Gitea Version

1.22.3

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

No response

Screenshots

No response

Git Version

2.30.2

Operating System

Linux

How are you running Gitea?

Official docker image

Database

PostgreSQL

@yp05327 yp05327 added type/proposal The new feature has not been accepted yet but needs to be discussed first. and removed type/bug labels Nov 4, 2024
@kemzeb
Copy link
Member

kemzeb commented Nov 6, 2024

Thanks for the report! This is unattended behavior... #31970 should have removed the commit description and just kept the summary:

message := strings.TrimRight(strings.SplitN(commit.Message, "\n", 1)[0], "\r")

Here we split the commit message with \n as the delimiter and we only choose the first line. It also removes any trailing /r if it exists.

Looking at the Go docs further, I may have misused SplitN() and should have done:

strings.SplitN(commit.Message, "\n", 2)[0]

... since the last element would be the "unsplit remainder" that the docs talk about. Currently we seem to be using the unsplit remainder (which would just be the message). Reading the code of strings.SplitN() also points to this being the case. That PR also had a unit test, but didn't catch this edge case since the commit summary was > 50 (while yours is < 50).

Not sure how I missed this. I'll be making a PR to correct this.

@kemzeb kemzeb added type/bug topic/webhooks and removed type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Nov 6, 2024
@lunny lunny added this to the 1.22.4 milestone Nov 6, 2024
@lunny lunny closed this as completed in fb03062 Nov 7, 2024
GiteaBot pushed a commit to GiteaBot/gitea that referenced this issue Nov 7, 2024
…itea#32432)

Resolves go-gitea#32371.

go-gitea#31970 should have just showed the commit summary, but
`strings.SplitN()` was misused such that we did not perform any
splitting at all and just used the message. This was not caught in the
unit test made in that PR since the test commit summary was > 50 (which
truncated away the commit description).

This snapshot resolves this and adds another unit test to ensure that we
only show the commit summary.
lunny pushed a commit that referenced this issue Nov 8, 2024
…) (#32447)

Backport #32432 by @kemzeb

Resolves #32371.

#31970 should have just showed the commit summary, but
`strings.SplitN()` was misused such that we did not perform any
splitting at all and just used the message. This was not caught in the
unit test made in that PR since the test commit summary was > 50 (which
truncated away the commit description).

This snapshot resolves this and adds another unit test to ensure that we
only show the commit summary.

Co-authored-by: Kemal Zebari <[email protected]>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Feb 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants