fix(approvals): populate Account/Payment/MonthlyCost on Approval queue rows (closes #733)

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({

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

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