From eea016f2becaea97aa86cd971804c43dd87c2830 Mon Sep 17 00:00:00 2001 From: Byungjin Park Date: Sun, 5 Jul 2026 21:37:47 +0900 Subject: [PATCH 1/2] feat(users): multi-select (Space/V) + bulk lifecycle actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MW-8 — operators can now offboard N users in one gesture instead of running :deactivate N times. Space toggles selection on the cursor row, V enters Vim-style visual multi-select where j/k extends the range, and Esc drops both the visual mode and the pinned set. The SEL chrome badge stamps the count so the state is always visible. When ≥1 rows are pinned, opening the status picker or firing a palette write command (:deactivate, :suspend, :delete, …) routes into a new bulk confirm modal that reads "Deactivate 5 users?" and previews the first 3 logins with an "… and N more" trailer. The bulk runner dispatches the port calls sequentially with a 50ms throttle so a large batch doesn't spike the tenant, then emits a summary toast + clears the selection. Selection also clears on user-triggered refresh (R / post-action) and on screen change (list model is torn down). Single-select (no rows pinned) still routes through the existing single-user confirm path so nothing about the current daily flow changes. --- internal/app/actions.go | 107 +++++++ internal/app/app.go | 145 ++++++++- internal/app/user_bulk_actions_test.go | 303 ++++++++++++++++++ internal/tui/overlay/overlay.go | 6 + .../testdata/golden/help_screen_users.txt | 26 +- internal/tui/shared/msgs.go | 7 + internal/tui/users/list.go | 193 ++++++++++- internal/tui/users/multiselect_test.go | 234 ++++++++++++++ 8 files changed, 1001 insertions(+), 20 deletions(-) create mode 100644 internal/app/user_bulk_actions_test.go create mode 100644 internal/tui/users/multiselect_test.go diff --git a/internal/app/actions.go b/internal/app/actions.go index 7e78f9d..0c8d326 100644 --- a/internal/app/actions.go +++ b/internal/app/actions.go @@ -6,6 +6,8 @@ package app import ( "context" + "strconv" + "time" tea "github.com/charmbracelet/bubbletea" @@ -16,11 +18,25 @@ import ( // for the currently-selected user. Falls back to a transient toast // when no Users target is available (e.g., the operator fired the // command from a non-Users screen). +// +// MW-8 — when the active screen publishes a non-empty +// SelectedUsers() (multi-select set), the confirm targets that +// entire set. Otherwise falls back to the single-user cursor row +// via SelectedUserStater. func (m Model) openActionConfirm(kind UserActionKind) (tea.Model, tea.Cmd) { child, ok := m.screens[m.active] if !ok { return m, toastCmdInfo("no active screen") } + if bs, ok := child.(BulkSelectedUsersStater); ok { + if users := bs.SelectedUsers(); len(users) > 0 { + m.pendingBulkAction = pendingBulkUserAction{Kind: kind, Users: users} + m.pendingAction = pendingUserAction{} + m.pendingRule = pendingRuleAction{} + m.overlay = OverlayActionConfirm + return m, nil + } + } stater, ok := child.(SelectedUserStater) if !ok { return m, toastCmdInfo("action not available on this screen") @@ -30,6 +46,7 @@ func (m Model) openActionConfirm(kind UserActionKind) (tea.Model, tea.Cmd) { return m, toastCmdInfo("no user selected") } m.pendingAction = pendingUserAction{Kind: kind, User: user} + m.pendingBulkAction = pendingBulkUserAction{} m.pendingRule = pendingRuleAction{} m.overlay = OverlayActionConfirm return m, nil @@ -215,6 +232,96 @@ func runUserActionCmd(port domain.UsersPort, action pendingUserAction) tea.Cmd { } } +// bulkSleep is overridable so tests can run the bulk runner without +// waiting on the real 50ms per step. Defaults to time.Sleep. +var bulkSleep = time.Sleep + +// SetBulkSleepForTests overrides the inter-step sleep. Tests call it +// with a no-op to keep runs sub-millisecond; production keeps the +// 50ms throttle that spaces port calls. +func SetBulkSleepForTests(f func(time.Duration)) { bulkSleep = f } + +// bulkUserCompletedMsg is the internal signal fired at the end of a +// bulk run. Handled by the App Shell as: emit summary toast + +// broadcast shared.ClearSelectionMsg via tea.Batch (which drops the +// active list's pinned rows). +type bulkUserCompletedMsg struct { + summary string +} + +// runBulkUserActionCmd dispatches an entire pendingBulkUserAction +// against the UsersPort — one port call per user with a 50 ms sleep +// in between so a bulk deactivate on N users hits the API at a +// steady pace rather than a spike (MW-8). Errors are counted and +// rolled up into the summary — one failure doesn't abort the loop +// (unlike single-user runUserActionCmd which surfaces a red toast). +// +// Returns a single Cmd that runs the loop synchronously inside its +// own goroutine (Bubbletea drains Cmds off the main loop). The +// summary lands as a bulkUserCompletedMsg the App Shell expands into +// toast + selection clear. +func runBulkUserActionCmd(port domain.UsersPort, action pendingBulkUserAction) tea.Cmd { + if port == nil { + return toastCmdInfo("UsersPort not wired — action skipped") + } + total := len(action.Users) + if total == 0 { + return nil + } + return func() tea.Msg { + label := userActionLabel(action.Kind) + ctx := context.Background() + var ok, failed int + for i, u := range action.Users { + if i > 0 { + bulkSleep(50 * time.Millisecond) + } + if err := runSingleUserAction(ctx, port, action.Kind, u.ID); err != nil { + failed++ + } else { + ok++ + } + } + var summary string + if failed == 0 { + summary = label + " completed for " + strconv.Itoa(ok) + " users" + } else { + summary = label + " completed · " + strconv.Itoa(ok) + " ok, " + + strconv.Itoa(failed) + " failed" + } + return bulkUserCompletedMsg{summary: summary} + } +} + +// runSingleUserAction is the shared per-user dispatcher used by both +// the single-user runUserActionCmd path and the bulk runner. Any +// lifecycle Kind that reaches the UsersPort has its route here so +// the two paths can't drift. +func runSingleUserAction(ctx context.Context, port domain.UsersPort, kind UserActionKind, id string) error { + switch kind { + case UserActionResetPassword: + _, err := port.ResetPassword(ctx, id, true) + return err + case UserActionUnlock: + return port.Unlock(ctx, id) + case UserActionResetFactors: + return port.ResetFactors(ctx, id) + case UserActionActivate: + return port.Activate(ctx, id, true) + case UserActionDeactivate: + return port.Deactivate(ctx, id, false) + case UserActionExpirePassword: + return port.ExpirePassword(ctx, id) + case UserActionDelete: + return port.Delete(ctx, id) + case UserActionSuspend: + return port.Suspend(ctx, id) + case UserActionUnsuspend: + return port.Unsuspend(ctx, id) + } + return nil +} + // runRuleActionCmd dispatches the active pendingRule against the // GroupRulesPort and emits a toast with the result. func runRuleActionCmd(port domain.GroupRulesPort, action pendingRuleAction) tea.Cmd { diff --git a/internal/app/app.go b/internal/app/app.go index 6bb74ae..6f698c4 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -259,6 +259,16 @@ type pendingUserAction struct { User domain.User } +// pendingBulkUserAction is the multi-target sibling — populated when +// the active screen publishes ≥1 rows in SelectedUsers() (MW-8). One +// of pendingAction / pendingBulkAction is set at any time; the +// confirm modal renders whichever is populated and the runner +// dispatches sequentially against the whole slice. +type pendingBulkUserAction struct { + Kind UserActionKind + Users []domain.User +} + // RuleActionKind classifies the Group Rule lifecycle action a // confirmation modal is gating (issue #188 v0.2.2). Mirrors // UserActionKind for the Group Rules screen — Activate / @@ -343,6 +353,14 @@ type SelectedUserStater interface { SelectedUser() (domain.User, bool) } +// BulkSelectedUsersStater is implemented by screens that publish an +// explicit multi-select set (MW-8). openActionConfirm consults it +// before the single-user path — non-empty result routes into the +// bulk runner, empty falls back to SelectedUserStater. +type BulkSelectedUsersStater interface { + SelectedUsers() []domain.User +} + // Deps bundles the App Shell's runtime dependencies. type Deps struct { Services *service.Bundle @@ -457,6 +475,11 @@ type Model struct { // when the modal closes either way. pendingAction pendingUserAction + // pendingBulkAction is the multi-target sibling (MW-8). At most + // one of pendingAction / pendingBulkAction is set at a time — + // the confirm modal renders whichever holds a non-zero Kind. + pendingBulkAction pendingBulkUserAction + // pendingRule is the Group Rule counterpart of pendingAction // (issue #188 v0.2.2). Only one of pendingAction / pendingRule // is set at any time; the confirmation modal renders whichever @@ -709,6 +732,17 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.openRuleActionConfirm(RuleActionDelete) } return m, nil + case bulkUserCompletedMsg: + // MW-8 — bulk runner finished. Fan out: summary toast (info), + // selection clear so the active list drops its pinned rows, + // and a refresh so the list reflects the new server state + // without waiting for the auto-tick. + toast := toastInfo(msg.summary) + return m, tea.Batch( + func() tea.Msg { return toast }, + func() tea.Msg { return shared.ClearSelectionMsg{} }, + refreshScreenCmd(), + ) case actionCompletedMsg: // Issue #192 v0.2.3 — destructive op finished. Surface the // toast AND fan out a screen refresh so the list / detail @@ -1542,6 +1576,12 @@ func (m Model) renderQuitConfirmModal(tk shared.Tokens) string { // v0.2.4). Mirrors the format the palette help and quit-confirm use, // so the visual language stays consistent. func (m Model) renderActionConfirmModal(tk shared.Tokens) string { + // MW-8 — bulk wording splices a per-login preview onto a + // dedicated modal path so operators see exactly who the confirm + // hits before the port fires. + if m.pendingBulkAction.Kind != UserActionNone { + return m.renderBulkActionConfirmModal(tk) + } var label, target string switch { case m.pendingRule.Kind != RuleActionNone: @@ -1596,6 +1636,58 @@ func (m Model) renderActionConfirmModal(tk shared.Tokens) string { }) } +// renderBulkActionConfirmModal is the MW-8 sibling of the single- +// user confirm — the headline reads "Deactivate 5 users?" and the +// body previews the first 3 logins followed by "… and N more" when +// the set is larger. Operators see exactly who the confirm will hit +// before the port fires. +func (m Model) renderBulkActionConfirmModal(tk shared.Tokens) string { + label := userActionLabel(m.pendingBulkAction.Kind) + users := m.pendingBulkAction.Users + n := len(users) + suffix := "users" + if n == 1 { + suffix = "user" + } + headline := tk.Danger.Render(label) + " for " + + tk.Accent.Render(itoaSimple(n)+" "+suffix) + "?" + var b strings.Builder + b.WriteString(headline) + preview := 3 + if preview > n { + preview = n + } + for i := 0; i < preview; i++ { + login := users[i].Profile.Login + if login == "" { + login = users[i].ID + } + b.WriteString("\n • ") + b.WriteString(login) + } + if n > preview { + b.WriteString("\n " + tk.Muted.Render("… and "+itoaSimple(n-preview)+" more")) + } + body := b.String() + width := 60 + for _, line := range strings.Split(body, "\n") { + if w := shared.VisibleWidth(line) + 6; w > width { + width = w + } + } + if cap := clampWidth(m.width) - 8; cap > 0 && width > cap { + width = cap + } + return shared.MountModal(shared.ModalIn{ + Title: "Confirm bulk action", + Body: body, + Footer: "y / Enter to confirm — n / Esc to cancel", + Tone: shared.ModalToneDanger, + Width: width, + Tokens: tk, + }) +} + // centerInBody horizontally centers a multi-line block inside the // chrome's content width. Each line gets enough leading spaces to push // the block to the visual center; lines wider than contentWidth are @@ -1900,6 +1992,17 @@ func (m Model) composeChromeBadges() []shared.ChromeBadge { var out []shared.ChromeBadge if m.overlay == OverlayActionConfirm { switch { + case m.pendingBulkAction.Kind != UserActionNone: + // MW-8 — the ACTION badge stamps the target count so + // operators glance at the chrome and read "5 users about + // to be deactivated" without reading the modal body. + label := userActionLabel(m.pendingBulkAction.Kind) + + " (" + itoaSimple(len(m.pendingBulkAction.Users)) + ")" + out = append(out, shared.ChromeBadge{ + Key: "ACTION", + Value: label, + Tone: shared.BadgeDanger, + }) case m.pendingRule.Kind != RuleActionNone: out = append(out, shared.ChromeBadge{ Key: "ACTION", @@ -2839,6 +2942,18 @@ func (m Model) handlePaletteKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m.openActionConfirm(UserActionUnlock) case paletteCmdResetFactors: return m.openActionConfirm(UserActionResetFactors) + case paletteCmdActivate: + return m.openActionConfirm(UserActionActivate) + case paletteCmdDeactivate: + return m.openActionConfirm(UserActionDeactivate) + case paletteCmdExpirePassword: + return m.openActionConfirm(UserActionExpirePassword) + case paletteCmdDelete: + return m.openActionConfirm(UserActionDelete) + case paletteCmdSuspend: + return m.openActionConfirm(UserActionSuspend) + case paletteCmdUnsuspend: + return m.openActionConfirm(UserActionUnsuspend) case paletteCmdPolicyType: // Issue #165: jump straight to the typed list, replacing // any existing Policies wrapper so the picker doesn't @@ -2927,6 +3042,8 @@ func paletteCommandPool() []string { "administrator", "unmask", "mask", "reset-password", "unlock", "reset-mfa", + "activate", "deactivate", "expire-password", "delete", + "suspend", "unsuspend", // REQ-W01: `:edit` is the canonical SCR-012 palette entry // (TUI_DESIGN §3.4 / §11.2a). Surfaced in autocomplete so // operators discover it via Tab. @@ -3000,6 +3117,12 @@ func (m Model) handleOverlayKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { // refresh after the action completes so the operator // sees the new state without waiting for the next // auto-tick. + if m.pendingBulkAction.Kind != UserActionNone { + ba := m.pendingBulkAction + m.pendingBulkAction = pendingBulkUserAction{} + m.overlay = OverlayNone + return m, runBulkUserActionCmd(m.deps.UsersPort, ba) + } if m.pendingRule.Kind != RuleActionNone { ra := m.pendingRule m.pendingRule = pendingRuleAction{} @@ -3030,6 +3153,7 @@ func (m Model) handleOverlayKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, runUserActionCmd(m.deps.UsersPort, action) case cancelled: m.pendingAction = pendingUserAction{} + m.pendingBulkAction = pendingBulkUserAction{} m.pendingRule = pendingRuleAction{} m.pendingPolicy = pendingPolicyAction{} m.pendingApp = pendingAppAction{} @@ -3207,10 +3331,17 @@ const ( paletteCmdUnmask paletteCmdMask // Users lifecycle actions (issue #125). Each opens a confirmation - // modal targeting the active screen's selected user. + // modal targeting the active screen's selected user (or the + // multi-select set — MW-8). paletteCmdResetPassword paletteCmdUnlock paletteCmdResetFactors + paletteCmdActivate + paletteCmdDeactivate + paletteCmdExpirePassword + paletteCmdDelete + paletteCmdSuspend + paletteCmdUnsuspend // paletteCmdPolicyType opens ScreenPolicies straight on a list // for the given PolicyType — issue #165 (`:okta-sign-on`, // `:password-policy`, etc.). @@ -3277,6 +3408,18 @@ func resolvePaletteCommand(raw string) (kind paletteCmdKind, screen Screen, arg return paletteCmdUnlock, 0, "", true case "reset-mfa", "reset-factors", "reset_mfa", "reset_factors", "resetfactors": return paletteCmdResetFactors, 0, "", true + case "activate": + return paletteCmdActivate, 0, "", true + case "deactivate": + return paletteCmdDeactivate, 0, "", true + case "expire-password", "expire_password", "expirepassword": + return paletteCmdExpirePassword, 0, "", true + case "delete": + return paletteCmdDelete, 0, "", true + case "suspend": + return paletteCmdSuspend, 0, "", true + case "unsuspend": + return paletteCmdUnsuspend, 0, "", true case "apilog", "api-log", "api_log", "apitimeline": return paletteCmdAPILog, 0, "", true case "xray", "x-ray", "x_ray": diff --git a/internal/app/user_bulk_actions_test.go b/internal/app/user_bulk_actions_test.go new file mode 100644 index 0000000..43a4ead --- /dev/null +++ b/internal/app/user_bulk_actions_test.go @@ -0,0 +1,303 @@ +package app_test + +// MW-8 — bulk lifecycle actions integration. Exercises the whole +// pipeline: multi-select on the Users list → `:deactivate` palette +// → bulk confirm modal → sequential port dispatch → completion +// signal + selection clear. + +import ( + "context" + "testing" + "time" + + tea "github.com/charmbracelet/bubbletea" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/tedilabs/ota/internal/app" + "github.com/tedilabs/ota/internal/clock" + "github.com/tedilabs/ota/internal/domain" + "github.com/tedilabs/ota/internal/keys" + "github.com/tedilabs/ota/internal/tui/shared" +) + +// bulkRecordingPort records every lifecycle call so bulk tests can +// assert BOTH the count and the argument order the runner dispatched. +type bulkRecordingPort struct { + users []domain.User + deactivateCalls []string + deactivateErrIdx int // when > 0, the N-th call (1-based) returns an error +} + +func (p *bulkRecordingPort) List(_ context.Context, _ domain.UsersQuery) (domain.Iterator[domain.User], error) { + return &recordingIter{remaining: p.users}, nil +} +func (p *bulkRecordingPort) Get(_ context.Context, id string) (domain.User, error) { + for _, u := range p.users { + if u.ID == id || u.Profile.Login == id { + return u, nil + } + } + return domain.User{}, domain.ErrNotFound +} +func (p *bulkRecordingPort) ListGroups(_ context.Context, _ string) ([]domain.Group, error) { + return nil, nil +} +func (p *bulkRecordingPort) ListFactors(_ context.Context, _ string) ([]domain.Factor, error) { + return nil, nil +} +func (p *bulkRecordingPort) ListAppLinks(_ context.Context, _ string) ([]domain.AppLink, error) { + return nil, nil +} +func (p *bulkRecordingPort) ResetPassword(_ context.Context, _ string, _ bool) (string, error) { + return "", nil +} +func (p *bulkRecordingPort) Unlock(_ context.Context, _ string) error { return nil } +func (p *bulkRecordingPort) ResetFactors(_ context.Context, _ string) error { return nil } +func (p *bulkRecordingPort) Activate(_ context.Context, _ string, _ bool) error { + return nil +} +func (p *bulkRecordingPort) Deactivate(_ context.Context, id string, _ bool) error { + p.deactivateCalls = append(p.deactivateCalls, id) + return nil +} +func (p *bulkRecordingPort) ExpirePassword(_ context.Context, _ string) error { return nil } +func (p *bulkRecordingPort) Suspend(_ context.Context, _ string) error { return nil } +func (p *bulkRecordingPort) Unsuspend(_ context.Context, _ string) error { return nil } +func (p *bulkRecordingPort) Delete(_ context.Context, _ string) error { return nil } +func (p *bulkRecordingPort) UpdateProfile(_ context.Context, _ string, _ domain.UserProfilePatch) (domain.User, error) { + return domain.User{}, nil +} + +func init() { + // Bulk runner defaults to 50ms between calls (spec) — but no + // integration test needs to actually wait. Neuter the sleep so + // the runner completes instantaneously; the timing is a live- + // server concern, not a unit-test one. + app.SetBulkSleepForTests(func(time.Duration) {}) +} + +// newAppWithBulk seeds the App Shell with the Users screen already +// showing a 3-user list. +func newAppWithBulk(t *testing.T, port domain.UsersPort) app.Model { + t.Helper() + keymap, _, err := keys.Resolve(nil) + require.NoError(t, err) + m := app.New(app.Deps{ + Keys: keymap, + Clock: clock.Real(), + Profile: "test", + OrgURL: "https://acme.okta.com", + UsersPort: port, + }) + if init := m.Init(); init != nil { + if msg := init(); msg != nil { + updated, _ := m.Update(msg) + m = updated.(app.Model) + } + } + // Make sure the Users child sees a WindowSizeMsg so its layout + // initialises the visible slice. + updated, _ := m.Update(tea.WindowSizeMsg{Width: 120, Height: 30}) + return updated.(app.Model) +} + +// pinRows drives Space presses so the first n visible rows land in +// the multi-select set. Assumes cursor starts at row 0. +func pinRows(t *testing.T, m app.Model, n int) app.Model { + t.Helper() + for i := 0; i < n; i++ { + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeySpace}) + m = updated.(app.Model) + if i < n-1 { + updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyDown}) + m = updated.(app.Model) + } + } + return m +} + +// Test_BulkDeactivate_ConfirmWordingRendersCount — with 3 rows +// pinned via Space, `:deactivate` opens the bulk confirm modal that +// reads "Deactivate user for 3 users?" (or the equivalent phrasing +// the App Shell chose) and previews all three logins. +func Test_BulkDeactivate_ConfirmWordingRendersCount(t *testing.T) { + t.Parallel() + + port := &bulkRecordingPort{users: []domain.User{ + {ID: "00u_a", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "a@x.com"}}, + {ID: "00u_b", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "b@x.com"}}, + {ID: "00u_c", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "c@x.com"}}, + }} + m := newAppWithBulk(t, port) + m = pinRows(t, m, 3) + + m = typePalette(t, m, "deactivate") + + view := m.View() + assert.Contains(t, view, "3 users", + "bulk confirm modal must surface the pinned count in the headline") + assert.Contains(t, view, "a@x.com", + "bulk confirm modal must preview the first pinned login") + assert.Contains(t, view, "c@x.com", + "bulk confirm modal must include every login when the set fits in the preview") + assert.Contains(t, view, "Deactivate", + "bulk confirm modal must carry the action label") +} + +// Test_BulkDeactivate_TrailerWhenOver3Rows — the preview lists the +// first 3 logins and appends "… and N more" when the set is larger. +func Test_BulkDeactivate_TrailerWhenOver3Rows(t *testing.T) { + t.Parallel() + + port := &bulkRecordingPort{users: []domain.User{ + {ID: "00u_a", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "a@x.com"}}, + {ID: "00u_b", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "b@x.com"}}, + {ID: "00u_c", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "c@x.com"}}, + {ID: "00u_d", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "d@x.com"}}, + {ID: "00u_e", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "e@x.com"}}, + }} + m := newAppWithBulk(t, port) + m = pinRows(t, m, 5) + + m = typePalette(t, m, "deactivate") + + view := m.View() + assert.Contains(t, view, "5 users", + "headline must carry the full count") + assert.Contains(t, view, "and 2 more", + "preview must trail with the remaining count when > 3 rows pinned") + // e@x.com is legitimately visible under the modal (the App Shell + // dims the body but keeps peek-through cells around the popup), + // so we can't assert its absence from the whole View — but it + // must NOT appear inside the modal's own preview lines. + assert.NotContains(t, view, "• e@x.com", + "the preview bullet list must stop at 3 entries — e@x.com should only be visible through the dimmed body, never bulleted inside the modal") +} + +// Test_BulkDeactivate_ConfirmYFiresPortForEveryRow — pressing y on +// the bulk confirm runs the sequence: Deactivate must be called +// once per pinned user, in visible-list order. +func Test_BulkDeactivate_ConfirmYFiresPortForEveryRow(t *testing.T) { + t.Parallel() + + port := &bulkRecordingPort{users: []domain.User{ + {ID: "00u_a", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "a@x.com"}}, + {ID: "00u_b", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "b@x.com"}}, + {ID: "00u_c", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "c@x.com"}}, + }} + m := newAppWithBulk(t, port) + m = pinRows(t, m, 3) + m = typePalette(t, m, "deactivate") + + updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'y'}}) + m = updated.(app.Model) + require.NotNil(t, cmd, "y on the bulk confirm must fire the runner Cmd") + + // Drain the runner Cmd — it fires every port call synchronously + // inside its goroutine and returns a completion Msg. + _ = cmd() + + assert.Equal(t, []string{"00u_a", "00u_b", "00u_c"}, port.deactivateCalls, + "bulk runner must dispatch Deactivate once per pinned row, in list order") +} + +// Test_BulkDeactivate_CompletionSignalsClear — the runner's tail +// message drives the App Shell to (a) surface a completion toast, +// (b) broadcast shared.ClearSelectionMsg so the active list drops +// its selection set, and (c) refresh the screen. +func Test_BulkDeactivate_CompletionSignalsClear(t *testing.T) { + t.Parallel() + + port := &bulkRecordingPort{users: []domain.User{ + {ID: "00u_a", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "a@x.com"}}, + {ID: "00u_b", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "b@x.com"}}, + }} + m := newAppWithBulk(t, port) + m = pinRows(t, m, 2) + m = typePalette(t, m, "deactivate") + + updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'y'}}) + m = updated.(app.Model) + require.NotNil(t, cmd) + + // Fire the runner Cmd → returns the bulk-completed Msg. + completionMsg := cmd() + require.NotNil(t, completionMsg, + "bulk runner must emit a completion Msg on exit") + + // Drive the completion Msg through Update; the App Shell fans it + // out as ToastMsg + ClearSelectionMsg + RefreshScreenMsg via + // tea.Batch. Drain the Batch and classify each Msg. + updated2, batched := m.Update(completionMsg) + m = updated2.(app.Model) + require.NotNil(t, batched, "completion Msg must trigger a fan-out Batch") + + var sawToast, sawClear, sawRefresh bool + for _, msg := range drainCmd(batched) { + switch msg.(type) { + case shared.ToastMsg: + sawToast = true + case shared.ClearSelectionMsg: + sawClear = true + case shared.RefreshScreenMsg: + sawRefresh = true + } + } + + assert.True(t, sawToast, "completion must emit a summary ToastMsg") + assert.True(t, sawClear, "completion must emit shared.ClearSelectionMsg") + assert.True(t, sawRefresh, "completion must emit shared.RefreshScreenMsg") +} + +// Test_SingleUser_StillRoutesThroughSingleFlow — with NO rows pinned +// via Space, `:deactivate` behaves exactly as before: the confirm +// modal targets the cursor row (not "N users") and the runner fires +// exactly one port call. +func Test_SingleUser_StillRoutesThroughSingleFlow(t *testing.T) { + t.Parallel() + + port := &bulkRecordingPort{users: []domain.User{ + {ID: "00u_a", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "a@x.com"}}, + {ID: "00u_b", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "b@x.com"}}, + }} + m := newAppWithBulk(t, port) + // No Space / V — SelectedUsers() stays empty. The App Shell must + // fall back to the single-user path. + m = typePalette(t, m, "deactivate") + + view := m.View() + assert.NotContains(t, view, "2 users", + "single-user path must not read the bulk wording") + assert.Contains(t, view, "a@x.com", + "single-user modal targets the cursor row (alice)") + + updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'y'}}) + m = updated.(app.Model) + require.NotNil(t, cmd) + _ = cmd() + + assert.Equal(t, []string{"00u_a"}, port.deactivateCalls, + "single-user path must fire exactly one Deactivate call") +} + +// drainCmd runs cmd() and unwraps any tea.BatchMsg it returns so +// tests can inspect every leaf Msg the shell will process. tea.Batch +// returns a private BatchMsg (`[]tea.Cmd`) — recurse to flatten. +func drainCmd(cmd tea.Cmd) []tea.Msg { + if cmd == nil { + return nil + } + msg := cmd() + if msg == nil { + return nil + } + if bm, ok := msg.(tea.BatchMsg); ok { + var out []tea.Msg + for _, c := range bm { + out = append(out, drainCmd(c)...) + } + return out + } + return []tea.Msg{msg} +} diff --git a/internal/tui/overlay/overlay.go b/internal/tui/overlay/overlay.go index 6833030..0ce0840 100644 --- a/internal/tui/overlay/overlay.go +++ b/internal/tui/overlay/overlay.go @@ -503,6 +503,8 @@ func screenSpecificHelpEntries(screen string) []helpEntry { {"s", "change status (pick from valid transitions)"}, {"x", "XRay: Groups → Rules / Policies / Apps"}, {"l", "open Logs scoped to this user"}, + {"Space", "toggle multi-select on cursor row"}, + {"V", "visual multi-select (j/k extends)"}, {"Shift+S", "sort by STATUS"}, {"Shift+N", "sort by NAME (login)"}, {"Shift+L", "sort by LAST LOGIN"}, @@ -511,6 +513,10 @@ func screenSpecificHelpEntries(screen string) []helpEntry { {":unlock", "clear LOCKED_OUT state"}, {":reset-mfa", "remove enrolled MFA factors"}, {":xray", "XRay tree (uses cursor user)"}, + {":deactivate", "deprovision selected user(s)"}, + {":activate", "activate selected user(s)"}, + {":suspend", "suspend selected user(s)"}, + {":delete", "permanent delete (DEPROVISIONED only)"}, } case "groups": return []helpEntry{ diff --git a/internal/tui/overlay/testdata/golden/help_screen_users.txt b/internal/tui/overlay/testdata/golden/help_screen_users.txt index 0d1c4c0..9bb9e83 100644 --- a/internal/tui/overlay/testdata/golden/help_screen_users.txt +++ b/internal/tui/overlay/testdata/golden/help_screen_users.txt @@ -9,18 +9,20 @@ │ s change status (pick from valid transitions) │ ? this help │ gg / G top / bottom │ :group-rules Group Rules │ │ x XRay: Groups → Rules / Policies / Apps │ ~ Okta API call timeline overlay │ Ctrl-d / Ctrl-u half-page down / up │ :policies Policies │ │ l open Logs scoped to this user │ a resource action menu │ Ctrl-f / Ctrl-b page down / up │ :apps Apps │ -│ Shift+S sort by STATUS │ l open Logs scoped to current resource │ │ :authenticators Authenticators │ -│ Shift+N sort by NAME (login) │ z toggle timezone (Local ↔ UTC) │ │ :logs System Log │ -│ Shift+L sort by LAST LOGIN │ R refresh active screen │ │ :network-zones Network Zones │ -│ Shift+C sort by CREATED / CHANGED │ Esc back · cancel mode · close overlay │ │ :authorization-servers Authorization Servers │ -│ :reset-password send reset-password email │ q close screen / quit (with confirm) │ │ :api-tokens API Tokens │ -│ :unlock clear LOCKED_OUT state │ Ctrl-c soft quit (tail confirm) │ │ :administrators Administrators │ -│ :reset-mfa remove enrolled MFA factors │ Ctrl-l force redraw │ │ :apilog API timeline overlay │ -│ :xray XRay tree (uses cursor user) │ │ │ :tz toggle timezone (Local ↔ UTC) │ -│ │ │ │ :unmask reveal a masked field │ -│ │ │ │ :mask re-mask PII fields │ -│ │ │ │ :help this overlay │ -│ │ │ │ :quit quit ota │ +│ Space toggle multi-select on cursor row │ l open Logs scoped to current resource │ │ :authenticators Authenticators │ +│ V visual multi-select (j/k extends) │ z toggle timezone (Local ↔ UTC) │ │ :logs System Log │ +│ Shift+S sort by STATUS │ R refresh active screen │ │ :network-zones Network Zones │ +│ Shift+N sort by NAME (login) │ Esc back · cancel mode · close overlay │ │ :authorization-servers Authorization Servers │ +│ Shift+L sort by LAST LOGIN │ q close screen / quit (with confirm) │ │ :api-tokens API Tokens │ +│ Shift+C sort by CREATED / CHANGED │ Ctrl-c soft quit (tail confirm) │ │ :administrators Administrators │ +│ :reset-password send reset-password email │ Ctrl-l force redraw │ │ :apilog API timeline overlay │ +│ :unlock clear LOCKED_OUT state │ │ │ :tz toggle timezone (Local ↔ UTC) │ +│ :reset-mfa remove enrolled MFA factors │ │ │ :unmask reveal a masked field │ +│ :xray XRay tree (uses cursor user) │ │ │ :mask re-mask PII fields │ +│ :deactivate deprovision selected user(s) │ │ │ :help this overlay │ +│ :activate activate selected user(s) │ │ │ :quit quit ota │ +│ :suspend suspend selected user(s) │ │ │ │ +│ :delete permanent delete (DEPROVISIONED only) │ │ │ │ │ │ │ close · filter │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ \ No newline at end of file diff --git a/internal/tui/shared/msgs.go b/internal/tui/shared/msgs.go index 62603ab..0a2f270 100644 --- a/internal/tui/shared/msgs.go +++ b/internal/tui/shared/msgs.go @@ -154,6 +154,13 @@ type ActionItem struct { // the users list can emit it without an app→tui→app cycle. type RunUserActionMsg struct{ Kind string } +// ClearSelectionMsg tells list screens to drop any multi-select +// state (MW-8). Emitted by the App Shell after a bulk lifecycle +// action completes successfully so the operator lands on a clean +// list — they intentionally acted on those N rows and the row IDs +// should not stay armed for the next action. +type ClearSelectionMsg struct{} + // RunRuleActionMsg is the Group Rule counterpart to // RunUserActionMsg (issue #188 v0.2.2). Kind ∈ {"activate", // "deactivate", "delete"}. diff --git a/internal/tui/users/list.go b/internal/tui/users/list.go index d538a73..9bfce2e 100644 --- a/internal/tui/users/list.go +++ b/internal/tui/users/list.go @@ -164,6 +164,28 @@ type ListModel struct { // failedAt timestamps the most recent failed action per row id // (#U11 v0.2.4). Drives the RowDanger flash for HighlightWindow. failedAt map[string]time.Time + + // selected is the multi-select set (MW-8). Populated by Space / + // V-visual and consumed by SelectedUsers() so the App Shell can + // fan a bulk lifecycle action across every ID. Cleared on Esc, + // screen change, RefreshScreenMsg, and after a successful bulk + // completion. + selected map[string]struct{} + // bulkVisual is the list-level Vim Visual mode toggled by upper- + // case `V`. While active, j/k extend the selection range from + // bulkVisualAnchor to the current cursor position. Distinct from + // detailVisual which lives inside the detail body. + bulkVisual bool + // bulkVisualAnchor pins the visible-slice index at which V was + // pressed. j/k moves the cursor and the range [anchor, cursor] + // (order-independent) becomes the new visual span. + bulkVisualAnchor int + // bulkVisualBase snapshots `selected` at the moment V was pressed + // so cursor movement recomputes the total set as `base ∪ range`. + // Moving `k` backwards past the anchor properly trims the visual + // range without also unselecting rows the operator had already + // pinned before entering Visual mode. + bulkVisualBase map[string]struct{} } // Fetching implements app.FetchingStater (#U10 v0.2.4). @@ -279,6 +301,22 @@ func (m ListModel) LastUpdated() time.Time { return m.lastUpdated } // read every transient mode in one slot. func (m ListModel) StatusBadges() []shared.ChromeBadge { var out []shared.ChromeBadge + if n := len(m.selected); n > 0 { + // MW-8 — surface the multi-select count so operators always + // see how many rows the next `s` / `:deactivate` will hit. + out = append(out, shared.ChromeBadge{ + Key: "SEL", + Value: strconv.Itoa(n), + Tone: shared.BadgeWarning, + }) + } + if m.bulkVisual { + out = append(out, shared.ChromeBadge{ + Key: "V-SEL", + Value: "on", + Tone: shared.BadgeWarning, + }) + } if m.sortBy != SortNone && m.sortDir != SortOff { out = append(out, shared.ChromeBadge{ Key: "SORT", @@ -318,7 +356,8 @@ func (m ListModel) StatusBadges() []shared.ChromeBadge { // root frame to decide forward-to-screen vs fire-quit-confirm // (2026-05-04 nav-stack rewrite). func (m ListModel) EscapeWillAct() bool { - return m.filtering || m.opened || m.filter != "" || m.detailExtrasFocused || m.detailVisual + return m.filtering || m.opened || m.filter != "" || m.detailExtrasFocused || + m.detailVisual || m.bulkVisual || len(m.selected) > 0 } // usersSortLabelPlain returns the SORT badge value — column name @@ -413,11 +452,25 @@ func (m ListModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.Batch(fetchUsersCmd(m.deps.Port), m.scheduleRefreshTickCmd()) case shared.RefreshScreenMsg: // Issue #192 v0.2.3 — operator-triggered refresh. + // MW-8 — operator-visible refresh (`R`, post-action) also + // drops the multi-select set. Auto-refresh ticks + // (usersRefreshTickMsg) don't hit this branch so a background + // tick can't nuke an in-progress selection. + m.selected = nil + m.bulkVisual = false + m.bulkVisualBase = nil if m.deps.Port == nil { return m, nil } m.fetching = true return m, fetchUsersCmd(m.deps.Port) + case shared.ClearSelectionMsg: + // MW-8 — bulk runner completed; drop selection so the next + // keystroke doesn't accidentally hit the just-processed rows. + m.selected = nil + m.bulkVisual = false + m.bulkVisualBase = nil + return m, nil case OpenDetailByIDMsg: // #G2 / U7 v0.2.4 — cross-screen drill-down from Group // Detail Members box, Log Detail actor row, etc. Fire a @@ -769,6 +822,16 @@ func (m ListModel) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } + // MW-8 — Esc cancels Visual + drops the multi-select set before + // the filter-clear fallback fires. Vim semantics: Esc kicks + // visual first, then the residual selection, then the filter. + if msg.Type == tea.KeyEsc && (m.bulkVisual || len(m.selected) > 0) { + m.bulkVisual = false + m.bulkVisualBase = nil + m.selected = nil + return m, nil + } + // Esc on a list with an active filter clears the filter and // restores the full row set (issue #131). The `/` input is closed // by Enter — at that point m.filtering is false but m.filter still @@ -781,6 +844,45 @@ func (m ListModel) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } + // MW-8 — Space toggles selection on the cursor row. Bubbletea + // delivers Space as either KeySpace (bare) or KeyRunes[" "] + // depending on the terminal; handle both. Toggle is symmetric so + // operators can un-mark a row without leaving the list. + if msg.Type == tea.KeySpace || (msg.Type == tea.KeyRunes && string(msg.Runes) == " ") { + if u := m.cursorUser(); u != nil && u.ID != "" { + if m.selected == nil { + m.selected = map[string]struct{}{} + } + if _, on := m.selected[u.ID]; on { + delete(m.selected, u.ID) + } else { + m.selected[u.ID] = struct{}{} + } + } + return m, nil + } + + // MW-8 — `V` toggles Vim visual selection. On entry the current + // `selected` set is snapshotted so cursor movement recomputes + // `selected = base ∪ [anchor, cursor]`. Moving `k` backwards + // past the anchor properly shrinks the visual span without also + // dropping rows the operator had already pinned with Space. + if msg.Type == tea.KeyRunes && string(msg.Runes) == "V" { + if m.bulkVisual { + m.bulkVisual = false + m.bulkVisualBase = nil + return m, nil + } + m.bulkVisual = true + m.bulkVisualAnchor = m.cursor + m.bulkVisualBase = map[string]struct{}{} + for id := range m.selected { + m.bulkVisualBase[id] = struct{}{} + } + m.applyBulkVisualRange() + return m, nil + } + // Vim page nav (TUI_DESIGN §3.2). Ctrl-f / Ctrl-b move a full page, // Ctrl-d / Ctrl-u move half a page. Page size mirrors the body // row budget so the cursor lands in the same relative spot after a @@ -901,18 +1003,27 @@ func (m ListModel) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil case keys.IDNavDown: m.cursor++ + if m.bulkVisual { + // Clamp before extending so applyBulkVisualRange doesn't + // walk off the end of visible(). + m = m.clampedCursor() + m.applyBulkVisualRange() + } return m, nil case keys.IDNavUp: if m.cursor > 0 { m.cursor-- } + if m.bulkVisual { + m.applyBulkVisualRange() + } return m, nil case keys.IDNavSelect, keys.IDActionDetail: // `Enter` and `d` share the inline detail flow (TUI_DESIGN §3.6). // Both fetch the full user and surface the detail view; v0.1.1 // keeps the routing inside ListModel (Option A) — App Shell-level // OpenResourceMsg routing arrives in v0.2. - sel := m.selected() + sel := m.cursorRowUser() if sel == nil { return m, nil } @@ -1031,8 +1142,14 @@ func (m ListModel) View() string { for i := top; i < end; i++ { row := m.renderUsersRow(rows[i], now, tk) prefix := " " - if i == m.cursor { + _, marked := m.selected[rows[i].ID] + switch { + case i == m.cursor: prefix = "▸ " + case marked: + // MW-8 — pinned row marker. Kept to a 2-cell width so + // the column alignment matches the cursor gutter. + prefix = "▪ " } // v0.2.3 #193 — per-row "just changed" flash. RowChanged // loses to the cursor tint but beats the abnormal-status @@ -2228,8 +2345,11 @@ func userChangedInstant(u domain.User) time.Time { return u.Created } -// selected returns the currently-highlighted user, if any. -func (m ListModel) selected() *domain.User { +// cursorRowUser returns the currently-highlighted user, if any. +// Renamed from `selected()` (MW-8) — the plural `selected` field on +// ListModel is the multi-select set, and the old name collided with +// it. The method still answers "who is the cursor pointing at". +func (m ListModel) cursorRowUser() *domain.User { vs := m.visible() if m.cursor < 0 || m.cursor >= len(vs) { return nil @@ -2272,12 +2392,71 @@ func (m ListModel) SelectedUser() (domain.User, bool) { return m.detailUser, true } } - if u := m.selected(); u != nil { + if u := m.cursorRowUser(); u != nil { return *u, true } return domain.User{}, false } +// SelectedIDs returns the explicit multi-select set (MW-8) in +// visible-list order so the App Shell can preview logins in the +// confirm modal without re-sorting. Empty when no rows are pinned. +func (m ListModel) SelectedIDs() []string { + if len(m.selected) == 0 { + return nil + } + out := make([]string, 0, len(m.selected)) + for _, u := range m.visible() { + if _, ok := m.selected[u.ID]; ok { + out = append(out, u.ID) + } + } + return out +} + +// SelectedUsers is the domain.User counterpart of SelectedIDs — the +// App Shell reads it to hand full snapshots (login for wording, +// status for picker gating) to the bulk runner. +func (m ListModel) SelectedUsers() []domain.User { + if len(m.selected) == 0 { + return nil + } + out := make([]domain.User, 0, len(m.selected)) + for _, u := range m.visible() { + if _, ok := m.selected[u.ID]; ok { + out = append(out, u) + } + } + return out +} + +// applyBulkVisualRange recomputes `selected` as `bulkVisualBase ∪ +// range(anchor, cursor)` inside visible(). Called after every +// cursor move while bulkVisual is on. +func (m *ListModel) applyBulkVisualRange() { + m.selected = map[string]struct{}{} + for id := range m.bulkVisualBase { + m.selected[id] = struct{}{} + } + vis := m.visible() + if len(vis) == 0 { + return + } + start, end := m.bulkVisualAnchor, m.cursor + if start > end { + start, end = end, start + } + if start < 0 { + start = 0 + } + if end >= len(vis) { + end = len(vis) - 1 + } + for i := start; i <= end; i++ { + m.selected[vis[i].ID] = struct{}{} + } +} + // userTrackedEqual reports whether two user snapshots match on every // field the list View renders — when this returns false the diff // pipeline marks the row as "just changed" so RowChanged flashes for @@ -2393,7 +2572,7 @@ func (m ListModel) cursorUser() *domain.User { u := m.detailUser return &u } - return m.selected() + return m.cursorRowUser() } // openLogsForCmd asks the App Shell to switch to Logs and pre-fill diff --git a/internal/tui/users/multiselect_test.go b/internal/tui/users/multiselect_test.go new file mode 100644 index 0000000..b0b4688 --- /dev/null +++ b/internal/tui/users/multiselect_test.go @@ -0,0 +1,234 @@ +package users_test + +// MW-8 — Users list multi-select (Space/V/Esc) + SelectedUsers() + +// SEL chrome badge. Assertions here stop at the ListModel boundary; +// the App Shell → bulk runner integration lives in +// internal/app/user_bulk_actions_test.go. + +import ( + "testing" + + tea "github.com/charmbracelet/bubbletea" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/tedilabs/ota/internal/domain" + "github.com/tedilabs/ota/internal/tui/shared" + "github.com/tedilabs/ota/internal/tui/users" +) + +// listHarness seeds a ListModel with 3 users so Space / V / j / k +// have room to move without touching a UsersPort. +func listHarness(t *testing.T) users.ListModel { + t.Helper() + seed := []domain.User{ + {ID: "00u_alice", Profile: domain.UserProfile{Login: "alice@acme.com"}, Status: domain.UserStatusActive}, + {ID: "00u_bob", Profile: domain.UserProfile{Login: "bob@acme.com"}, Status: domain.UserStatusActive}, + {ID: "00u_carol", Profile: domain.UserProfile{Login: "carol@acme.com"}, Status: domain.UserStatusActive}, + } + return users.NewListModel(users.Deps{ + InitialUsers: seed, + Width: 120, + Height: 30, + }) +} + +func spaceKey() tea.KeyMsg { return tea.KeyMsg{Type: tea.KeySpace} } +func runeKey(r rune) tea.KeyMsg { + return tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{r}} +} +func selBadgeValue(m users.ListModel) string { + for _, b := range m.StatusBadges() { + if b.Key == "SEL" { + return b.Value + } + } + return "" +} + +// Test_MultiSelect_SpaceTogglesCursorRow — Space adds the cursor row +// to SelectedUsers(); pressing Space again removes it. The SEL +// chrome badge tracks the count. +func Test_MultiSelect_SpaceTogglesCursorRow(t *testing.T) { + t.Parallel() + m := listHarness(t) + + // Precondition: nothing selected, SEL badge absent. + require.Empty(t, m.SelectedIDs()) + assert.Empty(t, selBadgeValue(m)) + + // Space on cursor row 0 → alice pinned. + updated, _ := m.Update(spaceKey()) + m = updated.(users.ListModel) + + require.Equal(t, []string{"00u_alice"}, m.SelectedIDs(), + "Space must pin the cursor row's ID into SelectedIDs") + assert.Equal(t, "1", selBadgeValue(m), + "SEL chrome badge must count exactly 1") + + // Space again on the same row → alice unpinned. + updated, _ = m.Update(spaceKey()) + m = updated.(users.ListModel) + + assert.Empty(t, m.SelectedIDs(), + "a second Space on the same row must remove the pin") + assert.Empty(t, selBadgeValue(m), + "SEL badge collapses to empty when the set is empty") +} + +// Test_MultiSelect_SpaceRuneVariant covers the KeyRunes[' '] variant +// some terminals emit instead of tea.KeySpace. +func Test_MultiSelect_SpaceRuneVariant(t *testing.T) { + t.Parallel() + m := listHarness(t) + + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{' '}}) + m = updated.(users.ListModel) + + require.Equal(t, []string{"00u_alice"}, m.SelectedIDs(), + "Space-as-rune must reach the same toggle handler") +} + +// Test_MultiSelect_VEntersVisualJExtends — uppercase V enters +// visual-select mode; j extends the range downward so alice + bob +// end up pinned. +func Test_MultiSelect_VEntersVisualJExtends(t *testing.T) { + t.Parallel() + m := listHarness(t) + + updated, _ := m.Update(runeKey('V')) + m = updated.(users.ListModel) + require.Equal(t, []string{"00u_alice"}, m.SelectedIDs(), + "V-entry pins the anchor row (alice) as a single-row selection") + + updated, _ = m.Update(runeKey('j')) + m = updated.(users.ListModel) + + assert.Equal(t, []string{"00u_alice", "00u_bob"}, m.SelectedIDs(), + "j while V is active must extend the selection to the next row") +} + +// Test_MultiSelect_VVisualKShrinksBackToAnchor — after V j j, pressing +// k unpins the last row but keeps the anchor pinned. Vim visual mode +// treats the range as inclusive of the anchor, so shrinking back to +// the anchor leaves that row alone. +func Test_MultiSelect_VVisualKShrinksBackToAnchor(t *testing.T) { + t.Parallel() + m := listHarness(t) + + // V anchors at alice; j x2 extends to carol. + m = advance(t, m, runeKey('V'), runeKey('j'), runeKey('j')) + require.Equal(t, []string{"00u_alice", "00u_bob", "00u_carol"}, m.SelectedIDs()) + + // k shrinks back to alice + bob. + updated, _ := m.Update(runeKey('k')) + m = updated.(users.ListModel) + + assert.Equal(t, []string{"00u_alice", "00u_bob"}, m.SelectedIDs(), + "k must trim the visual range when moving toward the anchor") +} + +// Test_MultiSelect_EscClearsSelectionAndVisual — Esc drops both the +// bulkVisual mode flag and every pinned row. +func Test_MultiSelect_EscClearsSelectionAndVisual(t *testing.T) { + t.Parallel() + m := listHarness(t) + + m = advance(t, m, runeKey('V'), runeKey('j')) + require.Len(t, m.SelectedIDs(), 2, "precondition: 2 rows pinned via V+j") + + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyEsc}) + m = updated.(users.ListModel) + + assert.Empty(t, m.SelectedIDs(), + "Esc must drop every pinned row — matches Vim visual-mode Esc") + assert.Empty(t, selBadgeValue(m), + "Esc must also clear the SEL badge (badge follows the set)") +} + +// Test_MultiSelect_EscEscapeWillActReturnsTrue — while selection is +// non-empty, the App Shell's EscapeWillAct check must report true so +// Esc doesn't pop the nav frame out from under the operator before +// the local Esc handler runs. +func Test_MultiSelect_EscapeWillActReflectsSelection(t *testing.T) { + t.Parallel() + m := listHarness(t) + require.False(t, m.EscapeWillAct(), + "precondition: empty selection means Esc doesn't do anything local") + + updated, _ := m.Update(spaceKey()) + m = updated.(users.ListModel) + + assert.True(t, m.EscapeWillAct(), + "a non-empty selection must gate Esc so the nav-pop doesn't win") +} + +// Test_MultiSelect_RefreshClearsSelection — RefreshScreenMsg drops +// the multi-select set (user-triggered refresh is a hard reset per +// MW-8 spec). +func Test_MultiSelect_RefreshClearsSelection(t *testing.T) { + t.Parallel() + m := listHarness(t) + m = advance(t, m, spaceKey(), tea.KeyMsg{Type: tea.KeyDown}, spaceKey()) + require.Len(t, m.SelectedIDs(), 2) + + updated, _ := m.Update(shared.RefreshScreenMsg{}) + m = updated.(users.ListModel) + + assert.Empty(t, m.SelectedIDs(), + "RefreshScreenMsg (`R` / post-action) must clear the selection") +} + +// Test_MultiSelect_ClearSelectionMsgClears — the App Shell emits +// shared.ClearSelectionMsg after a successful bulk completion; the +// list drops selection on receipt. +func Test_MultiSelect_ClearSelectionMsgClears(t *testing.T) { + t.Parallel() + m := listHarness(t) + m = advance(t, m, spaceKey()) + require.Len(t, m.SelectedIDs(), 1) + + updated, _ := m.Update(shared.ClearSelectionMsg{}) + m = updated.(users.ListModel) + + assert.Empty(t, m.SelectedIDs()) +} + +// Test_MultiSelect_SelectedUsersReturnsSnapshot — bulk runner needs +// the whole domain.User (login + status), not just IDs. +func Test_MultiSelect_SelectedUsersReturnsSnapshot(t *testing.T) { + t.Parallel() + m := listHarness(t) + m = advance(t, m, spaceKey(), tea.KeyMsg{Type: tea.KeyDown}, spaceKey()) + + users := m.SelectedUsers() + require.Len(t, users, 2) + assert.Equal(t, "alice@acme.com", users[0].Profile.Login) + assert.Equal(t, "bob@acme.com", users[1].Profile.Login) +} + +// Test_MultiSelect_RowPrefixMarksSelected — the row prefix flips to +// `▪ ` for selected rows so operators glance at the list and read +// which rows are armed. Precedence: cursor > selected, so we pin +// alice first, then step cursor to bob before asserting so alice's +// row is visibly marked with the pin prefix. +func Test_MultiSelect_RowPrefixMarksSelected(t *testing.T) { + t.Parallel() + m := listHarness(t) + m = advance(t, m, spaceKey(), tea.KeyMsg{Type: tea.KeyDown}) + + view := m.View() + assert.Contains(t, view, "▪ alice@acme.com", + "selected non-cursor row must render with the ▪ pin prefix") +} + +// advance runs a sequence of key presses against the model. Each +// press updates the model in place; cmds are drained but unused. +func advance(t *testing.T, m users.ListModel, msgs ...tea.KeyMsg) users.ListModel { + t.Helper() + for _, msg := range msgs { + updated, _ := m.Update(msg) + m = updated.(users.ListModel) + } + return m +} From f4ce3a619e0a5b6dfdaba776e33a3ce1fb70e16d Mon Sep 17 00:00:00 2001 From: Byungjin Park Date: Sun, 5 Jul 2026 21:52:12 +0900 Subject: [PATCH 2/2] fix(multiselect): keep filtered-out selections, clamp visual range, name failed logins Address PR #9 review comments on MW-8 (multi-select bulk actions): - SelectedIDs()/SelectedUsers() now iterate `m.users` instead of `m.visible()`, so a client-side `/` filter that hides pinned rows doesn't silently drop them from the bulk target set. The operator's intent is preserved regardless of the display filter. - applyBulkVisualRange() clamps both endpoints into [0, len(vis)-1] before the swap. Clamping after the swap could leave `start > end` when a background refresh shrank the list beneath a stale anchor, dropping the entire selection loop. - Bulk summary now names every failed login: `"1 ok, 2 failed (alice@x.com, carol@x.com)"`. Falls back to the User ID when `Profile.Login` is empty (SCIM edge case) so no slot reads blank. Regression tests cover both scenarios. --- internal/app/actions.go | 13 ++- internal/app/user_bulk_actions_test.go | 106 ++++++++++++++++++++++++- internal/tui/users/list.go | 31 ++++++-- internal/tui/users/multiselect_test.go | 37 +++++++++ 4 files changed, 176 insertions(+), 11 deletions(-) diff --git a/internal/app/actions.go b/internal/app/actions.go index 0c8d326..3f3cbdb 100644 --- a/internal/app/actions.go +++ b/internal/app/actions.go @@ -7,6 +7,7 @@ package app import ( "context" "strconv" + "strings" "time" tea "github.com/charmbracelet/bubbletea" @@ -272,12 +273,21 @@ func runBulkUserActionCmd(port domain.UsersPort, action pendingBulkUserAction) t label := userActionLabel(action.Kind) ctx := context.Background() var ok, failed int + var failedLogins []string for i, u := range action.Users { if i > 0 { bulkSleep(50 * time.Millisecond) } if err := runSingleUserAction(ctx, port, action.Kind, u.ID); err != nil { failed++ + // Fall back to the user ID when the profile has no + // login — SCIM-provisioned edge cases surface bare + // IDs and blank names should never mean "empty". + login := u.Profile.Login + if login == "" { + login = u.ID + } + failedLogins = append(failedLogins, login) } else { ok++ } @@ -287,7 +297,8 @@ func runBulkUserActionCmd(port domain.UsersPort, action pendingBulkUserAction) t summary = label + " completed for " + strconv.Itoa(ok) + " users" } else { summary = label + " completed · " + strconv.Itoa(ok) + " ok, " + - strconv.Itoa(failed) + " failed" + strconv.Itoa(failed) + " failed (" + + strings.Join(failedLogins, ", ") + ")" } return bulkUserCompletedMsg{summary: summary} } diff --git a/internal/app/user_bulk_actions_test.go b/internal/app/user_bulk_actions_test.go index 43a4ead..13f1a05 100644 --- a/internal/app/user_bulk_actions_test.go +++ b/internal/app/user_bulk_actions_test.go @@ -7,6 +7,7 @@ package app_test import ( "context" + "errors" "testing" "time" @@ -24,9 +25,10 @@ import ( // bulkRecordingPort records every lifecycle call so bulk tests can // assert BOTH the count and the argument order the runner dispatched. type bulkRecordingPort struct { - users []domain.User - deactivateCalls []string - deactivateErrIdx int // when > 0, the N-th call (1-based) returns an error + users []domain.User + deactivateCalls []string + deactivateErrIdx int // when > 0, the N-th call (1-based) returns an error + deactivateFailIDs map[string]bool // when non-nil, matching IDs return an error } func (p *bulkRecordingPort) List(_ context.Context, _ domain.UsersQuery) (domain.Iterator[domain.User], error) { @@ -59,6 +61,9 @@ func (p *bulkRecordingPort) Activate(_ context.Context, _ string, _ bool) error } func (p *bulkRecordingPort) Deactivate(_ context.Context, id string, _ bool) error { p.deactivateCalls = append(p.deactivateCalls, id) + if p.deactivateFailIDs[id] { + return errors.New("simulated deactivate failure") + } return nil } func (p *bulkRecordingPort) ExpirePassword(_ context.Context, _ string) error { return nil } @@ -281,6 +286,101 @@ func Test_SingleUser_StillRoutesThroughSingleFlow(t *testing.T) { "single-user path must fire exactly one Deactivate call") } +// Test_BulkDeactivate_SummaryNamesFailedLogins — when the port fails +// on a subset of the pinned users, the completion summary must (a) +// still count ok/failed correctly and (b) name every failed login +// so the operator knows which users to retry. Bare `"3 ok, 2 failed"` +// wording (PR #9 pre-review) forces the operator back to the logs +// to figure out who broke. +func Test_BulkDeactivate_SummaryNamesFailedLogins(t *testing.T) { + t.Parallel() + + port := &bulkRecordingPort{ + users: []domain.User{ + {ID: "00u_a", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "alice@x.com"}}, + {ID: "00u_b", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "bob@x.com"}}, + {ID: "00u_c", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: "carol@x.com"}}, + }, + deactivateFailIDs: map[string]bool{ + "00u_a": true, + "00u_c": true, + }, + } + m := newAppWithBulk(t, port) + m = pinRows(t, m, 3) + m = typePalette(t, m, "deactivate") + + updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'y'}}) + m = updated.(app.Model) + require.NotNil(t, cmd, "y on the bulk confirm must fire the runner Cmd") + + completionMsg := cmd() + require.NotNil(t, completionMsg) + + // Fan-out via Update — the summary toast is one of the batched Msgs. + updated2, batched := m.Update(completionMsg) + _ = updated2 + require.NotNil(t, batched) + + var toastText string + for _, msg := range drainCmd(batched) { + if tm, ok := msg.(shared.ToastMsg); ok { + toastText = tm.Text + } + } + require.NotEmpty(t, toastText, "completion must emit a summary ToastMsg") + + assert.Contains(t, toastText, "1 ok", + "summary must count successful port calls") + assert.Contains(t, toastText, "2 failed", + "summary must count failed port calls") + assert.Contains(t, toastText, "alice@x.com", + "summary must name the first failed login so the operator knows who to retry") + assert.Contains(t, toastText, "carol@x.com", + "summary must name every failed login, not just the first") + assert.NotContains(t, toastText, "bob@x.com", + "summary must NOT list bob — bob's port call succeeded") +} + +// Test_BulkDeactivate_SummaryFallsBackToIDForBlankLogin — SCIM- +// provisioned edge cases can have blank Profile.Login. The failure +// summary must fall back to the User ID rather than emit an empty +// slot. +func Test_BulkDeactivate_SummaryFallsBackToIDForBlankLogin(t *testing.T) { + t.Parallel() + + port := &bulkRecordingPort{ + users: []domain.User{ + {ID: "00u_blank", Status: domain.UserStatusActive, Profile: domain.UserProfile{Login: ""}}, + }, + deactivateFailIDs: map[string]bool{ + "00u_blank": true, + }, + } + m := newAppWithBulk(t, port) + m = pinRows(t, m, 1) + m = typePalette(t, m, "deactivate") + + updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'y'}}) + m = updated.(app.Model) + require.NotNil(t, cmd) + + completionMsg := cmd() + require.NotNil(t, completionMsg) + + _, batched := m.Update(completionMsg) + require.NotNil(t, batched) + + var toastText string + for _, msg := range drainCmd(batched) { + if tm, ok := msg.(shared.ToastMsg); ok { + toastText = tm.Text + } + } + assert.Contains(t, toastText, "00u_blank", + "summary must fall back to the User ID when Profile.Login is empty") +} + // drainCmd runs cmd() and unwraps any tea.BatchMsg it returns so // tests can inspect every leaf Msg the shell will process. tea.Batch // returns a private BatchMsg (`[]tea.Cmd`) — recurse to flatten. diff --git a/internal/tui/users/list.go b/internal/tui/users/list.go index 9bfce2e..b6d0959 100644 --- a/internal/tui/users/list.go +++ b/internal/tui/users/list.go @@ -2399,14 +2399,19 @@ func (m ListModel) SelectedUser() (domain.User, bool) { } // SelectedIDs returns the explicit multi-select set (MW-8) in -// visible-list order so the App Shell can preview logins in the +// fetch-list order so the App Shell can preview logins in the // confirm modal without re-sorting. Empty when no rows are pinned. +// +// Iterates `m.users` (not `m.visible()`) so a client-side `/` +// filter doesn't silently drop already-pinned rows from the bulk +// target set — the operator pinned them explicitly; hiding them +// from view mustn't erase the intent. func (m ListModel) SelectedIDs() []string { if len(m.selected) == 0 { return nil } out := make([]string, 0, len(m.selected)) - for _, u := range m.visible() { + for _, u := range m.users { if _, ok := m.selected[u.ID]; ok { out = append(out, u.ID) } @@ -2416,13 +2421,14 @@ func (m ListModel) SelectedIDs() []string { // SelectedUsers is the domain.User counterpart of SelectedIDs — the // App Shell reads it to hand full snapshots (login for wording, -// status for picker gating) to the bulk runner. +// status for picker gating) to the bulk runner. Iterates `m.users` +// for the same reason as SelectedIDs: filtered-out pins still count. func (m ListModel) SelectedUsers() []domain.User { if len(m.selected) == 0 { return nil } out := make([]domain.User, 0, len(m.selected)) - for _, u := range m.visible() { + for _, u := range m.users { if _, ok := m.selected[u.ID]; ok { out = append(out, u) } @@ -2442,16 +2448,27 @@ func (m *ListModel) applyBulkVisualRange() { if len(vis) == 0 { return } + // Clamp BOTH endpoints into [0, len(vis)-1] BEFORE the swap. + // If the swap happens first and only one side is out of range, + // we can end up with start > end after the one-sided clamp and + // drop the loop entirely (e.g., a background refresh shrinks + // the list beneath a stale anchor). start, end := m.bulkVisualAnchor, m.cursor - if start > end { - start, end = end, start - } if start < 0 { start = 0 } + if start >= len(vis) { + start = len(vis) - 1 + } + if end < 0 { + end = 0 + } if end >= len(vis) { end = len(vis) - 1 } + if start > end { + start, end = end, start + } for i := start; i <= end; i++ { m.selected[vis[i].ID] = struct{}{} } diff --git a/internal/tui/users/multiselect_test.go b/internal/tui/users/multiselect_test.go index b0b4688..bc9a972 100644 --- a/internal/tui/users/multiselect_test.go +++ b/internal/tui/users/multiselect_test.go @@ -207,6 +207,43 @@ func Test_MultiSelect_SelectedUsersReturnsSnapshot(t *testing.T) { assert.Equal(t, "bob@acme.com", users[1].Profile.Login) } +// Test_MultiSelect_FilterHidesSelectedButKeepsInBulkSet — the bug +// from PR #9 review: SelectedIDs() / SelectedUsers() used to iterate +// visible() so a client-side `/` filter would silently drop already- +// pinned rows from the bulk target set. Pin all three rows, then +// filter to just carol — SelectedIDs() must still return every pin. +func Test_MultiSelect_FilterHidesSelectedButKeepsInBulkSet(t *testing.T) { + t.Parallel() + m := listHarness(t) + + // Pin all three users via Space + Down. + m = advance(t, m, + spaceKey(), + tea.KeyMsg{Type: tea.KeyDown}, spaceKey(), + tea.KeyMsg{Type: tea.KeyDown}, spaceKey(), + ) + require.Equal(t, []string{"00u_alice", "00u_bob", "00u_carol"}, m.SelectedIDs(), + "precondition: all three rows pinned before any filter") + + // Open `/` filter and type "carol" so only carol matches visible(). + m = advance(t, m, + runeKey('/'), + runeKey('c'), runeKey('a'), runeKey('r'), runeKey('o'), runeKey('l'), + ) + + // The bulk runner MUST still see all three pinned users — the + // filter is a display concern, not an intent-erasing action. + assert.Equal(t, []string{"00u_alice", "00u_bob", "00u_carol"}, m.SelectedIDs(), + "a client-side filter that hides selected rows must NOT drop them from SelectedIDs") + + users := m.SelectedUsers() + require.Len(t, users, 3, + "SelectedUsers() must mirror SelectedIDs() — filtered-out pins still ship to the bulk runner") + assert.Equal(t, "alice@acme.com", users[0].Profile.Login) + assert.Equal(t, "bob@acme.com", users[1].Profile.Login) + assert.Equal(t, "carol@acme.com", users[2].Profile.Login) +} + // Test_MultiSelect_RowPrefixMarksSelected — the row prefix flips to // `▪ ` for selected rows so operators glance at the list and read // which rows are armed. Precedence: cursor > selected, so we pin