Skip to content

Commit eec7a39

Browse files
cristimcristim
authored
feat(plans): surface legacy no-account plans as Unassigned (closes #973) (#994)
* 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. * 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.
1 parent ede61ad commit eec7a39

10 files changed

Lines changed: 532 additions & 92 deletions

‎frontend/src/__tests__/plans.test.ts‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -562,6 +562,86 @@ describe('Plans Module', () => {
562562
const list = document.getElementById('plans-list');
563563
expect(list?.innerHTML).toContain('Null Services Plan');
564564
});
565+
566+
// Regression guard for issue #973: a plan with unassigned=true must
567+
// appear in the "Unassigned" section, and an assigned plan must NOT
568+
// appear there. Before the fix the backend silently dropped zero-account
569+
// plans from account-filtered responses, so no "Unassigned" section was
570+
// ever rendered.
571+
test('unassigned plan renders under Unassigned section, assigned plan does not (issue #973)', async () => {
572+
(api.getPlans as jest.Mock).mockResolvedValue({
573+
plans: [
574+
{
575+
id: 'assigned-plan',
576+
name: 'Assigned Plan',
577+
enabled: true,
578+
auto_purchase: false,
579+
unassigned: false,
580+
services: {
581+
'ec2': { provider: 'aws', service: 'ec2', enabled: true, term: 1, payment: 'no-upfront', coverage: 80 }
582+
},
583+
ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0, current_step: 0, total_steps: 1 }
584+
},
585+
{
586+
id: 'legacy-plan',
587+
name: 'Legacy Unscoped Plan',
588+
enabled: true,
589+
auto_purchase: false,
590+
unassigned: true,
591+
services: {
592+
'rds': { provider: 'aws', service: 'rds', enabled: true, term: 3, payment: 'no-upfront', coverage: 70 }
593+
},
594+
ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0, current_step: 0, total_steps: 1 }
595+
}
596+
]
597+
});
598+
(api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] });
599+
600+
await loadPlans();
601+
602+
const list = document.getElementById('plans-list');
603+
const html = list?.innerHTML ?? '';
604+
605+
// Both plans are rendered.
606+
expect(html).toContain('Assigned Plan');
607+
expect(html).toContain('Legacy Unscoped Plan');
608+
609+
// The "Unassigned" section header is present.
610+
expect(html).toContain('unassigned-plans-header');
611+
expect(html).toContain('Unassigned');
612+
613+
// The unassigned section header must appear AFTER the assigned plan card
614+
// (assigned plans come first, unassigned section is appended after).
615+
const assignedPos = html.indexOf('Assigned Plan');
616+
const unassignedHeaderPos = html.indexOf('unassigned-plans-header');
617+
const legacyPos = html.indexOf('Legacy Unscoped Plan');
618+
expect(assignedPos).toBeLessThan(unassignedHeaderPos);
619+
expect(unassignedHeaderPos).toBeLessThan(legacyPos);
620+
});
621+
622+
test('no Unassigned section when all plans are assigned', async () => {
623+
(api.getPlans as jest.Mock).mockResolvedValue({
624+
plans: [
625+
{
626+
id: 'plan-a',
627+
name: 'Normal Plan',
628+
enabled: true,
629+
auto_purchase: false,
630+
unassigned: false,
631+
services: {
632+
'ec2': { provider: 'aws', service: 'ec2', enabled: true, term: 1, payment: 'no-upfront', coverage: 80 }
633+
},
634+
ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0, current_step: 0, total_steps: 1 }
635+
}
636+
]
637+
});
638+
(api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] });
639+
640+
await loadPlans();
641+
642+
const list = document.getElementById('plans-list');
643+
expect(list?.innerHTML).not.toContain('unassigned-plans-header');
644+
});
565645
});
566646

