Skip to content

Send notifications for mentions in pull request code comments #14218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 2, 2021
Merged

Send notifications for mentions in pull request code comments #14218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b2b7884
mentions are not persisted when ENABLE_NOTIFY_MAIL=false #14187
jpraet Jan 2, 2021
043e6c9
No notifications are sent for mentions in review comments #14013
jpraet Jan 2, 2021
f5cece1
add UI notifications for mentions
jpraet Jan 2, 2021
c48006d
Not receiving any emails with setting "Only Email on Mention" #13450
jpraet Jan 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions models/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/references"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/structs"
api "code.gitea.io/gitea/modules/structs"
Expand Down Expand Up @@ -1848,6 +1849,19 @@ func (issue *Issue) updateClosedNum(e Engine) (err error) {
return
}

// FindAndUpdateIssueMentions finds users mentioned in the given content string, and saves them in the database.
func (issue *Issue) FindAndUpdateIssueMentions(ctx DBContext, doer *User, content string) (mentions []*User, err error) {
rawMentions := references.FindAllMentionsMarkdown(content)
mentions, err = issue.ResolveMentionsByVisibility(ctx, doer, rawMentions)
if err != nil {
return nil, fmt.Errorf("UpdateIssueMentions [%d]: %v", issue.ID, err)
}
if err = UpdateIssueMentions(ctx, issue.ID, mentions); err != nil {
return nil, fmt.Errorf("UpdateIssueMentions [%d]: %v", issue.ID, err)
}
return
}

