From 15c64122a667cf6a4a143fcf9f60c253133f3dcc Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 14 Jul 2024 07:38:45 +0200 Subject: [PATCH] [BUG] Don't fire notification for comment of pending review - When a comment was updated or deleted and was part of an pending/ongoing review, it would have triggered a notification, such as a webhook. - This patch checks if the comment is part of a pending review and then does not fire a notification and, in the case of updating a comment, does not save the content history because this is not necessary if it is still a "draft" comment given it is a pending comment (there is no need to see my embarrassing typos). - Adds integration tests. - Resolves https://codeberg.org/forgejo/forgejo/issues/4368 --- models/fixtures/comment.yml | 2 + release-notes/4487.md | 1 + services/issue/comments.go | 18 +++- services/issue/comments_test.go | 147 ++++++++++++++++++++++++++++++++ services/issue/main_test.go | 9 +- 5 files changed, 173 insertions(+), 4 deletions(-) create mode 100644 release-notes/4487.md create mode 100644 services/issue/comments_test.go diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml index fdf8908206..d2a1de6559 100644 --- a/models/fixtures/comment.yml +++ b/models/fixtures/comment.yml @@ -14,6 +14,7 @@ content: "good work!" created_unix: 946684811 updated_unix: 946684811 + content_version: 1 - id: 3 type: 0 # comment @@ -33,6 +34,7 @@ tree_path: "README.md" created_unix: 946684812 invalidated: false + content_version: 1 - id: 5 type: 21 # code comment diff --git a/release-notes/4487.md b/release-notes/4487.md new file mode 100644 index 0000000000..3c2767a427 --- /dev/null +++ b/release-notes/4487.md @@ -0,0 +1 @@ +Do not fire webhook notifications for updates and deletions of comments that are part of an ongoing review (a review that is still in draft). Also, content history will not be saved for such comments, to avoid exposing fixing embarrassing typos you've have made while the review was still pending. diff --git a/services/issue/comments.go b/services/issue/comments.go index d257c2612c..3ab577b83f 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -75,7 +75,12 @@ func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_m // UpdateComment updates information of comment. func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion int, doer *user_model.User, oldContent string) error { - needsContentHistory := c.Content != oldContent && c.Type.HasContentSupport() + if err := c.LoadReview(ctx); err != nil { + return err + } + isPartOfPendingReview := c.Review != nil && c.Review.Type == issues_model.ReviewTypePending + + needsContentHistory := c.Content != oldContent && c.Type.HasContentSupport() && !isPartOfPendingReview if needsContentHistory { hasContentHistory, err := issues_model.HasIssueContentHistory(ctx, c.IssueID, c.ID) if err != nil { @@ -104,7 +109,9 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion } } - notify_service.UpdateComment(ctx, doer, c, oldContent) + if !isPartOfPendingReview { + notify_service.UpdateComment(ctx, doer, c, oldContent) + } return nil } @@ -118,7 +125,12 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m return err } - notify_service.DeleteComment(ctx, doer, comment) + if err := comment.LoadReview(ctx); err != nil { + return err + } + if comment.Review == nil || comment.Review.Type != issues_model.ReviewTypePending { + notify_service.DeleteComment(ctx, doer, comment) + } return nil } diff --git a/services/issue/comments_test.go b/services/issue/comments_test.go new file mode 100644 index 0000000000..62547a584a --- /dev/null +++ b/services/issue/comments_test.go @@ -0,0 +1,147 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package issue_test + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + webhook_model "code.gitea.io/gitea/models/webhook" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + issue_service "code.gitea.io/gitea/services/issue" + "code.gitea.io/gitea/tests" + + _ "code.gitea.io/gitea/services/webhook" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDeleteComment(t *testing.T) { + // Use the webhook notification to check if a notification is fired for an action. + defer test.MockVariableValue(&setting.DisableWebhooks, false)() + require.NoError(t, unittest.PrepareTestDatabase()) + + t.Run("Normal comment", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 2}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) + unittest.AssertCount(t, &issues_model.Reaction{CommentID: comment.ID}, 2) + + require.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, &webhook_model.Webhook{ + RepoID: issue.RepoID, + IsActive: true, + Events: `{"choose_events":true,"events":{"issue_comment": true}}`, + })) + hookTaskCount := unittest.GetCount(t, &webhook_model.HookTask{}) + + require.NoError(t, issue_service.DeleteComment(db.DefaultContext, nil, comment)) + + // The comment doesn't exist anymore. + unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: comment.ID}) + // Reactions don't exist anymore for this comment. + unittest.AssertNotExistsBean(t, &issues_model.Reaction{CommentID: comment.ID}) + // Number of comments was decreased. + assert.EqualValues(t, issue.NumComments-1, unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}).NumComments) + // A notification was fired for the deletion of this comment. + assert.EqualValues(t, hookTaskCount+1, unittest.GetCount(t, &webhook_model.HookTask{})) + }) + + t.Run("Comment of pending review", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // We have to ensure that this comment's linked review is pending. + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 4}, "review_id != 0") + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: comment.ReviewID}) + assert.EqualValues(t, issues_model.ReviewTypePending, review.Type) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) + + require.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, &webhook_model.Webhook{ + RepoID: issue.RepoID, + IsActive: true, + Events: `{"choose_events":true,"events":{"issue_comment": true}}`, + })) + hookTaskCount := unittest.GetCount(t, &webhook_model.HookTask{}) + + require.NoError(t, issue_service.DeleteComment(db.DefaultContext, nil, comment)) + + // The comment doesn't exist anymore. + unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: comment.ID}) + // Ensure that the number of comments wasn't decreased. + assert.EqualValues(t, issue.NumComments, unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}).NumComments) + // No notification was fired for the deletion of this comment. + assert.EqualValues(t, hookTaskCount, unittest.GetCount(t, &webhook_model.HookTask{})) + }) +} + +func TestUpdateComment(t *testing.T) { + // Use the webhook notification to check if a notification is fired for an action. + defer test.MockVariableValue(&setting.DisableWebhooks, false)() + require.NoError(t, unittest.PrepareTestDatabase()) + + admin := unittest.AssertExistsAndLoadBean(t, &user_model.User{IsAdmin: true}) + t.Run("Normal comment", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 2}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) + unittest.AssertNotExistsBean(t, &issues_model.ContentHistory{CommentID: comment.ID}) + require.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, &webhook_model.Webhook{ + RepoID: issue.RepoID, + IsActive: true, + Events: `{"choose_events":true,"events":{"issue_comment": true}}`, + })) + hookTaskCount := unittest.GetCount(t, &webhook_model.HookTask{}) + oldContent := comment.Content + comment.Content = "Hello!" + + require.NoError(t, issue_service.UpdateComment(db.DefaultContext, comment, 1, admin, oldContent)) + + newComment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 2}) + // Content was updated. + assert.EqualValues(t, comment.Content, newComment.Content) + // Content version was updated. + assert.EqualValues(t, 2, newComment.ContentVersion) + // A notification was fired for the update of this comment. + assert.EqualValues(t, hookTaskCount+1, unittest.GetCount(t, &webhook_model.HookTask{})) + // Issue history was saved for this comment. + unittest.AssertExistsAndLoadBean(t, &issues_model.ContentHistory{CommentID: comment.ID, IsFirstCreated: true, ContentText: oldContent}) + unittest.AssertExistsAndLoadBean(t, &issues_model.ContentHistory{CommentID: comment.ID, ContentText: comment.Content}, "is_first_created = false") + }) + + t.Run("Comment of pending review", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 4}, "review_id != 0") + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: comment.ReviewID}) + assert.EqualValues(t, issues_model.ReviewTypePending, review.Type) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) + unittest.AssertNotExistsBean(t, &issues_model.ContentHistory{CommentID: comment.ID}) + require.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, &webhook_model.Webhook{ + RepoID: issue.RepoID, + IsActive: true, + Events: `{"choose_events":true,"events":{"issue_comment": true}}`, + })) + hookTaskCount := unittest.GetCount(t, &webhook_model.HookTask{}) + oldContent := comment.Content + comment.Content = "Hello!" + + require.NoError(t, issue_service.UpdateComment(db.DefaultContext, comment, 1, admin, oldContent)) + + newComment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 2}) + // Content was updated. + assert.EqualValues(t, comment.Content, newComment.Content) + // Content version was updated. + assert.EqualValues(t, 2, newComment.ContentVersion) + // No notification was fired for the update of this comment. + assert.EqualValues(t, hookTaskCount, unittest.GetCount(t, &webhook_model.HookTask{})) + // Issue history was not saved for this comment. + unittest.AssertNotExistsBean(t, &issues_model.ContentHistory{CommentID: comment.ID}) + }) +} diff --git a/services/issue/main_test.go b/services/issue/main_test.go index 5dac54183b..c3da441537 100644 --- a/services/issue/main_test.go +++ b/services/issue/main_test.go @@ -7,10 +7,17 @@ import ( "testing" "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/services/webhook" _ "code.gitea.io/gitea/models/actions" ) func TestMain(m *testing.M) { - unittest.MainTest(m) + unittest.MainTest(m, &unittest.TestOptions{ + SetUp: func() error { + setting.LoadQueueSettings() + return webhook.Init() + }, + }) }