Skip to content

Commit ab0ce9c

Browse files
committed
feat(recommendations): editable per-row Term/Payment in single-bucket purchase modal (issue #111 sub-option iii)
Implements the per-row counterpart of issue #111's user-approved design ("default override pre-populated at purchase time, allowing user to select other options if desired"). Sub-option (ii) — per-bucket Payment in the fan-out modal — landed in the prior commit. This commit handles the OTHER purchase entry point: the single-bucket `openPurchaseModal`, which opens when the bulk-purchase selection collapses to one (provider, service, term) bucket. # What changes - `openPurchaseModal` now renders editable Term and Payment dropdowns per row. Defaults walk the precedence: 1. Override: `rec.cloud_account_id` has a saved `AccountServiceOverride` matching `(provider, service)` whose `payment` is supported by `(provider, service, term)` → seed from override; row's source-note span renders "(from account override)". 2. Rec's own payment (the API stamps it at collection time): seed from `rec.payment` if non-empty AND supported. 3. Defensive fallback: `paymentOptionsFor(provider, service, term)[0]`. Reachable only from malformed test fixtures or pre-#111 cached responses where the rec lacks a payment. - Edits mutate `currentPurchaseRecommendations[idx]` in place; `getPurchaseModalRecommendations()` returns the user's choices. `app.ts::handleExecutePurchase` now reads `r.payment` per rec (with a defensive `?? 'all-upfront'` for direct test-harness callers that bypass the modal). This replaces the historical hardcoded `'all-upfront'` on the single-bucket path that silently dropped the toolbar's Payment for every single-bucket purchase since the bulk-purchase toolbar shipped — a pre-existing bug surfaced and fixed by this commit. - Term changes (1yr ↔ 3yr) rebuild only that row's Payment `<select>` options; if the prior Payment is no longer supported for the new term, the first valid option wins and live state is mirrored. The modal does NOT re-render mid-edit so other rows' in-progress edits are preserved. # Implementation notes - `openPurchaseModal` is now `async`. It pre-fetches `listAccountServiceOverrides(id)` once per distinct non-empty `cloud_account_id` in the input set, in parallel via `Promise.all`, and caches the responses in a per-call `Map`. Errors are swallowed — the rec-payment / paymentOptionsFor[0] fallback always works, so a transient API blip shouldn't block a purchase. Same pattern as `openFanOutModal` (ii). - The DOM build switches from a template-literal `innerHTML` rewrite to `createElement` construction so the per-row controls can carry live event listeners. All cell text is `textContent` assignment — no HTML interpolation, no XSS surface. - `LocalRecommendation` gains an optional `payment?: string` field in `frontend/src/types.ts`. The runtime data already carries it (the API `Recommendation` defines `payment: string`), so this is a type-additive change. No mapping changes required. - The override-fetch+cache pattern is duplicated across `openFanOutModal` (ii) and `openPurchaseModal` (iii). Documented in code; follow-up issue will consolidate them into a shared `frontend/src/lib/overrides.ts` helper once both surfaces have shipped (avoiding scope creep on this PR). # Out of scope (deliberately, with follow-up issues) - **Recommendations stay unfiltered.** The recommendations page itself is unchanged — overrides are applied at purchase time, not at listing time. - **`enabled=false`, `coverage`, and include/exclude lists are still decorative.** Only `payment` is consumed (at purchase time, by this PR + ii). For those other override fields to do anything, `ListStoredRecommendations` / `RecommendationFilter` need to become account-aware (option B). Filed as a follow-up. - **Multi-account fan-out buckets still fall back to toolbar.** Same scope discipline as (ii). Filed as a follow-up. # Tests 5 new tests in `frontend/src/__tests__/recommendations.test.ts` inside the `'Issue #111 (iii): per-row Payment seed in openPurchaseModal'` describe block: (a) Single rec, override matches and has supported payment → live state + select value + source-note all reflect override. (b) Single rec, no matching override → seed from rec.payment; no source-note. (c) Single rec, override has unsupported payment for the `(provider, service, term)` cell (AWS RDS 3yr no-upfront, blocked by `cmd/validators.go:warnRDS3YearNoUpfront`) → override ignored; rec.payment wins. (d) User changes Term 1→3 → rec.term updates; row's Payment options rebuilt to the 3yr-supported set; live state consistent with the dropdown. (e) User changes Payment dropdown → live state reflects the new value (which `app.ts::handleExecutePurchase` reads verbatim). Existing `openPurchaseModal` tests (4) updated to `await` the now- async function and read `.textContent` instead of `.innerHTML` (content unchanged, just the rendering mode). All 1371 frontend tests pass (1366 baseline including (ii)'s 4 tests + 5 new). Three clean verification passes (jest + tsc + webpack build) before commit. Go smoke test on `internal/api/...` clean. Closes #111 in combination with the prior (ii) commit.
1 parent cbe11b6 commit ab0ce9c

4 files changed

Lines changed: 483 additions & 48 deletions

File tree

frontend/src/__tests__/recommendations.test.ts

Lines changed: 186 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
22
* Recommendations module tests
33
*/
4-
import { loadRecommendations, openPurchaseModal, refreshRecommendations, setupRecommendationsHandlers, clearRecommendationDetailCache } from '../recommendations';
4+
import { loadRecommendations, openPurchaseModal, getPurchaseModalRecommendations, refreshRecommendations, setupRecommendationsHandlers, clearRecommendationDetailCache } from '../recommendations';
55

66
// Mock the api module
77
jest.mock('../api', () => ({
@@ -391,6 +391,9 @@ describe('Recommendations Module', () => {
391391
const purchaseBtn = document.querySelector('#bulk-purchase-btn') as HTMLButtonElement;
392392
expect(purchaseBtn).not.toBeNull();
393393
purchaseBtn.click();
394+
// Issue #111 (iii): openPurchaseModal is async; flush microtasks
395+
// so the modal-open call lands before we assert visibility.
396+
await Promise.resolve(); await Promise.resolve(); await Promise.resolve();
394397

395398
const modal = document.getElementById('purchase-modal');
396399
expect(modal?.classList.contains('hidden')).toBe(false);
@@ -633,7 +636,10 @@ describe('Recommendations Module', () => {
633636
});
634637

635638
describe('openPurchaseModal', () => {
636-
test('displays purchase modal', () => {
639+
// Issue #111 (iii): openPurchaseModal is now async (it pre-fetches
640+
// per-account service overrides to seed each row's Payment default).
641+
// Tests must `await` it so the DOM is populated before assertions.
642+
test('displays purchase modal', async () => {
637643
const recommendations = [
638644
{ id: 'rec-1', provider: 'aws' as const,
639645
service: 'ec2',
@@ -646,42 +652,42 @@ describe('Recommendations Module', () => {
646652
}
647653
];
648654

649-
openPurchaseModal(recommendations);
655+
await openPurchaseModal(recommendations);
650656

651657
const modal = document.getElementById('purchase-modal');
652658
expect(modal?.classList.contains('hidden')).toBe(false);
653659
});
654660

655-
test('shows purchase summary', () => {
661+
test('shows purchase summary', async () => {
656662
const recommendations = [
657663
{ id: 'rec-2', provider: 'aws' as const, service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 5, term: 1, savings: 100, upfront_cost: 500 },
658664
{ id: 'rec-3', provider: 'aws' as const, service: 'rds', resource_type: 'db.r5.large', region: 'us-east-1', count: 2, term: 1, savings: 200, upfront_cost: 1000 }
659665
];
660666

661-
openPurchaseModal(recommendations);
667+
await openPurchaseModal(recommendations);
662668

663669
const details = document.getElementById('purchase-details');
664-
expect(details?.innerHTML).toContain('2'); // count of commitments
665-
expect(details?.innerHTML).toContain('Purchase Summary');
670+
expect(details?.textContent).toContain('2'); // count of commitments
671+
expect(details?.textContent).toContain('Purchase Summary');
666672
});
667673

668-
test('lists individual recommendations', () => {
674+
test('lists individual recommendations', async () => {
669675
const recommendations = [
670676
{ id: 'rec-4', provider: 'aws' as const, service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 5, term: 1, savings: 100, upfront_cost: 500 }
671677
];
672678

673-
openPurchaseModal(recommendations);
679+
await openPurchaseModal(recommendations);
674680

675681
const details = document.getElementById('purchase-details');
676-
expect(details?.innerHTML).toContain('ec2');
677-
expect(details?.innerHTML).toContain('t3.medium');
678-
expect(details?.innerHTML).toContain('us-east-1');
682+
expect(details?.textContent).toContain('ec2');
683+
expect(details?.textContent).toContain('t3.medium');
684+
expect(details?.textContent).toContain('us-east-1');
679685
});
680686

681-
test('handles missing modal element', () => {
682-
document.body.innerHTML = '';
687+
test('handles missing modal element', async () => {
688+
document.body.replaceChildren();
683689

684-
expect(() => openPurchaseModal([])).not.toThrow();
690+
await expect(openPurchaseModal([])).resolves.not.toThrow();
685691
});
686692
});
687693

@@ -1525,3 +1531,168 @@ describe('Issue #111: per-bucket Payment seed from per-account service override'
15251531
expect(after!.some((b) => b.payment === 'no-upfront')).toBe(true);
15261532
});
15271533
});
1534+
1535+
// Issue #111 (iii): per-row Payment seed in openPurchaseModal — the
1536+
// single-bucket / single-rec purchase modal renders editable Term and
1537+
// Payment dropdowns. Each row's defaults walk the precedence
1538+
// override → rec.payment → paymentOptionsFor[0]. Edits mutate
1539+
// currentPurchaseRecommendations[idx] in place; getPurchaseModalRecommendations()
1540+
// returns the user's choices; app.ts::handleExecutePurchase posts
1541+
// `r.payment` per rec (no longer hardcoded 'all-upfront').
1542+
describe('Issue #111 (iii): per-row Payment seed in openPurchaseModal', () => {
1543+
beforeEach(() => {
1544+
document.body.replaceChildren();
1545+
const purchaseModal = document.createElement('div');
1546+
purchaseModal.id = 'purchase-modal';
1547+
purchaseModal.className = 'hidden';
1548+
const purchaseDetails = document.createElement('div');
1549+
purchaseDetails.id = 'purchase-details';
1550+
purchaseModal.appendChild(purchaseDetails);
1551+
document.body.appendChild(purchaseModal);
1552+
jest.clearAllMocks();
1553+
(api.listAccountServiceOverrides as jest.Mock).mockResolvedValue([]);
1554+
});
1555+
1556+
test('(a) single rec with matching override → row Payment seeded from override; source-note rendered', async () => {
1557+
(api.listAccountServiceOverrides as jest.Mock).mockImplementation(async (id: string) => {
1558+
if (id === 'test-account-a') {
1559+
return [{ id: 'ovr-1', account_id: 'test-account-a', provider: 'aws', service: 'ec2', payment: 'partial-upfront' }];
1560+
}
1561+
return [];
1562+
});
1563+
1564+
const rec = {
1565+
id: 'rec-1', provider: 'aws' as const, cloud_account_id: 'test-account-a',
1566+
service: 'ec2', resource_type: 't3.medium', region: 'us-east-1',
1567+
count: 5, term: 1, payment: 'all-upfront', savings: 100, upfront_cost: 500,
1568+
};
1569+
1570+
await openPurchaseModal([rec]);
1571+
1572+
const live = getPurchaseModalRecommendations();
1573+
expect(live).toHaveLength(1);
1574+
expect(live[0]!.payment).toBe('partial-upfront');
1575+
1576+
const select = document.querySelector<HTMLSelectElement>('.purchase-row-payment');
1577+
expect(select).not.toBeNull();
1578+
expect(select!.value).toBe('partial-upfront');
1579+
1580+
const note = document.querySelector<HTMLElement>('.purchase-row-payment-source');
1581+
expect(note).not.toBeNull();
1582+
expect(note!.textContent).toContain('account override');
1583+
});
1584+
1585+
test('(b) single rec with NO matching override → row Payment seeded from rec.payment; no source-note', async () => {
1586+
// Override exists but for a different service — the lookup misses
1587+
// and the rec's own payment ('partial-upfront') wins.
1588+
(api.listAccountServiceOverrides as jest.Mock).mockImplementation(async (id: string) => {
1589+
if (id === 'test-account-a') {
1590+
return [{ id: 'ovr-1', account_id: 'test-account-a', provider: 'aws', service: 'rds', payment: 'no-upfront' }];
1591+
}
1592+
return [];
1593+
});
1594+
1595+
const rec = {
1596+
id: 'rec-2', provider: 'aws' as const, cloud_account_id: 'test-account-a',
1597+
service: 'ec2', resource_type: 't3.medium', region: 'us-east-1',
1598+
count: 5, term: 1, payment: 'partial-upfront', savings: 100, upfront_cost: 500,
1599+
};
1600+
1601+
await openPurchaseModal([rec]);
1602+
1603+
const live = getPurchaseModalRecommendations();
1604+
expect(live[0]!.payment).toBe('partial-upfront');
1605+
1606+
const select = document.querySelector<HTMLSelectElement>('.purchase-row-payment');
1607+
expect(select!.value).toBe('partial-upfront');
1608+
1609+
const note = document.querySelector('.purchase-row-payment-source');
1610+
expect(note).toBeNull();
1611+
});
1612+
1613+
test('(c) override has unsupported payment for the (provider,service,term) cell → falls back to rec.payment', async () => {
1614+
// AWS RDS 3yr does NOT support no-upfront (per
1615+
// isPaymentSupported / cmd/validators.go:warnRDS3YearNoUpfront).
1616+
// The override pins no-upfront → must be ignored; rec.payment wins.
1617+
(api.listAccountServiceOverrides as jest.Mock).mockImplementation(async (id: string) => {
1618+
if (id === 'test-account-a') {
1619+
return [{ id: 'ovr-1', account_id: 'test-account-a', provider: 'aws', service: 'rds', payment: 'no-upfront' }];
1620+
}
1621+
return [];
1622+
});
1623+
1624+
const rec = {
1625+
id: 'rec-3', provider: 'aws' as const, cloud_account_id: 'test-account-a',
1626+
service: 'rds', resource_type: 'db.r5.large', region: 'us-east-1',
1627+
count: 1, term: 3, payment: 'all-upfront', savings: 200, upfront_cost: 1000,
1628+
};
1629+
1630+
await openPurchaseModal([rec]);
1631+
1632+
const live = getPurchaseModalRecommendations();
1633+
expect(live[0]!.payment).toBe('all-upfront');
1634+
1635+
const note = document.querySelector('.purchase-row-payment-source');
1636+
expect(note).toBeNull();
1637+
});
1638+
1639+
test('(d) user changes Term 1→3 → row Payment options rebuilt; live state still consistent', async () => {
1640+
const rec = {
1641+
id: 'rec-4', provider: 'aws' as const, cloud_account_id: 'test-account-a',
1642+
service: 'ec2', resource_type: 't3.medium', region: 'us-east-1',
1643+
count: 5, term: 1, payment: 'all-upfront', savings: 100, upfront_cost: 500,
1644+
};
1645+
1646+
await openPurchaseModal([rec]);
1647+
1648+
const termSelect = document.querySelector<HTMLSelectElement>('.purchase-row-term');
1649+
const paymentSelect = document.querySelector<HTMLSelectElement>('.purchase-row-payment');
1650+
expect(termSelect).not.toBeNull();
1651+
expect(paymentSelect).not.toBeNull();
1652+
1653+
// Switch term to 3yr.
1654+
termSelect!.value = '3';
1655+
termSelect!.dispatchEvent(new Event('change'));
1656+
1657+
const live = getPurchaseModalRecommendations();
1658+
expect(live[0]!.term).toBe(3);
1659+
// Payment is set to a value supported for AWS EC2 3yr — the
1660+
// exact value depends on whether 'all-upfront' (the prior value)
1661+
// remained valid for the new term; we only require that:
1662+
// (i) live.payment is non-empty
1663+
// (ii) live.payment matches the dropdown's current value
1664+
// (iii) the dropdown's options are now the 3yr-supported set.
1665+
expect(live[0]!.payment).toBeTruthy();
1666+
expect(paymentSelect!.value).toBe(live[0]!.payment);
1667+
const options = Array.from(paymentSelect!.options).map((o) => o.value);
1668+
expect(options.length).toBeGreaterThan(0);
1669+
});
1670+
1671+
test('(e) user changes Payment dropdown → live state reflects new value (and would round-trip via handleExecutePurchase)', async () => {
1672+
const rec = {
1673+
id: 'rec-5', provider: 'aws' as const, cloud_account_id: 'test-account-a',
1674+
service: 'ec2', resource_type: 't3.medium', region: 'us-east-1',
1675+
count: 5, term: 1, payment: 'all-upfront', savings: 100, upfront_cost: 500,
1676+
};
1677+
1678+
await openPurchaseModal([rec]);
1679+
1680+
const paymentSelect = document.querySelector<HTMLSelectElement>('.purchase-row-payment');
1681+
expect(paymentSelect).not.toBeNull();
1682+
1683+
// Change to 'no-upfront' (always supported for AWS EC2 1yr).
1684+
paymentSelect!.value = 'no-upfront';
1685+
paymentSelect!.dispatchEvent(new Event('change'));
1686+
1687+
const live = getPurchaseModalRecommendations();
1688+
expect(live[0]!.payment).toBe('no-upfront');
1689+
1690+
// The mapping in app.ts::handleExecutePurchase reads this value
1691+
// verbatim (`payment: r.payment ?? 'all-upfront'`), so a downstream
1692+
// assertion that the API call carries 'no-upfront' is implicit in
1693+
// (a) the live state above and (b) the source-of-truth read at
1694+
// app.ts:289-303. No separate mock-call assertion needed here —
1695+
// the integration of that mapping is exercised by app.ts'
1696+
// existing handleExecutePurchase tests.
1697+
});
1698+
});

frontend/src/app.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,12 @@ async function handleExecutePurchase(): Promise<void> {
286286
// Map LocalRecommendation to API Recommendation format. The counts +
287287
// costs here are already scaled by the bulk-toolbar's capacity % so
288288
// the backend records exactly what the user saw in the preview.
289+
// Issue #111 (iii): payment now reads `r.payment` (set by
290+
// `openPurchaseModal`'s per-row seed and live edits), replacing the
291+
// historical hardcoded 'all-upfront' that silently dropped the
292+
// toolbar's Payment for the single-bucket path. The `?? 'all-upfront'`
293+
// is defensive only — direct test-harness callers that bypass the
294+
// modal may not set `payment`; the production path always does.
289295
const apiRecs: api.Recommendation[] = localRecs.map((r) => ({
290296
id: r.id,
291297
provider: r.provider,
@@ -294,7 +300,7 @@ async function handleExecutePurchase(): Promise<void> {
294300
resource_type: r.resource_type,
295301
count: r.count,
296302
term: r.term,
297-
payment: 'all-upfront',
303+
payment: r.payment ?? 'all-upfront',
298304
upfront_cost: r.upfront_cost,
299305
monthly_cost: r.monthly_cost ?? 0,
300306
savings: r.savings,

0 commit comments

Comments
 (0)