[Refactor] move APIFormat() of Issue and Label to convert package (go…

…-gitea#10423) * Label: delete .APIFormat() and use convert.ToLabel() * move issue APIFormat too * add missing one * move TEST too * handle error -> return empty APIIssue * Apply suggestions from code review Co-Authored-By: guillep2k <[email protected]> Co-authored-by: zeripath <[email protected]> Co-authored-by: guillep2k <[email protected]>
S7evinK · Feb 29, 2020 · 7e8cdba · 7e8cdba
1 parent c32f3da
commit 7e8cdba
Show file tree

Hide file tree

Showing 11 changed files with 167 additions and 145 deletions.
diff --git a/models/issue.go b/models/issue.go
@@ -138,6 +138,11 @@ func (issue *Issue) GetPullRequest() (pr *PullRequest, err error) {
 	return
 }
 
+// LoadLabels loads labels
+func (issue *Issue) LoadLabels() error {
+	return issue.loadLabels(x)
+}
+
 func (issue *Issue) loadLabels(e Engine) (err error) {
 	if issue.Labels == nil {
 		issue.Labels, err = getLabelsByIssueID(e, issue.ID)
@@ -364,76 +369,6 @@ func (issue *Issue) State() api.StateType {
 	return api.StateOpen
 }
 
-// APIFormat assumes some fields assigned with values:
-// Required - Poster, Labels,
-// Optional - Milestone, Assignee, PullRequest
-func (issue *Issue) APIFormat() *api.Issue {
-	return issue.apiFormat(x)
-}
-
-func (issue *Issue) apiFormat(e Engine) *api.Issue {
-	issue.loadLabels(e)
-	apiLabels := make([]*api.Label, len(issue.Labels))
-	for i := range issue.Labels {
-		apiLabels[i] = issue.Labels[i].APIFormat()
-	}
-
-	issue.loadPoster(e)
-	issue.loadRepo(e)
-	apiIssue := &api.Issue{
-		ID:       issue.ID,
-		URL:      issue.APIURL(),
-		HTMLURL:  issue.HTMLURL(),
-		Index:    issue.Index,
-		Poster:   issue.Poster.APIFormat(),
-		Title:    issue.Title,
-		Body:     issue.Content,
-		Labels:   apiLabels,
-		State:    issue.State(),
-		Comments: issue.NumComments,
-		Created:  issue.CreatedUnix.AsTime(),
-		Updated:  issue.UpdatedUnix.AsTime(),
-	}
-
-	apiIssue.Repo = &api.RepositoryMeta{
-		ID:       issue.Repo.ID,
-		Name:     issue.Repo.Name,
-		Owner:    issue.Repo.OwnerName,
-		FullName: issue.Repo.FullName(),
-	}
-
-	if issue.ClosedUnix != 0 {
-		apiIssue.Closed = issue.ClosedUnix.AsTimePtr()
-	}
-
-	issue.loadMilestone(e)
-	if issue.Milestone != nil {
-		apiIssue.Milestone = issue.Milestone.APIFormat()
-	}
-
-	issue.loadAssignees(e)
-	if len(issue.Assignees) > 0 {
-		for _, assignee := range issue.Assignees {
-			apiIssue.Assignees = append(apiIssue.Assignees, assignee.APIFormat())
-		}
-		apiIssue.Assignee = issue.Assignees[0].APIFormat() // For compatibility, we're keeping the first assignee as `apiIssue.Assignee`
-	}
-	if issue.IsPull {
-		issue.loadPullRequest(e)
-		apiIssue.PullRequest = &api.PullRequestMeta{
-			HasMerged: issue.PullRequest.HasMerged,
-		}
-		if issue.PullRequest.HasMerged {
-			apiIssue.PullRequest.Merged = issue.PullRequest.MergedUnix.AsTimePtr()
-		}
-	}
-	if issue.DeadlineUnix != 0 {
-		apiIssue.Deadline = issue.DeadlineUnix.AsTimePtr()
-	}
-
-	return apiIssue
-}
-
 // HashTag returns unique hash tag for issue.
 func (issue *Issue) HashTag() string {
 	return "issue-" + com.ToStr(issue.ID)

diff --git a/models/issue_assignees.go b/models/issue_assignees.go
@@ -19,6 +19,11 @@ type IssueAssignees struct {
 	IssueID    int64 `xorm:"INDEX"`
 }
 
+// LoadAssignees load assignees of this issue.
+func (issue *Issue) LoadAssignees() error {
+	return issue.loadAssignees(x)
+}
+
 // This loads all assignees of an issue
 func (issue *Issue) loadAssignees(e Engine) (err error) {
 	// Reset maybe preexisting assignees

diff --git a/models/issue_label.go b/models/issue_label.go
@@ -12,8 +12,6 @@ import (
 	"strconv"
 	"strings"
 
-	api "code.gitea.io/gitea/modules/structs"
-
 	"xorm.io/builder"
 	"xorm.io/xorm"
 )
@@ -37,16 +35,6 @@ type Label struct {
 	IsExcluded      bool   `xorm:"-"`
 }
 
-// APIFormat converts a Label to the api.Label format
-func (label *Label) APIFormat() *api.Label {
-	return &api.Label{
-		ID:          label.ID,
-		Name:        label.Name,
-		Color:       strings.TrimLeft(label.Color, "#"),
-		Description: label.Description,
-	}
-}
-
 // GetLabelTemplateFile loads the label template file by given name,
 // then parses and returns a list of name-color pairs and optionally description.
 func GetLabelTemplateFile(name string) ([][3]string, error) {

diff --git a/models/issue_label_test.go b/models/issue_label_test.go
@@ -8,23 +8,11 @@ import (
 	"html/template"
 	"testing"
 
-	api "code.gitea.io/gitea/modules/structs"
-
 	"github.com/stretchr/testify/assert"
 )
 
 // TODO TestGetLabelTemplateFile
 
-func TestLabel_APIFormat(t *testing.T) {
-	assert.NoError(t, PrepareTestDatabase())
-	label := AssertExistsAndLoadBean(t, &Label{ID: 1}).(*Label)
-	assert.Equal(t, api.Label{
-		ID:    label.ID,
-		Name:  label.Name,
-		Color: "abcdef",
-	}, *label.APIFormat())
-}
-
 func TestLabel_CalOpenIssues(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	label := AssertExistsAndLoadBean(t, &Label{ID: 1}).(*Label)

diff --git a/modules/convert/issue.go b/modules/convert/issue.go
@@ -5,10 +5,96 @@
 package convert
 
 import (
+	"strings"
+
 	"code.gitea.io/gitea/models"
 	api "code.gitea.io/gitea/modules/structs"
 )
 
+// ToAPIIssue converts an Issue to API format
+// it assumes some fields assigned with values:
+// Required - Poster, Labels,
+// Optional - Milestone, Assignee, PullRequest
+func ToAPIIssue(issue *models.Issue) *api.Issue {
+	if err := issue.LoadLabels(); err != nil {
+		return &api.Issue{}
+	}
+	if err := issue.LoadPoster(); err != nil {
+		return &api.Issue{}
+	}
+	if err := issue.LoadRepo(); err != nil {
+		return &api.Issue{}
+	}
+
+	apiIssue := &api.Issue{
+		ID:       issue.ID,
+		URL:      issue.APIURL(),
+		HTMLURL:  issue.HTMLURL(),
+		Index:    issue.Index,
+		Poster:   issue.Poster.APIFormat(),
+		Title:    issue.Title,
+		Body:     issue.Content,
+		Labels:   ToLabelList(issue.Labels),
+		State:    issue.State(),
+		Comments: issue.NumComments,
+		Created:  issue.CreatedUnix.AsTime(),
+		Updated:  issue.UpdatedUnix.AsTime(),
+	}
+
+	apiIssue.Repo = &api.RepositoryMeta{
+		ID:       issue.Repo.ID,
+		Name:     issue.Repo.Name,
+		Owner:    issue.Repo.OwnerName,
+		FullName: issue.Repo.FullName(),
+	}
+
+	if issue.ClosedUnix != 0 {
+		apiIssue.Closed = issue.ClosedUnix.AsTimePtr()
+	}
+
+	if err := issue.LoadMilestone(); err != nil {
+		return &api.Issue{}
+	}
+	if issue.Milestone != nil {
+		apiIssue.Milestone = issue.Milestone.APIFormat()
+	}
+
+	if err := issue.LoadAssignees(); err != nil {
+		return &api.Issue{}
+	}
+	if len(issue.Assignees) > 0 {
+		for _, assignee := range issue.Assignees {
+			apiIssue.Assignees = append(apiIssue.Assignees, assignee.APIFormat())
+		}
+		apiIssue.Assignee = issue.Assignees[0].APIFormat() // For compatibility, we're keeping the first assignee as `apiIssue.Assignee`
+	}
+	if issue.IsPull {
+		if err := issue.LoadPullRequest(); err != nil {
+			return &api.Issue{}
+		}
+		apiIssue.PullRequest = &api.PullRequestMeta{
+			HasMerged: issue.PullRequest.HasMerged,
+		}
+		if issue.PullRequest.HasMerged {
+			apiIssue.PullRequest.Merged = issue.PullRequest.MergedUnix.AsTimePtr()
+		}
+	}
+	if issue.DeadlineUnix != 0 {
+		apiIssue.Deadline = issue.DeadlineUnix.AsTimePtr()
+	}
+
+	return apiIssue
+}
+
+// ToAPIIssueList converts an IssueList to API format
+func ToAPIIssueList(il models.IssueList) []*api.Issue {
+	result := make([]*api.Issue, len(il))
+	for i := range il {
+		result[i] = ToAPIIssue(il[i])
+	}
+	return result
+}
+
 // ToTrackedTime converts TrackedTime to API format
 func ToTrackedTime(t *models.TrackedTime) (apiT *api.TrackedTime) {
 	apiT = &api.TrackedTime{
@@ -20,7 +106,7 @@ func ToTrackedTime(t *models.TrackedTime) (apiT *api.TrackedTime) {
 		Created:  t.Created,
 	}
 	if t.Issue != nil {
-		apiT.Issue = t.Issue.APIFormat()
+		apiT.Issue = ToAPIIssue(t.Issue)
 	}
 	if t.User != nil {
 		apiT.UserName = t.User.Name
@@ -36,3 +122,22 @@ func ToTrackedTimeList(tl models.TrackedTimeList) api.TrackedTimeList {
 	}
 	return result
 }
+
+// ToLabel converts Label to API format
+func ToLabel(label *models.Label) *api.Label {
+	return &api.Label{
+		ID:          label.ID,
+		Name:        label.Name,
+		Color:       strings.TrimLeft(label.Color, "#"),
+		Description: label.Description,
+	}
+}
+
+// ToLabelList converts list of Label to API format
+func ToLabelList(labels []*models.Label) []*api.Label {
+	result := make([]*api.Label, len(labels))
+	for i := range labels {
+		result[i] = ToLabel(labels[i])
+	}
+	return result
+}
diff --git a/modules/convert/issue_test.go b/modules/convert/issue_test.go
@@ -0,0 +1,24 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package convert
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models"
+	api "code.gitea.io/gitea/modules/structs"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestLabel_ToLabel(t *testing.T) {
+	assert.NoError(t, models.PrepareTestDatabase())
+	label := models.AssertExistsAndLoadBean(t, &models.Label{ID: 1}).(*models.Label)
+	assert.Equal(t, &api.Label{
+		ID:    label.ID,
+		Name:  label.Name,
+		Color: "abcdef",
+	}, ToLabel(label))
+}
diff --git a/modules/convert/pull.go b/modules/convert/pull.go
@@ -31,7 +31,7 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
 		return nil
 	}
 
-	apiIssue := pr.Issue.APIFormat()
+	apiIssue := ToAPIIssue(pr.Issue)
 	if pr.BaseRepo == nil {
 		pr.BaseRepo, err = models.GetRepositoryByID(pr.BaseRepoID)
 		if err != nil {