Skip to content

Commit c5d0ff6

Browse files
authored
fix(opportunities): align Total Recommendations KPI with Showing-of count (#756)
The KPI counted parent-row services (6) while the "Showing of" line counted variants (23) on the same dataset. Users see big mismatched numbers and assume something is wrong. Both now count variants. Closes #748.
1 parent eda0c6b commit c5d0ff6

2 files changed

Lines changed: 55 additions & 8 deletions

File tree

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

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -336,13 +336,13 @@ describe('Recommendations Module', () => {
336336
// No selection: cards reflect the full visible set.
337337
(state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set());
338338
await loadRecommendations();
339-
expect(cardValue(/Recommendations/)).toBe('2'); // 2 cells, not 4 variants
339+
expect(cardValue(/Recommendations/)).toBe('4'); // 4 variants (closes #748: KPI now matches "Showing of")
340340
expect(cardValue(/Monthly Savings/)).toMatch(/\$300\b/);
341341
expect(cardValue(/Monthly Savings/)).toMatch(/\$400\b/);
342342
expect(cardValue(/Upfront/)).toMatch(/\$0\b/);
343343
expect(cardValue(/Upfront/)).toMatch(/\$3,?000\b/);
344344
// Card titles reflect the "all visible" mode.
345-
expect(cardTitle(/^2$/)).toBe('Total Recommendations');
345+
expect(cardTitle(/^4$/)).toBe('Total Recommendations');
346346

347347
// Tick one cell's pricey variant: cards narrow to that one rec.
348348
(state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['cell1-pricey']));
@@ -362,6 +362,48 @@ describe('Recommendations Module', () => {
362362
expect(cardValue(/Upfront/)).toMatch(/^\$1,?000$/); // 1000 + 0
363363
});
364364

365+
test('KPI "Total Recommendations" and "Showing X of X" agree on the same variant count (closes #748)', async () => {
366+
// 2 cells, 2 variants each = 4 total variants.
367+
// Before #748 the KPI showed 2 (cell count) while "Showing of" showed 4
368+
// (variant count). After the fix both display 4.
369+
const recs = [
370+
{ id: 'cell1-v1', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment_option: 'no-upfront', savings: 100, upfront_cost: 0 },
371+
{ id: 'cell1-v2', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment_option: 'all-upfront', savings: 150, upfront_cost: 1000 },
372+
{ id: 'cell2-v1', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment_option: 'no-upfront', savings: 200, upfront_cost: 0 },
373+
{ id: 'cell2-v2', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment_option: 'all-upfront', savings: 250, upfront_cost: 2000 },
374+
];
375+
(api.getRecommendations as jest.Mock).mockResolvedValue({
376+
summary: { total_count: 4, total_monthly_savings: 700, total_upfront_cost: 3000, avg_payback_months: 2 },
377+
recommendations: recs,
378+
regions: [],
379+
});
380+
(state.getRecommendations as jest.Mock).mockReturnValue(recs);
381+
(state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs);
382+
(state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({});
383+
(state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set());
384+
385+
await loadRecommendations();
386+
387+
// KPI card value.
388+
const summary = document.getElementById('recommendations-summary');
389+
const kpiCard = Array.from(summary?.querySelectorAll('.card') ?? [])
390+
.find((c) => /Recommendations/.test(c.querySelector('h3')?.textContent ?? ''));
391+
const kpiValue = kpiCard?.querySelector('.value')?.textContent ?? '';
392+
393+
// "Showing X of X" live region value.
394+
const liveText = document.querySelector('.recommendations-filter-live')?.textContent ?? '';
395+
const match = liveText.match(/Showing (\d+) of (\d+)/);
396+
const showingVisible = match?.[1] ?? '';
397+
const showingLoaded = match?.[2] ?? '';
398+
399+
// Both KPI and "Showing of" must report the same variant count (4).
400+
expect(kpiValue).toBe('4');
401+
expect(showingLoaded).toBe('4');
402+
expect(kpiValue).toBe(showingLoaded);
403+
// Parity between visible and loaded when no filter is active.
404+
expect(showingVisible).toBe(showingLoaded);
405+
});
406+
365407
test('row checkbox change event updates summary cards in place (PR #283 CR pass-2)', async () => {
366408
// Regression guard for the real DOM event path: verifies that dispatching
367409
// a `change` event on a row checkbox updates summary cards WITHOUT a

‎frontend/src/recommendations.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -492,17 +492,22 @@ function renderRecommendationsSummary(
492492
const period = state.getCostPeriod();
493493
const scaledSavingsMin = scaleCost(plr.savingsMin, period) ?? 0;
494494
const scaledSavingsMax = scaleCost(plr.savingsMax, period) ?? 0;
495-
const savingsText = plr.cellCount > 0 && plr.savingsMax > 0
495+
// Use target.length (variant count) for the guard conditions so they
496+
// stay consistent with the new KPI value below (closes #748).
497+
const hasRecs = target.length > 0;
498+
const savingsText = hasRecs && plr.savingsMax > 0
496499
? formatScaledRange(scaledSavingsMin, scaledSavingsMax, period)
497500
: formatCostForPeriod(0, period);
498-
const upfrontText = plr.cellCount > 0 && plr.upfrontMax > 0
501+
const upfrontText = hasRecs && plr.upfrontMax > 0
499502
? formatSavingsRange(plr.upfrontMin, plr.upfrontMax)
500503
: formatCurrency(0);
501-
const paybackText = plr.cellCount > 0 && plr.paybackMonthsMax > 0
504+
const paybackText = hasRecs && plr.paybackMonthsMax > 0
502505
? formatPaybackRange(plr.paybackMonthsMin, plr.paybackMonthsMax)
503506
: '0 months';
504-
// Total Recommendations counts CELLS not variants — each cell is the
505-
// user's actual decision unit.
507+
// issue #748: count VARIANTS (= target.length), not cells (= plr.cellCount).
508+
// "Showing X of X" in the filter-status bar also counts variants, so both
509+
// numbers now agree on the same dataset.
510+
const variantCount = target.length;
506511
const countLabel = isSelectionView ? 'Selected Recommendations' : 'Total Recommendations';
507512
const savingsLabel = (() => {
508513
if (period === 'monthly') {
@@ -517,7 +522,7 @@ function renderRecommendationsSummary(
517522
container.innerHTML = `
518523
<div class="card">
519524
<h3>${countLabel}</h3>
520-
<p class="value">${plr.cellCount}</p>
525+
<p class="value">${variantCount}</p>
521526
</div>
522527
<div class="card">
523528
<h3>${savingsLabel}</h3>

0 commit comments

Comments
 (0)