#1067: Deleting users should remove them from collaborator lists

- fix delete user but repository watches are not decreased

models/action.go

@@ -61,14 +61,14 @@ func init() {
 // Action represents user operation type and other information to repository.,
 // it implemented interface base.Actioner so that can be used in template render.
 type Action struct {
-	Id           int64
-	UserId       int64 // Receiver user id.
+	ID           int64 `xorm:"pk autoincr"`
+	UserID       int64 // Receiver user id.
 	OpType       ActionType
-	ActUserId    int64  // Action user id.
+	ActUserID    int64  // Action user id.
 	ActUserName  string // Action user name.
 	ActEmail     string
 	ActAvatar    string `xorm:"-"`
-	RepoId       int64
+	RepoID       int64
 	RepoUserName string
 	RepoName     string
 	RefName      string
@@ -319,10 +319,18 @@ func CommitRepoAction(userId, repoUserId int64, userName, actEmail string,
 		log.Debug("action.CommitRepoAction(updateIssuesCommit): ", err)
 	}
 
-	if err = NotifyWatchers(&Action{ActUserId: userId, ActUserName: userName, ActEmail: actEmail,
-		OpType: opType, Content: string(bs), RepoId: repoId, RepoUserName: repoUserName,
-		RepoName: repoName, RefName: refName,
-		IsPrivate: repo.IsPrivate}); err != nil {
+	if err = NotifyWatchers(&Action{
+		ActUserID:    userId,
+		ActUserName:  userName,
+		ActEmail:     actEmail,
+		OpType:       opType,
+		Content:      string(bs),
+		RepoID:       repoId,
+		RepoUserName: repoUserName,
+		RepoName:     repoName,
+		RefName:      refName,
+		IsPrivate:    repo.IsPrivate,
+	}); err != nil {
 		return errors.New("action.CommitRepoAction(NotifyWatchers): " + err.Error())
 
 	}
@@ -443,14 +451,15 @@ func CommitRepoAction(userId, repoUserId int64, userName, actEmail string,
 
 func newRepoAction(e Engine, u *User, repo *Repository) (err error) {
 	if err = notifyWatchers(e, &Action{
-		ActUserId:    u.Id,
+		ActUserID:    u.Id,
 		ActUserName:  u.Name,
 		ActEmail:     u.Email,
 		OpType:       CREATE_REPO,
-		RepoId:       repo.Id,
+		RepoID:       repo.Id,
 		RepoUserName: repo.Owner.Name,
 		RepoName:     repo.Name,
-		IsPrivate:    repo.IsPrivate}); err != nil {
+		IsPrivate:    repo.IsPrivate,
+	}); err != nil {
 		return fmt.Errorf("notify watchers '%d/%s'", u.Id, repo.Id)
 	}
 
@@ -465,11 +474,11 @@ func NewRepoAction(u *User, repo *Repository) (err error) {
 
 func transferRepoAction(e Engine, actUser, oldOwner, newOwner *User, repo *Repository) (err error) {
 	action := &Action{
-		ActUserId:    actUser.Id,
+		ActUserID:    actUser.Id,
 		ActUserName:  actUser.Name,
 		ActEmail:     actUser.Email,
 		OpType:       TRANSFER_REPO,
-		RepoId:       repo.Id,
+		RepoID:       repo.Id,
 		RepoUserName: newOwner.Name,
 		RepoName:     repo.Name,
 		IsPrivate:    repo.IsPrivate,

models/error.go

@@ -8,6 +8,59 @@ import (
 	"fmt"
 )
 
+//  ____ ___
+// |    |   \______ ___________
+// |    |   /  ___// __ \_  __ \
+// |    |  /\___ \\  ___/|  | \/
+// |______//____  >\___  >__|
+//              \/     \/
+
+type ErrUserOwnRepos struct {
+	UID int64
+}
+
+func IsErrUserOwnRepos(err error) bool {
+	_, ok := err.(ErrUserOwnRepos)
+	return ok
+}
+
+func (err ErrUserOwnRepos) Error() string {
+	return fmt.Sprintf("user still has ownership of repositories: [uid: %d]", err.UID)
+}
+
+type ErrUserHasOrgs struct {
+	UID int64
+}
+
+func IsErrUserHasOrgs(err error) bool {
+	_, ok := err.(ErrUserHasOrgs)
+	return ok
+}
+
+func (err ErrUserHasOrgs) Error() string {
+	return fmt.Sprintf("user still has membership of organizations: [uid: %d]", err.UID)
+}
+
+// ________                            .__                __  .__
+// \_____  \_______  _________    ____ |__|____________ _/  |_|__| ____   ____
+//  /   |   \_  __ \/ ___\__  \  /    \|  \___   /\__  \\   __\  |/  _ \ /    \
+// /    |    \  | \/ /_/  > __ \|   |  \  |/    /  / __ \|  | |  (  <_> )   |  \
+// \_______  /__|  \___  (____  /___|  /__/_____ \(____  /__| |__|\____/|___|  /
+//         \/     /_____/     \/     \/         \/     \/                    \/
+
+type ErrLastOrgOwner struct {
+	UID int64
+}
+
+func IsErrLastOrgOwner(err error) bool {
+	_, ok := err.(ErrLastOrgOwner)
+	return ok
+}
+
+func (err ErrLastOrgOwner) Error() string {
+	return fmt.Sprintf("user is the last member of owner team: [uid: %d]", err.UID)
+}
+
 // __________                           .__  __
 // \______   \ ____ ______   ____  _____|__|/  |_  ___________ ___.__.
 //  |       _// __ \\____ \ /  _ \/  ___/  \   __\/  _ \_  __ <   |  |

models/org.go

@@ -18,7 +18,6 @@ var (
 	ErrTeamAlreadyExist = errors.New("Team already exist")
 	ErrTeamNotExist     = errors.New("Team does not exist")
 	ErrTeamNameIllegal  = errors.New("Team name contains illegal characters")
-	ErrLastOrgOwner     = errors.New("The user to remove is the last member in owner team")
 )
 
 // IsOwnedBy returns true if given user is in the owner team.
@@ -339,18 +338,20 @@ func RemoveOrgUser(orgId, uid int64) error {
 
 	has, err := x.Where("uid=?", uid).And("org_id=?", orgId).Get(ou)
 	if err != nil {
-		return err
+		return fmt.Errorf("get org-user: %v", err)
 	} else if !has {
 		return nil
 	}
 
 	u, err := GetUserById(uid)
 	if err != nil {
-		return err
+		return fmt.Errorf("GetUserById: %v", err)
 	}
 	org, err := GetUserById(orgId)
 	if err != nil {
-		return err
+		return fmt.Errorf("get organization: %v", err)
+	} else if err = org.GetRepositories(); err != nil {
+		return fmt.Errorf("GetRepositories: %v", err)
 	}
 
 	// Check if the user to delete is the last member in owner team.
@@ -360,49 +361,39 @@ func RemoveOrgUser(orgId, uid int64) error {
 			return err
 		}
 		if t.NumMembers == 1 {
-			return ErrLastOrgOwner
+			return ErrLastOrgOwner{UID: uid}
 		}
 	}
 
 	sess := x.NewSession()
-	defer sess.Close()
+	defer sessionRelease(sess)
 	if err := sess.Begin(); err != nil {
 		return err
 	}
 
 	if _, err := sess.Id(ou.ID).Delete(ou); err != nil {
-		sess.Rollback()
 		return err
 	} else if _, err = sess.Exec("UPDATE `user` SET num_members=num_members-1 WHERE id=?", orgId); err != nil {
-		sess.Rollback()
 		return err
 	}
 
 	// Delete all repository accesses.
-	if err = org.GetRepositories(); err != nil {
-		sess.Rollback()
-		return err
-	}
-	access := &Access{
-		UserID: u.Id,
-	}
+	access := &Access{UserID: u.Id}
 	for _, repo := range org.Repos {
 		access.RepoID = repo.Id
 		if _, err = sess.Delete(access); err != nil {
-			sess.Rollback()
 			return err
-		} else if err = WatchRepo(u.Id, repo.Id, false); err != nil {
-			sess.Rollback()
+		} else if err = watchRepo(sess, u.Id, repo.Id, false); err != nil {
 			return err
 		}
 	}
 
 	// Delete member in his/her teams.
-	ts, err := GetUserTeams(org.Id, u.Id)
+	teams, err := getUserTeams(sess, org.Id, u.Id)
 	if err != nil {
 		return err
 	}
-	for _, t := range ts {
+	for _, t := range teams {
 		if err = removeTeamMember(sess, org.Id, t.ID, u.Id); err != nil {
 			return err
 		}
@@ -902,7 +893,7 @@ func removeTeamMember(e Engine, orgId, teamId, uid int64) error {
 
 	// Check if the user to delete is the last member in owner team.
 	if t.IsOwnerTeam() && t.NumMembers == 1 {
-		return ErrLastOrgOwner
+		return ErrLastOrgOwner{UID: uid}
 	}
 
 	t.NumMembers--

models/repo.go

@@ -819,9 +819,9 @@ func DeleteRepository(uid, repoID int64, userName string) error {
 		return err
 	} else if _, err = sess.Delete(&Access{RepoID: repo.Id}); err != nil {
 		return err
-	} else if _, err = sess.Delete(&Action{RepoId: repo.Id}); err != nil {
+	} else if _, err = sess.Delete(&Action{RepoID: repo.Id}); err != nil {
 		return err
-	} else if _, err = sess.Delete(&Watch{RepoId: repoID}); err != nil {
+	} else if _, err = sess.Delete(&Watch{RepoID: repoID}); err != nil {
 		return err
 	} else if _, err = sess.Delete(&Mirror{RepoId: repoID}); err != nil {
 		return err
@@ -1190,9 +1190,9 @@ func (repo *Repository) DeleteCollaborator(u *User) (err error) {
 
 // Watch is connection request for receiving repository notification.
 type Watch struct {
-	Id     int64
-	UserId int64 `xorm:"UNIQUE(watch)"`
-	RepoId int64 `xorm:"UNIQUE(watch)"`
+	ID     int64 `xorm:"pk autoincr"`
+	UserID int64 `xorm:"UNIQUE(watch)"`
+	RepoID int64 `xorm:"UNIQUE(watch)"`
 }
 
 // IsWatching checks if user has watched given repository.
@@ -1206,7 +1206,7 @@ func watchRepo(e Engine, uid, repoId int64, watch bool) (err error) {
 		if IsWatching(uid, repoId) {
 			return nil
 		}
-		if _, err = e.Insert(&Watch{RepoId: repoId, UserId: uid}); err != nil {
+		if _, err = e.Insert(&Watch{RepoID: repoId, UserID: uid}); err != nil {
 			return err
 		}
 		_, err = e.Exec("UPDATE `repository` SET num_watches = num_watches + 1 WHERE id = ?", repoId)
@@ -1229,7 +1229,7 @@ func WatchRepo(uid, repoId int64, watch bool) (err error) {
 
 func getWatchers(e Engine, rid int64) ([]*Watch, error) {
 	watches := make([]*Watch, 0, 10)
-	err := e.Find(&watches, &Watch{RepoId: rid})
+	err := e.Find(&watches, &Watch{RepoID: rid})
 	return watches, err
 }
 
@@ -1240,24 +1240,24 @@ func GetWatchers(rid int64) ([]*Watch, error) {
 
 func notifyWatchers(e Engine, act *Action) error {
 	// Add feeds for user self and all watchers.
-	watches, err := getWatchers(e, act.RepoId)
+	watches, err := getWatchers(e, act.RepoID)
 	if err != nil {
 		return fmt.Errorf("get watchers: %v", err)
 	}
 
 	// Add feed for actioner.
-	act.UserId = act.ActUserId
+	act.UserID = act.ActUserID
 	if _, err = e.InsertOne(act); err != nil {
 		return fmt.Errorf("insert new actioner: %v", err)
 	}
 
 	for i := range watches {
-		if act.ActUserId == watches[i].UserId {
+		if act.ActUserID == watches[i].UserID {
 			continue
 		}
 
-		act.Id = 0
-		act.UserId = watches[i].UserId
+		act.ID = 0
+		act.UserID = watches[i].UserID
 		if _, err = e.InsertOne(act); err != nil {
 			return fmt.Errorf("insert new action: %v", err)
 		}

models/user.go

@@ -36,8 +36,6 @@ const (
 )
 
 var (
-	ErrUserOwnRepos          = errors.New("User still have ownership of repositories")
-	ErrUserHasOrgs           = errors.New("User still have membership of organization")
 	ErrUserAlreadyExist      = errors.New("User already exist")
 	ErrUserNotExist          = errors.New("User does not exist")
 	ErrUserNotKeyOwner       = errors.New("User does not the owner of public key")
@@ -432,55 +430,75 @@ func UpdateUser(u *User) error {
 	return err
 }
 
+// DeleteBeans deletes all given beans, beans should contain delete conditions.
+func DeleteBeans(e Engine, beans ...interface{}) (err error) {
+	for i := range beans {
+		if _, err = e.Delete(beans[i]); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // FIXME: need some kind of mechanism to record failure. HINT: system notice
 // DeleteUser completely and permanently deletes everything of user.
 func DeleteUser(u *User) error {
 	// Check ownership of repository.
 	count, err := GetRepositoryCount(u)
 	if err != nil {
-		return errors.New("GetRepositoryCount: " + err.Error())
+		return fmt.Errorf("GetRepositoryCount: %v", err)
 	} else if count > 0 {
-		return ErrUserOwnRepos
+		return ErrUserOwnRepos{UID: u.Id}
 	}
 
 	// Check membership of organization.
 	count, err = u.GetOrganizationCount()
 	if err != nil {
-		return errors.New("GetOrganizationCount: " + err.Error())
+		return fmt.Errorf("GetOrganizationCount: %v", err)
 	} else if count > 0 {
-		return ErrUserHasOrgs
+		return ErrUserHasOrgs{UID: u.Id}
+	}
+
+	// Get watches before session.
+	watches := make([]*Watch, 0, 10)
+	if err = x.Where("user_id=?", u.Id).Find(&watches); err != nil {
+		return fmt.Errorf("get all watches: %v", err)
+	}
+	repoIDs := make([]int64, 0, len(watches))
+	for i := range watches {
+		repoIDs = append(repoIDs, watches[i].RepoID)
 	}
 
 	// FIXME: check issues, other repos' commits
-	// FIXME: roll backable in some point.
 
-	// Delete all followers.
-	if _, err = x.Delete(&Follow{FollowId: u.Id}); err != nil {
+	sess := x.NewSession()
+	defer sessionRelease(sess)
+	if err = sess.Begin(); err != nil {
 		return err
 	}
-	// Delete oauth2.
-	if _, err = x.Delete(&Oauth2{Uid: u.Id}); err != nil {
+
+	if err = DeleteBeans(sess,
+		&Follow{FollowID: u.Id},
+		&Oauth2{Uid: u.Id},
+		&Action{UserID: u.Id},
+		&Access{UserID: u.Id},
+		&Collaboration{UserID: u.Id},
+		&EmailAddress{Uid: u.Id},
+		&Watch{UserID: u.Id},
+	); err != nil {
 		return err
 	}
-	// Delete all feeds.
-	if _, err = x.Delete(&Action{UserId: u.Id}); err != nil {
+
+	// Decrease all watch numbers.
+	for i := range repoIDs {
+		if _, err = sess.Exec("UPDATE `repository` SET num_watches=num_watches-1 WHERE id=?", repoIDs[i]); err != nil {
 			return err
 		}
-	// Delete all watches.
-	if _, err = x.Delete(&Watch{UserId: u.Id}); err != nil {
-		return err
-	}
-	// Delete all accesses.
-	if _, err = x.Delete(&Access{UserID: u.Id}); err != nil {
-		return err
-	}
-	// Delete all alternative email addresses
-	if _, err = x.Delete(&EmailAddress{Uid: u.Id}); err != nil {
-		return err
 	}
+
 	// Delete all SSH keys.
 	keys := make([]*PublicKey, 0, 10)
-	if err = x.Find(&keys, &PublicKey{OwnerId: u.Id}); err != nil {
+	if err = sess.Find(&keys, &PublicKey{OwnerId: u.Id}); err != nil {
 		return err
 	}
 	for _, key := range keys {
@@ -489,13 +507,16 @@ func DeleteUser(u *User) error {
 		}
 	}
 
+	if _, err = sess.Delete(u); err != nil {
+		return err
+	}
+
 	// Delete user directory.
 	if err = os.RemoveAll(UserPath(u.Name)); err != nil {
 		return err
 	}
 
-	_, err = x.Delete(u)
-	return err
+	return sess.Commit()
 }
 
 // DeleteInactivateUsers deletes all inactivate users and email addresses.
@@ -777,8 +798,8 @@ func SearchUserByName(opt SearchOption) (us []*User, err error) {
 // Follow is connection request for receiving user notification.
 type Follow struct {
 	Id       int64
-	UserId   int64 `xorm:"unique(follow)"`
-	FollowId int64 `xorm:"unique(follow)"`
+	UserID   int64 `xorm:"unique(follow)"`
+	FollowID int64 `xorm:"unique(follow)"`
 }
 
 // FollowUser marks someone be another's follower.
@@ -787,7 +808,7 @@ func FollowUser(userId int64, followId int64) (err error) {
 	defer sess.Close()
 	sess.Begin()
 
-	if _, err = sess.Insert(&Follow{UserId: userId, FollowId: followId}); err != nil {
+	if _, err = sess.Insert(&Follow{UserID: userId, FollowID: followId}); err != nil {
 		sess.Rollback()
 		return err
 	}
@@ -812,7 +833,7 @@ func UnFollowUser(userId int64, unFollowId int64) (err error) {
 	defer session.Close()
 	session.Begin()
 
-	if _, err = session.Delete(&Follow{UserId: userId, FollowId: unFollowId}); err != nil {
+	if _, err = session.Delete(&Follow{UserID: userId, FollowID: unFollowId}); err != nil {
 		session.Rollback()
 		return err
 	}

modules/mailer/mail.go

@@ -163,7 +163,7 @@ func SendIssueNotifyMail(u, owner *models.User, repo *models.Repository, issue *
 
 	tos := make([]string, 0, len(ws))
 	for i := range ws {
-		uid := ws[i].UserId
+		uid := ws[i].UserID
 		if u.Id == uid {
 			continue
 		}

routers/admin/users.go

@@ -222,11 +222,11 @@ func DeleteUser(ctx *middleware.Context) {
 	}
 
 	if err = models.DeleteUser(u); err != nil {
-		switch err {
-		case models.ErrUserOwnRepos:
+		switch {
+		case models.IsErrUserOwnRepos(err):
 			ctx.Flash.Error(ctx.Tr("admin.users.still_own_repo"))
 			ctx.Redirect(setting.AppSubUrl + "/admin/users/" + ctx.Params(":userid"))
-		case models.ErrUserHasOrgs:
+		case models.IsErrUserHasOrgs(err):
 			ctx.Flash.Error(ctx.Tr("admin.users.still_has_org"))
 			ctx.Redirect(setting.AppSubUrl + "/admin/users/" + ctx.Params(":userid"))
 		default:

routers/org/members.go

@@ -61,14 +61,14 @@ func MembersAction(ctx *middleware.Context) {
 			return
 		}
 		err = org.RemoveMember(uid)
-		if err == models.ErrLastOrgOwner {
+		if models.IsErrLastOrgOwner(err) {
 			ctx.Flash.Error(ctx.Tr("form.last_org_owner"))
 			ctx.Redirect(ctx.Org.OrgLink + "/members")
 			return
 		}
 	case "leave":
 		err = org.RemoveMember(ctx.User.Id)
-		if err == models.ErrLastOrgOwner {
+		if models.IsErrLastOrgOwner(err) {
 			ctx.Flash.Error(ctx.Tr("form.last_org_owner"))
 			ctx.Redirect(ctx.Org.OrgLink + "/members")
 			return

routers/org/setting.go

@@ -87,13 +87,12 @@ func SettingsDelete(ctx *middleware.Context) {
 
 	org := ctx.Org.Organization
 	if ctx.Req.Method == "POST" {
-		// TODO: validate password.
+		// FIXME: validate password.
 		if err := models.DeleteOrganization(org); err != nil {
-			switch err {
-			case models.ErrUserOwnRepos:
+			if models.IsErrUserOwnRepos(err) {
 				ctx.Flash.Error(ctx.Tr("form.org_still_own_repo"))
 				ctx.Redirect(setting.AppSubUrl + "/org/" + org.LowerName + "/settings/delete")
-			default:
+			} else {
 				ctx.Handle(500, "DeleteOrganization", err)
 			}
 		} else {

routers/org/teams.go

@@ -91,7 +91,7 @@ func TeamsAction(ctx *middleware.Context) {
 	}
 
 	if err != nil {
-		if err == models.ErrLastOrgOwner {
+		if models.IsErrLastOrgOwner(err) {
 			ctx.Flash.Error(ctx.Tr("form.last_org_owner"))
 		} else {
 			log.Error(3, "Action(%s): %v", ctx.Params(":action"), err)

routers/repo/issue.go

@@ -270,12 +270,12 @@ func CreateIssuePost(ctx *middleware.Context, form auth.CreateIssueForm) {
 	}
 
 	act := &models.Action{
-		ActUserId:    ctx.User.Id,
+		ActUserID:    ctx.User.Id,
 		ActUserName:  ctx.User.Name,
 		ActEmail:     ctx.User.Email,
 		OpType:       models.CREATE_ISSUE,
 		Content:      fmt.Sprintf("%d|%s", issue.Index, issue.Name),
-		RepoId:       ctx.Repo.Repository.Id,
+		RepoID:       ctx.Repo.Repository.Id,
 		RepoUserName: ctx.Repo.Owner.Name,
 		RepoName:     ctx.Repo.Repository.Name,
 		RefName:      ctx.Repo.BranchName,
@@ -845,12 +845,12 @@ func Comment(ctx *middleware.Context) {
 
 	// Notify watchers.
 	act := &models.Action{
-		ActUserId:    ctx.User.Id,
+		ActUserID:    ctx.User.Id,
 		ActUserName:  ctx.User.LowerName,
 		ActEmail:     ctx.User.Email,
 		OpType:       models.COMMENT_ISSUE,
 		Content:      fmt.Sprintf("%d|%s", issue.Index, strings.Split(content, "\n")[0]),
-		RepoId:       ctx.Repo.Repository.Id,
+		RepoID:       ctx.Repo.Repository.Id,
 		RepoUserName: ctx.Repo.Owner.LowerName,
 		RepoName:     ctx.Repo.Repository.LowerName,
 	}

routers/user/home.go

@@ -104,7 +104,7 @@ func Dashboard(ctx *middleware.Context) {
 	for _, act := range actions {
 		if act.IsPrivate {
 			// This prevents having to retrieve the repository for each action
-			repo := &models.Repository{Id: act.RepoId, IsPrivate: true}
+			repo := &models.Repository{Id: act.RepoID, IsPrivate: true}
 			if act.RepoUserName != ctx.User.LowerName {
 				if has, _ := models.HasAccess(ctx.User, repo, models.ACCESS_MODE_READ); !has {
 					continue
@@ -216,7 +216,7 @@ func Profile(ctx *middleware.Context) {
 					continue
 				}
 				// This prevents having to retrieve the repository for each action
-				repo := &models.Repository{Id: act.RepoId, IsPrivate: true}
+				repo := &models.Repository{Id: act.RepoID, IsPrivate: true}
 				if act.RepoUserName != ctx.User.LowerName {
 					if has, _ := models.HasAccess(ctx.User, repo, models.ACCESS_MODE_READ); !has {
 						continue

routers/user/setting.go

@@ -451,20 +451,13 @@ func SettingsDelete(ctx *middleware.Context) {
 	ctx.Data["PageIsSettingsDelete"] = true
 
 	if ctx.Req.Method == "POST" {
-		// tmpUser := models.User{
-		// 	Passwd: ctx.Query("password"),
-		// 	Salt:   ctx.User.Salt,
-		// }
-		// tmpUser.EncodePasswd()
-		// if tmpUser.Passwd != ctx.User.Passwd {
-		// 	ctx.Flash.Error("Password is not correct. Make sure you are owner of this account.")
-		// } else {
+		// FIXME: validate password.
 		if err := models.DeleteUser(ctx.User); err != nil {
-			switch err {
-			case models.ErrUserOwnRepos:
+			switch {
+			case models.IsErrUserOwnRepos(err):
 				ctx.Flash.Error(ctx.Tr("form.still_own_repo"))
 				ctx.Redirect(setting.AppSubUrl + "/user/settings/delete")
-			case models.ErrUserHasOrgs:
+			case models.IsErrUserHasOrgs(err):
 				ctx.Flash.Error(ctx.Tr("form.still_has_org"))
 				ctx.Redirect(setting.AppSubUrl + "/user/settings/delete")
 			default: