Optimize and unit test Issue_ReplaceLabels (#1080)

models/issue.go

@@ -476,31 +476,24 @@ func (issue *Issue) ReplaceLabels(labels []*Label, doer *User) (err error) {
 	sort.Sort(labelSorter(issue.Labels))
 
 	var toAdd, toRemove []*Label
-	for _, l := range labels {
-		var exist bool
-		for _, oriLabel := range issue.Labels {
-			if oriLabel.ID == l.ID {
-				exist = true
-				break
-			}
-		}
-		if !exist {
-			toAdd = append(toAdd, l)
-		}
-	}
 
-	for _, oriLabel := range issue.Labels {
+	addIndex, removeIndex := 0, 0
-		var exist bool
+	for addIndex < len(labels) && removeIndex < len(issue.Labels) {
-		for _, l := range labels {
+		addLabel := labels[addIndex]
-			if oriLabel.ID == l.ID {
+		removeLabel := issue.Labels[removeIndex]
-				exist = true
+		if addLabel.ID == removeLabel.ID {
-				break
+			addIndex++
-			}
+			removeIndex++
-		}
+		} else if addLabel.ID < removeLabel.ID {
-		if !exist {
+			toAdd = append(toAdd, addLabel)
-			toRemove = append(toRemove, oriLabel)
+			addIndex++
+		} else {
+			toRemove = append(toRemove, removeLabel)
+			removeIndex++
 		}
 	}
+	toAdd = append(toAdd, labels[addIndex:]...)
+	toRemove = append(toRemove, issue.Labels[removeIndex:]...)
 
 	if len(toAdd) > 0 {
 		if err = issue.addLabels(sess, toAdd, doer); err != nil {
@@ -508,11 +501,9 @@ func (issue *Issue) ReplaceLabels(labels []*Label, doer *User) (err error) {
 		}
 	}
 
-	if len(toRemove) > 0 {
+	for _, l := range toRemove {
-		for _, l := range toRemove {
+		if err = issue.removeLabel(sess, doer, l); err != nil {
-			if err = issue.removeLabel(sess, doer, l); err != nil {
+			return fmt.Errorf("removeLabel: %v", err)
-				return fmt.Errorf("removeLabel: %v", err)
-			}
 		}
 	}
 

models/issue_test.go

@@ -0,0 +1,35 @@
+// Copyright 2017 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 models
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIssue_ReplaceLabels(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+
+	testSuccess := func(issueID int64, labelIDs []int64) {
+		issue := AssertExistsAndLoadBean(t, &Issue{ID: issueID}).(*Issue)
+		repo := AssertExistsAndLoadBean(t, &Repository{ID: issue.RepoID}).(*Repository)
+		doer := AssertExistsAndLoadBean(t, &User{ID: repo.OwnerID}).(*User)
+
+		labels := make([]*Label, len(labelIDs))
+		for i, labelID := range labelIDs {
+			labels[i] = AssertExistsAndLoadBean(t, &Label{ID: labelID, RepoID: repo.ID}).(*Label)
+		}
+		assert.NoError(t, issue.ReplaceLabels(labels, doer))
+		AssertCount(t, &IssueLabel{IssueID: issueID}, len(labelIDs))
+		for _, labelID := range labelIDs {
+			AssertExistsAndLoadBean(t, &IssueLabel{IssueID: issueID, LabelID: labelID})
+		}
+	}
+
+	testSuccess(1, []int64{2})
+	testSuccess(1, []int64{1, 2})
+	testSuccess(1, []int64{})
+}

models/setup_for_test.go

@@ -107,8 +107,9 @@ func AssertSuccessfulInsert(t *testing.T, beans ...interface{}) {
 	assert.NoError(t, err)
 }
 
-// AssertSuccessfulUpdate assert that bean is successfully updated
+// AssertCount assert the count of a bean
-func AssertSuccessfulUpdate(t *testing.T, bean interface{}, conditions ...interface{}) {
+func AssertCount(t *testing.T, bean interface{}, expected interface{}) {
-	_, err := x.Update(bean, conditions...)
+	actual, err := x.Count(bean)
 	assert.NoError(t, err)
+	assert.EqualValues(t, expected, actual)
 }