Skip to content

Issue content should not be updated when closing with comment #2833

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 3 commits into from
Nov 3, 2017
Merged

Issue content should not be updated when closing with comment #2833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5508747
Issue content should not be updated when closing with comment
lafriks Nov 2, 2017
d903754
Merge branch 'master' into fix/issue_comment_close
lunny Nov 3, 2017
a9b4dd5
Merge branch 'master' into fix/issue_comment_close
lunny Nov 3, 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
61 changes: 57 additions & 4 deletions integrations/issue_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func TestNoLoginViewIssue(t *testing.T) {
MakeRequest(t, req, http.StatusOK)
}

func testNewIssue(t *testing.T, session *TestSession, user, repo, title string) {
func testNewIssue(t *testing.T, session *TestSession, user, repo, title, content string) string {
Copy link
Member

Choose a reason for hiding this comment

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

nit: from the signature, it's not obvious what the function returns. Could we add a brief comment specifying that the created issue's URL is returned?


req := NewRequest(t, "GET", path.Join(user, repo, "issues", "new"))
resp := session.MakeRequest(t, req, http.StatusOK)
Expand All @@ -116,17 +116,70 @@ func testNewIssue(t *testing.T, session *TestSession, user, repo, title string)
link, exists := htmlDoc.doc.Find("form.ui.form").Attr("action")
assert.True(t, exists, "The template has changed")
req = NewRequestWithValues(t, "POST", link, map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"title": title,
"_csrf": htmlDoc.GetCSRF(),
"title": title,
"content": content,
})
resp = session.MakeRequest(t, req, http.StatusFound)

issueURL := RedirectURL(t, resp)
req = NewRequest(t, "GET", issueURL)
resp = session.MakeRequest(t, req, http.StatusOK)

htmlDoc = NewHTMLParser(t, resp.Body)
val := htmlDoc.doc.Find("#issue-title").Text()
assert.Equal(t, title, val)
val = htmlDoc.doc.Find(".comment-list .comments .comment .render-content p").First().Text()
assert.Equal(t, content, val)

return issueURL
}

func testIssueAddComment(t *testing.T, session *TestSession, issueURL, content, status string) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: it's not obvious what values are valid for status (without looking through other parts of the codebase), and if I see something like testIssueAddComment(t, sess, url, "hello", "close") in a test, it's not obvious that this will close the issue, in addition to making a comment.

Could we instead do:

func testIssueAddComment(t, session, issueURL, content) {
	testIssueAndCommentWithStatus(t,session,issueURL,content,"")
}

func testIssueAddClosingComment(t, session, issueURL, content) {
	testIssueAndCommentWithStatus(t,session,issueURL,content,"close")
}

// helper function
func testIssueAddCommentWithStatus(t, session, issueURL, content, status) {
	...
}

I think this will make it easier for future tests to reuse these helpers, and will make tests more readable.


req := NewRequest(t, "GET", issueURL)
resp := session.MakeRequest(t, req, http.StatusOK)

htmlDoc := NewHTMLParser(t, resp.Body)
link, exists := htmlDoc.doc.Find("#comment-form").Attr("action")
assert.True(t, exists, "The template has changed")

commentCount := htmlDoc.doc.Find(".comment-list .comments .comment .render-content").Length()

req = NewRequestWithValues(t, "POST", link, map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"content": content,
"status": status,
})
resp = session.MakeRequest(t, req, http.StatusFound)

req = NewRequest(t, "GET", RedirectURL(t, resp))
resp = session.MakeRequest(t, req, http.StatusOK)

htmlDoc = NewHTMLParser(t, resp.Body)

val := htmlDoc.doc.Find(".comment-list .comments .comment .render-content p").Eq(commentCount).Text()
assert.Equal(t, content, val)
}

func TestNewIssue(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user2")
testNewIssue(t, session, "user2", "repo1", "Title")
testNewIssue(t, session, "user2", "repo1", "Title", "Description")
}

