From 575e1d17ce0cc2bbba169a1f554300cc12b3d5b5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 15:36:00 +0200 Subject: [PATCH 1/2] feat(plans): surface legacy no-account plans as Unassigned (closes #973) Plans created before target_accounts was required (#743) have zero rows in plan_accounts and are invisible in account-filtered views because the JOIN on plan_accounts excludes them. Backend: buildListPlansQuery now uses LEFT JOIN + OR NOT EXISTS so that zero-account plans are included alongside matched-account plans when an account filter is active. A computed boolean column "unassigned" (true for zero-account plans, false otherwise) is selected so callers can bucket the two groups without a second query. The no-filter case continues to return all plans and sets unassigned=false. PurchasePlan gains an Unassigned field that is omitted from JSON when false. Frontend: renderPlans splits plans into assigned and unassigned buckets. Assigned plans render as before. Unassigned plans are appended under a clearly labeled "Unassigned" section header (class unassigned-plans-header). Account-scoped actions (Add Purchases, Edit, enable toggle) are suppressed for unassigned plans; History and Delete remain available. Account-name resolution is skipped for unassigned plans because they have no plan_accounts rows. Tests: backend adds TestPGXMock_ListPurchasePlans_UnassignedIncluded (zero-account plan flagged true, assigned plan flagged false) and TestHandler_HandleRequest_ListPlans_UnassignedFlagged (API-level regression guard). Frontend adds two loadPlans tests: one asserting the Unassigned section appears with the correct order, another asserting it is absent when all plans are assigned. --- frontend/src/__tests__/plans.test.ts | 80 ++++++++ frontend/src/api/types.ts | 4 + frontend/src/plans.ts | 192 +++++++++++------- internal/api/handler_test.go | 63 ++++++ internal/config/store_postgres.go | 35 +++- .../store_postgres_comprehensive_test.go | 19 +- .../config/store_postgres_pgxmock_test.go | 51 ++++- internal/config/types.go | 8 + 8 files changed, 362 insertions(+), 90 deletions(-) diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index e297cc445..14ec628f4 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -558,6 +558,86 @@ describe('Plans Module', () => { const list = document.getElementById('plans-list'); expect(list?.innerHTML).toContain('Null Services Plan'); }); + + // Regression guard for issue #973: a plan with unassigned=true must + // appear in the "Unassigned" section, and an assigned plan must NOT + // appear there. Before the fix the backend silently dropped zero-account + // plans from account-filtered responses, so no "Unassigned" section was + // ever rendered. + test('unassigned plan renders under Unassigned section, assigned plan does not (issue #973)', async () => { + (api.getPlans as jest.Mock).mockResolvedValue({ + plans: [ + { + id: 'assigned-plan', + name: 'Assigned Plan', + enabled: true, + auto_purchase: false, + unassigned: false, + services: { + 'ec2': { provider: 'aws', service: 'ec2', enabled: true, term: 1, payment: 'no-upfront', coverage: 80 } + }, + ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0, current_step: 0, total_steps: 1 } + }, + { + id: 'legacy-plan', + name: 'Legacy Unscoped Plan', + enabled: true, + auto_purchase: false, + unassigned: true, + services: { + 'rds': { provider: 'aws', service: 'rds', enabled: true, term: 3, payment: 'no-upfront', coverage: 70 } + }, + ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0, current_step: 0, total_steps: 1 } + } + ] + }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + await loadPlans(); + + const list = document.getElementById('plans-list'); + const html = list?.innerHTML ?? ''; + + // Both plans are rendered. + expect(html).toContain('Assigned Plan'); + expect(html).toContain('Legacy Unscoped Plan'); + + // The "Unassigned" section header is present. + expect(html).toContain('unassigned-plans-header'); + expect(html).toContain('Unassigned'); + + // The unassigned section header must appear AFTER the assigned plan card + // (assigned plans come first, unassigned section is appended after). + const assignedPos = html.indexOf('Assigned Plan'); + const unassignedHeaderPos = html.indexOf('unassigned-plans-header'); + const legacyPos = html.indexOf('Legacy Unscoped Plan'); + expect(assignedPos).toBeLessThan(unassignedHeaderPos); + expect(unassignedHeaderPos).toBeLessThan(legacyPos); + }); + + test('no Unassigned section when all plans are assigned', async () => { + (api.getPlans as jest.Mock).mockResolvedValue({ + plans: [ + { + id: 'plan-a', + name: 'Normal Plan', + enabled: true, + auto_purchase: false, + unassigned: false, + services: { + 'ec2': { provider: 'aws', service: 'ec2', enabled: true, term: 1, payment: 'no-upfront', coverage: 80 } + }, + ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0, current_step: 0, total_steps: 1 } + } + ] + }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + await loadPlans(); + + const list = document.getElementById('plans-list'); + expect(list?.innerHTML).not.toContain('unassigned-plans-header'); + }); }); describe('loadPlannedPurchases', () => { diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 77ebab5cf..47eee936e 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -191,6 +191,10 @@ export interface Plan { updated_at: string; next_execution_date?: string; last_execution_date?: string; + // unassigned is true for legacy plans that have zero plan_accounts rows. + // Such plans are surfaced under an "Unassigned" section in the Plans UI + // so operators can find and re-scope them (issue #973). + unassigned?: boolean; } export interface CreatePlanRequest { diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 8834ad6cb..913ac7807 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -789,6 +789,10 @@ interface BackendPlan { total_steps: number; }; next_execution_date?: string; + // unassigned is true for legacy plans that have zero plan_accounts rows + // (issue #973). The backend sets this flag when an account filter is + // active so the frontend can bucket them under an "Unassigned" section. + unassigned?: boolean; } // Pretty label for a service slug used inside the plan card. @@ -878,6 +882,85 @@ async function loadPlanAccountNames(planId: string, cardEl: Element): PromiseOverdue' + : ''; + // Read-only mode: unassigned plans cannot be purchased against until an + // operator re-assigns them to at least one account. + const isUnassigned = Boolean(plan.unassigned); + + return ` +
+
+

${escapeHtml(plan.name)}

+
+ ${status.label} + ${overdueBadge} + ${canManagePlan && !isUnassigned ? ` + + ` : ''} +
+
+
+
+
+ Provider + ${info.provider.toUpperCase()} +
+
+ Service + ${escapeHtml(info.service)} +
+
+ Term + ${formatTerm(info.term)} +
+
+ Coverage + ${info.coverage}% +
+
+ Ramp Schedule + ${formatBackendRampSchedule(rampSchedule)} +
+
+ Progress + ${rampSchedule.current_step || 0}/${rampSchedule.total_steps || 1} steps +
+ ${showNextDate ? ` +
+ Next Purchase + ${formatDate(plan.next_execution_date || '')} +
+ ` : ''} +
+
+ ${canManagePlan && !isUnassigned ? `` : ''} + ${canManagePlan && !isUnassigned ? `` : ''} + + ${canDeletePlan ? `` : ''} +
+
+
+ `; +} + function renderPlans(plans: LocalPlan[]): void { const container = document.getElementById('plans-list'); if (!container) return; @@ -893,83 +976,44 @@ function renderPlans(plans: LocalPlan[]): void { const canManagePlan = canAccess('update', 'plans'); const canDeletePlan = canAccess('delete', 'plans'); - container.innerHTML = plans.map(rawPlan => { - // Cast to BackendPlan to handle the actual API response format - const plan = rawPlan as unknown as BackendPlan; - const info = extractPlanInfo(plan); - const status = getStatusBadge(plan.enabled, plan.auto_purchase); - const rampSchedule = plan.ramp_schedule || { type: 'immediate', current_step: 0, total_steps: 1 }; - const overdue = isPlanOverdue(plan); - // Hide the stale next_execution_date for disabled plans — keeping it - // visible implies the plan will still run on that date, which it won't. - const showNextDate = Boolean(plan.next_execution_date) && plan.enabled; - const overdueBadge = overdue && plan.enabled - ? 'Overdue' - : ''; - - return ` -
-
-

${escapeHtml(plan.name)}

-
- ${status.label} - ${overdueBadge} - ${canManagePlan ? ` - - ` : ''} -
-
-
-
-
- Provider - ${info.provider.toUpperCase()} -
-
- Service - ${escapeHtml(info.service)} -
-
- Term - ${formatTerm(info.term)} -
-
- Coverage - ${info.coverage}% -
-
- Ramp Schedule - ${formatBackendRampSchedule(rampSchedule)} -
-
- Progress - ${rampSchedule.current_step || 0}/${rampSchedule.total_steps || 1} steps -
- ${showNextDate ? ` -
- Next Purchase - ${formatDate(plan.next_execution_date || '')} -
- ` : ''} -
-
- ${canManagePlan ? `` : ''} - ${canManagePlan ? `` : ''} - - ${canDeletePlan ? `` : ''} -
-
+ // Issue #973: split plans into assigned (have plan_accounts rows) and + // unassigned (legacy plans with zero plan_accounts rows, flagged by the + // backend). Unassigned plans are rendered under a separate read-only + // section so operators can discover and re-scope them. + const assignedPlans = plans.filter(p => !(p as unknown as BackendPlan).unassigned); + const unassignedPlans = plans.filter(p => (p as unknown as BackendPlan).unassigned); + + const assignedHtml = assignedPlans.map(rawPlan => + renderPlanCard(rawPlan as unknown as BackendPlan, canManagePlan, canDeletePlan) + ).join(''); + + let unassignedHtml = ''; + if (unassignedPlans.length > 0) { + const cards = unassignedPlans.map(rawPlan => + renderPlanCard(rawPlan as unknown as BackendPlan, canManagePlan, canDeletePlan) + ).join(''); + unassignedHtml = ` +
+

Unassigned

+ These legacy plans have no associated accounts and cannot be purchased against. Assign accounts or delete them.
+ ${cards} `; - }).join(''); + } - // Asynchronously populate account names per plan - container.querySelectorAll('.plan-card').forEach((card, idx) => { - const plan = plans[idx] as unknown as BackendPlan; - if (plan.id) void loadPlanAccountNames(plan.id, card); + container.innerHTML = assignedHtml + unassignedHtml; + + // Asynchronously populate account names per assigned plan card. + // Unassigned plans intentionally skip this: they have no plan_accounts + // rows so the API call would return an empty list. + container.querySelectorAll('.plan-card').forEach((card) => { + const planId = card.querySelector('[data-id]')?.dataset['id']; + // Find the matching plan to check unassigned status. + const allPlans = [...assignedPlans, ...unassignedPlans]; + const matchedPlan = allPlans.find(p => (p as unknown as BackendPlan).id === planId) as unknown as BackendPlan | undefined; + if (planId && matchedPlan && !matchedPlan.unassigned) { + void loadPlanAccountNames(planId, card); + } }); // Add event listeners diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index ac6b0e8b6..3a474b1c7 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -1341,6 +1341,69 @@ func TestHandler_HandleRequest_ListPlans_Error(t *testing.T) { assert.Equal(t, 500, resp.StatusCode) } +// TestHandler_HandleRequest_ListPlans_UnassignedFlagged is the regression +// guard for issue #973: a plan with zero plan_accounts rows (legacy +// "universal" plan) must appear in the response flagged as unassigned=true +// when an account filter is active. Before the fix such plans were silently +// dropped by the INNER JOIN on plan_accounts. +func TestHandler_HandleRequest_ListPlans_UnassignedFlagged(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{UserID: "admin-id", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "test-token").Return(adminSession, nil) + mockAuth.grantAdmin() + + // Simulate the store returning one assigned plan and one zero-account + // (unassigned) legacy plan. + assigned := config.PurchasePlan{ID: "11111111-1111-1111-1111-111111111111", Name: "Assigned Plan", Unassigned: false} + legacy := config.PurchasePlan{ID: "22222222-2222-2222-2222-222222222222", Name: "Legacy Plan", Unassigned: true} + mockStore.On("ListPurchasePlans", mock.Anything, mock.Anything).Return([]config.PurchasePlan{assigned, legacy}, nil) + + handler := &Handler{config: mockStore, auth: mockAuth, apiKey: "test-key"} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "X-API-Key": "test-key", + "Authorization": "Bearer test-token", + }, + QueryStringParameters: map[string]string{ + "account_ids": "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + }, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "GET", + Path: "/api/plans", + }, + }, + } + + resp, err := handler.HandleRequest(ctx, req) + require.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode) + + var body PlansResponse + require.NoError(t, json.Unmarshal([]byte(resp.Body), &body)) + require.Len(t, body.Plans, 2) + + // Find plans by ID to avoid order dependence. + plansByID := make(map[string]config.PurchasePlan, 2) + for _, p := range body.Plans { + plansByID[p.ID] = p + } + + // Assigned plan must not carry the unassigned flag. + ap, ok := plansByID["11111111-1111-1111-1111-111111111111"] + require.True(t, ok, "assigned plan missing from response") + assert.False(t, ap.Unassigned, "assigned plan must have unassigned=false") + + // Legacy zero-account plan must carry the unassigned flag. + lp, ok := plansByID["22222222-2222-2222-2222-222222222222"] + require.True(t, ok, "legacy unassigned plan missing from response") + assert.True(t, lp.Unassigned, "zero-account plan must have unassigned=true") +} + // Test for updateConfig error case - invalid JSON returns 500 (not 400) func TestHandler_HandleRequest_UpdateConfig_InvalidJSON(t *testing.T) { ctx := context.Background() diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index c09f2e9b9..b489e70d6 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -585,15 +585,28 @@ 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 the query returns all plans without an unassigned +// column (false by default for every plan). +// +// When accountIDs is non-empty the query uses a LEFT JOIN so that plans with +// zero rows in plan_accounts ("unassigned" legacy plans) are included +// alongside the matched-account plans. The boolean expression +// (NOT EXISTS (SELECT 1 FROM plan_accounts WHERE plan_id = pp.id)) is +// selected as the "unassigned" column: true for zero-account plans, false +// for every plan that has at least one plan_accounts row. This lets the +// caller bucket the two groups without a separate query. +// +// The WHERE clause retains the original account-filter semantics for +// assigned plans and adds OR NOT EXISTS ... to include unassigned ones. +// DISTINCT prevents duplicates when a plan matches multiple account IDs. func buildListPlansQuery(accountIDs []string) (query string, args []any) { if len(accountIDs) == 0 { return ` SELECT id, name, enabled, auto_purchase, notification_days_before, services, ramp_schedule, created_at, updated_at, - next_execution_date, last_execution_date, last_notification_sent + next_execution_date, last_execution_date, last_notification_sent, + false AS unassigned FROM purchase_plans ORDER BY created_at DESC `, nil @@ -607,18 +620,23 @@ func buildListPlansQuery(accountIDs []string) (query string, args []any) { query = fmt.Sprintf(` SELECT DISTINCT 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 + pp.next_execution_date, pp.last_execution_date, pp.last_notification_sent, + (NOT EXISTS (SELECT 1 FROM plan_accounts WHERE plan_id = pp.id)) AS unassigned FROM purchase_plans pp - JOIN plan_accounts pa ON pa.plan_id = pp.id + LEFT JOIN plan_accounts pa ON pa.plan_id = pp.id WHERE pa.account_id IN (%s) + OR NOT EXISTS (SELECT 1 FROM plan_accounts WHERE plan_id = pp.id) 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 both plans that +// reference at least one of the given accounts AND legacy plans with zero +// plan_accounts rows (flagged with Unassigned=true). Plans that have at +// least one account row are returned with Unassigned=false. The no-filter +// case returns all plans with Unassigned=false. func (s *PostgresStore) ListPurchasePlans(ctx context.Context, filter PurchasePlanFilter) ([]PurchasePlan, error) { query, args := buildListPlansQuery(filter.AccountIDs) rows, err := s.db.Query(ctx, query, args...) @@ -646,6 +664,7 @@ func (s *PostgresStore) ListPurchasePlans(ctx context.Context, filter PurchasePl &nextExecDate, &lastExecDate, &lastNotifSent, + &plan.Unassigned, ) if err != nil { return nil, fmt.Errorf("failed to scan purchase plan: %w", err) diff --git a/internal/config/store_postgres_comprehensive_test.go b/internal/config/store_postgres_comprehensive_test.go index 4f1f74760..dda071e1f 100644 --- a/internal/config/store_postgres_comprehensive_test.go +++ b/internal/config/store_postgres_comprehensive_test.go @@ -129,7 +129,8 @@ func (s *mockablePostgresStore) ListPurchasePlans(ctx context.Context, filter Pu query := ` SELECT id, name, enabled, auto_purchase, notification_days_before, services, ramp_schedule, created_at, updated_at, - next_execution_date, last_execution_date, last_notification_sent + next_execution_date, last_execution_date, last_notification_sent, + false AS unassigned FROM purchase_plans ORDER BY created_at DESC ` @@ -159,6 +160,7 @@ func (s *mockablePostgresStore) ListPurchasePlans(ctx context.Context, filter Pu &nextExecDate, &lastExecDate, &lastNotifSent, + &plan.Unassigned, ) if err != nil { return nil, err @@ -743,13 +745,14 @@ func TestListPurchasePlans_Success(t *testing.T) { "id", "name", "enabled", "auto_purchase", "notification_days_before", "services", "ramp_schedule", "created_at", "updated_at", "next_execution_date", "last_execution_date", "last_notification_sent", + "unassigned", }). AddRow("plan-1", "First Plan", true, false, 7, servicesJSON1, rampJSON1, now, now, - sql.NullTime{Time: nextExec, Valid: true}, sql.NullTime{}, sql.NullTime{}). + sql.NullTime{Time: nextExec, Valid: true}, sql.NullTime{}, sql.NullTime{}, false). AddRow("plan-2", "Second Plan", false, true, 3, servicesJSON2, rampJSON2, now, now, - sql.NullTime{}, sql.NullTime{}, sql.NullTime{}) + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}, false) mock.ExpectQuery(`SELECT id, name, enabled, auto_purchase, notification_days_before`). WillReturnRows(rows) @@ -783,6 +786,7 @@ func TestListPurchasePlans_Empty(t *testing.T) { "id", "name", "enabled", "auto_purchase", "notification_days_before", "services", "ramp_schedule", "created_at", "updated_at", "next_execution_date", "last_execution_date", "last_notification_sent", + "unassigned", }) mock.ExpectQuery(`SELECT id, name, enabled, auto_purchase, notification_days_before`). @@ -828,9 +832,10 @@ func TestListPurchasePlans_InvalidServicesJSON(t *testing.T) { "id", "name", "enabled", "auto_purchase", "notification_days_before", "services", "ramp_schedule", "created_at", "updated_at", "next_execution_date", "last_execution_date", "last_notification_sent", + "unassigned", }).AddRow("plan-1", "Bad Plan", true, false, 7, []byte("not valid json"), rampJSON, now, now, - sql.NullTime{}, sql.NullTime{}, sql.NullTime{}) + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}, false) mock.ExpectQuery(`SELECT id, name, enabled, auto_purchase, notification_days_before`). WillReturnRows(rows) @@ -856,9 +861,10 @@ func TestListPurchasePlans_InvalidRampScheduleJSON(t *testing.T) { "id", "name", "enabled", "auto_purchase", "notification_days_before", "services", "ramp_schedule", "created_at", "updated_at", "next_execution_date", "last_execution_date", "last_notification_sent", + "unassigned", }).AddRow("plan-1", "Bad Ramp Plan", true, false, 7, servicesJSON, []byte("{invalid}"), now, now, - sql.NullTime{}, sql.NullTime{}, sql.NullTime{}) + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}, false) mock.ExpectQuery(`SELECT id, name, enabled, auto_purchase, notification_days_before`). WillReturnRows(rows) @@ -1923,9 +1929,10 @@ func TestListPurchasePlans_RowsError(t *testing.T) { "id", "name", "enabled", "auto_purchase", "notification_days_before", "services", "ramp_schedule", "created_at", "updated_at", "next_execution_date", "last_execution_date", "last_notification_sent", + "unassigned", }).AddRow("plan-1", "Plan 1", true, false, 7, servicesJSON, rampJSON, now, now, - sql.NullTime{}, sql.NullTime{}, sql.NullTime{}). + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}, false). RowError(0, errors.New("row iteration error")) mock.ExpectQuery(`SELECT id, name, enabled, auto_purchase, notification_days_before`). diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index ad4f97dc8..74c6e824e 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -295,12 +295,13 @@ func TestPGXMock_ListPurchasePlans_Success(t *testing.T) { "id", "name", "enabled", "auto_purchase", "notification_days_before", "services", "ramp_schedule", "created_at", "updated_at", "next_execution_date", "last_execution_date", "last_notification_sent", + "unassigned", } rows := pgxmock.NewRows(cols). AddRow("p1", "Plan 1", true, false, 3, svcJSON, rampJSON, now, now, - sql.NullTime{}, sql.NullTime{}, sql.NullTime{}). + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}, false). AddRow("p2", "Plan 2", false, true, 7, svcJSON, rampJSON, now, now, - sql.NullTime{}, sql.NullTime{}, sql.NullTime{}) + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}, false) mock.ExpectQuery("SELECT").WillReturnRows(rows) plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{}) @@ -320,6 +321,52 @@ func TestPGXMock_ListPurchasePlans_Error(t *testing.T) { require.Error(t, err) } +// TestPGXMock_ListPurchasePlans_UnassignedIncluded verifies that when an +// account filter is active, plans with zero plan_accounts rows are returned +// alongside the matched-account plans, flagged with Unassigned=true. +// +// This is the regression guard for issue #973: before the fix the INNER JOIN +// on plan_accounts silently excluded zero-account legacy plans from every +// account-filtered response. +func TestPGXMock_ListPurchasePlans_UnassignedIncluded(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + svcJSON, _ := json.Marshal(map[string]ServiceConfig{}) + 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", + "unassigned", + } + // The query returns two rows: one assigned (unassigned=false) and one + // legacy zero-account plan (unassigned=true). + rows := pgxmock.NewRows(cols). + AddRow("assigned-id", "Assigned Plan", true, false, 3, svcJSON, rampJSON, now, now, + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}, false). + AddRow("legacy-id", "Legacy Plan", true, false, 3, svcJSON, rampJSON, now, now, + sql.NullTime{}, sql.NullTime{}, sql.NullTime{}, true) + mock.ExpectQuery("SELECT").WithArgs("acc-uuid").WillReturnRows(rows) + + plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{AccountIDs: []string{"acc-uuid"}}) + require.NoError(t, err) + require.Len(t, plans, 2) + + // The assigned plan must NOT be flagged unassigned. + assert.Equal(t, "assigned-id", plans[0].ID) + assert.False(t, plans[0].Unassigned, "assigned plan should have Unassigned=false") + + // The legacy zero-account plan must be flagged unassigned. + assert.Equal(t, "legacy-id", plans[1].ID) + assert.True(t, plans[1].Unassigned, "zero-account plan should have Unassigned=true") + + assert.NoError(t, mock.ExpectationsWereMet()) +} + // ─── UpdatePurchasePlan ─────────────────────────────────────────────────────── func TestPGXMock_UpdatePurchasePlan_NotFound(t *testing.T) { diff --git a/internal/config/types.go b/internal/config/types.go index ba5127595..5f5648f2a 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -135,6 +135,14 @@ type PurchasePlan struct { NextExecutionDate *time.Time `json:"next_execution_date,omitempty" dynamodbav:"next_execution_date,omitempty"` LastExecutionDate *time.Time `json:"last_execution_date,omitempty" dynamodbav:"last_execution_date,omitempty"` LastNotificationSent *time.Time `json:"last_notification_sent,omitempty" dynamodbav:"last_notification_sent,omitempty"` + // Unassigned is true when the plan has zero rows in plan_accounts. + // This can happen for legacy plans created before target_accounts was + // required (issue #743). Such plans are invisible when an account filter + // is active because the normal JOIN excludes them; ListPurchasePlans + // surfaces them alongside filtered results so operators can find and + // re-scope them. The field is omitted (false) in the no-filter case + // where all plans are returned unconditionally. + Unassigned bool `json:"unassigned,omitempty" dynamodbav:"unassigned,omitempty"` } // RampSchedule defines how purchases are spread over time From dbe378ff17a2d84a8a694f03bc0ed16b421ede40 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 15:59:36 +0200 Subject: [PATCH 2/2] test(plans): real DB test for Unassigned bucket query (refs #973) Add a testcontainers-backed integration test (TestPostgresStoreDB_ListPurchasePlans_UnassignedBucket) that exercises the LEFT JOIN + OR NOT EXISTS query introduced in #973 against a real Postgres instance. Seed layout: - planA assigned to accountX: must appear with Unassigned=false - planB with zero plan_accounts rows: must appear with Unassigned=true - planC assigned to accountY only: must NOT appear in accountX filter The discriminating assertion (planB present in accountX-filtered result) fails on the pre-fix INNER JOIN code and passes with the LEFT JOIN fix, so a regression back to INNER JOIN will be caught by CI. Also fix two pre-existing compile errors in store_postgres_test.go (package config_test): add the config. qualifier to PurchasePlanFilter and inline the unexported pf() helper that was inaccessible from the external test package. --- internal/config/store_postgres_test.go | 4 +- .../config/store_postgres_unassigned_test.go | 168 ++++++++++++++++++ 2 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 internal/config/store_postgres_unassigned_test.go diff --git a/internal/config/store_postgres_test.go b/internal/config/store_postgres_test.go index 65707d8e1..0ae422030 100644 --- a/internal/config/store_postgres_test.go +++ b/internal/config/store_postgres_test.go @@ -265,7 +265,7 @@ func TestPostgresStore_PurchasePlans(t *testing.T) { } // List all plans - retrieved, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{}) + retrieved, err := store.ListPurchasePlans(ctx, config.PurchasePlanFilter{}) require.NoError(t, err) assert.GreaterOrEqual(t, len(retrieved), 2) }) @@ -351,7 +351,7 @@ func TestPostgresStore_PurchaseHistory(t *testing.T) { Term: 3, Payment: "all-upfront", UpfrontCost: 2250.00, - MonthlyCost: pf(0), + MonthlyCost: func() *float64 { v := 0.0; return &v }(), EstimatedSavings: 450.00, // PlanID intentionally left empty since it needs to be a valid UUID PlanName: "Test Plan", diff --git a/internal/config/store_postgres_unassigned_test.go b/internal/config/store_postgres_unassigned_test.go new file mode 100644 index 000000000..9c98d1e0e --- /dev/null +++ b/internal/config/store_postgres_unassigned_test.go @@ -0,0 +1,168 @@ +//go:build integration +// +build integration + +package config + +// TestPostgresStoreDB_ListPurchasePlans_UnassignedBucket is a real-DB +// integration test that exercises the LEFT JOIN + OR NOT EXISTS query +// introduced in #973. It is the discriminating regression guard that +// would FAIL on the pre-fix INNER JOIN code, where plan B (zero +// plan_accounts rows) would be absent from the filtered result entirely. +// +// Seed layout: +// - accountX: a cloud_accounts row whose UUID is used as the filter. +// - accountY: a second cloud_accounts row. +// - planA: assigned to accountX via plan_accounts -> must appear with Unassigned=false. +// - planB: ZERO plan_accounts rows (legacy/unassigned) -> must appear with Unassigned=true. +// - planC: assigned only to accountY -> must NOT appear when filtering by [accountX]. +// +// Three assertions prove the SQL is correct: +// 1. planB is present in the accountX-filtered result (fails on INNER JOIN). +// 2. planB.Unassigned == true. +// 3. planC is absent from the accountX-filtered result. + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPostgresStoreDB_ListPurchasePlans_UnassignedBucket(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + defer cancel() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + if err != nil { + t.Skipf("Skipping integration test: cannot start PostgreSQL container: %v", err) + return + } + t.Cleanup(func() { container.Cleanup(context.Background()) }) + + err = migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", "") + if err != nil { + t.Skipf("Skipping integration test: migrations failed: %v", err) + return + } + + conn := container.DB + store := NewPostgresStore(conn) + + // ---- Seed cloud_accounts ---- + // These are inserted directly so we control the UUIDs used in plan_accounts. + accountXID := uuid.New().String() + accountYID := uuid.New().String() + insertAccount := func(id, name, externalID string) { + t.Helper() + _, err := conn.Exec(ctx, ` + INSERT INTO cloud_accounts + (id, name, enabled, provider, external_id, aws_is_org_root, created_at, updated_at) + VALUES ($1, $2, true, 'aws', $3, false, now(), now()) + `, id, name, externalID) + require.NoError(t, err, "seed cloud_accounts: %s", name) + } + insertAccount(accountXID, "Account X", "111111111111") + insertAccount(accountYID, "Account Y", "222222222222") + + t.Cleanup(func() { + // plan_accounts rows cascade on purchase_plan delete; cloud_accounts rows do not. + conn.Exec(context.Background(), "DELETE FROM cloud_accounts WHERE id = ANY($1::uuid[])", + []string{accountXID, accountYID}) + }) + + // ---- Seed purchase_plans ---- + makePlan := func(name string) *PurchasePlan { + return &PurchasePlan{ + Name: name, + Enabled: true, + NotificationDaysBefore: 3, + Services: map[string]ServiceConfig{}, + RampSchedule: PresetRampSchedules["immediate"], + } + } + planA := makePlan("Plan A - assigned to X") + planB := makePlan("Plan B - unassigned (legacy)") + planC := makePlan("Plan C - assigned to Y only") + + require.NoError(t, store.CreatePurchasePlan(ctx, planA)) + require.NoError(t, store.CreatePurchasePlan(ctx, planB)) + require.NoError(t, store.CreatePurchasePlan(ctx, planC)) + + t.Cleanup(func() { + conn.Exec(context.Background(), "DELETE FROM purchase_plans WHERE id = ANY($1::uuid[])", + []string{planA.ID, planB.ID, planC.ID}) + }) + + // ---- Seed plan_accounts ---- + insertPlanAccount := func(planID, accountID string) { + t.Helper() + _, err := conn.Exec(ctx, ` + INSERT INTO plan_accounts (plan_id, account_id) + VALUES ($1::uuid, $2::uuid) + ON CONFLICT DO NOTHING + `, planID, accountID) + require.NoError(t, err, "seed plan_accounts (%s -> %s)", planID, accountID) + } + insertPlanAccount(planA.ID, accountXID) // planA -> accountX + insertPlanAccount(planC.ID, accountYID) // planC -> accountY only + // planB gets NO plan_accounts row -- that is the legacy case. + + // ==================================================================== + // Sub-test 1: account-filtered query (filter = [accountX]) + // ==================================================================== + t.Run("account-filtered result includes unassigned plan and excludes wrong-account plan", func(t *testing.T) { + plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{AccountIDs: []string{accountXID}}) + require.NoError(t, err) + + byID := make(map[string]PurchasePlan, len(plans)) + for _, p := range plans { + byID[p.ID] = p + } + + // planA must appear, assigned (not unassigned). + if pa, ok := byID[planA.ID]; assert.True(t, ok, "planA (assigned to accountX) must be in result") { + assert.False(t, pa.Unassigned, "planA is assigned; Unassigned must be false") + } + + // planB must appear, flagged as unassigned. + // DISCRIMINATING ASSERTION: on pre-fix INNER JOIN this row is absent. + if pb, ok := byID[planB.ID]; assert.True(t, ok, + "planB (zero plan_accounts rows) must be included in filtered result (regression guard for #973 INNER JOIN bug)") { + assert.True(t, pb.Unassigned, "planB has no plan_accounts rows; Unassigned must be true") + } + + // planC must NOT appear (it belongs only to accountY). + _, planCPresent := byID[planC.ID] + assert.False(t, planCPresent, "planC (assigned to accountY only) must NOT appear when filtering by accountX") + }) + + // ==================================================================== + // Sub-test 2: no-filter query returns all three plans + // ==================================================================== + t.Run("no-filter result returns all plans; assigned plans have Unassigned=false", func(t *testing.T) { + plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{}) + require.NoError(t, err) + + byID := make(map[string]PurchasePlan, len(plans)) + for _, p := range plans { + byID[p.ID] = p + } + + // All three plans must be present. + assert.Contains(t, byID, planA.ID, "planA must appear in no-filter result") + assert.Contains(t, byID, planB.ID, "planB must appear in no-filter result") + assert.Contains(t, byID, planC.ID, "planC must appear in no-filter result") + + // In the no-filter path, all plans are returned with Unassigned=false + // (the query hard-codes `false AS unassigned`). + for _, p := range []PurchasePlan{byID[planA.ID], byID[planB.ID], byID[planC.ID]} { + assert.False(t, p.Unassigned, + "no-filter path hard-codes false AS unassigned; plan %s must have Unassigned=false", p.ID) + } + }) +}