Skip to content

feat(frontend/recs): On-Demand Monthly uses provider on_demand_cost directly (closes #330) #331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cristim wants to merge 2 commits into
base: feat/multicloud-web-frontend
Choose a base branch
from
Open

feat(frontend/recs): On-Demand Monthly uses provider on_demand_cost directly (closes #330) #331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3fb059
feat(frontend/recs): On-Demand Monthly column uses provider on_demand…
cristim May 7, 2026
6244beb
fix(frontend/recs): cover explicit-undefined + freshen filter comment…
cristim May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 51 additions & 48 deletions frontend/src/__tests__/recommendations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3499,6 +3499,9 @@ describe('effectiveSavingsPct', () => {
});

describe('onDemandMonthly', () => {
// #330: onDemandMonthly returns the provider-supplied on_demand_cost
// directly. No reconstruction fallback — when the provider didn't
// populate on_demand_cost, the column renders an em-dash.
const mk = (overrides: Partial<LocalRecommendation>): LocalRecommendation => ({
id: 'r',
provider: 'aws',
Expand All @@ -3513,54 +3516,52 @@ describe('onDemandMonthly', () => {
...overrides,
} as unknown as LocalRecommendation);

test('null monthly_cost returns null (data not provided)', () => {
expect(onDemandMonthly(mk({ monthly_cost: null }))).toBeNull();
expect(onDemandMonthly(mk({ monthly_cost: undefined }))).toBeNull();
test('returns provider on_demand_cost directly when populated', () => {
// The function must return the raw provider value, not any reconstruction.
const odm = onDemandMonthly(mk({
on_demand_cost: 500,
savings: 100,
upfront_cost: 0,
monthly_cost: 50,
term: 1,
}));
expect(odm).toBe(500);
});

test('term=0 returns null (anomaly — cannot amortize over zero months)', () => {
expect(onDemandMonthly(mk({ term: 0, monthly_cost: 50 }))).toBeNull();
test('null on_demand_cost returns null (no reconstruction fallback)', () => {
// Explicit null — common for legacy cached recs that pre-date #277/#312.
expect(onDemandMonthly(mk({ on_demand_cost: null }))).toBeNull();
});

test('no-upfront: on_demand_monthly = monthly_cost + savings', () => {
// upfront_cost = 0 → amortized = 0 → on_demand = 50 + 100 = 150
const odm = onDemandMonthly(mk({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 }));
expect(odm).not.toBeNull();
expect(odm!).toBeCloseTo(150, 2);
test('explicit undefined on_demand_cost returns null', () => {
// TypeScript's strict mode lets callers pass `undefined` rather than
// omitting the field; assert that branch explicitly so a future
// `r.on_demand_cost > 0` guard regression doesn't accept undefined.
expect(onDemandMonthly(mk({ on_demand_cost: undefined }))).toBeNull();
});

test('partial-upfront: on_demand_monthly includes amortized upfront', () => {
// upfront_cost = 600, term = 1 → amortized = 600/12 = 50/mo
// on_demand = 0 + 50 + 50 = 100
const odm = onDemandMonthly(mk({ savings: 50, upfront_cost: 600, monthly_cost: 0, term: 1 }));
expect(odm).not.toBeNull();
expect(odm!).toBeCloseTo(100, 2);
test('missing on_demand_cost field returns null', () => {
// Distinct from the explicit-undefined case: the property never
// existed on the object (e.g. legacy cached recs from before #277).
const r = mk({});
delete (r as { on_demand_cost?: unknown }).on_demand_cost;
expect(onDemandMonthly(r)).toBeNull();
});

test('3yr partial-upfront: on_demand_monthly amortizes upfront over 36 months', () => {
// upfront_cost = 7200, term = 3 → amortized = 7200/36 = 200/mo
// on_demand = 200 + 600 + 200 = 1000
const odm = onDemandMonthly(mk({ savings: 600, upfront_cost: 7200, monthly_cost: 200, term: 3 }));
expect(odm).not.toBeNull();
expect(odm!).toBeCloseTo(1000, 2);
test('on_demand_cost=0 returns null (provider treats 0 as not-populated)', () => {
// Same convention as the on_demand_cost preference branch in
// effectiveSavingsPct: zero means the provider didn't report it.
expect(onDemandMonthly(mk({ on_demand_cost: 0 }))).toBeNull();
});

test('monthly_cost=0 (all-upfront): on_demand_monthly = savings + amortized_upfront', () => {
// monthly_cost = 0 is valid known data (all-upfront commitment)
// upfront_cost = 1200, term = 1 → amortized = 100/mo
// on_demand = 0 + 200 + 100 = 300
const odm = onDemandMonthly(mk({ savings: 200, upfront_cost: 1200, monthly_cost: 0, term: 1 }));
expect(odm).not.toBeNull();
expect(odm!).toBeCloseTo(300, 2);
});

test('identity: on_demand_monthly = monthly_cost + savings + (upfront_cost / (term*12))', () => {
// The value must satisfy the documented formula exactly.
const r = mk({ savings: 120, upfront_cost: 3600, monthly_cost: 80, term: 3 });
const expected = 80 + 120 + (3600 / (3 * 12)); // = 80 + 120 + 100 = 300
const odm = onDemandMonthly(r);
expect(odm).not.toBeNull();
expect(odm!).toBeCloseTo(expected, 5);
test('positive on_demand_cost wins regardless of monthly_cost / term / upfront', () => {
// Whatever the supporting fields say, the provider value is authoritative.
expect(onDemandMonthly(mk({
on_demand_cost: 1500,
monthly_cost: null,
term: 0,
upfront_cost: 99999,
}))).toBe(1500);
});
});

Expand Down Expand Up @@ -3705,10 +3706,9 @@ describe('Monthly Cost + Effective % column rendering', () => {
);
});

test('On-Demand Monthly column renders formatCurrency when monthly_cost is non-null', async () => {
// savings=100, upfront_cost=0, monthly_cost=50, term=1
// on_demand_monthly = 50 + 100 + 0 = 150 → formatCurrency → "$150"
const rec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 });
test('On-Demand Monthly column renders formatCurrency from provider on_demand_cost (#330)', async () => {
// #330 — column shows the raw provider value, not a reconstruction.
const rec = baseRec({ on_demand_cost: 150, savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 });
(api.getRecommendations as jest.Mock).mockResolvedValue({
summary: {},
recommendations: [rec],
Expand All @@ -3723,14 +3723,16 @@ describe('Monthly Cost + Effective % column rendering', () => {
// Pin the specific on_demand_monthly cell by column index (not a scan of all tds).
const odmColIdx = Array.from(document.querySelectorAll('th'))
.findIndex((th) => th.getAttribute('data-sort') === 'on_demand_monthly');
expect(odmColIdx).toBeGreaterThanOrEqual(0);
const firstRowCells = Array.from(
document.querySelectorAll('tbody tr.recommendation-row td'),
);
expect(firstRowCells[odmColIdx]?.textContent).toBe('$150');
});

test('On-Demand Monthly column renders em-dash when monthly_cost is null', async () => {
const rec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: null, term: 1 });
test('On-Demand Monthly column renders em-dash when on_demand_cost is missing (#330)', async () => {
// #330 — without provider on_demand_cost, no reconstruction; cell is em-dash.
const rec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 });
(api.getRecommendations as jest.Mock).mockResolvedValue({
summary: {},
recommendations: [rec],
Expand All @@ -3748,10 +3750,11 @@ describe('Monthly Cost + Effective % column rendering', () => {
expect(firstRowCells[odmColIdx]?.textContent).toBe('—');
});

test('on_demand_monthly numeric filter: null monthly_cost row does not match = 0', async () => {
// When monthly_cost is null, on_demand_monthly returns null → NaN sentinel in numericCellValue.
// A filter expr "0" (equals zero) must NOT match such rows.
const nullRec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: null, term: 1 });
test('on_demand_monthly numeric filter: row with no on_demand_cost does not match = 0 (#330)', async () => {
// #330 — without provider on_demand_cost, onDemandMonthly returns null,
// which becomes a NaN sentinel in numericCellValue. A filter expr "0"
// (equals zero) must NOT match such rows.
const nullRec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 });
(api.getRecommendations as jest.Mock).mockResolvedValue({
summary: {},
recommendations: [nullRec],
Expand Down
45 changes: 23 additions & 22 deletions frontend/src/recommendations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,11 +402,11 @@ const SORTABLE_NUMERIC_COLUMNS: Record<string, (r: LocalRecommendation) => numbe
const period = state.getCostPeriod();
return scaleCost(r.monthly_cost, period) ?? Number.POSITIVE_INFINITY;
},
// onDemandMonthly returns null for null monthly_cost or term=0.
// POSITIVE_INFINITY places null rows at the bottom in ascending order and
// at the top in descending — consistent with monthly_cost. The base value
// is monthly; scale it to the selected display period so sorting matches
// what the user sees in the column.
// onDemandMonthly returns null when the provider didn't report
// on_demand_cost (or reported 0). POSITIVE_INFINITY is the null sentinel;
// groupsInSortOrder keeps nullish rows last in both ascending and descending
// sorts. The base value is monthly; scale it to the selected display period
// so sorting matches what the user sees in the column.
on_demand_monthly: (r) => {
const period = state.getCostPeriod();
return scaleCost(onDemandMonthly(r), period) ?? Number.POSITIVE_INFINITY;
Expand Down Expand Up @@ -752,23 +752,23 @@ export function effectiveSavingsPct(r: LocalRecommendation): number | null {
}

/**
* Computes the equivalent on-demand monthly cost by reversing the savings
* formula: on_demand_monthly = monthly_cost + savings + (upfront_cost / (term * 12)).
* Returns null when monthly_cost is null (provider didn't return a recurring
* breakdown — same semantics as the Monthly Cost column and effectiveSavingsPct).
* Returns null when term is 0 (anomaly — cannot amortize over zero months).
* Returns the provider-reported on-demand monthly cost (`r.on_demand_cost`)
* directly. Issue #330 — earlier behaviour reconstructed the value from
* `monthly_cost + savings + amortized_upfront`, which drifted from the
* provider's billed price for rounding edge cases (Azure all-upfront RIs,
* AWS Capacity Reservation discounts, partial-day proration). Aligning this
* with `effectiveSavingsPct`'s `hasOnDemand` branch makes the column show
* the same denominator the percentage column uses, so the two never disagree.
*
* Note: this reconstruction uses only monthly_cost + savings + amortized_upfront.
* It does NOT use on_demand_cost even when available, because the column's
* purpose is to display the reconstructed denominator so users can verify the
* formula against the raw fields visible in the same row. For the authoritative
* on-demand baseline used in effectiveSavingsPct, see that function.
* Returns `null` when `on_demand_cost` is missing, undefined, or `0` —
* cell renders `—` (same em-dash convention as the existing Monthly Cost
* column for null `monthly_cost`).
*/
export function onDemandMonthly(r: LocalRecommendation): number | null {
if (r.monthly_cost == null) return null;
if (!r.term) return null;
const amortized = r.upfront_cost / (r.term * 12);
return r.monthly_cost + r.savings + amortized;
if (r.on_demand_cost != null && r.on_demand_cost > 0) {
return r.on_demand_cost;
}
return null;
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -1082,9 +1082,10 @@ function numericCellValue(r: LocalRecommendation, col: state.RecommendationsColu
// Return NaN for null monthly_cost so numeric filter predicates (e.g. "= 0")
// don't match rows where the provider simply didn't report a monthly cost.
case 'monthly_cost': return scaleCost(r.monthly_cost, period) ?? Number.NaN;
// Return NaN for null on_demand_monthly (null monthly_cost or term=0) so any
// numeric predicate returns false rather than coincidentally matching 0.
// Scale to the active period so a numeric filter targets what the user sees.
// Return NaN for null on_demand_monthly (missing or zero on_demand_cost — see
// onDemandMonthly() for the contract) so any numeric predicate returns false
// rather than coincidentally matching 0. Scale to the active period so a
// numeric filter targets what the user sees.
case 'on_demand_monthly': return scaleCost(onDemandMonthly(r), period) ?? Number.NaN;
// Return NaN for null effective_savings_pct so any numeric predicate
// returns false rather than coincidentally matching 0.
Expand Down