Minor fix for PR #3624

.gopmfile

@@ -20,7 +20,7 @@ github.com/go-xorm/xorm = commit:3ad0b42
 github.com/gogits/chardet = commit:2404f77
 github.com/gogits/cron = commit:7f3990a
 github.com/gogits/git-module = commit:7129215
-github.com/gogits/go-gogs-client = commit:c52f7ee
+github.com/gogits/go-gogs-client = commit:3693f6a
 github.com/gogits/go-libravatar = commit:cd1abbd
 github.com/issue9/identicon = commit:d36b545
 github.com/jaytaylor/html2text = commit:52d9b78

glide.lock

@@ -45,7 +45,7 @@ imports:
 - name: github.com/gogits/git-module
   version: 71292151e50d262429f29515dd077d7f5beb8c66
 - name: github.com/gogits/go-gogs-client
-  version: c52f7ee0cc58d3cd6e379025552873a8df6de322
+  version: 3693f6a1a71302d146590845e35fc7fd530aae4e
 - name: github.com/gogits/go-libravatar
   version: cd1abbd55d09b793672732a7a1dfdaa12a40dfd0
 - name: github.com/issue9/identicon

models/issue.go

@@ -98,7 +98,7 @@ func (issue *Issue) loadAttributes(e Engine) (err error) {
 				issue.PosterID = -1
 				issue.Poster = NewGhostUser()
 			} else {
-				return fmt.Errorf("getUserByID.(poster) [%d]: %v", issue.PosterID, err)
+				return fmt.Errorf("getUserByID.(Poster) [%d]: %v", issue.PosterID, err)
 			}
 			return
 		}
@@ -780,7 +780,7 @@ func GetIssueByIndex(repoID, index int64) (*Issue, error) {
 	return issue, issue.LoadAttributes()
 }
 
