diff --git a/frontend/src/__tests__/history-approval-queue.test.ts b/frontend/src/__tests__/history-approval-queue.test.ts index 45d1378bb..bb0b8b939 100644 --- a/frontend/src/__tests__/history-approval-queue.test.ts +++ b/frontend/src/__tests__/history-approval-queue.test.ts @@ -58,10 +58,15 @@ jest.mock('../state', () => ({ subscribeAccount: jest.fn().mockReturnValue(() => {}), })); +jest.mock('../recommendations', () => ({ + getAccountName: jest.fn((id: string) => id), +})); + import * as api from '../api'; import { confirmDialog } from '../confirmDialog'; import { showToast } from '../toast'; import { getCurrentUser } from '../state'; +import { getAccountName } from '../recommendations'; const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', role: 'admin' }; @@ -263,4 +268,128 @@ describe('Approval queue card (issue #340 sub-task)', () => { expect(queue.querySelectorAll('.history-approve-btn[data-approve-id="p-pending"]').length).toBe(1); expect(list.querySelectorAll('.history-approve-btn[data-approve-id="p-pending"]').length).toBe(1); }); + + // Issue #704 — new column tests + + test('renders email in Created-by column when created_by_user_email is set', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ + purchase_id: 'p-email', + status: 'pending', + created_by_user_id: 'some-uuid', + created_by_user_email: 'alice@example.com', + }), + ], + }); + + await loadHistory(); + + const queue = document.getElementById('purchases-approval-queue')!; + expect(queue.textContent).toContain('alice@example.com'); + // Raw UUID must NOT appear when email is available. + expect(queue.textContent).not.toContain('some-uuid'); + }); + + test('falls back to UUID in Created-by column when email is absent', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ + purchase_id: 'p-uuid', + status: 'pending', + created_by_user_id: 'fallback-uuid', + // created_by_user_email intentionally absent + }), + ], + }); + + await loadHistory(); + + const queue = document.getElementById('purchases-approval-queue')!; + expect(queue.textContent).toContain('fallback-uuid'); + }); + + test('renders Account column via getAccountName when account_id is set', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (getAccountName as jest.Mock).mockReturnValue('Production Account'); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ + purchase_id: 'p-acct', + status: 'pending', + account_id: '123456789012', + }), + ], + }); + + await loadHistory(); + + const queue = document.getElementById('purchases-approval-queue')!; + expect(getAccountName).toHaveBeenCalledWith('123456789012'); + expect(queue.textContent).toContain('Production Account'); + }); + + test('renders Term and Payment columns', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ + purchase_id: 'p-term', + status: 'pending', + term: 3, + payment: 'no-upfront', + }), + ], + }); + + await loadHistory(); + + const queue = document.getElementById('purchases-approval-queue')!; + // formatTerm mock renders "3 Years" + expect(queue.textContent).toContain('3 Years'); + expect(queue.textContent).toContain('no-upfront'); + }); + + test('renders Monthly Cost column when monthly_cost is present', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ + purchase_id: 'p-monthly', + status: 'pending', + monthly_cost: 42.5, + }), + ], + }); + + await loadHistory(); + + const queue = document.getElementById('purchases-approval-queue')!; + // formatCurrency mock renders ".5" + expect(queue.textContent).toContain('.5'); + }); + + test('renders Account/Term/Payment/Monthly column headers', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ status: 'pending', created_by_user_id: ADMIN_USER.id })], + }); + + await loadHistory(); + + const queue = document.getElementById('purchases-approval-queue')!; + const headerText = queue.querySelector('thead')?.textContent || ''; + expect(headerText).toContain('Account'); + expect(headerText).toContain('Term'); + expect(headerText).toContain('Payment'); + expect(headerText).toContain('Monthly Cost'); + }); }); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 9287bde6b..8d2ba7a84 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -12,6 +12,7 @@ import { buildApprovalDetailsBody } from './approval-details'; import { showToast } from './toast'; import { getCurrentUser } from './state'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; +import { getAccountName } from './recommendations'; const VALID_PROVIDERS: api.Provider[] = ['aws', 'azure', 'gcp']; @@ -859,14 +860,32 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { const rows = pending.map(p => { const actions = renderPendingActionButtons(p); const actionsCell = actions || '-'; - const createdBy = p.created_by_user_id ? escapeHtml(p.created_by_user_id) : '-'; + // Show email when resolved; fall back to UUID so the cancel-own gate still + // has something human-readable to show. Fall back to "-" for scheduler rows. + const createdBy = p.created_by_user_email + ? escapeHtml(p.created_by_user_email) + : p.created_by_user_id + ? escapeHtml(p.created_by_user_id) + : '-'; + const accountCell = p.account_id + ? escapeHtml(getAccountName(p.account_id)) + : '-'; + const termCell = p.term ? escapeHtml(formatTerm(p.term)) : '-'; + const paymentCell = p.payment ? escapeHtml(p.payment) : '-'; + const monthlyCostCell = p.monthly_cost != null + ? formatCurrency(p.monthly_cost) + : '-'; const execIdAttr = p.purchase_id ? ` data-execution-id="${escapeHtml(p.purchase_id)}"` : ''; return ` ${formatDate(p.timestamp)} + ${accountCell} ${providerCell(p)} ${escapeHtml(p.service)} ${p.count} + ${termCell} + ${paymentCell} + ${monthlyCostCell} ${formatCurrency(p.upfront_cost)} ${formatCurrency(p.estimated_savings)} ${createdBy} @@ -880,9 +899,13 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { Date + Account Provider Service Count + Term + Payment + Monthly Cost Upfront Cost Monthly Savings Created by diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index fc069ffcb..17af005dd 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -37,6 +37,16 @@ let checkedPurchaseModalInitialised = false; // Cache of account ID → name for column display let accountNamesCache: Map = new Map(); +/** + * Returns the display name for a cloud account ID, falling back to the raw + * ID when the name has not been cached yet. Exported so other modules (e.g. + * history.ts Approval Queue) can resolve account names without importing the + * full recommendations data model. + */ +export function getAccountName(accountId: string): string { + return accountNamesCache.get(accountId) || accountId; +} + // issues #225 + #226: expand/collapse state for cell grouping. // Contains the cellKey strings of cells the user has explicitly expanded. // Cleared on page load / full refresh; survives per-column filter/sort re-renders. diff --git a/frontend/src/types.ts b/frontend/src/types.ts index c0be7afe0..0c9c66e61 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -180,8 +180,11 @@ export interface HistoryPurchase { region: string; count: number; term: number; + payment?: string; upfront_cost: number; + monthly_cost?: number; estimated_savings: number; + account_id?: string; plan_name?: string; // Status is set by the API to "completed" or "pending". Legacy pre-schema // rows come back without it; the UI treats absent status as completed for @@ -195,6 +198,12 @@ export interface HistoryPurchase { // non-ok rows. "failed" → backend's send-error message; "expired" → canned // 7-day-window reminder. Empty on completed / pending rows. status_description?: string; + // CreatedByUserEmail: resolved email of the user who created the + // underlying execution. Populated on synthesised execution rows when the + // auth lookup succeeds. Empty for scheduler-driven rows, legacy NULL- + // creator rows, and completed purchase_history rows. The Approval Queue + // renders this instead of the raw UUID when present. + created_by_user_email?: string; // CreatedByUserID: UUID of the user who created the underlying execution. // Populated on every synthesised purchase_executions row the History // endpoint returns (pending, notified, failed, expired, cancelled — see diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 33a460a4a..d6bea04d8 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -128,6 +128,7 @@ func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyF return nil } approver := h.resolvePendingApproverEmail(ctx) + userEmailCache := h.resolveUserEmails(ctx, executions) out := make([]config.PurchaseHistoryRecord, 0, len(executions)) for _, exec := range executions { // Dedup: a normal completed execution is already represented by its @@ -142,7 +143,36 @@ func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyF if !filters.matchesExecution(exec) { continue } - out = append(out, executionToHistoryRow(exec, approver)) + var createdByEmail string + if exec.CreatedByUserID != nil { + createdByEmail = userEmailCache[*exec.CreatedByUserID] + } + out = append(out, executionToHistoryRow(exec, approver, createdByEmail)) + } + return out +} + +// resolveUserEmails builds a map of user-ID to email by calling GetUser once +// per unique non-nil CreatedByUserID found in the execution list. Lookup +// failures are logged and skipped — a missing email degrades gracefully (the +// UI falls back to the raw UUID via created_by_user_id). Called once per +// /api/history request so the cost is proportional to the number of distinct +// creators, not the number of execution rows. +func (h *Handler) resolveUserEmails(ctx context.Context, executions []config.PurchaseExecution) map[string]string { + seen := make(map[string]struct{}) + for _, exec := range executions { + if exec.CreatedByUserID != nil && *exec.CreatedByUserID != "" { + seen[*exec.CreatedByUserID] = struct{}{} + } + } + out := make(map[string]string, len(seen)) + for uid := range seen { + user, err := h.auth.GetUser(ctx, uid) + if err != nil { + logging.Warnf("history: failed to resolve email for user %s: %v", uid, err) + continue + } + out[uid] = user.Email } return out } @@ -189,8 +219,9 @@ func (h *Handler) resolvePendingApproverEmail(ctx context.Context) string { // For pending/notified rows we attach the approver email so the UI can show // "awaiting approval from X"; for failed rows we surface the stored Error // message as the status description so the user sees WHY it failed (e.g. -// "send failed: Missing domain"). -func executionToHistoryRow(exec config.PurchaseExecution, approver string) config.PurchaseHistoryRecord { +// "send failed: Missing domain"). createdByEmail is the resolved email for +// the execution's creator (empty when not resolvable). +func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEmail string) config.PurchaseHistoryRecord { var accountID string if exec.CloudAccountID != nil { accountID = *exec.CloudAccountID @@ -204,16 +235,17 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver string) confi retryExecID = *exec.RetryExecutionID } row := config.PurchaseHistoryRecord{ - AccountID: accountID, - PurchaseID: exec.ExecutionID, - Timestamp: exec.ScheduledDate, - Provider: collapseRecommendationProvider(exec.Recommendations), - Count: len(exec.Recommendations), - PlanID: exec.PlanID, - Status: exec.Status, - CreatedByUserID: createdBy, - RetryExecutionID: retryExecID, - RetryAttemptN: exec.RetryAttemptN, + AccountID: accountID, + PurchaseID: exec.ExecutionID, + Timestamp: exec.ScheduledDate, + Provider: collapseRecommendationProvider(exec.Recommendations), + Count: len(exec.Recommendations), + PlanID: exec.PlanID, + Status: exec.Status, + CreatedByUserID: createdBy, + CreatedByUserEmail: createdByEmail, + RetryExecutionID: retryExecID, + RetryAttemptN: exec.RetryAttemptN, } projectRecommendationFields(&row, exec) // Compute ops_hint at read time (issue #47, Q3) so updates to the diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 0a70556fd..3d138cc52 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -1040,6 +1040,79 @@ func TestParseHistoryDateRange(t *testing.T) { }) } +// TestHandler_getHistory_CreatedByUserEmailResolved verifies that when a +// pending execution carries a non-nil CreatedByUserID, the returned history row +// includes the resolved email address in CreatedByUserEmail so the Approval +// Queue can show a name instead of a UUID. A lookup failure must degrade +// gracefully (email field stays empty, row still renders, no panic). +func TestHandler_getHistory_CreatedByUserEmailResolved(t *testing.T) { + creatorID := "user-uuid-1234" + creatorEmail := "alice@example.com" + approverEmail := "ops@example.com" + + t.Run("email resolved from auth service", func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + + exec := config.PurchaseExecution{ + ExecutionID: "pend-with-creator", + Status: "pending", + ScheduledDate: time.Now(), + CreatedByUserID: &creatorID, + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "us-east-1"}, + }, + } + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{exec}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + + mockAuth, req := adminHistoryReq(ctx) + mockAuth.On("GetUser", ctx, creatorID).Return(&User{ID: creatorID, Email: creatorEmail}, nil) + handler := &Handler{auth: mockAuth, config: mockStore} + + result, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err) + resp := result.(HistoryResponse) + + require.Len(t, resp.Purchases, 1) + row := resp.Purchases[0] + assert.Equal(t, creatorID, row.CreatedByUserID, "raw UUID must still be present for cancel-own gate") + assert.Equal(t, creatorEmail, row.CreatedByUserEmail, "email must be resolved for Approval Queue display") + }) + + t.Run("lookup failure degrades gracefully", func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + + exec := config.PurchaseExecution{ + ExecutionID: "pend-bad-user", + Status: "pending", + ScheduledDate: time.Now(), + CreatedByUserID: &creatorID, + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "us-east-1"}, + }, + } + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{exec}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + + mockAuth, req := adminHistoryReq(ctx) + mockAuth.On("GetUser", ctx, creatorID).Return(nil, errors.New("user not found")) + handler := &Handler{auth: mockAuth, config: mockStore} + + result, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err, "a user-lookup failure must not abort the history response") + resp := result.(HistoryResponse) + + require.Len(t, resp.Purchases, 1, "row must still render when email lookup fails") + row := resp.Purchases[0] + assert.Equal(t, creatorID, row.CreatedByUserID) + assert.Empty(t, row.CreatedByUserEmail, "email must be empty when lookup fails — UI falls back to UUID") + }) +} + // TestHandler_getHistory_CompletedExecutionNotDuplicated guards the dedup path. // The store loads "completed" executions now (so audit-gap rows can surface), // but a NORMAL completed execution (Error=="") is already represented by its diff --git a/internal/config/types.go b/internal/config/types.go index 7b10a1c64..8f25fa893 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -513,6 +513,15 @@ type PurchaseHistoryRecord struct { // rows loaded from the DB always leave this false. Excluded from DB // persistence (set only at read time on synthesised rows). IsAuditGap bool `json:"is_audit_gap,omitempty" dynamodbav:"-"` + // CreatedByUserEmail is the email address of the user who created the + // underlying execution, resolved from CreatedByUserID via the auth + // service. Populated only on synthesised execution rows (pending, + // notified, failed, expired, cancelled) when a valid user ID is + // present; empty for scheduler-driven executions, legacy NULL-creator + // rows, and completed purchase_history rows. Excluded from DB + // persistence (resolved at read time). The UI renders this in the + // Approval Queue "Created by" column instead of the raw UUID. + CreatedByUserEmail string `json:"created_by_user_email,omitempty" dynamodbav:"-"` } // RIExchangeRecord represents a record in the ri_exchange_history table