From 293f38ad0bb4965bb53f1f931edd978bc5d1d795 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Espino=20Garc=C3=ADa?= Date: Thu, 29 May 2025 10:41:14 +0200 Subject: [PATCH] Ensure users status is set to offline when deactivated (#30900) --- server/channels/api4/status.go | 2 +- server/channels/api4/status_test.go | 2 +- server/channels/app/notification_test.go | 8 +- server/channels/app/platform/helper_test.go | 2 +- server/channels/app/platform/status.go | 4 +- server/channels/app/platform/status_test.go | 125 ++++++++++++++++++ server/channels/app/platform/web_hub.go | 4 +- server/channels/app/plugin_api.go | 2 +- .../app/slashcommands/command_offline.go | 2 +- server/channels/app/status.go | 4 +- server/channels/app/user.go | 2 +- 11 files changed, 141 insertions(+), 16 deletions(-) diff --git a/server/channels/api4/status.go b/server/channels/api4/status.go index 089f3cefb7..00f70ca490 100644 --- a/server/channels/api4/status.go +++ b/server/channels/api4/status.go @@ -120,7 +120,7 @@ func updateUserStatus(c *Context, w http.ResponseWriter, r *http.Request) { case "online": c.App.SetStatusOnline(c.Params.UserId, true) case "offline": - c.App.SetStatusOffline(c.Params.UserId, true) + c.App.SetStatusOffline(c.Params.UserId, true, false) case "away": c.App.SetStatusAwayIfNeeded(c.Params.UserId, true) case "dnd": diff --git a/server/channels/api4/status_test.go b/server/channels/api4/status_test.go index 4cd0088e47..6e0f73c8d7 100644 --- a/server/channels/api4/status_test.go +++ b/server/channels/api4/status_test.go @@ -72,7 +72,7 @@ func TestGetUserStatus(t *testing.T) { }) t.Run("back to offline status", func(t *testing.T) { - th.App.SetStatusOffline(th.BasicUser.Id, true) + th.App.SetStatusOffline(th.BasicUser.Id, true, false) userStatus, _, err := client.GetUserStatus(context.Background(), th.BasicUser.Id, "") require.NoError(t, err) assert.Equal(t, "offline", userStatus.Status) diff --git a/server/channels/app/notification_test.go b/server/channels/app/notification_test.go index 406c53b773..9d508b2b82 100644 --- a/server/channels/app/notification_test.go +++ b/server/channels/app/notification_test.go @@ -2440,7 +2440,7 @@ func TestUserAllowsEmail(t *testing.T) { t.Run("should return true", func(t *testing.T) { user := th.CreateUser() - th.App.SetStatusOffline(user.Id, true) + th.App.SetStatusOffline(user.Id, true, false) channelMemberNotificationProps := model.StringMap{ model.EmailNotifyProp: model.ChannelNotifyDefault, @@ -2466,7 +2466,7 @@ func TestUserAllowsEmail(t *testing.T) { t.Run("should return false in case the EMAIL_NOTIFY_PROP is false", func(t *testing.T) { user := th.CreateUser() - th.App.SetStatusOffline(user.Id, true) + th.App.SetStatusOffline(user.Id, true, false) channelMemberNotificationProps := model.StringMap{ model.EmailNotifyProp: "false", @@ -2479,7 +2479,7 @@ func TestUserAllowsEmail(t *testing.T) { t.Run("should return false in case the MARK_UNREAD_NOTIFY_PROP is CHANNEL_MARK_UNREAD_MENTION", func(t *testing.T) { user := th.CreateUser() - th.App.SetStatusOffline(user.Id, true) + th.App.SetStatusOffline(user.Id, true, false) channelMemberNotificationProps := model.StringMap{ model.EmailNotifyProp: model.ChannelNotifyDefault, @@ -2492,7 +2492,7 @@ func TestUserAllowsEmail(t *testing.T) { t.Run("should return false in case the Post type is POST_AUTO_RESPONDER", func(t *testing.T) { user := th.CreateUser() - th.App.SetStatusOffline(user.Id, true) + th.App.SetStatusOffline(user.Id, true, false) channelMemberNotificationProps := model.StringMap{ model.EmailNotifyProp: model.ChannelNotifyDefault, diff --git a/server/channels/app/platform/helper_test.go b/server/channels/app/platform/helper_test.go index 826a34fca6..28308fde1a 100644 --- a/server/channels/app/platform/helper_test.go +++ b/server/channels/app/platform/helper_test.go @@ -46,7 +46,7 @@ type mockSuite struct { } func (ms *mockSuite) SetStatusLastActivityAt(userID string, activityAt int64) {} -func (ms *mockSuite) SetStatusOffline(userID string, manual bool) {} +func (ms *mockSuite) SetStatusOffline(userID string, manual bool, force bool) {} func (ms *mockSuite) IsUserAway(lastActivityAt int64) bool { return false } func (ms *mockSuite) SetStatusOnline(userID string, manual bool) {} func (ms *mockSuite) UpdateLastActivityAtIfNeeded(session model.Session) {} diff --git a/server/channels/app/platform/status.go b/server/channels/app/platform/status.go index 8047240964..93f567299c 100644 --- a/server/channels/app/platform/status.go +++ b/server/channels/app/platform/status.go @@ -352,13 +352,13 @@ func (ps *PlatformService) SetStatusOnline(userID string, manual bool) { } } -func (ps *PlatformService) SetStatusOffline(userID string, manual bool) { +func (ps *PlatformService) SetStatusOffline(userID string, manual bool, force bool) { if !*ps.Config().ServiceSettings.EnableUserStatuses { return } status, err := ps.GetStatus(userID) - if err == nil && status.Manual && !manual { + if !force && err == nil && status.Manual && !manual { return // manually set status always overrides non-manual one } diff --git a/server/channels/app/platform/status_test.go b/server/channels/app/platform/status_test.go index 7f791d555a..95da146887 100644 --- a/server/channels/app/platform/status_test.go +++ b/server/channels/app/platform/status_test.go @@ -52,3 +52,128 @@ func TestTruncateDNDEndTime(t *testing.T) { // 2025-Jan-20 at 00:00:10 GMT remains unchanged assert.Equal(t, int64(1737331200), truncateDNDEndTime(1737331200)) } + +func TestSetStatusOffline(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + user := th.BasicUser + + t.Run("when user statuses are disabled", func(t *testing.T) { + th.Service.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserStatuses = false + }) + + // Set initial status to online + status := &model.Status{ + UserId: user.Id, + Status: model.StatusOnline, + } + th.Service.SaveAndBroadcastStatus(status) + + // Try to set offline + th.Service.SetStatusOffline(user.Id, false, false) + + // Enable user statuses to see what is really in the database + th.Service.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserStatuses = true + }) + + // Status should remain unchanged + after, err := th.Service.GetStatus(user.Id) + require.Nil(t, err) + assert.Equal(t, model.StatusOnline, after.Status) + }) + + t.Run("when setting status manually over manually set status", func(t *testing.T) { + th.Service.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserStatuses = true + }) + + // Set initial status to online manually + status := &model.Status{ + UserId: user.Id, + Status: model.StatusOnline, + Manual: true, + } + th.Service.SaveAndBroadcastStatus(status) + + // Try to set offline non-manually + th.Service.SetStatusOffline(user.Id, false, false) + + // Status should remain unchanged because manual status takes precedence + after, err := th.Service.GetStatus(user.Id) + require.Nil(t, err) + assert.Equal(t, model.StatusOnline, after.Status) + assert.True(t, after.Manual) + }) + + t.Run("when force flag is true over manually set status", func(t *testing.T) { + th.Service.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserStatuses = true + }) + + // Set initial status to online manually + status := &model.Status{ + UserId: user.Id, + Status: model.StatusOnline, + Manual: true, + } + th.Service.SaveAndBroadcastStatus(status) + + // Try to set offline with force flag + th.Service.SetStatusOffline(user.Id, false, true) + + // Status should change despite being manual + after, err := th.Service.GetStatus(user.Id) + require.Nil(t, err) + assert.Equal(t, model.StatusOffline, after.Status) + assert.False(t, after.Manual) + }) + + t.Run("when setting status normally", func(t *testing.T) { + th.Service.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserStatuses = true + }) + + // Set initial status to online + status := &model.Status{ + UserId: user.Id, + Status: model.StatusOnline, + Manual: false, + } + th.Service.SaveAndBroadcastStatus(status) + + // Set offline + th.Service.SetStatusOffline(user.Id, false, false) + + // Status should change + after, err := th.Service.GetStatus(user.Id) + require.Nil(t, err) + assert.Equal(t, model.StatusOffline, after.Status) + assert.False(t, after.Manual) + }) + + t.Run("when setting status manually over normal status", func(t *testing.T) { + th.Service.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserStatuses = true + }) + + // Set initial status to online + status := &model.Status{ + UserId: user.Id, + Status: model.StatusOnline, + Manual: false, + } + th.Service.SaveAndBroadcastStatus(status) + + // Set offline manually + th.Service.SetStatusOffline(user.Id, true, false) + + // Status should change and be marked as manual + after, err := th.Service.GetStatus(user.Id) + require.Nil(t, err) + assert.Equal(t, model.StatusOffline, after.Status) + assert.True(t, after.Manual) + }) +} diff --git a/server/channels/app/platform/web_hub.go b/server/channels/app/platform/web_hub.go index d88fe19501..4ed0ec6855 100644 --- a/server/channels/app/platform/web_hub.go +++ b/server/channels/app/platform/web_hub.go @@ -580,7 +580,7 @@ func (h *Hub) Start() { // Only set to offline if there are no // active connections in other nodes as well. if clusterCnt == 0 { - h.platform.SetStatusOffline(userID, false) + h.platform.SetStatusOffline(userID, false, false) } }) continue @@ -704,7 +704,7 @@ func (h *Hub) Start() { case <-h.stop: for webConn := range connIndex.All() { webConn.Close() - h.platform.SetStatusOffline(webConn.UserId, false) + h.platform.SetStatusOffline(webConn.UserId, false, false) } h.explicitStop = true diff --git a/server/channels/app/plugin_api.go b/server/channels/app/plugin_api.go index 2bc6020c40..39b3384048 100644 --- a/server/channels/app/plugin_api.go +++ b/server/channels/app/plugin_api.go @@ -367,7 +367,7 @@ func (api *PluginAPI) UpdateUserStatus(userID, status string) (*model.Status, *m case model.StatusOnline: api.app.SetStatusOnline(userID, true) case model.StatusOffline: - api.app.SetStatusOffline(userID, true) + api.app.SetStatusOffline(userID, true, false) case model.StatusAway: api.app.SetStatusAwayIfNeeded(userID, true) case model.StatusDnd: diff --git a/server/channels/app/slashcommands/command_offline.go b/server/channels/app/slashcommands/command_offline.go index 8b013084cd..9c5c0ccb75 100644 --- a/server/channels/app/slashcommands/command_offline.go +++ b/server/channels/app/slashcommands/command_offline.go @@ -35,7 +35,7 @@ func (*OfflineProvider) GetCommand(a *app.App, T i18n.TranslateFunc) *model.Comm } func (*OfflineProvider) DoCommand(a *app.App, c request.CTX, args *model.CommandArgs, message string) *model.CommandResponse { - a.SetStatusOffline(args.UserId, true) + a.SetStatusOffline(args.UserId, true, false) return &model.CommandResponse{ResponseType: model.CommandResponseTypeEphemeral, Text: args.T("api.command_offline.success")} } diff --git a/server/channels/app/status.go b/server/channels/app/status.go index d5d3cd47bc..ad7d099e66 100644 --- a/server/channels/app/status.go +++ b/server/channels/app/status.go @@ -28,8 +28,8 @@ func (a *App) SetStatusOnline(userID string, manual bool) { a.Srv().Platform().SetStatusOnline(userID, manual) } -func (a *App) SetStatusOffline(userID string, manual bool) { - a.Srv().Platform().SetStatusOffline(userID, manual) +func (a *App) SetStatusOffline(userID string, manual bool, force bool) { + a.Srv().Platform().SetStatusOffline(userID, manual, force) } func (a *App) SetStatusAwayIfNeeded(userID string, manual bool) { diff --git a/server/channels/app/user.go b/server/channels/app/user.go index b78985b054..914f827655 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -959,7 +959,7 @@ func (a *App) UpdatePasswordAsUser(c request.CTX, userID, currentPassword, newPa } func (a *App) userDeactivated(c request.CTX, userID string) *model.AppError { - a.SetStatusOffline(userID, false) + a.SetStatusOffline(userID, false, true) user, err := a.GetUser(userID) if err != nil {