From 11f43139f5aa4a340a998d56d2c6188e6ccd8247 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 9 May 2025 21:31:07 -0700 Subject: [PATCH 1/5] When updating comment, if the content is the same, just return and not update the databse --- routers/api/v1/repo/issue_comment.go | 5 ++ routers/web/repo/issue_comment.go | 8 +++ tests/integration/repo_webhook_test.go | 73 +++++++++++++++++++++----- 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 0c572a06a8273..4f83a3621263e 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -610,6 +610,11 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) } oldContent := comment.Content + if form.Body == oldContent { + ctx.JSON(http.StatusOK, convert.ToAPIComment(ctx, ctx.Repo.Repository, comment)) + return + } + comment.Content = form.Body if err := issue_service.UpdateComment(ctx, comment, comment.ContentVersion, ctx.Doer, oldContent); err != nil { if errors.Is(err, user_model.ErrBlockedUser) { diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index 8adce26ccc2aa..64dc1cee28ec2 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -242,6 +242,14 @@ func UpdateCommentContent(ctx *context.Context) { oldContent := comment.Content newContent := ctx.FormString("content") contentVersion := ctx.FormInt("content_version") + if newContent == oldContent { + ctx.JSON(http.StatusOK, map[string]any{ + "content": oldContent, + "contentVersion": comment.ContentVersion, + "attachments": attachmentsHTML(ctx, comment.Attachments, oldContent), + }) + return + } // allow to save empty content comment.Content = newContent diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index 89df15b8de8c3..ebb654d4a7ce5 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -241,19 +241,68 @@ func Test_WebhookIssueComment(t *testing.T) { testAPICreateWebhookForRepo(t, session, "user2", "repo1", provider.URL(), "issue_comment") - // 2. trigger the webhook - issueURL := testNewIssue(t, session, "user2", "repo1", "Title2", "Description2") - testIssueAddComment(t, session, issueURL, "issue title2 comment1", "") + t.Run("create comment", func(t *testing.T) { + // 2. trigger the webhook + issueURL := testNewIssue(t, session, "user2", "repo1", "Title2", "Description2") + testIssueAddComment(t, session, issueURL, "issue title2 comment1", "") + + // 3. validate the webhook is triggered + assert.Equal(t, "issue_comment", triggeredEvent) + assert.Len(t, payloads, 1) + assert.EqualValues(t, "created", payloads[0].Action) + assert.Equal(t, "repo1", payloads[0].Issue.Repo.Name) + assert.Equal(t, "user2/repo1", payloads[0].Issue.Repo.FullName) + assert.Equal(t, "Title2", payloads[0].Issue.Title) + assert.Equal(t, "Description2", payloads[0].Issue.Body) + assert.Equal(t, "issue title2 comment1", payloads[0].Comment.Body) + }) - // 3. validate the webhook is triggered - assert.Equal(t, "issue_comment", triggeredEvent) - assert.Len(t, payloads, 1) - assert.EqualValues(t, "created", payloads[0].Action) - assert.Equal(t, "repo1", payloads[0].Issue.Repo.Name) - assert.Equal(t, "user2/repo1", payloads[0].Issue.Repo.FullName) - assert.Equal(t, "Title2", payloads[0].Issue.Title) - assert.Equal(t, "Description2", payloads[0].Issue.Body) - assert.Equal(t, "issue title2 comment1", payloads[0].Comment.Body) + t.Run("update comment", func(t *testing.T) { + payloads = make([]api.IssueCommentPayload, 0, 2) + triggeredEvent = "" + + // 2. trigger the webhook + issueURL := testNewIssue(t, session, "user2", "repo1", "Title3", "Description3") + commentID := testIssueAddComment(t, session, issueURL, "issue title3 comment1", "") + modifiedContent := "issue title2 comment1 - modified" + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d", "user2", "repo1", commentID), map[string]string{ + "_csrf": GetUserCSRFToken(t, session), + "content": modifiedContent, + }) + session.MakeRequest(t, req, http.StatusOK) + + // 3. validate the webhook is triggered + assert.Equal(t, "issue_comment", triggeredEvent) + assert.Len(t, payloads, 2) + assert.EqualValues(t, "edited", payloads[1].Action) + assert.Equal(t, "repo1", payloads[1].Issue.Repo.Name) + assert.Equal(t, "user2/repo1", payloads[1].Issue.Repo.FullName) + assert.Equal(t, "Title3", payloads[1].Issue.Title) + assert.Equal(t, "Description3", payloads[1].Issue.Body) + assert.Equal(t, modifiedContent, payloads[1].Comment.Body) + }) + + t.Run("Update comment with no content change", func(t *testing.T) { + payloads = make([]api.IssueCommentPayload, 0, 2) + triggeredEvent = "" + commentContent := "issue title3 comment1" + + // 2. trigger the webhook + issueURL := testNewIssue(t, session, "user2", "repo1", "Title3", "Description3") + commentID := testIssueAddComment(t, session, issueURL, commentContent, "") + + payloads = make([]api.IssueCommentPayload, 0, 2) + triggeredEvent = "" + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d", "user2", "repo1", commentID), map[string]string{ + "_csrf": GetUserCSRFToken(t, session), + "content": commentContent, + }) + session.MakeRequest(t, req, http.StatusOK) + + // 3. validate the webhook is not triggered because no content change + assert.Equal(t, "", triggeredEvent) + assert.Len(t, payloads, 0) + }) }) } From fb83abe39c00f9f5d0a1f519eb84652d18b66221 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 9 May 2025 22:00:38 -0700 Subject: [PATCH 2/5] Improvements --- routers/web/repo/issue_comment.go | 19 +++++++++++++++---- tests/integration/repo_webhook_test.go | 4 ++-- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index 64dc1cee28ec2..9c917ff55b6c7 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -239,20 +239,31 @@ func UpdateCommentContent(ctx *context.Context) { return } - oldContent := comment.Content newContent := ctx.FormString("content") contentVersion := ctx.FormInt("content_version") - if newContent == oldContent { + if newContent == comment.Content { + if contentVersion != comment.ContentVersion { + ctx.JSONError(ctx.Tr("repo.comments.edit.already_changed")) + return + } + + if err := comment.LoadAttachments(ctx); err != nil { + ctx.ServerError("LoadAttachments", err) + return + } + ctx.JSON(http.StatusOK, map[string]any{ - "content": oldContent, + "content": comment.Content, "contentVersion": comment.ContentVersion, - "attachments": attachmentsHTML(ctx, comment.Attachments, oldContent), + "attachments": attachmentsHTML(ctx, comment.Attachments, comment.Content), }) return } // allow to save empty content comment.Content = newContent + oldContent := comment.Content + if err = issue_service.UpdateComment(ctx, comment, contentVersion, ctx.Doer, oldContent); err != nil { if errors.Is(err, user_model.ErrBlockedUser) { ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user")) diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index ebb654d4a7ce5..5ac0a9298e414 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -300,8 +300,8 @@ func Test_WebhookIssueComment(t *testing.T) { session.MakeRequest(t, req, http.StatusOK) // 3. validate the webhook is not triggered because no content change - assert.Equal(t, "", triggeredEvent) - assert.Len(t, payloads, 0) + assert.Empty(t, triggeredEvent) + assert.Zero(t, payloads) }) }) } From bb1657a6e2f687afd2c96fa46d1dbddc986c661a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 9 May 2025 22:10:21 -0700 Subject: [PATCH 3/5] Improvements --- routers/web/repo/issue_comment.go | 42 +++++++++++-------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index 9c917ff55b6c7..9b51999fbde6c 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -241,38 +241,26 @@ func UpdateCommentContent(ctx *context.Context) { newContent := ctx.FormString("content") contentVersion := ctx.FormInt("content_version") - if newContent == comment.Content { - if contentVersion != comment.ContentVersion { - ctx.JSONError(ctx.Tr("repo.comments.edit.already_changed")) - return - } - - if err := comment.LoadAttachments(ctx); err != nil { - ctx.ServerError("LoadAttachments", err) - return - } - - ctx.JSON(http.StatusOK, map[string]any{ - "content": comment.Content, - "contentVersion": comment.ContentVersion, - "attachments": attachmentsHTML(ctx, comment.Attachments, comment.Content), - }) + if contentVersion != comment.ContentVersion { + ctx.JSONError(ctx.Tr("repo.comments.edit.already_changed")) return } - // allow to save empty content - comment.Content = newContent - oldContent := comment.Content + if newContent != comment.Content { + // allow to save empty content + oldContent := comment.Content + comment.Content = newContent - if err = issue_service.UpdateComment(ctx, comment, contentVersion, ctx.Doer, oldContent); err != nil { - if errors.Is(err, user_model.ErrBlockedUser) { - ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user")) - } else if errors.Is(err, issues_model.ErrCommentAlreadyChanged) { - ctx.JSONError(ctx.Tr("repo.comments.edit.already_changed")) - } else { - ctx.ServerError("UpdateComment", err) + if err = issue_service.UpdateComment(ctx, comment, contentVersion, ctx.Doer, oldContent); err != nil { + if errors.Is(err, user_model.ErrBlockedUser) { + ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user")) + } else if errors.Is(err, issues_model.ErrCommentAlreadyChanged) { + ctx.JSONError(ctx.Tr("repo.comments.edit.already_changed")) + } else { + ctx.ServerError("UpdateComment", err) + } + return } - return } if err := comment.LoadAttachments(ctx); err != nil { From d0ff33f00753a7b4d85364fd2d01ea137f24eb15 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 9 May 2025 22:15:21 -0700 Subject: [PATCH 4/5] Improvements --- routers/api/v1/repo/issue_comment.go | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 4f83a3621263e..cc342a9313c71 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -609,20 +609,17 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) return } - oldContent := comment.Content - if form.Body == oldContent { - ctx.JSON(http.StatusOK, convert.ToAPIComment(ctx, ctx.Repo.Repository, comment)) - return - } - - comment.Content = form.Body - if err := issue_service.UpdateComment(ctx, comment, comment.ContentVersion, ctx.Doer, oldContent); err != nil { - if errors.Is(err, user_model.ErrBlockedUser) { - ctx.APIError(http.StatusForbidden, err) - } else { - ctx.APIErrorInternal(err) + if form.Body != comment.Content { + oldContent := comment.Content + comment.Content = form.Body + if err := issue_service.UpdateComment(ctx, comment, comment.ContentVersion, ctx.Doer, oldContent); err != nil { + if errors.Is(err, user_model.ErrBlockedUser) { + ctx.APIError(http.StatusForbidden, err) + } else { + ctx.APIErrorInternal(err) + } + return } - return } ctx.JSON(http.StatusOK, convert.ToAPIComment(ctx, ctx.Repo.Repository, comment)) From b7c54e171fa97cd60f4160fad370050761d8d2d8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 10 May 2025 12:00:04 -0700 Subject: [PATCH 5/5] Fix test --- tests/integration/repo_webhook_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index 5ac0a9298e414..34c2090b7240b 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -301,7 +301,7 @@ func Test_WebhookIssueComment(t *testing.T) { // 3. validate the webhook is not triggered because no content change assert.Empty(t, triggeredEvent) - assert.Zero(t, payloads) + assert.Empty(t, payloads) }) }) }