diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 592e8953c..d83531b37 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -665,10 +665,15 @@ func parseHistoryDateBounds(startStr, endStr string) (time.Time, time.Time, erro // multi-rec basket that spans providers (e.g. aws+azure) matches any of // them — dropping such an execution because its collapsed display label // is "multiple" would hide real activity the user owns. -// - Account: matches when the execution's CloudAccountID is one of the -// filtered IDs. Executions with a NULL CloudAccountID are excluded once -// account_ids is non-empty, mirroring the SQL semantics on -// purchase_history.cloud_account_id (issue #211). +// - Account: matches when the execution's effective account ID is one of the +// filtered IDs. The effective account ID is exec.CloudAccountID when set; +// otherwise collapseRecommendationAccount(exec.Recommendations) provides +// the fallback, matching the same logic used in executionToHistoryRow. +// Web-initiated bulk purchases never set exec.CloudAccountID — only the +// per-rec field is populated — so without this fallback an account-filtered +// approval queue would silently drop all pending web purchases (issue #704). +// An execution whose effective account ID is "" (nil exec field AND recs +// disagree or have no account) is excluded once account_ids is non-empty. // - Date: matches when ScheduledDate is within [Start, End]. Inclusive // both sides; End is the end-of-day for YYYY-MM-DD inputs. func (f historyFilters) matchesExecution(exec config.PurchaseExecution) bool { @@ -682,10 +687,18 @@ func (f historyFilters) matchesExecution(exec config.PurchaseExecution) bool { // LegacyAccountID is intentionally NOT folded here because it is a // different (VARCHAR(20)) cloud-provider account number that the // fast path applies on the SQL side. - if exec.CloudAccountID == nil { - return false + // + // Use the same two-level account resolution as executionToHistoryRow: + // exec.CloudAccountID first, then the rec-level fallback. This ensures + // web bulk-purchase executions (exec.CloudAccountID == nil) are not + // silently dropped when the caller filters by account. + var accountID string + if exec.CloudAccountID != nil { + accountID = *exec.CloudAccountID + } else { + accountID = collapseRecommendationAccount(exec.Recommendations) } - if !stringInSlice(*exec.CloudAccountID, f.AccountIDs) { + if accountID == "" || !stringInSlice(accountID, f.AccountIDs) { return false } } diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index f577fbcb1..e58799f41 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -747,7 +747,7 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { } assert.True(t, ids["exec-in-A"], "execution in account A must survive the filter") assert.False(t, ids["exec-in-C"], "execution in unrelated account C must be filtered out") - assert.False(t, ids["exec-nil-account"], "execution with NULL CloudAccountID must be excluded once account_ids is non-empty") + assert.False(t, ids["exec-nil-account"], "execution with NULL exec.CloudAccountID AND no rec-level CloudAccountID must still be excluded (effective account is empty)") }) t.Run("legacy singular account_id is ignored when combined with new filters", func(t *testing.T) { @@ -1244,7 +1244,63 @@ func TestHandler_getHistory_ApprovalQueueColumnsPopulated(t *testing.T) { 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)") + 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)") + }) + + t.Run("account_ids filter matches pending row via rec-level fallback when exec.CloudAccountID is nil", func(t *testing.T) { + // Regression: matchesExecution previously rejected executions with a nil + // exec.CloudAccountID even when the rec carried the matching UUID. Web + // bulk-purchase flows only populate the per-rec CloudAccountID, so an + // account-filtered approval queue would silently drop all web-initiated + // pending rows. The fix makes matchesExecution apply the same two-level + // fallback as executionToHistoryRow (issue #704, CR #738). + ctx := context.Background() + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + recAccountID := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + monthly := 9.0 + pending := []config.PurchaseExecution{ + { + ExecutionID: "pend-nil-exec-account", + Status: "pending", + ScheduledDate: time.Now(), + // exec.CloudAccountID intentionally nil - mirrors web bulk-purchase + // flow. The rec carries the canonical account UUID. + Recommendations: []config.RecommendationRecord{ + { + Provider: "aws", + Service: "ec2", + Region: "us-east-1", + ResourceType: "m5.large", + Term: 1, + Payment: "no-upfront", + Count: 1, + UpfrontCost: 0.0, + MonthlyCost: &monthly, + CloudAccountID: &recAccountID, + }, + }, + }, + } + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(pending, nil) + // account_ids is set, so fetchPurchaseHistory routes to GetPurchaseHistoryFiltered + // (not GetAllPurchaseHistory). No DB rows needed; we only care about the + // execution path. + mockStore.On("GetPurchaseHistoryFiltered", ctx, "", []string{recAccountID}, (*time.Time)(nil), (*time.Time)(nil), 100). + Return([]config.PurchaseHistoryRecord{}, 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{"account_ids": recAccountID}) + require.NoError(t, err) + resp := result.(HistoryResponse) + require.Len(t, resp.Purchases, 1, "pending row must survive the account_ids filter via rec-level CloudAccountID fallback") + row := resp.Purchases[0] + + assert.Equal(t, recAccountID, row.AccountID, "AccountID must be resolved from the rec when exec.CloudAccountID is nil (#704)") }) }