567647
describe('loadPlannedPurchases', () => {

‎frontend/src/api/types.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,10 @@ export interface Plan {
206206
updated_at: string;
207207
next_execution_date?: string;
208208
last_execution_date?: string;
209+
// unassigned is true for legacy plans that have zero plan_accounts rows.
210+
// Such plans are surfaced under an "Unassigned" section in the Plans UI
211+
// so operators can find and re-scope them (issue #973).
212+
unassigned?: boolean;
209213
}
210214

211215
export interface CreatePlanRequest {

‎frontend/src/plans.ts‎

Lines changed: 118 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -818,6 +818,10 @@ interface BackendPlan {
818818
total_steps: number;
819819
};
820820
next_execution_date?: string;
821+
// unassigned is true for legacy plans that have zero plan_accounts rows
822+
// (issue #973). The backend sets this flag when an account filter is
823+
// active so the frontend can bucket them under an "Unassigned" section.
824+
unassigned?: boolean;
821825
}
822826

823827
// Pretty label for a service slug used inside the plan card.
@@ -907,6 +911,85 @@ async function loadPlanAccountNames(planId: string, cardEl: Element): Promise<vo
907911
}
908912
}
909913

914+
// renderPlanCard generates the HTML for a single plan card.
915+
// When the plan is unassigned (no plan_accounts rows) account-scoped
916+
// actions that require an account (Add Purchases, Edit) are suppressed
917+
// — only History and Delete remain — and a read-only "Unassigned" badge
918+
// is shown so operators can identify and re-scope the plan.
919+
function renderPlanCard(plan: BackendPlan, canManagePlan: boolean, canDeletePlan: boolean): string {
920+
const info = extractPlanInfo(plan);
921+
const status = getStatusBadge(plan.enabled, plan.auto_purchase);
922+
const rampSchedule = plan.ramp_schedule || { type: 'immediate', current_step: 0, total_steps: 1 };
923+
const overdue = isPlanOverdue(plan);
924+
// Hide the stale next_execution_date for disabled plans — keeping it
925+
// visible implies the plan will still run on that date, which it won't.
926+
const showNextDate = Boolean(plan.next_execution_date) && plan.enabled;
927+
const overdueBadge = overdue && plan.enabled
928+
? '<span class="status-badge badge-danger" title="Next purchase date is in the past">Overdue</span>'
929+
: '';
930+
// Read-only mode: unassigned plans cannot be purchased against until an
931+
// operator re-assigns them to at least one account.
932+
const isUnassigned = Boolean(plan.unassigned);
933+
934+
return `
935+
<div class="plan-card">
936+
<div class="plan-header">
937+
<h3>${escapeHtml(plan.name)}</h3>
938+
<div class="plan-status">
939+
<span class="status-badge ${status.class}">${status.label}</span>
940+
${overdueBadge}
941+
${canManagePlan && !isUnassigned ? `
942+
<label class="toggle-label">
943+
<input type="checkbox" data-action="toggle-plan" data-id="${plan.id}" ${plan.enabled ? 'checked' : ''}>
944+
<span class="slider"></span>
945+
</label>
946+
` : ''}
947+
</div>
948+
</div>
949+
<div class="plan-body">
950+
<div class="plan-details">
951+
<div class="plan-detail">
952+
<span class="plan-detail-label">Provider</span>
953+
<span class="plan-detail-value"><span class="provider-badge ${info.provider}">${info.provider.toUpperCase()}</span></span>
954+
</div>
955+
<div class="plan-detail">
956+
<span class="plan-detail-label">Service</span>
957+
<span class="plan-detail-value">${escapeHtml(info.service)}</span>
958+
</div>
959+
<div class="plan-detail">
960+
<span class="plan-detail-label">Term</span>
961+
<span class="plan-detail-value">${formatTerm(info.term)}</span>
962+
</div>
963+
<div class="plan-detail">
964+
<span class="plan-detail-label">Coverage</span>
965+
<span class="plan-detail-value">${info.coverage}%</span>
966+
</div>
967+
<div class="plan-detail">
968+
<span class="plan-detail-label">Ramp Schedule</span>
969+
<span class="plan-detail-value">${formatBackendRampSchedule(rampSchedule)}</span>
970+
</div>
971+
<div class="plan-detail">
972+
<span class="plan-detail-label">Progress</span>
973+
<span class="plan-detail-value">${rampSchedule.current_step || 0}/${rampSchedule.total_steps || 1} steps</span>
974+
</div>
975+
${showNextDate ? `
976+
<div class="plan-detail">
977+
<span class="plan-detail-label">Next Purchase</span>
978+
<span class="plan-detail-value">${formatDate(plan.next_execution_date || '')}</span>
979+
</div>
980+
` : ''}
981+
</div>
982+
<div class="plan-actions">
983+
${canManagePlan && !isUnassigned ? `<button data-action="add-purchases" data-id="${plan.id}" data-name="${escapeHtml(plan.name)}" class="primary">Add Purchases</button>` : ''}
984+
${canManagePlan && !isUnassigned ? `<button data-action="edit-plan" data-id="${plan.id}">Edit</button>` : ''}
985+
<button data-action="view-history" data-id="${plan.id}" class="secondary">History</button>
986+
${canDeletePlan ? `<button data-action="delete-plan" data-id="${plan.id}" class="danger">Delete</button>` : ''}
987+
</div>
988+
</div>
989+
</div>
990+
`;
991+
}
992+
910993
function renderPlans(plans: LocalPlan[]): void {
911994
const container = document.getElementById('plans-list');
912995
if (!container) return;
@@ -922,83 +1005,44 @@ function renderPlans(plans: LocalPlan[]): void {
9221005
const canManagePlan = canAccess('update', 'plans');
9231006
const canDeletePlan = canAccess('delete', 'plans');
9241007

925-
container.innerHTML = plans.map(rawPlan => {
926-
// Cast to BackendPlan to handle the actual API response format
927-
const plan = rawPlan as unknown as BackendPlan;
928-
const info = extractPlanInfo(plan);
929-
const status = getStatusBadge(plan.enabled, plan.auto_purchase);
930-
const rampSchedule = plan.ramp_schedule || { type: 'immediate', current_step: 0, total_steps: 1 };
931-
const overdue = isPlanOverdue(plan);
932-
// Hide the stale next_execution_date for disabled plans — keeping it
933-
// visible implies the plan will still run on that date, which it won't.
934-
const showNextDate = Boolean(plan.next_execution_date) && plan.enabled;
935-
const overdueBadge = overdue && plan.enabled
936-
? '<span class="status-badge badge-danger" title="Next purchase date is in the past">Overdue</span>'
937-
: '';
938-
939-
return `
940-
<div class="plan-card">
941-
<div class="plan-header">
942-
<h3>${escapeHtml(plan.name)}</h3>
943-
<div class="plan-status">
944-
<span class="status-badge ${status.class}">${status.label}</span>
945-
${overdueBadge}
946-
${canManagePlan ? `
947-
<label class="toggle-label">
948-
<input type="checkbox" data-action="toggle-plan" data-id="${plan.id}" ${plan.enabled ? 'checked' : ''}>
949-
<span class="slider"></span>
950-
</label>
951-
` : ''}
952-
</div>
953-
</div>
954-
<div class="plan-body">
955-
<div class="plan-details">
956-
<div class="plan-detail">
957-
<span class="plan-detail-label">Provider</span>
958-
<span class="plan-detail-value"><span class="provider-badge ${info.provider}">${info.provider.toUpperCase()}</span></span>
959-
</div>
960-
<div class="plan-detail">
961-
<span class="plan-detail-label">Service</span>
962-
<span class="plan-detail-value">${escapeHtml(info.service)}</span>
963-
</div>
964-
<div class="plan-detail">
965-
<span class="plan-detail-label">Term</span>
966-
<span class="plan-detail-value">${formatTerm(info.term)}</span>
967-
</div>
968-
<div class="plan-detail">
969-
<span class="plan-detail-label">Coverage</span>
970-
<span class="plan-detail-value">${info.coverage}%</span>
971-
</div>
972-
<div class="plan-detail">
973-
<span class="plan-detail-label">Ramp Schedule</span>
974-
<span class="plan-detail-value">${formatBackendRampSchedule(rampSchedule)}</span>
975-
</div>
976-
<div class="plan-detail">
977-
<span class="plan-detail-label">Progress</span>
978-
<span class="plan-detail-value">${rampSchedule.current_step || 0}/${rampSchedule.total_steps || 1} steps</span>
979-
</div>
980-
${showNextDate ? `
981-
<div class="plan-detail">
982-
<span class="plan-detail-label">Next Purchase</span>
983-
<span class="plan-detail-value">${formatDate(plan.next_execution_date || '')}</span>
984-
</div>
985-
` : ''}
986-
</div>
987-
<div class="plan-actions">
988-
${canManagePlan ? `<button data-action="add-purchases" data-id="${plan.id}" data-name="${escapeHtml(plan.name)}" class="primary">Add Purchases</button>` : ''}
989-
${canManagePlan ? `<button data-action="edit-plan" data-id="${plan.id}">Edit</button>` : ''}
990-
<button data-action="view-history" data-id="${plan.id}" class="secondary">History</button>
991-
${canDeletePlan ? `<button data-action="delete-plan" data-id="${plan.id}" class="danger">Delete</button>` : ''}
992-
</div>
993-
</div>
1008+
// Issue #973: split plans into assigned (have plan_accounts rows) and
1009+
// unassigned (legacy plans with zero plan_accounts rows, flagged by the
1010+
// backend). Unassigned plans are rendered under a separate read-only
1011+
// section so operators can discover and re-scope them.
1012+
const assignedPlans = plans.filter(p => !(p as unknown as BackendPlan).unassigned);
1013+
const unassignedPlans = plans.filter(p => (p as unknown as BackendPlan).unassigned);
1014+
1015+
const assignedHtml = assignedPlans.map(rawPlan =>
1016+
renderPlanCard(rawPlan as unknown as BackendPlan, canManagePlan, canDeletePlan)
1017+
).join('');
1018+
1019+
let unassignedHtml = '';
1020+
if (unassignedPlans.length > 0) {
1021+
const cards = unassignedPlans.map(rawPlan =>
1022+
renderPlanCard(rawPlan as unknown as BackendPlan, canManagePlan, canDeletePlan)
1023+
).join('');
1024+
unassignedHtml = `
1025+
<div class="plans-section-header unassigned-plans-header">
1026+
<h4>Unassigned</h4>
1027+
<span class="plans-section-description">These legacy plans have no associated accounts and cannot be purchased against. Assign accounts or delete them.</span>
9941028
</div>
1029+
${cards}
9951030
`;
996-
}).join('');
1031+
}
9971032

998-
// Asynchronously populate account names per plan
999-
container.querySelectorAll<HTMLElement>('.plan-card').forEach((card, idx) => {
1000-
const plan = plans[idx] as unknown as BackendPlan;
1001-
if (plan.id) void loadPlanAccountNames(plan.id, card);
1033+
container.innerHTML = assignedHtml + unassignedHtml;
1034+
1035+
// Asynchronously populate account names per assigned plan card.
1036+
// Unassigned plans intentionally skip this: they have no plan_accounts
1037+
// rows so the API call would return an empty list.
1038+
container.querySelectorAll<HTMLElement>('.plan-card').forEach((card) => {
1039+
const planId = card.querySelector<HTMLElement>('[data-id]')?.dataset['id'];
1040+
// Find the matching plan to check unassigned status.
1041+
const allPlans = [...assignedPlans, ...unassignedPlans];
1042+
const matchedPlan = allPlans.find(p => (p as unknown as BackendPlan).id === planId) as unknown as BackendPlan | undefined;
1043+
if (planId && matchedPlan && !matchedPlan.unassigned) {
1044+
void loadPlanAccountNames(planId, card);
1045+
}
10021046
});
10031047

10041048
// Add event listeners

‎internal/api/handler_test.go‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1341,6 +1341,69 @@ func TestHandler_HandleRequest_ListPlans_Error(t *testing.T) {
13411341
assert.Equal(t, 500, resp.StatusCode)
13421342
}
13431343

1344+
// TestHandler_HandleRequest_ListPlans_UnassignedFlagged is the regression
1345+
// guard for issue #973: a plan with zero plan_accounts rows (legacy
1346+
// "universal" plan) must appear in the response flagged as unassigned=true
1347+
// when an account filter is active. Before the fix such plans were silently
1348+
// dropped by the INNER JOIN on plan_accounts.
1349+
func TestHandler_HandleRequest_ListPlans_UnassignedFlagged(t *testing.T) {
1350+
ctx := context.Background()
1351+
mockStore := new(MockConfigStore)
1352+
mockAuth := new(MockAuthService)
1353+
1354+
adminSession := &Session{UserID: "admin-id", Email: "admin@example.com"}
1355+
mockAuth.On("ValidateSession", ctx, "test-token").Return(adminSession, nil)
1356+
mockAuth.grantAdmin()
1357+
1358+
// Simulate the store returning one assigned plan and one zero-account
1359+
// (unassigned) legacy plan.
1360+
assigned := config.PurchasePlan{ID: "11111111-1111-1111-1111-111111111111", Name: "Assigned Plan", Unassigned: false}
1361+
legacy := config.PurchasePlan{ID: "22222222-2222-2222-2222-222222222222", Name: "Legacy Plan", Unassigned: true}
1362+
mockStore.On("ListPurchasePlans", mock.Anything, mock.Anything).Return([]config.PurchasePlan{assigned, legacy}, nil)
1363+
1364+
handler := &Handler{config: mockStore, auth: mockAuth, apiKey: "test-key"}
1365+
1366+
req := &events.LambdaFunctionURLRequest{
1367+
Headers: map[string]string{
1368+
"X-API-Key": "test-key",
1369+
"Authorization": "Bearer test-token",
1370+
},
1371+
QueryStringParameters: map[string]string{
1372+
"account_ids": "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
1373+
},
1374+
RequestContext: events.LambdaFunctionURLRequestContext{
1375+
HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{
1376+
Method: "GET",
1377+
Path: "/api/plans",
1378+
},
1379+
},
1380+
}
1381+
1382+
resp, err := handler.HandleRequest(ctx, req)
1383+
require.NoError(t, err)
1384+
assert.Equal(t, 200, resp.StatusCode)
1385+
1386+
var body PlansResponse
1387+
require.NoError(t, json.Unmarshal([]byte(resp.Body), &body))
1388+
require.Len(t, body.Plans, 2)
1389+
1390+
// Find plans by ID to avoid order dependence.
1391+
plansByID := make(map[string]config.PurchasePlan, 2)
1392+
for _, p := range body.Plans {
1393+
plansByID[p.ID] = p
1394+
}
1395+
1396+
// Assigned plan must not carry the unassigned flag.
1397+
ap, ok := plansByID["11111111-1111-1111-1111-111111111111"]
1398+
require.True(t, ok, "assigned plan missing from response")
1399+
assert.False(t, ap.Unassigned, "assigned plan must have unassigned=false")
1400+
1401+
// Legacy zero-account plan must carry the unassigned flag.
1402+
lp, ok := plansByID["22222222-2222-2222-2222-222222222222"]
1403+
require.True(t, ok, "legacy unassigned plan missing from response")
1404+
assert.True(t, lp.Unassigned, "zero-account plan must have unassigned=true")
1405+
}
1406+
13441407
// Test for updateConfig error case - invalid JSON returns 500 (not 400)
13451408
func TestHandler_HandleRequest_UpdateConfig_InvalidJSON(t *testing.T) {
13461409
ctx := context.Background()

0 commit comments

Comments
 (0)