Repository navigation
fix(opportunities): prefill Create Purchase Plan modal from selected commitment - #778
Conversation
…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.
cristim
added
triaged
|
Warning Review limit reached
More reviews will be available in 11 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements prefilling of the "Create Purchase Plan" modal's purchase configuration and account fields when exactly one commitment is selected. Two new helper functions populate provider/service/term/payment dropdowns synchronously and fetch and add account chips asynchronously. ChangesCreate Purchase Plan Modal Prefill
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/__tests__/plans.test.ts (1)
1356-1366: ⚡ Quick winStrengthen the multi-commitment no-prefill regression assertion.
Line 1356 currently verifies only that
getAccountis not called. It doesn’t assert that provider/service/term/payment were left unchanged. Add explicit field assertions so this test actually guards “no prefill” behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/plans.test.ts` around lines 1356 - 1366, The test 'does not prefill when snapshot has more than one commitment' currently only asserts api.getAccount was not called; update it to explicitly assert that provider, service, term and payment fields were left unchanged after openCreatePlanModal([fixture, second]) — i.e., verify the provider/service select value is still the initial/default (not forced to fixture.service 'ec2'), the term select was not changed by prefillPurchaseConfigFromCommitment, and the payment/method field remains the original/default; locate the test and add assertions referencing openCreatePlanModal, populateTermSelect/updateCommitmentOptions/prefillPurchaseConfigFromCommitment flow and the fixture/second recommendations to confirm no fields were overwritten.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/plans.ts`:
- Around line 862-871: The async prefillAccountChipFromId function can mutate
planSelectedAccounts after a modal has been closed and reopened, so add a
session check to ignore stale responses: introduce a short-lived session
token/counter checked both when starting the async fetch and before mutating
state, or accept a sessionId parameter from the callers that open the modal (the
callers that launch the prefill should pass the current modal/session id); in
prefillAccountChipFromId verify the returned account's sessionId matches the
current session before pushing to planSelectedAccounts and calling
renderPlanAccountChips() and updatePlanAccountIdsField() so stale promises
cannot affect a new modal session.
---
Nitpick comments:
In `@frontend/src/__tests__/plans.test.ts`:
- Around line 1356-1366: The test 'does not prefill when snapshot has more than
one commitment' currently only asserts api.getAccount was not called; update it
to explicitly assert that provider, service, term and payment fields were left
unchanged after openCreatePlanModal([fixture, second]) — i.e., verify the
provider/service select value is still the initial/default (not forced to
fixture.service 'ec2'), the term select was not changed by
prefillPurchaseConfigFromCommitment, and the payment/method field remains the
original/default; locate the test and add assertions referencing
openCreatePlanModal,
populateTermSelect/updateCommitmentOptions/prefillPurchaseConfigFromCommitment
flow and the fixture/second recommendations to confirm no fields were
overwritten.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 424523bc-ac9a-4021-96b6-f262dc219287
📒 Files selected for processing (2)
frontend/src/__tests__/plans.test.tsfrontend/src/plans.ts
…e 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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…homogeneous commitments (closes #898) (#899) When 2+ commitments sharing the same provider/service/term/payment are selected and the user clicks "Plan from N selected", the Create Purchase Plan modal now prefills the Purchase Configuration from the shared values instead of showing defaults. The "Plan from N selected" button only enables on a homogeneous selection, so the first commitment is a valid representative. The account chip is prefilled only when every selected commitment carries the same cloud_account_id; a multi-account selection leaves it empty for the user to fill, since provider/service/term/payment homogeneity does not imply a single account. Extends the single-selection prefill from #770/#778 (QA finding 6.7).
1 participant
Summary
QA 6.5: Selecting one commitment in the Opportunities table and clicking "Plan from 1 selected" opened the Create Purchase Plan modal with default values. Now the modal's Purchase Configuration section is prefilled from the selected commitment.
Fix
Added
prefillPurchaseConfigFromCommitmentandprefillAccountChipFromIdhelpers.openCreatePlanModalcalls both whensnapshot.length === 1. Prefill is skipped for multi-commitment snapshots; all fields remain editable after prefill.Fields prefilled
aws/azure/gcp) -- set on the provider select.ec2/rds/compute/ etc.) -- set on the service select.1/3) --populateTermSelectcalled first, then value set.partial-upfront) --normalizePaymentValueapplied,populatePaymentSelectcalled first.api.getAccount(cloud_account_id)and added as a chip; silently skipped on failure or missing ID.Files changed
frontend/src/plans.tsfrontend/src/__tests__/plans.test.tsTest plan
prefill from single selected commitment (#770).Closes #770.
Summary by CodeRabbit