From f903ff73ff462db6721c37a59d84dca855617834 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 26 May 2026 17:41:26 +0200 Subject: [PATCH] fix(plans): include universal plans in account filter (closes #705) The Account filter on the Plans page excluded plans whose Target Account field was left blank (NULL plan_accounts entries), even though an empty Target Account means "all accounts of this provider". Root cause: buildListPlansQuery used an INNER JOIN on plan_accounts, so plans with no rows in that join table were silently dropped. Fix: replace the JOIN with a WHERE pp.id IN (...) that unions two sub-queries: 1. Targeted plans: explicitly linked to the given account(s) via plan_accounts (unchanged behaviour). 2. Universal plans: no plan_accounts rows at all, but whose services JSONB contains at least one service whose "provider" field matches the provider of any of the given cloud accounts. The accountIDs slice is now passed as a single $1 Postgres array argument (ANY($1::uuid[])), removing the per-ID placeholder loop and keeping the query shape static. --- internal/config/store_postgres.go | 59 ++++++++++++++----- .../config/store_postgres_pgxmock_test.go | 55 +++++++++++++++++ internal/config/types.go | 5 +- 3 files changed, 101 insertions(+), 18 deletions(-) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 7f0c77d9d..513b9214e 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -584,9 +584,20 @@ func (s *PostgresStore) DeletePurchasePlan(ctx context.Context, planID string) e } // buildListPlansQuery returns the SQL query and args for ListPurchasePlans. -// When accountIDs is non-empty the query JOINs plan_accounts and filters -// on account_id IN ($1, $2, …) using parameterised placeholders so the -// result is bounded to plans that reference at least one of the given accounts. +// +// When accountIDs is empty, all plans are returned. +// +// When accountIDs is non-empty, the result includes two categories of plans: +// 1. Plans explicitly linked to at least one of the given accounts via +// plan_accounts (targeted plans). +// 2. Plans with NO rows in plan_accounts at all (universal plans, i.e. the +// Target Account field was left blank, meaning "all accounts of this +// provider"). A universal plan is included only when at least one of the +// given accounts has a provider that appears in the plan's services JSONB +// (each service value carries a "provider" field). +// +// The accountIDs slice is passed as a single $1 Postgres array argument so +// that ANY($1) can be used in both sub-queries without repeating parameters. func buildListPlansQuery(accountIDs []string) (query string, args []any) { if len(accountIDs) == 0 { return ` @@ -597,27 +608,43 @@ func buildListPlansQuery(accountIDs []string) (query string, args []any) { ORDER BY created_at DESC `, nil } - placeholders := make([]string, len(accountIDs)) - args = make([]any, len(accountIDs)) - for i, id := range accountIDs { - placeholders[i] = fmt.Sprintf("$%d", i+1) - args[i] = id - } - query = fmt.Sprintf(` - SELECT DISTINCT pp.id, pp.name, pp.enabled, pp.auto_purchase, pp.notification_days_before, + // A single array argument lets both sub-queries reuse $1. + args = []any{accountIDs} + query = ` + SELECT pp.id, pp.name, pp.enabled, pp.auto_purchase, pp.notification_days_before, pp.services, pp.ramp_schedule, pp.created_at, pp.updated_at, pp.next_execution_date, pp.last_execution_date, pp.last_notification_sent FROM purchase_plans pp - JOIN plan_accounts pa ON pa.plan_id = pp.id - WHERE pa.account_id IN (%s) + WHERE pp.id IN ( + -- targeted plans: explicitly linked to one of the given accounts + SELECT pa.plan_id + FROM plan_accounts pa + WHERE pa.account_id = ANY($1::uuid[]) + + UNION + + -- universal plans: no target accounts set, provider matches + SELECT pp2.id + FROM purchase_plans pp2 + WHERE NOT EXISTS ( + SELECT 1 FROM plan_accounts pa2 WHERE pa2.plan_id = pp2.id + ) + AND EXISTS ( + SELECT 1 + FROM cloud_accounts ca + JOIN jsonb_each(pp2.services) AS svc(k, v) ON (v->>'provider') = ca.provider + WHERE ca.id = ANY($1::uuid[]) + ) + ) ORDER BY pp.created_at DESC - `, strings.Join(placeholders, ", ")) + ` return query, args } // ListPurchasePlans lists purchase plans, optionally filtered by account IDs. -// When filter.AccountIDs is non-empty the result is limited to plans that -// reference at least one of those accounts via the plan_accounts join table. +// When filter.AccountIDs is non-empty the result includes plans that reference +// at least one of those accounts (via plan_accounts) and universal plans (no +// target accounts) whose provider matches any of the given accounts. func (s *PostgresStore) ListPurchasePlans(ctx context.Context, filter PurchasePlanFilter) ([]PurchasePlan, error) { query, args := buildListPlansQuery(filter.AccountIDs) rows, err := s.db.Query(ctx, query, args...) diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index f43f1ea22..78f93ebcd 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -320,6 +320,61 @@ func TestPGXMock_ListPurchasePlans_Error(t *testing.T) { require.Error(t, err) } +// TestPGXMock_ListPurchasePlans_AccountFilter verifies that when AccountIDs is +// set the query passes a single array arg (the $1 ANY-placeholder used by +// both sub-queries) and returns the rows the DB produces. The SQL shape is +// tested at the query level; end-to-end provider-match logic requires a real +// Postgres instance and is covered by the DB-integration test suite. +func TestPGXMock_ListPurchasePlans_AccountFilter(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + accountID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + now := time.Now().Truncate(time.Second) + svcJSON, _ := json.Marshal(map[string]ServiceConfig{ + "aws:rds": {Provider: "aws", Service: "rds"}, + }) + rampJSON, _ := json.Marshal(RampSchedule{}) + cols := []string{ + "id", "name", "enabled", "auto_purchase", "notification_days_before", + "services", "ramp_schedule", "created_at", "updated_at", + "next_execution_date", "last_execution_date", "last_notification_sent", + } + rows := pgxmock.NewRows(cols). + AddRow("p1", "Targeted Plan", true, false, 3, svcJSON, rampJSON, now, now, + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}). + AddRow("p2", "Universal AWS Plan", true, false, 7, svcJSON, rampJSON, now, now, + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}) + + // The refactored query passes a single array arg for $1. + mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) + + plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{AccountIDs: []string{accountID}}) + require.NoError(t, err) + assert.Len(t, plans, 2) + assert.Equal(t, "Targeted Plan", plans[0].Name) + assert.Equal(t, "Universal AWS Plan", plans[1].Name) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_ListPurchasePlans_AccountFilterError verifies error propagation +// when the DB rejects the account-filtered query. +func TestPGXMock_ListPurchasePlans_AccountFilterError(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()). + WillReturnError(errors.New("db error")) + + _, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{ + AccountIDs: []string{"bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to list purchase plans") +} + // ─── UpdatePurchasePlan ─────────────────────────────────────────────────────── func TestPGXMock_UpdatePurchasePlan_NotFound(t *testing.T) { diff --git a/internal/config/types.go b/internal/config/types.go index 5535b4f63..e4413c3a0 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -394,8 +394,9 @@ type RecommendationFilter struct { // PurchasePlanFilter parameterises ListPurchasePlans. Zero-value means "no // filter" (all plans are returned). Non-empty AccountIDs restricts the result -// to plans that reference at least one of the given account IDs via the -// plan_accounts join table. +// to: (a) plans that reference at least one of the given account IDs via +// plan_accounts, and (b) plans with no target accounts (universal plans) +// whose provider matches any of the given accounts. type PurchasePlanFilter struct { AccountIDs []string // nil/empty = all plans }