From e3ca433083ba3efd156bcf48b51ad1a25f2a42f4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 19:56:29 +0200 Subject: [PATCH] feat(approvals): surface Account/Term/Payment/Monthly + user email in Approval queue Add Account, Term, Payment, Monthly Cost columns to the Approval queue renderer and show the creator's email (resolved from the auth service) instead of the raw UUID. Backend: PurchaseHistoryRecord gains CreatedByUserEmail (non-persisted). executionToHistoryRow accepts the resolved email; fetchExecutionsAsHistory builds a per-request user-ID-to-email cache via resolveUserEmails (one GetUser call per distinct creator, logs+skips on failure). Frontend types: HistoryPurchase extended with account_id, payment, monthly_cost, created_by_user_email (term was already present). recommendations.ts exports getAccountName() so history.ts can resolve account names from the shared cache without duplicating the map. renderApprovalQueue gains Account/Term/Payment/Monthly Cost columns; the Created-by cell falls back UUID -> "-" when email is absent. Effective Savings column: the existing "Monthly Savings" label already matches the actual semantic (estimated_savings is a monthly figure), so no rename or formula change is needed (row 1.3 resolved by option B). Tests: two new Go sub-tests for email resolution and graceful degradation; six new TS tests covering each new column and the email/UUID fallback chain. Closes #704 --- .../__tests__/history-approval-queue.test.ts | 129 ++++++++++++++++++ frontend/src/history.ts | 25 +++- frontend/src/recommendations.ts | 10 ++ frontend/src/types.ts | 9 ++ internal/api/handler_history.go | 58 ++++++-- internal/api/handler_history_test.go | 73 ++++++++++ internal/config/types.go | 9 ++ 7 files changed, 299 insertions(+), 14 deletions(-) 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