[MM-16777] Migrate Team.MigrateTeamMembers to Sync by default (#11584)

* Migrate Team.MigrateTeamMembers to Sync by default

* Remove commented out MigrateTeamMembers function

* Remove return nil when no team members in query

* Return nil when no more team members in query

* Remove commented out code lines

* Format MigrateTeamMembers code

* Return some data from MigrateTeamMembers function

* Return data and error from MigrateTeamMembers

* Use result for consistency

* Control loop to allow loop to break in storetest/team_store.go

* Fix test MigrateTeamMembers test in storetest/team_store.go

* Change err to e in storetest/team_store.go

migrations/advanced_permissions_phase_2.go

@@ -71,19 +71,18 @@ func (worker *Worker) runAdvancedPermissionsPhase2Migration(lastDone string) (bo
 
 	if progress.CurrentTable == "TeamMembers" {
 		// Run a TeamMembers migration batch.
-		if result := <-worker.app.Srv.Store.Team().MigrateTeamMembers(progress.LastTeamId, progress.LastUserId); result.Err != nil {
+		if result, err := worker.app.Srv.Store.Team().MigrateTeamMembers(progress.LastTeamId, progress.LastUserId); err != nil {
-			return false, progress.ToJson(), result.Err
+			return false, progress.ToJson(), err
 		} else {
-			if result.Data == nil {
+			if result == nil {
 				// We haven't progressed. That means that we've reached the end of this stage of the migration, and should now advance to the next stage.
 				progress.LastUserId = strings.Repeat("0", 26)
 				progress.CurrentTable = "ChannelMembers"
 				return false, progress.ToJson(), nil
 			}
 
-			data := result.Data.(map[string]string)
+			progress.LastTeamId = result["TeamId"]
-			progress.LastTeamId = data["TeamId"]
+			progress.LastUserId = result["UserId"]
-			progress.LastUserId = data["UserId"]
 		}
 	} else if progress.CurrentTable == "ChannelMembers" {
 		// Run a ChannelMembers migration batch.

store/sqlstore/team_store.go

@@ -802,70 +802,65 @@ func (s SqlTeamStore) GetTeamsByScheme(schemeId string, offset int, limit int) (
 // in batches as a single transaction per batch to ensure consistency but to also minimise execution time to avoid
 // causing unnecessary table locks. **THIS FUNCTION SHOULD NOT BE USED FOR ANY OTHER PURPOSE.** Executing this function
 // *after* the new Schemes functionality has been used on an installation will have unintended consequences.
-func (s SqlTeamStore) MigrateTeamMembers(fromTeamId string, fromUserId string) store.StoreChannel {
+func (s SqlTeamStore) MigrateTeamMembers(fromTeamId string, fromUserId string) (map[string]string, *model.AppError) {
-	return store.Do(func(result *store.StoreResult) {
+	var transaction *gorp.Transaction
-		var transaction *gorp.Transaction
+	var err error
-		var err error
 
-		if transaction, err = s.GetMaster().Begin(); err != nil {
+	if transaction, err = s.GetMaster().Begin(); err != nil {
-			result.Err = model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return nil, model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
-			return
+	}
+	defer finalizeTransaction(transaction)
+
+	var teamMembers []teamMember
+	if _, err := transaction.Select(&teamMembers, "SELECT * from TeamMembers WHERE (TeamId, UserId) > (:FromTeamId, :FromUserId) ORDER BY TeamId, UserId LIMIT 100", map[string]interface{}{"FromTeamId": fromTeamId, "FromUserId": fromUserId}); err != nil {
+		return nil, model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.select.app_error", nil, err.Error(), http.StatusInternalServerError)
+	}
+
+	if len(teamMembers) == 0 {
+		// No more team members in query result means that the migration has finished.
+		return nil, nil
+	}
+
+	for _, member := range teamMembers {
+		roles := strings.Fields(member.Roles)
+		var newRoles []string
+		if !member.SchemeAdmin.Valid {
+			member.SchemeAdmin = sql.NullBool{Bool: false, Valid: true}
 		}
-		defer finalizeTransaction(transaction)
+		if !member.SchemeUser.Valid {
+			member.SchemeUser = sql.NullBool{Bool: false, Valid: true}
+		}
+		if !member.SchemeGuest.Valid {
+			member.SchemeGuest = sql.NullBool{Bool: false, Valid: true}
+		}
+		for _, role := range roles {
+			if role == model.TEAM_ADMIN_ROLE_ID {
+				member.SchemeAdmin = sql.NullBool{Bool: true, Valid: true}
+			} else if role == model.TEAM_USER_ROLE_ID {
+				member.SchemeUser = sql.NullBool{Bool: true, Valid: true}
+			} else if role == model.TEAM_GUEST_ROLE_ID {
+				member.SchemeGuest = sql.NullBool{Bool: true, Valid: true}
+			} else {
+				newRoles = append(newRoles, role)
+			}
+		}
+		member.Roles = strings.Join(newRoles, " ")
 
-		var teamMembers []teamMember
+		if _, err := transaction.Update(&member); err != nil {
-		if _, err := transaction.Select(&teamMembers, "SELECT * from TeamMembers WHERE (TeamId, UserId) > (:FromTeamId, :FromUserId) ORDER BY TeamId, UserId LIMIT 100", map[string]interface{}{"FromTeamId": fromTeamId, "FromUserId": fromUserId}); err != nil {
+			return nil, model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.update.app_error", nil, err.Error(), http.StatusInternalServerError)
-			result.Err = model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.select.app_error", nil, err.Error(), http.StatusInternalServerError)
-			return
 		}
 
-		if len(teamMembers) == 0 {
+	}
-			// No more team members in query result means that the migration has finished.
-			return
-		}
 
-		for _, member := range teamMembers {
+	if err := transaction.Commit(); err != nil {
-			roles := strings.Fields(member.Roles)
+		return nil, model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
-			var newRoles []string
+	}
-			if !member.SchemeAdmin.Valid {
-				member.SchemeAdmin = sql.NullBool{Bool: false, Valid: true}
-			}
-			if !member.SchemeUser.Valid {
-				member.SchemeUser = sql.NullBool{Bool: false, Valid: true}
-			}
-			if !member.SchemeGuest.Valid {
-				member.SchemeGuest = sql.NullBool{Bool: false, Valid: true}
-			}
-			for _, role := range roles {
-				if role == model.TEAM_ADMIN_ROLE_ID {
-					member.SchemeAdmin = sql.NullBool{Bool: true, Valid: true}
-				} else if role == model.TEAM_USER_ROLE_ID {
-					member.SchemeUser = sql.NullBool{Bool: true, Valid: true}
-				} else if role == model.TEAM_GUEST_ROLE_ID {
-					member.SchemeGuest = sql.NullBool{Bool: true, Valid: true}
-				} else {
-					newRoles = append(newRoles, role)
-				}
-			}
-			member.Roles = strings.Join(newRoles, " ")
 
-			if _, err := transaction.Update(&member); err != nil {
+	data := make(map[string]string)
-				result.Err = model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.update.app_error", nil, err.Error(), http.StatusInternalServerError)
+	data["TeamId"] = teamMembers[len(teamMembers)-1].TeamId
-				return
+	data["UserId"] = teamMembers[len(teamMembers)-1].UserId
-			}
 
-		}
+	return data, nil
-
-		if err := transaction.Commit(); err != nil {
-			result.Err = model.NewAppError("SqlTeamStore.MigrateTeamMembers", "store.sql_team.migrate_team_members.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError)
-			return
-		}
-
-		data := make(map[string]string)
-		data["TeamId"] = teamMembers[len(teamMembers)-1].TeamId
-		data["UserId"] = teamMembers[len(teamMembers)-1].UserId
-		result.Data = data
-	})
 }
 
 func (s SqlTeamStore) ResetAllTeamSchemes() *model.AppError {

store/store.go

@@ -116,7 +116,7 @@ type TeamStore interface {
 	RemoveAllMembersByUser(userId string) *model.AppError
 	UpdateLastTeamIconUpdate(teamId string, curTime int64) *model.AppError
 	GetTeamsByScheme(schemeId string, offset int, limit int) ([]*model.Team, *model.AppError)
-	MigrateTeamMembers(fromTeamId string, fromUserId string) StoreChannel
+	MigrateTeamMembers(fromTeamId string, fromUserId string) (map[string]string, *model.AppError)
 	ResetAllTeamSchemes() *model.AppError
 	ClearAllCustomRoleAssignments() StoreChannel
 	AnalyticsGetTeamCountForScheme(schemeId string) StoreChannel

store/storetest/mocks/TeamStore.go

@@ -650,19 +650,28 @@ func (_m *TeamStore) InvalidateAllTeamIdsForUser(userId string) {
 }
 
 // MigrateTeamMembers provides a mock function with given fields: fromTeamId, fromUserId
-func (_m *TeamStore) MigrateTeamMembers(fromTeamId string, fromUserId string) store.StoreChannel {
+func (_m *TeamStore) MigrateTeamMembers(fromTeamId string, fromUserId string) (map[string]string, *model.AppError) {
 	ret := _m.Called(fromTeamId, fromUserId)
 
-	var r0 store.StoreChannel
+	var r0 map[string]string
-	if rf, ok := ret.Get(0).(func(string, string) store.StoreChannel); ok {
+	if rf, ok := ret.Get(0).(func(string, string) map[string]string); ok {
 		r0 = rf(fromTeamId, fromUserId)
 	} else {
 		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(store.StoreChannel)
+			r0 = ret.Get(0).(map[string]string)
 		}
 	}
 
-	return r0
+	var r1 *model.AppError
+	if rf, ok := ret.Get(1).(func(string, string) *model.AppError); ok {
+		r1 = rf(fromTeamId, fromUserId)
+	} else {
+		if ret.Get(1) != nil {
+			r1 = ret.Get(1).(*model.AppError)
+		}
+	}
+
+	return r0, r1
 }
 
 // PermanentDelete provides a mock function with given fields: teamId

store/storetest/team_store.go

@@ -1413,14 +1413,13 @@ func testTeamStoreMigrateTeamMembers(t *testing.T, ss store.Store) {
 	lastDoneUserId := strings.Repeat("0", 26)
 
 	for {
-		res := <-ss.Team().MigrateTeamMembers(lastDoneTeamId, lastDoneUserId)
+		res, e := ss.Team().MigrateTeamMembers(lastDoneTeamId, lastDoneUserId)
-		if assert.Nil(t, res.Err) {
+		if assert.Nil(t, e) {
-			if res.Data == nil {
+			if res == nil {
 				break
 			}
-			data := res.Data.(map[string]string)
+			lastDoneTeamId = res["TeamId"]
-			lastDoneTeamId = data["TeamId"]
+			lastDoneUserId = res["UserId"]
-			lastDoneUserId = data["UserId"]
 		}
 	}