From 302dae5bb982aad14324a4df61a018557f3dd24e Mon Sep 17 00:00:00 2001 From: Stephen Kiers Date: Fri, 9 Mar 2018 05:48:30 -0700 Subject: MM-9274- Sort Users in Channel by status (#8181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- app/apptestlib.go | 16 +++++++ app/server_test.go | 2 +- app/status.go | 32 +++++-------- app/status_test.go | 40 +++++++++++++++++ app/user.go | 17 +++++++ app/user_test.go | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 213 insertions(+), 23 deletions(-) create mode 100644 app/status_test.go (limited to 'app') diff --git a/app/apptestlib.go b/app/apptestlib.go index 9e5bfc637..01f5b0102 100644 --- a/app/apptestlib.go +++ b/app/apptestlib.go @@ -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) diff --git a/app/server_test.go b/app/server_test.go index de358b976..94771a44e 100644 --- a/app/server_test.go +++ b/app/server_test.go @@ -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 }) diff --git a/app/status.go b/app/status.go index d677f9a23..c8bff0d1a 100644 --- a/app/status.go +++ b/app/status.go @@ -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 { diff --git a/app/status_test.go b/app/status_test.go new file mode 100644 index 000000000..bf5736a48 --- /dev/null +++ b/app/status_test.go @@ -0,0 +1,40 @@ +// 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) + } + }) + } +} diff --git a/app/user.go b/app/user.go index c303cbc68..dbce296d2 100644 --- a/app/user.go +++ b/app/user.go @@ -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 diff --git a/app/user_test.go b/app/user_test.go index 38ff286b3..94052da61 100644 --- a/app/user_test.go +++ b/app/user_test.go @@ -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") + } + }) +} -- cgit v1.2.3-1-g7c22