Skip to content
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

Jump to bottom

Various fixes for issue mail notifications #7165

Merged
merged 2 commits into from
Jun 11, 2019
Merged

Various fixes for issue mail notifications #7165

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59d4674
Various fixes for issue mail notifications
silverwind Jun 9, 2019
def90c9
Merge branch 'master' into mail-issue-close
lunny Jun 11, 2019
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
19 changes: 13 additions & 6 deletions models/issue_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,16 +403,23 @@ func (c *Comment) mailParticipants(e Engine, opType ActionType, issue *Issue) (e
return fmt.Errorf("UpdateIssueMentions [%d]: %v", c.IssueID, err)
}

content := c.Content
if len(c.Content) > 0 {
if err = mailIssueCommentToParticipants(e, issue, c.Poster, c.Content, c, mentions); err != nil {
log.Error("mailIssueCommentToParticipants: %v", err)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, sending two mails is intended. One for each action (comment and close/reopen).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's just a comment, the switch below will not hit a case and only 1 mail is sent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One action sending two mails is annoying.

Copy link
Member Author

@silverwind silverwind Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. It's not one action after all if a user clicks "comment and close". Also it does match GitHub's behaviour, they also send two mails on "comment and close".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of GitHub sending two mails for "comment and close":

image

}

switch opType {
case ActionCloseIssue:
content = fmt.Sprintf("Closed #%d", issue.Index)
ct := fmt.Sprintf("Closed #%d.", issue.Index)
if err = mailIssueCommentToParticipants(e, issue, c.Poster, ct, c, mentions); err != nil {
log.Error("mailIssueCommentToParticipants: %v", err)
}
case ActionReopenIssue:
content = fmt.Sprintf("Reopened #%d", issue.Index)
}
if err = mailIssueCommentToParticipants(e, issue, c.Poster, content, c, mentions); err != nil {
log.Error("mailIssueCommentToParticipants: %v", err)
ct := fmt.Sprintf("Reopened #%d.", issue.Index)
if err = mailIssueCommentToParticipants(e, issue, c.Poster, ct, c, mentions); err != nil {
log.Error("mailIssueCommentToParticipants: %v", err)
}
}

return nil
Expand Down
35 changes: 25 additions & 10 deletions models/issue_mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,26 +118,41 @@ func mailIssueCommentToParticipants(e Engine, issue *Issue, doer *User, content

// MailParticipants sends new issue thread created emails to repository watchers
// and mentioned people.
func (issue *Issue) MailParticipants(opType ActionType) (err error) {
return issue.mailParticipants(x, opType)
func (issue *Issue) MailParticipants(doer *User, opType ActionType) (err error) {
return issue.mailParticipants(x, doer, opType)
}

func (issue *Issue) mailParticipants(e Engine, opType ActionType) (err error) {
func (issue *Issue) mailParticipants(e Engine, doer *User, opType ActionType) (err error) {
mentions := markup.FindAllMentions(issue.Content)

if err = UpdateIssueMentions(e, issue.ID, mentions); err != nil {
return fmt.Errorf("UpdateIssueMentions [%d]: %v", issue.ID, err)
}

var content = issue.Content
if len(issue.Content) > 0 {
if err = mailIssueCommentToParticipants(e, issue, doer, issue.Content, nil, mentions); err != nil {
log.Error("mailIssueCommentToParticipants: %v", err)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, it's intended.

}

switch opType {
case ActionCreateIssue, ActionCreatePullRequest:
if len(issue.Content) == 0 {
ct := fmt.Sprintf("Created #%d.", issue.Index)
if err = mailIssueCommentToParticipants(e, issue, doer, ct, nil, mentions); err != nil {
log.Error("mailIssueCommentToParticipants: %v", err)
}
}
case ActionCloseIssue, ActionClosePullRequest:
content = fmt.Sprintf("Closed #%d", issue.Index)
ct := fmt.Sprintf("Closed #%d.", issue.Index)
if err = mailIssueCommentToParticipants(e, issue, doer, ct, nil, mentions); err != nil {
log.Error("mailIssueCommentToParticipants: %v", err)
}
case ActionReopenIssue, ActionReopenPullRequest:
content = fmt.Sprintf("Reopened #%d", issue.Index)
}

if err = mailIssueCommentToParticipants(e, issue, issue.Poster, content, nil, mentions); err != nil {
log.Error("mailIssueCommentToParticipants: %v", err)
ct := fmt.Sprintf("Reopened #%d.", issue.Index)
if err = mailIssueCommentToParticipants(e, issue, doer, ct, nil, mentions); err != nil {
log.Error("mailIssueCommentToParticipants: %v", err)
}
}

return nil
Expand Down
6 changes: 3 additions & 3 deletions modules/notification/mail/mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func (m *mailNotifier) NotifyCreateIssueComment(doer *models.User, repo *models.
}

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

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

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