Commit 302dae5b authored by Stephen Kiers's avatar Stephen Kiers Committed by Joram Wilander

MM-9274- Sort Users in Channel by status (#8181)

* sort by lastActivity

* added status ordering to Users

* sort offline before dnd

* remove data not needed

* added seperate call for when order=‘status’ is on GetUser request

* remove PrintLn

* styling fix

* remove mistake

* mistake 2

* better comment

* explicit if statemnt

* writing tests

* removed manually added mocks

* generated mock

* ICU-668 Added unit tests

* style fix

* sort by lastActivity

* added status ordering to Users

* sort offline before dnd

* remove data not needed

* added seperate call for when order=‘status’ is on GetUser request

* remove PrintLn

* styling fix

* remove mistake

* mistake 2

* better comment

* explicit if statemnt

* writing tests

* removed manually added mocks

* generated mock

* ICU-668 Added unit tests

* style fix

* reverse dnd and offline

* Fixed app.SaveStatusAndBroadcast

* Fixed incorrect merge

* Fixing incorrect merge again
parent 31532f7f
......@@ -467,6 +467,22 @@ func (me *TestHelper) LinkUserToTeam(user *model.User, team *model.Team) {
utils.EnableDebugLogForTest()
}
func (me *TestHelper) AddUserToChannel(user *model.User, channel *model.Channel) *model.ChannelMember {
utils.DisableDebugLogForTest()
member, err := me.App.AddUserToChannel(user, channel)
if err != nil {
l4g.Error(err.Error())
l4g.Close()
time.Sleep(time.Second)
panic(err)
}
utils.EnableDebugLogForTest()
return member
}
func (me *TestHelper) GenerateTestEmail() string {
if me.App.Config().EmailSettings.SMTPServer != "dockerhost" && os.Getenv("CI_INBUCKET_PORT") == "" {
return strings.ToLower("success+" + model.NewId() + "@simulator.amazonses.com")
......
......@@ -290,16 +290,21 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) {
return
}
if sort != "" && sort != "last_activity_at" && sort != "create_at" {
if sort != "" && sort != "last_activity_at" && sort != "create_at" && sort != "status" {
c.SetInvalidUrlParam("sort")
return
}
// Currently only supports sorting on a team
// or sort="status" on inChannelId
if (sort == "last_activity_at" || sort == "create_at") && (inTeamId == "" || notInTeamId != "" || inChannelId != "" || notInChannelId != "" || withoutTeam != "") {
c.SetInvalidUrlParam("sort")
return
}
if sort == "status" && inChannelId == "" {
c.SetInvalidUrlParam("sort")
return
}
var profiles []*model.User
var err *model.AppError
......@@ -355,8 +360,11 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) {
c.SetPermissionError(model.PERMISSION_READ_CHANNEL)
return
}
profiles, err = c.App.GetUsersInChannelPage(inChannelId, c.Params.Page, c.Params.PerPage, c.IsSystemAdmin())
if sort == "status" {
profiles, err = c.App.GetUsersInChannelPageByStatus(inChannelId, c.Params.Page, c.Params.PerPage, c.IsSystemAdmin())
} else {
profiles, err = c.App.GetUsersInChannelPage(inChannelId, c.Params.Page, c.Params.PerPage, c.IsSystemAdmin())
}
} else {
// No permission check required
......
......@@ -2650,3 +2650,146 @@ func TestUserAccessTokenDisableConfig(t *testing.T) {
_, resp = Client.GetMe("")
CheckNoError(t, resp)
}
func TestGetUsersByStatus(t *testing.T) {
th := Setup()
defer th.TearDown()
team, err := th.App.CreateTeam(&model.Team{
DisplayName: "dn_" + model.NewId(),
Name: GenerateTestTeamName(),
Email: th.GenerateTestEmail(),
Type: model.TEAM_OPEN,
})
if err != nil {
t.Fatalf("failed to create team: %v", err)
}
channel, err := th.App.CreateChannel(&model.Channel{
DisplayName: "dn_" + model.NewId(),
Name: "name_" + model.NewId(),
Type: model.CHANNEL_OPEN,
TeamId: team.Id,
CreatorId: model.NewId(),
}, false)
if err != nil {
t.Fatalf("failed to create channel: %v", err)
}
createUserWithStatus := func(username string, status string) *model.User {
id := model.NewId()
user, err := th.App.CreateUser(&model.User{
Email: "success+" + id + "@simulator.amazonses.com",
Username: "un_" + username + "_" + id,
Nickname: "nn_" + id,
Password: "Password1",
})
if err != nil {
t.Fatalf("failed to create user: %v", err)
}
th.LinkUserToTeam(user, team)
th.AddUserToChannel(user, channel)
th.App.SaveAndBroadcastStatus(&model.Status{
UserId: user.Id,
Status: status,
Manual: true,
})
return user
}
// Creating these out of order in case that affects results
offlineUser1 := createUserWithStatus("offline1", model.STATUS_OFFLINE)
offlineUser2 := createUserWithStatus("offline2", model.STATUS_OFFLINE)
awayUser1 := createUserWithStatus("away1", model.STATUS_AWAY)
awayUser2 := createUserWithStatus("away2", model.STATUS_AWAY)
onlineUser1 := createUserWithStatus("online1", model.STATUS_ONLINE)
onlineUser2 := createUserWithStatus("online2", model.STATUS_ONLINE)
dndUser1 := createUserWithStatus("dnd1", model.STATUS_DND)
dndUser2 := createUserWithStatus("dnd2", model.STATUS_DND)
client := th.CreateClient()
if _, resp := client.Login(onlineUser2.Username, "Password1"); resp.Error != nil {
t.Fatal(resp.Error)
}
t.Run("sorting by status then alphabetical", func(t *testing.T) {
usersByStatus, resp := client.GetUsersInChannelByStatus(channel.Id, 0, 8, "")
if resp.Error != nil {
t.Fatal(resp.Error)
}
expectedUsersByStatus := []*model.User{
onlineUser1,
onlineUser2,
awayUser1,
awayUser2,
dndUser1,
dndUser2,
offlineUser1,
offlineUser2,
}
if len(usersByStatus) != len(expectedUsersByStatus) {
t.Fatalf("received only %v users, expected %v", len(usersByStatus), len(expectedUsersByStatus))
}
for i := range usersByStatus {
if usersByStatus[i].Id != expectedUsersByStatus[i].Id {
t.Fatalf("received user %v at index %v, expected %v", usersByStatus[i].Username, i, expectedUsersByStatus[i].Username)
}
}
})
t.Run("paging", func(t *testing.T) {
usersByStatus, resp := client.GetUsersInChannelByStatus(channel.Id, 0, 3, "")
if resp.Error != nil {
t.Fatal(resp.Error)
}
if len(usersByStatus) != 3 {
t.Fatal("received too many users")
}
if usersByStatus[0].Id != onlineUser1.Id && usersByStatus[1].Id != onlineUser2.Id {
t.Fatal("expected to receive online users first")
}
if usersByStatus[2].Id != awayUser1.Id {
t.Fatal("expected to receive away users second")
}
usersByStatus, resp = client.GetUsersInChannelByStatus(channel.Id, 1, 3, "")
if resp.Error != nil {
t.Fatal(resp.Error)
}
if usersByStatus[0].Id != awayUser2.Id {
t.Fatal("expected to receive away users second")
}
if usersByStatus[1].Id != dndUser1.Id && usersByStatus[2].Id != dndUser2.Id {
t.Fatal("expected to receive dnd users third")
}
usersByStatus, resp = client.GetUsersInChannelByStatus(channel.Id, 1, 4, "")
if resp.Error != nil {
t.Fatal(resp.Error)
}
if len(usersByStatus) != 4 {
t.Fatal("received too many users")
}
if usersByStatus[0].Id != dndUser1.Id && usersByStatus[1].Id != dndUser2.Id {
t.Fatal("expected to receive dnd users third")
}
if usersByStatus[2].Id != offlineUser1.Id && usersByStatus[3].Id != offlineUser2.Id {
t.Fatal("expected to receive offline users last")
}
})
}
......@@ -245,6 +245,22 @@ func (me *TestHelper) LinkUserToTeam(user *model.User, team *model.Team) {
utils.EnableDebugLogForTest()
}
func (me *TestHelper) AddUserToChannel(user *model.User, channel *model.Channel) *model.ChannelMember {
utils.DisableDebugLogForTest()
member, err := me.App.AddUserToChannel(user, channel)
if err != nil {
l4g.Error(err.Error())
l4g.Close()
time.Sleep(time.Second)
panic(err)
}
utils.EnableDebugLogForTest()
return member
}
func (me *TestHelper) TearDown() {
me.App.Shutdown()
os.Remove(me.tempConfigPath)
......
......@@ -26,7 +26,7 @@ func TestStartServerRateLimiterCriticalError(t *testing.T) {
// Attempt to use Rate Limiter with an invalid config
a.UpdateConfig(func(cfg *model.Config) {
*cfg.RateLimitSettings.Enable = true
*cfg.RateLimitSettings.Enable = true
*cfg.RateLimitSettings.MaxBurst = -100
})
......
......@@ -236,16 +236,7 @@ func (a *App) SetStatusOffline(userId string, manual bool) {
status = &model.Status{UserId: userId, Status: model.STATUS_OFFLINE, Manual: manual, LastActivityAt: model.GetMillis(), ActiveChannel: ""}
a.AddStatusCache(status)
if result := <-a.Srv.Store.Status().SaveOrUpdate(status); result.Err != nil {
l4g.Error(utils.T("api.status.save_status.error"), userId, result.Err)
}
event := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_STATUS_CHANGE, "", "", status.UserId, nil)
event.Add("status", model.STATUS_OFFLINE)
event.Add("user_id", status.UserId)
a.Publish(event)
a.SaveAndBroadcastStatus(status)
}
func (a *App) SetStatusAwayIfNeeded(userId string, manual bool) {
......@@ -277,16 +268,7 @@ func (a *App) SetStatusAwayIfNeeded(userId string, manual bool) {
status.Manual = manual
status.ActiveChannel = ""
a.AddStatusCache(status)
if result := <-a.Srv.Store.Status().SaveOrUpdate(status); result.Err != nil {
l4g.Error(utils.T("api.status.save_status.error"), userId, result.Err)
}
event := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_STATUS_CHANGE, "", "", status.UserId, nil)
event.Add("status", model.STATUS_AWAY)
event.Add("user_id", status.UserId)
a.Publish(event)
a.SaveAndBroadcastStatus(status)
}
func (a *App) SetStatusDoNotDisturb(userId string) {
......@@ -303,16 +285,22 @@ func (a *App) SetStatusDoNotDisturb(userId string) {
status.Status = model.STATUS_DND
status.Manual = true
a.SaveAndBroadcastStatus(status)
}
func (a *App) SaveAndBroadcastStatus(status *model.Status) *model.AppError {
a.AddStatusCache(status)
if result := <-a.Srv.Store.Status().SaveOrUpdate(status); result.Err != nil {
l4g.Error(utils.T("api.status.save_status.error"), userId, result.Err)
l4g.Error(utils.T("api.status.save_status.error"), status.UserId, result.Err)
}
event := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_STATUS_CHANGE, "", "", status.UserId, nil)
event.Add("status", model.STATUS_DND)
event.Add("status", status.Status)
event.Add("user_id", status.UserId)
a.Publish(event)
return nil
}
func GetStatusFromCache(userId string) *model.Status {
......
// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved.
// See License.txt for license information.
package app
import (
"testing"
"github.com/mattermost/mattermost-server/model"
)
func TestSaveStatus(t *testing.T) {
th := Setup().InitBasic()
defer th.TearDown()
user := th.BasicUser
for _, statusString := range []string{
model.STATUS_ONLINE,
model.STATUS_AWAY,
model.STATUS_DND,
model.STATUS_OFFLINE,
} {
t.Run(statusString, func(t *testing.T) {
status := &model.Status{
UserId: user.Id,
Status: statusString,
}
th.App.SaveAndBroadcastStatus(status)
after, err := th.App.GetStatus(user.Id)
if err != nil {
t.Fatalf("failed to get status after save: %v", err)
} else if after.Status != statusString {
t.Fatalf("failed to save status, got %v, expected %v", after.Status, statusString)
}
})
}
}
......@@ -505,6 +505,14 @@ func (a *App) GetUsersInChannel(channelId string, offset int, limit int) ([]*mod
}
}
func (a *App) GetUsersInChannelByStatus(channelId string, offset int, limit int) ([]*model.User, *model.AppError) {
if result := <-a.Srv.Store.User().GetProfilesInChannelByStatus(channelId, offset, limit); result.Err != nil {
return nil, result.Err
} else {
return result.Data.([]*model.User), nil
}
}
func (a *App) GetUsersInChannelMap(channelId string, offset int, limit int, asAdmin bool) (map[string]*model.User, *model.AppError) {
users, err := a.GetUsersInChannel(channelId, offset, limit)
if err != nil {
......@@ -530,6 +538,15 @@ func (a *App) GetUsersInChannelPage(channelId string, page int, perPage int, asA
return a.sanitizeProfiles(users, asAdmin), nil
}
func (a *App) GetUsersInChannelPageByStatus(channelId string, page int, perPage int, asAdmin bool) ([]*model.User, *model.AppError) {
users, err := a.GetUsersInChannelByStatus(channelId, page*perPage, perPage)
if err != nil {
return nil, err
}
return a.sanitizeProfiles(users, asAdmin), nil
}
func (a *App) GetUsersNotInChannel(teamId string, channelId string, offset int, limit int) ([]*model.User, *model.AppError) {
if result := <-a.Srv.Store.User().GetProfilesNotInChannel(teamId, channelId, offset, limit); result.Err != nil {
return nil, result.Err
......
......@@ -299,3 +299,132 @@ func createGitlabUser(t *testing.T, a *App, email string, username string) (*mod
return user, gitlabUserObj
}
func TestGetUsersByStatus(t *testing.T) {
th := Setup()
defer th.TearDown()
team := th.CreateTeam()
channel, err := th.App.CreateChannel(&model.Channel{
DisplayName: "dn_" + model.NewId(),
Name: "name_" + model.NewId(),
Type: model.CHANNEL_OPEN,
TeamId: team.Id,
CreatorId: model.NewId(),
}, false)
if err != nil {
t.Fatalf("failed to create channel: %v", err)
}
createUserWithStatus := func(username string, status string) *model.User {
id := model.NewId()
user, err := th.App.CreateUser(&model.User{
Email: "success+" + id + "@simulator.amazonses.com",
Username: "un_" + username + "_" + id,
Nickname: "nn_" + id,
Password: "Password1",
})
if err != nil {
t.Fatalf("failed to create user: %v", err)
}
th.LinkUserToTeam(user, team)
th.AddUserToChannel(user, channel)
th.App.SaveAndBroadcastStatus(&model.Status{
UserId: user.Id,
Status: status,
Manual: true,
})
return user
}
// Creating these out of order in case that affects results
awayUser1 := createUserWithStatus("away1", model.STATUS_AWAY)
awayUser2 := createUserWithStatus("away2", model.STATUS_AWAY)
dndUser1 := createUserWithStatus("dnd1", model.STATUS_DND)
dndUser2 := createUserWithStatus("dnd2", model.STATUS_DND)
offlineUser1 := createUserWithStatus("offline1", model.STATUS_OFFLINE)
offlineUser2 := createUserWithStatus("offline2", model.STATUS_OFFLINE)
onlineUser1 := createUserWithStatus("online1", model.STATUS_ONLINE)
onlineUser2 := createUserWithStatus("online2", model.STATUS_ONLINE)
t.Run("sorting by status then alphabetical", func(t *testing.T) {
usersByStatus, err := th.App.GetUsersInChannelPageByStatus(channel.Id, 0, 8, true)
if err != nil {
t.Fatal(err)
}
expectedUsersByStatus := []*model.User{
onlineUser1,
onlineUser2,
awayUser1,
awayUser2,
dndUser1,
dndUser2,
offlineUser1,
offlineUser2,
}
if len(usersByStatus) != len(expectedUsersByStatus) {
t.Fatalf("received only %v users, expected %v", len(usersByStatus), len(expectedUsersByStatus))
}
for i := range usersByStatus {
if usersByStatus[i].Id != expectedUsersByStatus[i].Id {
t.Fatalf("received user %v at index %v, expected %v", usersByStatus[i].Username, i, expectedUsersByStatus[i].Username)
}
}
})
t.Run("paging", func(t *testing.T) {
usersByStatus, err := th.App.GetUsersInChannelPageByStatus(channel.Id, 0, 3, true)
if err != nil {
t.Fatal(err)
}
if len(usersByStatus) != 3 {
t.Fatal("received too many users")
}
if usersByStatus[0].Id != onlineUser1.Id && usersByStatus[1].Id != onlineUser2.Id {
t.Fatal("expected to receive online users first")
}
if usersByStatus[2].Id != awayUser1.Id {
t.Fatal("expected to receive away users second")
}
usersByStatus, err = th.App.GetUsersInChannelPageByStatus(channel.Id, 1, 3, true)
if err != nil {
t.Fatal(err)
}
if usersByStatus[0].Id != awayUser2.Id {
t.Fatal("expected to receive away users second")
}
if usersByStatus[1].Id != dndUser1.Id && usersByStatus[2].Id != dndUser2.Id {
t.Fatal("expected to receive dnd users third")
}
usersByStatus, err = th.App.GetUsersInChannelPageByStatus(channel.Id, 1, 4, true)
if err != nil {
t.Fatal(err)
}
if len(usersByStatus) != 4 {
t.Fatal("received too many users")
}
if usersByStatus[0].Id != dndUser1.Id && usersByStatus[1].Id != dndUser2.Id {
t.Fatal("expected to receive dnd users third")
}
if usersByStatus[2].Id != offlineUser1.Id && usersByStatus[3].Id != offlineUser2.Id {
t.Fatal("expected to receive offline users last")
}
})
}
......@@ -695,7 +695,7 @@ func (c *Client4) GetUsersNotInTeam(teamId string, page int, perPage int, etag s
}
}
// GetUsersInChannel returns a page of users on a team. Page counting starts at 0.
// GetUsersInChannel returns a page of users in a channel. Page counting starts at 0.
func (c *Client4) GetUsersInChannel(channelId string, page int, perPage int, etag string) ([]*User, *Response) {
query := fmt.Sprintf("?in_channel=%v&page=%v&per_page=%v", channelId, page, perPage)
if r, err := c.DoApiGet(c.GetUsersRoute()+query, etag); err != nil {
......@@ -706,7 +706,18 @@ func (c *Client4) GetUsersInChannel(channelId string, page int, perPage int, eta
}
}
// GetUsersNotInChannel returns a page of users on a team. Page counting starts at 0.
// GetUsersInChannelStatus returns a page of users in a channel. Page counting starts at 0. Sorted by Status
func (c *Client4) GetUsersInChannelByStatus(channelId string, page int, perPage int, etag string) ([]*User, *Response) {
query := fmt.Sprintf("?in_channel=%v&page=%v&per_page=%v&sort=status", channelId, page, perPage)
if r, err := c.DoApiGet(c.GetUsersRoute()+query, etag); err != nil {
return nil, BuildErrorResponse(r, err)
} else {
defer closeBody(r)
return UserListFromJson(r.Body), BuildResponse(r)
}
}
// GetUsersNotInChannel returns a page of users not in a channel. Page counting starts at 0.
func (c *Client4) GetUsersNotInChannel(teamId, channelId string, page int, perPage int, etag string) ([]*User, *Response) {
query := fmt.Sprintf("?in_team=%v&not_in_channel=%v&page=%v&per_page=%v", teamId, channelId, page, perPage)
if r, err := c.DoApiGet(c.GetUsersRoute()+query, etag); err != nil {
......
......@@ -412,7 +412,18 @@ func (us SqlUserStore) GetProfilesInChannel(channelId string, offset int, limit
return store.Do(func(result *store.StoreResult) {
var users []*model.User
query := "SELECT Users.* FROM Users, ChannelMembers WHERE ChannelMembers.ChannelId = :ChannelId AND Users.Id = ChannelMembers.UserId ORDER BY Users.Username ASC LIMIT :Limit OFFSET :Offset"
query := `
SELECT
Users.*
FROM
Users, ChannelMembers
WHERE
ChannelMembers.ChannelId = :ChannelId
AND Users.Id = ChannelMembers.UserId
ORDER BY
Users.Username ASC
LIMIT :Limit OFFSET :Offset
`
if _, err := us.GetReplica().Select(&users, query, map[string]interface{}{"ChannelId": channelId, "Offset": offset, "Limit": limit}); err != nil {
result.Err = model.NewAppError("SqlUserStore.GetProfilesInChannel", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError)
......@@ -427,6 +438,42 @@ func (us SqlUserStore) GetProfilesInChannel(channelId string, offset int, limit
})
}
func (us SqlUserStore) GetProfilesInChannelByStatus(channelId string, offset int, limit int) store.StoreChannel {
return store.Do(func(result *store.StoreResult) {
var users []*model.User
query := `
SELECT
Users.*
FROM Users
INNER JOIN ChannelMembers ON Users.Id = ChannelMembers.UserId
LEFT JOIN Status ON Users.Id = Status.UserId
WHERE
ChannelMembers.ChannelId = :ChannelId
ORDER BY
CASE Status
WHEN 'online' THEN 1
WHEN 'away' THEN 2
WHEN 'dnd' THEN 3
ELSE 4
END,
Users.Username ASC
LIMIT :Limit OFFSET :Offset
`
if _, err := us.GetReplica().Select(&users, query, map[string]interface{}{"ChannelId": channelId, "Offset": offset, "Limit": limit}); err != nil {
result.Err = model.NewAppError("SqlUserStore.GetProfilesInChannelByStatus", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError)
} else {
for _, u := range users {
u.Sanitize(map[string]bool{})
}
result.Data = users
}
})
}
func (us SqlUserStore) GetAllProfilesInChannel(channelId string, allowFromCache bool) store.StoreChannel {
return store.Do(func(result *store.StoreResult) {
if allowFromCache {
......
......@@ -216,6 +216,7 @@ type UserStore interface {
InvalidateProfilesInChannelCacheByUser(userId string)
InvalidateProfilesInChannelCache(channelId string)
GetProfilesInChannel(channelId string, offset int, limit int) StoreChannel
GetProfilesInChannelByStatus(channelId string, offset int, limit int) StoreChannel
GetAllProfilesInChannel(channelId string, allowFromCache bool) StoreChannel
GetProfilesNotInChannel(teamId string, channelId string, offset int, limit int) StoreChannel
GetProfilesWithoutTeam(offset int, limit int) StoreChannel
......
......@@ -354,6 +354,22 @@ func (_m *UserStore) GetProfilesInChannel(channelId string, offset int, limit in
return r0
}
// GetProfilesInChannelByStatus provides a mock function with given fields: channelId, offset, limit
func (_m *UserStore) GetProfilesInChannelByStatus(channelId string, offset int, limit int) store.StoreChannel {
ret := _m.Called(channelId, offset, limit)
var r0 store.StoreChannel
if rf, ok := ret.Get(0).(func(string, int, int) store.StoreChannel); ok {
r0 = rf(channelId, offset, limit)
} else {
if ret.Get(0) != nil {
r0 = ret.Get(0).(store.StoreChannel)
}
}
return r0
}