loading up the forgejo repo on tangled to test page performance
fix: no notification for replies to pending comments (#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
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7167
Reviewed-by: 0ko <0ko@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>
+174
-2
+19
-1
routers/web/repo/pull_review.go
+19
-1
routers/web/repo/pull_review.go
···
82
82
attachments = form.Files
83
83
}
84
84
85
+
// If the reply is made to a comment that is part of a pending review, then
86
+
// this comment also should be seen as part of that pending review. Consider
87
+
// it to be a pending review by default, except when `single_review` was
88
+
// passed.
89
+
pendingReview := !form.SingleReview
90
+
if form.Reply > 0 {
91
+
r, err := issues_model.GetReviewByID(ctx, form.Reply)
92
+
if err != nil {
93
+
ctx.ServerError("GetReviewByID", err)
94
+
return
95
+
}
96
+
if r.IssueID != issue.ID {
97
+
ctx.NotFound("Review does not belong to pull request", nil)
98
+
return
99
+
}
100
+
pendingReview = r.Type == issues_model.ReviewTypePending
101
+
}
102
+
85
103
comment, err := pull_service.CreateCodeComment(ctx,
86
104
ctx.Doer,
87
105
ctx.Repo.GitRepo,
···
89
107
signedLine,
90
108
form.Content,
91
109
form.TreePath,
92
-
!form.SingleReview,
110
+
pendingReview,
93
111
form.Reply,
94
112
form.LatestCommitID,
95
113
attachments,
-1
templates/repo/diff/comment_form.tmpl
-1
templates/repo/diff/comment_form.tmpl
···
31
31
{{if $.reply}}
32
32
<button class="ui submit primary tiny button btn-reply" type="submit">{{ctx.Locale.Tr "repo.diff.comment.reply"}}</button>
33
33
<input type="hidden" name="reply" value="{{$.reply}}">
34
-
<input type="hidden" name="single_review" value="true">
35
34
{{else}}
36
35
{{if $.root.CurrentReview}}
37
36
<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>
+25
tests/integration/fixtures/TestPullRequestReplyMail/comment.yml
+25
tests/integration/fixtures/TestPullRequestReplyMail/comment.yml
···
1
+
-
2
+
id: 1001
3
+
type: 21 # code comment
4
+
poster_id: 2
5
+
issue_id: 2
6
+
content: "Still pending"
7
+
review_id: 1002
8
+
line: 4
9
+
tree_path: "README.md"
10
+
created_unix: 1730000000
11
+
invalidated: false
12
+
content_version: 1
13
+
14
+
-
15
+
id: 1002
16
+
type: 21 # code comment
17
+
poster_id: 2
18
+
issue_id: 2
19
+
content: "Visible"
20
+
review_id: 1001
21
+
line: 3
22
+
tree_path: "README.md"
23
+
created_unix: 1720000000
24
+
invalidated: false
25
+
content_version: 1
+17
tests/integration/fixtures/TestPullRequestReplyMail/review.yml
+17
tests/integration/fixtures/TestPullRequestReplyMail/review.yml
···
1
+
-
2
+
id: 1001
3
+
type: 2
4
+
reviewer_id: 2
5
+
issue_id: 2
6
+
content: "Normal review"
7
+
updated_unix: 1720000000
8
+
created_unix: 1720000000
9
+
10
+
-
11
+
id: 1002
12
+
type: 0
13
+
reviewer_id: 2
14
+
issue_id: 2
15
+
content: "Pending review"
16
+
updated_unix: 1730000000
17
+
created_unix: 1730000000
+113
tests/integration/pull_review_test.go
+113
tests/integration/pull_review_test.go
···
23
23
repo_module "code.gitea.io/gitea/modules/repository"
24
24
"code.gitea.io/gitea/modules/test"
25
25
issue_service "code.gitea.io/gitea/services/issue"
26
+
"code.gitea.io/gitea/services/mailer"
26
27
repo_service "code.gitea.io/gitea/services/repository"
27
28
files_service "code.gitea.io/gitea/services/repository/files"
28
29
"code.gitea.io/gitea/tests"
···
649
650
doc := NewHTMLParser(t, resp.Body)
650
651
return doc.Find(`.notification_count`).Text()
651
652
}
653
+
654
+
func TestPullRequestReplyMail(t *testing.T) {
655
+
defer tests.AddFixtures("tests/integration/fixtures/TestPullRequestReplyMail/")()
656
+
defer tests.PrepareTestEnv(t)()
657
+
658
+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
659
+
session := loginUser(t, user.Name)
660
+
661
+
t.Run("Reply to pending review comment", func(t *testing.T) {
662
+
defer tests.PrintCurrentTest(t)()
663
+
664
+
called := false
665
+
defer test.MockVariableValue(&mailer.SendAsync, func(...*mailer.Message) {
666
+
called = true
667
+
})()
668
+
669
+
review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 1002}, "type = 0")
670
+
671
+
req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{
672
+
"_csrf": GetCSRF(t, session, "/user2/repo1/pulls/2"),
673
+
"origin": "diff",
674
+
"content": "Just a comment!",
675
+
"side": "proposed",
676
+
"line": "4",
677
+
"path": "README.md",
678
+
"reply": strconv.FormatInt(review.ID, 10),
679
+
})
680
+
session.MakeRequest(t, req, http.StatusOK)
681
+
682
+
assert.False(t, called)
683
+
unittest.AssertExistsIf(t, true, &issues_model.Comment{Content: "Just a comment!", ReviewID: review.ID, IssueID: 2})
684
+
})
685
+
686
+
t.Run("Start a review", func(t *testing.T) {
687
+
defer tests.PrintCurrentTest(t)()
688
+
689
+
called := false
690
+
defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) {
691
+
called = true
692
+
})()
693
+
694
+
req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{
695
+
"_csrf": GetCSRF(t, session, "/user2/repo1/pulls/2"),
696
+
"origin": "diff",
697
+
"content": "Notification time 2!",
698
+
"side": "proposed",
699
+
"line": "2",
700
+
"path": "README.md",
701
+
})
702
+
session.MakeRequest(t, req, http.StatusOK)
703
+
704
+
assert.False(t, called)
705
+
unittest.AssertExistsIf(t, true, &issues_model.Comment{Content: "Notification time 2!", IssueID: 2})
706
+
})
707
+
708
+
t.Run("Create a single comment", func(t *testing.T) {
709
+
t.Run("As a reply", func(t *testing.T) {
710
+
defer tests.PrintCurrentTest(t)()
711
+
712
+
called := false
713
+
defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) {
714
+
assert.Len(t, msgs, 2)
715
+
assert.Equal(t, "user1@example.com", msgs[0].To)
716
+
assert.EqualValues(t, "Re: [user2/repo1] issue2 (PR #2)", msgs[0].Subject)
717
+
assert.Contains(t, msgs[0].Body, "Notification time!")
718
+
called = true
719
+
})()
720
+
721
+
review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 1001, Type: issues_model.ReviewTypeComment})
722
+
723
+
req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{
724
+
"_csrf": GetCSRF(t, session, "/user2/repo1/pulls/2"),
725
+
"origin": "diff",
726
+
"content": "Notification time!",
727
+
"side": "proposed",
728
+
"line": "3",
729
+
"path": "README.md",
730
+
"reply": strconv.FormatInt(review.ID, 10),
731
+
})
732
+
session.MakeRequest(t, req, http.StatusOK)
733
+
734
+
assert.True(t, called)
735
+
unittest.AssertExistsIf(t, true, &issues_model.Comment{Content: "Notification time!", ReviewID: review.ID, IssueID: 2})
736
+
})
737
+
t.Run("On a new line", func(t *testing.T) {
738
+
defer tests.PrintCurrentTest(t)()
739
+
740
+
called := false
741
+
defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) {
742
+
assert.Len(t, msgs, 2)
743
+
assert.Equal(t, "user1@example.com", msgs[0].To)
744
+
assert.EqualValues(t, "Re: [user2/repo1] issue2 (PR #2)", msgs[0].Subject)
745
+
assert.Contains(t, msgs[0].Body, "Notification time 2!")
746
+
called = true
747
+
})()
748
+
749
+
req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{
750
+
"_csrf": GetCSRF(t, session, "/user2/repo1/pulls/2"),
751
+
"origin": "diff",
752
+
"content": "Notification time 2!",
753
+
"side": "proposed",
754
+
"line": "5",
755
+
"path": "README.md",
756
+
"single_review": "true",
757
+
})
758
+
session.MakeRequest(t, req, http.StatusOK)
759
+
760
+
assert.True(t, called)
761
+
unittest.AssertExistsIf(t, true, &issues_model.Comment{Content: "Notification time 2!", IssueID: 2})
762
+
})
763
+
})
764
+
}