From 7e8cdba18120c4588d1921f07593a9a66eaa1411 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 29 Feb 2020 03:49:50 +0100 Subject: [PATCH] [Refactor] move APIFormat() of Issue and Label to convert package (#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 <18600385+guillep2k@users.noreply.github.com> Co-authored-by: zeripath Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> --- models/issue.go | 75 ++--------------- models/issue_assignees.go | 5 ++ models/issue_label.go | 12 --- models/issue_label_test.go | 12 --- modules/convert/issue.go | 107 +++++++++++++++++++++++- modules/convert/issue_test.go | 24 ++++++ modules/convert/pull.go | 2 +- modules/notification/webhook/webhook.go | 22 ++--- routers/api/v1/repo/issue.go | 21 ++--- routers/api/v1/repo/issue_label.go | 19 +---- routers/api/v1/repo/label.go | 13 ++- 11 files changed, 167 insertions(+), 145 deletions(-) create mode 100644 modules/convert/issue_test.go diff --git a/models/issue.go b/models/issue.go index d356682f01..340a431ad1 100644 --- 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 index f03980193f..577ed860f3 100644 --- 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 index 9e492dbec1..c111afb2ff 100644 --- 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 index d831792861..6f51473fcb 100644 --- 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 index d88a742dbb..d0985b6be1 100644 --- 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 new file mode 100644 index 0000000000..a7286d0766 --- /dev/null +++ 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 index fa22977d02..cf2bf88aa8 100644 --- 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 { diff --git a/modules/notification/webhook/webhook.go b/modules/notification/webhook/webhook.go index c1795d537b..f598d5b1f7 100644 --- a/modules/notification/webhook/webhook.go +++ b/modules/notification/webhook/webhook.go @@ -59,7 +59,7 @@ func (m *webhookNotifier) NotifyIssueClearLabels(doer *models.User, issue *model err = webhook_module.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{ Action: api.HookIssueLabelCleared, Index: issue.Index, - Issue: issue.APIFormat(), + Issue: convert.ToAPIIssue(issue), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), }) @@ -155,7 +155,7 @@ func (m *webhookNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *mo mode, _ := models.AccessLevelUnit(doer, issue.Repo, models.UnitTypeIssues) apiIssue := &api.IssuePayload{ Index: issue.Index, - Issue: issue.APIFormat(), + Issue: convert.ToAPIIssue(issue), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), } @@ -202,7 +202,7 @@ func (m *webhookNotifier) NotifyIssueChangeTitle(doer *models.User, issue *model From: oldTitle, }, }, - Issue: issue.APIFormat(), + Issue: convert.ToAPIIssue(issue), Repository: issue.Repo.APIFormat(mode), Sender: issue.Poster.APIFormat(), }) @@ -237,7 +237,7 @@ func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *mode } else { apiIssue := &api.IssuePayload{ Index: issue.Index, - Issue: issue.APIFormat(), + Issue: convert.ToAPIIssue(issue), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), } @@ -267,7 +267,7 @@ func (m *webhookNotifier) NotifyNewIssue(issue *models.Issue) { if err := webhook_module.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{ Action: api.HookIssueOpened, Index: issue.Index, - Issue: issue.APIFormat(), + Issue: convert.ToAPIIssue(issue), Repository: issue.Repo.APIFormat(mode), Sender: issue.Poster.APIFormat(), }); err != nil { @@ -327,7 +327,7 @@ func (m *webhookNotifier) NotifyIssueChangeContent(doer *models.User, issue *mod From: oldContent, }, }, - Issue: issue.APIFormat(), + Issue: convert.ToAPIIssue(issue), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), }) @@ -355,7 +355,7 @@ func (m *webhookNotifier) NotifyUpdateComment(doer *models.User, c *models.Comme mode, _ := models.AccessLevel(doer, c.Issue.Repo) if err := webhook_module.PrepareWebhooks(c.Issue.Repo, models.HookEventIssueComment, &api.IssueCommentPayload{ Action: api.HookIssueCommentEdited, - Issue: c.Issue.APIFormat(), + Issue: convert.ToAPIIssue(c.Issue), Comment: c.APIFormat(), Changes: &api.ChangesPayload{ Body: &api.ChangesFromPayload{ @@ -375,7 +375,7 @@ func (m *webhookNotifier) NotifyCreateIssueComment(doer *models.User, repo *mode mode, _ := models.AccessLevel(doer, repo) if err := webhook_module.PrepareWebhooks(repo, models.HookEventIssueComment, &api.IssueCommentPayload{ Action: api.HookIssueCommentCreated, - Issue: issue.APIFormat(), + Issue: convert.ToAPIIssue(issue), Comment: comment.APIFormat(), Repository: repo.APIFormat(mode), Sender: doer.APIFormat(), @@ -404,7 +404,7 @@ func (m *webhookNotifier) NotifyDeleteComment(doer *models.User, comment *models if err := webhook_module.PrepareWebhooks(comment.Issue.Repo, models.HookEventIssueComment, &api.IssueCommentPayload{ Action: api.HookIssueCommentDeleted, - Issue: comment.Issue.APIFormat(), + Issue: convert.ToAPIIssue(comment.Issue), Comment: comment.APIFormat(), Repository: comment.Issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), @@ -449,7 +449,7 @@ func (m *webhookNotifier) NotifyIssueChangeLabels(doer *models.User, issue *mode err = webhook_module.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{ Action: api.HookIssueLabelUpdated, Index: issue.Index, - Issue: issue.APIFormat(), + Issue: convert.ToAPIIssue(issue), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), }) @@ -491,7 +491,7 @@ func (m *webhookNotifier) NotifyIssueChangeMilestone(doer *models.User, issue *m err = webhook_module.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{ Action: hookAction, Index: issue.Index, - Issue: issue.APIFormat(), + Issue: convert.ToAPIIssue(issue), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), }) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index c400ec6f3c..2f08ba6ea6 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -13,6 +13,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/convert" issue_indexer "code.gitea.io/gitea/modules/indexer/issues" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -168,13 +169,8 @@ func SearchIssues(ctx *context.APIContext) { return } - apiIssues := make([]*api.Issue, len(issues)) - for i := range issues { - apiIssues[i] = issues[i].APIFormat() - } - ctx.SetLinkHeader(issueCount, setting.UI.IssuePagingNum) - ctx.JSON(http.StatusOK, &apiIssues) + ctx.JSON(http.StatusOK, convert.ToAPIIssueList(issues)) } // ListIssues list the issues of a repository @@ -287,13 +283,8 @@ func ListIssues(ctx *context.APIContext) { return } - apiIssues := make([]*api.Issue, len(issues)) - for i := range issues { - apiIssues[i] = issues[i].APIFormat() - } - ctx.SetLinkHeader(ctx.Repo.Repository.NumIssues, listOptions.PageSize) - ctx.JSON(http.StatusOK, &apiIssues) + ctx.JSON(http.StatusOK, convert.ToAPIIssueList(issues)) } // GetIssue get an issue of a repository @@ -335,7 +326,7 @@ func GetIssue(ctx *context.APIContext) { } return } - ctx.JSON(http.StatusOK, issue.APIFormat()) + ctx.JSON(http.StatusOK, convert.ToAPIIssue(issue)) } // CreateIssue create an issue of a repository @@ -450,7 +441,7 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) { ctx.Error(http.StatusInternalServerError, "GetIssueByID", err) return } - ctx.JSON(http.StatusCreated, issue.APIFormat()) + ctx.JSON(http.StatusCreated, convert.ToAPIIssue(issue)) } // EditIssue modify an issue of a repository @@ -596,7 +587,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { ctx.InternalServerError(err) return } - ctx.JSON(http.StatusCreated, issue.APIFormat()) + ctx.JSON(http.StatusCreated, convert.ToAPIIssue(issue)) } // UpdateIssueDeadline updates an issue deadline diff --git a/routers/api/v1/repo/issue_label.go b/routers/api/v1/repo/issue_label.go index a9fb42a186..8089891265 100644 --- a/routers/api/v1/repo/issue_label.go +++ b/routers/api/v1/repo/issue_label.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/convert" api "code.gitea.io/gitea/modules/structs" issue_service "code.gitea.io/gitea/services/issue" ) @@ -59,11 +60,7 @@ func ListIssueLabels(ctx *context.APIContext) { return } - apiLabels := make([]*api.Label, len(issue.Labels)) - for i := range issue.Labels { - apiLabels[i] = issue.Labels[i].APIFormat() - } - ctx.JSON(http.StatusOK, &apiLabels) + ctx.JSON(http.StatusOK, convert.ToLabelList(issue.Labels)) } // AddIssueLabels add labels for an issue @@ -118,11 +115,7 @@ func AddIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) { return } - apiLabels := make([]*api.Label, len(labels)) - for i := range labels { - apiLabels[i] = labels[i].APIFormat() - } - ctx.JSON(http.StatusOK, &apiLabels) + ctx.JSON(http.StatusOK, convert.ToLabelList(labels)) } // DeleteIssueLabel delete a label for an issue @@ -248,11 +241,7 @@ func ReplaceIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) { return } - apiLabels := make([]*api.Label, len(labels)) - for i := range labels { - apiLabels[i] = labels[i].APIFormat() - } - ctx.JSON(http.StatusOK, &apiLabels) + ctx.JSON(http.StatusOK, convert.ToLabelList(labels)) } // ClearIssueLabels delete all the labels for an issue diff --git a/routers/api/v1/repo/label.go b/routers/api/v1/repo/label.go index 422046001d..95dbbc9551 100644 --- a/routers/api/v1/repo/label.go +++ b/routers/api/v1/repo/label.go @@ -13,6 +13,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/convert" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/routers/api/v1/utils" ) @@ -53,11 +54,7 @@ func ListLabels(ctx *context.APIContext) { return } - apiLabels := make([]*api.Label, len(labels)) - for i := range labels { - apiLabels[i] = labels[i].APIFormat() - } - ctx.JSON(http.StatusOK, &apiLabels) + ctx.JSON(http.StatusOK, convert.ToLabelList(labels)) } // GetLabel get label by repository and label id @@ -107,7 +104,7 @@ func GetLabel(ctx *context.APIContext) { return } - ctx.JSON(http.StatusOK, label.APIFormat()) + ctx.JSON(http.StatusOK, convert.ToLabel(label)) } // CreateLabel create a label for a repository @@ -159,7 +156,7 @@ func CreateLabel(ctx *context.APIContext, form api.CreateLabelOption) { ctx.Error(http.StatusInternalServerError, "NewLabel", err) return } - ctx.JSON(http.StatusCreated, label.APIFormat()) + ctx.JSON(http.StatusCreated, convert.ToLabel(label)) } // EditLabel modify a label for a repository @@ -228,7 +225,7 @@ func EditLabel(ctx *context.APIContext, form api.EditLabelOption) { ctx.ServerError("UpdateLabel", err) return } - ctx.JSON(http.StatusOK, label.APIFormat()) + ctx.JSON(http.StatusOK, convert.ToLabel(label)) } // DeleteLabel delete a label for a repository