diff --git a/frontend/src/__tests__/history-column-filters.test.ts b/frontend/src/__tests__/history-column-filters.test.ts index 4de38746b..98cf584c3 100644 --- a/frontend/src/__tests__/history-column-filters.test.ts +++ b/frontend/src/__tests__/history-column-filters.test.ts @@ -67,6 +67,13 @@ const rows: HistoryPurchase[] = [ ]; describe('applyPurchaseHistoryColumnFilters', () => { + test('unknown upfront cost does not match a known-zero filter', () => { + const out = applyPurchaseHistoryColumnFilters([ + mkRow({ purchase_id: 'unknown', upfront_cost: null }), + mkRow({ purchase_id: 'zero', upfront_cost: 0 }), + ], { upfront_cost: { kind: 'expr', expr: '0' } }); + expect(out.map(row => row.purchase_id)).toEqual(['zero']); + }); test('numeric expr: savings > 100 narrows to high-saving rows', () => { const filters: PurchaseHistoryColumnFilters = { savings: { kind: 'expr', expr: '>100' }, diff --git a/frontend/src/__tests__/history-marketplace-sell-button.test.ts b/frontend/src/__tests__/history-marketplace-sell-button.test.ts index 80419d47c..a946a7504 100644 --- a/frontend/src/__tests__/history-marketplace-sell-button.test.ts +++ b/frontend/src/__tests__/history-marketplace-sell-button.test.ts @@ -152,6 +152,16 @@ function sellIds(): (string | undefined)[] { } describe('History inline Sell on Marketplace button (issue #292)', () => { + test('unknown upfront disables listing without showing a fabricated price', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [makeRow({ purchase_id: 'ri-unknown-cost', upfront_cost: null })] }); + await loadHistory(); + const button = document.querySelector('.history-marketplace-sell-btn'); + expect(button).not.toBeNull(); + button?.click(); + expect(button?.disabled).toBe(true); + expect(api.createMarketplaceListing).not.toHaveBeenCalled(); + expect(confirmDialog).not.toHaveBeenCalled(); + }); beforeEach(() => { setupDOM(); jest.clearAllMocks(); diff --git a/frontend/src/__tests__/history.test.ts b/frontend/src/__tests__/history.test.ts index 5efb91601..f2b8ebf97 100644 --- a/frontend/src/__tests__/history.test.ts +++ b/frontend/src/__tests__/history.test.ts @@ -63,6 +63,7 @@ jest.mock('../state', () => ({ })); import * as api from '../api'; +import { getAmortizeUpfront } from '../state'; import { switchTab } from '../navigation'; import { canAccess } from '../permissions'; @@ -82,6 +83,26 @@ describe('History Module', () => { jest.clearAllMocks(); mockGetCurrentProvider.mockReturnValue('all'); mockGetCurrentAccountIDs.mockReturnValue([]); + (getAmortizeUpfront as jest.Mock).mockReturnValue(false); + }); + + test.each([false, true])('unknown upfront with amortization %s preserves recurring versus effective cost', async (amortize) => { + (getAmortizeUpfront as jest.Mock).mockReturnValue(amortize); + const purchase = { + timestamp: '2026-09-29T00:00:00Z', provider: 'aws', service: 'ec2', resource_type: 'm5.large', + region: 'us-east-1', count: 1, term: 1, upfront_cost: null, monthly_cost: 50, estimated_savings: 10, + }; + (api.getHistory as jest.Mock).mockResolvedValue({ summary: { total_upfront: null }, purchases: [ + { ...purchase, purchase_id: 'complete', status: 'completed' }, + { ...purchase, purchase_id: 'pending', status: 'pending' }, + ] }); + await loadHistory(); + const history = document.querySelector('#history-list tr[data-execution-id="complete"]'); + const queue = document.querySelector('#purchases-approval-queue tr[data-execution-id="pending"]'); + expect(history?.querySelectorAll('td')[8]?.textContent).toBe('--'); + expect(history?.querySelectorAll('td')[9]?.textContent).toBe(amortize ? '-' : '$50'); + expect(queue?.querySelectorAll('td')[7]?.textContent).toBe(amortize ? '-' : '$50'); + expect(queue?.querySelectorAll('td')[8]?.textContent).toBe('--'); }); describe('initHistoryDateRange', () => { diff --git a/frontend/src/__tests__/inventory.test.ts b/frontend/src/__tests__/inventory.test.ts index fa8121edc..010177181 100644 --- a/frontend/src/__tests__/inventory.test.ts +++ b/frontend/src/__tests__/inventory.test.ts @@ -146,6 +146,21 @@ function clearDOM(): void { while (document.body.firstChild) document.body.removeChild(document.body.firstChild); } +test.each([false, true])('inventory unknown upfront with amortization %s', async (amortize) => { + clearDOM(); + buildInventoryDOM(); + (state.getAmortizeUpfront as jest.Mock).mockReturnValue(amortize); + (api.listActiveCommitments as jest.Mock).mockResolvedValue([makeCommitment({ upfront_cost: null, monthly_cost: 50 })]); + try { + await loadActiveCommitments(); + const cell = document.querySelector('#active-commitments-list tbody tr')?.querySelectorAll('td')[8]; + expect(cell?.textContent).toBe(amortize ? '\u2014' : '$50'); + } finally { + (state.getAmortizeUpfront as jest.Mock).mockReturnValue(false); + clearDOM(); + } +}); + describe('Inventory & Coverage sub-section switching', () => { beforeEach(() => { buildInventoryDOM(); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index f59b3736f..315212241 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -273,7 +273,7 @@ export interface PurchaseHistory { count: number; term: number; payment: string; - upfront_cost: number; + upfront_cost: number | null; monthly_cost: number | null; estimated_savings: number; plan_id?: string; @@ -663,7 +663,7 @@ export interface SavingsAnalyticsSummary { export interface SavingsDataPoint { timestamp: string; total_savings: number; - total_upfront: number; + total_upfront: number | null; purchase_count: number; cumulative_savings: number; by_service?: Record; @@ -679,7 +679,7 @@ export interface SavingsBreakdownResponse { export interface SavingsBreakdownValue { total_savings: number; - total_upfront: number; + total_upfront: number | null; purchase_count: number; percentage: number; } @@ -876,7 +876,7 @@ export interface InventoryCommitment { payment_option?: string; start_date: string; end_date: string; - upfront_cost: number; + upfront_cost: number | null; /** null when the provider API did not return a monthly recurring breakdown; "$X.XX" when populated, "—" when null. */ monthly_cost: number | null; estimated_savings: number; @@ -897,7 +897,7 @@ export interface InventoryCommitment { */ export interface CoverageServiceRow { service: string; - covered_monthly: number; + covered_monthly: number | null; on_demand_monthly: number; coverage_pct: number | null; } diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 0f5554c17..8ba3cd390 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -960,7 +960,7 @@ function purchaseHistoryNumericCellValue( ): number { switch (col) { case 'count': return p.count ?? 0; - case 'upfront_cost': return p.upfront_cost ?? 0; + case 'upfront_cost': return p.upfront_cost ?? Number.NaN; case 'savings': return p.estimated_savings ?? 0; case 'provider': case 'service': @@ -1149,7 +1149,7 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { const amortize = state.getAmortizeUpfront(); const rawMonthly = p.monthly_cost != null ? p.monthly_cost : null; const displayMonthly = (rawMonthly != null && amortize) - ? amortizedMonthly(rawMonthly, p.upfront_cost, p.term) + ? (p.upfront_cost == null ? null : amortizedMonthly(rawMonthly, p.upfront_cost, p.term)) : rawMonthly; const monthlyCostCell = displayMonthly != null ? formatCurrency(displayMonthly) @@ -1471,6 +1471,12 @@ function wireRowActionHandlers(container: HTMLElement): void { // Look up the purchase record so we can show a meaningful price summary. const purchase = lastPurchases.find(p => p.purchase_id === id); + if (!purchase || purchase.upfront_cost == null) { + btn.disabled = true; + showToast({ message: 'Upfront cost is unknown; marketplace pricing is unavailable.', kind: 'error' }); + return; + } + // Build a pricing modal body with RI summary and fee breakdown. const bodyEl = document.createElement('div'); bodyEl.className = 'marketplace-pricing-modal-body'; @@ -1491,7 +1497,7 @@ function wireRowActionHandlers(container: HTMLElement): void { // EXACTLY: floor (not round), floored at 1 (not 0), so the number // shown here is the same number the backend actually lists at. const remainingMonths = Math.max(1, Math.floor(termMonths - elapsedMonths)); - const upfront = purchase.upfront_cost ?? 0; + const upfront = purchase.upfront_cost; const count = purchase.count > 0 ? purchase.count : 1; // Mirror marketplaceResidualPerUnit + resolveMarketplacePriceSchedule's // default branch in internal/api/handler_marketplace.go EXACTLY, so @@ -1689,7 +1695,7 @@ function approvalQueueNumericCellValue( // Return NaN for null monthly_cost so numeric predicates (e.g. "= 0") // don't match rows where the provider didn't report a monthly cost. case 'monthly_cost': return p.monthly_cost == null ? Number.NaN : p.monthly_cost; - case 'upfront_cost': return p.upfront_cost ?? 0; + case 'upfront_cost': return p.upfront_cost ?? Number.NaN; case 'savings': return p.estimated_savings ?? 0; case 'provider': case 'account': @@ -1814,7 +1820,7 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { const amortize = state.getAmortizeUpfront(); const rawMonthly = p.monthly_cost != null ? p.monthly_cost : null; const displayMonthly = (rawMonthly != null && amortize) - ? amortizedMonthly(rawMonthly, p.upfront_cost, p.term) + ? (p.upfront_cost == null ? null : amortizedMonthly(rawMonthly, p.upfront_cost, p.term)) : rawMonthly; const monthlyCostCell = displayMonthly != null ? formatCurrency(displayMonthly) diff --git a/frontend/src/inventory.ts b/frontend/src/inventory.ts index a63ddf4de..81d80de63 100644 --- a/frontend/src/inventory.ts +++ b/frontend/src/inventory.ts @@ -271,7 +271,7 @@ function buildCommitmentRow(c: api.InventoryCommitment): HTMLTableRowElement { const amortize = state.getAmortizeUpfront(); let displayMonthly: number | null = c.monthly_cost; if (displayMonthly != null && amortize) { - displayMonthly = amortizedMonthly(displayMonthly, c.upfront_cost, c.term_years); + displayMonthly = c.upfront_cost == null ? null : amortizedMonthly(displayMonthly, c.upfront_cost, c.term_years); } appendCell(tr, displayMonthly != null ? formatCurrency(displayMonthly) : '—'); diff --git a/frontend/src/types.ts b/frontend/src/types.ts index aeb9823fd..89663abb3 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -201,7 +201,7 @@ export interface HistorySummary { total_pending?: number; total_failed?: number; total_expired?: number; - total_upfront?: number; + total_upfront?: number | null; total_monthly_savings?: number; total_annual_savings?: number; } @@ -216,7 +216,7 @@ export interface HistoryPurchase { count: number; term: number; payment?: string; - upfront_cost: number; + upfront_cost: number | null; monthly_cost?: number | null; estimated_savings: number; account_id?: string; @@ -321,7 +321,7 @@ export interface SavingsAnalyticsSummary { export interface SavingsDataPoint { timestamp: string; total_savings: number; - total_upfront: number; + total_upfront: number | null; purchase_count: number; cumulative_savings: number; by_service?: Record; @@ -337,7 +337,7 @@ export interface SavingsBreakdownResponse { export interface SavingsBreakdownValue { total_savings: number; - total_upfront: number; + total_upfront: number | null; purchase_count: number; percentage: number; } diff --git a/frontend/tests-e2e/nullable-purchase-cost.spec.ts b/frontend/tests-e2e/nullable-purchase-cost.spec.ts new file mode 100644 index 000000000..50a4814fa --- /dev/null +++ b/frontend/tests-e2e/nullable-purchase-cost.spec.ts @@ -0,0 +1,36 @@ +import { test, expect } from '@playwright/test'; +import { mockApi, seedAuth } from './fixtures/recs'; + +test('unknown upfront remains unknown through history and inventory amortization toggles', async ({ page }) => { + const errors: string[] = []; + page.on('pageerror', error => errors.push(error.message)); + await seedAuth(page); + await mockApi(page); + const purchase = { + purchase_id: '11111111-1111-1111-1111-111111111111', timestamp: new Date().toISOString(), + provider: 'aws', service: 'ec2', resource_type: 'm5.large', region: 'us-east-1', + count: 1, term: 1, upfront_cost: null, monthly_cost: 50, estimated_savings: 10, + offering_class: 'standard', status: 'completed', account_id: 'account-1', + }; + await page.route('**/api/history**', route => route.fulfill({ json: { summary: { total_upfront: null }, purchases: [purchase] } })); + await page.route('**/api/inventory/commitments**', route => route.fulfill({ json: { commitments: [{ + ...purchase, id: 'account-1:purchase', term_years: 1, start_date: purchase.timestamp, + end_date: '2027-09-29T00:00:00Z', status: 'active', + }] } })); + + await page.goto('/purchases/history'); + const history = page.locator('#history-list tbody tr').first(); + await expect(history.locator('td').nth(8)).toHaveText('--'); + await expect(history.locator('td').nth(9)).toHaveText('$50'); + await page.locator('#history-amortize-checkbox').check(); + await expect(history.locator('td').nth(9)).toHaveText('-'); + await page.locator('#history-amortize-checkbox').uncheck(); + await expect(history.locator('td').nth(9)).toHaveText('$50'); + + await page.goto('/inventory/active-commitments'); + const inventory = page.locator('#active-commitments-list tbody tr').first(); + await expect(inventory.locator('td').nth(8)).toHaveText('$50'); + await page.locator('#inventory-amortize-checkbox').check(); + await expect(inventory.locator('td').nth(8)).toHaveText('\u2014'); + expect(errors).toEqual([]); +}); diff --git a/go.mod b/go.mod index e271e72ae..d778fc2c6 100644 --- a/go.mod +++ b/go.mod @@ -84,10 +84,10 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.4.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.4.0 - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928205306-c5cf5c7fbad8 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928205306-c5cf5c7fbad8 - github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928205306-c5cf5c7fbad8 - github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928205306-c5cf5c7fbad8 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928214714-ce9513612901 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928214714-ce9513612901 + github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928214714-ce9513612901 + github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901 github.com/aws/aws-lambda-go v1.47.0 github.com/aws/aws-sdk-go-v2/service/kms v1.50.4 github.com/aws/aws-sdk-go-v2/service/lambda v1.89.0 diff --git a/go.sum b/go.sum index 2d4fdd810..f5cb30944 100644 --- a/go.sum +++ b/go.sum @@ -92,14 +92,14 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.54.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 h1:s0WlVbf9qpvkh1c/uDAPElam0WrL7fHRIidgZJ7UqZI= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928205306-c5cf5c7fbad8 h1:/4MDV2fttTEnjt37Iq49v5SGMvYuBH68q99r9E+juOE= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928205306-c5cf5c7fbad8/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928205306-c5cf5c7fbad8 h1:qUmZE0uvsWKU6Gtglo3h61jVEvqoihFjig6e1r/IvDk= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928205306-c5cf5c7fbad8/go.mod h1:tIUCLVX6utdBPiP6dAjY7GUfwIQngeSMK5A9aRqe6gA= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928205306-c5cf5c7fbad8 h1:eyAXwyDwIvEI9vdHb2TqkNPRuhbMYWDXN4751LOltKA= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928205306-c5cf5c7fbad8/go.mod h1:zgCL/ozOkcZUDbEC7a2UwW+6MPcLoBlEU2WUZ0TORUs= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928205306-c5cf5c7fbad8 h1:qs2P8PsqKcFl6eGMGvi7EfMXDuni3pqkc8vDRESppKA= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928205306-c5cf5c7fbad8/go.mod h1:UnMqDe2mBUHHF6gnKAgJn9/JeXa7WZqQxfWPs4unGrg= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928214714-ce9513612901 h1:JWQwkwshjBoEuoIuFK1KHh70yZDYQTrm5GRloD6ojjU= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928214714-ce9513612901/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928214714-ce9513612901 h1:18MxQd7+ZQ+Z2uNbZfpIJQ00JWdSdTVSmVWBzg4R0DA= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928214714-ce9513612901/go.mod h1:tIUCLVX6utdBPiP6dAjY7GUfwIQngeSMK5A9aRqe6gA= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928214714-ce9513612901 h1:iSdHYdmGUjcjtSmgGZxptBzDuLrJ9KR2mh1/x7FNvSY= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928214714-ce9513612901/go.mod h1:zgCL/ozOkcZUDbEC7a2UwW+6MPcLoBlEU2WUZ0TORUs= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901 h1:i6OwXLUheudN3GfwnYXdKuEq8vPdE9qkNJ+r71LRLKA= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901/go.mod h1:UnMqDe2mBUHHF6gnKAgJn9/JeXa7WZqQxfWPs4unGrg= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= github.com/aws/aws-lambda-go v1.47.0 h1:0H8s0vumYx/YKs4sE7YM0ktwL2eWse+kfopsRI1sXVI= diff --git a/internal/analytics/collector.go b/internal/analytics/collector.go index 2790671c7..859ec423e 100644 --- a/internal/analytics/collector.go +++ b/internal/analytics/collector.go @@ -51,13 +51,14 @@ func NewCollector(cfg CollectorConfig, configStore config.StoreInterface) (*Coll // aggregateData holds aggregated savings data for one // (cloud_account_id|account_id|service|provider|region|commitment_type) bucket. type aggregateData struct { - accountID string - cloudAccountID *string - service string - provider string - region string - commitmentType string - commitment float64 + accountID string + cloudAccountID *string + service string + provider string + region string + commitmentType string + commitment float64 + commitmentUnknown bool // usage accumulates the recurring (monthly) cost of the commitments in this // bucket as the covered-usage proxy. usageKnown stays false until at least // one contributing row carried a non-nil MonthlyCost, so a bucket made up @@ -179,7 +180,11 @@ func aggregatePurchases(ctx context.Context, purchases []config.PurchaseHistoryR agg.savings += p.EstimatedSavings // Upfront commitment amortized to a monthly run-rate over the term. // Term > 0 guaranteed above. - agg.commitment += p.UpfrontCost / (float64(p.Term) * MonthsPerYear) + if p.UpfrontCost == nil { + agg.commitmentUnknown = true + } else { + agg.commitment += *p.UpfrontCost / (float64(p.Term) * MonthsPerYear) + } // H2: real covered usage from the recurring monthly cost when present. // Nil MonthlyCost (e.g. AWS all-upfront) contributes nothing and leaves // usage unknown rather than implicitly $0. @@ -209,6 +214,10 @@ func commitmentTypeFor(service string) string { func buildSnapshots(serviceMap map[string]*aggregateData, now time.Time) []SavingsSnapshot { snapshots := make([]SavingsSnapshot, 0, len(serviceMap)) for _, agg := range serviceMap { + var commitment *float64 + if !agg.commitmentUnknown { + commitment = &agg.commitment + } var usage *float64 if agg.usageKnown { u := agg.usage @@ -223,7 +232,7 @@ func buildSnapshots(serviceMap map[string]*aggregateData, now time.Time) []Savin Service: agg.service, Region: agg.region, CommitmentType: agg.commitmentType, - TotalCommitment: agg.commitment, + TotalCommitment: commitment, TotalUsage: usage, TotalSavings: agg.savings, CoveragePercentage: nil, diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index d74adbfc4..1493d9245 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -498,7 +498,7 @@ func activeRecord(provider, service, region string, term int, savings, upfront f Region: region, Term: term, EstimatedSavings: savings, - UpfrontCost: upfront, + UpfrontCost: new(float64(upfront)), } } @@ -563,7 +563,7 @@ func TestCollectorCollect(t *testing.T) { expired := config.PurchaseHistoryRecord{ AccountID: "123456789012", Timestamp: time.Now().AddDate(-3, 0, 0), Provider: "aws", Service: "rds", Region: "us-east-1", - Term: 1, EstimatedSavings: 100, UpfrontCost: 500, + Term: 1, EstimatedSavings: 100, UpfrontCost: new(float64(500)), } cfgStore := &mockConfigStore{ getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { @@ -589,7 +589,8 @@ func TestCollectorCollect(t *testing.T) { assert.Equal(t, "rds", s.Service) assert.Equal(t, "RI", s.CommitmentType) assert.Greater(t, s.TotalSavings, 0.0) - assert.Greater(t, s.TotalCommitment, 0.0) + require.NotNil(t, s.TotalCommitment) + assert.Greater(t, *s.TotalCommitment, 0.0) }) t.Run("aggregates multiple purchases for same bucket", func(t *testing.T) { @@ -661,7 +662,7 @@ func TestCollectorCollect(t *testing.T) { require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) require.Len(t, store.savedSnapshots, 1) assert.InDelta(t, monthlySavings, store.savedSnapshots[0].TotalSavings, 0.001) - assert.InDelta(t, upfront/(1*MonthsPerYear), store.savedSnapshots[0].TotalCommitment, 0.001) + assert.InDelta(t, upfront/(1*MonthsPerYear), *store.savedSnapshots[0].TotalCommitment, 0.001) }) // ── Regression tests for the latent data bugs (#1023) ── @@ -680,9 +681,10 @@ func TestCollectorCollect(t *testing.T) { s := store.savedSnapshots[0] // Only the good row contributed: no +Inf/NaN, exactly one active purchase. assert.Equal(t, 1, s.Metadata["active_purchases"]) - assert.False(t, math.IsInf(s.TotalCommitment, 0), "commitment must not be Inf") - assert.False(t, math.IsNaN(s.TotalCommitment), "commitment must not be NaN") - assert.InDelta(t, 500.0/(1*MonthsPerYear), s.TotalCommitment, 0.001) + require.NotNil(t, s.TotalCommitment) + assert.False(t, math.IsInf(*s.TotalCommitment, 0), "commitment must not be Inf") + assert.False(t, math.IsNaN(*s.TotalCommitment), "commitment must not be NaN") + assert.InDelta(t, 500.0/(1*MonthsPerYear), *s.TotalCommitment, 0.001) }) t.Run("H1: a negative Term is also skipped", func(t *testing.T) { diff --git a/internal/analytics/interfaces.go b/internal/analytics/interfaces.go index 48d82fbff..6327042e9 100644 --- a/internal/analytics/interfaces.go +++ b/internal/analytics/interfaces.go @@ -25,7 +25,7 @@ type SavingsSnapshot struct { Region string `json:"region"` CommitmentType string `json:"commitment_type"` ID string `json:"id"` - TotalCommitment float64 `json:"total_commitment"` + TotalCommitment *float64 `json:"total_commitment"` TotalSavings float64 `json:"total_savings"` } diff --git a/internal/analytics/nullable_purchase_cost_test.go b/internal/analytics/nullable_purchase_cost_test.go new file mode 100644 index 000000000..2f3b2318e --- /dev/null +++ b/internal/analytics/nullable_purchase_cost_test.go @@ -0,0 +1,50 @@ +package analytics + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCollectorUnknownUpfront(t *testing.T) { + for _, costs := range [][]*float64{{nil}, {nil, new(float64(120))}, {new(float64(120)), nil}, {new(float64)}} { + purchases := make([]config.PurchaseHistoryRecord, len(costs), len(costs)+1) + unknown := false + for i, cost := range costs { + purchases[i] = activeRecord("aws", "ec2", "us-east-1", 1, 10, 0) + purchases[i].UpfrontCost = cost + unknown = unknown || cost == nil + } + other := activeRecord("aws", "ec2", "us-east-1", 1, 20, 240) + other.AccountID = "other-account" + purchases = append(purchases, other) + groups, _, _, err := aggregatePurchases(context.Background(), purchases, time.Now()) + require.NoError(t, err) + snapshots := buildSnapshots(groups, time.Now()) + require.Len(t, snapshots, 2) + for _, snapshot := range snapshots { + if snapshot.AccountID == other.AccountID { + require.NotNil(t, snapshot.TotalCommitment) + assert.Equal(t, 20.0, *snapshot.TotalCommitment) + continue + } + assert.Equal(t, 10*float64(len(costs)), snapshot.TotalSavings) + assert.Equal(t, len(costs), snapshot.Metadata["active_purchases"]) + body, err := json.Marshal(snapshot) + require.NoError(t, err) + if unknown { + assert.Nil(t, snapshot.TotalCommitment) + assert.Contains(t, string(body), `"total_commitment":null`) + } else { + require.NotNil(t, snapshot.TotalCommitment) + assert.Zero(t, *snapshot.TotalCommitment) + assert.Contains(t, string(body), `"total_commitment":0`) + } + } + } +} diff --git a/internal/analytics/postgres_analytics.go b/internal/analytics/postgres_analytics.go index 64bceae4e..20088ca81 100644 --- a/internal/analytics/postgres_analytics.go +++ b/internal/analytics/postgres_analytics.go @@ -2,6 +2,7 @@ package analytics import ( "context" + "database/sql" "encoding/json" "fmt" "sort" @@ -255,6 +256,7 @@ func (s *PostgresAnalyticsStore) QuerySavings(ctx context.Context, req QueryRequ snapshots := make([]SavingsSnapshot, 0) for rows.Next() { var snapshot SavingsSnapshot + var commitment sql.NullFloat64 var metadataJSON []byte err := rows.Scan( @@ -266,7 +268,7 @@ func (s *PostgresAnalyticsStore) QuerySavings(ctx context.Context, req QueryRequ &snapshot.Service, &snapshot.Region, &snapshot.CommitmentType, - &snapshot.TotalCommitment, + &commitment, &snapshot.TotalUsage, &snapshot.TotalSavings, &snapshot.CoveragePercentage, @@ -275,6 +277,9 @@ func (s *PostgresAnalyticsStore) QuerySavings(ctx context.Context, req QueryRequ if err != nil { return nil, fmt.Errorf("failed to scan snapshot: %w", err) } + if commitment.Valid { + snapshot.TotalCommitment = &commitment.Float64 + } if len(metadataJSON) > 0 { if err := json.Unmarshal(metadataJSON, &snapshot.Metadata); err != nil { diff --git a/internal/analytics/postgres_analytics_db_test.go b/internal/analytics/postgres_analytics_db_test.go index 3d79db0a8..5e364ec6d 100644 --- a/internal/analytics/postgres_analytics_db_test.go +++ b/internal/analytics/postgres_analytics_db_test.go @@ -77,7 +77,7 @@ func TestPostgresAnalyticsStore_SaveSnapshot_DB(t *testing.T) { Service: "rds", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 1000.00, + TotalCommitment: new(float64(1000.00)), TotalUsage: f64ptr(800.00), TotalSavings: 200.00, CoveragePercentage: f64ptr(80.00), @@ -101,7 +101,7 @@ func TestPostgresAnalyticsStore_SaveSnapshot_DB(t *testing.T) { Service: "cloudsql", Region: "us-central1", CommitmentType: "RI", - TotalCommitment: 500.00, + TotalCommitment: new(float64(500.00)), TotalUsage: f64ptr(400.00), TotalSavings: 100.00, CoveragePercentage: f64ptr(80.00), @@ -121,7 +121,7 @@ func TestPostgresAnalyticsStore_SaveSnapshot_DB(t *testing.T) { Service: "SavingsPlans", Region: "us-east-1", CommitmentType: "SavingsPlan", - TotalCommitment: 2000.00, + TotalCommitment: new(float64(2000.00)), TotalUsage: f64ptr(1500.00), TotalSavings: 500.00, CoveragePercentage: f64ptr(75.00), @@ -159,7 +159,7 @@ func TestPostgresAnalyticsStore_QuerySavings_DB(t *testing.T) { Service: "rds", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 1000.00, + TotalCommitment: new(float64(1000.00)), TotalUsage: f64ptr(800.00), TotalSavings: 200.00, CoveragePercentage: f64ptr(80.00), @@ -171,7 +171,7 @@ func TestPostgresAnalyticsStore_QuerySavings_DB(t *testing.T) { Service: "elasticache", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 500.00, + TotalCommitment: new(float64(500.00)), TotalUsage: f64ptr(400.00), TotalSavings: 100.00, CoveragePercentage: f64ptr(80.00), @@ -183,7 +183,7 @@ func TestPostgresAnalyticsStore_QuerySavings_DB(t *testing.T) { Service: "cloudsql", Region: "us-central1", CommitmentType: "RI", - TotalCommitment: 750.00, + TotalCommitment: new(float64(750.00)), TotalUsage: f64ptr(600.00), TotalSavings: 150.00, CoveragePercentage: f64ptr(80.00), @@ -533,7 +533,7 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals_DB(t *testing.T) { Service: "rds", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 1000.00, + TotalCommitment: new(float64(1000.00)), TotalUsage: f64ptr(800.00), TotalSavings: 200.00, CoveragePercentage: f64ptr(80.00), diff --git a/internal/analytics/postgres_analytics_integration_test.go b/internal/analytics/postgres_analytics_integration_test.go index 1f0f5a365..885cbc88b 100644 --- a/internal/analytics/postgres_analytics_integration_test.go +++ b/internal/analytics/postgres_analytics_integration_test.go @@ -39,7 +39,7 @@ func TestPostgresAnalyticsStore_SaveSnapshot(t *testing.T) { Service: "rds", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 1000.00, + TotalCommitment: new(float64(1000.00)), TotalUsage: f64ptr(800.00), TotalSavings: 200.00, CoveragePercentage: f64ptr(80.00), @@ -63,7 +63,7 @@ func TestPostgresAnalyticsStore_SaveSnapshot(t *testing.T) { Service: "cloudsql", Region: "us-central1", CommitmentType: "RI", - TotalCommitment: 500.00, + TotalCommitment: new(float64(500.00)), TotalUsage: f64ptr(400.00), TotalSavings: 100.00, CoveragePercentage: f64ptr(80.00), @@ -83,7 +83,7 @@ func TestPostgresAnalyticsStore_SaveSnapshot(t *testing.T) { Service: "SavingsPlans", Region: "us-east-1", CommitmentType: "SavingsPlan", - TotalCommitment: 2000.00, + TotalCommitment: new(float64(2000.00)), TotalUsage: f64ptr(1500.00), TotalSavings: 500.00, CoveragePercentage: f64ptr(75.00), @@ -119,7 +119,7 @@ func TestPostgresAnalyticsStore_QuerySavings(t *testing.T) { Service: "rds", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 1000.00, + TotalCommitment: new(float64(1000.00)), TotalUsage: f64ptr(800.00), TotalSavings: 200.00, CoveragePercentage: f64ptr(80.00), @@ -131,7 +131,7 @@ func TestPostgresAnalyticsStore_QuerySavings(t *testing.T) { Service: "elasticache", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 500.00, + TotalCommitment: new(float64(500.00)), TotalUsage: f64ptr(400.00), TotalSavings: 100.00, CoveragePercentage: f64ptr(80.00), @@ -143,7 +143,7 @@ func TestPostgresAnalyticsStore_QuerySavings(t *testing.T) { Service: "cloudsql", Region: "us-central1", CommitmentType: "RI", - TotalCommitment: 750.00, + TotalCommitment: new(float64(750.00)), TotalUsage: f64ptr(600.00), TotalSavings: 150.00, CoveragePercentage: f64ptr(80.00), @@ -248,7 +248,7 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals(t *testing.T) { Service: "rds", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 1000.00, + TotalCommitment: new(float64(1000.00)), TotalUsage: f64ptr(800.00), TotalSavings: 200.00, CoveragePercentage: f64ptr(80.00), diff --git a/internal/analytics/postgres_analytics_pgxmock_test.go b/internal/analytics/postgres_analytics_pgxmock_test.go index 5babf69c2..d724a8a17 100644 --- a/internal/analytics/postgres_analytics_pgxmock_test.go +++ b/internal/analytics/postgres_analytics_pgxmock_test.go @@ -47,7 +47,7 @@ func TestPostgresAnalyticsStore_SaveSnapshot_Success(t *testing.T) { Service: "ec2", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 1000.0, + TotalCommitment: new(float64(1000.0)), TotalUsage: f64ptr(900.0), TotalSavings: 100.0, CoveragePercentage: f64ptr(90.0), diff --git a/internal/analytics/postgres_analytics_test.go b/internal/analytics/postgres_analytics_test.go index 79ceff8a6..ca511d251 100644 --- a/internal/analytics/postgres_analytics_test.go +++ b/internal/analytics/postgres_analytics_test.go @@ -82,7 +82,7 @@ func TestSaveSnapshot(t *testing.T) { Service: "rds", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 100.0, + TotalCommitment: new(float64(100.0)), TotalUsage: f64ptr(80.0), TotalSavings: 20.0, CoveragePercentage: f64ptr(80.0), @@ -846,7 +846,7 @@ func TestSavingsSnapshot(t *testing.T) { Service: "rds", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 100.50, + TotalCommitment: new(float64(100.50)), TotalUsage: f64ptr(80.25), TotalSavings: 20.25, CoveragePercentage: f64ptr(80.0), @@ -862,7 +862,7 @@ func TestSavingsSnapshot(t *testing.T) { assert.Equal(t, "rds", snapshot.Service) assert.Equal(t, "us-east-1", snapshot.Region) assert.Equal(t, "RI", snapshot.CommitmentType) - assert.InDelta(t, 100.50, snapshot.TotalCommitment, 0.001) + assert.InDelta(t, 100.50, *snapshot.TotalCommitment, 0.001) require.NotNil(t, snapshot.TotalUsage) assert.InDelta(t, 80.25, *snapshot.TotalUsage, 0.001) assert.InDelta(t, 20.25, snapshot.TotalSavings, 0.001) @@ -881,7 +881,7 @@ func TestSavingsSnapshot(t *testing.T) { Service: "rds", Region: "us-east-1", CommitmentType: "RI", - TotalCommitment: 100.50, + TotalCommitment: new(float64(100.50)), TotalUsage: f64ptr(80.25), TotalSavings: 20.25, CoveragePercentage: f64ptr(80.0), diff --git a/internal/api/analytics_postgres.go b/internal/api/analytics_postgres.go index d443aac6b..7b3453da4 100644 --- a/internal/api/analytics_postgres.go +++ b/internal/api/analytics_postgres.go @@ -3,6 +3,7 @@ package api import ( "context" + "database/sql" "fmt" "sort" "strings" @@ -176,7 +177,7 @@ func (c *PostgresAnalyticsClient) QueryHistory( service, provider, SUM(estimated_savings)::float8 AS savings, - SUM(upfront_cost)::float8 AS upfront, + CASE WHEN COUNT(upfront_cost) = COUNT(*) THEN SUM(upfront_cost)::float8 END AS upfront, COUNT(*) AS purchases FROM purchase_history WHERE timestamp >= $1 @@ -195,12 +196,13 @@ func (c *PostgresAnalyticsClient) QueryHistory( // Fold rows into bucket → HistoryDataPoint with per-service/provider maps. bucketIndex := make(map[time.Time]*HistoryDataPoint) var bucketOrder []time.Time - summary := &HistorySummary{} + summary := &HistorySummary{TotalUpfront: new(float64)} for rows.Next() { var bucket time.Time var service, provider string - var savings, upfront float64 + var savings float64 + var upfront sql.NullFloat64 var purchases int if err := rows.Scan(&bucket, &service, &provider, &savings, &upfront, &purchases); err != nil { return nil, nil, fmt.Errorf("scan purchase_history row: %w", err) @@ -209,20 +211,25 @@ func (c *PostgresAnalyticsClient) QueryHistory( dp, ok := bucketIndex[bucket] if !ok { dp = &HistoryDataPoint{ - Timestamp: bucket, - ByService: make(map[string]float64), - ByProvider: make(map[string]float64), + TotalUpfront: new(float64), + Timestamp: bucket, + ByService: make(map[string]float64), + ByProvider: make(map[string]float64), } bucketIndex[bucket] = dp bucketOrder = append(bucketOrder, bucket) } dp.TotalSavings += savings - dp.TotalUpfront += upfront + var cost *float64 + if upfront.Valid { + cost = &upfront.Float64 + } + dp.TotalUpfront = sumKnownCosts(dp.TotalUpfront, cost) + summary.TotalUpfront = sumKnownCosts(summary.TotalUpfront, cost) dp.PurchaseCount += purchases dp.ByService[service] += savings dp.ByProvider[provider] += savings - summary.TotalUpfront += upfront summary.TotalMonthlySavings += savings summary.TotalPurchases += purchases } @@ -270,7 +277,7 @@ func (c *PostgresAnalyticsClient) QueryBreakdown( query := fmt.Sprintf(` SELECT %s AS bucket, SUM(estimated_savings)::float8 AS savings, - SUM(upfront_cost)::float8 AS upfront, + CASE WHEN COUNT(upfront_cost) = COUNT(*) THEN SUM(upfront_cost)::float8 END AS upfront, COUNT(*) AS purchases FROM purchase_history WHERE timestamp >= $1 @@ -289,7 +296,7 @@ func (c *PostgresAnalyticsClient) QueryBreakdown( type rawRow struct { bucket string savings float64 - upfront float64 + upfront sql.NullFloat64 purchases int } var raws []rawRow @@ -308,13 +315,17 @@ func (c *PostgresAnalyticsClient) QueryBreakdown( result := make(map[string]BreakdownValue, len(raws)) for _, r := range raws { + var upfront *float64 + if r.upfront.Valid { + upfront = &r.upfront.Float64 + } pct := 0.0 if totalSavings > 0 { pct = (r.savings / totalSavings) * 100.0 } result[r.bucket] = BreakdownValue{ TotalSavings: r.savings, - TotalUpfront: r.upfront, + TotalUpfront: upfront, PurchaseCount: r.purchases, Percentage: pct, } diff --git a/internal/api/analytics_postgres_test.go b/internal/api/analytics_postgres_test.go index 1baf5aef6..dbbc8e1e7 100644 --- a/internal/api/analytics_postgres_test.go +++ b/internal/api/analytics_postgres_test.go @@ -79,7 +79,7 @@ func TestQueryHistory_Success(t *testing.T) { // Bucket 1 aggregates: savings 140, upfront 60, purchases 3. assert.Equal(t, bucket1, points[0].Timestamp) assert.InDelta(t, 140.0, points[0].TotalSavings, 1e-9) - assert.InDelta(t, 60.0, points[0].TotalUpfront, 1e-9) + assert.InDelta(t, 60.0, *points[0].TotalUpfront, 1e-9) assert.Equal(t, 3, points[0].PurchaseCount) assert.InDelta(t, 100.0, points[0].ByService["ec2"], 1e-9) assert.InDelta(t, 40.0, points[0].ByService["rds"], 1e-9) @@ -94,7 +94,7 @@ func TestQueryHistory_Success(t *testing.T) { require.NotNil(t, summary) assert.Equal(t, 4, summary.TotalPurchases) assert.Equal(t, 4, summary.TotalCompleted) - assert.InDelta(t, 60.0, summary.TotalUpfront, 1e-9) + assert.InDelta(t, 60.0, *summary.TotalUpfront, 1e-9) assert.InDelta(t, 215.0, summary.TotalMonthlySavings, 1e-9) assert.InDelta(t, 215.0*12, summary.TotalAnnualSavings, 1e-9) @@ -137,7 +137,7 @@ func TestQueryBreakdown_Success(t *testing.T) { ec2 := out["ec2"] assert.InDelta(t, 300.0, ec2.TotalSavings, 1e-9) - assert.InDelta(t, 150.0, ec2.TotalUpfront, 1e-9) + assert.InDelta(t, 150.0, *ec2.TotalUpfront, 1e-9) assert.Equal(t, 5, ec2.PurchaseCount) assert.InDelta(t, 75.0, ec2.Percentage, 1e-9) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index d0688b567..85e673c22 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -488,7 +488,7 @@ func projectRecommendationFields(row *config.PurchaseHistoryRecord, exec config. row.Region = r.Region row.Term = r.Term row.Payment = r.Payment - row.UpfrontCost = r.UpfrontCost + row.UpfrontCost = &r.UpfrontCost row.EstimatedSavings = r.Savings row.MonthlyCost = r.MonthlyCost return @@ -499,7 +499,7 @@ func projectRecommendationFields(row *config.PurchaseHistoryRecord, exec config. row.Term = collapseRecommendationTerm(recs) row.Payment = collapseRecommendationPayment(recs) row.MonthlyCost = sumRecommendationMonthlyCostPtr(recs) - row.UpfrontCost = exec.TotalUpfrontCost + row.UpfrontCost = &exec.TotalUpfrontCost row.EstimatedSavings = exec.EstimatedSavings } @@ -1020,7 +1020,7 @@ func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, se } func summarizePurchaseHistory(purchases []config.PurchaseHistoryRecord) HistorySummary { - summary := HistorySummary{TotalPurchases: len(purchases)} + summary := HistorySummary{TotalPurchases: len(purchases), TotalUpfront: new(float64)} for _rvc := range purchases { p := purchases[_rvc] // Revoked commitments are excluded from dollar KPIs regardless of their @@ -1085,7 +1085,7 @@ func summarizePurchaseHistory(purchases []config.PurchaseHistoryRecord) HistoryS if p.IsAuditGap { continue } - summary.TotalUpfront += p.UpfrontCost + summary.TotalUpfront = sumKnownCosts(summary.TotalUpfront, p.UpfrontCost) summary.TotalMonthlySavings += p.EstimatedSavings } summary.TotalAnnualSavings = summary.TotalMonthlySavings * 12 diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 1f08287b4..e679f6651 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -34,7 +34,7 @@ func TestHandler_getHistory(t *testing.T) { mockStore := new(MockConfigStore) history := []config.PurchaseHistoryRecord{ - {AccountID: "123456789012", PurchaseID: "purchase-1", UpfrontCost: 100.0, EstimatedSavings: 10.0}, + {AccountID: "123456789012", PurchaseID: "purchase-1", UpfrontCost: new(float64(100.0)), EstimatedSavings: 10.0}, } // account_id "123456789012" is not a known cloud_accounts UUID, so it is @@ -59,7 +59,7 @@ func TestHandler_getHistory(t *testing.T) { assert.Equal(t, 1, historyResp.Summary.TotalPurchases) assert.Equal(t, 1, historyResp.Summary.TotalCompleted) assert.Equal(t, 0, historyResp.Summary.TotalPending) - assert.Equal(t, 100.0, historyResp.Summary.TotalUpfront) + assert.Equal(t, 100.0, *historyResp.Summary.TotalUpfront) assert.Equal(t, 10.0, historyResp.Summary.TotalMonthlySavings) assert.Equal(t, 120.0, historyResp.Summary.TotalAnnualSavings) } @@ -69,8 +69,8 @@ func TestHandler_getHistory_AllAccounts(t *testing.T) { mockStore := new(MockConfigStore) history := []config.PurchaseHistoryRecord{ - {AccountID: "111111111111", PurchaseID: "purchase-1", UpfrontCost: 100.0, EstimatedSavings: 10.0}, - {AccountID: "222222222222", PurchaseID: "purchase-2", UpfrontCost: 200.0, EstimatedSavings: 20.0}, + {AccountID: "111111111111", PurchaseID: "purchase-1", UpfrontCost: new(float64(100.0)), EstimatedSavings: 10.0}, + {AccountID: "222222222222", PurchaseID: "purchase-2", UpfrontCost: new(float64(200.0)), EstimatedSavings: 20.0}, } mockStore.On("GetAllPurchaseHistory", ctx, 100).Return(history, nil) @@ -88,7 +88,7 @@ func TestHandler_getHistory_AllAccounts(t *testing.T) { assert.Len(t, historyResp.Purchases, 2) assert.Equal(t, 2, historyResp.Summary.TotalPurchases) assert.Equal(t, 2, historyResp.Summary.TotalCompleted) - assert.Equal(t, 300.0, historyResp.Summary.TotalUpfront) + assert.Equal(t, 300.0, *historyResp.Summary.TotalUpfront) assert.Equal(t, 30.0, historyResp.Summary.TotalMonthlySavings) } @@ -101,7 +101,7 @@ func TestHandler_getHistory_IncludesPending(t *testing.T) { mockStore := new(MockConfigStore) completed := []config.PurchaseHistoryRecord{ - {AccountID: "acc-1", PurchaseID: "done-1", UpfrontCost: 500.0, EstimatedSavings: 50.0}, + {AccountID: "acc-1", PurchaseID: "done-1", UpfrontCost: new(float64(500.0)), EstimatedSavings: 50.0}, } pending := []config.PurchaseExecution{ { @@ -136,7 +136,7 @@ func TestHandler_getHistory_IncludesPending(t *testing.T) { assert.Equal(t, 2, historyResp.Summary.TotalPurchases) assert.Equal(t, 1, historyResp.Summary.TotalCompleted) assert.Equal(t, 1, historyResp.Summary.TotalPending) - assert.Equal(t, 500.0, historyResp.Summary.TotalUpfront, "pending spend must not inflate committed totals") + assert.Equal(t, 500.0, *historyResp.Summary.TotalUpfront, "pending spend must not inflate committed totals") assert.Equal(t, 50.0, historyResp.Summary.TotalMonthlySavings) // Locate the pending row by PurchaseID and assert its shape. @@ -150,7 +150,7 @@ func TestHandler_getHistory_IncludesPending(t *testing.T) { require.NotNil(t, pendingRow, "pending execution must render as a history row") assert.Equal(t, "pending", pendingRow.Status) assert.Equal(t, "aws", pendingRow.Provider, "single-provider execution must collapse to that provider") - assert.Equal(t, 999.0, pendingRow.UpfrontCost) + assert.Equal(t, 999.0, *pendingRow.UpfrontCost) assert.Equal(t, 2, pendingRow.Count) assert.Equal(t, "2 commitment(s)", pendingRow.ResourceType) assert.Equal(t, approverEmail, pendingRow.Approver, "pending rows must expose the approver email so the UI can tell the user whose inbox to check") @@ -686,7 +686,7 @@ func TestHandler_getHistory_IncludesInFlightApprovals(t *testing.T) { mockStore := new(MockConfigStore) completed := []config.PurchaseHistoryRecord{ - {AccountID: "acc-1", PurchaseID: "done-1", UpfrontCost: 500.0, EstimatedSavings: 50.0}, + {AccountID: "acc-1", PurchaseID: "done-1", UpfrontCost: new(float64(500.0)), EstimatedSavings: 50.0}, } approver := "ops@example.com" inFlight := []config.PurchaseExecution{ @@ -735,7 +735,7 @@ func TestHandler_getHistory_IncludesInFlightApprovals(t *testing.T) { assert.Equal(t, 3, resp.Summary.TotalPurchases) assert.Equal(t, 1, resp.Summary.TotalCompleted) assert.Equal(t, 2, resp.Summary.TotalInProgress, "approved+running must count as in-progress") - assert.Equal(t, 500.0, resp.Summary.TotalUpfront, "in-flight spend must not inflate committed totals") + assert.Equal(t, 500.0, *resp.Summary.TotalUpfront, "in-flight spend must not inflate committed totals") assert.Equal(t, 50.0, resp.Summary.TotalMonthlySavings) } @@ -800,7 +800,7 @@ func TestHandler_getHistory_InProgressRowMapsRecFields(t *testing.T) { assert.Equal(t, "eu-west-1", row.Region, "region must be the rec's, not 'multiple'") assert.Equal(t, 1, row.Term, "term must be the rec's 1yr, not 0 ('0 Years')") assert.Equal(t, 1, row.Count) - assert.Equal(t, 0.0, row.UpfrontCost, "upfront must come from the rec") + assert.Equal(t, 0.0, *row.UpfrontCost, "upfront must come from the rec") assert.Equal(t, 1.2333, row.EstimatedSavings, "savings must come from the rec") require.NotNil(t, row.MonthlyCost, "monthly cost must come from the rec") assert.InDelta(t, 2.117, *row.MonthlyCost, 1e-9, "monthly cost must come from the rec") @@ -856,7 +856,7 @@ func TestHandler_getHistory_AuditGapCompletedVisible(t *testing.T) { // purchase_history rows AND this synthesized row, so its execution-level // dollars must NOT be added to the committed totals (those come from the // purchase_history rows that actually saved). - assert.Equal(t, 0.0, resp.Summary.TotalUpfront, "audit-gap row must not contribute execution-level dollars (double-count risk)") + assert.Equal(t, 0.0, *resp.Summary.TotalUpfront, "audit-gap row must not contribute execution-level dollars (double-count risk)") assert.Equal(t, 0.0, resp.Summary.TotalMonthlySavings) } @@ -901,7 +901,7 @@ func TestHandler_getHistory_PartiallyCompletedVisible(t *testing.T) { assert.Contains(t, row.StatusDescription, "partially completed", "the partial outcome must be surfaced to the user") assert.True(t, row.IsAuditGap, "partial row must carry IsAuditGap so its execution-level dollars are excluded") assert.Equal(t, 1, resp.Summary.TotalCompleted, "money was committed, so it counts as completed") - assert.Equal(t, 0.0, resp.Summary.TotalUpfront, "partial row must not contribute execution-level dollars (committed dollars come from purchase_history rows)") + assert.Equal(t, 0.0, *resp.Summary.TotalUpfront, "partial row must not contribute execution-level dollars (committed dollars come from purchase_history rows)") assert.Equal(t, 0.0, resp.Summary.TotalMonthlySavings) } @@ -925,7 +925,7 @@ func TestHandler_getHistory_CompletedDBRowWithDescriptionStillCounts(t *testing. PurchaseID: "ri-commitment-1", Status: "completed", StatusDescription: "approved by ops@example.com", - UpfrontCost: 700.0, + UpfrontCost: new(float64(700.0)), EstimatedSavings: 70.0, }, } @@ -944,7 +944,7 @@ func TestHandler_getHistory_CompletedDBRowWithDescriptionStillCounts(t *testing. require.Len(t, resp.Purchases, 1) assert.False(t, resp.Purchases[0].IsAuditGap, "DB-loaded completed rows are never audit gaps") assert.Equal(t, 1, resp.Summary.TotalCompleted) - assert.Equal(t, 700.0, resp.Summary.TotalUpfront, "a completed DB row with a StatusDescription must still count its committed dollars") + assert.Equal(t, 700.0, *resp.Summary.TotalUpfront, "a completed DB row with a StatusDescription must still count its committed dollars") assert.Equal(t, 70.0, resp.Summary.TotalMonthlySavings) } @@ -986,7 +986,7 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { // SQL path: must be called via GetPurchaseHistoryFiltered with // provider="aws" and no other filters set. Return only the aws row. filtered := []config.PurchaseHistoryRecord{ - {AccountID: "acc-aws", PurchaseID: "p-aws", Provider: "aws", UpfrontCost: 100.0}, + {AccountID: "acc-aws", PurchaseID: "p-aws", Provider: "aws", UpfrontCost: new(float64(100.0))}, } mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{Provider: "aws", Limit: config.DefaultListLimit}). Return(filtered, nil).Once() @@ -1668,7 +1668,7 @@ func TestHandler_getHistory_ExternalIDOnlyAccount(t *testing.T) { // BOTH the UUID and the resolved external id. The row itself has the external // AccountID and (implicitly) a NULL cloud_account_id. bRow := []config.PurchaseHistoryRecord{ - {AccountID: accountBExternal, PurchaseID: "p-B", Provider: "aws", Service: "ec2", UpfrontCost: 100.0, EstimatedSavings: 25.0}, + {AccountID: accountBExternal, PurchaseID: "p-B", Provider: "aws", Service: "ec2", UpfrontCost: new(float64(100.0)), EstimatedSavings: 25.0}, } mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ AccountIDs: []string{accountBUUID}, @@ -1726,7 +1726,7 @@ func TestHandler_getHistory_CompletedExecutionNotDuplicated(t *testing.T) { mockStore := new(MockConfigStore) completed := []config.PurchaseHistoryRecord{ - {AccountID: "acc-1", PurchaseID: "ri-commitment-1", UpfrontCost: 400.0, EstimatedSavings: 40.0}, + {AccountID: "acc-1", PurchaseID: "ri-commitment-1", UpfrontCost: new(float64(400.0)), EstimatedSavings: 40.0}, } // A clean completed execution exists alongside a purchase_history row. The // execution (exec-clean-1) and the purchase_history row (ri-commitment-1) @@ -1758,7 +1758,7 @@ func TestHandler_getHistory_CompletedExecutionNotDuplicated(t *testing.T) { require.Len(t, resp.Purchases, 1, "a clean completed execution must not duplicate its purchase_history row") assert.Equal(t, "ri-commitment-1", resp.Purchases[0].PurchaseID, "the surviving row is the purchase_history row") assert.Equal(t, 1, resp.Summary.TotalCompleted) - assert.Equal(t, 400.0, resp.Summary.TotalUpfront) + assert.Equal(t, 400.0, *resp.Summary.TotalUpfront) } // TestSummarizePurchaseHistory_CancelledExcludedFromKPIs is the regression @@ -1771,11 +1771,11 @@ func TestHandler_getHistory_CompletedExecutionNotDuplicated(t *testing.T) { func TestSummarizePurchaseHistory_CancelledExcludedFromKPIs(t *testing.T) { purchases := []config.PurchaseHistoryRecord{ // Three completed rows that should contribute to the KPI totals. - {Status: "completed", UpfrontCost: 100.0, EstimatedSavings: 10.0}, - {Status: "completed", UpfrontCost: 200.0, EstimatedSavings: 20.0}, - {Status: "", UpfrontCost: 50.0, EstimatedSavings: 5.0}, // legacy row, no status + {Status: "completed", UpfrontCost: new(float64(100.0)), EstimatedSavings: 10.0}, + {Status: "completed", UpfrontCost: new(float64(200.0)), EstimatedSavings: 20.0}, + {Status: "", UpfrontCost: new(float64(50.0)), EstimatedSavings: 5.0}, // legacy row, no status // One pending row that should be counted as pending, not completed. - {Status: "pending", UpfrontCost: 999.0, EstimatedSavings: 99.0}, + {Status: "pending", UpfrontCost: new(float64(999.0)), EstimatedSavings: 99.0}, // Two canceled rows — the regression case from issue #736. Neither must // appear in the dollar KPIs or TotalCompleted. One uses the new US // spelling (config.StatusCanceled) and one the legacy British spelling @@ -1785,8 +1785,8 @@ func TestSummarizePurchaseHistory_CancelledExcludedFromKPIs(t *testing.T) { // the legacy value without a literal the US-locale misspell linter would // flag (and without a nolint). Drop the legacy fixture once the contract // migration (#1278) normalizes the data. - {Status: config.StatusCanceled, UpfrontCost: 500.0, EstimatedSavings: 50.0}, - {Status: config.LegacyStatusCanceled, UpfrontCost: 750.0, EstimatedSavings: 75.0}, + {Status: config.StatusCanceled, UpfrontCost: new(float64(500.0)), EstimatedSavings: 50.0}, + {Status: config.LegacyStatusCanceled, UpfrontCost: new(float64(750.0)), EstimatedSavings: 75.0}, } summary := summarizePurchaseHistory(purchases) @@ -1795,7 +1795,7 @@ func TestSummarizePurchaseHistory_CancelledExcludedFromKPIs(t *testing.T) { assert.Equal(t, 3, summary.TotalCompleted, "canceled rows must not inflate TotalCompleted") assert.Equal(t, 1, summary.TotalPending) - assert.InDelta(t, 350.0, summary.TotalUpfront, 0.001, + assert.InDelta(t, 350.0, *summary.TotalUpfront, 0.001, "canceled upfront cost must not be included in TotalUpfront (issues #625, #736)") assert.InDelta(t, 35.0, summary.TotalMonthlySavings, 0.001, "canceled savings must not be included in TotalMonthlySavings (issues #625, #736)") @@ -1814,8 +1814,8 @@ func TestSummarizePurchaseHistory_CancelledExcludedFromKPIs(t *testing.T) { // so it must land in TotalPending like those two, not TotalCompleted. func TestSummarizePurchaseHistory_ScheduledExcludedFromKPIs(t *testing.T) { purchases := []config.PurchaseHistoryRecord{ - {Status: "completed", UpfrontCost: 100.0, EstimatedSavings: 10.0}, - {Status: "scheduled", UpfrontCost: 999.0, EstimatedSavings: 99.0}, + {Status: "completed", UpfrontCost: new(float64(100.0)), EstimatedSavings: 10.0}, + {Status: "scheduled", UpfrontCost: new(float64(999.0)), EstimatedSavings: 99.0}, } summary := summarizePurchaseHistory(purchases) @@ -1823,7 +1823,7 @@ func TestSummarizePurchaseHistory_ScheduledExcludedFromKPIs(t *testing.T) { assert.Equal(t, 2, summary.TotalPurchases) assert.Equal(t, 1, summary.TotalCompleted, "scheduled row must not inflate TotalCompleted") assert.Equal(t, 1, summary.TotalPending, "scheduled is a pre-purchase state, grouped with pending/notified") - assert.InDelta(t, 100.0, summary.TotalUpfront, 0.001, + assert.InDelta(t, 100.0, *summary.TotalUpfront, 0.001, "the scheduled row's upfront cost must not be counted as already spent (issue #239)") assert.InDelta(t, 10.0, summary.TotalMonthlySavings, 0.001, "the scheduled row's savings must not be counted as realized (issue #239)") @@ -1916,16 +1916,16 @@ func TestSummarizePurchaseHistory_CancelPendingDoesNotChangeKPIs(t *testing.T) { // Baseline: three approved (completed) rows. baseline := make([]config.PurchaseHistoryRecord, 0, 4) baseline = append(baseline, - config.PurchaseHistoryRecord{Status: "completed", UpfrontCost: 100.0, EstimatedSavings: 10.0}, - config.PurchaseHistoryRecord{Status: "completed", UpfrontCost: 200.0, EstimatedSavings: 20.0}, - config.PurchaseHistoryRecord{Status: "completed", UpfrontCost: 300.0, EstimatedSavings: 30.0}, + config.PurchaseHistoryRecord{Status: "completed", UpfrontCost: new(float64(100.0)), EstimatedSavings: 10.0}, + config.PurchaseHistoryRecord{Status: "completed", UpfrontCost: new(float64(200.0)), EstimatedSavings: 20.0}, + config.PurchaseHistoryRecord{Status: "completed", UpfrontCost: new(float64(300.0)), EstimatedSavings: 30.0}, ) before := summarizePurchaseHistory(baseline) // After: same rows plus one canceled execution (the pending that got canceled). withCancelled := append(baseline, config.PurchaseHistoryRecord{ //nolint:gocritic Status: "cancelled", //nolint:misspell // DB schema value 'cancelled' -- see migration 000001_initial_schema.up.sql - UpfrontCost: 999.0, + UpfrontCost: new(float64(999.0)), EstimatedSavings: 99.0, }) after := summarizePurchaseHistory(withCancelled) @@ -2074,13 +2074,13 @@ func TestSummarizePurchaseHistory_RevokedExcludedFromKPIs(t *testing.T) { purchases := []config.PurchaseHistoryRecord{ // Completed, non-revoked: must contribute to dollar totals. - {Status: "completed", UpfrontCost: 100.0, EstimatedSavings: 10.0}, + {Status: "completed", UpfrontCost: new(float64(100.0)), EstimatedSavings: 10.0}, // Revoked: term still open but revoked_at is set. // Pre-fix: fell through to the completed branch, inflating TotalUpfront // by 500 and TotalMonthlySavings by 50. - {Status: "completed", UpfrontCost: 500.0, EstimatedSavings: 50.0, RevokedAt: &revokedAt}, + {Status: "completed", UpfrontCost: new(float64(500.0)), EstimatedSavings: 50.0, RevokedAt: &revokedAt}, // Revoked with empty status (legacy DB row): same guard must apply. - {Status: "", UpfrontCost: 750.0, EstimatedSavings: 75.0, RevokedAt: &revokedAt}, + {Status: "", UpfrontCost: new(float64(750.0)), EstimatedSavings: 75.0, RevokedAt: &revokedAt}, } summary := summarizePurchaseHistory(purchases) @@ -2091,7 +2091,7 @@ func TestSummarizePurchaseHistory_RevokedExcludedFromKPIs(t *testing.T) { assert.Equal(t, 1, summary.TotalCompleted, "revoked rows must not inflate TotalCompleted") - assert.InDelta(t, 100.0, summary.TotalUpfront, 0.001, + assert.InDelta(t, 100.0, *summary.TotalUpfront, 0.001, "revoked upfront cost must not appear in TotalUpfront") assert.InDelta(t, 10.0, summary.TotalMonthlySavings, 0.001, "revoked savings must not appear in TotalMonthlySavings") diff --git a/internal/api/handler_inventory.go b/internal/api/handler_inventory.go index 594d12e9d..59a6af2b7 100644 --- a/internal/api/handler_inventory.go +++ b/internal/api/handler_inventory.go @@ -205,13 +205,19 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu // (MonthlyCost nil, UpfrontCost > 0 — typical for Azure RIs) still // registers as covered instead of being silently dropped (issue: Azure // showed $0 coverage while the dashboard reported active commitments). - coveredByKey := make(map[string]float64) + coveredByKey := make(map[string]*float64) for _rvc := range purchases { p := purchases[_rvc] if !isActiveCommitment(p, now) { continue } - coveredByKey[p.Provider+":"+p.Service] += commitmentCoveredMonthly(p) + key := p.Provider + ":" + p.Service + cost := commitmentCoveredMonthly(p) + total, exists := coveredByKey[key] + if !exists { + total = new(float64) + } + coveredByKey[key] = sumKnownCosts(total, cost) } // --- on-demand gap: recommendations ------------------------------------- @@ -295,15 +301,18 @@ func aggregateOnDemandByKey(recs []config.RecommendationRecord, providerFilter s // its recurring MonthlyCost. The scheduler only writes Term >= 1 rows, so this // guard matches analytics.Collector's skip-bad-term defense rather than papering // over real data. -func commitmentCoveredMonthly(p config.PurchaseHistoryRecord) float64 { +func commitmentCoveredMonthly(p config.PurchaseHistoryRecord) *float64 { + if p.UpfrontCost == nil { + return nil + } var covered float64 if p.MonthlyCost != nil { covered += *p.MonthlyCost } if p.Term > 0 { - covered += p.UpfrontCost / (float64(p.Term) * analytics.MonthsPerYear) + covered += *p.UpfrontCost / (float64(p.Term) * analytics.MonthsPerYear) } - return covered + return &covered } // buildCoverageBreakdown constructs the CoverageBreakdownResponse from @@ -314,7 +323,7 @@ func commitmentCoveredMonthly(p config.PurchaseHistoryRecord) float64 { // A provider that appears in neither map gets Services=nil and // OverallCoveragePct=nil (no usage detected). func buildCoverageBreakdown( - coveredByKey map[string]float64, + coveredByKey map[string]*float64, onDemandByKey map[string]float64, ) CoverageBreakdownResponse { // Collect all (provider, service) keys appearing in either map. @@ -333,7 +342,10 @@ func buildCoverageBreakdown( providerSvcMap := make(map[string][]CoverageServiceRow) for ps := range keySet { key := ps.provider + ":" + ps.service - covered := coveredByKey[key] + covered, exists := coveredByKey[key] + if !exists { + covered = new(float64) + } onDemand := onDemandByKey[key] row := CoverageServiceRow{ Service: ps.service, @@ -368,9 +380,10 @@ func buildCoverageBreakdown( continue } - var totalCovered, totalOnDemand float64 + totalCovered := new(float64) + var totalOnDemand float64 for _, r := range rows { - totalCovered += r.CoveredMonthly + totalCovered = sumKnownCosts(totalCovered, r.CoveredMonthly) totalOnDemand += r.OnDemandMonthly } sections = append(sections, ProviderCoverageSection{ @@ -387,12 +400,15 @@ func buildCoverageBreakdown( // Returns nil when both inputs are zero (no usage signal) to preserve the // "absent" semantic per feedback_nullable_not_zero — callers should render // nil as "N/A", not "0%". -func coveragePct(covered, onDemand float64) *float64 { - total := covered + onDemand +func coveragePct(covered *float64, onDemand float64) *float64 { + if covered == nil { + return nil + } + total := *covered + onDemand if total == 0 { return nil } - pct := covered / total * 100 + pct := *covered / total * 100 return &pct } diff --git a/internal/api/handler_inventory_test.go b/internal/api/handler_inventory_test.go index 9b45d3d57..81ccdfe90 100644 --- a/internal/api/handler_inventory_test.go +++ b/internal/api/handler_inventory_test.go @@ -66,7 +66,7 @@ func TestHandler_listActiveCommitments_FiltersExpired(t *testing.T) { now := time.Now() purchases := []config.PurchaseHistoryRecord{ // Active: bought 6 months ago, 1-year term — 6 months remaining. - { + {UpfrontCost: new(float64), AccountID: "acc-active", PurchaseID: "p-active", Provider: "aws", @@ -80,7 +80,7 @@ func TestHandler_listActiveCommitments_FiltersExpired(t *testing.T) { EstimatedSavings: 30.0, }, // Expired: bought 2 years ago, 1-year term. - { + {UpfrontCost: new(float64), AccountID: "acc-expired", PurchaseID: "p-expired", Provider: "aws", @@ -144,7 +144,7 @@ func TestHandler_listActiveCommitments_AccountFilter(t *testing.T) { now := time.Now() purchases := []config.PurchaseHistoryRecord{ - { + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-1", Provider: "aws", @@ -187,7 +187,7 @@ func TestHandler_listActiveCommitments_ProviderFilter(t *testing.T) { now := time.Now() purchases := []config.PurchaseHistoryRecord{ - { + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-aws", Provider: "aws", @@ -197,7 +197,7 @@ func TestHandler_listActiveCommitments_ProviderFilter(t *testing.T) { Count: 1, MonthlyCost: float64Ptr(80.0), }, - { + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-azure", Provider: "azure", @@ -239,7 +239,7 @@ func TestHandler_listActiveCommitments_SortedByExpiry(t *testing.T) { // "out of order" so the sort step actually has work to do. purchases := []config.PurchaseHistoryRecord{ // 30 months remaining (3y term, bought 6mo ago). - { + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-long", Provider: "aws", @@ -249,7 +249,7 @@ func TestHandler_listActiveCommitments_SortedByExpiry(t *testing.T) { Count: 1, }, // 6 months remaining (1y term, bought 6mo ago). - { + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-short", Provider: "aws", @@ -259,7 +259,7 @@ func TestHandler_listActiveCommitments_SortedByExpiry(t *testing.T) { Count: 1, }, // 18 months remaining (3y term, bought 18mo ago). - { + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-mid", Provider: "aws", @@ -308,7 +308,7 @@ func splitPurchaseID(id string) string { // cause the two views to disagree about which commitments are active. func TestHandler_isActiveCommitment_Predicate(t *testing.T) { now := time.Now() - p := config.PurchaseHistoryRecord{ + p := config.PurchaseHistoryRecord{UpfrontCost: new(float64), Timestamp: now.AddDate(-1, 0, 0), Term: 1, // 1y term, started 1y ago — at the boundary. } @@ -319,7 +319,7 @@ func TestHandler_isActiveCommitment_Predicate(t *testing.T) { assert.True(t, isActiveCommitment(p, now.Add(-time.Hour)), "a commitment one hour before its expiry must still be active") - expired := config.PurchaseHistoryRecord{ + expired := config.PurchaseHistoryRecord{UpfrontCost: new(float64), Timestamp: now.AddDate(-2, 0, 0), Term: 1, } @@ -335,9 +335,9 @@ func TestHandler_isActiveCommitment_Predicate(t *testing.T) { // sums are correctly attributed per service and the overall coverage% is // computed across all services in the provider. func TestBuildCoverageBreakdown_SingleProvider(t *testing.T) { - covered := map[string]float64{ - "aws:ec2": 200.0, - "aws:rds": 100.0, + covered := map[string]*float64{ + "aws:ec2": new(float64(200.0)), + "aws:rds": new(float64(100.0)), } onDemand := map[string]float64{ "aws:ec2": 300.0, // ec2 coverage = 200/(200+300) = 40% @@ -355,14 +355,14 @@ func TestBuildCoverageBreakdown_SingleProvider(t *testing.T) { // Services are sorted alphabetically; ec2 < rds. ec2 := aws.Services[0] assert.Equal(t, "ec2", ec2.Service) - assert.Equal(t, 200.0, ec2.CoveredMonthly) + assert.Equal(t, 200.0, *ec2.CoveredMonthly) assert.Equal(t, 300.0, ec2.OnDemandMonthly) require.NotNil(t, ec2.CoveragePct) assert.InDelta(t, 40.0, *ec2.CoveragePct, 0.001, "ec2 coverage = 200/500 * 100") rds := aws.Services[1] assert.Equal(t, "rds", rds.Service) - assert.Equal(t, 100.0, rds.CoveredMonthly) + assert.Equal(t, 100.0, *rds.CoveredMonthly) assert.Equal(t, 0.0, rds.OnDemandMonthly) require.NotNil(t, rds.CoveragePct) assert.InDelta(t, 100.0, *rds.CoveragePct, 0.001, "rds coverage = 100/100 * 100") @@ -376,7 +376,7 @@ func TestBuildCoverageBreakdown_SingleProvider(t *testing.T) { // data in either map gets Services=nil and OverallCoveragePct=nil — not // a zero — per feedback_nullable_not_zero. func TestBuildCoverageBreakdown_EmptyProvider(t *testing.T) { - covered := map[string]float64{"aws:ec2": 100.0} + covered := map[string]*float64{"aws:ec2": new(float64(100.0))} onDemand := map[string]float64{"aws:ec2": 100.0} resp := buildCoverageBreakdown(covered, onDemand) @@ -394,13 +394,13 @@ func TestBuildCoverageBreakdown_EmptyProvider(t *testing.T) { // TestBuildCoverageBreakdown_ZeroBothSides verifies that a service with // both covered=0 and on_demand=0 produces a nil CoveragePct, not 0. func TestBuildCoverageBreakdown_ZeroBothSides(t *testing.T) { - assert.Nil(t, coveragePct(0, 0), "no usage: coverage% must be nil, not 0") + assert.Nil(t, coveragePct(new(float64), 0), "no usage: coverage% must be nil, not 0") } // TestBuildCoverageBreakdown_OnlyOnDemand verifies that a provider with // recommendations but no commitments shows 0% coverage (not nil). func TestBuildCoverageBreakdown_OnlyOnDemand(t *testing.T) { - covered := map[string]float64{} + covered := map[string]*float64{} onDemand := map[string]float64{"azure:compute": 500.0} resp := buildCoverageBreakdown(covered, onDemand) @@ -432,7 +432,7 @@ func TestHandler_getCoverageBreakdown_Integration(t *testing.T) { now := time.Now() purchases := []config.PurchaseHistoryRecord{ - { + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-1", Provider: "aws", @@ -501,7 +501,7 @@ func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { // acc-1 rows here. The provider chip is applied in-memory by // fetchCommitmentRecords to drop the azure row. acc1Purchases := []config.PurchaseHistoryRecord{ - { + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-aws-acc1", Provider: "aws", @@ -510,7 +510,7 @@ func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { Term: 1, MonthlyCost: float64Ptr(200.0), }, - { + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-azure-acc1", Provider: "azure", @@ -576,7 +576,7 @@ func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { ec2 := aws.Services[0] assert.Equal(t, "ec2", ec2.Service) - assert.Equal(t, 200.0, ec2.CoveredMonthly, "covered comes from acc-1 purchase only") + assert.Equal(t, 200.0, *ec2.CoveredMonthly, "covered comes from acc-1 purchase only") assert.Equal(t, 300.0, ec2.OnDemandMonthly, "on-demand comes from acc-1 rec only") require.NotNil(t, ec2.CoveragePct) // 200 / (200+300) * 100 = 40 @@ -631,7 +631,7 @@ func TestHandler_getCoverageBreakdown_AzureAllUpfrontConsistency(t *testing.T) { Service: "compute", Timestamp: now.AddDate(0, -2, 0), // active: 1y term started 2mo ago Term: 1, - UpfrontCost: 1200.0, + UpfrontCost: new(float64(1200.0)), MonthlyCost: nil, // all-upfront: no recurring charge at the commitment layer EstimatedSavings: 166.0, } @@ -681,7 +681,7 @@ func TestHandler_getCoverageBreakdown_AzureAllUpfrontConsistency(t *testing.T) { compute := azure.Services[0] assert.Equal(t, "compute", compute.Service) // $1200 upfront / (1yr * 12mo) = $100/mo amortized covered spend. - assert.InDelta(t, 100.0, compute.CoveredMonthly, 0.001, + assert.InDelta(t, 100.0, *compute.CoveredMonthly, 0.001, "covered monthly = amortized upfront for an all-upfront commitment") assert.Equal(t, 0.0, compute.OnDemandMonthly) require.NotNil(t, compute.CoveragePct) @@ -715,7 +715,7 @@ func TestHandler_listActiveCommitments_NoTruncationBeyond1000(t *testing.T) { now := time.Now() // Oldest row: 3-year commitment purchased ~2.5 years ago, still active. - oldActive := config.PurchaseHistoryRecord{ + oldActive := config.PurchaseHistoryRecord{UpfrontCost: new(float64), AccountID: "111122223333", PurchaseID: "p-old-active", Provider: "aws", @@ -729,7 +729,7 @@ func TestHandler_listActiveCommitments_NoTruncationBeyond1000(t *testing.T) { // 1000 newer rows: 1-year commitments purchased ~2 years ago, all expired. newestFirst := make([]config.PurchaseHistoryRecord, 0, config.MaxListLimit) for i := config.MaxListLimit - 1; i >= 0; i-- { - newestFirst = append(newestFirst, config.PurchaseHistoryRecord{ + newestFirst = append(newestFirst, config.PurchaseHistoryRecord{UpfrontCost: new(float64), AccountID: "111122223333", PurchaseID: fmt.Sprintf("p-expired-%04d", i), Provider: "aws", diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 9fa73b0c0..f56e88449 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -600,7 +600,10 @@ func checkSuppliedScheduleFloor(tiers []MarketplacePriceTier, remainingMonths, o // NOT the raw term, which would overprice older RIs). originalTerm is the full // contract term in months. count is the row's instance count (pricing is per // instance). upfrontCost is the row-total upfront paid across all instances. -func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingMonths, originalTerm, count int, upfrontCost float64) ([]MarketplacePriceTier, error) { +func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingMonths, originalTerm, count int, upfrontCost *float64) ([]MarketplacePriceTier, error) { + if upfrontCost == nil { + return nil, fmt.Errorf("upfront cost is unknown; cannot price this marketplace listing") + } if len(supplied) > 0 { for i, t := range supplied { if t.TermMonths <= 0 { @@ -610,7 +613,7 @@ func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingM return nil, fmt.Errorf("price_schedule[%d]: price must be positive (received %.4f); use a non-zero listing price", i, t.Price) } } - if err := checkSuppliedScheduleFloor(supplied, remainingMonths, originalTerm, count, upfrontCost); err != nil { + if err := checkSuppliedScheduleFloor(supplied, remainingMonths, originalTerm, count, *upfrontCost); err != nil { return nil, err } return supplied, nil @@ -625,7 +628,7 @@ func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingM if remainingMonths <= 0 { remainingMonths = 1 // defensive: should not happen for an active RI } - listPrice := marketplaceResidualPerUnit(remainingMonths, originalTerm, count, upfrontCost) * awsMarketplaceBuyerDiscountFactor + listPrice := marketplaceResidualPerUnit(remainingMonths, originalTerm, count, *upfrontCost) * awsMarketplaceBuyerDiscountFactor if listPrice <= 0 { // A no-upfront RI (upfrontCost <= 0) or an unknown/elapsed term // (originalTerm <= 0) makes marketplaceResidualPerUnit return 0, which diff --git a/internal/api/handler_marketplace_test.go b/internal/api/handler_marketplace_test.go index bdc89e7b5..2cadc9484 100644 --- a/internal/api/handler_marketplace_test.go +++ b/internal/api/handler_marketplace_test.go @@ -105,7 +105,7 @@ func standardRow() *config.PurchaseHistoryRecord { // multiplies by 12 before passing to computeRemainingMonths and // resolveMarketplacePriceSchedule, which both work in months. Term: 3, - UpfrontCost: 1200, + UpfrontCost: new(float64(1200)), Timestamp: time.Now(), } } @@ -351,7 +351,7 @@ func TestMarketplaceList_DBFailureCompensatingRollback(t *testing.T) { func TestMarketplaceList_DefaultScheduleProrationMath(t *testing.T) { // remaining=12, term=12, count=1, upfront=1200 => per-unit residual // 1200*12/12/1 = 1200; default price = 1200*0.95 = 1140. - schedule, err := resolveMarketplacePriceSchedule(nil, 12, 12, 1, 1200) + schedule, err := resolveMarketplacePriceSchedule(nil, 12, 12, 1, new(float64(1200))) require.NoError(t, err) require.Len(t, schedule, 1) assert.Equal(t, int64(12), schedule[0].TermMonths) @@ -360,7 +360,7 @@ func TestMarketplaceList_DefaultScheduleProrationMath(t *testing.T) { // Half the term elapsed, count=1: remaining=6, term=12, upfront=1200. // per-unit residual = 1200*6/12/1 = 600; recurring is excluded (the buyer // assumes it post-transfer); default price = 600*0.95 = 570. - schedule, err = resolveMarketplacePriceSchedule(nil, 6, 12, 1, 1200) + schedule, err = resolveMarketplacePriceSchedule(nil, 6, 12, 1, new(float64(1200))) require.NoError(t, err) require.Len(t, schedule, 1) assert.Equal(t, int64(6), schedule[0].TermMonths) @@ -370,7 +370,7 @@ func TestMarketplaceList_DefaultScheduleProrationMath(t *testing.T) { // the residual by the instance count, so the default per-unit price is a // third of the count=1 price (1200/3=400 per-unit residual; 400*0.95=380), // NOT the row-total (which would 3x-overprice each listed unit). - schedule, err = resolveMarketplacePriceSchedule(nil, 12, 12, 3, 1200) + schedule, err = resolveMarketplacePriceSchedule(nil, 12, 12, 3, new(float64(1200))) require.NoError(t, err) require.Len(t, schedule, 1) assert.Equal(t, int64(12), schedule[0].TermMonths) @@ -483,7 +483,7 @@ func TestMarketplaceList_ClaimErrorMapsToInternal(t *testing.T) { func TestResolveMarketplacePriceSchedule_ZeroPriceRejected(t *testing.T) { _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ {TermMonths: 6, Price: 0}, - }, 6, 12, 1, 1200) + }, 6, 12, 1, new(float64(1200))) require.Error(t, err, "Price=0 must be rejected") assert.Contains(t, err.Error(), "positive") } @@ -497,7 +497,7 @@ func TestResolveMarketplacePriceSchedule_ZeroPriceRejected(t *testing.T) { func TestResolveMarketplacePriceSchedule_BelowFloorRejected(t *testing.T) { _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ {TermMonths: 12, Price: 5.0}, - }, 12, 12, 1, 1200) + }, 12, 12, 1, new(float64(1200))) require.Error(t, err, "a $5 one-time price on a $1,200 residual must be rejected by the per-tier floor") assert.Contains(t, err.Error(), "floor") assert.Contains(t, err.Error(), "residual") @@ -511,13 +511,13 @@ func TestResolveMarketplacePriceSchedule_BelowFloorRejected(t *testing.T) { func TestResolveMarketplacePriceSchedule_PerUnitFloor(t *testing.T) { _, errBelow := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ {TermMonths: 12, Price: 19.0}, - }, 12, 12, 3, 1200) + }, 12, 12, 3, new(float64(1200))) require.Error(t, errBelow, "$19 is below the $20 per-unit floor (5%% of $400 per-unit residual)") assert.Contains(t, errBelow.Error(), "per-unit residual") schedule, errAbove := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ {TermMonths: 12, Price: 21.0}, - }, 12, 12, 3, 1200) + }, 12, 12, 3, new(float64(1200))) require.NoError(t, errAbove, "$21 clears the $20 per-unit floor") require.Len(t, schedule, 1) assert.InDelta(t, 21.0, schedule[0].Price, 0.001) @@ -533,14 +533,14 @@ func TestResolveMarketplacePriceSchedule_PerUnitFloor(t *testing.T) { // This FAILS on the pre-fix code (no error, schedule[0].Price == 0) and // PASSES after (explicit error, nil schedule). func TestResolveMarketplacePriceSchedule_ZeroDefaultPriceRejected(t *testing.T) { - schedule, err := resolveMarketplacePriceSchedule(nil, 12, 12, 1, 0) + schedule, err := resolveMarketplacePriceSchedule(nil, 12, 12, 1, new(float64)) require.Error(t, err, "a no-upfront RI must not silently produce a $0 default listing") assert.Nil(t, schedule) assert.Contains(t, err.Error(), "cannot compute a default listing price") // Same failure mode when the term is unknown (originalTerm <= 0), e.g. an // imported/external row where the contract term could not be resolved. - schedule, err = resolveMarketplacePriceSchedule(nil, 12, 0, 1, 1200) + schedule, err = resolveMarketplacePriceSchedule(nil, 12, 0, 1, new(float64(1200))) require.Error(t, err, "an unknown term must not silently produce a $0 default listing") assert.Nil(t, schedule) assert.Contains(t, err.Error(), "cannot compute a default listing price") @@ -554,7 +554,7 @@ func TestResolveMarketplacePriceSchedule_ZeroDefaultPriceRejected(t *testing.T) func TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected(t *testing.T) { _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ {TermMonths: 12, Price: 1000}, - }, 6, 12, 1, 1200) + }, 6, 12, 1, new(float64(1200))) require.Error(t, err, "term_months exceeding remaining term must be rejected") assert.Contains(t, err.Error(), "exceeds the RI's remaining term") } @@ -692,7 +692,7 @@ func TestMarketplaceList_TermYearsConvertedToMonths(t *testing.T) { row := standardRow() row.Term = 3 // 3-year RI (stored in years) row.Timestamp = time.Now().Add(-6 * 30 * 24 * time.Hour) // purchased ~6 months ago - row.UpfrontCost = 3600 + row.UpfrontCost = new(float64(3600)) row.Count = 1 cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). @@ -737,7 +737,7 @@ func TestMarketplaceList_TermYearsConvertedToMonths(t *testing.T) { // 5% floor (~$150 on a $3,000 prorated residual). schedule, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ {TermMonths: 30, Price: 2500}, - }, 30, 36, 1, 3600) + }, 30, 36, 1, new(float64(3600))) require.NoError(t, err, "a {TermMonths:30, Price:2500} schedule must be accepted when remainingMonths=30 (post-fix)") require.Len(t, schedule, 1) @@ -754,7 +754,7 @@ func TestMarketplaceList_TermYearsConvertedToMonths(t *testing.T) { // old broken inputs -- must still reject it). _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ {TermMonths: 30, Price: 2500}, - }, 1, 3, 1, 3600) + }, 1, 3, 1, new(float64(3600))) require.Error(t, err, "pre-fix inputs (remainingMonths=1, originalTerm=3 months) must reject a 30-month tier") assert.Contains(t, err.Error(), "exceeds the RI's remaining term") diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index b988edabe..3c6e90e0a 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -288,12 +288,12 @@ func TestPerAccountPerms_RecommendationDetail_AllowedAccountReturns200(t *testin func TestPerAccountPerms_History_ListFilter(t *testing.T) { ctx := context.Background() - rowA := config.PurchaseHistoryRecord{ + rowA := config.PurchaseHistoryRecord{UpfrontCost: new(float64), AccountID: permsAccA, PurchaseID: "hist-a", Status: "completed", } - rowB := config.PurchaseHistoryRecord{ + rowB := config.PurchaseHistoryRecord{UpfrontCost: new(float64), AccountID: permsAccB, PurchaseID: "hist-b", Status: "completed", @@ -557,7 +557,7 @@ func TestPerAccountPerms_DashboardSummary_CommitmentMetricsExcludeOtherAccounts( // Only account A's purchase history is returned by the scoped query; the // account-B row (200/mo) is filtered out at the store and must not appear. accountARows := []config.PurchaseHistoryRecord{ - {CloudAccountID: permsPtr(permsAccA), Timestamp: purchaseTime, Term: 1, Service: "ec2", EstimatedSavings: 100.0}, + {UpfrontCost: new(float64), CloudAccountID: permsPtr(permsAccA), Timestamp: purchaseTime, Term: 1, Service: "ec2", EstimatedSavings: 100.0}, } mockStore := new(MockConfigStore) @@ -826,7 +826,7 @@ func TestPerAccountPerms_CoverageBreakdown_RecsFilteredByAllowedAccounts(t *test now := time.Now() // Active commitment for account A: contributes to covered side. - purchaseA := config.PurchaseHistoryRecord{ + purchaseA := config.PurchaseHistoryRecord{UpfrontCost: new(float64), AccountID: permsAccA, PurchaseID: "p-cov-a", Provider: "aws", @@ -894,7 +894,7 @@ func TestPerAccountPerms_CoverageBreakdown_RecsFilteredByAllowedAccounts(t *test ec2 := aws.Services[0] assert.Equal(t, "ec2", ec2.Service) - assert.Equal(t, 200.0, ec2.CoveredMonthly, "covered side must reflect account-A commitment") + assert.Equal(t, 200.0, *ec2.CoveredMonthly, "covered side must reflect account-A commitment") assert.Equal(t, 200.0, ec2.OnDemandMonthly, "on-demand side must include only account-A rec (200); account-B rec (200) must be excluded") // coverage = 200/(200+200) * 100 = 50% @@ -910,7 +910,7 @@ func TestPerAccountPerms_CoverageBreakdown_AdminSeesAll(t *testing.T) { ctx := context.Background() now := time.Now() - purchaseA := config.PurchaseHistoryRecord{ + purchaseA := config.PurchaseHistoryRecord{UpfrontCost: new(float64), AccountID: permsAccA, PurchaseID: "p-cov-admin-a", Provider: "aws", @@ -987,7 +987,7 @@ func TestPerAccountPerms_InventoryCommitments_ScopedSQLRead(t *testing.T) { ctx := context.Background() now := time.Now() - purchaseA := config.PurchaseHistoryRecord{ + purchaseA := config.PurchaseHistoryRecord{UpfrontCost: new(float64), AccountID: permsAccA, PurchaseID: "p-inv-a", Provider: "aws", diff --git a/internal/api/nullable_purchase_cost_integration_test.go b/internal/api/nullable_purchase_cost_integration_test.go new file mode 100644 index 000000000..247eb6669 --- /dev/null +++ b/internal/api/nullable_purchase_cost_integration_test.go @@ -0,0 +1,159 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/analytics" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNullablePurchaseCostPostgres(t *testing.T) { + ctx := context.Background() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, container.Cleanup(ctx)) }) + pool := container.DB.Pool() + path := filepath.Join("..", "database", "postgres", "migrations") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 100)) + store := config.NewPostgresStore(container.DB) + now := time.Now().UTC().Truncate(time.Second) + legacy := &config.PurchaseHistoryRecord{AccountID: "legacy", PurchaseID: "legacy", Timestamp: now, Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "no-upfront", UpfrontCost: new(float64), MonthlyCost: new(float64(40))} + require.NoError(t, store.SavePurchaseHistory(ctx, legacy)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 101)) + up, err := os.ReadFile(filepath.Join(path, "000101_nullable_purchase_upfront_cost.up.sql")) + require.NoError(t, err) + _, err = pool.Exec(ctx, string(up)) + require.NoError(t, err) + loaded, err := store.GetPurchaseHistoryByPurchaseID(ctx, legacy.PurchaseID) + require.NoError(t, err) + require.NotNil(t, loaded.UpfrontCost) + assert.Zero(t, *loaded.UpfrontCost) + assert.Equal(t, legacy.MonthlyCost, loaded.MonthlyCost) + + client := NewPostgresAnalyticsClient(container.DB) + for _, tc := range []struct { + name string + costs []*float64 + want *float64 + }{ + {"known", []*float64{new(float64(120)), new(float64(30))}, new(float64(150))}, + {"zero", []*float64{new(float64)}, new(float64)}, + {"unknown", []*float64{nil}, nil}, + {"mixed", []*float64{new(float64(120)), nil}, nil}, + {"empty", nil, new(float64)}, + } { + t.Run(tc.name, func(t *testing.T) { + for i, cost := range tc.costs { + record := *legacy + record.AccountID = tc.name + record.PurchaseID = fmt.Sprintf("%s-%d", tc.name, i) + record.UpfrontCost = cost + record.EstimatedSavings = 10 + require.NoError(t, store.SavePurchaseHistory(ctx, &record)) + single, singleErr := store.GetPurchaseHistoryByPurchaseID(ctx, record.PurchaseID) + require.NoError(t, singleErr) + assert.Equal(t, cost, single.UpfrontCost) + assert.Equal(t, legacy.MonthlyCost, single.MonthlyCost) + } + rows, rowsErr := store.GetPurchaseHistory(ctx, tc.name, 100) + require.NoError(t, rowsErr) + require.Len(t, rows, len(tc.costs)) + assert.Equal(t, tc.want, summarizePurchaseHistory(rows).TotalUpfront) + scope := map[string][]string{"aws": {tc.name}} + active, activeErr := store.GetActivePurchaseHistory(ctx, now, nil, scope) + require.NoError(t, activeErr) + require.Len(t, active, len(tc.costs)) + points, summary, queryErr := client.QueryHistory(ctx, nil, scope, "aws", now.Add(-time.Hour), now.Add(time.Hour), "daily") + require.NoError(t, queryErr) + assert.Equal(t, tc.want, summary.TotalUpfront) + assert.Equal(t, len(tc.costs), summary.TotalPurchases) + assert.Equal(t, float64(len(tc.costs))*10, summary.TotalMonthlySavings) + breakdown, breakdownErr := client.QueryBreakdown(ctx, nil, scope, now.Add(-time.Hour), now.Add(time.Hour), "service") + require.NoError(t, breakdownErr) + if len(tc.costs) > 0 { + require.Len(t, points, 1) + assert.Equal(t, tc.want, points[0].TotalUpfront) + assert.Equal(t, tc.want, breakdown["ec2"].TotalUpfront) + } + }) + } + for _, acrossDays := range []bool{false, true} { + account := fmt.Sprintf("groups-%t", acrossDays) + for i, service := range []string{"ec2", "rds"} { + record := *legacy + record.AccountID, record.PurchaseID, record.Service = account, account+service, service + record.UpfrontCost = new(float64(120)) + if i == 1 { + record.UpfrontCost = nil + if acrossDays { + record.Timestamp = now.Add(-24 * time.Hour) + } + } + require.NoError(t, store.SavePurchaseHistory(ctx, &record)) + } + points, summary, queryErr := client.QueryHistory(ctx, nil, map[string][]string{"aws": {account}}, "aws", now.Add(-48*time.Hour), now.Add(time.Hour), "daily") + require.NoError(t, queryErr) + assert.Nil(t, summary.TotalUpfront) + if !acrossDays { + require.Len(t, points, 1) + assert.Nil(t, points[0].TotalUpfront) + } else { + require.Len(t, points, 2) + } + } + _, err = pool.Exec(ctx, `UPDATE purchase_history SET revocation_in_flight=true WHERE purchase_id='unknown-0'`) + require.NoError(t, err) + inFlight, err := store.GetPurchaseHistoryInFlight(ctx) + require.NoError(t, err) + require.Len(t, inFlight, 1) + assert.Nil(t, inFlight[0].UpfrontCost) + + snapshots := analytics.NewPostgresAnalyticsStore(container.DB) + for i, cost := range []*float64{nil, nil, new(float64), new(float64(12.5))} { + snapshot := analytics.SavingsSnapshot{Timestamp: now, AccountID: fmt.Sprintf("snapshot-%d", i), Provider: "aws", Service: "ec2", Region: "us-east-1", CommitmentType: "RI", TotalCommitment: cost, TotalSavings: 10} + if i == 0 { + require.NoError(t, snapshots.SaveSnapshot(ctx, &snapshot)) + } else { + require.NoError(t, snapshots.BulkInsertSnapshots(ctx, []analytics.SavingsSnapshot{snapshot})) + } + loadedSnapshots, queryErr := snapshots.QuerySavings(ctx, analytics.QueryRequest{StartDate: now.Add(-time.Hour), EndDate: now.Add(time.Hour), AccountExternalIDsByProvider: map[string][]string{"aws": {snapshot.AccountID}}}) + require.NoError(t, queryErr) + require.Len(t, loadedSnapshots, 1) + assert.Equal(t, cost, loadedSnapshots[0].TotalCommitment) + body, marshalErr := json.Marshal(loadedSnapshots[0]) + require.NoError(t, marshalErr) + if cost == nil { + assert.Contains(t, string(body), `"total_commitment":null`) + } + } + _, err = pool.Exec(ctx, `INSERT INTO purchase_history (account_id,purchase_id,timestamp,provider,service,region,resource_type,term,payment) VALUES ('omitted','omitted',NOW(),'aws','ec2','us-east-1','m5.large',1,'all-upfront')`) + require.NoError(t, err) + var omitted *float64 + require.NoError(t, pool.QueryRow(ctx, `SELECT upfront_cost::float8 FROM purchase_history WHERE purchase_id='omitted'`).Scan(&omitted)) + assert.Nil(t, omitted) + + down, err := os.ReadFile(filepath.Join(path, "000101_nullable_purchase_upfront_cost.down.sql")) + require.NoError(t, err) + conn, err := pool.Acquire(ctx) + require.NoError(t, err) + defer conn.Release() + _, err = conn.Exec(ctx, string(down)) + require.Error(t, err, "rollback must refuse to replace unknown costs with zero") + _, err = conn.Exec(ctx, "ROLLBACK") + require.NoError(t, err) + var unknownCount int + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM purchase_history WHERE upfront_cost IS NULL`).Scan(&unknownCount)) + assert.Equal(t, 5, unknownCount) +} diff --git a/internal/api/nullable_purchase_cost_test.go b/internal/api/nullable_purchase_cost_test.go new file mode 100644 index 000000000..d398c0cc7 --- /dev/null +++ b/internal/api/nullable_purchase_cost_test.go @@ -0,0 +1,137 @@ +package api + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestPurchaseHistoryUnknownUpfront(t *testing.T) { + for _, costs := range [][]*float64{{nil}, {nil, new(float64(120))}, {new(float64(120)), nil}} { + rows := make([]config.PurchaseHistoryRecord, len(costs)) + for i, cost := range costs { + rows[i] = config.PurchaseHistoryRecord{UpfrontCost: cost, EstimatedSavings: 10} + } + summary := summarizePurchaseHistory(rows) + assert.Nil(t, summary.TotalUpfront) + assert.Equal(t, len(costs), summary.TotalCompleted) + assert.Equal(t, 10*float64(len(costs)), summary.TotalMonthlySavings) + } + for _, rows := range [][]config.PurchaseHistoryRecord{nil, {{UpfrontCost: new(float64)}}, {{Status: "pending"}}} { + summary := summarizePurchaseHistory(rows) + require.NotNil(t, summary.TotalUpfront) + assert.Zero(t, *summary.TotalUpfront) + } +} + +func TestCoverageUnknownUpfront(t *testing.T) { + assert.Nil(t, commitmentCoveredMonthly(config.PurchaseHistoryRecord{Term: 1, MonthlyCost: new(float64(50))})) + known := commitmentCoveredMonthly(config.PurchaseHistoryRecord{Term: 1, UpfrontCost: new(float64), MonthlyCost: new(float64(50))}) + require.NotNil(t, known) + assert.Equal(t, 50.0, *known) + + for _, unknownService := range []string{"a", "z"} { + resp := buildCoverageBreakdown(map[string]*float64{ + "aws:" + unknownService: nil, + "aws:m": new(float64(50)), + "azure:compute": new(float64(50)), + }, map[string]float64{"gcp:compute": 100, "azure:compute": 50}) + for _, provider := range resp.Providers { + switch provider.Provider { + case "aws": + assert.Nil(t, provider.OverallCoveragePct) + case "azure": + require.NotNil(t, provider.OverallCoveragePct) + assert.Equal(t, 50.0, *provider.OverallCoveragePct) + case "gcp": + require.NotNil(t, provider.OverallCoveragePct) + assert.Zero(t, *provider.OverallCoveragePct) + require.Len(t, provider.Services, 1) + require.NotNil(t, provider.Services[0].CoveredMonthly) + assert.Zero(t, *provider.Services[0].CoveredMonthly) + } + } + } +} + +func TestCoverageHandlerUnknownUpfrontSameService(t *testing.T) { + for _, unknownFirst := range []bool{false, true} { + name := "known first" + if unknownFirst { + name = "unknown first" + } + t.Run(name, func(t *testing.T) { + ctx := context.Background() + store, scheduler := &MockConfigStore{}, &MockScheduler{} + known := config.PurchaseHistoryRecord{AccountID: "acc-1", Provider: "aws", Service: "ec2", Timestamp: time.Now().Add(-time.Hour), Term: 1, UpfrontCost: new(float64(120)), MonthlyCost: new(float64(50))} + unknown := known + unknown.UpfrontCost = nil + purchases := []config.PurchaseHistoryRecord{known, unknown} + if unknownFirst { + purchases[0], purchases[1] = purchases[1], purchases[0] + } + store.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)).Return(purchases, nil) + store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{}, nil + } + scheduler.On("ListRecommendations", ctx, config.RecommendationFilter{}).Return([]config.RecommendationRecord{{Provider: "aws", Service: "ec2", Savings: 100}}, nil) + auth, req := adminInventoryReq(ctx) + handler := &Handler{auth: auth, config: store, scheduler: scheduler} + result, err := handler.getCoverageBreakdown(ctx, req, map[string]string{}) + require.NoError(t, err) + resp, ok := result.(CoverageBreakdownResponse) + require.True(t, ok) + require.Len(t, resp.Providers, 3) + aws := resp.Providers[0] + require.Equal(t, "aws", aws.Provider) + require.Len(t, aws.Services, 1) + assert.Nil(t, aws.Services[0].CoveredMonthly) + assert.Nil(t, aws.Services[0].CoveragePct) + assert.Equal(t, 100.0, aws.Services[0].OnDemandMonthly) + assert.Nil(t, aws.OverallCoveragePct) + store.AssertExpectations(t) + scheduler.AssertExpectations(t) + }) + } +} + +func TestMarketplaceUnknownUpfront(t *testing.T) { + for _, tc := range []struct { + name, body string + cost *float64 + allowed bool + }{ + {name: "unknown default"}, + {name: "unknown valid custom", body: `{"price_schedule":[{"term_months":1,"price":100}]}`}, + {name: "zero valid custom", body: `{"price_schedule":[{"term_months":1,"price":100}]}`, cost: new(float64), allowed: true}, + } { + t.Run(tc.name, func(t *testing.T) { + store, authSvc, ec2 := &MockConfigStore{}, &MockAuthService{}, &stubMarketplaceEC2{} + adminSession(authSvc) + row := standardRow() + row.UpfrontCost = tc.cost + store.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID).Return(row, nil) + if tc.allowed { + store.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID).Return(true, nil) + store.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-default", config.ListingStateActive).Return(nil) + } + req := marketplaceReq() + req.Body = tc.body + _, err := newMarketplaceHandler(store, authSvc, ec2).marketplaceList(context.Background(), req, validMarketplacePurchaseID) + if tc.allowed { + require.NoError(t, err) + assert.Equal(t, 1, ec2.createCallCount) + } else { + require.ErrorContains(t, err, "upfront cost is unknown") + assert.Zero(t, ec2.createCallCount) + store.AssertNotCalled(t, "ClaimMarketplaceListingSlot", mock.Anything, mock.Anything) + } + store.AssertExpectations(t) + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index ab550fe11..7ebb15675 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3058,6 +3058,7 @@ components: total_upfront: type: number format: double + nullable: true total_monthly_savings: type: number format: double @@ -3092,6 +3093,7 @@ components: upfront_cost: type: number format: double + nullable: true monthly_cost: type: number format: double @@ -3135,6 +3137,7 @@ components: total_upfront: type: number format: double + nullable: true purchase_count: type: integer cumulative_savings: @@ -3176,6 +3179,7 @@ components: total_upfront: type: number format: double + nullable: true purchase_count: type: integer percentage: diff --git a/internal/api/purchase_pricing.go b/internal/api/purchase_pricing.go index d8d25c3eb..7d76a764a 100644 --- a/internal/api/purchase_pricing.go +++ b/internal/api/purchase_pricing.go @@ -307,6 +307,13 @@ func databaseDetailMismatch(o, m *common.DatabaseDetails) string { return mismatchField("az_config", o.AZConfig, m.AZConfig) } +func sumKnownCosts(a, b *float64) *float64 { + if a == nil || b == nil { + return nil + } + return new(*a + *b) +} + func scaledCost(v *float64, ratio float64) *float64 { if v == nil { return nil diff --git a/internal/api/types.go b/internal/api/types.go index e5a2ec56e..7dfc9e4e2 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -103,7 +103,7 @@ type HistoryDataPoint struct { ByService map[string]float64 `json:"by_service,omitempty"` ByProvider map[string]float64 `json:"by_provider,omitempty"` TotalSavings float64 `json:"total_savings"` - TotalUpfront float64 `json:"total_upfront"` + TotalUpfront *float64 `json:"total_upfront"` PurchaseCount int `json:"purchase_count"` CumulativeSavings float64 `json:"cumulative_savings"` } @@ -119,10 +119,10 @@ type HistorySummaryAnalytics struct { // BreakdownValue represents savings breakdown by dimension. type BreakdownValue struct { - TotalSavings float64 `json:"total_savings"` - TotalUpfront float64 `json:"total_upfront"` - PurchaseCount int `json:"purchase_count"` - Percentage float64 `json:"percentage"` + TotalSavings float64 `json:"total_savings"` + TotalUpfront *float64 `json:"total_upfront"` + PurchaseCount int `json:"purchase_count"` + Percentage float64 `json:"percentage"` } // PurchaseManagerInterface defines purchase manager methods used by handler. @@ -650,7 +650,7 @@ type InventoryCommitment struct { ID string `json:"id"` PaymentOption string `json:"payment_option,omitempty"` TermYears int `json:"term_years"` - UpfrontCost float64 `json:"upfront_cost"` + UpfrontCost *float64 `json:"upfront_cost"` EstimatedSavings float64 `json:"estimated_savings"` Count int `json:"count"` } @@ -673,7 +673,7 @@ type InventoryCommitmentsResponse struct { type CoverageServiceRow struct { CoveragePct *float64 `json:"coverage_pct"` Service string `json:"service"` - CoveredMonthly float64 `json:"covered_monthly"` + CoveredMonthly *float64 `json:"covered_monthly"` OnDemandMonthly float64 `json:"on_demand_monthly"` } @@ -932,8 +932,8 @@ type HistorySummary struct { // TotalRevoked counts commitments that have been revoked/refunded via // MarkPurchaseRevoked. They are excluded from TotalCompleted and all dollar // totals because the provider has canceled the commitment. - TotalRevoked int `json:"total_revoked"` - TotalUpfront float64 `json:"total_upfront"` - TotalMonthlySavings float64 `json:"total_monthly_savings"` - TotalAnnualSavings float64 `json:"total_annual_savings"` + TotalRevoked int `json:"total_revoked"` + TotalUpfront *float64 `json:"total_upfront"` + TotalMonthlySavings float64 `json:"total_monthly_savings"` + TotalAnnualSavings float64 `json:"total_annual_savings"` } diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 0ef8b6969..6aa651cf2 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -2311,6 +2311,7 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, // cyclomatic complexity under the gocyclo budget as columns accrue across // issues #290 (revocation) and #292 (marketplace). type purchaseHistoryNullables struct { + upfrontCost sql.NullFloat64 monthlyCost sql.NullFloat64 planID sql.NullString planName sql.NullString @@ -2329,6 +2330,7 @@ type purchaseHistoryNullables struct { // an explicit $0 recurring charge); the revocation-window timestamps map // directly onto their pointer fields. func (n *purchaseHistoryNullables) applyTo(record *PurchaseHistoryRecord) { + record.UpfrontCost = nullPtrFromNullFloat64(n.upfrontCost) if n.monthlyCost.Valid { v := n.monthlyCost.Float64 record.MonthlyCost = &v @@ -2376,7 +2378,7 @@ func scanPurchaseHistoryRow(rows pgx.Rows) (PurchaseHistoryRecord, error) { &record.Count, &record.Term, &record.Payment, - &record.UpfrontCost, + &n.upfrontCost, &n.monthlyCost, &record.EstimatedSavings, &n.planID, @@ -2444,7 +2446,7 @@ func (s *PostgresStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purc &r.Count, &r.Term, &r.Payment, - &r.UpfrontCost, + &n.upfrontCost, &r.MonthlyCost, &r.EstimatedSavings, &n.planID, @@ -2578,6 +2580,7 @@ func (s *PostgresStore) GetPurchaseHistoryInFlight(ctx context.Context) ([]*Purc var planID, planName, cloudAccountID sql.NullString var revocationWindowClosesAt, revokedAt *time.Time var revokedVia, supportCaseID sql.NullString + var upfrontCost sql.NullFloat64 if err := rows.Scan( &r.AccountID, @@ -2590,7 +2593,7 @@ func (s *PostgresStore) GetPurchaseHistoryInFlight(ctx context.Context) ([]*Purc &r.Count, &r.Term, &r.Payment, - &r.UpfrontCost, + &upfrontCost, &r.MonthlyCost, &r.EstimatedSavings, &planID, @@ -2604,6 +2607,7 @@ func (s *PostgresStore) GetPurchaseHistoryInFlight(ctx context.Context) ([]*Purc ); err != nil { return nil, fmt.Errorf("GetPurchaseHistoryInFlight scan: %w", err) } + r.UpfrontCost = nullPtrFromNullFloat64(upfrontCost) if planID.Valid { r.PlanID = planID.String @@ -3847,6 +3851,13 @@ func nullPtrFromNullString(ns sql.NullString) *string { return &s } +func nullPtrFromNullFloat64(n sql.NullFloat64) *float64 { + if !n.Valid { + return nil + } + return &n.Float64 +} + // ========================================== // NOTIFICATION MUTES (issue #297) // ========================================== diff --git a/internal/config/store_postgres_additional_test.go b/internal/config/store_postgres_additional_test.go index 6b8c640dd..5858db136 100644 --- a/internal/config/store_postgres_additional_test.go +++ b/internal/config/store_postgres_additional_test.go @@ -125,7 +125,7 @@ func (s *additionalMockStore) queryPurchaseHistory(ctx context.Context, query st records := make([]PurchaseHistoryRecord, 0) for rows.Next() { var record PurchaseHistoryRecord - var monthlyCost sql.NullFloat64 + var monthlyCost, upfrontCost sql.NullFloat64 var planID, planName sql.NullString err := rows.Scan( @@ -139,7 +139,7 @@ func (s *additionalMockStore) queryPurchaseHistory(ctx context.Context, query st &record.Count, &record.Term, &record.Payment, - &record.UpfrontCost, + &upfrontCost, &monthlyCost, &record.EstimatedSavings, &planID, @@ -150,6 +150,9 @@ func (s *additionalMockStore) queryPurchaseHistory(ctx context.Context, query st return nil, err } + if upfrontCost.Valid { + record.UpfrontCost = &upfrontCost.Float64 + } if monthlyCost.Valid { v := monthlyCost.Float64 record.MonthlyCost = &v diff --git a/internal/config/store_postgres_comprehensive_test.go b/internal/config/store_postgres_comprehensive_test.go index d936a1f61..eec923de8 100644 --- a/internal/config/store_postgres_comprehensive_test.go +++ b/internal/config/store_postgres_comprehensive_test.go @@ -415,7 +415,7 @@ func (s *mockablePostgresStore) queryPurchaseHistory(ctx context.Context, query for rows.Next() { var record PurchaseHistoryRecord var planID, planName sql.NullString - var monthlyCost sql.NullFloat64 + var monthlyCost, upfrontCost sql.NullFloat64 err := rows.Scan( &record.AccountID, @@ -428,7 +428,7 @@ func (s *mockablePostgresStore) queryPurchaseHistory(ctx context.Context, query &record.Count, &record.Term, &record.Payment, - &record.UpfrontCost, + &upfrontCost, &monthlyCost, &record.EstimatedSavings, &planID, @@ -439,6 +439,9 @@ func (s *mockablePostgresStore) queryPurchaseHistory(ctx context.Context, query return nil, err } + if upfrontCost.Valid { + record.UpfrontCost = &upfrontCost.Float64 + } if monthlyCost.Valid { v := monthlyCost.Float64 record.MonthlyCost = &v @@ -1288,7 +1291,7 @@ func TestSavePurchaseHistory_Success(t *testing.T) { Count: 3, Term: 3, Payment: "all-upfront", - UpfrontCost: 2250.00, + UpfrontCost: new(float64(2250.00)), MonthlyCost: pf(0), EstimatedSavings: 450.00, PlanID: "plan-123", @@ -1985,7 +1988,7 @@ func TestQueryPurchaseHistory_AllFieldsPresent(t *testing.T) { assert.Equal(t, 2, record.Count) assert.Equal(t, 3, record.Term) assert.Equal(t, "all-upfront", record.Payment) - assert.Equal(t, 3000.0, record.UpfrontCost) + assert.Equal(t, 3000.0, *record.UpfrontCost) require.NotNil(t, record.MonthlyCost, "monthly_cost 0.0 from DB must scan as non-nil pointer") assert.Equal(t, 0.0, *record.MonthlyCost) assert.Equal(t, 600.0, record.EstimatedSavings) diff --git a/internal/config/store_postgres_coverage_test.go b/internal/config/store_postgres_coverage_test.go index 2be18fdd0..95c9b494f 100644 --- a/internal/config/store_postgres_coverage_test.go +++ b/internal/config/store_postgres_coverage_test.go @@ -493,7 +493,7 @@ func TestPostgresStore_SavePurchaseHistory_NilDB(t *testing.T) { Count: 3, Term: 3, Payment: "all-upfront", - UpfrontCost: 2250.00, + UpfrontCost: new(float64(2250.00)), MonthlyCost: pf(0), EstimatedSavings: 450.00, PlanID: "plan-123", diff --git a/internal/config/store_postgres_db_test.go b/internal/config/store_postgres_db_test.go index 65c78a3f9..ede56a0a3 100644 --- a/internal/config/store_postgres_db_test.go +++ b/internal/config/store_postgres_db_test.go @@ -704,7 +704,7 @@ func TestPostgresStoreDB_PurchaseHistory(t *testing.T) { Count: 3, Term: 3, Payment: "all-upfront", - UpfrontCost: 2250.00, + UpfrontCost: new(float64(2250.00)), MonthlyCost: pf(0), EstimatedSavings: 450.00, PlanID: "", @@ -751,7 +751,7 @@ func TestPostgresStoreDB_PurchaseHistory(t *testing.T) { Count: 1, Term: 1, Payment: "all-upfront", - UpfrontCost: 1200.00, + UpfrontCost: new(float64(1200.00)), MonthlyCost: nil, EstimatedSavings: 240.00, } diff --git a/internal/config/store_postgres_test.go b/internal/config/store_postgres_test.go index b57a7c322..c287a861c 100644 --- a/internal/config/store_postgres_test.go +++ b/internal/config/store_postgres_test.go @@ -350,7 +350,7 @@ func TestPostgresStore_PurchaseHistory(t *testing.T) { Count: 3, Term: 3, Payment: "all-upfront", - UpfrontCost: 2250.00, + UpfrontCost: new(float64(2250.00)), MonthlyCost: func() *float64 { v := 0.0; return &v }(), EstimatedSavings: 450.00, // PlanID intentionally left empty since it needs to be a valid UUID diff --git a/internal/config/types.go b/internal/config/types.go index 3955887a4..d5e1fab78 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -796,7 +796,7 @@ type PurchaseHistoryRecord struct { Count int `json:"count" dynamodbav:"count"` Term int `json:"term" dynamodbav:"term"` Payment string `json:"payment" dynamodbav:"payment"` - UpfrontCost float64 `json:"upfront_cost" dynamodbav:"upfront_cost"` + UpfrontCost *float64 `json:"upfront_cost" dynamodbav:"upfront_cost"` // MonthlyCost is nil when the provider API did not return a monthly // recurring breakdown for this commitment (e.g. Azure all-upfront where // no recurring charge exists at the commitment layer). GCP commitments diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 6e1f5c47c..19bdd90c4 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -335,7 +335,7 @@ func TestPurchaseHistoryRecord_Fields(t *testing.T) { Count: 5, Term: 1, Payment: "no-upfront", - UpfrontCost: 0, + UpfrontCost: new(float64(0)), MonthlyCost: pf(150.00), EstimatedSavings: 50.00, PlanID: "plan-123", @@ -353,7 +353,7 @@ func TestPurchaseHistoryRecord_Fields(t *testing.T) { assert.Equal(t, 5, rec.Count) assert.Equal(t, 1, rec.Term) assert.Equal(t, "no-upfront", rec.Payment) - assert.Equal(t, float64(0), rec.UpfrontCost) + assert.Equal(t, float64(0), *rec.UpfrontCost) require.NotNil(t, rec.MonthlyCost) assert.Equal(t, 150.00, *rec.MonthlyCost) assert.Equal(t, 50.00, rec.EstimatedSavings) diff --git a/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go index 6502751d7..e2483280c 100644 --- a/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go +++ b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go @@ -59,7 +59,7 @@ func TestAnalyticsNestedRollup_COR02(t *testing.T) { Service: "rds", Region: region, CommitmentType: commitmentType, - TotalCommitment: savings * 10, + TotalCommitment: new(float64(savings * 10)), TotalSavings: savings, } } diff --git a/internal/database/postgres/migrations/000101_nullable_purchase_upfront_cost.down.sql b/internal/database/postgres/migrations/000101_nullable_purchase_upfront_cost.down.sql new file mode 100644 index 000000000..0b719d230 --- /dev/null +++ b/internal/database/postgres/migrations/000101_nullable_purchase_upfront_cost.down.sql @@ -0,0 +1,9 @@ +BEGIN; + +-- SET NOT NULL refuses rollback if unknown costs exist; never fabricate zeros. +ALTER TABLE purchase_history ALTER COLUMN upfront_cost SET NOT NULL; +ALTER TABLE savings_snapshots ALTER COLUMN total_commitment SET NOT NULL; +ALTER TABLE purchase_history ALTER COLUMN upfront_cost SET DEFAULT 0.00; +ALTER TABLE savings_snapshots ALTER COLUMN total_commitment SET DEFAULT 0.00; + +COMMIT; diff --git a/internal/database/postgres/migrations/000101_nullable_purchase_upfront_cost.up.sql b/internal/database/postgres/migrations/000101_nullable_purchase_upfront_cost.up.sql new file mode 100644 index 000000000..1141ede13 --- /dev/null +++ b/internal/database/postgres/migrations/000101_nullable_purchase_upfront_cost.up.sql @@ -0,0 +1,7 @@ +ALTER TABLE purchase_history ALTER COLUMN upfront_cost DROP NOT NULL; +ALTER TABLE purchase_history ALTER COLUMN upfront_cost DROP DEFAULT; +ALTER TABLE savings_snapshots ALTER COLUMN total_commitment DROP NOT NULL; +ALTER TABLE savings_snapshots ALTER COLUMN total_commitment DROP DEFAULT; + +-- Existing view commitment columns remain known-sample statistics, not complete totals. +-- Runtime APIs read nullable snapshots or aggregate purchase history directly. diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 716aeb4dc..cca8433d0 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1138,8 +1138,12 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen return result, fmt.Errorf("purchase was not successful") } - logging.Infof("purchase[%s]: %s/%s/%s/%s PurchaseCommitment succeeded in %s (commitmentID=%s, cost=%.2f)", - opts.ExecutionID, rec.Provider, rec.Service, rec.Region, rec.ResourceType, elapsed, result.CommitmentID, result.Cost) + cost := "unknown" + if result.Cost != nil { + cost = fmt.Sprintf("%.2f", *result.Cost) + } + logging.Infof("purchase[%s]: %s/%s/%s/%s PurchaseCommitment succeeded in %s (commitmentID=%s, cost=%s)", + opts.ExecutionID, rec.Provider, rec.Service, rec.Region, rec.ResourceType, elapsed, result.CommitmentID, cost) return result, nil } diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 6acb5f678..3a29c5487 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -1,6 +1,7 @@ package purchase import ( + "bytes" "context" "errors" "fmt" @@ -9,6 +10,7 @@ import ( "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/aws/aws-sdk-go-v2/aws" @@ -37,82 +39,102 @@ func hasPerRecDeadline(max time.Duration) func(context.Context) bool { } func TestManager_ExecutePurchase(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - mockSTS := new(MockSTSClient) - mockFactory := new(MockProviderFactory) - mockProviderInst := new(MockProvider) - mockServiceClient := new(MockServiceClient) + for _, tc := range []struct { + name string + cost *float64 + logged string + }{ + {"unknown", nil, "cost=unknown"}, + {"zero", new(float64), "cost=0.00"}, + {"known", new(float64(125.5)), "cost=125.50"}, + } { + t.Run(tc.name, func(t *testing.T) { + var logs bytes.Buffer + previousOutput := logging.SetOutput(&logs) + defer logging.SetOutput(previousOutput) + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockSTS := new(MockSTSClient) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) - plan := &config.PurchasePlan{ - ID: "plan-123", - Name: "Test Plan", - } + plan := &config.PurchasePlan{ + ID: "plan-123", + Name: "Test Plan", + } - exec := &config.PurchaseExecution{ - ExecutionID: "exec-123", - PlanID: "plan-123", - StepNumber: 1, - Recommendations: []config.RecommendationRecord{ - { - Provider: "aws", - Service: "ec2", - ResourceType: "m5.large", - Region: "us-east-1", - Count: 5, - Savings: 100.0, - UpfrontCost: 500.0, - Selected: true, - }, - { - Provider: "aws", - Service: "rds", - ResourceType: "db.r5.large", - Region: "us-west-2", - Count: 2, - Savings: 50.0, - UpfrontCost: 200.0, - Selected: false, // Not selected - }, - }, - } + exec := &config.PurchaseExecution{ + ExecutionID: "exec-123", + PlanID: "plan-123", + StepNumber: 1, + Recommendations: []config.RecommendationRecord{ + { + Provider: "aws", + Service: "ec2", + ResourceType: "m5.large", + Region: "us-east-1", + Count: 5, + Savings: 100.0, + UpfrontCost: 500.0, + Selected: true, + }, + { + Provider: "aws", + Service: "rds", + ResourceType: "db.r5.large", + Region: "us-west-2", + Count: 2, + Savings: 50.0, + UpfrontCost: 200.0, + Selected: false, // Not selected + }, + }, + } - mockStore.On("GetPurchasePlan", ctx, "plan-123").Return(plan, nil) - mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) - mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) - mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{ - Account: aws.String("123456789012"), - }, nil) + mockStore.On("GetPurchasePlan", ctx, "plan-123").Return(plan, nil) + mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Run(func(args mock.Arguments) { + assert.Equal(t, tc.cost, args.Get(1).(*config.PurchaseHistoryRecord).UpfrontCost) + }).Return(nil) + mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) + mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{ + Account: aws.String("123456789012"), + }, nil) - // Mock provider factory to return a mock provider - mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), "aws", mock.Anything).Return(mockProviderInst, nil) - mockProviderInst.On("GetServiceClient", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) - mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ - Success: true, - CommitmentID: "ri-12345", - }, nil) + // Mock provider factory to return a mock provider + mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), "aws", mock.Anything).Return(mockProviderInst, nil) + mockProviderInst.On("GetServiceClient", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) + mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ + Success: true, + CommitmentID: "ri-12345", + Cost: tc.cost, + }, nil) - manager := &Manager{ - config: mockStore, - email: mockEmail, - stsClient: mockSTS, - providerFactory: mockFactory, - dashboardURL: "https://dashboard.example.com", - } + manager := &Manager{ + config: mockStore, + email: mockEmail, + stsClient: mockSTS, + providerFactory: mockFactory, + dashboardURL: "https://dashboard.example.com", + } - err := manager.executePurchase(ctx, exec) - require.NoError(t, err) + err := manager.executePurchase(ctx, exec) + require.NoError(t, err) - // Verify that only selected recommendation was purchased - assert.True(t, exec.Recommendations[0].Purchased) - assert.NotEmpty(t, exec.Recommendations[0].PurchaseID) - assert.False(t, exec.Recommendations[1].Purchased) + // Verify that only selected recommendation was purchased + assert.True(t, exec.Recommendations[0].Purchased) + assert.NotEmpty(t, exec.Recommendations[0].PurchaseID) + assert.False(t, exec.Recommendations[1].Purchased) - mockStore.AssertExpectations(t) - mockEmail.AssertExpectations(t) - mockSTS.AssertExpectations(t) - mockFactory.AssertExpectations(t) + mockStore.AssertExpectations(t) + mockEmail.AssertExpectations(t) + mockSTS.AssertExpectations(t) + mockFactory.AssertExpectations(t) + assert.Contains(t, logs.String(), tc.logged) + assert.NotContains(t, logs.String(), "%!") + }) + } } // TestManager_ExecutePurchase_WebSourcePropagates covers the gap noted in the