From f58da41520fdc49da45a895e7f6e3b2c1c2f10e5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 14:18:35 +0200 Subject: [PATCH 1/8] feat(api): compute per-plan health score for the Plans list (closes #340 follow-up, revives #376) Adds computePlanHealth, a pure 0-100 scoring function derived from a plan's ramp-schedule attributes and its recent failed/cancelled executions, with named penalty factors (overdue, failed_executions, cancelled_executions, stalled, behind_schedule, disabled_midway) so a bad score is actionable instead of opaque. listPlans now returns PlanWithHealth (PurchasePlan plus health_score/ health_factors) computed at read time; the score is never persisted. Reuses the existing GetExecutionsByStatuses call shape rather than adding a new store method. If that fetch fails, every plan defaults to a perfect score rather than a partial score or a 500. --- internal/api/handler_plans.go | 39 +++- internal/api/handler_plans_test.go | 95 ++++++++ internal/api/handler_test.go | 8 +- internal/api/openapi.yaml | 33 ++- internal/api/plan_health.go | 253 ++++++++++++++++++++++ internal/api/plan_health_test.go | 333 +++++++++++++++++++++++++++++ internal/api/types.go | 14 +- 7 files changed, 771 insertions(+), 4 deletions(-) create mode 100644 internal/api/plan_health.go create mode 100644 internal/api/plan_health_test.go diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index e175d8471..85301f0bb 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -45,7 +45,44 @@ func (h *Handler) listPlans(ctx context.Context, req *events.LambdaFunctionURLRe } } - return &PlansResponse{Plans: plans}, nil + return &PlansResponse{Plans: h.attachPlanHealth(ctx, plans, now)}, nil +} + +// attachPlanHealth computes the per-plan health-score badge (issue #340 +// follow-up) for every plan in the list. Reuses the existing +// GetExecutionsByStatuses call shape -- the same set the History handler +// already uses (handler_history.go), capped at config.DefaultListLimit -- +// instead of adding a new plan-scoped store method. +// +// If the executions fetch fails, the failure is logged and every plan +// defaults to a perfect score with no factors, rather than computing a +// partial score from attributes alone (which would look complete but +// silently omit the failed/canceled-execution factors) or failing the +// whole request with a 500. A degraded-but-visible plans list beats an +// opaque failure on a page that isn't primarily about purchase executions. +func (h *Handler) attachPlanHealth(ctx context.Context, plans []config.PurchasePlan, now time.Time) []PlanWithHealth { + result := make([]PlanWithHealth, len(plans)) + + execs, err := h.config.GetExecutionsByStatuses(ctx, planHealthExecutionStatuses, config.DefaultListLimit) + if err != nil { + logging.Warnf("listPlans: GetExecutionsByStatuses failed, plan health scores default to 100: %v", err) + for i := range plans { + result[i] = PlanWithHealth{PurchasePlan: plans[i], HealthScore: planHealthScoreMax} + } + return result + } + + execsByPlan := make(map[string][]config.PurchaseExecution, len(execs)) + for _rvc := range execs { + e := execs[_rvc] + execsByPlan[e.PlanID] = append(execsByPlan[e.PlanID], e) + } + + for i := range plans { + score, factors := computePlanHealth(plans[i], now, execsByPlan[plans[i].ID]) + result[i] = PlanWithHealth{PurchasePlan: plans[i], HealthScore: score, HealthFactors: factors} + } + return result } // calculateNextExecutionDate calculates the next execution date for a plan. diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 10c383ee7..ed7ad6fd2 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -33,6 +33,10 @@ func TestHandler_listPlans(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + // listPlans now also fetches recent failed/canceled executions to + // compute each plan's health-score badge (issue #340 follow-up). + mockStore.On("GetExecutionsByStatuses", ctx, planHealthExecutionStatuses, config.DefaultListLimit). + Return([]config.PurchaseExecution{}, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -45,6 +49,10 @@ func TestHandler_listPlans(t *testing.T) { require.NoError(t, err) assert.Len(t, result.Plans, 2) + // Neither seeded plan has ramp-schedule or execution data that would + // trigger a penalty, so both default to a perfect score. + assert.Equal(t, 100, result.Plans[0].HealthScore) + assert.Equal(t, 100, result.Plans[1].HealthScore) } func TestHandler_listPlans_AccountIDsFilter(t *testing.T) { @@ -67,6 +75,8 @@ func TestHandler_listPlans_AccountIDsFilter(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() mockStore.On("ListPurchasePlans", ctx, expectedFilter).Return(plans, nil) + mockStore.On("GetExecutionsByStatuses", ctx, planHealthExecutionStatuses, config.DefaultListLimit). + Return([]config.PurchaseExecution{}, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -83,6 +93,91 @@ func TestHandler_listPlans_AccountIDsFilter(t *testing.T) { assert.Equal(t, "Account Plan", result.Plans[0].Name) } +// TestHandler_listPlans_HealthScoreReflectsFailedExecutions is the +// end-to-end wiring check for the issue #340 health-score badge: a plan +// with failed executions in the store must come back from listPlans with a +// penalized HealthScore and the matching HealthFactors entry, and a +// failed execution belonging to a DIFFERENT plan must not bleed into this +// plan's score. +func TestHandler_listPlans_HealthScoreReflectsFailedExecutions(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + + troubledPlanID := "11111111-1111-1111-1111-111111111111" + otherPlanID := "22222222-2222-2222-2222-222222222222" + plans := []config.PurchasePlan{ + {ID: troubledPlanID, Name: "Troubled Plan", Enabled: true}, + {ID: otherPlanID, Name: "Other Plan", Enabled: true}, + } + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + mockStore.On("GetExecutionsByStatuses", ctx, planHealthExecutionStatuses, config.DefaultListLimit). + Return([]config.PurchaseExecution{ + {PlanID: troubledPlanID, Status: "failed"}, + {PlanID: troubledPlanID, Status: "failed"}, + {PlanID: otherPlanID, Status: config.StatusCanceled}, + }, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}} + + result, err := handler.listPlans(ctx, req, map[string]string{}) + require.NoError(t, err) + require.Len(t, result.Plans, 2) + + byID := make(map[string]PlanWithHealth, 2) + for _, p := range result.Plans { + byID[p.ID] = p + } + + troubled := byID[troubledPlanID] + assert.Equal(t, 100-2*penaltyPerFailedExec, troubled.HealthScore) + require.Len(t, troubled.HealthFactors, 1) + assert.Equal(t, HealthFactorFailedExecutions, troubled.HealthFactors[0].Code) + + other := byID[otherPlanID] + assert.Equal(t, 100-penaltyPerCanceledExec, other.HealthScore) + require.Len(t, other.HealthFactors, 1) + assert.Equal(t, HealthFactorCanceledExecutions, other.HealthFactors[0].Code) +} + +// TestHandler_listPlans_HealthScoreDegradesToDefaultOnExecutionsFetchError +// verifies the documented degraded-mode behavior in attachPlanHealth: if +// GetExecutionsByStatuses fails, listPlans still succeeds (never a 500 on +// a page that isn't primarily about purchase executions) and every plan +// gets a perfect default score with no factors, rather than a partially +// computed score that would look complete but silently omit the +// execution-based factors. +func TestHandler_listPlans_HealthScoreDegradesToDefaultOnExecutionsFetchError(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + + plans := []config.PurchasePlan{ + {ID: "11111111-1111-1111-1111-111111111111", Name: "Some Plan", Enabled: true}, + } + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + mockStore.On("GetExecutionsByStatuses", ctx, planHealthExecutionStatuses, config.DefaultListLimit). + Return(nil, errors.New("db unavailable")) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}} + + result, err := handler.listPlans(ctx, req, map[string]string{}) + require.NoError(t, err) + require.Len(t, result.Plans, 1) + assert.Equal(t, 100, result.Plans[0].HealthScore) + assert.Empty(t, result.Plans[0].HealthFactors) +} + func TestHandler_createPlan(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 52ef69675..0c7d80855 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -661,6 +661,10 @@ func TestHandler_HandleRequest_ListPlans(t *testing.T) { plans := []config.PurchasePlan{{ID: "11111111-1111-1111-1111-111111111111"}} mockStore.On("ListPurchasePlans", mock.Anything, mock.Anything).Return(plans, nil) + // listPlans now also fetches recent failed/canceled executions to + // compute each plan's health-score badge (issue #340 follow-up). + mockStore.On("GetExecutionsByStatuses", mock.Anything, mock.Anything, mock.Anything). + Return([]config.PurchaseExecution{}, nil) handler := &Handler{config: mockStore, auth: mockAuth, apiKey: "test-key"} @@ -1473,6 +1477,8 @@ func TestHandler_HandleRequest_ListPlans_UnassignedFlagged(t *testing.T) { 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) + mockStore.On("GetExecutionsByStatuses", mock.Anything, mock.Anything, mock.Anything). + Return([]config.PurchaseExecution{}, nil) handler := &Handler{config: mockStore, auth: mockAuth, apiKey: "test-key"} @@ -1501,7 +1507,7 @@ func TestHandler_HandleRequest_ListPlans_UnassignedFlagged(t *testing.T) { require.Len(t, body.Plans, 2) // Find plans by ID to avoid order dependence. - plansByID := make(map[string]config.PurchasePlan, 2) + plansByID := make(map[string]PlanWithHealth, 2) for _, p := range body.Plans { plansByID[p.ID] = p } diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 04a9bfcee..0232c07b6 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2339,7 +2339,7 @@ components: plans: type: array items: - $ref: '#/components/schemas/PurchasePlan' + $ref: '#/components/schemas/PlanWithHealth' PlanRequest: type: object @@ -2422,6 +2422,37 @@ components: format: date-time nullable: true + # PlanWithHealth wraps PurchasePlan with a read-time-only health-score + # badge (issue #340 follow-up): score/factors are computed from the + # plan's attributes and recent executions, never persisted, so they + # can't drift into the PurchasePlan schema above. + PlanWithHealth: + allOf: + - $ref: '#/components/schemas/PurchasePlan' + - type: object + properties: + health_score: + type: integer + minimum: 0 + maximum: 100 + description: 0-100 plan health score. Green >= 80, amber 50-79, red < 50. + health_factors: + type: array + items: + $ref: '#/components/schemas/PlanHealthFactor' + + PlanHealthFactor: + type: object + description: One penalty subtracted from a plan's health score, named so a bad score is actionable instead of opaque. + properties: + code: + type: string + enum: [overdue, failed_executions, canceled_executions, stalled, behind_schedule, disabled_midway] + penalty: + type: integer + note: + type: string + RampSchedule: type: object properties: diff --git a/internal/api/plan_health.go b/internal/api/plan_health.go new file mode 100644 index 000000000..4b0caa595 --- /dev/null +++ b/internal/api/plan_health.go @@ -0,0 +1,253 @@ +// Package api provides the HTTP API handlers for the CUDly dashboard. +package api + +import ( + "fmt" + "time" + + "github.com/LeanerCloud/CUDly/internal/config" +) + +// PlanHealthFactorCode identifies a single scoring factor in a plan's +// health-score breakdown (issue #340 follow-up). Typed so the frontend +// tooltip renders deterministic wording per factor instead of parsing free +// text out of Note. +type PlanHealthFactorCode string + +// Health-factor codes, in the same order computePlanHealth evaluates them. +const ( + HealthFactorOverdue PlanHealthFactorCode = "overdue" + HealthFactorFailedExecutions PlanHealthFactorCode = "failed_executions" + HealthFactorCanceledExecutions PlanHealthFactorCode = "canceled_executions" + HealthFactorStalled PlanHealthFactorCode = "stalled" + HealthFactorBehindSchedule PlanHealthFactorCode = "behind_schedule" + HealthFactorDisabledMidway PlanHealthFactorCode = "disabled_midway" +) + +// PlanHealthFactor is one penalty applied when computing a plan's health +// score. Returned alongside the score so a bad score is actionable instead +// of opaque: the frontend tooltip enumerates every factor and its penalty. +type PlanHealthFactor struct { + Code PlanHealthFactorCode `json:"code"` + Penalty int `json:"penalty"` + Note string `json:"note"` +} + +// Score bounds and per-factor weights for computePlanHealth. Named +// constants rather than inline magic numbers so the scoring table in the +// package doc / issue #340 has a single source of truth. +const ( + planHealthScoreMax = 100 + planHealthScoreMin = 0 + + penaltyOverdue = 30 + penaltyPerFailedExec = 10 + penaltyPerCanceledExec = 5 + penaltyStalled = 15 + penaltyBehindSchedule = 20 + penaltyDisabledMidway = 25 + + // maxCountedFailedExecs / maxCountedCanceledExecs cap how many rows + // count toward their respective penalty, so a plan with a long failure + // history can't score below the documented worst case for that factor + // (-40 / -20 respectively). + maxCountedFailedExecs = 4 + maxCountedCanceledExecs = 4 +) + +// planHealthStatusFailed mirrors the "failed" execution-status literal used +// throughout internal/api (e.g. handler_purchases.go finalizePurchaseStatus). +// No config.Status* constant exists for it today -- only StatusCanceled and +// LegacyStatusCanceled do -- so this local constant is the typed handle for +// this file. +const planHealthStatusFailed = "failed" + +// planHealthExecutionStatuses selects the execution rows computePlanHealth +// needs (failed + both spellings of canceled). Passed to the existing +// GetExecutionsByStatuses store method -- the same call shape the History +// handler already uses (see handler_history.go), capped at +// config.DefaultListLimit -- rather than adding a new plan-scoped store +// method. +var planHealthExecutionStatuses = []string{ + planHealthStatusFailed, + config.StatusCanceled, + config.LegacyStatusCanceled, +} + +// computePlanHealth derives a 0-100 health score for a single purchase plan +// from its ramp-schedule attributes and its own executions (the caller is +// expected to have already filtered `executions` down to this plan's +// PlanID). Starts at 100 and subtracts weighted penalties, clamped to +// [planHealthScoreMin, planHealthScoreMax]. The returned factors document +// exactly which penalties applied so a bad score is actionable instead of +// opaque (issue #340 follow-up, PR #376). +// +// A completed plan (RampSchedule.IsComplete(), guarded against the +// degenerate TotalSteps == 0 case) short-circuits to a perfect score: +// historical failure/cancellation rows and a disabled toggle-off after +// completion are expected end-of-life noise, not signals an operator +// should chase. +func computePlanHealth(plan config.PurchasePlan, now time.Time, executions []config.PurchaseExecution) (int, []PlanHealthFactor) { + if plan.RampSchedule.TotalSteps > 0 && plan.RampSchedule.IsComplete() { + return planHealthScoreMax, nil + } + + factors := collectPlanHealthFactors(plan, now, executions) + + score := planHealthScoreMax + for _, f := range factors { + score -= f.Penalty + } + if score < planHealthScoreMin { + score = planHealthScoreMin + } + return score, factors +} + +// collectPlanHealthFactors runs every per-factor check and returns the +// subset that applies to plan, in table order (overdue, failed_executions, +// canceled_executions, then the mutually-exclusive stalled/behind_schedule +// pair, then disabled_midway). Split out from computePlanHealth so each +// factor stays an independent, individually testable function. +func collectPlanHealthFactors(plan config.PurchasePlan, now time.Time, executions []config.PurchaseExecution) []PlanHealthFactor { + var factors []PlanHealthFactor + if f, ok := overdueFactor(plan, now); ok { + factors = append(factors, f) + } + if f, ok := failedExecutionsFactor(executions); ok { + factors = append(factors, f) + } + if f, ok := canceledExecutionsFactor(executions); ok { + factors = append(factors, f) + } + if f, ok := scheduleFactor(plan, now); ok { + factors = append(factors, f) + } + if f, ok := disabledMidwayFactor(plan); ok { + factors = append(factors, f) + } + return factors +} + +// overdueFactor: enabled AND next_execution_date < now. +func overdueFactor(plan config.PurchasePlan, now time.Time) (PlanHealthFactor, bool) { + if !plan.Enabled || plan.NextExecutionDate == nil || !plan.NextExecutionDate.Before(now) { + return PlanHealthFactor{}, false + } + return PlanHealthFactor{ + Code: HealthFactorOverdue, + Penalty: penaltyOverdue, + Note: "next purchase date has passed", + }, true +} + +// failedExecutionsFactor: -10 per failed execution, capped at 4 (-40 max). +// The note reports the true (uncapped) count so an operator can see the +// full extent even when the penalty itself is capped. +func failedExecutionsFactor(executions []config.PurchaseExecution) (PlanHealthFactor, bool) { + count := countExecutionsByStatus(executions, planHealthStatusFailed) + if count == 0 { + return PlanHealthFactor{}, false + } + counted := count + if counted > maxCountedFailedExecs { + counted = maxCountedFailedExecs + } + return PlanHealthFactor{ + Code: HealthFactorFailedExecutions, + Penalty: counted * penaltyPerFailedExec, + Note: fmt.Sprintf("%d failed execution(s)", count), + }, true +} + +// canceledExecutionsFactor: -5 per canceled execution, capped at 4 (-20 +// max). Counts both spellings of "canceled" (config.StatusCanceled and the +// legacy config.LegacyStatusCanceled) so pre-#1278 rows aren't invisible to +// scoring. +func canceledExecutionsFactor(executions []config.PurchaseExecution) (PlanHealthFactor, bool) { + count := countExecutionsByStatus(executions, config.StatusCanceled, config.LegacyStatusCanceled) + if count == 0 { + return PlanHealthFactor{}, false + } + counted := count + if counted > maxCountedCanceledExecs { + counted = maxCountedCanceledExecs + } + return PlanHealthFactor{ + Code: HealthFactorCanceledExecutions, + Penalty: counted * penaltyPerCanceledExec, + Note: fmt.Sprintf("%d canceled execution(s)", count), + }, true +} + +// countExecutionsByStatus counts executions whose Status matches any of the +// supplied values. +func countExecutionsByStatus(executions []config.PurchaseExecution, statuses ...string) int { + count := 0 + for _rvc := range executions { + status := executions[_rvc].Status + for _, s := range statuses { + if status == s { + count++ + break + } + } + } + return count +} + +// scheduleFactor evaluates the ramp schedule's progress against wall-clock +// time and returns at most one of two mutually exclusive factors: +// +// - stalled (-15): enabled, past the first scheduled interval, but no +// step has executed yet (CurrentStep == 0). +// - behind_schedule (-20): CurrentStep lags the step implied by +// StartDate + StepIntervalDays, in any other case (including a +// disabled plan that was never started). +// +// Plans with no positive StepIntervalDays (e.g. "immediate" ramp schedules) +// have no schedule position to fall behind on and are skipped entirely -- +// there is no divide-by-zero guard needed elsewhere because this is the +// only place StepIntervalDays is used as a divisor. +func scheduleFactor(plan config.PurchasePlan, now time.Time) (PlanHealthFactor, bool) { + ramp := plan.RampSchedule + if ramp.StepIntervalDays <= 0 { + return PlanHealthFactor{}, false + } + + daysSinceStart := int(now.Sub(ramp.StartDate).Hours() / 24) + if daysSinceStart < ramp.StepIntervalDays { + return PlanHealthFactor{}, false + } + + if plan.Enabled && ramp.CurrentStep == 0 { + return PlanHealthFactor{ + Code: HealthFactorStalled, + Penalty: penaltyStalled, + Note: "enabled but no purchases have executed since the first scheduled step", + }, true + } + + expectedStep := daysSinceStart / ramp.StepIntervalDays + if ramp.CurrentStep >= expectedStep { + return PlanHealthFactor{}, false + } + return PlanHealthFactor{ + Code: HealthFactorBehindSchedule, + Penalty: penaltyBehindSchedule, + Note: fmt.Sprintf("on step %d, expected step %d by now", ramp.CurrentStep, expectedStep), + }, true +} + +// disabledMidwayFactor: disabled with 0 < current_step < total_steps. +func disabledMidwayFactor(plan config.PurchasePlan) (PlanHealthFactor, bool) { + ramp := plan.RampSchedule + if plan.Enabled || ramp.CurrentStep <= 0 || ramp.TotalSteps <= 0 || ramp.CurrentStep >= ramp.TotalSteps { + return PlanHealthFactor{}, false + } + return PlanHealthFactor{ + Code: HealthFactorDisabledMidway, + Penalty: penaltyDisabledMidway, + Note: "disabled before completing its ramp schedule", + }, true +} diff --git a/internal/api/plan_health_test.go b/internal/api/plan_health_test.go new file mode 100644 index 000000000..2fdecf853 --- /dev/null +++ b/internal/api/plan_health_test.go @@ -0,0 +1,333 @@ +package api + +import ( + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// factorCodes extracts the Code of each factor, in order, for easy +// assertion without caring about penalty/note wording in most tests. +func factorCodes(factors []PlanHealthFactor) []PlanHealthFactorCode { + codes := make([]PlanHealthFactorCode, len(factors)) + for i, f := range factors { + codes[i] = f.Code + } + return codes +} + +func TestComputePlanHealth_HealthyPlanScoresPerfect(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: true, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 1, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -1), // just started, well within the first interval + }, + } + + score, factors := computePlanHealth(plan, now, nil) + + assert.Equal(t, 100, score) + assert.Empty(t, factors) +} + +func TestComputePlanHealth_CompletedPlanShortCircuitsRegardlessOfOtherIssues(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + overdueDate := now.AddDate(0, 0, -5) + plan := config.PurchasePlan{ + Enabled: false, // would otherwise trip disabled_midway + NextExecutionDate: &overdueDate, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 4, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -60), + }, + } + executions := []config.PurchaseExecution{ + {Status: "failed"}, {Status: "failed"}, {Status: config.StatusCanceled}, + } + + score, factors := computePlanHealth(plan, now, executions) + + assert.Equal(t, 100, score) + assert.Empty(t, factors) +} + +func TestComputePlanHealth_Overdue(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + past := now.AddDate(0, 0, -1) + plan := config.PurchasePlan{ + Enabled: true, + NextExecutionDate: &past, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 30, + CurrentStep: 1, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -1), + }, + } + + score, factors := computePlanHealth(plan, now, nil) + + require.Len(t, factors, 1) + assert.Equal(t, HealthFactorOverdue, factors[0].Code) + assert.Equal(t, 30, factors[0].Penalty) + assert.Equal(t, 100-30, score) +} + +func TestComputePlanHealth_OverdueRequiresEnabled(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + past := now.AddDate(0, 0, -1) + plan := config.PurchasePlan{ + Enabled: false, + NextExecutionDate: &past, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 30, + CurrentStep: 1, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -1), + }, + } + + _, factors := computePlanHealth(plan, now, nil) + + assert.NotContains(t, factorCodes(factors), HealthFactorOverdue) +} + +func TestComputePlanHealth_FailedExecutionsPenaltyCapsAtFour(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: true, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 30, + CurrentStep: 1, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -1), + }, + } + // 6 failed executions: penalty must cap at 4 * 10 = 40, but the note + // must still report the true count of 6. + var executions []config.PurchaseExecution + for i := 0; i < 6; i++ { + executions = append(executions, config.PurchaseExecution{Status: "failed"}) + } + + score, factors := computePlanHealth(plan, now, executions) + + require.Len(t, factors, 1) + assert.Equal(t, HealthFactorFailedExecutions, factors[0].Code) + assert.Equal(t, 40, factors[0].Penalty) + assert.Contains(t, factors[0].Note, "6 failed") + assert.Equal(t, 100-40, score) +} + +func TestComputePlanHealth_CanceledExecutionsPenaltyCapsAtFourAndCountsBothSpellings(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: true, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 30, + CurrentStep: 1, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -1), + }, + } + // 3 canonical + 2 legacy-spelled = 5 total canceled; penalty caps at + // 4 * 5 = 20 but the note reports the true count of 5. + executions := []config.PurchaseExecution{ + {Status: config.StatusCanceled}, + {Status: config.StatusCanceled}, + {Status: config.StatusCanceled}, + {Status: config.LegacyStatusCanceled}, + {Status: config.LegacyStatusCanceled}, + } + + score, factors := computePlanHealth(plan, now, executions) + + require.Len(t, factors, 1) + assert.Equal(t, HealthFactorCanceledExecutions, factors[0].Code) + assert.Equal(t, 20, factors[0].Penalty) + assert.Contains(t, factors[0].Note, "5 canceled") + assert.Equal(t, 100-20, score) +} + +func TestComputePlanHealth_Stalled(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: true, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 0, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -10), // past the first interval + }, + } + + score, factors := computePlanHealth(plan, now, nil) + + require.Len(t, factors, 1) + assert.Equal(t, HealthFactorStalled, factors[0].Code) + assert.Equal(t, 15, factors[0].Penalty) + assert.Equal(t, 100-15, score) +} + +func TestComputePlanHealth_BehindSchedule(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: true, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 1, // expected step by day 30 is 4 + TotalSteps: 6, + StartDate: now.AddDate(0, 0, -30), + }, + } + + score, factors := computePlanHealth(plan, now, nil) + + require.Len(t, factors, 1) + assert.Equal(t, HealthFactorBehindSchedule, factors[0].Code) + assert.Equal(t, 20, factors[0].Penalty) + assert.Contains(t, factors[0].Note, "step 1") + assert.Equal(t, 100-20, score) +} + +func TestComputePlanHealth_StalledAndBehindScheduleAreMutuallyExclusive(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: true, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 0, + TotalSteps: 6, + StartDate: now.AddDate(0, 0, -30), + }, + } + + _, factors := computePlanHealth(plan, now, nil) + + codes := factorCodes(factors) + assert.Contains(t, codes, HealthFactorStalled) + assert.NotContains(t, codes, HealthFactorBehindSchedule) +} + +func TestComputePlanHealth_ImmediatePlanSkipsScheduleFactors(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: true, + RampSchedule: config.RampSchedule{ + Type: "immediate", + StepIntervalDays: 0, // no positive interval: nothing to fall behind on + CurrentStep: 0, + TotalSteps: 1, + StartDate: now.AddDate(0, 0, -30), + }, + } + + _, factors := computePlanHealth(plan, now, nil) + + codes := factorCodes(factors) + assert.NotContains(t, codes, HealthFactorStalled) + assert.NotContains(t, codes, HealthFactorBehindSchedule) +} + +func TestComputePlanHealth_DisabledMidway(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: false, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 2, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -1), + }, + } + + score, factors := computePlanHealth(plan, now, nil) + + require.Len(t, factors, 1) + assert.Equal(t, HealthFactorDisabledMidway, factors[0].Code) + assert.Equal(t, 25, factors[0].Penalty) + assert.Equal(t, 100-25, score) +} + +func TestComputePlanHealth_DisabledButNeverStartedIsBehindScheduleNotDisabledMidway(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: false, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 0, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -30), + }, + } + + _, factors := computePlanHealth(plan, now, nil) + + codes := factorCodes(factors) + assert.Contains(t, codes, HealthFactorBehindSchedule) + assert.NotContains(t, codes, HealthFactorDisabledMidway) + assert.NotContains(t, codes, HealthFactorStalled) +} + +func TestComputePlanHealth_ScoreClampsAtZeroWhenPenaltiesStack(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + past := now.AddDate(0, 0, -1) + plan := config.PurchasePlan{ + Enabled: false, + NextExecutionDate: &past, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 1, // way behind the expected step + TotalSteps: 10, + StartDate: now.AddDate(0, 0, -90), + }, + } + var executions []config.PurchaseExecution + for i := 0; i < 6; i++ { + executions = append(executions, config.PurchaseExecution{Status: "failed"}) + } + for i := 0; i < 6; i++ { + executions = append(executions, config.PurchaseExecution{Status: config.StatusCanceled}) + } + + score, factors := computePlanHealth(plan, now, executions) + + assert.Equal(t, 0, score) + // disabled_midway note: overdue only requires Enabled, which is false + // here, so overdue must NOT fire even though NextExecutionDate is past. + assert.NotContains(t, factorCodes(factors), HealthFactorOverdue) + assert.Contains(t, factorCodes(factors), HealthFactorDisabledMidway) + assert.Contains(t, factorCodes(factors), HealthFactorBehindSchedule) + assert.Contains(t, factorCodes(factors), HealthFactorFailedExecutions) + assert.Contains(t, factorCodes(factors), HealthFactorCanceledExecutions) +} + +func TestComputePlanHealth_UnrelatedExecutionStatusesAreNotCounted(t *testing.T) { + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + plan := config.PurchasePlan{ + Enabled: true, + RampSchedule: config.RampSchedule{ + StepIntervalDays: 30, + CurrentStep: 1, + TotalSteps: 4, + StartDate: now.AddDate(0, 0, -1), + }, + } + executions := []config.PurchaseExecution{ + {Status: "pending"}, {Status: "notified"}, {Status: "completed"}, {Status: "approved"}, + } + + score, factors := computePlanHealth(plan, now, executions) + + assert.Equal(t, 100, score) + assert.Empty(t, factors) +} diff --git a/internal/api/types.go b/internal/api/types.go index 293d21fda..3d98a4afe 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -442,7 +442,19 @@ type RecommendationDetailResponse struct { // PlansResponse holds the purchase plans response. type PlansResponse struct { - Plans []config.PurchasePlan `json:"plans"` + Plans []PlanWithHealth `json:"plans"` +} + +// PlanWithHealth wraps a config.PurchasePlan with its computed health-score +// badge data for the Plans list view (issue #340 follow-up, PR #376). The +// score/factors are derived at read time from the plan's own attributes and +// its recent executions -- see computePlanHealth in plan_health.go -- and +// are never persisted, so they can't drift into the config.PurchasePlan +// struct (and by extension the DynamoDB/Postgres columns backing it). +type PlanWithHealth struct { + config.PurchasePlan + HealthScore int `json:"health_score"` + HealthFactors []PlanHealthFactor `json:"health_factors,omitempty"` } // CurrentUserResponse holds the current user response. From e330750fe77539c6f45e29c189205460d4ed1d0d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 14:19:06 +0200 Subject: [PATCH 2/8] feat(frontend): render per-plan health-score badge on the Plans tab (closes #340 follow-up, revives #376) Adds a colour-banded health badge (green >= 80, amber 50-79, red < 50) next to the status badge on each plan card, reusing the existing status-badge/badge-success/badge-warning/badge-danger palette instead of new CSS. The badge's title tooltip enumerates every penalty factor returned by the backend so a bad score is actionable. health_score/health_factors are optional on Plan/LocalPlan/BackendPlan so a cached response from before this feature shipped still renders the card without the badge. Every factor note goes through escapeHtmlAttr before it lands in the title attribute. --- frontend/src/__tests__/plans.test.ts | 99 ++++++++++++++++++++++++++++ frontend/src/api/index.ts | 1 + frontend/src/api/types.ts | 16 +++++ frontend/src/plans.ts | 38 +++++++++++ frontend/src/types.ts | 4 ++ 5 files changed, 158 insertions(+) diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index d35465c18..16da13143 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -3070,4 +3070,103 @@ describe('Plans Module', () => { expect(paymentSelect.value).toBe('all-upfront'); }); }); + + // Issue #340 follow-up (PR #376): per-plan health-score badge. + describe('plan health-score badge', () => { + const basePlan = { + id: 'plan-1', + name: 'Test Plan', + enabled: true, + auto_purchase: true, + services: { + ec2: { provider: 'aws', service: 'ec2', enabled: true, term: 1, payment: 'all-upfront', coverage: 80 } + }, + ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0 } + }; + + test('renders a green badge for a healthy score (>= 80)', async () => { + (api.getPlans as jest.Mock).mockResolvedValue({ + plans: [{ ...basePlan, health_score: 90, health_factors: [] }] + }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + await loadPlans(); + + const list = document.getElementById('plans-list'); + expect(list?.innerHTML).toContain('badge-success'); + expect(list?.innerHTML).toContain('Health: 90'); + }); + + test('renders an amber badge for a medium score (50-79)', async () => { + (api.getPlans as jest.Mock).mockResolvedValue({ + plans: [{ + ...basePlan, + health_score: 65, + health_factors: [{ code: 'behind_schedule', penalty: 20, note: 'on step 1, expected step 3 by now' }] + }] + }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + await loadPlans(); + + const list = document.getElementById('plans-list'); + expect(list?.innerHTML).toContain('badge-warning'); + expect(list?.innerHTML).toContain('Health: 65'); + }); + + test('renders a red badge for a poor score (< 50)', async () => { + (api.getPlans as jest.Mock).mockResolvedValue({ + plans: [{ + ...basePlan, + health_score: 30, + health_factors: [ + { code: 'overdue', penalty: 30, note: 'next purchase date has passed' }, + { code: 'failed_executions', penalty: 40, note: '4 failed execution(s)' } + ] + }] + }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + await loadPlans(); + + const list = document.getElementById('plans-list'); + expect(list?.innerHTML).toContain('badge-danger'); + expect(list?.innerHTML).toContain('Health: 30'); + }); + + test('omits the badge entirely when health_score is absent (pre-feature API response)', async () => { + (api.getPlans as jest.Mock).mockResolvedValue({ plans: [{ ...basePlan }] }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + await loadPlans(); + + const list = document.getElementById('plans-list'); + expect(list?.innerHTML).not.toContain('Health:'); + }); + + test('escapes health factor notes in the tooltip (XSS regression)', async () => { + const maliciousNote = '">'; + (api.getPlans as jest.Mock).mockResolvedValue({ + plans: [{ + ...basePlan, + health_score: 40, + health_factors: [{ code: 'stalled', penalty: 15, note: maliciousNote }] + }] + }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + await loadPlans(); + + const list = document.getElementById('plans-list'); + // Pre-fix, the unescaped `"` would close the title attribute early and + // the rest would parse as a live element. Post-fix the + // quote is entity-encoded so the whole malicious string stays inert + // text inside the title attribute -- assert on the parsed DOM (not the + // raw HTML string, since HTML serializers are free to leave `<`/`>` + // un-re-escaped inside an already-safe attribute value). + expect(list?.querySelector('img')).toBeNull(); + const badge = list?.querySelector('.badge-danger[title]'); + expect(badge?.getAttribute('title')).toContain(maliciousNote); + }); + }); }); diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index e59ac15d9..8b5c3e22f 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -17,6 +17,7 @@ export type { Recommendation, RecommendationFilters, Plan, + PlanHealthFactor, CreatePlanRequest, PurchaseHistory, HistoryFilters, diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index ec7dd3786..5e0ad933a 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -227,6 +227,22 @@ export interface Plan { // Such plans are surfaced under an "Unassigned" section in the Plans UI // so operators can find and re-scope them (issue #973). unassigned?: boolean; + // Health score/factors are computed at read time on GET /plans (never + // persisted) by computePlanHealth server-side (issue #340 follow-up). + // Optional so an older cached response served during a partial deploy + // still renders the card cleanly -- the badge just doesn't appear. + health_score?: number; + health_factors?: PlanHealthFactor[]; +} + +// PlanHealthFactor is one penalty subtracted from a plan's health score +// (issue #340 follow-up). `code` is a fixed vocabulary from the backend +// (see internal/api/plan_health.go); `note` is a human-readable reason +// suitable for the health-badge tooltip. +export interface PlanHealthFactor { + code: string; + penalty: number; + note: string; } export interface CreatePlanRequest { diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 369f99078..f69331920 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -861,6 +861,43 @@ interface BackendPlan { // (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; + // Health score/factors are computed at read time on GET /plans (never + // persisted) -- see computePlanHealth in internal/api/plan_health.go + // (issue #340 follow-up). Optional so an older cached response served + // during a partial deploy still renders the card cleanly. + health_score?: number; + health_factors?: api.PlanHealthFactor[]; +} + +// Score bands for the per-plan health badge (issue #340 follow-up). Green +// >= 80 (healthy), amber 50-79 (needs attention), red < 50 (action needed). +// Reuses the existing status-badge palette (badge-success/badge-warning/ +// badge-danger) already used elsewhere on this card rather than introducing +// new CSS. +const HEALTH_SCORE_GOOD_THRESHOLD = 80; +const HEALTH_SCORE_WARN_THRESHOLD = 50; + +function healthBadgeClass(score: number): string { + if (score >= HEALTH_SCORE_GOOD_THRESHOLD) return 'badge-success'; + if (score >= HEALTH_SCORE_WARN_THRESHOLD) return 'badge-warning'; + return 'badge-danger'; +} + +// healthBadgeHtml renders the per-plan health-score badge. Returns '' when +// health_score is absent (older API response during a partial deploy) so +// the card still renders cleanly without it. The tooltip enumerates every +// penalty factor so a bad score is actionable instead of opaque; every +// factor note is escaped since it renders inside an HTML attribute (title) +// via innerHTML (feedback_innerhtml_xss: all API-sourced values must be +// escaped here even though the backend generates these notes itself). +function healthBadgeHtml(plan: BackendPlan): string { + if (typeof plan.health_score !== 'number') return ''; + const factors = plan.health_factors || []; + const factorLines = factors.length > 0 + ? factors.map(f => `-${f.penalty}: ${f.note}`) + : ['No issues detected']; + const tooltip = [`Plan health: ${plan.health_score}/100`, ...factorLines].join('\n'); + return `Health: ${plan.health_score}`; } // Pretty label for a service slug used inside the plan card. @@ -992,6 +1029,7 @@ function renderPlanCard(plan: BackendPlan, canManagePlan: boolean, canDeletePlan

${escapeHtml(plan.name)}

${status.label} + ${healthBadgeHtml(plan)} ${overdueBadge} ${canManagePlan && !isUnassigned ? `