func TestIssueCommentClose(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user2")
issueURL := testNewIssue(t, session, "user2", "repo1", "Title", "Description")
testIssueAddComment(t, session, issueURL, "Test comment 1", "")
testIssueAddComment(t, session, issueURL, "Test comment 2", "")
testIssueAddComment(t, session, issueURL, "Test comment 3", "close")

// Validate that issue content has not been updated
req := NewRequest(t, "GET", issueURL)
resp := session.MakeRequest(t, req, http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
val := htmlDoc.doc.Find(".comment-list .comments .comment .render-content p").First().Text()
assert.Equal(t, "Description", val)
}
6 changes: 3 additions & 3 deletions integrations/repo_activity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ func TestRepoActivity(t *testing.T) {
testPullCreate(t, session, "user1", "repo1", "feat/much_better_readme")

// Create issues (3 new issues)
testNewIssue(t, session, "user2", "repo1", "Issue 1")
testNewIssue(t, session, "user2", "repo1", "Issue 2")
testNewIssue(t, session, "user2", "repo1", "Issue 3")
testNewIssue(t, session, "user2", "repo1", "Issue 1", "Description 1")
testNewIssue(t, session, "user2", "repo1", "Issue 2", "Description 2")
testNewIssue(t, session, "user2", "repo1", "Issue 3", "Description 3")

// Create releases (1 new release)
createNewRelease(t, session, "/user2/repo1", "v1.0.0", "v1.0.0", false, false)
Expand Down
10 changes: 5 additions & 5 deletions models/issue_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,15 +272,15 @@ func (c *Comment) MailParticipants(e Engine, opType ActionType, issue *Issue) (e
return fmt.Errorf("UpdateIssueMentions [%d]: %v", c.IssueID, err)
}

content := c.Content

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

Expand Down
8 changes: 4 additions & 4 deletions models/issue_mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func (issue *Issue) mailSubject() string {
// 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(e Engine, issue *Issue, doer *User, comment *Comment, mentions []string) error {
func mailIssueCommentToParticipants(e Engine, issue *Issue, doer *User, content string, comment *Comment, mentions []string) error {
if !setting.Service.EnableNotifyMail {
return nil
}
Expand Down Expand Up @@ -80,7 +80,7 @@ func mailIssueCommentToParticipants(e Engine, issue *Issue, doer *User, comment
names = append(names, participants[i].Name)
}

SendIssueCommentMail(issue, doer, comment, tos)
SendIssueCommentMail(issue, doer, content, comment, tos)

// Mail mentioned people and exclude watchers.
names = append(names, doer.Name)
Expand All @@ -92,7 +92,7 @@ func mailIssueCommentToParticipants(e Engine, issue *Issue, doer *User, comment

tos = append(tos, mentions[i])
}
SendIssueMentionMail(issue, doer, comment, getUserEmailsByNames(e, tos))
SendIssueMentionMail(issue, doer, content, comment, getUserEmailsByNames(e, tos))

return nil
}
Expand All @@ -109,7 +109,7 @@ func (issue *Issue) mailParticipants(e Engine) (err error) {
return fmt.Errorf("UpdateIssueMentions [%d]: %v", issue.ID, err)
}

if err = mailIssueCommentToParticipants(e, issue, issue.Poster, nil, mentions); err != nil {
if err = mailIssueCommentToParticipants(e, issue, issue.Poster, issue.Content, nil, mentions); err != nil {
log.Error(4, "mailIssueCommentToParticipants: %v", err)
}

Expand Down
18 changes: 9 additions & 9 deletions models/mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,9 @@ 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 {
func composeIssueCommentMessage(issue *Issue, doer *User, content string, comment *Comment, tplName base.TplName, tos []string, info string) *mailer.Message {
subject := issue.mailSubject()
body := string(markup.RenderByType(markdown.MarkupName, []byte(issue.Content), issue.Repo.HTMLURL(), issue.Repo.ComposeMetas()))
body := string(markup.RenderByType(markdown.MarkupName, []byte(content), issue.Repo.HTMLURL(), issue.Repo.ComposeMetas()))

data := make(map[string]interface{}, 10)
if comment != nil {
Expand All @@ -161,30 +161,30 @@ func composeIssueCommentMessage(issue *Issue, doer *User, comment *Comment, tplN
}
data["Doer"] = doer

var content bytes.Buffer
var mailBody bytes.Buffer

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

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

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

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

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