From cfb852502e8cb2ca61bade0f33b6e635f08dcf2f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:01:28 -0700 Subject: [PATCH 1/4] fix(api): current_savings zero for services with no active commitments (closes #1031) summarizeRecommendationsWithCoverage incorrectly wrote CurrentSavings to the coverage-scaled potential amount (same as PotentialSavings). Because getDashboardSummary only overwrites CurrentSavings for services present in the purchase-history result, services with recommendations but no active purchases shipped a non-zero current_savings equal to potential_savings. This was wrong: the frontend renders current_savings as committed/realized savings from active purchase history, so an uncommitted service must ship 0. The partial fix in #926 (aggregateActiveCommitmentsPerService + purchase- history overwrite) was correct in concept but left the incorrect assignment in summarizeRecommendationsWithCoverage active, masking the bug for services that had no purchases. Fix: remove svc.CurrentSavings += scaled from summarizeRecommendationsWithCoverage. CurrentSavings is getDashboardSummary's responsibility (via the overwrite loop that calls aggregateActiveCommitmentsPerService). The function comment and the three affected test assertions/functions are updated accordingly. Regression test added: TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments asserts current_savings: 0 for services with recommendations but no purchases. --- internal/api/handler_dashboard.go | 17 ++-- internal/api/handler_dashboard_test.go | 103 ++++++++++++++++++------- 2 files changed, 80 insertions(+), 40 deletions(-) diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index d6d49920e..2d2b31eaa 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -221,6 +221,12 @@ func (h *Handler) filterDashboardRecommendations(ctx context.Context, session *S // therefore correct: it projects "how much would I save if I only bought // RIs to cover X% of my instances" against the 100%-coverage baseline // that every provider gives us. Verified by TestSummarizeRecommendationsWithCoverage_100PctContract. +// +// Note: CurrentSavings is NOT set here. It represents committed/realized +// savings from active purchase history, populated separately in +// getDashboardSummary via aggregateActiveCommitmentsPerService. A service +// with recommendations but no active purchases correctly ships +// current_savings: 0. (Issue #1031) func summarizeRecommendationsWithCoverage( //nolint:gocritic // unnamedResult: return names would conflict with body locals recs []config.RecommendationRecord, coverageByKey map[string]float64, @@ -245,17 +251,6 @@ func summarizeRecommendationsWithCoverage( //nolint:gocritic // unnamedResult: r total += scaled svc := byService[rep.rec.Service] svc.PotentialSavings += scaled - // CurrentSavings is the committed/realized monthly savings for the - // service: the full 100%-coverage potential (rec.Savings) projected - // down to the operator-configured coverage %. scaledSavings already - // computes exactly that (rec.Savings * min(coverage,100)/100), so we - // reuse it rather than re-deriving the coverage lookup. When no - // coverage override exists the rec falls through to full savings, so - // CurrentSavings == PotentialSavings (nothing committed-away yet); a - // configured coverage < 100 pulls CurrentSavings below the potential. - // Issue #908: this field was previously never set, so the Home - // chart's current-savings underlay always rendered as $0. - svc.CurrentSavings += scaled byService[rep.rec.Service] = svc } return total, byService diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 99f9ed5c0..a1228d4b6 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -265,9 +265,10 @@ func TestSummarizeRecommendationsWithCoverage(t *testing.T) { total, byService := summarizeRecommendationsWithCoverage(tc.recs, tc.coverage) assert.InDelta(t, tc.wantTotal, total, 0.0001) assert.InDelta(t, tc.wantTotal, byService["rds"].PotentialSavings, 0.0001) - // Issue #908: CurrentSavings (committed/realized) is sourced from - // the same coverage-scaled amount, so it tracks the scaled total. - assert.InDelta(t, tc.wantTotal, byService["rds"].CurrentSavings, 0.0001) + // CurrentSavings is populated by getDashboardSummary from purchase + // history, not here — it must remain 0 from this function (issue #1031). + assert.Equal(t, 0.0, byService["rds"].CurrentSavings, + "summarize must not set CurrentSavings") }) } } @@ -322,15 +323,14 @@ func TestSummarizeRecommendationsWithCoverage_100PctContract(t *testing.T) { "nil coverage map must return un-scaled savings (issue #201 contract)") } -// TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings is the -// issue #908 regression: by_service[svc].current_savings must be populated -// (and keyed/scaled identically to potential) so the Home chart's -// current-savings underlay renders instead of being a flat $0 series. -// -// Before the fix, summarizeRecommendationsWithCoverage set only -// PotentialSavings, leaving CurrentSavings at its float64 zero value for -// every service regardless of configured coverage. -func TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings(t *testing.T) { +// TestSummarizeRecommendationsWithCoverage_DoesNotSetCurrentSavings asserts +// that summarizeRecommendationsWithCoverage only populates PotentialSavings. +// CurrentSavings represents committed/realized savings from active purchase +// history and is populated separately in getDashboardSummary via +// aggregateActiveCommitmentsPerService. Mixing the two here would cause a +// service with recommendations but no purchases to falsely report non-zero +// CurrentSavings (issue #1031). +func TestSummarizeRecommendationsWithCoverage_DoesNotSetCurrentSavings(t *testing.T) { acctA := "acct-A" keyEC2 := config.AccountConfigKey(acctA, "aws", "ec2") keyRDS := config.AccountConfigKey(acctA, "aws", "rds") @@ -345,21 +345,18 @@ func TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings(t *testing _, byService := summarizeRecommendationsWithCoverage(recs, coverage) - // current_savings is non-zero where coverage exists and is scaled the - // same way potential is (rec.Savings * coverage/100), keyed per service. - assert.InDelta(t, 600.0, byService["ec2"].CurrentSavings, 0.001, - "ec2 current_savings = 1000 * 60/100") - assert.InDelta(t, 100.0, byService["rds"].CurrentSavings, 0.001, - "rds current_savings = 400 * 25/100") - - // And it matches PotentialSavings (both flow from the same scaled amount). - assert.InDelta(t, byService["ec2"].PotentialSavings, byService["ec2"].CurrentSavings, 0.001) - assert.InDelta(t, byService["rds"].PotentialSavings, byService["rds"].CurrentSavings, 0.001) - - // Sanity: every populated service has a strictly positive current_savings - // when coverage is configured (the bug produced 0 here). - require.Positive(t, byService["ec2"].CurrentSavings) - require.Positive(t, byService["rds"].CurrentSavings) + // PotentialSavings is scaled correctly by coverage. + assert.InDelta(t, 600.0, byService["ec2"].PotentialSavings, 0.001, + "ec2 potential_savings = 1000 * 60/100") + assert.InDelta(t, 100.0, byService["rds"].PotentialSavings, 0.001, + "rds potential_savings = 400 * 25/100") + + // CurrentSavings is NOT set here — it remains the float64 zero value. + // getDashboardSummary populates it from purchase history (issue #1031). + assert.Equal(t, 0.0, byService["ec2"].CurrentSavings, + "summarize must not set CurrentSavings; that is getDashboardSummary's responsibility") + assert.Equal(t, 0.0, byService["rds"].CurrentSavings, + "summarize must not set CurrentSavings; that is getDashboardSummary's responsibility") } // TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell is the @@ -408,8 +405,8 @@ func TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell(t *testing. // sum of all variants (100+200+300+50 = 650). assert.InDelta(t, 350.0, byService["ec2"].PotentialSavings, 0.0001, "by_service potential must sum per-cell MAX (300+50), not all variants") - assert.InDelta(t, 350.0, byService["ec2"].CurrentSavings, 0.0001, - "by_service current must dedupe identically to potential") + assert.Equal(t, 0.0, byService["ec2"].CurrentSavings, + "summarize must not set CurrentSavings; getDashboardSummary populates it from purchase history") // The headline total dedupes the same way. assert.InDelta(t, 350.0, total, 0.0001, "total must sum per-cell MAX (300+50), not all variants") @@ -1269,6 +1266,54 @@ func TestHandler_getDashboardSummary_CurrentSavingsJSON(t *testing.T) { "current_savings field must carry the active purchase's EstimatedSavings") } +// TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments is the +// issue #1031 regression: a service with recommendations but no active purchase +// history must ship current_savings: 0, not current_savings: potential_savings. +// +// The bug was that summarizeRecommendationsWithCoverage set CurrentSavings to +// the same scaled value as PotentialSavings. getDashboardSummary only overwrites +// CurrentSavings for services that appear in the purchase-history result, so +// services with no purchases retained the wrong non-zero value. +func TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments(t *testing.T) { + ctx := context.Background() + + // No purchases at all — represents a new account that has recommendations + // but has not yet acted on them. + mockScheduler := new(MockScheduler) + mockStore := new(MockConfigStore) + + mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return( + []config.RecommendationRecord{ + {Service: "EC2", Savings: 500.0}, + {Service: "RDS", Savings: 300.0}, + }, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) + mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return( + []config.PurchaseHistoryRecord{}, nil) + + mockAuth, req := adminDashboardReq(ctx) + handler := &Handler{ + auth: mockAuth, + scheduler: mockScheduler, + config: mockStore, + } + + result, err := handler.getDashboardSummary(ctx, req, map[string]string{}) + require.NoError(t, err) + + // PotentialSavings must be populated from recommendations. + assert.InDelta(t, 500.0, result.ByService["EC2"].PotentialSavings, 0.001) + assert.InDelta(t, 300.0, result.ByService["RDS"].PotentialSavings, 0.001) + + // CurrentSavings must be 0 — no active commitments exist yet. + // Before the fix, this incorrectly equalled PotentialSavings because + // summarizeRecommendationsWithCoverage also wrote CurrentSavings (issue #1031). + assert.Equal(t, 0.0, result.ByService["EC2"].CurrentSavings, + "no active commitments: current_savings must be 0, not equal to potential_savings") + assert.Equal(t, 0.0, result.ByService["RDS"].CurrentSavings, + "no active commitments: current_savings must be 0, not equal to potential_savings") +} + func TestHandler_calculateCurrentCoverage(t *testing.T) { handler := &Handler{} From 3fd944cec986a17774dba7d353e1f1c25d7fbcd1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 11 Jul 2026 00:03:17 +0200 Subject: [PATCH 2/4] fix(api/dashboard): YTD month accuracy, store error log, fresh slice, first-service dedup --- internal/api/handler_dashboard.go | 72 ++++++++---- internal/api/handler_dashboard_test.go | 145 +++++++++++++++++++++++++ internal/api/handler_purchases.go | 22 ++-- 3 files changed, 202 insertions(+), 37 deletions(-) diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 2d2b31eaa..1b4b08f39 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -4,6 +4,7 @@ package api import ( "context" "fmt" + "sort" "strings" "time" @@ -174,9 +175,8 @@ func (h *Handler) filterDashboardRecommendations(ctx context.Context, session *S } nameByID := h.resolveAccountNamesByID(ctx) - filtered := recs[:0] - for _rvc := range recs { - rec := recs[_rvc] + filtered := make([]config.RecommendationRecord, 0, len(recs)) + for _, rec := range recs { if rec.CloudAccountID == nil { continue } @@ -452,26 +452,37 @@ func (h *Handler) getUpcomingPurchases(ctx context.Context, req *events.LambdaFu return &UpcomingPurchaseResponse{Purchases: upcoming}, nil } +// firstServiceConfig returns the ServiceConfig for the lexicographically first +// key in plan.Services, giving a deterministic result regardless of map +// iteration order. Returns the zero value when the map is empty (both callers +// tolerate empty provider/service strings — they fall back to blank display +// fields rather than panicking). +func firstServiceConfig(plan *config.PurchasePlan) config.ServiceConfig { + if len(plan.Services) == 0 { + return config.ServiceConfig{} + } + keys := make([]string, 0, len(plan.Services)) + for k := range plan.Services { + keys = append(keys, k) + } + sort.Strings(keys) + return plan.Services[keys[0]] +} + // upcomingFromExecution projects a (plan, execution) pair onto the // UpcomingPurchase response. Uses the first service entry from the plan as // representative — the response shape doesn't support multi-service plans. // Step number comes from the execution row directly (already stamped by the // scheduler at instance-create time). func upcomingFromExecution(plan *config.PurchasePlan, exec *config.PurchaseExecution) UpcomingPurchase { - var provider, service string - for _rvc := range plan.Services { - svcCfg := plan.Services[_rvc] - provider = svcCfg.Provider - service = svcCfg.Service - break - } + svc := firstServiceConfig(plan) return UpcomingPurchase{ ExecutionID: exec.ExecutionID, PlanID: plan.ID, PlanName: plan.Name, ScheduledDate: exec.ScheduledDate.Format("2006-01-02"), - Provider: provider, - Service: service, + Provider: svc.Provider, + Service: svc.Service, StepNumber: exec.StepNumber, TotalSteps: plan.RampSchedule.TotalSteps, EstimatedSavings: exec.EstimatedSavings, @@ -609,7 +620,7 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, purchases, err := h.config.GetActivePurchaseHistory(ctx, asOf, accountUUIDs, accountExternalIDsByProvider) if err != nil { - // Log error but don't fail the dashboard request. + logging.Errorf("dashboard: failed to fetch commitment purchases; KPIs will be zeroed: %v", err) return nil, false } return purchases, true @@ -658,21 +669,38 @@ func (h *Handler) calculateCommitmentMetrics(ctx context.Context, accountUUIDs [ // above (same gate), so it is intentionally NOT summed here to avoid // double-counting. EstimatedSavings is in monthly units (see doc above). // - // YTD savings: count from year start or from purchase date, whichever - // is later, using a 30-day month approximation (same as original). - if p.Timestamp.Before(yearStart) { - monthsSinceYearStart := int(currentTime.Sub(yearStart).Hours() / (24 * 30)) - ytdSavings += p.EstimatedSavings * float64(monthsSinceYearStart) - } else { - monthsSincePurchase := int(currentTime.Sub(p.Timestamp).Hours() / (24 * 30)) - ytdSavings += p.EstimatedSavings * float64(monthsSincePurchase) + // YTD savings: count whole calendar months from year start or from the + // purchase date, whichever is later. Use AddDate-stepping rather than + // dividing by a 30-day constant so January (31 days) and February (28/29 + // days) are handled correctly and the result never truncates a partial + // month that spans a 29-30-31-day boundary. + countFrom := yearStart + if p.Timestamp.After(yearStart) { + countFrom = p.Timestamp } + ytdSavings += p.EstimatedSavings * float64(elapsedWholeMonths(countFrom, currentTime)) } return activeCommitments, committedMonthly, ytdSavings, savingsByService } -// calculateCurrentCoverage calculates the current coverage percentage. +// elapsedWholeMonths returns the number of complete calendar months between +// from and to, counted by AddDate-stepping rather than dividing by a fixed +// 30-day constant. This avoids truncation errors for months of varying length +// (e.g. a 28-day gap in February scores 0 under the 30-day divisor, 1 here). +// Returns 0 when to is before or equal to from. +func elapsedWholeMonths(from, to time.Time) int { + if !to.After(from) { + return 0 + } + months := 0 + for from.AddDate(0, months+1, 0).Before(to) || from.AddDate(0, months+1, 0).Equal(to) { + months++ + } + return months +} + +// calculateCurrentCoverage calculates the current coverage percentage func (h *Handler) calculateCurrentCoverage(potentialSavings, committedMonthly float64) float64 { if potentialSavings == 0 { return 100.0 // No recommendations means 100% coverage diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index a1228d4b6..74e14e955 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -1439,3 +1439,148 @@ func TestHandler_getUpcomingPurchases_Errors(t *testing.T) { assert.Len(t, result.Purchases, 0) }) } + +// TestElapsedWholeMonths verifies that elapsedWholeMonths counts calendar months +// correctly using AddDate-stepping rather than a 30-day divisor. +// Regression for 01-M2: with integer-truncated 30-day arithmetic a 28-day +// gap in February would score 0 months, and a year-start-to-now gap spanning +// months of varying length could be off by 1. +func TestElapsedWholeMonths(t *testing.T) { + t.Run("same instant returns 0", func(t *testing.T) { + now := time.Date(2026, 3, 15, 12, 0, 0, 0, time.UTC) + assert.Equal(t, 0, elapsedWholeMonths(now, now)) + }) + + t.Run("to before from returns 0", func(t *testing.T) { + from := time.Date(2026, 3, 15, 0, 0, 0, 0, time.UTC) + to := time.Date(2026, 2, 1, 0, 0, 0, 0, time.UTC) + assert.Equal(t, 0, elapsedWholeMonths(from, to)) + }) + + t.Run("exactly 1 month returns 1", func(t *testing.T) { + from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + to := time.Date(2026, 2, 1, 0, 0, 0, 0, time.UTC) + assert.Equal(t, 1, elapsedWholeMonths(from, to)) + }) + + t.Run("28-day February gap counts as 1 month, not 0", func(t *testing.T) { + // 30-day-divisor arithmetic: int(28*24 / (24*30)) = 0. AddDate-stepping: 1. + from := time.Date(2026, 2, 1, 0, 0, 0, 0, time.UTC) + to := time.Date(2026, 3, 1, 0, 0, 0, 0, time.UTC) // Feb has 28 days in 2026 + assert.Equal(t, 1, elapsedWholeMonths(from, to)) + }) + + t.Run("partial month not yet complete returns floor", func(t *testing.T) { + from := time.Date(2026, 1, 15, 0, 0, 0, 0, time.UTC) + to := time.Date(2026, 2, 14, 0, 0, 0, 0, time.UTC) // 30 days but < 1 full calendar month + assert.Equal(t, 0, elapsedWholeMonths(from, to)) + }) + + t.Run("6 complete months returns 6", func(t *testing.T) { + from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + to := time.Date(2026, 7, 1, 0, 0, 0, 0, time.UTC) + assert.Equal(t, 6, elapsedWholeMonths(from, to)) + }) + + t.Run("cross-year gap counts correctly", func(t *testing.T) { + from := time.Date(2025, 10, 1, 0, 0, 0, 0, time.UTC) + to := time.Date(2026, 3, 1, 0, 0, 0, 0, time.UTC) + assert.Equal(t, 5, elapsedWholeMonths(from, to)) + }) +} + +// TestFilterDashboardRecommendations_FreshSlice verifies that filtering writes +// into a fresh backing array and does not corrupt the caller's input slice. +// Regression for 01-L2: the previous recs[:0] form reused the input's backing +// array, so any element written during filtering overwrote the original. +func TestFilterDashboardRecommendations_FreshSlice(t *testing.T) { + ctx := context.Background() + + // Seed: two accounts A (allowed) and B (blocked). After filtering, only + // A's rec survives. With recs[:0] the filtering loop wrote A's rec into + // position [0] of the shared backing array, which also holds the original + // input[0]; but if B's rec came first (input[0]=B, input[1]=A), the write + // would corrupt input[0]. We reproduce this arrangement explicitly. + accountAID := "aaaa-1111" + accountBID := "bbbb-2222" + // B comes first in the input so that recs[:0] corruption is detectable. + input := []config.RecommendationRecord{ + {CloudAccountID: &accountBID, Service: "rds", Savings: 200.0}, + {CloudAccountID: &accountAID, Service: "ec2", Savings: 100.0}, + } + // Keep a snapshot of input[0] before filtering. + originalFirst := input[0] + + mockStore := new(MockConfigStore) + // resolveAccountNamesByID calls ListCloudAccounts via the Fn override path. + listCalled := false + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + listCalled = true + return []config.CloudAccount{ + {ID: accountAID, Name: "Account A"}, + {ID: accountBID, Name: "Account B"}, + }, nil + } + + mockAuth := new(MockAuthService) + // Session restricted to account A only. + session := &Session{UserID: "user-1"} + mockAuth.On("GetAllowedAccountsAPI", ctx, "user-1").Return([]string{accountAID}, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + handler := &Handler{auth: mockAuth, config: mockStore} + _ = listCalled // asserted implicitly: if not called the name map is empty and MatchesAccount uses only ID + + filtered, err := handler.filterDashboardRecommendations(ctx, session, input) + require.NoError(t, err) + + // Only account A's rec must pass through. + require.Len(t, filtered, 1) + assert.Equal(t, "ec2", filtered[0].Service) + + // input[0] must be unchanged — recs[:0] would have overwritten it with + // the first accepted element (ec2) since filter appends in order. + assert.Equal(t, originalFirst, input[0], + "recs[:0] reuse corrupts the backing array; fresh slice must not mutate input") +} + +// TestFirstServiceConfig verifies that firstServiceConfig returns the service +// from the lexicographically first key, giving a deterministic result regardless +// of map iteration order. +// Regression for 01-N1: both upcomingFromExecution and buildPlannedPurchase +// used an unordered map range + break, which could return a different service on +// each call when the plan has more than one entry. +func TestFirstServiceConfig(t *testing.T) { + t.Run("empty services returns zero value", func(t *testing.T) { + plan := &config.PurchasePlan{Services: map[string]config.ServiceConfig{}} + got := firstServiceConfig(plan) + assert.Equal(t, config.ServiceConfig{}, got) + }) + + t.Run("single service returned", func(t *testing.T) { + plan := &config.PurchasePlan{ + Services: map[string]config.ServiceConfig{ + "ec2": {Provider: "aws", Service: "ec2", Term: 1, Payment: "no_upfront"}, + }, + } + got := firstServiceConfig(plan) + assert.Equal(t, "aws", got.Provider) + assert.Equal(t, "ec2", got.Service) + }) + + t.Run("multiple services returns lexicographically first key deterministically", func(t *testing.T) { + plan := &config.PurchasePlan{ + Services: map[string]config.ServiceConfig{ + "rds": {Provider: "aws", Service: "rds", Term: 3}, + "ec2": {Provider: "aws", Service: "ec2", Term: 1}, + "elasticache": {Provider: "aws", Service: "elasticache", Term: 1}, + }, + } + // "ec2" < "elasticache" < "rds" lexicographically. + for i := 0; i < 20; i++ { + got := firstServiceConfig(plan) + assert.Equal(t, "ec2", got.Service, + "must always return ec2 (lexicographically first key), iteration %d", i) + } + }) +} diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 79b4b9ef1..416b5c2d3 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -171,30 +171,22 @@ func (h *Handler) isPlanAllowedCached(ctx context.Context, session *Session, pla } // buildPlannedPurchase converts a (plan, execution) pair into the API-facing PlannedPurchase. -// Provider/service/term/payment are taken from the first service entry, matching prior behavior. +// Provider/service/term/payment are taken from firstServiceConfig (lexicographic key order) +// so the result is deterministic regardless of map iteration order. func buildPlannedPurchase(plan *config.PurchasePlan, exec *config.PurchaseExecution) PlannedPurchase { - var provider, service, payment string - var term int - for _rvc := range plan.Services { - svcCfg := plan.Services[_rvc] - provider = svcCfg.Provider - service = svcCfg.Service - term = svcCfg.Term - payment = svcCfg.Payment - break - } + svc := firstServiceConfig(plan) return PlannedPurchase{ ID: exec.ExecutionID, PlanID: exec.PlanID, PlanName: plan.Name, ScheduledDate: exec.ScheduledDate.Format("2006-01-02"), - Provider: provider, - Service: service, + Provider: svc.Provider, + Service: svc.Service, ResourceType: "Various", Region: "Multiple", Count: len(exec.Recommendations), - Term: term, - Payment: payment, + Term: svc.Term, + Payment: svc.Payment, EstimatedSavings: exec.EstimatedSavings, UpfrontCost: exec.TotalUpfrontCost, Status: exec.Status, From 127584fe818f2603a84d8c09ec3d96b67c7f5c30 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 19:26:11 +0300 Subject: [PATCH 3/4] fix(test): update CurrentSavingsZeroWhenNoCommitments to mock GetActivePurchaseHistory getDashboardSummary now calls GetActivePurchaseHistory (active-only, uncapped) instead of GetAllPurchaseHistory to aggregate commitment KPIs (see fetchCommitmentPurchases). The regression test was written before this change and still mocked the old method, causing a testify panic on unexpected method call. Replace the GetAllPurchaseHistory mock with GetActivePurchaseHistory using three mock.Anything matchers (asOf, accountUUIDs, accountExternalIDsByProvider), matching the pattern already used in the sibling CurrentSavingsPopulated and CurrentSavingsJSON tests. --- internal/api/handler_dashboard_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 74e14e955..b89cbc4e2 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -1288,7 +1288,8 @@ func TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments(t *test {Service: "RDS", Savings: 300.0}, }, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return( + // No account filter: the all-accounts active fetch path runs. + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return( []config.PurchaseHistoryRecord{}, nil) mockAuth, req := adminDashboardReq(ctx) From 36268e0a9831d71f6f5b35e2fb1895967ee15742 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 20:03:02 +0300 Subject: [PATCH 4/4] lint(api): fix rangeValCopy and godot findings in handler_dashboard.go golangci-lint (gocritic, godot) flagged three issues introduced by the dashboard commits: - Line 179: range-value copy of config.RecommendationRecord (312 bytes) per iteration; switch to index-based _rvc pattern consistent with the rest of handler_dashboard.go. - Line 229: godot: comment block before summarizeRecommendationsWithCoverage ended in ")" not "."; reword to end in period. - Line 703: godot: calculateCurrentCoverage doc comment missing trailing period. No behavioural change. --- internal/api/handler_dashboard.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 1b4b08f39..4f76016f7 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -176,13 +176,13 @@ func (h *Handler) filterDashboardRecommendations(ctx context.Context, session *S nameByID := h.resolveAccountNamesByID(ctx) filtered := make([]config.RecommendationRecord, 0, len(recs)) - for _, rec := range recs { - if rec.CloudAccountID == nil { + for _rvc := range recs { + if recs[_rvc].CloudAccountID == nil { continue } - id := *rec.CloudAccountID + id := *recs[_rvc].CloudAccountID if auth.MatchesAccount(allowed, id, nameByID[id]) { - filtered = append(filtered, rec) + filtered = append(filtered, recs[_rvc]) } } return filtered, nil @@ -226,7 +226,7 @@ func (h *Handler) filterDashboardRecommendations(ctx context.Context, session *S // savings from active purchase history, populated separately in // getDashboardSummary via aggregateActiveCommitmentsPerService. A service // with recommendations but no active purchases correctly ships -// current_savings: 0. (Issue #1031) +// current_savings: 0 (issue #1031). func summarizeRecommendationsWithCoverage( //nolint:gocritic // unnamedResult: return names would conflict with body locals recs []config.RecommendationRecord, coverageByKey map[string]float64, @@ -700,7 +700,7 @@ func elapsedWholeMonths(from, to time.Time) int { return months } -// calculateCurrentCoverage calculates the current coverage percentage +// calculateCurrentCoverage calculates the current coverage percentage. func (h *Handler) calculateCurrentCoverage(potentialSavings, committedMonthly float64) float64 { if potentialSavings == 0 { return 100.0 // No recommendations means 100% coverage