MM-15276: Migrate Team.Update to sync by default (#10693)

* MM-15276: Migrate Team.Update to sync by default

* MM-15276: Addressing review comments and change Update func signature similar to other interface Update method

* update store mocks for update fn

* addressing review comments

api4/post_test.go

@@ -602,7 +602,6 @@ func TestUpdatePost(t *testing.T) {
 	assert.NotEqual(t, rpost3.EditAt, rrupost3.EditAt)
 	assert.NotEqual(t, rpost3.Attachments(), rrupost3.Attachments())
 
-
 	Client.Logout()
 	_, resp = Client.UpdatePost(rpost.Id, rpost)
 	CheckUnauthorizedStatus(t, resp)

api4/scheme_test.go

@@ -304,9 +304,8 @@ func TestGetTeamsForScheme(t *testing.T) {
 	assert.Zero(t, len(l2))
 
 	team1.SchemeId = &scheme1.Id
-	result2 := <-th.App.Srv.Store.Team().Update(team1)
-	assert.Nil(t, result2.Err)
-	team1 = result2.Data.(*model.Team)
+	team1, err := th.App.Srv.Store.Team().Update(team1)
+	assert.Nil(t, err)
 
 	l3, r3 := th.SystemAdminClient.GetTeamsForScheme(scheme1.Id, 0, 100)
 	CheckNoError(t, r3)

app/team.go

@@ -136,12 +136,7 @@ func (a *App) UpdateTeam(team *model.Team) (*model.Team, *model.AppError) {
 }
 
 func (a *App) updateTeamUnsanitized(team *model.Team) (*model.Team, *model.AppError) {
-	result := <-a.Srv.Store.Team().Update(team)
-	if result.Err != nil {
-		return nil, result.Err
-	}
-
-	return result.Data.(*model.Team), nil
+	return a.Srv.Store.Team().Update(team)
 }
 
 // RenameTeam is used to rename the team Name and the DisplayName fields
@@ -180,8 +175,8 @@ func (a *App) UpdateTeamScheme(team *model.Team) (*model.Team, *model.AppError)
 
 	oldTeam.SchemeId = team.SchemeId
 
-	if result := <-a.Srv.Store.Team().Update(oldTeam); result.Err != nil {
-		return nil, result.Err
+	if oldTeam, err = a.Srv.Store.Team().Update(oldTeam); err != nil {
+		return nil, err
 	}
 
 	a.sendTeamEvent(oldTeam, model.WEBSOCKET_EVENT_UPDATE_TEAM)
@@ -1051,8 +1046,8 @@ func (a *App) PermanentDeleteTeamId(teamId string) *model.AppError {
 
 func (a *App) PermanentDeleteTeam(team *model.Team) *model.AppError {
 	team.DeleteAt = model.GetMillis()
-	if result := <-a.Srv.Store.Team().Update(team); result.Err != nil {
-		return result.Err
+	if _, err := a.Srv.Store.Team().Update(team); err != nil {
+		return err
 	}
 
 	if result := <-a.Srv.Store.Channel().GetTeamChannels(team.Id); result.Err != nil {
@@ -1090,8 +1085,8 @@ func (a *App) SoftDeleteTeam(teamId string) *model.AppError {
 	}
 
 	team.DeleteAt = model.GetMillis()
-	if result := <-a.Srv.Store.Team().Update(team); result.Err != nil {
-		return result.Err
+	if team, err = a.Srv.Store.Team().Update(team); err != nil {
+		return err
 	}
 
 	a.sendTeamEvent(team, model.WEBSOCKET_EVENT_DELETE_TEAM)
@@ -1104,11 +1099,12 @@ func (a *App) RestoreTeam(teamId string) *model.AppError {
 	if err != nil {
 		return err
 	}
+
 	team.DeleteAt = 0
-	result := <-a.Srv.Store.Team().Update(team)
-	if result.Err != nil {
-		return result.Err
+	if team, err = a.Srv.Store.Team().Update(team); err != nil {
+		return err
 	}
+
 	a.sendTeamEvent(team, model.WEBSOCKET_EVENT_RESTORE_TEAM)
 	return nil
 }

model/post_test.go

@@ -181,10 +181,8 @@ func TestPostSanitizeProps(t *testing.T) {
 }
 
 func TestPost_AttachmentsEqual(t *testing.T) {
-	post1 := &Post {
-	}
-	post2 := &Post {
-	}
+	post1 := &Post{}
+	post2 := &Post{}
 	for name, tc := range map[string]struct {
 		Attachments1 []*SlackAttachment
 		Attachments2 []*SlackAttachment

services/httpservice/client_test.go

@@ -181,7 +181,7 @@ func TestIsOwnIP(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			if got, _ := IsOwnIP(tt.ip); got != tt.want {
 				t.Errorf("IsOwnIP() = %v, want %v", got, tt.want)
-				t.Errorf(tt.ip.String());
+				t.Errorf(tt.ip.String())
 			}
 		})
 	}

store/sqlstore/team_store.go

@@ -194,23 +194,22 @@ func (s SqlTeamStore) Save(team *model.Team) store.StoreChannel {
 	})
 }
 
-func (s SqlTeamStore) Update(team *model.Team) store.StoreChannel {
-	return store.Do(func(result *store.StoreResult) {
+func (s SqlTeamStore) Update(team *model.Team) (*model.Team, *model.AppError) {
+
 	team.PreUpdate()
 
-		if result.Err = team.IsValid(); result.Err != nil {
-			return
+	if err := team.IsValid(); err != nil {
+		return nil, err
 	}
 
 	oldResult, err := s.GetMaster().Get(model.Team{}, team.Id)
 	if err != nil {
-			result.Err = model.NewAppError("SqlTeamStore.Update", "store.sql_team.update.finding.app_error", nil, "id="+team.Id+", "+err.Error(), http.StatusInternalServerError)
-			return
+		return nil, model.NewAppError("SqlTeamStore.Update", "store.sql_team.update.finding.app_error", nil, "id="+team.Id+", "+err.Error(), http.StatusInternalServerError)
+
 	}
 
 	if oldResult == nil {
-			result.Err = model.NewAppError("SqlTeamStore.Update", "store.sql_team.update.find.app_error", nil, "id="+team.Id, http.StatusBadRequest)
-			return
+		return nil, model.NewAppError("SqlTeamStore.Update", "store.sql_team.update.find.app_error", nil, "id="+team.Id, http.StatusBadRequest)
 	}
 
 	oldTeam := oldResult.(*model.Team)
@@ -219,16 +218,13 @@ func (s SqlTeamStore) Update(team *model.Team) store.StoreChannel {
 
 	count, err := s.GetMaster().Update(team)
 	if err != nil {
-			result.Err = model.NewAppError("SqlTeamStore.Update", "store.sql_team.update.updating.app_error", nil, "id="+team.Id+", "+err.Error(), http.StatusInternalServerError)
-			return
+		return nil, model.NewAppError("SqlTeamStore.Update", "store.sql_team.update.updating.app_error", nil, "id="+team.Id+", "+err.Error(), http.StatusInternalServerError)
 	}
 	if count != 1 {
-			result.Err = model.NewAppError("SqlTeamStore.Update", "store.sql_team.update.app_error", nil, "id="+team.Id, http.StatusInternalServerError)
-			return
+		return nil, model.NewAppError("SqlTeamStore.Update", "store.sql_team.update.app_error", nil, "id="+team.Id, http.StatusInternalServerError)
 	}
 
-		result.Data = team
-	})
+	return team, nil
 }
 
 func (s SqlTeamStore) UpdateDisplayName(name string, teamId string) store.StoreChannel {

store/store.go

@@ -82,7 +82,7 @@ type Store interface {
 
 type TeamStore interface {
 	Save(team *model.Team) StoreChannel
-	Update(team *model.Team) StoreChannel
+	Update(team *model.Team) (*model.Team, *model.AppError)
 	UpdateDisplayName(name string, teamId string) StoreChannel
 	Get(id string) StoreChannel
 	GetByName(name string) StoreChannel

store/storetest/group_supplier.go

@@ -1011,16 +1011,16 @@ func testPendingAutoAddTeamMembers(t *testing.T, ss store.Store) {
 
 	// No result if Team deleted
 	team.DeleteAt = model.GetMillis()
-	res = <-ss.Team().Update(team)
-	require.Nil(t, res.Err)
+	team, err := ss.Team().Update(team)
+	require.Nil(t, err)
 	res = <-ss.Group().TeamMembersToAdd(0)
 	require.Nil(t, res.Err)
 	require.Len(t, res.Data, 0)
 
 	// reset state of team and verify
 	team.DeleteAt = 0
-	res = <-ss.Team().Update(team)
-	require.Nil(t, res.Err)
+	team, err = ss.Team().Update(team)
+	require.Nil(t, err)
 	res = <-ss.Group().TeamMembersToAdd(0)
 	require.Nil(t, res.Err)
 	require.Len(t, res.Data, 1)

store/storetest/mocks/TeamStore.go

@@ -606,19 +606,28 @@ func (_m *TeamStore) SearchPrivate(term string) store.StoreChannel {
 }
 
 // Update provides a mock function with given fields: team
-func (_m *TeamStore) Update(team *model.Team) store.StoreChannel {
+func (_m *TeamStore) Update(team *model.Team) (*model.Team, *model.AppError) {
 	ret := _m.Called(team)
 
-	var r0 store.StoreChannel
-	if rf, ok := ret.Get(0).(func(*model.Team) store.StoreChannel); ok {
+	var r0 *model.Team
+	if rf, ok := ret.Get(0).(func(*model.Team) *model.Team); ok {
 		r0 = rf(team)
 	} else {
 		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(store.StoreChannel)
+			r0 = ret.Get(0).(*model.Team)
 		}
 	}
 
-	return r0
+	var r1 *model.AppError
+	if rf, ok := ret.Get(1).(func(*model.Team) *model.AppError); ok {
+		r1 = rf(team)
+	} else {
+		if ret.Get(1) != nil {
+			r1 = ret.Get(1).(*model.AppError)
+		}
+	}
+
+	return r0, r1
 }
 
 // UpdateDisplayName provides a mock function with given fields: name, teamId

store/storetest/team_store.go

@@ -27,7 +27,7 @@ func TestTeamStore(t *testing.T, ss store.Store) {
 	t.Run("SearchAll", func(t *testing.T) { testTeamStoreSearchAll(t, ss) })
 	t.Run("SearchOpen", func(t *testing.T) { testTeamStoreSearchOpen(t, ss) })
 	t.Run("SearchPrivate", func(t *testing.T) { testTeamStoreSearchPrivate(t, ss) })
-	t.Run("GetByIniviteId", func(t *testing.T) { testTeamStoreGetByIniviteId(t, ss) })
+	t.Run("GetByInviteId", func(t *testing.T) { testTeamStoreGetByInviteId(t, ss) })
 	t.Run("ByUserId", func(t *testing.T) { testTeamStoreByUserId(t, ss) })
 	t.Run("GetAllTeamListing", func(t *testing.T) { testGetAllTeamListing(t, ss) })
 	t.Run("GetAllTeamPageListing", func(t *testing.T) { testGetAllTeamPageListing(t, ss) })
@@ -86,17 +86,17 @@ func testTeamStoreUpdate(t *testing.T, ss store.Store) {
 
 	time.Sleep(100 * time.Millisecond)
 
-	if err := (<-ss.Team().Update(&o1)).Err; err != nil {
+	if _, err := ss.Team().Update(&o1); err != nil {
 		t.Fatal(err)
 	}
 
 	o1.Id = "missing"
-	if err := (<-ss.Team().Update(&o1)).Err; err == nil {
+	if _, err := ss.Team().Update(&o1); err == nil {
 		t.Fatal("Update should have failed because of missing key")
 	}
 
 	o1.Id = model.NewId()
-	if err := (<-ss.Team().Update(&o1)).Err; err == nil {
+	if _, err := ss.Team().Update(&o1); err == nil {
 		t.Fatal("Update should have faile because id change")
 	}
 }
@@ -385,7 +385,7 @@ func testTeamStoreSearchPrivate(t *testing.T, ss store.Store) {
 	}
 }
 
-func testTeamStoreGetByIniviteId(t *testing.T, ss store.Store) {
+func testTeamStoreGetByInviteId(t *testing.T, ss store.Store) {
 	o1 := model.Team{}
 	o1.DisplayName = "DisplayName"
 	o1.Name = "z-z-z" + model.NewId() + "b"
@@ -416,7 +416,8 @@ func testTeamStoreGetByIniviteId(t *testing.T, ss store.Store) {
 	}
 
 	o2.InviteId = ""
-	<-ss.Team().Update(&o2)
+	_, err := ss.Team().Update(&o2)
+	require.Nil(t, err)
 
 	if r1 := <-ss.Team().GetByInviteId(o2.Id); r1.Err != nil {
 		t.Fatal(r1.Err)

store/storetest/user_store.go

@@ -3513,8 +3513,8 @@ func testUserStoreGetTeamGroupUsers(t *testing.T, ss store.Store) {
 
 	// update team to be group-constrained
 	team.GroupConstrained = model.NewBool(true)
-	res = <-ss.Team().Update(team)
-	require.Nil(t, res.Err)
+	team, err := ss.Team().Update(team)
+	require.Nil(t, err)
 
 	// still returns user (being group-constrained has no effect)
 	requireNUsers(1)