-func getIssueByID(e Engine, id int64) (*Issue, error) {
+func getRawIssueByID(e Engine, id int64) (*Issue, error) {
 	issue := new(Issue)
 	has, err := e.Id(id).Get(issue)
 	if err != nil {
@@ -788,7 +788,15 @@ func getIssueByID(e Engine, id int64) (*Issue, error) {
 	} else if !has {
 		return nil, ErrIssueNotExist{id, 0, 0}
 	}
-	return issue, issue.LoadAttributes()
+	return issue, nil
+}
+
+func getIssueByID(e Engine, id int64) (*Issue, error) {
+	issue, err := getRawIssueByID(e, id)
+	if err != nil {
+		return nil, err
+	}
+	return issue, issue.loadAttributes(e)
 }
 
 // GetIssueByID returns an issue by given ID.
@@ -1745,10 +1753,14 @@ func GetAttachmentsByIssueID(issueID int64) ([]*Attachment, error) {
 	return getAttachmentsByIssueID(x, issueID)
 }
 
+func getAttachmentsByCommentID(e Engine, commentID int64) ([]*Attachment, error) {
+	attachments := make([]*Attachment, 0, 10)
+	return attachments, e.Where("comment_id=?", commentID).Find(&attachments)
+}
+
 // GetAttachmentsByCommentID returns all attachments if comment by given ID.
 func GetAttachmentsByCommentID(commentID int64) ([]*Attachment, error) {
-	attachments := make([]*Attachment, 0, 10)
-	return attachments, x.Where("comment_id=?", commentID).Find(&attachments)
+	return getAttachmentsByCommentID(x, commentID)
 }
 
 // DeleteAttachment deletes the given attachment and optionally the associated file.

models/issue_comment.go

@@ -51,8 +51,9 @@ type Comment struct {
 	ID              int64 `xorm:"pk autoincr"`
 	Type            CommentType
 	PosterID        int64
-	Poster          *User `xorm:"-"`
-	IssueID         int64 `xorm:"INDEX"`
+	Poster          *User  `xorm:"-"`
+	IssueID         int64  `xorm:"INDEX"`
+	Issue           *Issue `xorm:"-"`
 	CommitID        int64
 	Line            int64
 	Content         string `xorm:"TEXT"`
@@ -82,84 +83,70 @@ func (c *Comment) BeforeUpdate() {
 }
 
 func (c *Comment) AfterSet(colName string, _ xorm.Cell) {
-	var err error
 	switch colName {
-	case "id":
-		c.Attachments, err = GetAttachmentsByCommentID(c.ID)
-		if err != nil {
-			log.Error(3, "GetAttachmentsByCommentID[%d]: %v", c.ID, err)
-		}
+	case "created_unix":
+		c.Created = time.Unix(c.CreatedUnix, 0).Local()
+	case "updated_unix":
+		c.Updated = time.Unix(c.UpdatedUnix, 0).Local()
+	}
+}
 
-	case "poster_id":
+func (c *Comment) loadAttributes(e Engine) (err error) {
+	if c.Poster == nil {
 		c.Poster, err = GetUserByID(c.PosterID)
 		if err != nil {
 			if IsErrUserNotExist(err) {
 				c.PosterID = -1
 				c.Poster = NewGhostUser()
 			} else {
-				log.Error(3, "GetUserByID[%d]: %v", c.ID, err)
+				return fmt.Errorf("getUserByID.(Poster) [%d]: %v", c.PosterID, err)
 			}
 		}
-	case "created_unix":
-		c.Created = time.Unix(c.CreatedUnix, 0).Local()
-	case "updated_unix":
-		c.Updated = time.Unix(c.UpdatedUnix, 0).Local()
 	}
-}
 
-func (c *Comment) AfterDelete() {
-	_, err := DeleteAttachmentsByComment(c.ID, true)
+	if c.Issue == nil {
+		c.Issue, err = getRawIssueByID(e, c.IssueID)
+		if err != nil {
+			return fmt.Errorf("getIssueByID [%d]: %v", c.IssueID, err)
+		}
+	}
 
-	if err != nil {
-		log.Info("Could not delete files for comment %d on issue #%d: %s", c.ID, c.IssueID, err)
+	if c.Attachments == nil {
+		c.Attachments, err = getAttachmentsByCommentID(e, c.ID)
+		if err != nil {
+			return fmt.Errorf("getAttachmentsByCommentID [%d]: %v", c.ID, err)
+		}
 	}
+
+	return nil
 }
 
-func (c *Comment) HTMLURL() string {
-	issue, err := GetIssueByID(c.IssueID)
-	if err != nil { // Silently dropping errors :unamused:
-		log.Error(4, "GetIssueByID(%d): %v", c.IssueID, err)
-		return ""
-	}
-	return fmt.Sprintf("%s#issuecomment-%d", issue.HTMLURL(), c.ID)
+func (c *Comment) LoadAttributes() error {
+	return c.loadAttributes(x)
 }
 
-func (c *Comment) IssueURL() string {
-	issue, err := GetIssueByID(c.IssueID)
-	if err != nil { // Silently dropping errors :unamused:
-		log.Error(4, "GetIssueByID(%d): %v", c.IssueID, err)
-		return ""
-	}
+func (c *Comment) AfterDelete() {
+	_, err := DeleteAttachmentsByComment(c.ID, true)
 
-	if issue.IsPull {
-		return ""
+	if err != nil {
+		log.Info("Could not delete files for comment %d on issue #%d: %s", c.ID, c.IssueID, err)
 	}
-	return issue.HTMLURL()
 }
 
-func (c *Comment) PRURL() string {
-	issue, err := GetIssueByID(c.IssueID)
-	if err != nil { // Silently dropping errors :unamused:
-		log.Error(4, "GetIssueByID(%d): %v", c.IssueID, err)
-		return ""
-	}
-
-	if !issue.IsPull {
-		return ""
-	}
-	return issue.HTMLURL()
+func (c *Comment) HTMLURL() string {
+	return fmt.Sprintf("%s#issuecomment-%d", c.Issue.HTMLURL(), c.ID)
 }
 
+// This method assumes following fields have been assigned with valid values:
+// Required - Poster, Issue
 func (c *Comment) APIFormat() *api.Comment {
 	return &api.Comment{
-		ID:       c.ID,
-		Poster:   c.Poster.APIFormat(),
-		HTMLURL:  c.HTMLURL(),
-		IssueURL: c.IssueURL(),
-		PRURL:    c.PRURL(),
-		Body:     c.Content,
-		Created:  c.Created,
-		Updated:  c.Updated,
+		ID:      c.ID,
+		HTMLURL: c.HTMLURL(),
+		Poster:  c.Poster.APIFormat(),
+		Body:    c.Content,
+		Created: c.Created,
+		Updated: c.Updated,
 	}
 }
 
@@ -294,7 +281,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err
 		comment.MailParticipants(act.OpType, opts.Issue)
 	}
 
-	return comment, nil
+	return comment, comment.loadAttributes(e)
 }
 
 func createStatusComment(e *xorm.Session, doer *User, repo *Repository, issue *Issue) (*Comment, error) {
@@ -389,7 +376,18 @@ func GetCommentByID(id int64) (*Comment, error) {
 	} else if !has {
 		return nil, ErrCommentNotExist{id, 0}
 	}
-	return c, nil
+	return c, c.LoadAttributes()
+}
+
+// FIXME: use CommentList to improve performance.
+func loadCommentsAttributes(e Engine, comments []*Comment) (err error) {
+	for i := range comments {
+		if err = comments[i].loadAttributes(e); err != nil {
+			return fmt.Errorf("loadAttributes [%d]: %v", comments[i].ID, err)
+		}
+	}
+
+	return nil
 }
 
 func getCommentsByIssueIDSince(e Engine, issueID, since int64) ([]*Comment, error) {
@@ -398,7 +396,11 @@ func getCommentsByIssueIDSince(e Engine, issueID, since int64) ([]*Comment, erro
 	if since > 0 {
 		sess.And("updated_unix >= ?", since)
 	}
-	return comments, sess.Find(&comments)
+
+	if err := sess.Find(&comments); err != nil {
+		return nil, err
+	}
+	return comments, loadCommentsAttributes(e, comments)
 }
 
 func getCommentsByRepoIDSince(e Engine, repoID, since int64) ([]*Comment, error) {
@@ -407,7 +409,10 @@ func getCommentsByRepoIDSince(e Engine, repoID, since int64) ([]*Comment, error)
 	if since > 0 {
 		sess.And("updated_unix >= ?", since)
 	}
-	return comments, sess.Find(&comments)
+	if err := sess.Find(&comments); err != nil {
+		return nil, err
+	}
+	return comments, loadCommentsAttributes(e, comments)
 }
 
 func getCommentsByIssueID(e Engine, issueID int64) ([]*Comment, error) {

routers/api/v1/repo/issue_comment.go

@@ -84,7 +84,7 @@ func EditIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
 		return
 	}
 
-	if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin()) {
+	if ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin() {
 		ctx.Status(403)
 		return
 	} else if comment.Type != models.COMMENT_TYPE_COMMENT {
@@ -111,7 +111,7 @@ func DeleteIssueComment(ctx *context.APIContext) {
 		return
 	}
 
-	if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin()) {
+	if ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin() {
 		ctx.Status(403)
 		return
 	} else if comment.Type != models.COMMENT_TYPE_COMMENT {