From c49b18c95e309ac8a7cb4296f5c67e9a41288604 Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Fri, 15 May 2026 17:14:48 +0300 Subject: [PATCH 1/3] Improving UX for DMs sent on PR comments - Preventing edit or delete PR message events from sending a "new comment" DM notification - Improving logic that cleans body to prevent issues with html comments in the message from interfering with visibility of message --- server/plugin/template.go | 64 +++++++++++-- server/plugin/test_utils.go | 11 ++- server/plugin/webhook.go | 104 ++++++++++++++++++++- server/plugin/webhook_test.go | 171 +++++++++++++++++++++++++++++++++- 4 files changed, 333 insertions(+), 17 deletions(-) diff --git a/server/plugin/template.go b/server/plugin/template.go index e9dc3f8e1..67a00471a 100644 --- a/server/plugin/template.go +++ b/server/plugin/template.go @@ -68,6 +68,19 @@ func init() { return mdCommentRegex.ReplaceAllString(body, "") } + funcMap["cleanBody"] = func(body string) string { + cleaned := body + if strings.Contains(cleaned, "notifications@github.com") { + cleaned = strings.Split(cleaned, "\n\nOn")[0] + } + cleaned = mdCommentRegex.ReplaceAllString(cleaned, "") + cleaned = strings.TrimSpace(cleaned) + if cleaned == "" { + return "" + } + return cleaned + } + // Replace any GitHub username with its corresponding Mattermost username, if any funcMap["replaceAllGitHubUsernames"] = func(body string) string { return gitHubUsernameRegex.ReplaceAllStringFunc(body, func(matched string) string { @@ -358,41 +371,78 @@ Reviewers: {{range $i, $el := .RequestedReviewers -}} {{- if $i}}, {{end}}{{temp {{template "repo" .GetRepo}} New review comment by {{template "user" .GetSender}} on {{template "pullRequest" .GetPullRequest}}: {{.GetComment.GetBody | trimBody | replaceAllGitHubUsernames}} +`)) + + template.Must(masterTemplate.New("reviewCommentMentionNotification").Funcs(funcMap).Parse(` +{{template "user" .GetSender}} mentioned you in a review comment on [{{.GetRepo.GetFullName}}#{{.GetPullRequest.GetNumber}}]({{.GetComment.GetHTMLURL}}) - {{.GetPullRequest.GetTitle}}: +{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}} +{{- if $body}} +{{$body | quote}} +{{- end}} +`)) + + template.Must(masterTemplate.New("reviewCommentAuthorNotification").Funcs(funcMap).Parse(` +{{template "user" .GetSender}} commented on your pull request [{{.GetRepo.GetFullName}}#{{.GetPullRequest.GetNumber}}]({{.GetComment.GetHTMLURL}}) - {{.GetPullRequest.GetTitle}}: +{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}} +{{- if $body}} +{{$body | quote}} +{{- end}} `)) template.Must(masterTemplate.New("commentMentionNotification").Funcs(funcMap).Parse(` {{template "user" .GetSender}} mentioned you on [{{.GetRepo.GetFullName}}#{{.Issue.GetNumber}}]({{.GetComment.GetHTMLURL}}) - {{.Issue.GetTitle}}: -{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}} +{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}} +{{- if $body}} +{{$body | quote}} +{{- end}} `)) template.Must(masterTemplate.New("commentAuthorPullRequestNotification").Funcs(funcMap).Parse(` {{template "user" .GetSender}} commented on your pull request {{template "eventRepoIssueFullLinkWithTitle" .}}: -{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}} +{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}} +{{- if $body}} +{{$body | quote}} +{{- end}} `)) template.Must(masterTemplate.New("commentAssigneePullRequestNotification").Funcs(funcMap).Parse(` {{template "user" .GetSender}} commented on pull request you are assigned to {{template "eventRepoIssueFullLinkWithTitle" .}}: -{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}} +{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}} +{{- if $body}} +{{$body | quote}} +{{- end}} `)) template.Must(masterTemplate.New("commentAssigneeIssueNotification").Funcs(funcMap).Parse(` {{template "user" .GetSender}} commented on an issue you are assigned to {{template "eventRepoIssueFullLinkWithTitle" .}}: -{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}} +{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}} +{{- if $body}} +{{$body | quote}} +{{- end}} `)) template.Must(masterTemplate.New("commentAssigneeSelfMentionPullRequestNotification").Funcs(funcMap).Parse(` {{template "user" .GetSender}} mentioned you on a pull request that you are assigned to {{template "eventRepoIssueFullLinkWithTitle" .}}: -{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}} +{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}} +{{- if $body}} +{{$body | quote}} +{{- end}} `)) template.Must(masterTemplate.New("commentAssigneeSelfMentionIssueNotification").Funcs(funcMap).Parse(` {{template "user" .GetSender}} mentioned you on an issue that you are assigned to {{template "eventRepoIssueFullLinkWithTitle" .}}: -{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}} +{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}} +{{- if $body}} +{{$body | quote}} +{{- end}} `)) template.Must(masterTemplate.New("commentAuthorIssueNotification").Funcs(funcMap).Parse(` {{template "user" .GetSender}} commented on your issue {{template "eventRepoIssueFullLinkWithTitle" .}}: -{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}} +{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}} +{{- if $body}} +{{$body | quote}} +{{- end}} `)) template.Must(masterTemplate.New("pullRequestNotification").Funcs(funcMap).Parse(` diff --git a/server/plugin/test_utils.go b/server/plugin/test_utils.go index 49c1f6242..3a4f76373 100644 --- a/server/plugin/test_utils.go +++ b/server/plugin/test_utils.go @@ -280,8 +280,9 @@ func GetMockPullRequestReviewEvent(action, state, repo string, isPrivate bool, r } } -func GetMockPullRequestReviewCommentEvent() *github.PullRequestReviewCommentEvent { +func GetMockPullRequestReviewCommentEvent(action, body, sender string) *github.PullRequestReviewCommentEvent { return &github.PullRequestReviewCommentEvent{ + Action: github.String(action), Repo: &github.Repository{ Name: github.String(MockRepoName), FullName: github.String(MockOrgRepo), @@ -290,13 +291,15 @@ func GetMockPullRequestReviewCommentEvent() *github.PullRequestReviewCommentEven }, Comment: &github.PullRequestComment{ ID: github.Int64(12345), - Body: github.String("This is a review comment"), + Body: github.String(body), HTMLURL: github.String(fmt.Sprintf("%s%s/pull/1#discussion_r12345", GithubBaseURL, MockOrgRepo)), }, Sender: &github.User{ - Login: github.String(MockUserLogin), + Login: github.String(sender), + }, + PullRequest: &github.PullRequest{ + User: &github.User{Login: github.String(MockIssueAuthor)}, }, - PullRequest: &github.PullRequest{}, } } diff --git a/server/plugin/webhook.go b/server/plugin/webhook.go index 8cbc783d6..9a94c9d5a 100644 --- a/server/plugin/webhook.go +++ b/server/plugin/webhook.go @@ -287,6 +287,8 @@ func (p *Plugin) handleWebhook(w http.ResponseWriter, r *http.Request) { repo = event.GetRepo() handler = func() { p.postPullRequestReviewCommentEvent(event) + p.handleReviewCommentMentionNotification(event) + p.handleReviewCommentAuthorNotification(event) } case *github.PushEvent: repo = ConvertPushEventRepositoryToRepository(event.GetRepo()) @@ -1019,12 +1021,16 @@ func (p *Plugin) postPullRequestReviewEvent(event *github.PullRequestReviewEvent func (p *Plugin) postPullRequestReviewCommentEvent(event *github.PullRequestReviewCommentEvent) { repo := event.GetRepo() + if event.GetAction() != actionCreated { + return + } + subs := p.GetSubscribedChannelsForRepository(repo) if len(subs) == 0 { return } - newReviewMessage, err := renderTemplate("newReviewComment", event) + message, err := renderTemplate("newReviewComment", event) if err != nil { p.client.Log.Warn("Failed to render template", "error", err.Error()) return @@ -1061,7 +1067,7 @@ func (p *Plugin) postPullRequestReviewCommentEvent(event *github.PullRequestRevi continue } - post := p.makeBotPost(newReviewMessage, "custom_git_pr_comment") + post := p.makeBotPost(message, "custom_git_pr_comment") repoName := strings.ToLower(repo.GetFullName()) commentID := event.GetComment().GetID() @@ -1077,6 +1083,95 @@ func (p *Plugin) postPullRequestReviewCommentEvent(event *github.PullRequestRevi } } +func (p *Plugin) handleReviewCommentMentionNotification(event *github.PullRequestReviewCommentEvent) { + if event.GetAction() != actionCreated { + return + } + + body := event.GetComment().GetBody() + + if strings.Contains(body, "notifications@github.com") { + body = strings.Split(body, "\n\nOn")[0] + } + + mentionedUsernames := parseGitHubUsernamesFromText(body) + + message, err := renderTemplate("reviewCommentMentionNotification", event) + if err != nil { + p.client.Log.Warn("Failed to render template", "error", err.Error()) + return + } + + for _, username := range mentionedUsernames { + if username == event.GetSender().GetLogin() { + continue + } + + if username == event.GetPullRequest().GetUser().GetLogin() { + continue + } + + userID := p.getGitHubToUserIDMapping(username) + if userID == "" { + continue + } + + if event.GetRepo().GetPrivate() && !p.permissionToRepo(userID, event.GetRepo().GetFullName()) { + continue + } + + if p.senderMutedByReceiver(userID, event.GetSender().GetLogin()) { + continue + } + + channel, err := p.client.Channel.GetDirect(userID, p.BotUserID) + if err != nil { + continue + } + + post := p.makeBotPost(message, "custom_git_mention") + post.ChannelId = channel.Id + if err = p.client.Post.CreatePost(post); err != nil { + p.client.Log.Warn("Error creating review comment mention post", "error", err.Error()) + } + + p.sendRefreshEvent(userID) + } +} + +func (p *Plugin) handleReviewCommentAuthorNotification(event *github.PullRequestReviewCommentEvent) { + author := event.GetPullRequest().GetUser().GetLogin() + if author == event.GetSender().GetLogin() { + return + } + + if event.GetAction() != actionCreated { + return + } + + authorUserID := p.getGitHubToUserIDMapping(author) + if authorUserID == "" { + return + } + + if event.GetRepo().GetPrivate() && !p.permissionToRepo(authorUserID, event.GetRepo().GetFullName()) { + return + } + + if p.senderMutedByReceiver(authorUserID, event.GetSender().GetLogin()) { + return + } + + message, err := renderTemplate("reviewCommentAuthorNotification", event) + if err != nil { + p.client.Log.Warn("Failed to render template", "error", err.Error()) + return + } + + p.CreateBotDMPost(authorUserID, message, "custom_git_author") + p.sendRefreshEvent(authorUserID) +} + func (p *Plugin) handleCommentMentionNotification(event *github.IssueCommentEvent) { action := event.GetAction() if action == actionEdited || action == actionDeleted { @@ -1205,6 +1300,11 @@ func (p *Plugin) handleCommentAuthorNotification(event *github.IssueCommentEvent } func (p *Plugin) handleCommentAssigneeNotification(event *github.IssueCommentEvent) { + action := event.GetAction() + if action == actionEdited || action == actionDeleted { + return + } + author := event.GetIssue().GetUser().GetLogin() assignees := event.GetIssue().Assignees repoName := event.GetRepo().GetFullName() diff --git a/server/plugin/webhook_test.go b/server/plugin/webhook_test.go index c152fd309..1f1b9b404 100644 --- a/server/plugin/webhook_test.go +++ b/server/plugin/webhook_test.go @@ -483,9 +483,14 @@ func TestPostPullRequestReviewCommentEvent(t *testing.T) { event *github.PullRequestReviewCommentEvent setup func(*plugintest.API, *mocks.MockKvStore) }{ + { + name: "Non-created action is ignored", + event: GetMockPullRequestReviewCommentEvent(actionEdited, "body", MockUserLogin), + setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, + }, { name: "No subscriptions found", - event: GetMockPullRequestReviewCommentEvent(), + event: GetMockPullRequestReviewCommentEvent(actionCreated, "This is a review comment", MockUserLogin), setup: func(_ *plugintest.API, mockKVStore *mocks.MockKvStore) { mockKVStore.EXPECT().Get(SubscriptionsKey, mock.MatchedBy(func(val any) bool { _, ok := val.(**Subscriptions) @@ -495,7 +500,7 @@ func TestPostPullRequestReviewCommentEvent(t *testing.T) { }, { name: "Error creating post", - event: GetMockPullRequestReviewCommentEvent(), + event: GetMockPullRequestReviewCommentEvent(actionCreated, "This is a review comment", MockUserLogin), setup: func(mockAPI *plugintest.API, mockKVStore *mocks.MockKvStore) { mockSubscription(mockKVStore) mockAPI.On("CreatePost", mock.Anything).Return(nil, &model.AppError{Message: "error creating post"}).Times(1) @@ -503,8 +508,8 @@ func TestPostPullRequestReviewCommentEvent(t *testing.T) { }, }, { - name: "Successful handling of pull request review comment event", - event: GetMockPullRequestReviewCommentEvent(), + name: "Successful handling of created review comment event", + event: GetMockPullRequestReviewCommentEvent(actionCreated, "This is a review comment", MockUserLogin), setup: func(mockAPI *plugintest.API, mockKVStore *mocks.MockKvStore) { mockSubscription(mockKVStore) mockAPI.On("CreatePost", mock.Anything).Return(&model.Post{}, nil).Times(1) @@ -526,6 +531,154 @@ func TestPostPullRequestReviewCommentEvent(t *testing.T) { } } +func TestHandleReviewCommentMentionNotification(t *testing.T) { + tests := []struct { + name string + event *github.PullRequestReviewCommentEvent + setup func(*plugintest.API, *mocks.MockKvStore) + }{ + { + name: "Unsupported action edited", + event: GetMockPullRequestReviewCommentEvent(actionEdited, "mention @otherUser", MockUserLogin), + setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, + }, + { + name: "Commenter is the same as mentioned user", + event: GetMockPullRequestReviewCommentEvent(actionCreated, "mention @mockUser", MockUserLogin), + setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, + }, + { + name: "Mentioned user is PR author (handled separately)", + event: GetMockPullRequestReviewCommentEvent(actionCreated, "mention @issueAuthor", MockUserLogin), + setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, + }, + { + name: "Mentioned user not mapped to Mattermost", + event: GetMockPullRequestReviewCommentEvent(actionCreated, "mention @otherUser", MockUserLogin), + setup: func(_ *plugintest.API, mockKVStore *mocks.MockKvStore) { + mockKVStore.EXPECT().Get("otherUser_githubusername", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).Return(nil).Times(1) + }, + }, + { + name: "Successful mention notification", + event: GetMockPullRequestReviewCommentEvent(actionCreated, "mention @otherUser", MockUserLogin), + setup: func(mockAPI *plugintest.API, mockKVStore *mocks.MockKvStore) { + mockKVStore.EXPECT().Get("otherUser_githubusername", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).DoAndReturn(setByteValue("otherUserID")).Times(1) + mockKVStore.EXPECT().Get("otherUserID-muted-users", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).Return(nil).Times(1) + mockKVStore.EXPECT().Get("otherUserID_githubtoken", mock.MatchedBy(func(val any) bool { + _, ok := val.(**GitHubUserInfo) + return ok + })).Return(nil).Times(1) + mockAPI.On("GetDirectChannel", "otherUserID", "mockBotID").Return(&model.Channel{Id: "mockChannelID"}, nil).Times(1) + mockAPI.On("CreatePost", mock.Anything).Return(&model.Post{}, nil).Times(1) + }, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + mockKVStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKVStore) + + mockAPI.ExpectedCalls = nil + tc.setup(mockAPI, mockKVStore) + + p.handleReviewCommentMentionNotification(tc.event) + + mockAPI.AssertExpectations(t) + }) + } +} + +func TestHandleReviewCommentAuthorNotification(t *testing.T) { + tests := []struct { + name string + event *github.PullRequestReviewCommentEvent + setup func(*plugintest.API, *mocks.MockKvStore) + }{ + { + name: "Unsupported action edited", + event: GetMockPullRequestReviewCommentEvent(actionEdited, "body", MockUserLogin), + setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, + }, + { + name: "Sender is the PR author", + event: GetMockPullRequestReviewCommentEvent(actionCreated, "body", MockIssueAuthor), + setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, + }, + { + name: "Author not mapped to Mattermost", + event: GetMockPullRequestReviewCommentEvent(actionCreated, "body", MockUserLogin), + setup: func(_ *plugintest.API, mockKVStore *mocks.MockKvStore) { + mockKVStore.EXPECT().Get("issueAuthor_githubusername", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).Return(nil).Times(1) + }, + }, + { + name: "Successful author notification", + event: GetMockPullRequestReviewCommentEvent(actionCreated, "body", MockUserLogin), + setup: func(mockAPI *plugintest.API, mockKVStore *mocks.MockKvStore) { + mockKVStore.EXPECT().Get("issueAuthor_githubusername", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).DoAndReturn(setByteValue("authorUserID")).Times(1) + mockKVStore.EXPECT().Get("authorUserID-muted-users", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).Return(nil).Times(1) + mockKVStore.EXPECT().Get("authorUserID_githubtoken", mock.MatchedBy(func(val any) bool { + _, ok := val.(**GitHubUserInfo) + return ok + })).Return(nil).Times(1) + mockAPI.On("GetDirectChannel", "authorUserID", "mockBotID").Return(&model.Channel{Id: "mockChannelID"}, nil).Times(1) + mockAPI.On("CreatePost", mock.Anything).Return(&model.Post{}, nil).Times(1) + }, + }, + { + name: "Muted sender suppresses author notification", + event: GetMockPullRequestReviewCommentEvent(actionCreated, "body", MockUserLogin), + setup: func(_ *plugintest.API, mockKVStore *mocks.MockKvStore) { + mockKVStore.EXPECT().Get("issueAuthor_githubusername", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).DoAndReturn(setByteValue("authorUserID")).Times(1) + mockKVStore.EXPECT().Get("authorUserID-muted-users", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).DoAndReturn(func(key string, value any) error { + if v, ok := value.(*[]byte); ok { + *v = []byte(MockUserLogin) + } + return nil + }).Times(1) + }, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + mockKVStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKVStore) + + mockAPI.ExpectedCalls = nil + tc.setup(mockAPI, mockKVStore) + + p.handleReviewCommentAuthorNotification(tc.event) + + mockAPI.AssertExpectations(t) + }) + } +} + func TestHandleCommentMentionNotification(t *testing.T) { tests := []struct { name string @@ -762,6 +915,16 @@ func TestHandleCommentAssigneeNotification(t *testing.T) { event *github.IssueCommentEvent setup func(*plugintest.API, *mocks.MockKvStore) }{ + { + name: "Edited action is ignored", + event: GetMockIssueCommentEventWithAssignees("pull", actionEdited, "mockBody", "mockUser", []string{"assigneeUser"}), + setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, + }, + { + name: "Deleted action is ignored", + event: GetMockIssueCommentEventWithAssignees("pull", actionDeleted, "mockBody", "mockUser", []string{"assigneeUser"}), + setup: func(_ *plugintest.API, _ *mocks.MockKvStore) {}, + }, { name: "Unsupported issue type", event: GetMockIssueCommentEventWithAssignees("mockType", actionCreated, "mockBody", "mockUser", []string{"assigneeUser"}), From 42b3f3549a51ce567fe951432fab032fdc1c043e Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Fri, 15 May 2026 18:41:45 +0300 Subject: [PATCH 2/3] Fixing issues found in review --- server/plugin/webhook.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/server/plugin/webhook.go b/server/plugin/webhook.go index 9a94c9d5a..c26da4d6e 100644 --- a/server/plugin/webhook.go +++ b/server/plugin/webhook.go @@ -1094,6 +1094,8 @@ func (p *Plugin) handleReviewCommentMentionNotification(event *github.PullReques body = strings.Split(body, "\n\nOn")[0] } + body = mdCommentRegex.ReplaceAllString(body, "") + mentionedUsernames := parseGitHubUsernamesFromText(body) message, err := renderTemplate("reviewCommentMentionNotification", event) @@ -1103,11 +1105,11 @@ func (p *Plugin) handleReviewCommentMentionNotification(event *github.PullReques } for _, username := range mentionedUsernames { - if username == event.GetSender().GetLogin() { + if strings.EqualFold(username, event.GetSender().GetLogin()) { continue } - if username == event.GetPullRequest().GetUser().GetLogin() { + if strings.EqualFold(username, event.GetPullRequest().GetUser().GetLogin()) { continue } @@ -1141,7 +1143,7 @@ func (p *Plugin) handleReviewCommentMentionNotification(event *github.PullReques func (p *Plugin) handleReviewCommentAuthorNotification(event *github.PullRequestReviewCommentEvent) { author := event.GetPullRequest().GetUser().GetLogin() - if author == event.GetSender().GetLogin() { + if strings.EqualFold(author, event.GetSender().GetLogin()) { return } From ea29a91d2d40270cf34fa1b0b15ad5c212efd28f Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Tue, 26 May 2026 21:31:03 +0300 Subject: [PATCH 3/3] Removed useless empty string return --- server/plugin/template.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/server/plugin/template.go b/server/plugin/template.go index 67a00471a..407bd527e 100644 --- a/server/plugin/template.go +++ b/server/plugin/template.go @@ -75,9 +75,6 @@ func init() { } cleaned = mdCommentRegex.ReplaceAllString(cleaned, "") cleaned = strings.TrimSpace(cleaned) - if cleaned == "" { - return "" - } return cleaned }