[MM-69691] Fix /secure-connection status ordering and malformed table header (#37347)

* [MM-69691] Fix /secure-connection status table sorting and header

Sort deleted connections to the bottom (active first, ordered by
CreateAt within each group) and remove the spurious empty cell from
the Markdown table header separator so it has 9 cells matching the
header and data rows.

Co-authored-by: mattermost-code <matty-code@mattermost.com>

* [MM-69691] Add test for /secure-connection status output

Verifies deleted connections sort to the bottom (active first, ordered
by CreateAt within each group) and that the Markdown table header
separator has exactly nine cells matching the columns.

Co-authored-by: mattermost-code <matty-code@mattermost.com>

* [MM-69691] Strengthen /secure-connection status test assertions

Assert the separator row equals the exact nine-cell string and has
nine alignment cells, verify data rows have nine cells, and cover
CreateAt ordering (overriding alphabetical) plus stable ordering for
equal CreateAt within a group.

Co-authored-by: mattermost-code <matty-code@mattermost.com>

* [MM-69691] Use strings.SplitSeq in status test to satisfy linter

Co-authored-by: mattermost-code <matty-code@mattermost.com>

* [MM-69691] Retrigger CI after buildenv artifact infra failure

Co-authored-by: mattermost-code <matty-code@mattermost.com>

* [MM-69691] Retrigger Enterprise CI after npm ECONNRESET flake

Co-authored-by: mattermost-code <matty-code@mattermost.com>

* Add missing return in doStatus error path

Address review feedback from @wiggin77: the error branch in doStatus
built an error response but discarded it, so execution fell through to
the empty-list check on a nil list. Return the response instead.

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: mattermost-code <matty-code@mattermost.com>
This commit is contained in:
cursor[bot]
2026-07-04 16:16:11 -04:00
committed by GitHub
co-authored by Cursor Agent mattermost-code
parent 5433e6eef9
commit ce23427d98
2 changed files with 119 additions and 2 deletions
@@ -6,6 +6,7 @@ package slashcommands
import (
"errors"
"fmt"
"sort"
"strings"
"github.com/mattermost/mattermost/server/public/model"
@@ -221,17 +222,27 @@ func (rp *RemoteProvider) doRemove(a *app.App, args *model.CommandArgs, margs ma
func (rp *RemoteProvider) doStatus(a *app.App, args *model.CommandArgs, _ map[string]string) *model.CommandResponse {
list, err := a.GetAllRemoteClusters(0, 999999, model.RemoteClusterQueryFilter{IncludeDeleted: true})
if err != nil {
response(args.T("api.command_remote.fetch_status.error", map[string]any{"Error": err.Error()}))
return response(args.T("api.command_remote.fetch_status.error", map[string]any{"Error": err.Error()}))
}
if len(list) == 0 {
return response("** " + args.T("api.command_remote.remotes_not_found") + " **")
}
// Show active connections first, then deleted ones, ordered by creation time within each group.
sort.SliceStable(list, func(i, j int) bool {
iDeleted := list[i].DeleteAt != 0
jDeleted := list[j].DeleteAt != 0
if iDeleted != jDeleted {
return !iDeleted
}
return list[i].CreateAt < list[j].CreateAt
})
var sb strings.Builder
fmt.Fprintf(&sb, "%s \n", args.T("api.command_remote.remote_table_header"))
// | Secure Connection | Display name | ConnectionID | Site URL | Default Team | Invite accepted | Online | Last ping | Deleted |
fmt.Fprintf(&sb, "| :---- | :---- | :---- | :---- | :---- | :---- | :---- | :---- | | :---- |\n")
fmt.Fprintf(&sb, "| :---- | :---- | :---- | :---- | :---- | :---- | :---- | :---- | :---- |\n")
for _, rc := range list {
accepted := formatBool(args.T, rc.IsConfirmed())
@@ -0,0 +1,106 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.
package slashcommands
import (
"strings"
"testing"
"github.com/stretchr/testify/require"
"github.com/mattermost/mattermost/server/public/model"
)
func TestRemoteProviderDoStatus(t *testing.T) {
th := setupForSharedChannels(t).initBasic(t)
th.addPermissionToRole(t, model.PermissionManageSecureConnections.Id, th.BasicUser.Roles)
// seedRemote creates a remote cluster. When deleted is true it is soft-deleted
// after creation so it carries a non-zero DeleteAt, mirroring a removed connection.
seedRemote := func(t *testing.T, displayName string, createAt int64, deleted bool) {
t.Helper()
rc, appErr := th.App.AddRemoteCluster(&model.RemoteCluster{
RemoteId: model.NewId(),
Name: "remote-" + model.NewId(),
DisplayName: displayName,
SiteURL: "https://" + model.NewId() + ".example.com",
Token: model.NewId(),
CreateAt: createAt,
CreatorId: th.BasicUser.Id,
})
require.Nil(t, appErr)
if deleted {
_, appErr = th.App.DeleteRemoteCluster(rc.RemoteId)
require.Nil(t, appErr)
}
}
// The store returns rows ordered by DisplayName (see sqlRemoteClusterStore.GetAll),
// so without the fix the rows come back interleaved: AAA, BBB, CCC, DDD.
//
// CreateAt values are chosen so the fix's behavior is unambiguous:
// - Active group: CCC (100) must sort before AAA (300), proving the CreateAt
// ordering overrides the store's alphabetical order.
// - Deleted group: BBB and DDD share a CreateAt, so the stable sort must
// preserve their store order (BBB before DDD).
// Expected final order: CCC, AAA, BBB, DDD.
seedRemote(t, "AAA Active", 300, false)
seedRemote(t, "BBB Deleted", 200, true)
seedRemote(t, "CCC Active", 100, false)
seedRemote(t, "DDD Deleted", 200, true)
args := &model.CommandArgs{
T: func(s string, args ...any) string { return s },
UserId: th.BasicUser.Id,
TeamId: th.BasicTeam.Id,
ChannelId: th.BasicChannel.Id,
Command: "/secure-connection status",
}
resp := (&RemoteProvider{}).DoCommand(th.App, th.Context, args, "")
require.NotNil(t, resp)
output := resp.Text
// separatorLine returns the Markdown table separator row (the line made up of
// alignment cells) from the command output.
separatorLine := func() string {
for line := range strings.SplitSeq(output, "\n") {
if strings.HasPrefix(strings.TrimSpace(line), "| :----") {
return line
}
}
return ""
}
t.Run("header separator has exactly nine cells matching the columns", func(t *testing.T) {
sep := separatorLine()
require.NotEmpty(t, sep, "expected a Markdown separator row in the output")
require.Equal(t, "| :---- | :---- | :---- | :---- | :---- | :---- | :---- | :---- | :---- |", sep,
"separator row must have exactly nine cells to match the nine-column header and data rows")
require.Equal(t, 9, strings.Count(sep, ":----"), "separator must contain exactly nine alignment cells")
})
t.Run("data rows have nine cells matching the header", func(t *testing.T) {
for line := range strings.SplitSeq(output, "\n") {
if strings.Contains(line, "AAA Active") {
// A row with 9 cells is delimited by 10 pipe characters.
require.Equal(t, 10, strings.Count(line, "|"), "each data row must have nine cells")
return
}
}
require.Fail(t, "expected to find the AAA Active data row in the output")
})
t.Run("active connections sort before deleted ones, by CreateAt then stably", func(t *testing.T) {
rowOrder := []string{"CCC Active", "AAA Active", "BBB Deleted", "DDD Deleted"}
lastIdx := -1
for _, name := range rowOrder {
idx := strings.Index(output, name)
require.GreaterOrEqual(t, idx, 0, "expected %q to appear in the status output", name)
require.Greater(t, idx, lastIdx, "expected %q to appear after the previous row", name)
lastIdx = idx
}
})
}