// ResolveMentionsByVisibility returns the users mentioned in an issue, removing those that
// don't have access to reading it. Teams are expanded into their users, but organizations are ignored.
func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, mentions []string) (users []*User, err error) {
Expand Down
8 changes: 4 additions & 4 deletions modules/notification/action/action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func NewNotifier() base.Notifier {
return &actionNotifier{}
}

func (a *actionNotifier) NotifyNewIssue(issue *models.Issue) {
func (a *actionNotifier) NotifyNewIssue(issue *models.Issue, mentions []*models.User) {
if err := issue.LoadPoster(); err != nil {
log.Error("issue.LoadPoster: %v", err)
return
Expand Down Expand Up @@ -88,7 +88,7 @@ func (a *actionNotifier) NotifyIssueChangeStatus(doer *models.User, issue *model

// NotifyCreateIssueComment notifies comment on an issue to notifiers
func (a *actionNotifier) NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
issue *models.Issue, comment *models.Comment) {
issue *models.Issue, comment *models.Comment, mentions []*models.User) {
act := &models.Action{
ActUserID: doer.ID,
ActUser: doer,
Expand Down Expand Up @@ -120,7 +120,7 @@ func (a *actionNotifier) NotifyCreateIssueComment(doer *models.User, repo *model
}
}

func (a *actionNotifier) NotifyNewPullRequest(pull *models.PullRequest) {
func (a *actionNotifier) NotifyNewPullRequest(pull *models.PullRequest, mentions []*models.User) {
if err := pull.LoadIssue(); err != nil {
log.Error("pull.LoadIssue: %v", err)
return
Expand Down Expand Up @@ -203,7 +203,7 @@ func (a *actionNotifier) NotifyForkRepository(doer *models.User, oldRepo, repo *
}
}

func (a *actionNotifier) NotifyPullRequestReview(pr *models.PullRequest, review *models.Review, comment *models.Comment) {
func (a *actionNotifier) NotifyPullRequestReview(pr *models.PullRequest, review *models.Review, comment *models.Comment, mentions []*models.User) {
if err := review.LoadReviewer(); err != nil {
log.Error("LoadReviewer '%d/%d': %v", review.ID, review.ReviewerID, err)
return
Expand Down
11 changes: 6 additions & 5 deletions modules/notification/base/notifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type Notifier interface {
NotifyRenameRepository(doer *models.User, repo *models.Repository, oldRepoName string)
NotifyTransferRepository(doer *models.User, repo *models.Repository, oldOwnerName string)

NotifyNewIssue(*models.Issue)
NotifyNewIssue(issue *models.Issue, mentions []*models.User)
NotifyIssueChangeStatus(*models.User, *models.Issue, *models.Comment, bool)
NotifyIssueChangeMilestone(doer *models.User, issue *models.Issue, oldMilestoneID int64)
NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment)
Expand All @@ -32,15 +32,16 @@ type Notifier interface {
NotifyIssueChangeLabels(doer *models.User, issue *models.Issue,
addedLabels []*models.Label, removedLabels []*models.Label)

NotifyNewPullRequest(*models.PullRequest)
NotifyNewPullRequest(pr *models.PullRequest, mentions []*models.User)
NotifyMergePullRequest(*models.PullRequest, *models.User)
NotifyPullRequestSynchronized(doer *models.User, pr *models.PullRequest)
NotifyPullRequestReview(*models.PullRequest, *models.Review, *models.Comment)
NotifyPullRequestReview(pr *models.PullRequest, review *models.Review, comment *models.Comment, mentions []*models.User)
NotifyPullRequestCodeComment(pr *models.PullRequest, comment *models.Comment, mentions []*models.User)
NotifyPullRequestChangeTargetBranch(doer *models.User, pr *models.PullRequest, oldBranch string)
NotifyPullRequestPushCommits(doer *models.User, pr *models.PullRequest, comment *models.Comment)

NotifyCreateIssueComment(*models.User, *models.Repository,
*models.Issue, *models.Comment)
NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
issue *models.Issue, comment *models.Comment, mentions []*models.User)
NotifyUpdateComment(*models.User, *models.Comment, string)
NotifyDeleteComment(*models.User, *models.Comment)

Expand Down
12 changes: 8 additions & 4 deletions modules/notification/base/null.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,27 @@ func (*NullNotifier) Run() {

// NotifyCreateIssueComment places a place holder function
func (*NullNotifier) NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
issue *models.Issue, comment *models.Comment) {
issue *models.Issue, comment *models.Comment, mentions []*models.User) {
}

// NotifyNewIssue places a place holder function
func (*NullNotifier) NotifyNewIssue(issue *models.Issue) {
func (*NullNotifier) NotifyNewIssue(issue *models.Issue, mentions []*models.User) {
}

// NotifyIssueChangeStatus places a place holder function
func (*NullNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
}

// NotifyNewPullRequest places a place holder function
func (*NullNotifier) NotifyNewPullRequest(pr *models.PullRequest) {
func (*NullNotifier) NotifyNewPullRequest(pr *models.PullRequest, mentions []*models.User) {
}

// NotifyPullRequestReview places a place holder function
func (*NullNotifier) NotifyPullRequestReview(pr *models.PullRequest, r *models.Review, comment *models.Comment) {
func (*NullNotifier) NotifyPullRequestReview(pr *models.PullRequest, r *models.Review, comment *models.Comment, mentions []*models.User) {
}

// NotifyPullRequestCodeComment places a place holder function
func (*NullNotifier) NotifyPullRequestCodeComment(pr *models.PullRequest, comment *models.Comment, mentions []*models.User) {
}

// NotifyMergePullRequest places a place holder function
Expand Down
6 changes: 3 additions & 3 deletions modules/notification/indexer/indexer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func NewNotifier() base.Notifier {
}

func (r *indexerNotifier) NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
issue *models.Issue, comment *models.Comment) {
issue *models.Issue, comment *models.Comment, mentions []*models.User) {
if comment.Type == models.CommentTypeComment {
if issue.Comments == nil {
if err := issue.LoadDiscussComments(); err != nil {
Expand All @@ -45,11 +45,11 @@ func (r *indexerNotifier) NotifyCreateIssueComment(doer *models.User, repo *mode
}
}

func (r *indexerNotifier) NotifyNewIssue(issue *models.Issue) {
func (r *indexerNotifier) NotifyNewIssue(issue *models.Issue, mentions []*models.User) {
issue_indexer.UpdateIssueIndexer(issue)
}

func (r *indexerNotifier) NotifyNewPullRequest(pr *models.PullRequest) {
func (r *indexerNotifier) NotifyNewPullRequest(pr *models.PullRequest, mentions []*models.User) {
issue_indexer.UpdateIssueIndexer(pr.Issue)
}

Expand Down
28 changes: 17 additions & 11 deletions modules/notification/mail/mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func NewNotifier() base.Notifier {
}

func (m *mailNotifier) NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
issue *models.Issue, comment *models.Comment) {
issue *models.Issue, comment *models.Comment, mentions []*models.User) {
var act models.ActionType
if comment.Type == models.CommentTypeClose {
act = models.ActionCloseIssue
Expand All @@ -41,13 +41,13 @@ func (m *mailNotifier) NotifyCreateIssueComment(doer *models.User, repo *models.
act = 0
}

if err := mailer.MailParticipantsComment(comment, act, issue); err != nil {
if err := mailer.MailParticipantsComment(comment, act, issue, mentions); err != nil {
log.Error("MailParticipantsComment: %v", err)
}
}

func (m *mailNotifier) NotifyNewIssue(issue *models.Issue) {
if err := mailer.MailParticipants(issue, issue.Poster, models.ActionCreateIssue); err != nil {
func (m *mailNotifier) NotifyNewIssue(issue *models.Issue, mentions []*models.User) {
if err := mailer.MailParticipants(issue, issue.Poster, models.ActionCreateIssue, mentions); err != nil {
log.Error("MailParticipants: %v", err)
}
}
Expand All @@ -69,18 +69,18 @@ func (m *mailNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.
}
}

if err := mailer.MailParticipants(issue, doer, actionType); err != nil {
if err := mailer.MailParticipants(issue, doer, actionType, nil); err != nil {
log.Error("MailParticipants: %v", err)
}
}

func (m *mailNotifier) NotifyNewPullRequest(pr *models.PullRequest) {
if err := mailer.MailParticipants(pr.Issue, pr.Issue.Poster, models.ActionCreatePullRequest); err != nil {
func (m *mailNotifier) NotifyNewPullRequest(pr *models.PullRequest, mentions []*models.User) {
if err := mailer.MailParticipants(pr.Issue, pr.Issue.Poster, models.ActionCreatePullRequest, mentions); err != nil {
log.Error("MailParticipants: %v", err)
}
}

func (m *mailNotifier) NotifyPullRequestReview(pr *models.PullRequest, r *models.Review, comment *models.Comment) {
func (m *mailNotifier) NotifyPullRequestReview(pr *models.PullRequest, r *models.Review, comment *models.Comment, mentions []*models.User) {
var act models.ActionType
if comment.Type == models.CommentTypeClose {
act = models.ActionCloseIssue
Expand All @@ -89,11 +89,17 @@ func (m *mailNotifier) NotifyPullRequestReview(pr *models.PullRequest, r *models
} else if comment.Type == models.CommentTypeComment {
act = models.ActionCommentPull
}
if err := mailer.MailParticipantsComment(comment, act, pr.Issue); err != nil {
if err := mailer.MailParticipantsComment(comment, act, pr.Issue, mentions); err != nil {
log.Error("MailParticipantsComment: %v", err)
}
}

func (m *mailNotifier) NotifyPullRequestCodeComment(pr *models.PullRequest, comment *models.Comment, mentions []*models.User) {
if err := mailer.MailMentionsComment(pr, comment, mentions); err != nil {
log.Error("MailMentionsComment: %v", err)
}
}

func (m *mailNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment) {
// mail only sent to added assignees and not self-assignee
if !removed && doer.ID != assignee.ID && assignee.EmailNotifications() == models.EmailNotificationsEnabled {
Expand All @@ -115,7 +121,7 @@ func (m *mailNotifier) NotifyMergePullRequest(pr *models.PullRequest, doer *mode
return
}
pr.Issue.Content = ""
if err := mailer.MailParticipants(pr.Issue, doer, models.ActionMergePullRequest); err != nil {
if err := mailer.MailParticipants(pr.Issue, doer, models.ActionMergePullRequest, nil); err != nil {
log.Error("MailParticipants: %v", err)
}
}
Expand Down Expand Up @@ -143,7 +149,7 @@ func (m *mailNotifier) NotifyPullRequestPushCommits(doer *models.User, pr *model
}
comment.Content = ""

m.NotifyCreateIssueComment(doer, comment.Issue.Repo, comment.Issue, comment)
m.NotifyCreateIssueComment(doer, comment.Issue.Repo, comment.Issue, comment, nil)
}

func (m *mailNotifier) NotifyNewRelease(rel *models.Release) {
Expand Down
23 changes: 15 additions & 8 deletions modules/notification/notification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,16 @@ func NewContext() {

// NotifyCreateIssueComment notifies issue comment related message to notifiers
func NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
issue *models.Issue, comment *models.Comment) {
issue *models.Issue, comment *models.Comment, mentions []*models.User) {
for _, notifier := range notifiers {
notifier.NotifyCreateIssueComment(doer, repo, issue, comment)
notifier.NotifyCreateIssueComment(doer, repo, issue, comment, mentions)
}
}

// NotifyNewIssue notifies new issue to notifiers
func NotifyNewIssue(issue *models.Issue) {
func NotifyNewIssue(issue *models.Issue, mentions []*models.User) {
for _, notifier := range notifiers {
notifier.NotifyNewIssue(issue)
notifier.NotifyNewIssue(issue, mentions)
}
}

Expand All @@ -67,9 +67,9 @@ func NotifyMergePullRequest(pr *models.PullRequest, doer *models.User) {
}

// NotifyNewPullRequest notifies new pull request to notifiers
func NotifyNewPullRequest(pr *models.PullRequest) {
func NotifyNewPullRequest(pr *models.PullRequest, mentions []*models.User) {
for _, notifier := range notifiers {
notifier.NotifyNewPullRequest(pr)
notifier.NotifyNewPullRequest(pr, mentions)
}
}

Expand All @@ -81,9 +81,16 @@ func NotifyPullRequestSynchronized(doer *models.User, pr *models.PullRequest) {
}

// NotifyPullRequestReview notifies new pull request review
func NotifyPullRequestReview(pr *models.PullRequest, review *models.Review, comment *models.Comment) {
func NotifyPullRequestReview(pr *models.PullRequest, review *models.Review, comment *models.Comment, mentions []*models.User) {
for _, notifier := range notifiers {
notifier.NotifyPullRequestReview(pr, review, comment)
notifier.NotifyPullRequestReview(pr, review, comment, mentions)
}
}

// NotifyPullRequestCodeComment notifies new pull request code comment
func NotifyPullRequestCodeComment(pr *models.PullRequest, comment *models.Comment, mentions []*models.User) {
for _, notifier := range notifiers {
notifier.NotifyPullRequestCodeComment(pr, comment, mentions)
}
}

Expand Down
55 changes: 51 additions & 4 deletions modules/notification/ui/ui.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (ns *notificationService) Run() {
}

func (ns *notificationService) NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
issue *models.Issue, comment *models.Comment) {
issue *models.Issue, comment *models.Comment, mentions []*models.User) {
var opts = issueNotificationOpts{
IssueID: issue.ID,
NotificationAuthorID: doer.ID,
Expand All @@ -60,13 +60,31 @@ func (ns *notificationService) NotifyCreateIssueComment(doer *models.User, repo
opts.CommentID = comment.ID
}
_ = ns.issueQueue.Push(opts)
for _, mention := range mentions {
var opts = issueNotificationOpts{
IssueID: issue.ID,
NotificationAuthorID: doer.ID,
ReceiverID: mention.ID,
}
if comment != nil {
opts.CommentID = comment.ID
}
_ = ns.issueQueue.Push(opts)
}
}

func (ns *notificationService) NotifyNewIssue(issue *models.Issue) {
func (ns *notificationService) NotifyNewIssue(issue *models.Issue, mentions []*models.User) {
_ = ns.issueQueue.Push(issueNotificationOpts{
IssueID: issue.ID,
NotificationAuthorID: issue.Poster.ID,
})
for _, mention := range mentions {
_ = ns.issueQueue.Push(issueNotificationOpts{
IssueID: issue.ID,
NotificationAuthorID: issue.Poster.ID,
ReceiverID: mention.ID,
})
}
}

func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
Expand All @@ -83,7 +101,7 @@ func (ns *notificationService) NotifyMergePullRequest(pr *models.PullRequest, do
})
}

func (ns *notificationService) NotifyNewPullRequest(pr *models.PullRequest) {
func (ns *notificationService) NotifyNewPullRequest(pr *models.PullRequest, mentions []*models.User) {
if err := pr.LoadIssue(); err != nil {
log.Error("Unable to load issue: %d for pr: %d: Error: %v", pr.IssueID, pr.ID, err)
return
Expand All @@ -92,9 +110,16 @@ func (ns *notificationService) NotifyNewPullRequest(pr *models.PullRequest) {
IssueID: pr.Issue.ID,
NotificationAuthorID: pr.Issue.PosterID,
})
for _, mention := range mentions {
_ = ns.issueQueue.Push(issueNotificationOpts{
IssueID: pr.Issue.ID,
NotificationAuthorID: pr.Issue.PosterID,
ReceiverID: mention.ID,
})
}
}

func (ns *notificationService) NotifyPullRequestReview(pr *models.PullRequest, r *models.Review, c *models.Comment) {
func (ns *notificationService) NotifyPullRequestReview(pr *models.PullRequest, r *models.Review, c *models.Comment, mentions []*models.User) {
var opts = issueNotificationOpts{
IssueID: pr.Issue.ID,
NotificationAuthorID: r.Reviewer.ID,
Expand All @@ -103,6 +128,28 @@ func (ns *notificationService) NotifyPullRequestReview(pr *models.PullRequest, r
opts.CommentID = c.ID
}
_ = ns.issueQueue.Push(opts)
for _, mention := range mentions {
var opts = issueNotificationOpts{
IssueID: pr.Issue.ID,
NotificationAuthorID: r.Reviewer.ID,
ReceiverID: mention.ID,
}
if c != nil {
opts.CommentID = c.ID
}
_ = ns.issueQueue.Push(opts)
}
}

func (ns *notificationService) NotifyPullRequestCodeComment(pr *models.PullRequest, c *models.Comment, mentions []*models.User) {
for _, mention := range mentions {
_ = ns.issueQueue.Push(issueNotificationOpts{
IssueID: pr.Issue.ID,
NotificationAuthorID: c.Poster.ID,
CommentID: c.ID,
ReceiverID: mention.ID,
})
}
}

func (ns *notificationService) NotifyPullRequestPushCommits(doer *models.User, pr *models.PullRequest, comment *models.Comment) {
Expand Down
Loading