From 0f0ebbd9d12a785d5e2ccf448fe6d8bac958f070 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Villablanca=20V=C3=A1squez?= Date: Fri, 14 Jun 2019 14:32:24 -0400 Subject: [PATCH] Migrate `Team.UpdateMember` to Sync by default (#11140) * Team.UpdateMember is sync by default * Fix go vet: declaration of err shadows declaration at line 261 * Fix shadowing variables * fix undefined err * fix shadowing variables --- api4/apitestlib.go | 8 ++--- app/team.go | 29 ++++++++------- store/sqlstore/team_store.go | 58 ++++++++++++++---------------- store/store.go | 2 +- store/storetest/mocks/TeamStore.go | 19 +++++++--- store/storetest/team_store.go | 6 ++-- 6 files changed, 63 insertions(+), 59 deletions(-) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 3ab9785e20..f42cb76c92 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -767,9 +767,9 @@ func (me *TestHelper) UpdateUserToTeamAdmin(user *model.User, team *model.Team) if tm, err := me.App.Srv.Store.Team().GetMember(team.Id, user.Id); err == nil { tm.SchemeAdmin = true - if sr := <-me.App.Srv.Store.Team().UpdateMember(tm); sr.Err != nil { + if _, err = me.App.Srv.Store.Team().UpdateMember(tm); err != nil { utils.EnableDebugLogForTest() - panic(sr.Err) + panic(err) } } else { utils.EnableDebugLogForTest() @@ -787,9 +787,9 @@ func (me *TestHelper) UpdateUserToNonTeamAdmin(user *model.User, team *model.Tea if tm, err := me.App.Srv.Store.Team().GetMember(team.Id, user.Id); err == nil { tm.SchemeAdmin = false - if sr := <-me.App.Srv.Store.Team().UpdateMember(tm); sr.Err != nil { + if _, err = me.App.Srv.Store.Team().UpdateMember(tm); err != nil { utils.EnableDebugLogForTest() - panic(sr.Err) + panic(err) } } else { utils.EnableDebugLogForTest() diff --git a/app/team.go b/app/team.go index 3bc30be8fa..9b3f46300e 100644 --- a/app/team.go +++ b/app/team.go @@ -270,7 +270,8 @@ func (a *App) UpdateTeamMemberRoles(teamId string, userId string, newRoles strin member.SchemeAdmin = false for _, roleName := range strings.Fields(newRoles) { - role, err := a.GetRoleByName(roleName) + var role *model.Role + role, err = a.GetRoleByName(roleName) if err != nil { err.StatusCode = http.StatusBadRequest return nil, err @@ -304,11 +305,10 @@ func (a *App) UpdateTeamMemberRoles(teamId string, userId string, newRoles strin member.ExplicitRoles = strings.Join(newExplicitRoles, " ") - result := <-a.Srv.Store.Team().UpdateMember(member) - if result.Err != nil { - return nil, result.Err + member, err = a.Srv.Store.Team().UpdateMember(member) + if err != nil { + return nil, err } - member = result.Data.(*model.TeamMember) a.ClearSessionCacheForUser(userId) @@ -336,11 +336,10 @@ func (a *App) UpdateTeamMemberSchemeRoles(teamId string, userId string, isScheme member.ExplicitRoles = RemoveRoles([]string{model.TEAM_GUEST_ROLE_ID, model.TEAM_USER_ROLE_ID, model.TEAM_ADMIN_ROLE_ID}, member.ExplicitRoles) } - result := <-a.Srv.Store.Team().UpdateMember(member) - if result.Err != nil { - return nil, result.Err + member, err = a.Srv.Store.Team().UpdateMember(member) + if err != nil { + return nil, err } - member = result.Data.(*model.TeamMember) a.ClearSessionCacheForUser(userId) @@ -532,12 +531,12 @@ func (a *App) joinUserToTeam(team *model.Team, user *model.User) (*model.TeamMem return nil, false, model.NewAppError("joinUserToTeam", "app.team.join_user_to_team.max_accounts.app_error", nil, "teamId="+tm.TeamId, http.StatusBadRequest) } - tmr := <-a.Srv.Store.Team().UpdateMember(tm) - if tmr.Err != nil { - return nil, false, tmr.Err + member, err := a.Srv.Store.Team().UpdateMember(tm) + if err != nil { + return nil, false, err } - return tmr.Data.(*model.TeamMember), false, nil + return member, false, nil } func (a *App) JoinUserToTeam(team *model.Team, user *model.User, userRequestorId string) *model.AppError { @@ -886,8 +885,8 @@ func (a *App) LeaveTeam(team *model.Team, user *model.User, requestorId string) teamMember.Roles = "" teamMember.DeleteAt = model.GetMillis() - if result := <-a.Srv.Store.Team().UpdateMember(teamMember); result.Err != nil { - return result.Err + if _, err := a.Srv.Store.Team().UpdateMember(teamMember); err != nil { + return err } if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil { diff --git a/store/sqlstore/team_store.go b/store/sqlstore/team_store.go index 8a3780175b..71081f54f9 100644 --- a/store/sqlstore/team_store.go +++ b/store/sqlstore/team_store.go @@ -553,41 +553,35 @@ func (s SqlTeamStore) SaveMember(member *model.TeamMember, maxUsersPerTeam int) }) } -func (s SqlTeamStore) UpdateMember(member *model.TeamMember) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - member.PreUpdate() +func (s SqlTeamStore) UpdateMember(member *model.TeamMember) (*model.TeamMember, *model.AppError) { + member.PreUpdate() - if result.Err = member.IsValid(); result.Err != nil { - return + if err := member.IsValid(); err != nil { + return nil, err + } + + if _, err := s.GetMaster().Update(NewTeamMemberFromModel(member)); err != nil { + return nil, model.NewAppError("SqlTeamStore.UpdateMember", "store.sql_team.save_member.save.app_error", nil, err.Error(), http.StatusInternalServerError) + } + + query := s.getTeamMembersWithSchemeSelectQuery(). + Where(sq.Eq{"TeamMembers.TeamId": member.TeamId}). + Where(sq.Eq{"TeamMembers.UserId": member.UserId}) + + queryString, args, err := query.ToSql() + if err != nil { + return nil, model.NewAppError("SqlTeamStore.UpdateMember", "store.sql_team.get_member.app_error", nil, err.Error(), http.StatusInternalServerError) + } + + var retrievedMember teamMemberWithSchemeRoles + if err := s.GetMaster().SelectOne(&retrievedMember, queryString, args...); err != nil { + if err == sql.ErrNoRows { + return nil, model.NewAppError("SqlTeamStore.UpdateMember", "store.sql_team.get_member.missing.app_error", nil, "team_id="+member.TeamId+"user_id="+member.UserId+","+err.Error(), http.StatusNotFound) } + return nil, model.NewAppError("SqlTeamStore.UpdateMember", "store.sql_team.get_member.app_error", nil, "team_id="+member.TeamId+"user_id="+member.UserId+","+err.Error(), http.StatusInternalServerError) + } - if _, err := s.GetMaster().Update(NewTeamMemberFromModel(member)); err != nil { - result.Err = model.NewAppError("SqlTeamStore.UpdateMember", "store.sql_team.save_member.save.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } - - query := s.getTeamMembersWithSchemeSelectQuery(). - Where(sq.Eq{"TeamMembers.TeamId": member.TeamId}). - Where(sq.Eq{"TeamMembers.UserId": member.UserId}) - - queryString, args, err := query.ToSql() - if err != nil { - result.Err = model.NewAppError("SqlTeamStore.UpdateMember", "store.sql_team.get_member.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } - - var retrievedMember teamMemberWithSchemeRoles - if err := s.GetMaster().SelectOne(&retrievedMember, queryString, args...); err != nil { - if err == sql.ErrNoRows { - result.Err = model.NewAppError("SqlTeamStore.UpdateMember", "store.sql_team.get_member.missing.app_error", nil, "team_id="+member.TeamId+"user_id="+member.UserId+","+err.Error(), http.StatusNotFound) - return - } - result.Err = model.NewAppError("SqlTeamStore.UpdateMember", "store.sql_team.get_member.app_error", nil, "team_id="+member.TeamId+"user_id="+member.UserId+","+err.Error(), http.StatusInternalServerError) - return - } - - result.Data = retrievedMember.ToModel() - }) + return retrievedMember.ToModel(), nil } func (s SqlTeamStore) GetMember(teamId string, userId string) (*model.TeamMember, *model.AppError) { diff --git a/store/store.go b/store/store.go index cfae448454..b19ad3278a 100644 --- a/store/store.go +++ b/store/store.go @@ -101,7 +101,7 @@ type TeamStore interface { PermanentDelete(teamId string) StoreChannel AnalyticsTeamCount() (int64, *model.AppError) SaveMember(member *model.TeamMember, maxUsersPerTeam int) StoreChannel - UpdateMember(member *model.TeamMember) StoreChannel + UpdateMember(member *model.TeamMember) (*model.TeamMember, *model.AppError) GetMember(teamId string, userId string) (*model.TeamMember, *model.AppError) GetMembers(teamId string, offset int, limit int, restrictions *model.ViewUsersRestrictions) ([]*model.TeamMember, *model.AppError) GetMembersByIds(teamId string, userIds []string, restrictions *model.ViewUsersRestrictions) ([]*model.TeamMember, *model.AppError) diff --git a/store/storetest/mocks/TeamStore.go b/store/storetest/mocks/TeamStore.go index ed84790d3e..aaae14db1d 100644 --- a/store/storetest/mocks/TeamStore.go +++ b/store/storetest/mocks/TeamStore.go @@ -818,19 +818,28 @@ func (_m *TeamStore) UpdateLastTeamIconUpdate(teamId string, curTime int64) stor } // UpdateMember provides a mock function with given fields: member -func (_m *TeamStore) UpdateMember(member *model.TeamMember) store.StoreChannel { +func (_m *TeamStore) UpdateMember(member *model.TeamMember) (*model.TeamMember, *model.AppError) { ret := _m.Called(member) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(*model.TeamMember) store.StoreChannel); ok { + var r0 *model.TeamMember + if rf, ok := ret.Get(0).(func(*model.TeamMember) *model.TeamMember); ok { r0 = rf(member) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.TeamMember) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(*model.TeamMember) *model.AppError); ok { + r1 = rf(member) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // UserBelongsToTeams provides a mock function with given fields: userId, teamIds diff --git a/store/storetest/team_store.go b/store/storetest/team_store.go index 60adfa989f..a1af56f0aa 100644 --- a/store/storetest/team_store.go +++ b/store/storetest/team_store.go @@ -985,11 +985,13 @@ func testSaveTeamMemberMaxMembers(t *testing.T, ss store.Store) { } // Leaving the team from the UI sets DeleteAt instead of using TeamStore.RemoveMember - store.Must(ss.Team().UpdateMember(&model.TeamMember{ + if _, err := ss.Team().UpdateMember(&model.TeamMember{ TeamId: team.Id, UserId: userIds[0], DeleteAt: 1234, - })) + }); err != nil { + panic(err) + } if totalMemberCount, err := ss.Team().GetTotalMemberCount(team.Id); err != nil { t.Fatal(err)