When updating comment, if the content is the same, just return and not update the databse (#34422)

Fix #34318

Author: lunny

routers/api/v1/repo/issue_comment.go

@@ -609,15 +609,17 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
 		return
 	}
 
-	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)
+	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))

routers/web/repo/issue_comment.go

@@ -239,21 +239,28 @@ func UpdateCommentContent(ctx *context.Context) {
 		return
 	}
 
-	oldContent := comment.Content
 	newContent := ctx.FormString("content")
 	contentVersion := ctx.FormInt("content_version")
+	if contentVersion != comment.ContentVersion {
+		ctx.JSONError(ctx.Tr("repo.comments.edit.already_changed"))
+		return
+	}
 
-	// allow to save empty 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 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)
+			}
+			return
 		}
-		return
 	}
 
 	if err := comment.LoadAttachments(ctx); err != nil {

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.Empty(t, triggeredEvent)
+			assert.Empty(t, payloads)
+		})
 	})
 }