diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 4486ae8b32..f50a13beea 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -1348,23 +1348,25 @@ func (us SqlUserStore) GetProfilesNotInTeam(teamId string, offset int, limit int func (us SqlUserStore) GetEtagForProfilesNotInTeam(teamId string) store.StoreChannel { return store.Do(func(result *store.StoreResult) { - updateAt, err := us.GetReplica().SelectInt(` - SELECT - u.UpdateAt - FROM Users u - LEFT JOIN TeamMembers tm - ON tm.UserId = u.Id - AND tm.TeamId = :TeamId - AND tm.DeleteAt = 0 - WHERE tm.UserId IS NULL - ORDER BY u.UpdateAt DESC - LIMIT 1 - `, map[string]interface{}{"TeamId": teamId}) + var querystr string + querystr = ` + SELECT + CONCAT(MAX(UpdateAt), '.', COUNT(Id)) as etag + FROM + Users as u + LEFT JOIN TeamMembers tm + ON tm.UserId = u.Id + AND tm.TeamId = :TeamId + AND tm.DeleteAt = 0 + WHERE + tm.UserId IS NULL + ` + etag, err := us.GetReplica().SelectStr(querystr, map[string]interface{}{"TeamId": teamId}) if err != nil { result.Data = fmt.Sprintf("%v.%v", model.CurrentVersion, model.GetMillis()) } else { - result.Data = fmt.Sprintf("%v.%v", model.CurrentVersion, updateAt) + result.Data = fmt.Sprintf("%v.%v", model.CurrentVersion, etag) } }) } diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 9910aba292..3138ceffb5 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -2891,10 +2891,7 @@ func testUserStoreGetProfilesNotInTeam(t *testing.T, ss store.Store) { store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u2.Id}, -1)) u2.UpdateAt = store.Must(ss.User().UpdateUpdateAt(u2.Id)).(int64) - // GetEtagForProfilesNotInTeam only works if the most recent user is added to the team, - // otherwise the timestamp simply never changes: see https://mattermost.atlassian.net/browse/MM-13721. t.Run("etag for profiles not in team 1 after update", func(t *testing.T) { - t.Skip() result := <-ss.User().GetEtagForProfilesNotInTeam(teamId) require.Nil(t, result.Err) etag2 = result.Data.(string) @@ -2938,9 +2935,10 @@ func testUserStoreGetProfilesNotInTeam(t *testing.T, ss store.Store) { // Ensure update at timestamp changes time.Sleep(time.Millisecond * 10) - u4 := &model.User{} - u4.Email = MakeEmail() - store.Must(ss.User().Save(u4)) + u4 := store.Must(ss.User().Save(&model.User{ + Email: MakeEmail(), + Username: "u4" + model.NewId(), + })).(*model.User) defer func() { store.Must(ss.User().PermanentDelete(u4.Id)) }() store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u4.Id}, -1)) @@ -2950,6 +2948,23 @@ func testUserStoreGetProfilesNotInTeam(t *testing.T, ss store.Store) { etag4 := result.Data.(string) require.Equal(t, etag3, etag4, "etag should not have changed") }) + + // Add u3 to team 2 + store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: teamId2, UserId: u3.Id}, -1)) + u3.UpdateAt = store.Must(ss.User().UpdateUpdateAt(u3.Id)).(int64) + + // GetEtagForProfilesNotInTeam produces a new etag every time a member, not + // in the team, gets a new UpdateAt value. In the case that an older member + // in the set joins a different team, their UpdateAt value changes, thus + // creating a new etag (even though the user set doesn't change). A hashing + // solution, which only uses UserIds, would solve this issue. + t.Run("etag for profiles not in team 1 after u3 added to team 2", func(t *testing.T) { + t.Skip() + result := <-ss.User().GetEtagForProfilesNotInTeam(teamId) + require.Nil(t, result.Err) + etag4 := result.Data.(string) + require.Equal(t, etag3, etag4, "etag should not have changed") + }) } func testUserStoreClearAllCustomRoleAssignments(t *testing.T, ss store.Store) {