From 0c0155daf7b9a6176c64d568a0516ef6478630c4 Mon Sep 17 00:00:00 2001 From: forgejo-backport-action Date: Sun, 9 Mar 2025 16:06:01 +0000 Subject: [PATCH] [v10.0/forgejo] fix: no notification for replies to pending comments (#7178) **Backport:** https://codeberg.org/forgejo/forgejo/pulls/7167 - Replies to pending review comments no longer generate a notification, this was caused by an incomplete determination if the comment was part of the pending review or not. - The logic was reworked to do the following if it's part of a pending review: It is not a single review and if it's a reply then the comment it is replying to is part of a pending review. - Added integration test. - Resolves forgejo/forgejo#7151 Co-authored-by: Gusted Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7178 Reviewed-by: Gusted Co-authored-by: forgejo-backport-action Co-committed-by: forgejo-backport-action --- routers/web/repo/pull_review.go | 20 +++- templates/repo/diff/comment_form.tmpl | 1 - .../TestPullRequestReplyMail/comment.yml | 25 ++++ .../TestPullRequestReplyMail/review.yml | 17 +++ tests/integration/pull_review_test.go | 113 ++++++++++++++++++ 5 files changed, 174 insertions(+), 2 deletions(-) create mode 100644 tests/integration/fixtures/TestPullRequestReplyMail/comment.yml create mode 100644 tests/integration/fixtures/TestPullRequestReplyMail/review.yml diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index e8a3c48d7f..eb8dd83d9c 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -82,6 +82,24 @@ func CreateCodeComment(ctx *context.Context) { attachments = form.Files } + // If the reply is made to a comment that is part of a pending review, then + // this comment also should be seen as part of that pending review. Consider + // it to be a pending review by default, except when `single_review` was + // passed. + pendingReview := !form.SingleReview + if form.Reply > 0 { + r, err := issues_model.GetReviewByID(ctx, form.Reply) + if err != nil { + ctx.ServerError("GetReviewByID", err) + return + } + if r.IssueID != issue.ID { + ctx.NotFound("Review does not belong to pull request", nil) + return + } + pendingReview = r.Type == issues_model.ReviewTypePending + } + comment, err := pull_service.CreateCodeComment(ctx, ctx.Doer, ctx.Repo.GitRepo, @@ -89,7 +107,7 @@ func CreateCodeComment(ctx *context.Context) { signedLine, form.Content, form.TreePath, - !form.SingleReview, + pendingReview, form.Reply, form.LatestCommitID, attachments, diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 856b3da01a..bb29583059 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -31,7 +31,6 @@ {{if $.reply}} - {{else}} {{if $.root.CurrentReview}} diff --git a/tests/integration/fixtures/TestPullRequestReplyMail/comment.yml b/tests/integration/fixtures/TestPullRequestReplyMail/comment.yml new file mode 100644 index 0000000000..2dc407e7a4 --- /dev/null +++ b/tests/integration/fixtures/TestPullRequestReplyMail/comment.yml @@ -0,0 +1,25 @@ +- + id: 1001 + type: 21 # code comment + poster_id: 2 + issue_id: 2 + content: "Still pending" + review_id: 1002 + line: 4 + tree_path: "README.md" + created_unix: 1730000000 + invalidated: false + content_version: 1 + +- + id: 1002 + type: 21 # code comment + poster_id: 2 + issue_id: 2 + content: "Visible" + review_id: 1001 + line: 3 + tree_path: "README.md" + created_unix: 1720000000 + invalidated: false + content_version: 1 diff --git a/tests/integration/fixtures/TestPullRequestReplyMail/review.yml b/tests/integration/fixtures/TestPullRequestReplyMail/review.yml new file mode 100644 index 0000000000..a882ce1caf --- /dev/null +++ b/tests/integration/fixtures/TestPullRequestReplyMail/review.yml @@ -0,0 +1,17 @@ +- + id: 1001 + type: 2 + reviewer_id: 2 + issue_id: 2 + content: "Normal review" + updated_unix: 1720000000 + created_unix: 1720000000 + +- + id: 1002 + type: 0 + reviewer_id: 2 + issue_id: 2 + content: "Pending review" + updated_unix: 1730000000 + created_unix: 1730000000 diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index e1db171f16..49f2bcc9b8 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -23,6 +23,7 @@ import ( repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/test" issue_service "code.gitea.io/gitea/services/issue" + "code.gitea.io/gitea/services/mailer" repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" "code.gitea.io/gitea/tests" @@ -604,3 +605,115 @@ func getUserNotificationCount(t *testing.T, session *TestSession, csrf string) s doc := NewHTMLParser(t, resp.Body) return doc.Find(`.notification_count`).Text() } + +func TestPullRequestReplyMail(t *testing.T) { + defer tests.AddFixtures("tests/integration/fixtures/TestPullRequestReplyMail/")() + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + session := loginUser(t, user.Name) + + t.Run("Reply to pending review comment", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + called := false + defer test.MockVariableValue(&mailer.SendAsync, func(...*mailer.Message) { + called = true + })() + + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 1002}, "type = 0") + + req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ + "_csrf": GetCSRF(t, session, "/user2/repo1/pulls/2"), + "origin": "diff", + "content": "Just a comment!", + "side": "proposed", + "line": "4", + "path": "README.md", + "reply": strconv.FormatInt(review.ID, 10), + }) + session.MakeRequest(t, req, http.StatusOK) + + assert.False(t, called) + unittest.AssertExistsIf(t, true, &issues_model.Comment{Content: "Just a comment!", ReviewID: review.ID, IssueID: 2}) + }) + + t.Run("Start a review", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + called := false + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + called = true + })() + + req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ + "_csrf": GetCSRF(t, session, "/user2/repo1/pulls/2"), + "origin": "diff", + "content": "Notification time 2!", + "side": "proposed", + "line": "2", + "path": "README.md", + }) + session.MakeRequest(t, req, http.StatusOK) + + assert.False(t, called) + unittest.AssertExistsIf(t, true, &issues_model.Comment{Content: "Notification time 2!", IssueID: 2}) + }) + + t.Run("Create a single comment", func(t *testing.T) { + t.Run("As a reply", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + called := false + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + assert.Len(t, msgs, 2) + assert.Equal(t, "user1@example.com", msgs[0].To) + assert.EqualValues(t, "Re: [user2/repo1] issue2 (PR #2)", msgs[0].Subject) + assert.Contains(t, msgs[0].Body, "Notification time!") + called = true + })() + + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 1001, Type: issues_model.ReviewTypeComment}) + + req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ + "_csrf": GetCSRF(t, session, "/user2/repo1/pulls/2"), + "origin": "diff", + "content": "Notification time!", + "side": "proposed", + "line": "3", + "path": "README.md", + "reply": strconv.FormatInt(review.ID, 10), + }) + session.MakeRequest(t, req, http.StatusOK) + + assert.True(t, called) + unittest.AssertExistsIf(t, true, &issues_model.Comment{Content: "Notification time!", ReviewID: review.ID, IssueID: 2}) + }) + t.Run("On a new line", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + called := false + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + assert.Len(t, msgs, 2) + assert.Equal(t, "user1@example.com", msgs[0].To) + assert.EqualValues(t, "Re: [user2/repo1] issue2 (PR #2)", msgs[0].Subject) + assert.Contains(t, msgs[0].Body, "Notification time 2!") + called = true + })() + + req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ + "_csrf": GetCSRF(t, session, "/user2/repo1/pulls/2"), + "origin": "diff", + "content": "Notification time 2!", + "side": "proposed", + "line": "5", + "path": "README.md", + "single_review": "true", + }) + session.MakeRequest(t, req, http.StatusOK) + + assert.True(t, called) + unittest.AssertExistsIf(t, true, &issues_model.Comment{Content: "Notification time 2!", IssueID: 2}) + }) + }) +}