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

jonasfranz · 2017-05-04T16:32:41Z

Reference: #1669

Signed-off-by: Jonas <info@jonasfranz.software>

Added explanation to return statement. Signed-off-by: Jonas <info@jonasfranz.software>

Added explanation to return statement + documentation. Signed-off-by: Jonas <info@jonasfranz.software>

Signed-off-by: Jonas Franz <info@jonasfranz.software>

bkcsoft · 2017-05-04T19:19:02Z

models/mail.go

+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())


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

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

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

Signed-off-by: Jonas Franz <info@jonasfranz.software>

bkcsoft

All the changes

bkcsoft · 2017-05-04T19:36:42Z

models/issue_mail.go

+		return fmt.Errorf("PrepareMailToParticipants: %v", err)
+	}
+
+	SendIssueCommentMail(issue, doer, comment, tos)


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

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

bkcsoft · 2017-05-04T19:36:49Z

models/issue_mail.go

 // 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 {
+


bkcsoft · 2017-05-04T19:37:19Z

models/issue_mail.go

+
+	names, tos, err := prepareMailToParticipants(issue, doer)
+
+	if err != nil {


no newline between set and checks :)

bkcsoft · 2017-05-04T19:38:43Z

models/issue_mail.go

 	}

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


the error should still we wrapped:

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

bkcsoft · 2017-05-04T19:39:10Z

models/issue_mail.go

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


Same here, wrap the error :)

bkcsoft · 2017-05-04T19:39:56Z

models/issue_mail.go

+}
+
+// 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) {


I'm not really a fan for named returns, got a reason to do that?

bkcsoft · 2017-05-04T19:40:14Z

models/issue_mail.go

 	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


bkcsoft · 2017-05-04T19:40:59Z

models/issue_mail.go

@@ -95,7 +135,7 @@ 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 {


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

bkcsoft · 2017-05-04T19:41:15Z

models/issue_mail.go

@@ -95,7 +135,7 @@ 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 {
 		log.Error(4, "mailIssueCommentToParticipants: %v", err)


technically wrong here, but maybe not later on ;)

Signed-off-by: Jonas Franz <info@jonasfranz.software>

bkcsoft · 2017-05-04T20:30:34Z

models/issue_mail.go

@@ -95,7 +104,7 @@ 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 {
 		log.Error(4, "mailIssueCommentToParticipants: %v", err)


Wrong error-message, otherwise LGTM 🎉

…pants. Signed-off-by: Jonas Franz <info@jonasfranz.software>

bkcsoft · 2017-05-04T20:39:30Z

LGTM, but untested

lafriks · 2017-05-04T21:55:15Z

models/issue_mail.go

@@ -18,11 +18,11 @@ 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.


Shouldn't this comment be changed back to original?

lafriks · 2017-05-04T21:57:56Z

models/mail.go

+
+	data := make(map[string]interface{}, 10)
+	if comment != nil {
+		data = composeTplData(subject, body, issue.HTMLURL()+"#"+comment.HashTag())


Why comment.HTMLURL is not used here?

No because comment.HTMLURL returns an empty string by printing an error to the console (see gitter chat).

@JonasFranzDEV Gitter chat is not searchable, mind sharing with the rest of the class? As I'm quite sure there's a better solution to this than re-implementing the wheel 🙂

Signed-off-by: Jonas Franz <info@jonasfranz.software>

lafriks

Looks good but have to test it

lafriks · 2017-05-04T22:11:13Z

models/mail.go

+
+	data := make(map[string]interface{}, 10)
+	if comment != nil {
+		data = composeTplData(subject, body, issue.HTMLURL()+"#"+comment.HashTag())


lunny · 2017-05-25T02:29:11Z

LGTM

jonasfranz added 4 commits May 4, 2017 17:58

Added comment's hashtag to url for mail notifications.

74774a4

Signed-off-by: Jonas <info@jonasfranz.software>

Added comment's hashtag to url for mail notifications.

f385727

Added explanation to return statement. Signed-off-by: Jonas <info@jonasfranz.software>

Added comment's hashtag to url for mail notifications.

e05eb96

Added explanation to return statement + documentation. Signed-off-by: Jonas <info@jonasfranz.software>

Added comment's hashtag to url for mail notifications.

5cb36ca

Signed-off-by: Jonas Franz <info@jonasfranz.software>

bkcsoft suggested changes May 4, 2017

View reviewed changes

Replacing in-line link generation with HTMLURL. (+gofmt)

2dd674e

Signed-off-by: Jonas Franz <info@jonasfranz.software>

bkcsoft suggested changes May 4, 2017

View reviewed changes

Replaced action-based model with nil-based model. (+gofmt)

716bf81

Signed-off-by: Jonas Franz <info@jonasfranz.software>

bkcsoft suggested changes May 4, 2017

View reviewed changes

Replaced mailIssueActionToParticipants with mailIssueCommentToPartici…

edaa576

…pants. Signed-off-by: Jonas Franz <info@jonasfranz.software>

bkcsoft approved these changes May 4, 2017

View reviewed changes

tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 4, 2017

lafriks requested changes May 4, 2017

View reviewed changes

Updating comment for mailIssueCommentToParticipants

b0e22ab

Signed-off-by: Jonas Franz <info@jonasfranz.software>

lafriks approved these changes May 4, 2017

View reviewed changes

lunny added this to the 1.2.0 milestone May 5, 2017

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label May 5, 2017

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 25, 2017

lunny merged commit 03912ce into go-gitea:master May 25, 2017

lunny mentioned this pull request May 25, 2017

Add #issuecomment to E-Mail notification #1669

Closed

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jonasfranz commented May 4, 2017

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft left a comment

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft May 4, 2017

bkcsoft commented May 4, 2017

lafriks May 4, 2017 •

edited

Loading

lafriks May 4, 2017

jonasfranz May 4, 2017

lafriks May 4, 2017

bkcsoft May 8, 2017

lafriks left a comment

lafriks May 4, 2017

lunny commented May 25, 2017


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

		if err != nil {

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

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

Conversation

jonasfranz commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented May 4, 2017

lafriks May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented May 25, 2017

lafriks May 4, 2017 •

edited

Loading