[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 <postmaster@gusted.xyz>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7178
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>

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,

templates/repo/diff/comment_form.tmpl

@@ -31,7 +31,6 @@
 				{{if $.reply}}
 					<button class="ui submit primary tiny button btn-reply" type="submit">{{ctx.Locale.Tr "repo.diff.comment.reply"}}</button>
 					<input type="hidden" name="reply" value="{{$.reply}}">
-					<input type="hidden" name="single_review" value="true">
 				{{else}}
 					{{if $.root.CurrentReview}}
 						<button name="pending_review" type="submit" class="ui submit primary tiny button btn-add-comment">{{ctx.Locale.Tr "repo.diff.comment.add_review_comment"}}</button>

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

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

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})
+		})
+	})
+}