From 46356d882d96287a3c12a8788c1df42e8ef53f65 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 19:48:48 +0200 Subject: [PATCH 1/3] fix(ui/purchases): include payment in bucket key so multi-bucket Configure Purchase preserves per-rec payment Before this fix, recs with identical (provider, service, term) but different per-rec payment values were bucketed together under the same key and seeded from toolbar.payment ('all-upfront' by default), silently overriding each rec's actual payment option. The Configure Purchase modal then submitted all commitments as 'all-upfront' regardless of what the recommendations specified. Two-part fix: 1. Add `payment` to the bucket key at handleBulkPurchaseClick so recs with different payments fan into separate, payment-uniform buckets. Mirrors the existing 'bundle B' term-key approach. 2. Update resolveBucketPaymentSeed to seed from recs[0].payment (uniform within the bucket after fix 1) before falling back to toolbar.payment, matching the resolvePerRecPaymentSeed precedence used by the single-bucket path. Closes #699 --- .../src/__tests__/recommendations.test.ts | 39 +++++++++ frontend/src/recommendations.ts | 83 ++++++++++++------- 2 files changed, 92 insertions(+), 30 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index e0ab8dbf8..523049f2d 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -2793,6 +2793,45 @@ describe('Issue #111: per-bucket Payment seed from per-account service override' // At least one bucket payment must now be 'no-upfront'. expect(after!.some((b) => b.payment === 'no-upfront')).toBe(true); }); + + // Regression: issue #699. Before the fix, recs with the same + // (provider, service, term) but different rec.payment values were + // collapsed into one bucket and seeded from toolbar.payment + // ('all-upfront'), silently overriding each rec's actual payment. + // Fix: include `payment` in the bucket key so each distinct payment + // fans into its own bucket; resolveBucketPaymentSeed then uses + // recs[0].payment (uniform within the bucket) as its seed. + test('(e) issue #699: same (provider, service, term) but different rec.payment fans into separate buckets seeded from rec.payment, not toolbar', async () => { + // Two recs: same aws/ec2/1yr but one is partial-upfront and one is + // no-upfront. Different resource_type so each is its own cell (not + // collapsed by pickBestVariantPerCell). + const recs = [ + { id: 'x1', provider: 'aws', cloud_account_id: 'test-account-a', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment: 'partial-upfront', savings: 100, upfront_cost: 500 }, + { id: 'x2', provider: 'aws', cloud_account_id: 'test-account-a', service: 'ec2', resource_type: 'm5.large', region: 'us-east-1', count: 1, term: 1, payment: 'no-upfront', savings: 150, upfront_cost: 0 }, + ]; + setupMixedTermRecs(recs); + // No account overrides — resolveBucketPaymentSeed must seed from + // rec.payment, not the toolbar default (all-upfront). + (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 buckets = getFanOutBuckets(); + expect(buckets).not.toBeNull(); + // After the fix: 2 buckets (one per payment variant). + expect(buckets!.length).toBe(2); + // Each bucket carries the correct per-rec payment, not the toolbar default. + const payments = buckets!.map((b) => b.payment).sort(); + expect(payments).toEqual(['no-upfront', 'partial-upfront']); + // paymentSource is 'toolbar' (rec.payment fallback path, no override), + // confirming the seed came from the rec, not an account override. + for (const b of buckets!) { + expect(b.paymentSource).toBe('toolbar'); + } + }); }); // Issue #111 (iii): per-row Payment seed in openPurchaseModal — the diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index b26750c9f..62995c7be 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3134,10 +3134,13 @@ function handleBulkPurchaseClick(recommendations: LocalRecommendation[]): void { return; } - // Bucket by (provider, service, term). Bundle B added `term` to the key: - // each rec is purchased with its OWN per-row term (the toolbar Term - // selector is gone). Multi-term selections legitimately fan out into - // multiple buckets, e.g. AWS EC2 1y + AWS EC2 3y → 2 buckets. + // Bucket by (provider, service, term, payment). Bundle B added `term` to + // the key so multi-term selections fan out into separate buckets. Issue + // #699 adds `payment` for the same reason: recs with identical + // (provider, service, term) but different per-rec payment values must + // also land in separate buckets so each bucket is payment-uniform and + // resolveBucketPaymentSeed can seed from recs[0].payment rather than + // falling back to the toolbar default ('all-upfront'). // // Issue #132: SP recs (savings-plans-{compute,ec2instance,sagemaker, // database}) collapse into a single bucket per (provider, term) so an @@ -3152,7 +3155,7 @@ function handleBulkPurchaseClick(recommendations: LocalRecommendation[]): void { const buckets = new Map(); for (const r of scaled) { const bucketService = isSavingsPlanService(r.service) ? SAVINGS_PLANS_BUCKET_KEY : r.service; - const key = `${r.provider}|${bucketService}|${r.term}`; + const key = `${r.provider}|${bucketService}|${r.term}|${r.payment ?? ''}`; const existing = buckets.get(key); if (existing) existing.push(r); else buckets.set(key, [r]); @@ -3265,8 +3268,7 @@ function resolveBucketPaymentSeed( toolbar: BulkPurchaseToolbarState, overridesByAccount: Map, ): { payment: BulkPurchaseToolbarState['payment']; source: 'override' | 'toolbar' } { - const fallback = { payment: toolbar.payment, source: 'toolbar' as const }; - if (recs.length === 0) return fallback; + if (recs.length === 0) return { payment: toolbar.payment, source: 'toolbar' }; const r0 = recs[0]!; const provider = r0.provider as CompatProvider; const term = r0.term as 1 | 3; @@ -3274,33 +3276,54 @@ function resolveBucketPaymentSeed( // Single-account check: every rec must carry the same non-empty cloud_account_id. const accountIDs = new Set(); for (const r of recs) { - if (!r.cloud_account_id) return fallback; // any rec missing an id → toolbar + if (!r.cloud_account_id) { + // Any rec missing an account id skips the override lookup; fall + // through to the rec.payment seed below. + accountIDs.clear(); + break; + } accountIDs.add(r.cloud_account_id); } - if (accountIDs.size !== 1) return fallback; - const accountID = recs[0]!.cloud_account_id!; - - const overrides = overridesByAccount.get(accountID); - if (!overrides) return fallback; + if (accountIDs.size === 1) { + const accountID = recs[0]!.cloud_account_id!; + const overrides = overridesByAccount.get(accountID); + if (overrides) { + // Match on the per-rec service (NOT bucket.service) — see the comment + // above for the SP-canonical-bucket-key future-proofing rationale. + const match = overrides.find( + (o) => o.provider === provider && o.service === r0.service, + ); + if ( + match + && match.payment + // Defensive: only honour the override when the (provider, service, + // term, payment) combo is actually supported. A stale or hand-saved + // override pointing at an unsupported payment for this term shouldn't + // poison the dropdown — fall through to rec.payment seed. + && isPaymentSupported(provider, r0.service, term, match.payment as CompatPayment) + ) { + return { + payment: match.payment as BulkPurchaseToolbarState['payment'], + source: 'override', + }; + } + } + } - // Match on the per-rec service (NOT bucket.service) — see the comment - // above for the SP-canonical-bucket-key future-proofing rationale. - const match = overrides.find( - (o) => o.provider === provider && o.service === r0.service, - ); - if (!match || !match.payment) return fallback; - - // Defensive: only honour the override when the (provider, service, - // term, payment) combo is actually supported. A stale or hand-saved - // override pointing at an unsupported payment for this term shouldn't - // poison the dropdown — fall back to toolbar. - if (!isPaymentSupported(provider, r0.service, term, match.payment as CompatPayment)) { - return fallback; + // Issue #699: since `payment` is now part of the bucket key, every bucket + // is payment-uniform (all recs share the same rec.payment). Seed from + // recs[0].payment when it's a supported value for this (provider, service, + // term) cell instead of blindly falling back to toolbar.payment + // ('all-upfront'). Multi-account buckets, missing-payment recs, and + // unsupported payment values still fall through to toolbar. + if ( + r0.payment + && isPaymentSupported(provider, r0.service, term, r0.payment as CompatPayment) + ) { + return { payment: r0.payment as BulkPurchaseToolbarState['payment'], source: 'toolbar' }; } - return { - payment: match.payment as BulkPurchaseToolbarState['payment'], - source: 'override', - }; + + return { payment: toolbar.payment, source: 'toolbar' }; } async function openFanOutModal( From 8e21a1bef311242c44339b5c046c408fd09d86d5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 22:25:29 +0200 Subject: [PATCH 2/3] fix(ui/purchases): normalize upfront/monthly synonyms before bucket key Use normalizeBulkPayment() to map upstream payment synonyms ('upfront' -> 'all-upfront') before building the fan-out bucket key and before seeding resolveBucketPaymentSeed. Without this, two recs that share the same effective payment but differ only in the synonym form ('upfront' vs 'all-upfront') were keyed into separate buckets, defeating the #699 fix. Closes: CR Major finding on PR #710 (coderabbitai comment 3299769088). --- .../src/__tests__/recommendations.test.ts | 38 +++++++++++++++++- frontend/src/recommendations.ts | 40 +++++++++++++++++-- 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 523049f2d..03771e78b 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -2596,7 +2596,7 @@ describe('Bundle B: term-aware bucketing in the Purchase flow', () => { // payment unsupported by the (provider, service, term) cell) the // bucket falls back to the toolbar payment. describe('Issue #111: per-bucket Payment seed from per-account service override', () => { - beforeEach(() => { + beforeEach(async () => { document.body.replaceChildren(); const recsTab = document.createElement('div'); recsTab.id = 'opportunities-tab'; @@ -2618,6 +2618,11 @@ describe('Issue #111: per-bucket Payment seed from per-account service override' jest.clearAllMocks(); // Default: empty overrides — overridden per-test. (api.listAccountServiceOverrides as jest.Mock).mockResolvedValue([]); + // Module-level fan-out state survives test isolation; reset it so a + // previous test's openFanOutModal call does not leak into tests that + // assert the single-bucket happy path (getFanOutBuckets() === null). + const { clearFanOutBuckets } = await import('../recommendations'); + clearFanOutBuckets(); }); // Force a multi-bucket fan-out by mixing terms (1yr + 3yr both @@ -2832,6 +2837,37 @@ describe('Issue #111: per-bucket Payment seed from per-account service override' expect(b.paymentSource).toBe('toolbar'); } }); + + // Regression: CR finding on PR #710. 'upfront' is a synonym for + // 'all-upfront' used by some upstream rows (Azure canonical form). + // Both must land in ONE bucket with the normalized 'all-upfront' seed. + test('(f) payment synonyms upfront / all-upfront collapse into one bucket seeded as all-upfront', async () => { + // Two recs, same aws/ec2/1yr, but one carries 'upfront' (Azure + // synonym) and the other carries 'all-upfront' (canonical form). + // Different resource_type so each is its own cell and both survive + // pickBestVariantPerCell without collapsing. + // After normalization they share the same bucket key and should + // produce a SINGLE bucket (not two), forcing the single-bucket happy + // path (openPurchaseModal, not openFanOutModal). + const recs = [ + { id: 'f1', provider: 'aws', cloud_account_id: 'test-account-a', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment: 'upfront', savings: 100, upfront_cost: 500 }, + { id: 'f2', provider: 'aws', cloud_account_id: 'test-account-a', service: 'ec2', resource_type: 'm5.large', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', savings: 150, upfront_cost: 600 }, + ]; + 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 buckets = getFanOutBuckets(); + // Single bucket expected: 'upfront' normalized to 'all-upfront' before keying. + // Both recs share the same normalized key so no fan-out is triggered and + // getFanOutBuckets() returns null (single-bucket happy path opens + // openPurchaseModal instead of openFanOutModal). + expect(buckets).toBeNull(); + }); }); // Issue #111 (iii): per-row Payment seed in openPurchaseModal — the diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 62995c7be..ad9d7b4ef 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -2712,6 +2712,34 @@ const BULK_PURCHASE_LS_KEY = 'cudly.recommendations.bulkPurchase.v1'; // is silently ignored on read — no migration shim needed. type BulkPurchasePayment = 'all-upfront' | 'partial-upfront' | 'no-upfront' | 'monthly'; +// Normalize payment synonyms that upstream rows may carry (e.g. Azure's +// 'upfront') to the canonical BulkPurchasePayment forms used throughout the +// bucket-key and toolbar machinery. Returns null for unknown or absent values +// so callers can fall back safely. +// +// Mappings: +// 'upfront' -> 'all-upfront' (Azure canonical synonym) +// 'all-upfront' -> 'all-upfront' (AWS / pass-through) +// 'partial-upfront' -> 'partial-upfront' (pass-through) +// 'no-upfront' -> 'no-upfront' (pass-through) +// 'monthly' -> 'monthly' (GCP canonical; kept as-is) +// anything else / undefined -> null +function normalizeBulkPayment(payment: string | undefined): BulkPurchasePayment | null { + switch (payment) { + case 'upfront': + case 'all-upfront': + return 'all-upfront'; + case 'partial-upfront': + return 'partial-upfront'; + case 'no-upfront': + return 'no-upfront'; + case 'monthly': + return 'monthly'; + default: + return null; + } +} + // Centralized bucket-level payment compatibility check. A bucket is // compatible iff EVERY rec in it has a supported (provider, service, // term, payment) combination. Used by the bulk-buy fan-out path to @@ -3155,7 +3183,7 @@ function handleBulkPurchaseClick(recommendations: LocalRecommendation[]): void { const buckets = new Map(); for (const r of scaled) { const bucketService = isSavingsPlanService(r.service) ? SAVINGS_PLANS_BUCKET_KEY : r.service; - const key = `${r.provider}|${bucketService}|${r.term}|${r.payment ?? ''}`; + const key = `${r.provider}|${bucketService}|${r.term}|${normalizeBulkPayment(r.payment) ?? ''}`; const existing = buckets.get(key); if (existing) existing.push(r); else buckets.set(key, [r]); @@ -3316,11 +3344,15 @@ function resolveBucketPaymentSeed( // term) cell instead of blindly falling back to toolbar.payment // ('all-upfront'). Multi-account buckets, missing-payment recs, and // unsupported payment values still fall through to toolbar. + // + // Normalize first so upstream synonym forms ('upfront', 'monthly') map to + // the canonical BulkPurchasePayment values before the support check. + const recPayment = normalizeBulkPayment(r0.payment); if ( - r0.payment - && isPaymentSupported(provider, r0.service, term, r0.payment as CompatPayment) + recPayment + && isPaymentSupported(provider, r0.service, term, recPayment as CompatPayment) ) { - return { payment: r0.payment as BulkPurchaseToolbarState['payment'], source: 'toolbar' }; + return { payment: recPayment, source: 'toolbar' }; } return { payment: toolbar.payment, source: 'toolbar' }; From 40f2826968fcf970fc264680f0cef7dfcf0a040e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 26 May 2026 00:17:12 +0200 Subject: [PATCH 3/3] fix(ui/purchases): normalize override payment before seeding bucket In resolveBucketPaymentSeed, match.payment was passed raw to isPaymentSupported and returned directly. An account override carrying Azure's 'upfront' synonym would seed FanOutBucket.payment = 'upfront', a value the fan-out dropdown never renders (it only lists canonical BulkPurchasePayment values), while the backing state still held 'upfront'. Fix: compute overridePayment = normalizeBulkPayment(match?.payment) first, then use overridePayment in both the isPaymentSupported guard and the return value. Synonyms like 'upfront' now normalize to 'all-upfront' before reaching the bucket, keeping the displayed value and submitted value in sync. Add test (f): azure/vm override carrying 'upfront' produces two fan-out buckets each seeded with payment='all-upfront' and source='override'. Rename previous test (f) to (g) to preserve ordering. Resolves CR outside-diff finding on PR #710. --- .../src/__tests__/recommendations.test.ts | 38 ++++++++++++++++++- frontend/src/recommendations.ts | 8 ++-- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 03771e78b..e06a597d6 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -2838,10 +2838,46 @@ describe('Issue #111: per-bucket Payment seed from per-account service override' } }); + // CR finding on PR #710: account override carrying Azure's 'upfront' synonym + // must be normalized before seeding the bucket, so the fan-out dropdown + // receives 'all-upfront' (not the raw 'upfront') and paymentSource is + // 'override', not 'toolbar'. + test('(f) account override with upfront (Azure synonym) normalizes to all-upfront with source override', async () => { + // Two azure/vm recs, same single account, different terms to force fan-out. + const recs = [ + { id: 'g1', provider: 'azure', cloud_account_id: 'az-account-a', service: 'vm', resource_type: 'Standard_D2s_v3', region: 'eastus', count: 1, term: 1, payment: 'all-upfront', savings: 120, upfront_cost: 600 }, + { id: 'g2', provider: 'azure', cloud_account_id: 'az-account-a', service: 'vm', resource_type: 'Standard_D2s_v3', region: 'eastus', count: 1, term: 3, payment: 'all-upfront', savings: 300, upfront_cost: 1200 }, + ]; + setupMixedTermRecs(recs); + // Override uses the Azure-canonical 'upfront' synonym — before the fix this + // would seed FanOutBucket.payment = 'upfront' (not rendered by the dropdown); + // after the fix it normalizes to 'all-upfront'. + (api.listAccountServiceOverrides as jest.Mock).mockImplementation(async (id: string) => { + if (id === 'az-account-a') { + return [{ id: 'ovr-az', account_id: 'az-account-a', provider: 'azure', service: 'vm', payment: '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(); + expect(buckets!.length).toBe(2); + for (const b of buckets!) { + // Override 'upfront' must normalize to canonical 'all-upfront'. + expect(b.payment).toBe('all-upfront'); + expect(b.paymentSource).toBe('override'); + } + }); + // Regression: CR finding on PR #710. 'upfront' is a synonym for // 'all-upfront' used by some upstream rows (Azure canonical form). // Both must land in ONE bucket with the normalized 'all-upfront' seed. - test('(f) payment synonyms upfront / all-upfront collapse into one bucket seeded as all-upfront', async () => { + test('(g) payment synonyms upfront / all-upfront collapse into one bucket seeded as all-upfront', async () => { // Two recs, same aws/ec2/1yr, but one carries 'upfront' (Azure // synonym) and the other carries 'all-upfront' (canonical form). // Different resource_type so each is its own cell and both survive diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index ad9d7b4ef..5f7b54f7a 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3321,17 +3321,17 @@ function resolveBucketPaymentSeed( const match = overrides.find( (o) => o.provider === provider && o.service === r0.service, ); + const overridePayment = normalizeBulkPayment(match?.payment); if ( - match - && match.payment + overridePayment // Defensive: only honour the override when the (provider, service, // term, payment) combo is actually supported. A stale or hand-saved // override pointing at an unsupported payment for this term shouldn't // poison the dropdown — fall through to rec.payment seed. - && isPaymentSupported(provider, r0.service, term, match.payment as CompatPayment) + && isPaymentSupported(provider, r0.service, term, overridePayment) ) { return { - payment: match.payment as BulkPurchaseToolbarState['payment'], + payment: overridePayment, source: 'override', }; }