Skip to content

Adding #issuecomment to the URL in E-Mail notifications #1674

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 8 commits into from
May 25, 2017
Merged

Adding #issuecomment to the URL in E-Mail notifications #1674

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
74774a4
Added comment's hashtag to url for mail notifications.
jonasfranz May 4, 2017
f385727
Added comment's hashtag to url for mail notifications.
jonasfranz May 4, 2017
e05eb96
Added comment's hashtag to url for mail notifications.
jonasfranz May 4, 2017
5cb36ca
Added comment's hashtag to url for mail notifications.
jonasfranz May 4, 2017
2dd674e
Replacing in-line link generation with HTMLURL. (+gofmt)
jonasfranz May 4, 2017
716bf81
Replaced action-based model with nil-based model. (+gofmt)
jonasfranz May 4, 2017
edaa576
Replaced mailIssueActionToParticipants with mailIssueCommentToPartici…
jonasfranz May 4, 2017
b0e22ab
Updating comment for mailIssueCommentToParticipants
jonasfranz May 4, 2017
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
2 changes: 1 addition & 1 deletion models/issue_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ func (c *Comment) MailParticipants(e Engine, opType ActionType, issue *Issue) (e
case ActionReopenIssue:
issue.Content = fmt.Sprintf("Reopened #%d", issue.Index)
}
if err = mailIssueCommentToParticipants(issue, c.Poster, mentions); err != nil {
if err = mailIssueCommentToParticipants(issue, c.Poster, c, mentions); err != nil {
log.Error(4, "mailIssueCommentToParticipants: %v", err)
}

Expand Down
92 changes: 68 additions & 24 deletions models/issue_mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,77 @@ func (issue *Issue) mailSubject() string {
return fmt.Sprintf("[%s] %s (#%d)", issue.Repo.Name, issue.Title, issue.Index)
}

// mailIssueCommentToParticipants can be used for both new issue creation and comment.

// mailIssueCommentToParticipants can be used only for comment.
Copy link
Member

@lafriks lafriks May 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't this comment be changed back to original?

// This function sends two list of emails:
// 1. Repository watchers and users who are participated in comments.
// 2. Users who are not in 1. but get mentioned in current issue/comment.
func mailIssueCommentToParticipants(issue *Issue, doer *User, mentions []string) error {
func mailIssueCommentToParticipants(issue *Issue, doer *User, comment *Comment, mentions []string) error {

Copy link
Member

Choose a reason for hiding this comment

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

no newline

names, tos, err := prepareMailToParticipants(issue, doer)

if(err != nil) {
return fmt.Errorf("PrepareMailToParticipants: %v", err)
}

SendIssueCommentMail(issue, doer, comment, tos)
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we merge these 2 (almost identical) functions by doing

if comment != nil {
  SendIssueCommentMail(...)
} else {
  SendIssueActionMail(...)
}


// Mail mentioned people and exclude watchers.
names = append(names, doer.Name)
tos = make([]string, 0, len(mentions)) // list of user names.
for i := range mentions {
if com.IsSliceContainsStr(names, mentions[i]) {
continue
}

tos = append(tos, mentions[i])
}
SendIssueMentionMail(issue, doer, comment, GetUserEmailsByNames(tos))

return nil
}

// mailIssueActionToParticipants can be used for creation or pull requests.
// This function sends two list of emails:
// 1. Repository watchers and users who are participated in comments.
// 2. Users who are not in 1. but get mentioned in current issue/comment.
func mailIssueActionToParticipants(issue *Issue, doer *User, mentions []string) error {
names, tos, err := prepareMailToParticipants(issue, doer)

if(err != nil) {
return fmt.Errorf("PrepareMailToParticipants: %v", err)
}

SendIssueActionMail(issue, doer, tos)

// Mail mentioned people and exclude watchers.
names = append(names, doer.Name)
tos = make([]string, 0, len(mentions)) // list of user names.
for i := range mentions {
if com.IsSliceContainsStr(names, mentions[i]) {
continue
}

tos = append(tos, mentions[i])
}
SendIssueMentionInActionMail(issue, doer, GetUserEmailsByNames(tos))

return nil
}

// prepareMailToParticipants creates the tos and names list for an issue and the issue's creator.
func prepareMailToParticipants(issue *Issue, doer *User) (tos []string, names []string, error error) {
if !setting.Service.EnableNotifyMail {
return nil
return nil, nil, nil
}

watchers, err := GetWatchers(issue.RepoID)
if err != nil {
return fmt.Errorf("GetWatchers [repo_id: %d]: %v", issue.RepoID, err)
return nil, nil, err
Copy link
Member

Choose a reason for hiding this comment

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

the error should still we wrapped:

  return nil, nil, fmt.Errorf("GetWatchers [repo_id: %d]: %v", issue.RepoID, err)

}
participants, err := GetParticipantsByIssueID(issue.ID)
if err != nil {
return fmt.Errorf("GetParticipantsByIssueID [issue_id: %d]: %v", issue.ID, err)
return nil, nil, err
Copy link
Member

Choose a reason for hiding this comment

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

Same here, wrap the error :)

}

// In case the issue poster is not watching the repository,
Expand All @@ -42,16 +97,16 @@ func mailIssueCommentToParticipants(issue *Issue, doer *User, mentions []string)
participants = append(participants, issue.Poster)
}

tos := make([]string, 0, len(watchers)) // List of email addresses.
names := make([]string, 0, len(watchers))
tos = make([]string, 0, len(watchers)) // List of email addresses.
names = make([]string, 0, len(watchers))
for i := range watchers {
if watchers[i].UserID == doer.ID {
continue
}

to, err := GetUserByID(watchers[i].UserID)
if err != nil {
return fmt.Errorf("GetUserByID [%d]: %v", watchers[i].UserID, err)
return nil, nil, err
Copy link
Member

Choose a reason for hiding this comment

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

Wrap error

}
if to.IsOrganization() {
continue
Expand All @@ -70,23 +125,10 @@ func mailIssueCommentToParticipants(issue *Issue, doer *User, mentions []string)
tos = append(tos, participants[i].Email)
names = append(names, participants[i].Name)
}
SendIssueCommentMail(issue, doer, tos)

// Mail mentioned people and exclude watchers.
names = append(names, doer.Name)
tos = make([]string, 0, len(mentions)) // list of user names.
for i := range mentions {
if com.IsSliceContainsStr(names, mentions[i]) {
continue
}

tos = append(tos, mentions[i])
}
SendIssueMentionMail(issue, doer, GetUserEmailsByNames(tos))

return nil
return tos, names, nil
}


// MailParticipants sends new issue thread created emails to repository watchers
// and mentioned people.
func (issue *Issue) MailParticipants() (err error) {
Expand All @@ -95,9 +137,11 @@ func (issue *Issue) MailParticipants() (err error) {
return fmt.Errorf("UpdateIssueMentions [%d]: %v", issue.ID, err)
}

if err = mailIssueCommentToParticipants(issue, issue.Poster, mentions); err != nil {
if err = mailIssueActionToParticipants(issue, issue.Poster, mentions); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

here you'd just call mailIssueCommentToParticipants(issue, issue.Poster, nil, mentions) instead

log.Error(4, "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.

technically wrong here, but maybe not later on ;)

Copy link
Member

Choose a reason for hiding this comment

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

Wrong error-message, otherwise LGTM 🎉

}

return nil
}


40 changes: 37 additions & 3 deletions models/mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,24 @@ func composeTplData(subject, body, link string) map[string]interface{} {
return data
}

func composeIssueCommentMessage(issue *Issue, doer *User, comment *Comment, tplName base.TplName, tos []string, info string) *mailer.Message {
subject := issue.mailSubject()
body := string(markdown.RenderString(issue.Content, issue.Repo.HTMLURL(), issue.Repo.ComposeMetas()))
data := composeTplData(subject, body, issue.HTMLURL() + "#" + comment.HashTag())
Copy link
Member

Choose a reason for hiding this comment

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

There's comment.HTMLURL() to replace issue.HTMLURL() + "#" + comment.HashTag() ... Also go-fmt :)

Copy link
Member

Choose a reason for hiding this comment

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

If you'd like, you can change comment.HTMLURL() to do this instead 😄

	return fmt.Sprintf("%s#%s", issue.HTMLURL(), c.HashTag())


data["Doer"] = doer

var content bytes.Buffer

if err := templates.ExecuteTemplate(&content, string(tplName), data); err != nil {
log.Error(3, "Template: %v", err)
}

msg := mailer.NewMessageFrom(tos, fmt.Sprintf(`"%s" <%s>`, doer.DisplayName(), setting.MailService.FromEmail), subject, content.String())
msg.Info = fmt.Sprintf("Subject: %s, %s", subject, info)
return msg
}

func composeIssueMessage(issue *Issue, doer *User, tplName base.TplName, tos []string, info string) *mailer.Message {
subject := issue.mailSubject()
body := string(markdown.RenderString(issue.Content, issue.Repo.HTMLURL(), issue.Repo.ComposeMetas()))
Expand All @@ -166,16 +184,32 @@ func composeIssueMessage(issue *Issue, doer *User, tplName base.TplName, tos []s
}

// SendIssueCommentMail composes and sends issue comment emails to target receivers.
func SendIssueCommentMail(issue *Issue, doer *User, tos []string) {
func SendIssueCommentMail(issue *Issue, doer *User, comment *Comment, tos []string) {
if len(tos) == 0 {
return
}

mailer.SendAsync(composeIssueMessage(issue, doer, mailIssueComment, tos, "issue comment"))
mailer.SendAsync(composeIssueCommentMessage(issue, doer, comment, mailIssueComment, tos, "issue comment"))
}

// SendIssueMentionMail composes and sends issue mention emails to target receivers.
func SendIssueMentionMail(issue *Issue, doer *User, tos []string) {
func SendIssueMentionMail(issue *Issue, doer *User, comment *Comment, tos []string) {
if len(tos) == 0 {
return
}
mailer.SendAsync(composeIssueCommentMessage(issue, doer, comment, mailIssueMention, tos, "issue mention"))
}
// SendIssueActionMail composes and sends issue action emails to target receivers.
func SendIssueActionMail(issue *Issue, doer *User, tos []string) {
if len(tos) == 0 {
return
}

mailer.SendAsync(composeIssueMessage(issue, doer, mailIssueComment, tos, "issue comment"))
}

// SendIssueMentionInActionMail composes and sends issue mention emails to target receivers. Only applies if no comment is given.
func SendIssueMentionInActionMail(issue *Issue, doer *User, tos []string) {
if len(tos) == 0 {
return
}
Expand Down