#3290 better code structure and batch minor improvements

3 files changed, 52 insertions, 123 deletions

diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go
index 80131af6cd..e4398458f5 100644
--- a/routers/api/v1/api.go
+++ b/routers/api/v1/api.go
@@ -244,7 +244,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 					m.Group("/:index", func() {
 						m.Combo("").Get(repo.GetIssue).Patch(bind(api.EditIssueOption{}), repo.EditIssue)
 						m.Group("/labels", func() {
-							m.Combo("").Get(repo.GetIssueLabels).
+							m.Combo("").Get(repo.ListIssueLabels).
 								Post(bind(api.IssueLabelsOption{}), repo.AddIssueLabels).
 								Put(bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels).
 								Delete(repo.ClearIssueLabels)
@@ -255,8 +255,8 @@ func RegisterRoutes(m *macaron.Macaron) {
 				})
 				m.Group("/labels", func() {
 					m.Combo("").Get(repo.ListLabels).
-						Post(bind(api.LabelOption{}), repo.CreateLabel)
-					m.Combo("/:id").Get(repo.GetLabel).Patch(bind(api.LabelOption{}), repo.EditLabel).
+						Post(bind(api.CreateLabelOption{}), repo.CreateLabel)
+					m.Combo("/:id").Get(repo.GetLabel).Patch(bind(api.EditLabelOption{}), repo.EditLabel).
 						Delete(repo.DeleteLabel)
 				})
 			}, RepoAssignment())
diff --git a/routers/api/v1/repo/issue_label.go b/routers/api/v1/repo/issue_label.go
index ea697b27c4..e7cc63cde8 100644
--- a/routers/api/v1/repo/issue_label.go
+++ b/routers/api/v1/repo/issue_label.go
@@ -12,7 +12,7 @@ import (
 	"github.com/gogits/gogs/routers/api/v1/convert"
 )
 
-func GetIssueLabels(ctx *context.APIContext) {
+func ListIssueLabels(ctx *context.APIContext) {
 	issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
 	if err != nil {
 		if models.IsErrIssueNotExist(err) {
@@ -27,7 +27,6 @@ func GetIssueLabels(ctx *context.APIContext) {
 	for i := range issue.Labels {
 		apiLabels[i] = convert.ToLabel(issue.Labels[i])
 	}
-
 	ctx.JSON(200, &apiLabels)
 }
 
@@ -47,41 +46,31 @@ func AddIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) {
 		return
 	}
 
-	var labels []*models.Label
-	if labels, err = filterLabelsByRepoID(form.Labels, issue.RepoID); err != nil {
-		ctx.Error(400, "filterLabelsByRepoID", err)
+	labels, err := models.GetLabelsInRepoByIDs(ctx.Repo.Repository.ID, form.Labels)
+	if err != nil {
+		ctx.Error(500, "GetLabelsInRepoByIDs", err)
 		return
 	}
 
-	for i := range labels {
-		if !models.HasIssueLabel(issue.ID, labels[i].ID) {
-			if err := models.NewIssueLabel(issue, labels[i]); err != nil {
-				ctx.Error(500, "NewIssueLabel", err)
-				return
-			}
-		}
+	if err = issue.AddLabels(labels); err != nil {
+		ctx.Error(500, "AddLabels", err)
+		return
 	}
 
-	// Refresh issue to get the updated list of labels from the DB
-	issue, err = models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
+	labels, err = models.GetLabelsByIssueID(issue.ID)
 	if err != nil {
-		if models.IsErrIssueNotExist(err) {
-			ctx.Status(404)
-		} else {
-			ctx.Error(500, "GetIssueByIndex", err)
-		}
+		ctx.Error(500, "GetLabelsByIssueID", err)
 		return
 	}
 
-	apiLabels := make([]*api.Label, len(issue.Labels))
-	for i := range issue.Labels {
-		apiLabels[i] = convert.ToLabel(issue.Labels[i])
+	apiLabels := make([]*api.Label, len(labels))
+	for i := range labels {
+		apiLabels[i] = convert.ToLabel(labels[i])
 	}
-
 	ctx.JSON(200, &apiLabels)
 }
 
-func ReplaceIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) {
+func DeleteIssueLabel(ctx *context.APIContext) {
 	if !ctx.Repo.IsWriter() {
 		ctx.Status(403)
 		return
@@ -97,44 +86,25 @@ func ReplaceIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) {
 		return
 	}
 
-	var labels []*models.Label
-	if labels, err = filterLabelsByRepoID(form.Labels, issue.RepoID); err != nil {
-		ctx.Error(400, "filterLabelsByRepoID", err)
-		return
-	}
-
-	if err := issue.ClearLabels(); err != nil {
-		ctx.Error(500, "ClearLabels", err)
-		return
-	}
-
-	for i := range labels {
-		if err := models.NewIssueLabel(issue, labels[i]); err != nil {
-			ctx.Error(500, "NewIssueLabel", err)
-			return
-		}
-	}
-
-	// Refresh issue to get the updated list of labels from the DB
-	issue, err = models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
+	label, err := models.GetLabelInRepoByID(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id"))
 	if err != nil {
-		if models.IsErrIssueNotExist(err) {
-			ctx.Status(404)
+		if models.IsErrLabelNotExist(err) {
+			ctx.Error(422, "", err)
 		} else {
-			ctx.Error(500, "GetIssueByIndex", err)
+			ctx.Error(500, "GetLabelInRepoByID", err)
 		}
 		return
 	}
 
-	apiLabels := make([]*api.Label, len(issue.Labels))
-	for i := range issue.Labels {
-		apiLabels[i] = convert.ToLabel(issue.Labels[i])
+	if err := models.DeleteIssueLabel(issue, label); err != nil {
+		ctx.Error(500, "DeleteIssueLabel", err)
+		return
 	}
 
-	ctx.JSON(200, &apiLabels)
+	ctx.Status(204)
 }
 
-func DeleteIssueLabel(ctx *context.APIContext) {
+func ReplaceIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) {
 	if !ctx.Repo.IsWriter() {
 		ctx.Status(403)
 		return
@@ -150,22 +120,28 @@ func DeleteIssueLabel(ctx *context.APIContext) {
 		return
 	}
 
-	label, err := models.GetLabelByID(ctx.ParamsInt64(":id"))
+	labels, err := models.GetLabelsInRepoByIDs(ctx.Repo.Repository.ID, form.Labels)
 	if err != nil {
-		if models.IsErrLabelNotExist(err) {
-			ctx.Status(400)
-		} else {
-			ctx.Error(500, "GetLabelByID", err)
-		}
+		ctx.Error(500, "GetLabelsInRepoByIDs", err)
 		return
 	}
 
-	if err := models.DeleteIssueLabel(issue, label); err != nil {
-		ctx.Error(500, "DeleteIssueLabel", err)
+	if err := issue.ReplaceLabels(labels); err != nil {
+		ctx.Error(500, "ReplaceLabels", err)
 		return
 	}
 
-	ctx.Status(204)
+	labels, err = models.GetLabelsByIssueID(issue.ID)
+	if err != nil {
+		ctx.Error(500, "GetLabelsByIssueID", err)
+		return
+	}
+
+	apiLabels := make([]*api.Label, len(labels))
+	for i := range labels {
+		apiLabels[i] = convert.ToLabel(labels[i])
+	}
+	ctx.JSON(200, &apiLabels)
 }
 
 func ClearIssueLabels(ctx *context.APIContext) {
@@ -191,29 +167,3 @@ func ClearIssueLabels(ctx *context.APIContext) {
 
 	ctx.Status(204)
 }
-
-func filterLabelsByRepoID(labelIDs []int64, repoID int64) ([]*models.Label, error) {
-	labels := make([]*models.Label, 0, len(labelIDs))
-	errors := make([]error, 0, len(labelIDs))
-
-	for i := range labelIDs {
-		label, err := models.GetLabelByID(labelIDs[i])
-		if err != nil {
-			errors = append(errors, err)
-		} else if label.RepoID != repoID {
-			errors = append(errors, models.ErrLabelNotValidForRepository{label.ID, repoID})
-		} else {
-			labels = append(labels, label)
-		}
-	}
-
-	errorCount := len(errors)
-
-	if errorCount == 1 {
-		return labels, errors[0]
-	} else if errorCount > 1 {
-		return labels, models.ErrMultipleErrors{errors}
-	}
-
-	return labels, nil
-}
diff --git a/routers/api/v1/repo/label.go b/routers/api/v1/repo/label.go
index 92e199af84..dfd0b1ddda 100644
--- a/routers/api/v1/repo/label.go
+++ b/routers/api/v1/repo/label.go
@@ -9,14 +9,13 @@ import (
 
 	"github.com/gogits/gogs/models"
 	"github.com/gogits/gogs/modules/context"
-	"github.com/gogits/gogs/modules/log"
 	"github.com/gogits/gogs/routers/api/v1/convert"
 )
 
 func ListLabels(ctx *context.APIContext) {
 	labels, err := models.GetLabelsByRepoID(ctx.Repo.Repository.ID)
 	if err != nil {
-		ctx.Error(500, "Labels", err)
+		ctx.Error(500, "GetLabelsByRepoID", err)
 		return
 	}
 
@@ -24,17 +23,16 @@ func ListLabels(ctx *context.APIContext) {
 	for i := range labels {
 		apiLabels[i] = convert.ToLabel(labels[i])
 	}
-
 	ctx.JSON(200, &apiLabels)
 }
 
 func GetLabel(ctx *context.APIContext) {
-	label, err := models.GetLabelByID(ctx.ParamsInt64(":id"))
+	label, err := models.GetLabelInRepoByID(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id"))
 	if err != nil {
 		if models.IsErrLabelNotExist(err) {
 			ctx.Status(404)
 		} else {
-			ctx.Error(500, "GetLabelByID", err)
+			ctx.Error(500, "GetLabelByRepoID", err)
 		}
 		return
 	}
@@ -42,7 +40,7 @@ func GetLabel(ctx *context.APIContext) {
 	ctx.JSON(200, convert.ToLabel(label))
 }
 
-func CreateLabel(ctx *context.APIContext, form api.LabelOption) {
+func CreateLabel(ctx *context.APIContext, form api.CreateLabelOption) {
 	if !ctx.Repo.IsWriter() {
 		ctx.Status(403)
 		return
@@ -53,43 +51,35 @@ func CreateLabel(ctx *context.APIContext, form api.LabelOption) {
 		Color:  form.Color,
 		RepoID: ctx.Repo.Repository.ID,
 	}
-	err := models.NewLabel(label)
-	if err != nil {
+	if err := models.NewLabel(label); err != nil {
 		ctx.Error(500, "NewLabel", err)
 		return
 	}
-
-	label, err = models.GetLabelByID(label.ID)
-	if err != nil {
-		ctx.Error(500, "GetLabelByID", err)
-		return
-	}
 	ctx.JSON(201, convert.ToLabel(label))
 }
 
-func EditLabel(ctx *context.APIContext, form api.LabelOption) {
+func EditLabel(ctx *context.APIContext, form api.EditLabelOption) {
 	if !ctx.Repo.IsWriter() {
 		ctx.Status(403)
 		return
 	}
 
-	label, err := models.GetLabelByID(ctx.ParamsInt64(":id"))
+	label, err := models.GetLabelInRepoByID(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id"))
 	if err != nil {
 		if models.IsErrLabelNotExist(err) {
 			ctx.Status(404)
 		} else {
-			ctx.Error(500, "GetLabelByID", err)
+			ctx.Error(500, "GetLabelByRepoID", err)
 		}
 		return
 	}
 
-	if len(form.Name) > 0 {
-		label.Name = form.Name
+	if form.Name != nil {
+		label.Name = *form.Name
 	}
-	if len(form.Color) > 0 {
-		label.Color = form.Color
+	if form.Color != nil {
+		label.Color = *form.Color
 	}
-
 	if err := models.UpdateLabel(label); err != nil {
 		ctx.Handle(500, "UpdateLabel", err)
 		return
@@ -103,21 +93,10 @@ func DeleteLabel(ctx *context.APIContext) {
 		return
 	}
 
-	label, err := models.GetLabelByID(ctx.ParamsInt64(":id"))
-	if err != nil {
-		if models.IsErrLabelNotExist(err) {
-			ctx.Status(404)
-		} else {
-			ctx.Error(500, "GetLabelByID", err)
-		}
-		return
-	}
-
 	if err := models.DeleteLabel(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id")); err != nil {
 		ctx.Error(500, "DeleteLabel", err)
 		return
 	}
 
-	log.Trace("Label deleted: %s %s", label.ID, label.Name)
 	ctx.Status(204)
 }