From ab4eaf898ba2c91d87197d2980f1eca7bf17754f Mon Sep 17 00:00:00 2001 From: Martin Kraft Date: Fri, 28 Jun 2019 07:31:32 -0400 Subject: [PATCH] Changes method name, removes error case. (#11427) * Changes method name, removes previous error case. * Fixes test now that the method is idempotent. --- api4/channel_test.go | 6 ++-- api4/team_test.go | 8 ++--- app/group.go | 4 +-- app/group_test.go | 10 +++--- app/syncables_test.go | 6 ++-- store/sqlstore/group_store.go | 7 +--- store/store.go | 2 +- store/storetest/group_store.go | 56 ++++++++++++++--------------- store/storetest/mocks/GroupStore.go | 32 ++++++++--------- store/storetest/user_store.go | 12 +++---- 10 files changed, 69 insertions(+), 74 deletions(-) diff --git a/api4/channel_test.go b/api4/channel_test.go index fbb1ddff26..61f46817cf 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -2178,7 +2178,7 @@ func TestAddChannelMember(t *testing.T) { require.Nil(t, appErr) // Add user to group - _, appErr = th.App.CreateOrRestoreGroupMember(th.Group.Id, user.Id) + _, appErr = th.App.UpsertGroupMember(th.Group.Id, user.Id) require.Nil(t, appErr) _, resp = th.SystemAdminClient.AddChannelMember(privateChannel.Id, user.Id) @@ -2748,9 +2748,9 @@ func TestChannelMembersMinusGroupMembers(t *testing.T) { group1 := th.CreateGroup() group2 := th.CreateGroup() - _, err = th.App.CreateOrRestoreGroupMember(group1.Id, user1.Id) + _, err = th.App.UpsertGroupMember(group1.Id, user1.Id) require.Nil(t, err) - _, err = th.App.CreateOrRestoreGroupMember(group2.Id, user2.Id) + _, err = th.App.UpsertGroupMember(group2.Id, user2.Id) require.Nil(t, err) // No permissions diff --git a/api4/team_test.go b/api4/team_test.go index 18bed8e11a..267b4caf21 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -1540,7 +1540,7 @@ func TestAddTeamMember(t *testing.T) { require.Nil(t, err) // Add user to group - _, err = th.App.CreateOrRestoreGroupMember(th.Group.Id, otherUser.Id) + _, err = th.App.UpsertGroupMember(th.Group.Id, otherUser.Id) require.Nil(t, err) _, resp = th.SystemAdminClient.AddTeamMember(team.Id, otherUser.Id) @@ -1755,7 +1755,7 @@ func TestAddTeamMembers(t *testing.T) { require.Nil(t, err) // Add user to group - _, err = th.App.CreateOrRestoreGroupMember(th.Group.Id, userList[0]) + _, err = th.App.UpsertGroupMember(th.Group.Id, userList[0]) require.Nil(t, err) _, resp = Client.AddTeamMembers(team.Id, userList) @@ -2525,9 +2525,9 @@ func TestTeamMembersMinusGroupMembers(t *testing.T) { group1 := th.CreateGroup() group2 := th.CreateGroup() - _, err = th.App.CreateOrRestoreGroupMember(group1.Id, user1.Id) + _, err = th.App.UpsertGroupMember(group1.Id, user1.Id) require.Nil(t, err) - _, err = th.App.CreateOrRestoreGroupMember(group2.Id, user2.Id) + _, err = th.App.UpsertGroupMember(group2.Id, user2.Id) require.Nil(t, err) // No permissions diff --git a/app/group.go b/app/group.go index 588c913e23..33f58a0925 100644 --- a/app/group.go +++ b/app/group.go @@ -79,8 +79,8 @@ func (a *App) GetGroupMemberUsersPage(groupID string, page int, perPage int) ([] return members, count, nil } -func (a *App) CreateOrRestoreGroupMember(groupID string, userID string) (*model.GroupMember, *model.AppError) { - result := <-a.Srv.Store.Group().CreateOrRestoreMember(groupID, userID) +func (a *App) UpsertGroupMember(groupID string, userID string) (*model.GroupMember, *model.AppError) { + result := <-a.Srv.Store.Group().UpsertMember(groupID, userID) if result.Err != nil { return nil, result.Err } diff --git a/app/group_test.go b/app/group_test.go index f4192a5206..e716963f51 100644 --- a/app/group_test.go +++ b/app/group_test.go @@ -105,20 +105,20 @@ func TestCreateOrRestoreGroupMember(t *testing.T) { defer th.TearDown() group := th.CreateGroup() - g, err := th.App.CreateOrRestoreGroupMember(group.Id, th.BasicUser.Id) + g, err := th.App.UpsertGroupMember(group.Id, th.BasicUser.Id) require.Nil(t, err) require.NotNil(t, g) - g, err = th.App.CreateOrRestoreGroupMember(group.Id, th.BasicUser.Id) - require.NotNil(t, err) - require.Nil(t, g) + g, err = th.App.UpsertGroupMember(group.Id, th.BasicUser.Id) + require.Nil(t, err) + require.NotNil(t, g) } func TestDeleteGroupMember(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() group := th.CreateGroup() - groupMember, err := th.App.CreateOrRestoreGroupMember(group.Id, th.BasicUser.Id) + groupMember, err := th.App.UpsertGroupMember(group.Id, th.BasicUser.Id) require.Nil(t, err) require.NotNil(t, groupMember) diff --git a/app/syncables_test.go b/app/syncables_test.go index 3a2e104ccb..b6d820fd8f 100644 --- a/app/syncables_test.go +++ b/app/syncables_test.go @@ -89,12 +89,12 @@ func TestCreateDefaultMemberships(t *testing.T) { singer1 := th.BasicUser scientist1 := th.BasicUser2 - _, err = th.App.CreateOrRestoreGroupMember(gleeGroup.Id, singer1.Id) + _, err = th.App.UpsertGroupMember(gleeGroup.Id, singer1.Id) if err != nil { t.Errorf("test groupmember not created: %s", err.Error()) } - scientistGroupMember, err := th.App.CreateOrRestoreGroupMember(scienceGroup.Id, scientist1.Id) + scientistGroupMember, err := th.App.UpsertGroupMember(scienceGroup.Id, scientist1.Id) if err != nil { t.Errorf("test groupmember not created: %s", err.Error()) } @@ -375,7 +375,7 @@ func TestDeleteGroupMemberships(t *testing.T) { require.Equal(t, 3, int(cmemberCount)) // add a user to the group - _, err = th.App.CreateOrRestoreGroupMember(group.Id, th.SystemAdminUser.Id) + _, err = th.App.UpsertGroupMember(group.Id, th.SystemAdminUser.Id) require.Nil(t, err) // run the delete diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index 83e89c6dff..cb9c78d0b9 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -335,7 +335,7 @@ func (s *SqlGroupStore) GetMemberCount(groupID string) store.StoreChannel { }) } -func (s *SqlGroupStore) CreateOrRestoreMember(groupID string, userID string) store.StoreChannel { +func (s *SqlGroupStore) UpsertMember(groupID string, userID string) store.StoreChannel { return store.Do(func(result *store.StoreResult) { member := &model.GroupMember{ @@ -362,11 +362,6 @@ func (s *SqlGroupStore) CreateOrRestoreMember(groupID string, userID string) sto } } - if retrievedMember != nil && retrievedMember.DeleteAt == 0 { - result.Err = model.NewAppError("SqlGroupStore.GroupCreateOrRestoreMember", "store.sql_group.uniqueness_error", nil, "group_id="+member.GroupId+", user_id="+member.UserId, http.StatusBadRequest) - return - } - if retrievedMember == nil { if err := s.GetMaster().Insert(member); err != nil { if IsUniqueConstraintError(err, []string{"GroupId", "UserId", "groupmembers_pkey", "PRIMARY"}) { diff --git a/store/store.go b/store/store.go index ea2ab4f969..d00bcbfe49 100644 --- a/store/store.go +++ b/store/store.go @@ -583,7 +583,7 @@ type GroupStore interface { GetMemberUsers(groupID string) StoreChannel GetMemberUsersPage(groupID string, offset int, limit int) StoreChannel GetMemberCount(groupID string) StoreChannel - CreateOrRestoreMember(groupID string, userID string) StoreChannel + UpsertMember(groupID string, userID string) StoreChannel DeleteMember(groupID string, userID string) StoreChannel CreateGroupSyncable(groupSyncable *model.GroupSyncable) (*model.GroupSyncable, *model.AppError) diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index 0b15e3b4f6..0526cc99fe 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -26,7 +26,7 @@ func TestGroupStore(t *testing.T, ss store.Store) { t.Run("GetMemberUsers", func(t *testing.T) { testGroupGetMemberUsers(t, ss) }) t.Run("GetMemberUsersPage", func(t *testing.T) { testGroupGetMemberUsersPage(t, ss) }) - t.Run("CreateOrRestoreMember", func(t *testing.T) { testGroupCreateOrRestoreMember(t, ss) }) + t.Run("UpsertMember", func(t *testing.T) { testGroupCreateOrRestoreMember(t, ss) }) t.Run("DeleteMember", func(t *testing.T) { testGroupDeleteMember(t, ss) }) t.Run("CreateGroupSyncable", func(t *testing.T) { testCreateGroupSyncable(t, ss) }) @@ -457,7 +457,7 @@ func testGroupGetMemberUsers(t *testing.T, ss store.Store) { require.Nil(t, res.Err) user1 := res.Data.(*model.User) - res = <-ss.Group().CreateOrRestoreMember(group.Id, user1.Id) + res = <-ss.Group().UpsertMember(group.Id, user1.Id) require.Nil(t, res.Err) u2 := &model.User{ @@ -468,7 +468,7 @@ func testGroupGetMemberUsers(t *testing.T, ss store.Store) { require.Nil(t, res.Err) user2 := res.Data.(*model.User) - res = <-ss.Group().CreateOrRestoreMember(group.Id, user2.Id) + res = <-ss.Group().UpsertMember(group.Id, user2.Id) require.Nil(t, res.Err) // Check returns members @@ -511,7 +511,7 @@ func testGroupGetMemberUsersPage(t *testing.T, ss store.Store) { require.Nil(t, res.Err) user1 := res.Data.(*model.User) - res = <-ss.Group().CreateOrRestoreMember(group.Id, user1.Id) + res = <-ss.Group().UpsertMember(group.Id, user1.Id) require.Nil(t, res.Err) u2 := &model.User{ @@ -522,7 +522,7 @@ func testGroupGetMemberUsersPage(t *testing.T, ss store.Store) { require.Nil(t, res.Err) user2 := res.Data.(*model.User) - res = <-ss.Group().CreateOrRestoreMember(group.Id, user2.Id) + res = <-ss.Group().UpsertMember(group.Id, user2.Id) require.Nil(t, res.Err) // Check returns members @@ -580,7 +580,7 @@ func testGroupCreateOrRestoreMember(t *testing.T, ss store.Store) { user := res2.Data.(*model.User) // Happy path - res3 := <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) + res3 := <-ss.Group().UpsertMember(group.Id, user.Id) require.Nil(t, res3.Err) d2 := res3.Data.(*model.GroupMember) require.Equal(t, d2.GroupId, group.Id) @@ -589,16 +589,16 @@ func testGroupCreateOrRestoreMember(t *testing.T, ss store.Store) { require.Zero(t, d2.DeleteAt) // Duplicate composite key (GroupId, UserId) - res4 := <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) - require.Equal(t, res4.Err.Id, "store.sql_group.uniqueness_error") + res4 := <-ss.Group().UpsertMember(group.Id, user.Id) + require.Nil(t, res4.Err) // Invalid GroupId - res6 := <-ss.Group().CreateOrRestoreMember(model.NewId(), user.Id) + res6 := <-ss.Group().UpsertMember(model.NewId(), user.Id) require.Equal(t, res6.Err.Id, "store.insert_error") // Restores a deleted member - res := <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) - require.NotNil(t, res.Err) + res := <-ss.Group().UpsertMember(group.Id, user.Id) + require.Nil(t, res.Err) res = <-ss.Group().DeleteMember(group.Id, user.Id) require.Nil(t, res.Err) @@ -606,7 +606,7 @@ func testGroupCreateOrRestoreMember(t *testing.T, ss store.Store) { res = <-ss.Group().GetMemberUsers(group.Id) beforeRestoreCount := len(res.Data.([]*model.User)) - res = <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) + res = <-ss.Group().UpsertMember(group.Id, user.Id) require.Nil(t, res.Err) res = <-ss.Group().GetMemberUsers(group.Id) @@ -637,7 +637,7 @@ func testGroupDeleteMember(t *testing.T, ss store.Store) { user := res2.Data.(*model.User) // Create member - res3 := <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) + res3 := <-ss.Group().UpsertMember(group.Id, user.Id) require.Nil(t, res3.Err) d1 := res3.Data.(*model.GroupMember) @@ -945,7 +945,7 @@ func testPendingAutoAddTeamMembers(t *testing.T, ss store.Store) { user = res.Data.(*model.User) // Create GroupMember - res = <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) + res = <-ss.Group().UpsertMember(group.Id, user.Id) require.Nil(t, res.Err) // Create Team @@ -981,7 +981,7 @@ func testPendingAutoAddTeamMembers(t *testing.T, ss store.Store) { // Delete and restore GroupMember should return result res = <-ss.Group().DeleteMember(group.Id, user.Id) require.Nil(t, res.Err) - res = <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) + res = <-ss.Group().UpsertMember(group.Id, user.Id) require.Nil(t, res.Err) teamMembers, err = ss.Group().TeamMembersToAdd(syncable.CreateAt + 1) require.Nil(t, err) @@ -1071,7 +1071,7 @@ func testPendingAutoAddTeamMembers(t *testing.T, ss store.Store) { require.Len(t, teamMembers, 0) // restore group member and verify - res = <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) + res = <-ss.Group().UpsertMember(group.Id, user.Id) teamMembers, err = ss.Group().TeamMembersToAdd(0) require.Nil(t, err) require.Len(t, teamMembers, 1) @@ -1108,7 +1108,7 @@ func testPendingAutoAddChannelMembers(t *testing.T, ss store.Store) { user = res.Data.(*model.User) // Create GroupMember - res = <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) + res = <-ss.Group().UpsertMember(group.Id, user.Id) require.Nil(t, res.Err) // Create Channel @@ -1140,7 +1140,7 @@ func testPendingAutoAddChannelMembers(t *testing.T, ss store.Store) { // Delete and restore GroupMember should return result res = <-ss.Group().DeleteMember(group.Id, user.Id) require.Nil(t, res.Err) - res = <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) + res = <-ss.Group().UpsertMember(group.Id, user.Id) require.Nil(t, res.Err) channelMembers, err = ss.Group().ChannelMembersToAdd(syncable.CreateAt + 1) require.Nil(t, err) @@ -1229,7 +1229,7 @@ func testPendingAutoAddChannelMembers(t *testing.T, ss store.Store) { require.Len(t, channelMembers, 0) // restore group member and verify - res = <-ss.Group().CreateOrRestoreMember(group.Id, user.Id) + res = <-ss.Group().UpsertMember(group.Id, user.Id) require.Nil(t, res.Err) channelMembers, err = ss.Group().ChannelMembersToAdd(0) require.Nil(t, err) @@ -1458,10 +1458,10 @@ func pendingMemberRemovalsDataSetup(t *testing.T, ss store.Store) *removalsData userC = res.Data.(*model.User) // add users to group (but not userC) - res = <-ss.Group().CreateOrRestoreMember(group.Id, userA.Id) + res = <-ss.Group().UpsertMember(group.Id, userA.Id) require.Nil(t, res.Err) - res = <-ss.Group().CreateOrRestoreMember(group.Id, userB.Id) + res = <-ss.Group().UpsertMember(group.Id, userB.Id) require.Nil(t, res.Err) // create channels @@ -1653,7 +1653,7 @@ func testGetGroupsByChannel(t *testing.T, ss store.Store) { res = <-ss.User().Save(u1) require.Nil(t, res.Err) user1 := res.Data.(*model.User) - <-ss.Group().CreateOrRestoreMember(group1.Id, user1.Id) + <-ss.Group().UpsertMember(group1.Id, user1.Id) group1WithMemberCount := model.Group(*group1) group1WithMemberCount.MemberCount = model.NewInt(1) @@ -1854,7 +1854,7 @@ func testGetGroupsByTeam(t *testing.T, ss store.Store) { res = <-ss.User().Save(u1) require.Nil(t, res.Err) user1 := res.Data.(*model.User) - <-ss.Group().CreateOrRestoreMember(group1.Id, user1.Id) + <-ss.Group().UpsertMember(group1.Id, user1.Id) group1WithMemberCount := model.Group(*group1) group1WithMemberCount.MemberCount = model.NewInt(1) @@ -2096,7 +2096,7 @@ func testGetGroups(t *testing.T, ss store.Store) { res = <-ss.User().Save(u1) require.Nil(t, res.Err) user1 := res.Data.(*model.User) - <-ss.Group().CreateOrRestoreMember(group1.Id, user1.Id) + <-ss.Group().UpsertMember(group1.Id, user1.Id) group1WithMemberCount := model.Group(*group1) group1WithMemberCount.MemberCount = model.NewInt(1) @@ -2299,11 +2299,11 @@ func testTeamMembersMinusGroupMembers(t *testing.T, ss store.Store) { // Add even users to even group, and the inverse for i := 0; i < numberOfUsers; i++ { groupIndex := int(math.Mod(float64(i), 2)) - res := <-ss.Group().CreateOrRestoreMember(groups[groupIndex].Id, users[i].Id) + res := <-ss.Group().UpsertMember(groups[groupIndex].Id, users[i].Id) require.Nil(t, res.Err) // Add everyone to group 2 - res = <-ss.Group().CreateOrRestoreMember(groups[numberOfGroups-1].Id, users[i].Id) + res = <-ss.Group().UpsertMember(groups[numberOfGroups-1].Id, users[i].Id) require.Nil(t, res.Err) } @@ -2453,11 +2453,11 @@ func testChannelMembersMinusGroupMembers(t *testing.T, ss store.Store) { // Add even users to even group, and the inverse for i := 0; i < numberOfUsers; i++ { groupIndex := int(math.Mod(float64(i), 2)) - res := <-ss.Group().CreateOrRestoreMember(groups[groupIndex].Id, users[i].Id) + res := <-ss.Group().UpsertMember(groups[groupIndex].Id, users[i].Id) require.Nil(t, res.Err) // Add everyone to group 2 - res = <-ss.Group().CreateOrRestoreMember(groups[numberOfGroups-1].Id, users[i].Id) + res = <-ss.Group().UpsertMember(groups[numberOfGroups-1].Id, users[i].Id) require.Nil(t, res.Err) } diff --git a/store/storetest/mocks/GroupStore.go b/store/storetest/mocks/GroupStore.go index ecb5df6605..e7fab98543 100644 --- a/store/storetest/mocks/GroupStore.go +++ b/store/storetest/mocks/GroupStore.go @@ -221,22 +221,6 @@ func (_m *GroupStore) CreateGroupSyncable(groupSyncable *model.GroupSyncable) (* return r0, r1 } -// CreateOrRestoreMember provides a mock function with given fields: groupID, userID -func (_m *GroupStore) CreateOrRestoreMember(groupID string, userID string) store.StoreChannel { - ret := _m.Called(groupID, userID) - - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, string) store.StoreChannel); ok { - r0 = rf(groupID, userID) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) - } - } - - return r0 -} - // Delete provides a mock function with given fields: groupID func (_m *GroupStore) Delete(groupID string) store.StoreChannel { ret := _m.Called(groupID) @@ -655,3 +639,19 @@ func (_m *GroupStore) UpdateGroupSyncable(groupSyncable *model.GroupSyncable) (* return r0, r1 } + +// UpsertMember provides a mock function with given fields: groupID, userID +func (_m *GroupStore) UpsertMember(groupID string, userID string) store.StoreChannel { + ret := _m.Called(groupID, userID) + + var r0 store.StoreChannel + if rf, ok := ret.Get(0).(func(string, string) store.StoreChannel); ok { + r0 = rf(groupID, userID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(store.StoreChannel) + } + } + + return r0 +} diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 21c5d8a4ab..35b2d4860d 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -1134,7 +1134,7 @@ func testUserStoreGetProfilesNotInChannel(t *testing.T, ss store.Store) { // add two members to the group for _, u := range []*model.User{u1, u2} { - res := <-ss.Group().CreateOrRestoreMember(group.Id, u.Id) + res := <-ss.Group().UpsertMember(group.Id, u.Id) require.Nil(t, res.Err) } @@ -3467,7 +3467,7 @@ func testUserStoreGetProfilesNotInTeam(t *testing.T, ss store.Store) { // add two members to the group for _, u := range []*model.User{u1, u2} { - res := <-ss.Group().CreateOrRestoreMember(group.Id, u.Id) + res := <-ss.Group().UpsertMember(group.Id, u.Id) require.Nil(t, res.Err) } @@ -3773,9 +3773,9 @@ func testUserStoreGetTeamGroupUsers(t *testing.T, ss store.Store) { groupB := testGroups[1] // add members to groups - res = <-ss.Group().CreateOrRestoreMember(groupA.Id, userGroupA.Id) + res = <-ss.Group().UpsertMember(groupA.Id, userGroupA.Id) require.Nil(t, res.Err) - res = <-ss.Group().CreateOrRestoreMember(groupB.Id, userGroupB.Id) + res = <-ss.Group().UpsertMember(groupB.Id, userGroupB.Id) require.Nil(t, res.Err) // association one group to team @@ -3895,9 +3895,9 @@ func testUserStoreGetChannelGroupUsers(t *testing.T, ss store.Store) { groupB := testGroups[1] // add members to groups - res = <-ss.Group().CreateOrRestoreMember(groupA.Id, userGroupA.Id) + res = <-ss.Group().UpsertMember(groupA.Id, userGroupA.Id) require.Nil(t, res.Err) - res = <-ss.Group().CreateOrRestoreMember(groupB.Id, userGroupB.Id) + res = <-ss.Group().UpsertMember(groupB.Id, userGroupB.Id) require.Nil(t, res.Err) // association one group to channel