[MM-13721] Fix the etag function GetEtagForProfilesNotInTeam (#10360)

* Initial solution for Draft PR

* Reformat tabs to spaces for readability

* Remove my comments and debugging lines for Core Commiter to see changes
more easily

* Remove all comments and show only code changes

* Match indentation spacing of new query to old query to make diff reading
easier for core committer

* Remove everything except what want to show core commiter

* Restrucure query and resulting etag value.
get number of users not in team from SELECT subquery.
etag return values is now of the form
<model.CurrenVersion>.<UpdateAt>.<number_profiles_not_int_team>

* Remove skipped test for solution to https://mattermost.atlassian.net/browse/MM-13721.
Remove comment for failing description of test
store u4 with prepended "u4".  Similar to u1, u2, u3.  This is easier
for debugging when looking in the database
Added skipped test:
  check that etag does not change when a user, not in team 1, is added
  to different team.  UpdateAt will change, but users in the set does
  not

* Remove skipped test for solution to https://mattermost.atlassian.net/browse/MM-13721.
Remove comment for failing description of test
store u4 with prepended "u4".  Similar to u1, u2, u3.  This is easier
for debugging when looking in the database
Added skipped test:
  check that etag does not change when a user, not in team 1, is added
  to different team.  UpdateAt will change, but users in the set does
  not

* remove commented out tests

* Restructure and simplify the SQL query for GetEtagForProfilesNotInTeam.
- Build the query to get all profiles not in a specified team
- select latest UpdateAt Value by getting Max value from UpdateAt field.
- select Number of profiles not in Team from count of the returned Ids

The previous query required building a complex query with multiple
joins and repeated code in a select subquery, and derived table

* Format SQL styling indentation, spaces around equal signs, and new lines

* Add description for skipped test

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)
 		}
 	})
 }

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) {