From c4e834bf63a3bf71e7b6a9853671d04477e985e3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 00:34:25 +0200 Subject: [PATCH 1/2] fix(opportunities): prefill Create Purchase Plan modal from selected commitment (closes #770) When exactly 1 commitment is selected and "Plan from 1 selected" is clicked, openCreatePlanModal now calls prefillPurchaseConfigFromCommitment to set the provider, service, term, and payment selects from the commitment record. If the commitment carries a cloud_account_id, prefillAccountChipFromId fetches the account via api.getAccount and adds it as a pre-selected chip so the Target Accounts field is also prefilled. All prefilled fields remain editable. Missing/null fields are handled gracefully (field is left at its reset default). Prefill is skipped when snapshot.length != 1 (multi-commitment plans require manual configuration). Account lookup failure silently no-ops so the user can still search and add the account manually. --- frontend/src/__tests__/plans.test.ts | 103 ++++++++++++++++++++++++++- frontend/src/plans.ts | 68 ++++++++++++++++++ 2 files changed, 170 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index 333805fbe..be996d928 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -19,7 +19,8 @@ jest.mock('../api', () => ({ createPlannedPurchases: jest.fn(), listPlanAccounts: jest.fn().mockResolvedValue([]), setPlanAccounts: jest.fn().mockResolvedValue(undefined), - listAccounts: jest.fn().mockResolvedValue([]) + listAccounts: jest.fn().mockResolvedValue([]), + getAccount: jest.fn().mockResolvedValue(null) })); // Mock state module @@ -1264,6 +1265,106 @@ describe('Plans Module', () => { expect((document.getElementById('plan-id') as HTMLInputElement).value).toBe(''); }); + + // #770: Purchase Configuration section should be prefilled from the + // selected commitment when exactly one commitment is passed in the snapshot. + describe('prefill from single selected commitment (#770)', () => { + const fixture: api.Recommendation = { + id: 'rec-770', + provider: 'aws', + service: 'ec2', + region: 'us-east-1', + resource_type: 't3.medium', + count: 1, + term: 1, + payment: 'partial-upfront', + upfront_cost: 100, + monthly_cost: 20, + savings: 15, + selected: true, + purchased: false, + cloud_account_id: 'acct-uuid-123', + }; + + test('prefills provider and service selects', () => { + openCreatePlanModal([fixture]); + + expect((document.getElementById('plan-provider') as HTMLSelectElement).value).toBe('aws'); + expect((document.getElementById('plan-service') as HTMLSelectElement).value).toBe('ec2'); + }); + + test('prefills term and payment selects', () => { + openCreatePlanModal([fixture]); + + expect((document.getElementById('plan-term') as HTMLSelectElement).value).toBe('1'); + expect((document.getElementById('plan-payment') as HTMLSelectElement).value).toBe('partial-upfront'); + }); + + test('calls populateTermSelect and populatePaymentSelect with provider+service', () => { + openCreatePlanModal([fixture]); + + expect(populateTermSelect).toHaveBeenCalledWith( + expect.any(HTMLSelectElement), 'aws', 'ec2' + ); + expect(populatePaymentSelect).toHaveBeenCalledWith( + expect.any(HTMLSelectElement), 'aws', 'ec2' + ); + }); + + test('fetches account by cloud_account_id to prefill chip', async () => { + (api.getAccount as jest.Mock).mockResolvedValueOnce({ + id: 'acct-uuid-123', + name: 'Prod AWS', + external_id: '123456789012', + }); + + openCreatePlanModal([fixture]); + + // Wait for the async prefillAccountChipFromId to settle + await Promise.resolve(); + await Promise.resolve(); + + expect(api.getAccount).toHaveBeenCalledWith('acct-uuid-123'); + const chips = document.getElementById('plan-accounts-selected'); + expect(chips?.textContent).toContain('Prod AWS'); + const hiddenIds = (document.getElementById('plan-account-ids') as HTMLInputElement).value; + expect(hiddenIds).toContain('acct-uuid-123'); + }); + + test('does not throw and leaves account section empty when getAccount fails', async () => { + (api.getAccount as jest.Mock).mockRejectedValueOnce(new Error('network error')); + + openCreatePlanModal([fixture]); + + await Promise.resolve(); + await Promise.resolve(); + + // Account section should be empty -- no chip added + const hiddenIds = (document.getElementById('plan-account-ids') as HTMLInputElement).value; + expect(hiddenIds).toBe(''); + }); + + test('skips prefill when no cloud_account_id present', async () => { + const noAccount: api.Recommendation = { ...fixture, cloud_account_id: undefined }; + openCreatePlanModal([noAccount]); + + await Promise.resolve(); + + expect(api.getAccount).not.toHaveBeenCalled(); + }); + + test('does not prefill when snapshot has more than one commitment', () => { + const second: api.Recommendation = { ...fixture, id: 'rec-771', service: 'rds' }; + openCreatePlanModal([fixture, second]); + + // populateTermSelect is called by setupRampScheduleHandlers / updateCommitmentOptions + // but NOT by prefillPurchaseConfigFromCommitment (which only runs for length===1) + // The provider/service select should NOT be forced to either rec's values + // We assert that the service select was not forced to 'ec2' alone + // (a multi-commitment plan requires manual selection) + expect(api.getAccount).not.toHaveBeenCalled(); + }); + }); }); describe('openNewPlanModal', () => { diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 39a9cb395..b2f0f40e3 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -822,6 +822,58 @@ async function setupPlanAccountsSection(planId?: string): Promise { } } +/** + * Prefill the Purchase Configuration section (provider / service / term / + * payment) from a single selected commitment. Called after form.reset() so + * the defaults are already in place; each field is still editable. (#770) + */ +function prefillPurchaseConfigFromCommitment(rec: api.Recommendation): void { + const providerSelect = document.getElementById('plan-provider') as HTMLSelectElement | null; + const serviceSelect = document.getElementById('plan-service') as HTMLSelectElement | null; + const termSelect = document.getElementById('plan-term') as HTMLSelectElement | null; + const paymentSelect = document.getElementById('plan-payment') as HTMLSelectElement | null; + + if (!providerSelect || !serviceSelect || !termSelect || !paymentSelect) return; + + const provider = rec.provider ?? ''; + const service = rec.service ?? ''; + + if (provider) providerSelect.value = provider; + if (service) serviceSelect.value = service; + + // Repopulate term/payment options for the chosen provider+service, then + // apply the commitment's own values so the dropdowns are consistent. + if (provider && service) { + populateTermSelect(termSelect, provider, service); + populatePaymentSelect(paymentSelect, provider, service); + } + + if (rec.term != null) termSelect.value = String(rec.term); + const normalizedPayment = rec.payment ? normalizePaymentValue(rec.payment, provider) : ''; + if (normalizedPayment) paymentSelect.value = normalizedPayment; +} + +/** + * Fetch the account with the given internal UUID and add it as a pre-selected + * chip in the plan modal accounts section. Runs after setupPlanAccountsSection + * has reset the chip list for the create flow. Silently no-ops on failure so + * the user can still pick the account manually. (#770) + */ +async function prefillAccountChipFromId(accountId: string): Promise { + try { + const account = await api.getAccount(accountId); + // Guard: only add if the chip is not already present (e.g. a concurrent + // edit flow somehow set it) and the account record is usable. + if (account && account.id && !planSelectedAccounts.some(a => a.id === account.id)) { + planSelectedAccounts.push({ id: account.id, name: account.name, external_id: account.external_id }); + renderPlanAccountChips(); + updatePlanAccountIdsField(); + } + } catch { + // Non-critical: the user can still search and add the account manually. + } +} + /** * Open create plan modal with selected recommendations. * @@ -847,6 +899,13 @@ export function openCreatePlanModal(snapshot?: readonly api.Recommendation[]): v (document.getElementById('plan-id') as HTMLInputElement).value = ''; (document.getElementById('plan-form') as HTMLFormElement | null)?.reset(); + // When exactly one commitment is selected, prefill the Purchase + // Configuration fields so the user does not have to re-enter them. + // Fields are still fully editable after prefill. (#770) + if (pendingPlanRecommendations.length === 1) { + prefillPurchaseConfigFromCommitment(pendingPlanRecommendations[0]!); + } + // Set up ramp schedule change handlers for dynamic plan name setupRampScheduleHandlers(); @@ -856,7 +915,16 @@ export function openCreatePlanModal(snapshot?: readonly api.Recommendation[]): v // Generate initial plan name updatePlanNameFromSchedule(); + // setupPlanAccountsSection clears planSelectedAccounts and re-renders. + // When a single commitment carries a cloud_account_id, we look up that + // account after the section has reset and add it as a pre-selected chip. void setupPlanAccountsSection(); + if (pendingPlanRecommendations.length === 1) { + const accountId = pendingPlanRecommendations[0]!.cloud_account_id; + if (accountId) { + void prefillAccountChipFromId(accountId); + } + } const planModal = document.getElementById('plan-modal'); if (planModal) { From 220f53eec4a110e4c83cc724b5645bbe4fcda3ad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 14:44:12 +0200 Subject: [PATCH 2/2] fix(plans): guard prefillAccountChipFromId against stale-modal promise resolution Introduce a module-level planModalSession counter that is incremented each time openCreatePlanModal opens (stamping a new session). prefillAccountChipFromId now accepts the session value captured at call time and silently discards the api.getAccount result if the counter has advanced (i.e. the modal was closed and reopened while the promise was in-flight), preventing wrong-modal pollution of planSelectedAccounts. Addresses #770 CR Major finding. Add a focused regression test: open modal, issue slow getAccount, close, reopen, resolve the stale promise, assert no chip was added to the new session. --- frontend/src/__tests__/plans.test.ts | 33 ++++++++++++++++++++++++++++ frontend/src/plans.ts | 25 +++++++++++++++++++-- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index be996d928..afe3ebda3 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -1353,6 +1353,39 @@ describe('Plans Module', () => { expect(api.getAccount).not.toHaveBeenCalled(); }); + test('discards stale getAccount result when modal is reopened before promise resolves', async () => { + // Simulate a slow first getAccount call that resolves after the modal + // is closed and a new modal session starts (the race condition fixed + // by the planModalSession guard — #770 CR Major). + let resolveFirstCall!: (value: api.CloudAccount) => void; + const firstCallPromise = new Promise(resolve => { + resolveFirstCall = resolve; + }); + + (api.getAccount as jest.Mock) + .mockReturnValueOnce(firstCallPromise) // first open: hangs + .mockResolvedValueOnce(null); // second open: no account + + // First modal open with cloud_account_id + openCreatePlanModal([fixture]); + + // Close and reopen — this increments planModalSession + closePlanModal(); + const noAccount: api.Recommendation = { ...fixture, cloud_account_id: undefined }; + openCreatePlanModal([noAccount]); + + // Now resolve the stale first promise — should be discarded + resolveFirstCall({ id: 'acct-uuid-123', name: 'Prod AWS', external_id: '123456789012' } as api.CloudAccount); + await Promise.resolve(); + await Promise.resolve(); + + // The stale chip must NOT have been added to the new modal session + const hiddenIds = (document.getElementById('plan-account-ids') as HTMLInputElement).value; + expect(hiddenIds).toBe(''); + const chips = document.getElementById('plan-accounts-selected'); + expect(chips?.textContent).not.toContain('Prod AWS'); + }); + test('does not prefill when snapshot has more than one commitment', () => { const second: api.Recommendation = { ...fixture, id: 'rec-771', service: 'rds' }; openCreatePlanModal([fixture, second]); diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index b2f0f40e3..fc232d5ca 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -701,6 +701,12 @@ export function closePlanModal(): void { // Selected accounts for the plan modal let planSelectedAccounts: Array<{ id: string; name: string; external_id: string }> = []; +// Monotonically incrementing counter scoped to the plan modal lifecycle. +// Incremented each time the create modal opens so that async callbacks +// from a previous session (stale promises) can detect they're out-of-date +// and discard their results rather than mutating state in the new session. +let planModalSession = 0; + /** * Render selected account chips in the plan modal */ @@ -858,10 +864,20 @@ function prefillPurchaseConfigFromCommitment(rec: api.Recommendation): void { * chip in the plan modal accounts section. Runs after setupPlanAccountsSection * has reset the chip list for the create flow. Silently no-ops on failure so * the user can still pick the account manually. (#770) + * + * @param accountId - Internal UUID of the account to prefill. + * @param session - planModalSession value captured at call time. If the + * modal is closed and reopened before this promise resolves, + * the counter will have advanced and the stale result is + * discarded to prevent wrong-modal pollution. (#770 CR) */ -async function prefillAccountChipFromId(accountId: string): Promise { +async function prefillAccountChipFromId(accountId: string, session: number): Promise { try { const account = await api.getAccount(accountId); + // Session guard: discard the result if the modal was closed and reopened + // while this promise was in-flight. planModalSession is incremented on + // each new modal open, so a mismatch means this callback is stale. + if (session !== planModalSession) return; // Guard: only add if the chip is not already present (e.g. a concurrent // edit flow somehow set it) and the account record is usable. if (account && account.id && !planSelectedAccounts.some(a => a.id === account.id)) { @@ -915,6 +931,11 @@ export function openCreatePlanModal(snapshot?: readonly api.Recommendation[]): v // Generate initial plan name updatePlanNameFromSchedule(); + // Stamp a new session so any in-flight prefillAccountChipFromId promise + // from a prior modal open can detect it belongs to a stale session and + // discard its result without mutating planSelectedAccounts. (#770 CR) + planModalSession += 1; + // setupPlanAccountsSection clears planSelectedAccounts and re-renders. // When a single commitment carries a cloud_account_id, we look up that // account after the section has reset and add it as a pre-selected chip. @@ -922,7 +943,7 @@ export function openCreatePlanModal(snapshot?: readonly api.Recommendation[]): v if (pendingPlanRecommendations.length === 1) { const accountId = pendingPlanRecommendations[0]!.cloud_account_id; if (accountId) { - void prefillAccountChipFromId(accountId); + void prefillAccountChipFromId(accountId, planModalSession); } }