diff --git a/server/plugin/test_utils.go b/server/plugin/test_utils.go index 3a4f76373..5f3e33e68 100644 --- a/server/plugin/test_utils.go +++ b/server/plugin/test_utils.go @@ -303,6 +303,12 @@ func GetMockPullRequestReviewCommentEvent(action, body, sender string) *github.P } } +func GetMockPullRequestReviewCommentReplyEvent(action, body, sender string, replyTo int64) *github.PullRequestReviewCommentEvent { + event := GetMockPullRequestReviewCommentEvent(action, body, sender) + event.Comment.InReplyTo = github.Int64(replyTo) + return event +} + func GetMockIssueCommentEvent(action, body, sender string) *github.IssueCommentEvent { return &github.IssueCommentEvent{ Action: github.String(action), diff --git a/server/plugin/webhook.go b/server/plugin/webhook.go index c26da4d6e..c169d1ab6 100644 --- a/server/plugin/webhook.go +++ b/server/plugin/webhook.go @@ -1151,6 +1151,11 @@ func (p *Plugin) handleReviewCommentAuthorNotification(event *github.PullRequest return } + // Top-level comments on a submitted review are already DMed via handlePullRequestReviewNotification. + if event.GetComment().GetInReplyTo() == 0 { + return + } + authorUserID := p.getGitHubToUserIDMapping(author) if authorUserID == "" { return diff --git a/server/plugin/webhook_test.go b/server/plugin/webhook_test.go index 1f1b9b404..55f553a84 100644 --- a/server/plugin/webhook_test.go +++ b/server/plugin/webhook_test.go @@ -611,12 +611,17 @@ func TestHandleReviewCommentAuthorNotification(t *testing.T) { }, { name: "Sender is the PR author", - event: GetMockPullRequestReviewCommentEvent(actionCreated, "body", MockIssueAuthor), + event: GetMockPullRequestReviewCommentReplyEvent(actionCreated, "body", MockIssueAuthor, 99), setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, }, { - name: "Author not mapped to Mattermost", + name: "Top-level review comment is covered by the review submitted notification", event: GetMockPullRequestReviewCommentEvent(actionCreated, "body", MockUserLogin), + setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, + }, + { + name: "Author not mapped to Mattermost", + event: GetMockPullRequestReviewCommentReplyEvent(actionCreated, "body", MockUserLogin, 99), setup: func(_ *plugintest.API, mockKVStore *mocks.MockKvStore) { mockKVStore.EXPECT().Get("issueAuthor_githubusername", mock.MatchedBy(func(val any) bool { _, ok := val.(*[]uint8) @@ -625,8 +630,8 @@ func TestHandleReviewCommentAuthorNotification(t *testing.T) { }, }, { - name: "Successful author notification", - event: GetMockPullRequestReviewCommentEvent(actionCreated, "body", MockUserLogin), + name: "Successful author notification for thread reply", + event: GetMockPullRequestReviewCommentReplyEvent(actionCreated, "body", MockUserLogin, 99), setup: func(mockAPI *plugintest.API, mockKVStore *mocks.MockKvStore) { mockKVStore.EXPECT().Get("issueAuthor_githubusername", mock.MatchedBy(func(val any) bool { _, ok := val.(*[]uint8) @@ -646,7 +651,7 @@ func TestHandleReviewCommentAuthorNotification(t *testing.T) { }, { name: "Muted sender suppresses author notification", - event: GetMockPullRequestReviewCommentEvent(actionCreated, "body", MockUserLogin), + event: GetMockPullRequestReviewCommentReplyEvent(actionCreated, "body", MockUserLogin, 99), setup: func(_ *plugintest.API, mockKVStore *mocks.MockKvStore) { mockKVStore.EXPECT().Get("issueAuthor_githubusername", mock.MatchedBy(func(val any) bool { _, ok := val.(*[]uint8)