Fix and test for delete user (#1713)

* Fix and test for delete user

* Run updates in batches

* Unit test

Author: ethantkoenig

integrations/delete_user_test.go

@@ -0,0 +1,41 @@
+// 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 integrations
+
+import (
+	"bytes"
+	"net/http"
+	"net/url"
+	"testing"
+
+	"code.gitea.io/gitea/models"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDeleteUser(t *testing.T) {
+	prepareTestEnv(t)
+
+	session := loginUser(t, "user1", "password")
+
+	req, err := http.NewRequest("GET", "/admin/users/8", nil)
+	assert.NoError(t, err)
+	resp := session.MakeRequest(t, req)
+	assert.EqualValues(t, http.StatusOK, resp.HeaderCode)
+
+	doc, err := NewHtmlParser(resp.Body)
+	assert.NoError(t, err)
+	req, err = http.NewRequest("POST", "/admin/users/8/delete",
+		bytes.NewBufferString(url.Values{
+			"_csrf": []string{doc.GetInputValueByName("_csrf")},
+		}.Encode()))
+	assert.NoError(t, err)
+	req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
+	resp = session.MakeRequest(t, req)
+	assert.EqualValues(t, http.StatusOK, resp.HeaderCode)
+
+	models.AssertNotExistsBean(t, &models.User{ID: 8})
+	models.CheckConsistencyFor(t, &models.User{})
+}

models/consistency_test.go renamed to models/consistency.go

@@ -12,9 +12,9 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-// ConsistencyCheckable a type that can be tested for database consistency
-type ConsistencyCheckable interface {
-	CheckForConsistency(t *testing.T)
+// consistencyCheckable a type that can be tested for database consistency
+type consistencyCheckable interface {
+	checkForConsistency(t *testing.T)
 }
 
 // CheckConsistencyForAll test that the entire database is consistent
@@ -44,12 +44,12 @@ func CheckConsistencyFor(t *testing.T, beansToCheck ...interface{}) {
 
 		for i := 0; i < sliceValue.Len(); i++ {
 			entity := sliceValue.Index(i).Interface()
-			checkable, ok := entity.(ConsistencyCheckable)
+			checkable, ok := entity.(consistencyCheckable)
 			if !ok {
 				t.Errorf("Expected %+v (of type %T) to be checkable for consistency",
 					entity, entity)
 			} else {
-				checkable.CheckForConsistency(t)
+				checkable.checkForConsistency(t)
 			}
 		}
 	}
@@ -68,7 +68,7 @@ func assertCount(t *testing.T, bean interface{}, expected int) {
 		"Failed consistency test, the counted bean (of type %T) was %+v", bean, bean)
 }
 
-func (user *User) CheckForConsistency(t *testing.T) {
+func (user *User) checkForConsistency(t *testing.T) {
 	assertCount(t, &Repository{OwnerID: user.ID}, user.NumRepos)
 	assertCount(t, &Star{UID: user.ID}, user.NumStars)
 	assertCount(t, &OrgUser{OrgID: user.ID}, user.NumMembers)
@@ -81,7 +81,7 @@ func (user *User) CheckForConsistency(t *testing.T) {
 	}
 }
 
-func (repo *Repository) CheckForConsistency(t *testing.T) {
+func (repo *Repository) checkForConsistency(t *testing.T) {
 	assert.Equal(t, repo.LowerName, strings.ToLower(repo.Name), "repo: %+v", repo)
 	assertCount(t, &Star{RepoID: repo.ID}, repo.NumStars)
 	assertCount(t, &Watch{RepoID: repo.ID}, repo.NumWatches)
@@ -112,7 +112,7 @@ func (repo *Repository) CheckForConsistency(t *testing.T) {
 		"Unexpected number of closed milestones for repo %+v", repo)
 }
 
-func (issue *Issue) CheckForConsistency(t *testing.T) {
+func (issue *Issue) checkForConsistency(t *testing.T) {
 	actual := getCount(t, x.Where("type=?", CommentTypeComment), &Comment{IssueID: issue.ID})
 	assert.EqualValues(t, issue.NumComments, actual,
 		"Unexpected number of comments for issue %+v", issue)
@@ -122,21 +122,21 @@ func (issue *Issue) CheckForConsistency(t *testing.T) {
 	}
 }
 
-func (pr *PullRequest) CheckForConsistency(t *testing.T) {
+func (pr *PullRequest) checkForConsistency(t *testing.T) {
 	issue := AssertExistsAndLoadBean(t, &Issue{ID: pr.IssueID}).(*Issue)
 	assert.True(t, issue.IsPull)
 	assert.EqualValues(t, issue.Index, pr.Index)
 }
 
-func (milestone *Milestone) CheckForConsistency(t *testing.T) {
+func (milestone *Milestone) checkForConsistency(t *testing.T) {
 	assertCount(t, &Issue{MilestoneID: milestone.ID}, milestone.NumIssues)
 
 	actual := getCount(t, x.Where("is_closed=?", true), &Issue{MilestoneID: milestone.ID})
 	assert.EqualValues(t, milestone.NumClosedIssues, actual,
 		"Unexpected number of closed issues for milestone %+v", milestone)
 }
 
-func (label *Label) CheckForConsistency(t *testing.T) {
+func (label *Label) checkForConsistency(t *testing.T) {
 	issueLabels := make([]*IssueLabel, 0, 10)
 	assert.NoError(t, x.Find(&issueLabels, &IssueLabel{LabelID: label.ID}))
 	assert.EqualValues(t, label.NumIssues, len(issueLabels),
@@ -155,12 +155,12 @@ func (label *Label) CheckForConsistency(t *testing.T) {
 		"Unexpected number of closed issues for label %+v", label)
 }
 
-func (team *Team) CheckForConsistency(t *testing.T) {
+func (team *Team) checkForConsistency(t *testing.T) {
 	assertCount(t, &TeamUser{TeamID: team.ID}, team.NumMembers)
 	assertCount(t, &TeamRepo{TeamID: team.ID}, team.NumRepos)
 }
 
-func (action *Action) CheckForConsistency(t *testing.T) {
+func (action *Action) checkForConsistency(t *testing.T) {
 	repo := AssertExistsAndLoadBean(t, &Repository{ID: action.RepoID}).(*Repository)
 	owner := AssertExistsAndLoadBean(t, &User{ID: repo.OwnerID}).(*User)
 	actor := AssertExistsAndLoadBean(t, &User{ID: action.ActUserID}).(*User)

models/fixtures/follow.yml

@@ -2,3 +2,13 @@
   id: 1
   user_id: 4
   follow_id: 2
+
+-
+  id: 2
+  user_id: 8
+  follow_id: 2
+
+-
+  id: 3
+  user_id: 2
+  follow_id: 8

models/fixtures/user.yml

@@ -4,9 +4,9 @@
   name: user1
   full_name: User One
   email: [email protected]
-  passwd: password
+  passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password
   type: 0 # individual
-  salt: salt
+  salt: ZogKvWdyEx
   is_admin: true
   avatar: avatar1
   avatar_email: [email protected]
@@ -26,7 +26,8 @@
   avatar_email: [email protected]
   num_repos: 2
   num_stars: 2
-  num_followers: 1
+  num_followers: 2
+  num_following: 1
   is_active: true
 
 -
@@ -123,6 +124,8 @@
   avatar_email: [email protected]
   num_repos: 0
   is_active: true
+  num_followers: 1
+  num_following: 1
 
 -
   id: 9

models/main_test.go

@@ -0,0 +1,26 @@
+package models
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"code.gitea.io/gitea/modules/setting"
+)
+
+func TestMain(m *testing.M) {
+	if err := CreateTestEngine(); err != nil {
+		fmt.Printf("Error creating test engine: %v\n", err)
+		os.Exit(1)
+	}
+
+	setting.AppURL = "https://try.gitea.io/"
+	setting.RunUser = "runuser"
+	setting.SSH.Port = 3000
+	setting.SSH.Domain = "try.gitea.io"
+	setting.RepoRootPath = filepath.Join(os.TempDir(), "repos")
+	setting.AppDataPath = filepath.Join(os.TempDir(), "appdata")
+
+	os.Exit(m.Run())
+}

models/setup_for_test.go renamed to models/unit_tests.go

@@ -5,38 +5,18 @@
 package models
 
 import (
-	"fmt"
-	"os"
-	"path/filepath"
 	"testing"
 
-	"code.gitea.io/gitea/modules/setting"
-
 	"github.com/go-xorm/core"
 	"github.com/go-xorm/xorm"
 	_ "github.com/mattn/go-sqlite3" // for the test engine
 	"github.com/stretchr/testify/assert"
 	"gopkg.in/testfixtures.v2"
 )
 
+// NonexistentID an ID that will never exist
 const NonexistentID = 9223372036854775807
 
-func TestMain(m *testing.M) {
-	if err := CreateTestEngine(); err != nil {
-		fmt.Printf("Error creating test engine: %v\n", err)
-		os.Exit(1)
-	}
-
-	setting.AppURL = "https://try.gitea.io/"
-	setting.RunUser = "runuser"
-	setting.SSH.Port = 3000
-	setting.SSH.Domain = "try.gitea.io"
-	setting.RepoRootPath = filepath.Join(os.TempDir(), "repos")
-	setting.AppDataPath = filepath.Join(os.TempDir(), "appdata")
-
-	os.Exit(m.Run())
-}
-
 // CreateTestEngine create an xorm engine for testing
 func CreateTestEngine() error {
 	var err error
@@ -52,6 +32,7 @@ func CreateTestEngine() error {
 	return InitFixtures(&testfixtures.SQLite{}, "fixtures/")
 }
 
+// TestFixturesAreConsistent assert that test fixtures are consistent
 func TestFixturesAreConsistent(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	CheckConsistencyForAll(t)

models/user.go

@@ -924,38 +924,41 @@ func deleteUser(e *xorm.Session, u *User) error {
 	}
 
 	// ***** START: Watch *****
-	watches := make([]*Watch, 0, 10)
-	if err = e.Find(&watches, &Watch{UserID: u.ID}); err != nil {
+	watchedRepoIDs := make([]int64, 0, 10)
+	if err = e.Table("watch").Cols("watch.repo_id").
+		Where("watch.user_id = ?", u.ID).Find(&watchedRepoIDs); err != nil {
 		return fmt.Errorf("get all watches: %v", err)
 	}
-	for i := range watches {
-		if _, err = e.Exec("UPDATE `repository` SET num_watches=num_watches-1 WHERE id=?", watches[i].RepoID); err != nil {
-			return fmt.Errorf("decrease repository watch number[%d]: %v", watches[i].RepoID, err)
-		}
+	if _, err = e.Decr("num_watches").In("id", watchedRepoIDs).Update(new(Repository)); err != nil {
+		return fmt.Errorf("decrease repository num_watches: %v", err)
 	}
 	// ***** END: Watch *****
 
 	// ***** START: Star *****
-	stars := make([]*Star, 0, 10)
-	if err = e.Find(&stars, &Star{UID: u.ID}); err != nil {
+	starredRepoIDs := make([]int64, 0, 10)
+	if err = e.Table("star").Cols("star.repo_id").
+		Where("star.uid = ?", u.ID).Find(&starredRepoIDs); err != nil {
 		return fmt.Errorf("get all stars: %v", err)
-	}
-	for i := range stars {
-		if _, err = e.Exec("UPDATE `repository` SET num_stars=num_stars-1 WHERE id=?", stars[i].RepoID); err != nil {
-			return fmt.Errorf("decrease repository star number[%d]: %v", stars[i].RepoID, err)
-		}
+	} else if _, err = e.Decr("num_watches").In("id", starredRepoIDs).Update(new(Repository)); err != nil {
+		return fmt.Errorf("decrease repository num_stars: %v", err)
 	}
 	// ***** END: Star *****
 
 	// ***** START: Follow *****
-	followers := make([]*Follow, 0, 10)
-	if err = e.Find(&followers, &Follow{UserID: u.ID}); err != nil {
-		return fmt.Errorf("get all followers: %v", err)
+	followeeIDs := make([]int64, 0, 10)
+	if err = e.Table("follow").Cols("follow.follow_id").
+		Where("follow.user_id = ?", u.ID).Find(&followeeIDs); err != nil {
+		return fmt.Errorf("get all followees: %v", err)
+	} else if _, err = e.Decr("num_followers").In("id", followeeIDs).Update(new(User)); err != nil {
+		return fmt.Errorf("decrease user num_followers: %v", err)
 	}
-	for i := range followers {
-		if _, err = e.Exec("UPDATE `user` SET num_followers=num_followers-1 WHERE id=?", followers[i].UserID); err != nil {
-			return fmt.Errorf("decrease user follower number[%d]: %v", followers[i].UserID, err)
-		}
+
+	followerIDs := make([]int64, 0, 10)
+	if err = e.Table("follow").Cols("follow.user_id").
+		Where("follow.follow_id = ?", u.ID).Find(&followerIDs); err != nil {
+		return fmt.Errorf("get all followers: %v", err)
+	} else if _, err = e.Decr("num_following").In("id", followerIDs).Update(new(User)); err != nil {
+		return fmt.Errorf("decrease user num_following: %v", err)
 	}
 	// ***** END: Follow *****
 
@@ -965,6 +968,7 @@ func deleteUser(e *xorm.Session, u *User) error {
 		&Access{UserID: u.ID},
 		&Watch{UserID: u.ID},
 		&Star{UID: u.ID},
+		&Follow{UserID: u.ID},
 		&Follow{FollowID: u.ID},
 		&Action{UserID: u.ID},
 		&IssueUser{UID: u.ID},

models/user_test.go

@@ -36,5 +36,36 @@ func TestCanCreateOrganization(t *testing.T) {
 	user.AllowCreateOrganization = true
 	assert.True(t, admin.CanCreateOrganization())
 	assert.False(t, user.CanCreateOrganization())
+}
+
+func TestDeleteUser(t *testing.T) {
+	test := func(userID int64) {
+		assert.NoError(t, PrepareTestDatabase())
+		user := AssertExistsAndLoadBean(t, &User{ID: userID}).(*User)
+
+		ownedRepos := make([]*Repository, 0, 10)
+		assert.NoError(t, x.Find(&ownedRepos, &Repository{OwnerID: userID}))
+		if len(ownedRepos) > 0 {
+			err := DeleteUser(user)
+			assert.Error(t, err)
+			assert.True(t, IsErrUserOwnRepos(err))
+			return
+		}
 
+		orgUsers := make([]*OrgUser, 0, 10)
+		assert.NoError(t, x.Find(&orgUsers, &OrgUser{UID: userID}))
+		for _, orgUser := range orgUsers {
+			if err := RemoveOrgUser(orgUser.OrgID, orgUser.UID); err != nil {
+				assert.True(t, IsErrLastOrgOwner(err))
+				return
+			}
+		}
+		assert.NoError(t, DeleteUser(user))
+		AssertNotExistsBean(t, &User{ID: userID})
+		CheckConsistencyFor(t, &User{}, &Repository{})
+	}
+	test(2)
+	test(4)
+	test(8)
+	test(11)
 }