From b2b9328b2420c856772a3787596efcb9447c531d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 26 May 2026 14:46:56 +0200 Subject: [PATCH] fix(approvals): populate Account/Payment/MonthlyCost on Approval queue rows (closes #733) PR #713 added the Account, Term, Payment, and Monthly Cost columns to the Approval queue card; the columns render but every row showed "-" because the backend never copied those fields onto the synthesised PurchaseHistoryRecord: - Account: executionToHistoryRow read exec.CloudAccountID, but the web-initiated bulk-purchase flow (buildPendingExecution) only populates the per-recommendation CloudAccountID and leaves the execution-level field nil. Fall back to a new collapseRecommendationAccount helper that returns the shared rec CloudAccountID, or "" (rendered as "-") for a basket genuinely spanning accounts. - Payment: projectRecommendationFields populated Service, ResourceType, Region, Term, UpfrontCost, EstimatedSavings, and (single-rec only) MonthlyCost from the rec, but never set row.Payment. Single-rec now copies r.Payment; multi-rec collapses via collapseRecommendationPayment (same pattern as the existing Provider/Service/Term collapsers), returning "" when recs disagree so the dash fallback stays honest. - MonthlyCost (multi-rec): only the single-rec branch mapped it; the multi-rec branch now sums per-rec MonthlyCost via sumRecommendationMonthlyCost (nil contributes 0, matching the single-rec treatment of nil MonthlyCost). Regression tests: - TestHandler_getHistory_ApprovalQueueColumnsPopulated pins all three shapes (single-rec, multi-rec uniform, multi-rec heterogeneous Payment) so a future refactor cannot silently re-empty the cells. - TestHandler_getHistory_InProgressRowMapsRecFields extended with a Payment assertion. - history-approval-queue.test.ts adds a frontend test that mocks the populated API shape and asserts the cells show real values, not "-". --- .../__tests__/history-approval-queue.test.ts | 44 ++++++ internal/api/handler_history.go | 71 +++++++++ internal/api/handler_history_test.go | 135 ++++++++++++++++++ 3 files changed, 250 insertions(+) diff --git a/frontend/src/__tests__/history-approval-queue.test.ts b/frontend/src/__tests__/history-approval-queue.test.ts index bb0b8b939..f407a145a 100644 --- a/frontend/src/__tests__/history-approval-queue.test.ts +++ b/frontend/src/__tests__/history-approval-queue.test.ts @@ -376,6 +376,50 @@ describe('Approval queue card (issue #340 sub-task)', () => { expect(queue.textContent).toContain('.5'); }); + // Issue #733 — regression guard. PR #713 shipped the columns but the + // backend never populated account_id / payment / monthly_cost on synthesised + // execution rows, so every Approval-queue cell rendered as "-". The fix is + // backend-side (handler_history.go now sources Account/Payment/MonthlyCost + // from the rec when exec.CloudAccountID is nil and copies Payment off the + // rec). This test pins the frontend contract end-to-end: when the API + // returns the populated shape we expect after the fix, the cells must show + // the real values, not the dash fallback. + test('Approval queue cells show real values, not "-", when backend returns populated row (#733)', 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-733', + status: 'pending', + account_id: '123456789012', + term: 1, + payment: 'all-upfront', + monthly_cost: 7.5, + created_by_user_id: ADMIN_USER.id, + }), + ], + }); + + await loadHistory(); + + const queue = document.getElementById('purchases-approval-queue')!; + const queueRow = queue.querySelector('tr[data-execution-id="p-733"]')!; + expect(queueRow).toBeTruthy(); + + const cellTexts = Array.from(queueRow.querySelectorAll('td')).map((c) => c.textContent || ''); + // Find by content rather than column index so a reorder doesn't break this. + expect(cellTexts).toEqual(expect.arrayContaining([ + expect.stringContaining('Production Account'), + expect.stringContaining('1 Year'), + expect.stringContaining('all-upfront'), + expect.stringContaining('7.5'), + ])); + // No dash fallback may appear on the populated cells. + expect(cellTexts).not.toContain('-'); + }); + test('renders Account/Term/Payment/Monthly column headers', async () => { (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); (api.getHistory as jest.Mock).mockResolvedValue({ diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index d6bea04d8..c24513f84 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -225,6 +225,15 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma var accountID string if exec.CloudAccountID != nil { accountID = *exec.CloudAccountID + } else { + // Web-initiated bulk-purchase executions (handler_purchases.go's + // buildPendingExecution) never populate exec.CloudAccountID — the + // per-rec CloudAccountID is the canonical source. Fall back to that + // so the Approval queue's Account cell shows the actual account ID + // instead of "-". Returns "" when recs disagree (a basket that + // genuinely spans accounts honestly renders as the dash fallback + // rather than a misleading single account). + accountID = collapseRecommendationAccount(exec.Recommendations) } var createdBy string if exec.CreatedByUserID != nil { @@ -368,6 +377,7 @@ func projectRecommendationFields(row *config.PurchaseHistoryRecord, exec config. row.ResourceType = r.ResourceType row.Region = r.Region row.Term = r.Term + row.Payment = r.Payment row.UpfrontCost = r.UpfrontCost row.EstimatedSavings = r.Savings if r.MonthlyCost != nil { @@ -379,6 +389,8 @@ func projectRecommendationFields(row *config.PurchaseHistoryRecord, exec config. row.ResourceType = fmt.Sprintf("%d commitment(s)", len(recs)) row.Service = collapseRecommendationService(recs) row.Term = collapseRecommendationTerm(recs) + row.Payment = collapseRecommendationPayment(recs) + row.MonthlyCost = sumRecommendationMonthlyCost(recs) row.UpfrontCost = exec.TotalUpfrontCost row.EstimatedSavings = exec.EstimatedSavings } @@ -432,6 +444,65 @@ func collapseRecommendationProvider(recs []config.RecommendationRecord) string { return p } +// collapseRecommendationPayment returns the payment option shared by every +// recommendation in an execution, or "" when they disagree (or the slice is +// empty). Empty renders as the dash fallback in the Approval queue rather +// than a misleading single payment string for a basket that mixes options. +func collapseRecommendationPayment(recs []config.RecommendationRecord) string { + if len(recs) == 0 { + return "" + } + p := recs[0].Payment + for _, r := range recs[1:] { + if r.Payment != p { + return "" + } + } + return p +} + +// collapseRecommendationAccount returns the cloud-account ID shared by every +// recommendation in an execution, or "" when they disagree (or none have one +// set). Used as the Account fallback when exec.CloudAccountID is nil — +// notably for web-initiated bulk purchases, which only populate the per-rec +// CloudAccountID and leave the execution-level field blank. +func collapseRecommendationAccount(recs []config.RecommendationRecord) string { + if len(recs) == 0 { + return "" + } + var first string + if recs[0].CloudAccountID != nil { + first = *recs[0].CloudAccountID + } + for _, r := range recs[1:] { + var cur string + if r.CloudAccountID != nil { + cur = *r.CloudAccountID + } + if cur != first { + return "" + } + } + return first +} + +// sumRecommendationMonthlyCost adds up the per-rec MonthlyCost values in a +// multi-rec execution so the Approval queue's Monthly Cost cell shows the +// committed recurring spend for the full basket. Nil per-rec entries +// contribute 0 (the provider API did not return a monthly breakdown for +// that rec) — the same treatment as the single-rec branch, which only +// copies MonthlyCost when non-nil and otherwise leaves the row's field at +// the zero value. +func sumRecommendationMonthlyCost(recs []config.RecommendationRecord) float64 { + var total float64 + for _, r := range recs { + if r.MonthlyCost != nil { + total += *r.MonthlyCost + } + } + return total +} + // MaxHistoryDateRangeDays caps the inclusive start/end window the History // handler accepts on a single request. Mirrors the analytics cap (issue // #414 / PR #529): an unbounded range turns the WHERE-on-timestamp into a diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 3d138cc52..256cba7eb 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -479,6 +479,7 @@ func TestHandler_getHistory_InProgressRowMapsRecFields(t *testing.T) { assert.Equal(t, 0.0, row.UpfrontCost, "upfront must come from the rec") assert.Equal(t, 1.2333, row.EstimatedSavings, "savings must come from the rec") assert.Equal(t, 2.117, row.MonthlyCost, "monthly cost must come from the rec") + assert.Equal(t, "no-upfront", row.Payment, "payment must come from the rec, not be left blank (#733)") assert.NotEmpty(t, row.StatusDescription, "in-progress rows must carry a human-readable status description, not render as a finished purchase") }) @@ -1113,6 +1114,140 @@ func TestHandler_getHistory_CreatedByUserEmailResolved(t *testing.T) { }) } +// TestHandler_getHistory_ApprovalQueueColumnsPopulated is the issue #733 +// regression guard. PR #713 added the Approval queue's Account, Term, Payment, +// and Monthly Cost columns to the frontend; the backend handler was missing the +// data plumbing for Account (web-initiated pending executions never set +// exec.CloudAccountID — the rec carries it), Payment (never copied off the rec +// in projectRecommendationFields), and multi-rec MonthlyCost (only the single- +// rec branch was mapped). Without these, every Approval queue cell rendered as +// the "-" fallback. Three sub-tests pin the contract: +// - single-rec: Payment is copied from the rec; Account falls back to the +// rec's CloudAccountID when exec.CloudAccountID is nil. +// - multi-rec uniform: Payment + Account collapse to the shared value; +// MonthlyCost sums across recs. +// - multi-rec mixed Payment: Payment collapses to "" (the frontend's "-" +// fallback) rather than silently picking a single value for a basket +// that genuinely mixes payment options. +func TestHandler_getHistory_ApprovalQueueColumnsPopulated(t *testing.T) { + approverEmail := "ops@example.com" + + t.Run("single-rec pending row carries Account + Payment + MonthlyCost", func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + monthly := 7.5 + accountID := "123456789012" + pending := []config.PurchaseExecution{ + { + ExecutionID: "pend-single", + Status: "pending", + ScheduledDate: time.Now(), + // exec.CloudAccountID intentionally nil — matches the + // real-world web bulk-purchase flow which only populates + // the per-rec CloudAccountID. + Recommendations: []config.RecommendationRecord{ + { + Provider: "aws", + Service: "ec2", + Region: "us-east-1", + ResourceType: "t4g.nano", + Term: 1, + Payment: "all-upfront", + Count: 1, + UpfrontCost: 100.0, + MonthlyCost: &monthly, + Savings: 2.5, + CloudAccountID: &accountID, + }, + }, + }, + } + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(pending, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + + mockAuth, req := adminHistoryReq(ctx) + 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, accountID, row.AccountID, "Account must fall back to rec.CloudAccountID when exec.CloudAccountID is nil (#733)") + assert.Equal(t, "all-upfront", row.Payment, "Payment must be copied from the single rec (#733)") + assert.Equal(t, 7.5, row.MonthlyCost, "MonthlyCost must come from the rec") + }) + + t.Run("multi-rec uniform pending row collapses Account + Payment, sums MonthlyCost", func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + monthlyA := 3.0 + monthlyB := 4.5 + accountID := "987654321098" + pending := []config.PurchaseExecution{ + { + ExecutionID: "pend-multi-uniform", + Status: "pending", + ScheduledDate: time.Now(), + TotalUpfrontCost: 250.0, + EstimatedSavings: 12.0, + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "us-east-1", Payment: "no-upfront", MonthlyCost: &monthlyA, CloudAccountID: &accountID}, + {Provider: "aws", Service: "ec2", Region: "us-east-1", Payment: "no-upfront", MonthlyCost: &monthlyB, CloudAccountID: &accountID}, + }, + }, + } + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(pending, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + + mockAuth, req := adminHistoryReq(ctx) + 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, accountID, row.AccountID, "Account must collapse to the shared rec value (#733)") + assert.Equal(t, "no-upfront", row.Payment, "Payment must collapse to the shared rec value (#733)") + assert.InDelta(t, 7.5, row.MonthlyCost, 1e-9, "MonthlyCost must sum across recs (#733)") + }) + + t.Run("multi-rec heterogeneous Payment collapses to empty for honest dash fallback", func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + pending := []config.PurchaseExecution{ + { + ExecutionID: "pend-multi-mixed", + Status: "pending", + ScheduledDate: time.Now(), + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "us-east-1", Payment: "all-upfront"}, + {Provider: "aws", Service: "ec2", Region: "us-east-1", Payment: "no-upfront"}, + }, + }, + } + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(pending, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + + mockAuth, req := adminHistoryReq(ctx) + 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.Empty(t, row.Payment, "Payment must collapse to empty when recs disagree — the dash fallback is more honest than a single arbitrary value (#733)") + }) +} + // 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