diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 30bf8668d..99ce5e85a 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -3125,6 +3125,158 @@ describe('Issue #111: per-bucket Payment seed from per-account service override' // openPurchaseModal instead of openFanOutModal). expect(buckets).toBeNull(); }); + + // Issue #197: multi-account bucket exposes per-rec payment defaults seeded + // from each rec's account override. The bucket-level payment falls back to + // the toolbar; perRecPayments overrides per rec. + test('(h) issue #197: multi-account bucket carries per-rec payment map seeded from each account override', async () => { + // Two recs, same (provider, service, term, payment) bucket key so they + // land in ONE bucket, but different cloud_account_ids — triggers the + // multi-account per-rec seeding path. Need a second bucket (different term) + // to force fan-out via openFanOutModal. + const recs = [ + { id: 'h1', provider: 'aws', cloud_account_id: 'acct-x', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', savings: 100, upfront_cost: 500 }, + { id: 'h2', provider: 'aws', cloud_account_id: 'acct-y', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', savings: 150, upfront_cost: 600 }, + // Second bucket (3yr, single account) just to force fan-out modal. + { id: 'h3', provider: 'aws', cloud_account_id: 'acct-x', service: 'ec2', resource_type: 'm5.large', region: 'us-east-1', count: 1, term: 3, payment: 'all-upfront', savings: 300, upfront_cost: 1200 }, + ]; + setupMixedTermRecs(recs); + // acct-x prefers partial-upfront; acct-y prefers no-upfront. + (api.listAccountServiceOverrides as jest.Mock).mockImplementation(async (id: string) => { + if (id === 'acct-x') return [{ id: 'ovr-x', account_id: 'acct-x', provider: 'aws', service: 'ec2', payment: 'partial-upfront' }]; + if (id === 'acct-y') return [{ id: 'ovr-y', account_id: 'acct-y', provider: 'aws', service: 'ec2', payment: 'no-upfront' }]; + return []; + }); + + await loadRecommendations(); + (document.getElementById('bulk-purchase-btn') as HTMLButtonElement).click(); + await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); + + const { getFanOutBuckets } = await import('../recommendations'); + const buckets = getFanOutBuckets(); + expect(buckets).not.toBeNull(); + const bucket1yr = buckets!.find((b) => b.term === 1)!; + expect(bucket1yr).toBeDefined(); + + // The 1yr bucket is multi-account: perRecPayments must be present. + expect(bucket1yr.perRecPayments).toBeDefined(); + const prp = bucket1yr.perRecPayments!; + // h1 (acct-x) seeded from partial-upfront override. + expect(prp.get('h1')).toBe('partial-upfront'); + // h2 (acct-y) seeded from no-upfront override. + expect(prp.get('h2')).toBe('no-upfront'); + + // Bucket-level payment falls back to toolbar (multi-account, no single override). + expect(bucket1yr.paymentSource).toBe('toolbar'); + + // Per-rec dropdowns must be rendered in the modal. + const perRecSelects = document.querySelectorAll('.fanout-per-rec-payment'); + expect(perRecSelects.length).toBeGreaterThanOrEqual(2); + const h1Select = Array.from(perRecSelects).find((s) => s.dataset['recId'] === 'h1'); + const h2Select = Array.from(perRecSelects).find((s) => s.dataset['recId'] === 'h2'); + expect(h1Select?.value).toBe('partial-upfront'); + expect(h2Select?.value).toBe('no-upfront'); + }); + + test('(i) issue #197: per-rec dropdown change updates perRecPayments in module state', async () => { + // Same multi-account 1yr bucket as (h), plus a 3yr bucket to force fan-out. + const recs = [ + { id: 'i1', provider: 'aws', cloud_account_id: 'acct-p', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', savings: 100, upfront_cost: 500 }, + { id: 'i2', provider: 'aws', cloud_account_id: 'acct-q', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', savings: 120, upfront_cost: 550 }, + { id: 'i3', provider: 'aws', cloud_account_id: 'acct-p', service: 'ec2', resource_type: 'm5.large', region: 'us-east-1', count: 1, term: 3, payment: 'all-upfront', savings: 300, upfront_cost: 1200 }, + ]; + setupMixedTermRecs(recs); + (api.listAccountServiceOverrides as jest.Mock).mockResolvedValue([]); + + await loadRecommendations(); + (document.getElementById('bulk-purchase-btn') as HTMLButtonElement).click(); + await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); + + const { getFanOutBuckets } = await import('../recommendations'); + const before = getFanOutBuckets(); + const bucket1yr = before!.find((b) => b.term === 1)!; + expect(bucket1yr.perRecPayments).toBeDefined(); + // No overrides: both recs match the bucket default (all-upfront), so the + // map holds ONLY explicit overrides — i1/i2 are absent and fall back to + // b.payment via the execute path. (Eager population would make the + // bucket-level dropdown a no-op for these rows.) + expect(bucket1yr.perRecPayments!.has('i1')).toBe(false); + expect(bucket1yr.perRecPayments!.has('i2')).toBe(false); + + // User changes the i1 dropdown to no-upfront — now an explicit override. + const i1Select = Array.from( + document.querySelectorAll('.fanout-per-rec-payment'), + ).find((s) => s.dataset['recId'] === 'i1')!; + expect(i1Select).toBeDefined(); + i1Select.value = 'no-upfront'; + i1Select.dispatchEvent(new Event('change')); + + const after = getFanOutBuckets(); + const afterBucket1yr = after!.find((b) => b.term === 1)!; + expect(afterBucket1yr.perRecPayments!.get('i1')).toBe('no-upfront'); + // i2 still follows the bucket default — absent from the override map. + expect(afterBucket1yr.perRecPayments!.has('i2')).toBe(false); + + // Setting i1 back to the bucket default removes the override again so the + // row resumes tracking the bucket-level dropdown. + i1Select.value = 'all-upfront'; + i1Select.dispatchEvent(new Event('change')); + const reset = getFanOutBuckets(); + expect(reset!.find((b) => b.term === 1)!.perRecPayments!.has('i1')).toBe(false); + }); + + // Issue #197 regression (CR #838): the bucket-level Payment dropdown must + // remain effective for multi-account rows that follow the bucket default. + // Before the fix, openFanOutModal eagerly wrote every rec into + // perRecPayments, so changing the bucket dropdown only mutated b.payment + // while the execute path still read the stale per-rec entry — making the + // visible control a no-op for unedited rows. + test('(j) issue #197: bucket-level Payment change propagates to non-overridden recs in the POST payload', async () => { + const recs = [ + { id: 'j1', provider: 'aws', cloud_account_id: 'acct-r', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', savings: 100, upfront_cost: 500 }, + { id: 'j2', provider: 'aws', cloud_account_id: 'acct-s', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', savings: 120, upfront_cost: 550 }, + { id: 'j3', provider: 'aws', cloud_account_id: 'acct-r', service: 'ec2', resource_type: 'm5.large', region: 'us-east-1', count: 1, term: 3, payment: 'all-upfront', savings: 300, upfront_cost: 1200 }, + ]; + setupMixedTermRecs(recs); + (api.listAccountServiceOverrides as jest.Mock).mockResolvedValue([]); + + await loadRecommendations(); + (document.getElementById('bulk-purchase-btn') as HTMLButtonElement).click(); + await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); + + // Change the multi-account 1yr bucket's bucket-level Payment dropdown. + const bucketSelects = Array.from( + document.querySelectorAll('.fanout-bucket-payment'), + ); + // The 1yr bucket is the multi-account one (renders per-rec selects); find + // the bucket section that contains per-rec rows. + const targetSelect = bucketSelects.find((sel) => { + const section = sel.closest('.fanout-bucket'); + return section?.querySelector('.fanout-per-rec-payment') != null; + })!; + expect(targetSelect).toBeDefined(); + targetSelect.value = 'no-upfront'; + targetSelect.dispatchEvent(new Event('change')); + + const { getFanOutBuckets } = await import('../recommendations'); + const buckets = getFanOutBuckets()!; + const bucket1yr = buckets.find((b) => b.term === 1)!; + // Execute path: payment = perRecPayments.get(id) ?? b.payment. With the + // override map empty for unedited rows, both recs must post the NEW + // bucket payment. + const resolved = (id: string): string => bucket1yr.perRecPayments?.get(id) ?? bucket1yr.payment; + expect(resolved('j1')).toBe('no-upfront'); + expect(resolved('j2')).toBe('no-upfront'); + + // The visible per-rec selects must reflect the new bucket default too. + const perRecSelects = Array.from( + document.querySelectorAll('.fanout-per-rec-payment'), + ); + const j1Select = perRecSelects.find((s) => s.dataset['recId'] === 'j1'); + const j2Select = perRecSelects.find((s) => s.dataset['recId'] === 'j2'); + expect(j1Select?.value).toBe('no-upfront'); + expect(j2Select?.value).toBe('no-upfront'); + }); }); // Issue #111 (iii): per-row Payment seed in openPurchaseModal — the diff --git a/frontend/src/app.ts b/frontend/src/app.ts index 124b7e0dc..ba0fce672 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -470,17 +470,19 @@ async function handleFanOutExecute(buckets: FanOutBucket[]): Promise { // server-provided rec so `details`, `engine`, `cloud_account_id`, and // any future additions flow through unchanged. Only `payment`, // `monthly_cost`, `selected`, and `purchased` are overridden: `payment` - // comes from the bucket (user's per-bucket choice), `monthly_cost` is - // coerced to null for absent values, and the purchase-intent flags are - // forced to their canonical values. Passing `details` ensures - // non-default platforms (Windows EC2, dedicated tenancy, AZ-scoped RIs, - // non-default-engine RDS/Cache) reach the backend correctly (issue #597). + // comes from perRecPayments[rec.id] for multi-account buckets (issue #197) + // or from the bucket-level payment for single-account buckets. + // `monthly_cost` is coerced to null for absent values, and the + // purchase-intent flags are forced to their canonical values. Passing + // `details` ensures non-default platforms (Windows EC2, dedicated tenancy, + // AZ-scoped RIs, non-default-engine RDS/Cache) reach the backend correctly + // (issue #597). const promises = buckets.map((b) => api.executePurchase( b.recs.map((r) => ({ ...r, monthly_cost: r.monthly_cost ?? null, - payment: b.payment, + payment: b.perRecPayments?.get(r.id) ?? b.payment, selected: true, purchased: false, })), diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index b0ab6f8ce..2bbbeb8ca 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3226,6 +3226,13 @@ function handleBulkPurchaseClick(recommendations: LocalRecommendation[]): void { // The user can change the per-bucket Payment via the dropdown // rendered in the modal; the `change` handler updates `payment` (and // keeps `paymentSource` so the source note doesn't lie about origin). +// +// `perRecPayments` (issue #197): set only for multi-account buckets. +// When present, each rec gets its own Payment dropdown seeded from +// its account's override (if available), falling back to the +// bucket-level `payment`. handleFanOutExecute uses the per-rec +// value when sending the POST so each rec's account override is +// honoured even inside a mixed-account bucket. export interface FanOutBucket { provider: CompatProvider; service: string; @@ -3234,6 +3241,10 @@ export interface FanOutBucket { capacityPercent: number; recs: LocalRecommendation[]; // scaled by capacityPercent paymentSource: 'override' | 'toolbar'; + // Per-rec payment overrides for multi-account buckets (issue #197). + // Present only when the bucket spans 2+ distinct cloud_account_id values. + // Keys are rec.id; values are the resolved payment for that rec. + perRecPayments?: Map; } // Fan-out modal state. app.ts's Send-for-Approval click reads these @@ -3242,7 +3253,12 @@ export interface FanOutBucket { let currentFanOutBuckets: FanOutBucket[] | null = null; export function getFanOutBuckets(): FanOutBucket[] | null { - return currentFanOutBuckets ? currentFanOutBuckets.map((b) => ({ ...b })) : null; + if (!currentFanOutBuckets) return null; + return currentFanOutBuckets.map((b) => ({ + ...b, + // Deep-copy the per-rec map so callers can't mutate module state. + perRecPayments: b.perRecPayments ? new Map(b.perRecPayments) : undefined, + })); } export function clearFanOutBuckets(): void { @@ -3342,28 +3358,22 @@ async function openFanOutModal( bucketEntries: Array<[string, LocalRecommendation[]]>, toolbar: BulkPurchaseToolbarState, ): Promise { - // Pre-fetch service-overrides for every account that's the SOLE - // account in any bucket — these are the only buckets eligible for - // the override seed (multi-account buckets always fall back to - // toolbar). One fetch per distinct accountID; cached for the - // lifetime of this openFanOutModal call. Errors are swallowed: the - // toolbar-seed fallback always works, so a transient API failure - // shouldn't block the modal. - const eligibleAccountIDs = new Set(); + // Pre-fetch service-overrides for every distinct account referenced by + // any rec in any bucket. Single-account buckets use overridesByAccount + // to seed the bucket-level payment (issue #111). Multi-account buckets + // (issue #197) also use it to seed each rec's per-rec payment default. + // One fetch per distinct accountID; cached for the lifetime of this + // openFanOutModal call. Errors are swallowed: the toolbar-seed fallback + // always works, so a transient API failure shouldn't block the modal. + const allAccountIDs = new Set(); for (const [, recs] of bucketEntries) { - if (recs.length === 0) continue; - const ids = new Set(); for (const r of recs) { - if (r.cloud_account_id) ids.add(r.cloud_account_id); - } - if (ids.size === 1) { - const only = recs[0]?.cloud_account_id; - if (only) eligibleAccountIDs.add(only); + if (r.cloud_account_id) allAccountIDs.add(r.cloud_account_id); } } const overridesByAccount = new Map(); await Promise.all( - Array.from(eligibleAccountIDs).map(async (id) => { + Array.from(allAccountIDs).map(async (id) => { try { const list = await api.listAccountServiceOverrides(id); overridesByAccount.set(id, list); @@ -3383,6 +3393,45 @@ async function openFanOutModal( // section header can render the mixed-plan-type label; the per- // rec slugs on recs[].service are what the backend sees. const bucketService = isSavingsPlanService(r.service) ? SAVINGS_PLANS_BUCKET_KEY : r.service; + + // Issue #197: for multi-account buckets, build a per-rec payment + // map seeded from each rec's account override (when available and + // supported), falling back to the bucket-level payment. This lets + // each account's payment policy apply inside a mixed-account bucket. + const distinctAccountIDs = new Set(recs.map((rec) => rec.cloud_account_id).filter(Boolean)); + let perRecPayments: Map | undefined; + if (distinctAccountIDs.size > 1) { + perRecPayments = new Map(); + const bucketPayment = seed.payment; + for (const rec of recs) { + let recPayment: BulkPurchasePayment = bucketPayment; + if (rec.cloud_account_id) { + const overrides = overridesByAccount.get(rec.cloud_account_id); + if (overrides) { + const recTerm = rec.term as 1 | 3; + const match = overrides.find( + (o) => o.provider === (rec.provider as CompatProvider) && o.service === rec.service, + ); + const overridePayment = normalizeBulkPayment(match?.payment); + if ( + overridePayment + && isPaymentSupported(rec.provider as CompatProvider, rec.service, recTerm, overridePayment) + ) { + recPayment = overridePayment; + } + } + } + // Only record an explicit override; recs that match the bucket + // default are intentionally left out of the map so they keep + // following the bucket-level dropdown via the `?? b.payment` + // fallback on the execute path. Eagerly populating every rec would + // make the bucket-level control a no-op for unedited rows. + if (recPayment !== bucketPayment) { + perRecPayments.set(rec.id, recPayment); + } + } + } + return { provider: r.provider as CompatProvider, service: bucketService, @@ -3394,6 +3443,7 @@ async function openFanOutModal( paymentSource: seed.source, capacityPercent: toolbar.capacity, recs, + perRecPayments, }; }); currentFanOutBuckets = buckets; @@ -3523,6 +3573,23 @@ function renderFanOutBucketSection(b: FanOutBucket): HTMLElement { } b.payment = next; renderStatus(); + // Re-sync any visible per-rec selects whose ids are NOT explicit + // overrides: those rows follow the bucket default, so their displayed + // value must track the new bucket payment. Rows with an explicit + // override (present in perRecPayments) keep their own value. + if (b.perRecPayments) { + const perRecSelects = section.querySelectorAll('.fanout-per-rec-payment'); + perRecSelects.forEach((sel) => { + const recId = sel.dataset['recId']; + if (!recId || b.perRecPayments!.has(recId)) return; + // Only re-sync when this rec actually supports the new payment. + // Per-rec options derive from rec.service, which can differ from + // b.service in mixed-SP buckets; skip rows where `next` isn't an + // option so the displayed value never diverges from what posts. + const supported = Array.from(sel.options).some((o) => o.value === next); + if (supported) sel.value = next; + }); + } }); paymentLabel.appendChild(paymentSelect); paymentRow.appendChild(paymentLabel); @@ -3534,6 +3601,68 @@ function renderFanOutBucketSection(b: FanOutBucket): HTMLElement { } section.appendChild(paymentRow); + // Issue #197: when the bucket spans multiple accounts, render a per-rec + // Payment dropdown for each rec so each account's override policy applies + // independently. The bucket-level dropdown above still acts as a fallback + // default but is labelled to make the per-rec row the primary surface. + if (b.perRecPayments) { + const perRecNote = document.createElement('p'); + perRecNote.className = 'fanout-per-rec-note'; + perRecNote.textContent = 'Multi-account bucket: each commitment can use its own payment option.'; + section.appendChild(perRecNote); + + const perRecList = document.createElement('ul'); + perRecList.className = 'fanout-per-rec-list'; + for (const rec of b.recs) { + const currentPayment = b.perRecPayments.get(rec.id) ?? b.payment; + const li = document.createElement('li'); + li.className = 'fanout-per-rec-item'; + + const recLabel = document.createElement('span'); + recLabel.className = 'fanout-per-rec-label'; + // Show account + resource_type so the user can associate the row. + recLabel.textContent = `${rec.cloud_account_id ?? 'unknown'} / ${rec.resource_type}`; + li.appendChild(recLabel); + + const recSelect = document.createElement('select'); + recSelect.className = 'fanout-per-rec-payment'; + recSelect.dataset['recId'] = rec.id; + for (const opt of paymentOptionsFor(b.provider, rec.service, b.term)) { + const option = document.createElement('option'); + option.value = opt; + option.textContent = opt; + if (opt === currentPayment) option.selected = true; + recSelect.appendChild(option); + } + recSelect.addEventListener('change', () => { + const next = recSelect.value as BulkPurchasePayment; + // Keep perRecPayments as the explicit-override set: when the user + // picks the current bucket default, drop the entry so the row tracks + // future bucket-level changes again; otherwise record the override. + const applyToMap = (map: Map | undefined): void => { + if (!map) return; + if (next === b.payment) { + map.delete(rec.id); + } else { + map.set(rec.id, next); + } + }; + // Update module state and the local bucket reference. + if (currentFanOutBuckets) { + const idx = currentFanOutBuckets.findIndex((cb) => cb.recs === b.recs); + if (idx >= 0) { + applyToMap(currentFanOutBuckets[idx]!.perRecPayments); + } + } + applyToMap(b.perRecPayments); + }); + + li.appendChild(recSelect); + perRecList.appendChild(li); + } + section.appendChild(perRecList); + } + const bucketTotal = b.recs.reduce( (acc, r) => ({ count: acc.count + r.count,