MM-16477 Add api to get users modified since a given time (#11406)

* MM-16477 Add api to get users modified since a given time

* Address feedback

api4/user.go

@@ -15,6 +15,7 @@ import (
 	"github.com/mattermost/mattermost-server/app"
 	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/model"
+	"github.com/mattermost/mattermost-server/store"
 	"github.com/mattermost/mattermost-server/utils"
 )
 
@@ -615,13 +616,29 @@ func getUsersByIds(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	sinceString := r.URL.Query().Get("since")
+
+	options := &store.UserGetByIdsOpts{
+		IsAdmin: c.IsSystemAdmin(),
+	}
+
+	if len(sinceString) > 0 {
+		since, parseError := strconv.ParseInt(sinceString, 10, 64)
+		if parseError != nil {
+			c.SetInvalidParam("since")
+			return
+		}
+		options.Since = since
+	}
+
 	restrictions, err := c.App.GetViewUsersRestrictions(c.App.Session.UserId)
 	if err != nil {
 		c.Err = err
 		return
 	}
+	options.ViewRestrictions = restrictions
 
-	users, err := c.App.GetUsersByIds(userIds, c.IsSystemAdmin(), restrictions)
+	users, err := c.App.GetUsersByIds(userIds, options)
 	if err != nil {
 		c.Err = err
 		return

api4/user_test.go

@@ -1099,32 +1099,71 @@ func TestGetUsersByIds(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 
-	users, resp := th.Client.GetUsersByIds([]string{th.BasicUser.Id})
+	t.Run("should return the user", func(t *testing.T) {
-	CheckNoError(t, resp)
+		users, resp := th.Client.GetUsersByIds([]string{th.BasicUser.Id})
 
-	if users[0].Id != th.BasicUser.Id {
+		CheckNoError(t, resp)
-		t.Fatal("returned wrong user")
-	}
-	CheckUserSanitization(t, users[0])
 
-	_, resp = th.Client.GetUsersByIds([]string{})
+		assert.Equal(t, th.BasicUser.Id, users[0].Id)
-	CheckBadRequestStatus(t, resp)
+		CheckUserSanitization(t, users[0])
+	})
 
-	users, resp = th.Client.GetUsersByIds([]string{"junk"})
+	t.Run("should return error when no IDs are specified", func(t *testing.T) {
-	CheckNoError(t, resp)
+		_, resp := th.Client.GetUsersByIds([]string{})
-	if len(users) > 0 {
-		t.Fatal("no users should be returned")
-	}
 
-	users, resp = th.Client.GetUsersByIds([]string{"junk", th.BasicUser.Id})
+		CheckBadRequestStatus(t, resp)
-	CheckNoError(t, resp)
+	})
-	if len(users) != 1 {
-		t.Fatal("1 user should be returned")
-	}
 
-	th.Client.Logout()
+	t.Run("should not return an error for invalid IDs", func(t *testing.T) {
-	_, resp = th.Client.GetUsersByIds([]string{th.BasicUser.Id})
+		users, resp := th.Client.GetUsersByIds([]string{"junk"})
-	CheckUnauthorizedStatus(t, resp)
+
+		CheckNoError(t, resp)
+		if len(users) > 0 {
+			t.Fatal("no users should be returned")
+		}
+	})
+
+	t.Run("should still return users for valid IDs when invalid IDs are specified", func(t *testing.T) {
+		users, resp := th.Client.GetUsersByIds([]string{"junk", th.BasicUser.Id})
+
+		CheckNoError(t, resp)
+		if len(users) != 1 {
+			t.Fatal("1 user should be returned")
+		}
+	})
+
+	t.Run("should return error when not logged in", func(t *testing.T) {
+		th.Client.Logout()
+
+		_, resp := th.Client.GetUsersByIds([]string{th.BasicUser.Id})
+		CheckUnauthorizedStatus(t, resp)
+	})
+}
+
+func TestGetUsersByIdsWithOptions(t *testing.T) {
+	t.Run("should only return specified users that have been updated since the given time", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
+
+		// Users before the timestamp shouldn't be returned
+		user1, err := th.App.CreateUser(&model.User{Email: th.GenerateTestEmail(), Username: model.NewId(), Password: model.NewId()})
+		require.Nil(t, err)
+
+		user2, err := th.App.CreateUser(&model.User{Email: th.GenerateTestEmail(), Username: model.NewId(), Password: model.NewId()})
+		require.Nil(t, err)
+
+		// Users not in the list of IDs shouldn't be returned
+		_, err = th.App.CreateUser(&model.User{Email: th.GenerateTestEmail(), Username: model.NewId(), Password: model.NewId()})
+		require.Nil(t, err)
+
+		users, resp := th.Client.GetUsersByIdsWithOptions([]string{user1.Id, user2.Id}, &model.UserGetByIdsOptions{
+			Since: user2.UpdateAt - 1,
+		})
+
+		assert.Nil(t, resp.Error)
+		assert.Len(t, users, 1)
+		assert.Equal(t, users[0].Id, user2.Id)
+	})
 }
 
 func TestGetUsersByGroupChannelIds(t *testing.T) {

app/channel.go

@@ -434,7 +434,7 @@ func (a *App) createGroupChannel(userIds []string, creatorId string) (*model.Cha
 		return nil, model.NewAppError("CreateGroupChannel", "api.channel.create_group.bad_size.app_error", nil, "", http.StatusBadRequest)
 	}
 
-	result := <-a.Srv.Store.User().GetProfileByIds(userIds, true, nil)
+	result := <-a.Srv.Store.User().GetProfileByIds(userIds, nil, true)
 	if result.Err != nil {
 		return nil, result.Err
 	}
@@ -483,7 +483,7 @@ func (a *App) GetGroupChannel(userIds []string) (*model.Channel, *model.AppError
 		return nil, model.NewAppError("GetGroupChannel", "api.channel.create_group.bad_size.app_error", nil, "", http.StatusBadRequest)
 	}
 
-	result := <-a.Srv.Store.User().GetProfileByIds(userIds, true, nil)
+	result := <-a.Srv.Store.User().GetProfileByIds(userIds, nil, true)
 	if result.Err != nil {
 		return nil, result.Err
 	}

app/slack.go

@@ -66,7 +66,7 @@ func replaceUserIds(userStore store.UserStore, text string) string {
 			userIds = append(userIds, match[1])
 		}
 
-		if res := <-userStore.GetProfileByIds(userIds, true, nil); res.Err == nil {
+		if res := <-userStore.GetProfileByIds(userIds, nil, true); res.Err == nil {
 			for _, user := range res.Data.([]*model.User) {
 				text = strings.Replace(text, "<@"+user.Id+">", "@"+user.Username, -1)
 			}

app/user.go

@@ -617,12 +617,15 @@ func (a *App) GetChannelGroupUsers(channelID string) ([]*model.User, *model.AppE
 	return a.Srv.Store.User().GetChannelGroupUsers(channelID)
 }
 
-func (a *App) GetUsersByIds(userIds []string, asAdmin bool, viewRestrictions *model.ViewUsersRestrictions) ([]*model.User, *model.AppError) {
+func (a *App) GetUsersByIds(userIds []string, options *store.UserGetByIdsOpts) ([]*model.User, *model.AppError) {
-	result := <-a.Srv.Store.User().GetProfileByIds(userIds, viewRestrictions == nil, viewRestrictions)
+	allowFromCache := options.ViewRestrictions == nil
+
+	result := <-a.Srv.Store.User().GetProfileByIds(userIds, options, allowFromCache)
 	if result.Err != nil {
 		return nil, result.Err
 	}
-	return a.sanitizeProfiles(result.Data.([]*model.User), asAdmin), nil
+
+	return a.sanitizeProfiles(result.Data.([]*model.User), options.IsAdmin), nil
 }
 
 func (a *App) GetUsersByGroupChannelIds(channelIds []string, asAdmin bool) (map[string][]*model.User, *model.AppError) {
@@ -1713,7 +1716,7 @@ func (a *App) esSearchUsersInTeam(teamId, term string, options *model.UserSearch
 		return nil, err
 	}
 
-	result := <-a.Srv.Store.User().GetProfileByIds(usersIds, false, nil)
+	result := <-a.Srv.Store.User().GetProfileByIds(usersIds, nil, false)
 	if result.Err != nil {
 		return nil, result.Err
 	}
@@ -1800,8 +1803,8 @@ func (a *App) esAutocompleteUsersInChannel(teamId, channelId, term string, optio
 	if err != nil {
 		return nil, err
 	}
-	uchan := a.Srv.Store.User().GetProfileByIds(uchanIds, false, nil)
+	uchan := a.Srv.Store.User().GetProfileByIds(uchanIds, nil, false)
-	nuchan := a.Srv.Store.User().GetProfileByIds(nuchanIds, false, nil)
+	nuchan := a.Srv.Store.User().GetProfileByIds(nuchanIds, nil, false)
 	autocomplete := &model.UserAutocompleteInChannel{}
 
 	result := <-uchan
@@ -1890,7 +1893,7 @@ func (a *App) esAutocompleteUsersInTeam(teamId, term string, options *model.User
 		return nil, err
 	}
 
-	result := <-a.Srv.Store.User().GetProfileByIds(usersIds, false, nil)
+	result := <-a.Srv.Store.User().GetProfileByIds(usersIds, nil, false)
 	if result.Err != nil {
 		return nil, result.Err
 	}

app/user_viewmembers_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/mattermost/mattermost-server/model"
+	"github.com/mattermost/mattermost-server/store"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -828,7 +829,10 @@ func TestResctrictedViewMembers(t *testing.T) {
 
 		for _, tc := range testCases {
 			t.Run(tc.Name, func(t *testing.T) {
-				results, err := th.App.GetUsersByIds(tc.UserIds, false, tc.Restrictions)
+				results, err := th.App.GetUsersByIds(tc.UserIds, &store.UserGetByIdsOpts{
+					IsAdmin:          false,
+					ViewRestrictions: tc.Restrictions,
+				})
 				require.Nil(t, err)
 				ids := []string{}
 				for _, result := range results {

model/client4.go

@@ -936,6 +936,26 @@ func (c *Client4) GetUsersByIds(userIds []string) ([]*User, *Response) {
 	return UserListFromJson(r.Body), BuildResponse(r)
 }
 
+// GetUsersByIds returns a list of users based on the provided user ids.
+func (c *Client4) GetUsersByIdsWithOptions(userIds []string, options *UserGetByIdsOptions) ([]*User, *Response) {
+	v := url.Values{}
+	if options.Since != 0 {
+		v.Set("since", fmt.Sprintf("%d", options.Since))
+	}
+
+	url := c.GetUsersRoute() + "/ids"
+	if len(v) > 0 {
+		url += "?" + v.Encode()
+	}
+
+	r, err := c.DoApiPost(url, ArrayToJson(userIds))
+	if err != nil {
+		return nil, BuildErrorResponse(r, err)
+	}
+	defer closeBody(r)
+	return UserListFromJson(r.Body), BuildResponse(r)
+}
+
 // GetUsersByUsernames returns a list of users based on the provided usernames.
 func (c *Client4) GetUsersByUsernames(usernames []string) ([]*User, *Response) {
 	r, err := c.DoApiPost(c.GetUsersRoute()+"/usernames", ArrayToJson(usernames))

model/user_get.go

@@ -29,3 +29,8 @@ type UserGetOptions struct {
 	// Page size
 	PerPage int
 }
+
+type UserGetByIdsOptions struct {
+	// Since filters the users based on their UpdateAt timestamp.
+	Since int64
+}

store/sqlstore/user_store.go

@@ -10,6 +10,7 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/Masterminds/squirrel"
 	sq "github.com/Masterminds/squirrel"
 	"github.com/mattermost/gorp"
 
@@ -846,8 +847,12 @@ func (us SqlUserStore) GetNewUsersForTeam(teamId string, offset, limit int, view
 	return users, nil
 }
 
-func (us SqlUserStore) GetProfileByIds(userIds []string, allowFromCache bool, viewRestrictions *model.ViewUsersRestrictions) store.StoreChannel {
+func (us SqlUserStore) GetProfileByIds(userIds []string, options *store.UserGetByIdsOpts, allowFromCache bool) store.StoreChannel {
 	return store.Do(func(result *store.StoreResult) {
+		if options == nil {
+			options = &store.UserGetByIdsOpts{}
+		}
+
 		users := []*model.User{}
 		remainingUserIds := make([]string, 0)
 
@@ -856,7 +861,10 @@ func (us SqlUserStore) GetProfileByIds(userIds []string, allowFromCache bool, vi
 				if cacheItem, ok := profileByIdsCache.Get(userId); ok {
 					u := &model.User{}
 					*u = *cacheItem.(*model.User)
-					users = append(users, u)
+
+					if options.Since == 0 || u.UpdateAt > options.Since {
+						users = append(users, u)
+					}
 				} else {
 					remainingUserIds = append(remainingUserIds, userId)
 				}
@@ -884,7 +892,13 @@ func (us SqlUserStore) GetProfileByIds(userIds []string, allowFromCache bool, vi
 			}).
 			OrderBy("u.Username ASC")
 
-		query = applyViewRestrictionsFilter(query, viewRestrictions, true)
+		if options.Since > 0 {
+			query = query.Where(squirrel.Gt(map[string]interface{}{
+				"u.UpdateAt": options.Since,
+			}))
+		}
+
+		query = applyViewRestrictionsFilter(query, options.ViewRestrictions, true)
 
 		queryString, args, err := query.ToSql()
 		if err != nil {

store/store.go

@@ -268,7 +268,7 @@ type UserStore interface {
 	GetProfilesByUsernames(usernames []string, viewRestrictions *model.ViewUsersRestrictions) StoreChannel
 	GetAllProfiles(options *model.UserGetOptions) StoreChannel
 	GetProfiles(options *model.UserGetOptions) StoreChannel
-	GetProfileByIds(userId []string, allowFromCache bool, viewRestrictions *model.ViewUsersRestrictions) StoreChannel
+	GetProfileByIds(userIds []string, options *UserGetByIdsOpts, allowFromCache bool) StoreChannel
 	GetProfileByGroupChannelIdsForUser(userId string, channelIds []string) (map[string][]*model.User, *model.AppError)
 	InvalidatProfileCacheForUser(userId string)
 	GetByEmail(email string) (*model.User, *model.AppError)
@@ -628,3 +628,14 @@ type ChannelSearchOpts struct {
 	IncludeDeleted       bool
 	ExcludeChannelNames  []string
 }
+
+type UserGetByIdsOpts struct {
+	// IsAdmin tracks whether or not the request is being made by an administrator. Does nothing when provided by a client.
+	IsAdmin bool
+
+	// Restrict to search in a list of teams and channels. Does nothing when provided by a client.
+	ViewRestrictions *model.ViewUsersRestrictions
+
+	// Since filters the users based on their UpdateAt timestamp.
+	Since int64
+}

store/storetest/mocks/UserStore.go

@@ -447,13 +447,13 @@ func (_m *UserStore) GetProfileByGroupChannelIdsForUser(userId string, channelId
 	return r0, r1
 }
 
-// GetProfileByIds provides a mock function with given fields: userId, allowFromCache, viewRestrictions
+// GetProfileByIds provides a mock function with given fields: userIds, options, allowFromCache
-func (_m *UserStore) GetProfileByIds(userId []string, allowFromCache bool, viewRestrictions *model.ViewUsersRestrictions) store.StoreChannel {
+func (_m *UserStore) GetProfileByIds(userIds []string, options *store.UserGetByIdsOpts, allowFromCache bool) store.StoreChannel {
-	ret := _m.Called(userId, allowFromCache, viewRestrictions)
+	ret := _m.Called(userIds, options, allowFromCache)
 
 	var r0 store.StoreChannel
-	if rf, ok := ret.Get(0).(func([]string, bool, *model.ViewUsersRestrictions) store.StoreChannel); ok {
+	if rf, ok := ret.Get(0).(func([]string, *store.UserGetByIdsOpts, bool) store.StoreChannel); ok {
-		r0 = rf(userId, allowFromCache, viewRestrictions)
+		r0 = rf(userIds, options, allowFromCache)
 	} else {
 		if ret.Get(0) != nil {
 			r0 = ret.Get(0).(store.StoreChannel)

store/storetest/user_store.go

@@ -1187,35 +1187,51 @@ func testUserStoreGetProfilesByIds(t *testing.T, ss store.Store) {
 	u3.IsBot = true
 	defer func() { require.Nil(t, ss.Bot().PermanentDelete(u3.Id)) }()
 
+	u4 := store.Must(ss.User().Save(&model.User{
+		Email:    MakeEmail(),
+		Username: "u4" + model.NewId(),
+	})).(*model.User)
+	defer func() { require.Nil(t, ss.User().PermanentDelete(u4.Id)) }()
+
 	t.Run("get u1 by id, no caching", func(t *testing.T) {
-		result := <-ss.User().GetProfileByIds([]string{u1.Id}, false, nil)
+		result := <-ss.User().GetProfileByIds([]string{u1.Id}, nil, false)
 		require.Nil(t, result.Err)
 		assert.Equal(t, []*model.User{sanitized(u1)}, result.Data.([]*model.User))
 	})
 
 	t.Run("get u1 by id, caching", func(t *testing.T) {
-		result := <-ss.User().GetProfileByIds([]string{u1.Id}, true, nil)
+		result := <-ss.User().GetProfileByIds([]string{u1.Id}, nil, true)
 		require.Nil(t, result.Err)
 		assert.Equal(t, []*model.User{sanitized(u1)}, result.Data.([]*model.User))
 	})
 
 	t.Run("get u1, u2, u3 by id, no caching", func(t *testing.T) {
-		result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, false, nil)
+		result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, false)
 		require.Nil(t, result.Err)
 		assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, result.Data.([]*model.User))
 	})
 
 	t.Run("get u1, u2, u3 by id, caching", func(t *testing.T) {
-		result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, true, nil)
+		result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, true)
 		require.Nil(t, result.Err)
 		assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, result.Data.([]*model.User))
 	})
 
 	t.Run("get unknown id, caching", func(t *testing.T) {
-		result := <-ss.User().GetProfileByIds([]string{"123"}, true, nil)
+		result := <-ss.User().GetProfileByIds([]string{"123"}, nil, true)
 		require.Nil(t, result.Err)
 		assert.Equal(t, []*model.User{}, result.Data.([]*model.User))
 	})
+
+	t.Run("should only return users with UpdateAt greater than the since time", func(t *testing.T) {
+		result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id, u4.Id}, &store.UserGetByIdsOpts{
+			Since: u2.CreateAt,
+		}, true)
+		require.Nil(t, result.Err)
+
+		// u3 comes from the cache, and u4 does not
+		assert.Equal(t, []*model.User{sanitized(u3), sanitized(u4)}, result.Data.([]*model.User))
+	})
 }
 
 func testUserStoreGetProfileByGroupChannelIdsForUser(t *testing.T, ss store.Store) {