From 7434f994925110c25fbbce17fc8a62816d606ef7 Mon Sep 17 00:00:00 2001 From: "cursor[bot]" <206951365+cursor[bot]@users.noreply.github.com> Date: Fri, 3 Jul 2026 17:31:31 -0400 Subject: [PATCH] [MM-69688] Fix /share-channel status always showing "--" for Last Sync (#37345) * [MM-69688] Fix /share-channel status Last Sync always showing -- GetRemotesStatus never selected the sync cursor columns, so SharedChannelRemoteStatus.NextSyncAt was always the zero value and formatTimestamp rendered "--". Select GREATEST of the LastPostUpdateAt and LastMembersSyncAt cursors as LastSyncAt and render that field. Co-authored-by: mattermost-code * [MM-69688] Cover LastSyncAt in GetRemotesStatus store test Advance the post and members sync cursors and assert GetRemotesStatus reports LastSyncAt as the greater of the two, and reports zero when a remote has never synced. Co-authored-by: mattermost-code * [MM-69688] Add command and formatTimestamp coverage for Last Sync Add doStatus command-layer tests asserting the /share-channel status output renders a timestamp for a synced remote and "--" for an unsynced one, a formatTimestamp unit test for the zero/today/older branches, and harden the store test so both GREATEST operands are non-zero. Co-authored-by: mattermost-code * [MM-69688] Assert the non-today timestamp format in formatTimestamp test Co-authored-by: mattermost-code * Trigger CI re-run after transient Postgres shard 3 Docker Hub failure Co-authored-by: mattermost-code * [MM-69688] Include LastPostCreateAt in Last Sync computation --------- Co-authored-by: Cursor Agent Co-authored-by: mattermost-code --- .../app/slashcommands/command_share.go | 2 +- .../app/slashcommands/command_share_test.go | 81 +++++++++++++++++++ .../channels/app/slashcommands/util_test.go | 23 ++++++ .../store/sqlstore/shared_channel_store.go | 2 +- .../store/storetest/shared_channel_store.go | 61 ++++++++++++++ server/public/model/shared_channel.go | 2 +- 6 files changed, 168 insertions(+), 3 deletions(-) diff --git a/server/channels/app/slashcommands/command_share.go b/server/channels/app/slashcommands/command_share.go index 57e7b4cec912..c71c3d3f5a7b 100644 --- a/server/channels/app/slashcommands/command_share.go +++ b/server/channels/app/slashcommands/command_share.go @@ -301,7 +301,7 @@ func (sp *ShareProvider) doStatus(a *app.App, args *model.CommandArgs, _ map[str accepted := formatBool(args.T, status.IsInviteAccepted) online := formatBool(args.T, isOnline(status.LastPingAt)) - lastSync := formatTimestamp(status.NextSyncAt) + lastSync := formatTimestamp(status.LastSyncAt) fmt.Fprintf(&sb, "| %s | %s | %s | %s | %s | %s |\n", status.DisplayName, status.SiteURL, readonly, accepted, online, lastSync) diff --git a/server/channels/app/slashcommands/command_share_test.go b/server/channels/app/slashcommands/command_share_test.go index 2a8a967a155c..1ccfecd68c72 100644 --- a/server/channels/app/slashcommands/command_share_test.go +++ b/server/channels/app/slashcommands/command_share_test.go @@ -127,6 +127,87 @@ func TestShareProviderDoCommand(t *testing.T) { }) } +func TestShareProviderDoStatus(t *testing.T) { + seedInvitedRemote := func(t *testing.T, th *TestHelper, channelID string) *model.SharedChannelRemote { + t.Helper() + rc, err := th.App.AddRemoteCluster(&model.RemoteCluster{ + RemoteId: model.NewId(), + Name: "remote-" + model.NewId(), + DisplayName: "Remote Status Display", + SiteURL: "https://remote-status.example.com", + Token: model.NewId(), + Topics: "topic", + CreateAt: model.GetMillis(), + LastPingAt: model.GetMillis(), + CreatorId: model.NewId(), + }) + require.Nil(t, err) + + scr, saveErr := th.App.SaveSharedChannelRemote(&model.SharedChannelRemote{ + Id: model.NewId(), + ChannelId: channelID, + RemoteId: rc.RemoteId, + CreatorId: th.BasicUser.Id, + IsInviteAccepted: true, + IsInviteConfirmed: true, + }) + require.NoError(t, saveErr) + return scr + } + + statusArgs := func(th *TestHelper, channelID string) *model.CommandArgs { + return &model.CommandArgs{ + T: func(s string, args ...any) string { return s }, + ChannelId: channelID, + UserId: th.BasicUser.Id, + TeamId: th.BasicTeam.Id, + Command: "/share-channel status", + } + } + + t.Run("renders the last sync timestamp for a remote that has synced", func(t *testing.T) { + th := setupForSharedChannels(t).initBasic(t) + th.addPermissionToRole(t, model.PermissionManageSharedChannels.Id, th.BasicUser.Roles) + + mockSyncService := app.NewMockSharedChannelService(th.Server.GetSharedChannelSyncService()) + th.Server.SetSharedChannelSyncService(mockSyncService) + + channel := th.CreateChannel(t, th.BasicTeam, WithShared(true)) + scr := seedInvitedRemote(t, th, channel.Id) + + syncAt := model.GetMillis() - 5000 + require.NoError(t, th.App.Srv().Store().SharedChannel().UpdateRemoteCursor(scr.Id, model.GetPostsSinceForSyncCursor{ + LastPostUpdateAt: syncAt, + LastPostUpdateID: model.NewId(), + })) + + commandProvider := ShareProvider{} + response := commandProvider.DoCommand(th.App, th.Context, statusArgs(th, channel.Id), "") + + require.Contains(t, response.Text, "Remote Status Display") + assert.Contains(t, response.Text, "Today ", "expected a rendered timestamp in the Last Sync column, got:\n%s", response.Text) + assert.NotContains(t, response.Text, "| -- |", "Last Sync column should not be the placeholder for a synced remote:\n%s", response.Text) + }) + + t.Run("renders a placeholder when the remote has never synced", func(t *testing.T) { + th := setupForSharedChannels(t).initBasic(t) + th.addPermissionToRole(t, model.PermissionManageSharedChannels.Id, th.BasicUser.Roles) + + mockSyncService := app.NewMockSharedChannelService(th.Server.GetSharedChannelSyncService()) + th.Server.SetSharedChannelSyncService(mockSyncService) + + channel := th.CreateChannel(t, th.BasicTeam, WithShared(true)) + seedInvitedRemote(t, th, channel.Id) + + commandProvider := ShareProvider{} + response := commandProvider.DoCommand(th.App, th.Context, statusArgs(th, channel.Id), "") + + require.Contains(t, response.Text, "Remote Status Display") + assert.Contains(t, response.Text, "| -- |", "expected the Last Sync placeholder for an unsynced remote, got:\n%s", response.Text) + assert.NotContains(t, response.Text, "Today ", "unsynced remote should not render a timestamp:\n%s", response.Text) + }) +} + func TestShareProviderGetAutoCompleteListItemsPermission(t *testing.T) { connectionIDArg := func() *model.AutocompleteArg { return &model.AutocompleteArg{Name: "connectionID"} diff --git a/server/channels/app/slashcommands/util_test.go b/server/channels/app/slashcommands/util_test.go index ad55494b99d7..cd419eb7ee93 100644 --- a/server/channels/app/slashcommands/util_test.go +++ b/server/channels/app/slashcommands/util_test.go @@ -4,9 +4,13 @@ package slashcommands import ( + "strings" "testing" + "time" "github.com/stretchr/testify/assert" + + "github.com/mattermost/mattermost/server/public/model" ) func TestParseNamedArgs(t *testing.T) { @@ -38,3 +42,22 @@ func TestParseNamedArgs(t *testing.T) { assert.Equal(t, tt.m, m, tt.name) } } + +func TestFormatTimestamp(t *testing.T) { + t.Run("zero renders as placeholder", func(t *testing.T) { + assert.Equal(t, "--", formatTimestamp(0)) + }) + + t.Run("today renders with a Today prefix", func(t *testing.T) { + got := formatTimestamp(model.GetMillis()) + assert.True(t, strings.HasPrefix(got, "Today "), "expected a Today prefix, got %q", got) + }) + + t.Run("an earlier day renders the full date without the placeholder", func(t *testing.T) { + twoDaysAgo := model.GetMillis() - int64(48*time.Hour/time.Millisecond) + got := formatTimestamp(twoDaysAgo) + assert.NotEqual(t, "--", got) + assert.False(t, strings.HasPrefix(got, "Today "), "expected a full date, got %q", got) + assert.Equal(t, model.GetTimeForMillis(twoDaysAgo).Format("Jan 2 15:04:05 MST 2006"), got) + }) +} diff --git a/server/channels/store/sqlstore/shared_channel_store.go b/server/channels/store/sqlstore/shared_channel_store.go index a20ac8f91dfe..429d87979f0a 100644 --- a/server/channels/store/sqlstore/shared_channel_store.go +++ b/server/channels/store/sqlstore/shared_channel_store.go @@ -679,7 +679,7 @@ func (s SqlSharedChannelStore) GetRemotesStatus(channelId string) ([]*model.Shar status := []*model.SharedChannelRemoteStatus{} query := s.getQueryBuilder(). - Select("scr.ChannelId, scr.RemoteId, rc.DisplayName, rc.SiteURL, rc.LastPingAt, sc.ReadOnly, scr.IsInviteAccepted"). + Select("scr.ChannelId, scr.RemoteId, rc.DisplayName, rc.SiteURL, rc.LastPingAt, sc.ReadOnly, scr.IsInviteAccepted, GREATEST(COALESCE(scr.LastPostCreateAt, 0), COALESCE(scr.LastPostUpdateAt, 0), COALESCE(scr.LastMembersSyncAt, 0)) AS LastSyncAt"). From("SharedChannelRemotes scr, RemoteClusters rc, SharedChannels sc"). Where("scr.RemoteId = rc.RemoteId"). Where("scr.DeleteAt = 0"). diff --git a/server/channels/store/storetest/shared_channel_store.go b/server/channels/store/storetest/shared_channel_store.go index b7be4cfe4beb..4695588ed64b 100644 --- a/server/channels/store/storetest/shared_channel_store.go +++ b/server/channels/store/storetest/shared_channel_store.go @@ -815,6 +815,37 @@ func testGetRemotesStatus(t *testing.T, rctx request.CTX, ss store.Store) { _, err = ss.SharedChannel().SaveRemote(scr2) require.NoError(t, err) + // Advance the sync cursors so the reported LastSyncAt reflects real activity. + // scr1's members sync is newer than its post sync, so LastSyncAt must track the + // members cursor. scr2's post-create cursor is the newest of all three cursors, + // so LastSyncAt must track it. The post-create cursor advances independently of + // the post-update cursor (see GetPostsSinceForSync), so it can be the most recent + // sync activity and must be one of the GREATEST operands. + now := model.GetMillis() + scr1PostSyncAt := now - 5000 + scr1MembersSyncAt := now - 1000 + scr2PostCreateAt := now - 500 + scr2PostUpdateAt := now - 3000 + scr2MembersSyncAt := now - 7000 + + err = ss.SharedChannel().UpdateRemoteCursor(scr1.Id, model.GetPostsSinceForSyncCursor{ + LastPostUpdateAt: scr1PostSyncAt, + LastPostUpdateID: model.NewId(), + }) + require.NoError(t, err) + err = ss.SharedChannel().UpdateRemoteMembershipCursor(scr1.Id, scr1MembersSyncAt) + require.NoError(t, err) + + err = ss.SharedChannel().UpdateRemoteCursor(scr2.Id, model.GetPostsSinceForSyncCursor{ + LastPostCreateAt: scr2PostCreateAt, + LastPostCreateID: model.NewId(), + LastPostUpdateAt: scr2PostUpdateAt, + LastPostUpdateID: model.NewId(), + }) + require.NoError(t, err) + err = ss.SharedChannel().UpdateRemoteMembershipCursor(scr2.Id, scr2MembersSyncAt) + require.NoError(t, err) + t.Run("Get remotes status for channel", func(t *testing.T) { statuses, err := ss.SharedChannel().GetRemotesStatus(channel.Id) require.NoError(t, err) @@ -833,6 +864,8 @@ func testGetRemotesStatus(t *testing.T, rctx request.CTX, ss store.Store) { assert.Equal(t, rc1.DisplayName, s1.DisplayName) assert.Equal(t, rc1.SiteURL, s1.SiteURL) assert.True(t, s1.IsInviteAccepted) + // LastSyncAt is the greater of the post and members cursors. + assert.Equal(t, scr1MembersSyncAt, s1.LastSyncAt) s2 := statusMap[rc2.RemoteId] require.NotNil(t, s2) @@ -841,6 +874,34 @@ func testGetRemotesStatus(t *testing.T, rctx request.CTX, ss store.Store) { assert.Equal(t, rc2.DisplayName, s2.DisplayName) assert.Equal(t, rc2.SiteURL, s2.SiteURL) assert.False(t, s2.IsInviteAccepted) + // The post-create cursor is the newest cursor, so LastSyncAt tracks it. If the + // post-create cursor were excluded from GREATEST, this would report the older + // post-update cursor instead. + assert.Equal(t, scr2PostCreateAt, s2.LastSyncAt) + }) + + t.Run("Reports zero last sync when the remote has never synced", func(t *testing.T) { + unsyncedChannel, err := createTestChannel(ss, rctx, "test_remotes_status_unsynced") + require.NoError(t, err) + + _, scErr := shareChannel(ss, unsyncedChannel, true, "") + require.NoError(t, scErr) + + scrUnsynced := &model.SharedChannelRemote{ + Id: model.NewId(), + ChannelId: unsyncedChannel.Id, + RemoteId: rc1.RemoteId, + CreatorId: rc1.CreatorId, + IsInviteAccepted: true, + IsInviteConfirmed: true, + } + _, err = ss.SharedChannel().SaveRemote(scrUnsynced) + require.NoError(t, err) + + statuses, err := ss.SharedChannel().GetRemotesStatus(unsyncedChannel.Id) + require.NoError(t, err) + require.Len(t, statuses, 1) + assert.Zero(t, statuses[0].LastSyncAt) }) t.Run("Get remotes status for channel with no remotes", func(t *testing.T) { diff --git a/server/public/model/shared_channel.go b/server/public/model/shared_channel.go index 2c9ab8d1460d..c1a3c46ed8b1 100644 --- a/server/public/model/shared_channel.go +++ b/server/public/model/shared_channel.go @@ -167,7 +167,7 @@ type SharedChannelRemoteStatus struct { DisplayName string `json:"display_name"` SiteURL string `json:"site_url"` LastPingAt int64 `json:"last_ping_at"` - NextSyncAt int64 `json:"next_sync_at"` + LastSyncAt int64 `json:"last_sync_at"` ReadOnly bool `json:"readonly"` IsInviteAccepted bool `json:"is_invite_accepted"` Token string `json:"token"`