Conversation
The Account filter on the Plans page excluded plans whose Target Account
field was left blank (NULL plan_accounts entries), even though an empty
Target Account means "all accounts of this provider".
Root cause: buildListPlansQuery used an INNER JOIN on plan_accounts, so
plans with no rows in that join table were silently dropped.
Fix: replace the JOIN with a WHERE pp.id IN (...) that unions two
sub-queries:
1. Targeted plans: explicitly linked to the given account(s) via
plan_accounts (unchanged behaviour).
2. Universal plans: no plan_accounts rows at all, but whose services
JSONB contains at least one service whose "provider" field matches
the provider of any of the given cloud accounts.
The accountIDs slice is now passed as a single $1 Postgres array
argument (ANY($1::uuid[])), removing the per-ID placeholder loop and
keeping the query shape static.
cristim
added
triaged
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR extends purchase-plan filtering to return both explicitly targeted plans (referenced via ChangesAccount-based purchase plan filtering
Sequence DiagramsequenceDiagram
participant Client
participant ListPurchasePlans
participant buildListPlansQuery
participant Postgres
Client->>ListPurchasePlans: ListPurchasePlans(AccountIDs=[...])
ListPurchasePlans->>buildListPlansQuery: Build filtered query
buildListPlansQuery->>Postgres: SELECT (targeted via plan_accounts UNION universal via services) WHERE uuid[] ANY
Postgres->>Postgres: Targeted: plan_accounts JOIN plan IDs
Postgres->>Postgres: Universal: no plan_accounts + provider match
Postgres-->>ListPurchasePlans: Combined result set
ListPurchasePlans-->>Client: Purchase plans
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 27, 2026
cristim
added a commit
that referenced
this pull request
May 27, 2026
…lans.sql Two minor fixes per CR #743 review: - F3: header comment referenced PR #739 (the now-closed account-filter fix) but the operator-driven cleanup is tracked by issue #742, not #739. Update the reference so future readers find the right issue. - F4: the fan-out cleanup example (option b) attached every cloud account matching the plan's provider, including disabled accounts. Add `ca.enabled = true` to the JOIN so disabled accounts aren't swept into a re-attach pass.
cristim
added a commit
that referenced
this pull request
May 27, 2026
…eation (#743) * feat(api/plans): require non-empty target_accounts on plan creation Eliminates the "universal plans" class — purchase_plans rows with no matching plan_accounts row — that the historical "leave Target Account blank to mean all-accounts-of-this-provider" UX created. Plans now must be tied to at least one cloud_account. Changes: - PlanRequest gains target_accounts ([]string), validated non-empty + per- entry UUID-formed at POST /api/plans. Rejected requests return 400 with a clear message; the store is never touched. - createPlan inserts the plan_accounts rows immediately after CreatePurchasePlan inside the same handler, rolling the plan row back if provider-match validation or the SetPlanAccounts write fails. This keeps the invariant "every purchase_plans row has at least one plan_accounts row" end-to-end so a fresh universal plan can no longer appear via a partial failure. - PUT /api/plans/:id/accounts now rejects an empty account_ids body. Closes the back-door for re-creating a universal plan by clearing the list on an existing plan. - OpenAPI schema marks target_accounts as required with minItems: 1. Tests: - TestHandler_createPlan_RejectsEmptyTargetAccounts (missing field + empty array): both return 400; CreatePurchasePlan never reached. - TestHandler_createPlan_RejectsInvalidTargetAccountUUID: malformed UUID rejected before any DB write. - TestSetPlanAccounts_RejectsEmpty: PUT-with-empty-list returns 400, SetPlanAccounts never reached. - Existing TestHandler_createPlan + TestHandler_HandleRequest_CreatePlan extended to include target_accounts + GetCloudAccount stub for the new provider-match path. - Two router dispatch tests that used empty account_ids switched to a single valid UUID — they were never asserting on body content. Existing universal plans in the DB are NOT touched by this PR — those require operator review (delete / fan-out / manual reassignment). See scripts/list_universal_plans.sql in a follow-up commit for the diagnostic. * fix(frontend/plans): require Target Account in New Purchase Plan modal Matches the new backend contract (POST /api/plans rejects empty target_accounts). Without this change the user could fill in every other plan field and only see the failure as a toast after the POST round-trip landed. Changes: - Modal help text + section heading mark Target Accounts as required (was "Leave empty to target all enabled accounts" — that semantics no longer exists). - savePlan rejects an empty plan-account-ids field with a clear toast before the createPlan/updatePlan call ever fires. - Selected account IDs are stamped onto the request as target_accounts so the backend can validate + persist atomically on POST. The follow-up PUT /api/plans/:id/accounts call is preserved as defence-in- depth for the update flow (which still uses the 2-step write). - New refreshPlanSaveButtonState() disables the Save Plan submit button whenever the chip list is empty and re-enables it as soon as the user adds one. The button title surfaces the reason to mouseover. - CreatePlanRequest.target_accounts added to the API types so the request shape is checked at compile time. Tests: - "rejects submit and never calls createPlan when Target Accounts is empty (universal-plans fix)": empty hidden field → no createPlan call, no setPlanAccounts call, toast surfaces the requirement. - "forwards selected target_accounts on createPlan (universal-plans fix)": multi-account chip list flows through to the request body. - DOM setup gained #plan-accounts-selected, #plan-account-ids, and a submit button so the new code paths have something to drive. - Existing tests in the savePlan describe block now stamp a single UUID in beforeEach; tests that re-open the modal (form.reset() clears the field) call a stampAccountIds() helper to re-stamp before submit. * chore(scripts): add list_universal_plans.sql for operator-driven cleanup Read-only diagnostic for existing universal plans (purchase_plans rows with no matching plan_accounts row). The companion API change rejects new ones, but pre-existing rows from the legacy "leave Target Account blank" UX remain until an operator decides per-plan. The file documents three cleanup options inline so the operator can pick without leaving psql: (a) DELETE the plan if genuinely orphaned (no consumer, no executions) (b) Fan out: insert plan_accounts rows for every enabled cloud_account whose provider matches the plan's services map (c) Manual review per plan via the (now-required-Target-Account) UI No destructive operations live in the script — the operator runs the SELECT, decides per plan, and executes one of the three documented follow-ups by hand. A separate ops-tracking issue captures the broader cleanup plan + scheduling. * fix(api/plans): log rollback-delete failures during create-plan transaction When validatePlanAccountProviders or SetPlanAccounts fails after a successful CreatePurchasePlan, the handler rolls the plan row back to preserve the "every purchase_plans row has at least one plan_accounts row" invariant. The rollback DeletePurchasePlan error was previously discarded, so a transient DB failure could leave a partial plan row behind unnoticed — the exact universal-plan state this PR is trying to eliminate. Wrap the rollback delete in a closure that logs at WARN with the plan ID when it fails, so an operator can clean up manually. The caller still receives the original cause unchanged (no API contract change). Adds a regression test asserting that: - DeletePurchasePlan is called on the rollback path - The user-facing error is the original SetPlanAccounts error wrapped as "accounts: ..." (not the rollback error) - The rollback error message does not leak into the returned error CR #743 finding F1. * fix(api/openapi): scope target_accounts requirement to create-plan only PlanRequest is reused across POST /api/plans, PUT /api/plans/{id}, and PATCH /api/plans/{id}. Adding target_accounts to the shared schema's required array forced the field on every update payload too, which contradicts updatePlan's design (omitting target_accounts on PUT/PATCH means "leave the existing account list unchanged"; the back-door is closed at setPlanAccounts itself by rejecting empty arrays). Move the requirement onto the POST operation only via an inline allOf that composes PlanRequest with `required: [target_accounts]`. The shared schema goes back to `required: [name]`, leaving update payloads unaffected while still advertising the create contract correctly to client codegen and OpenAPI validators. CR #743 finding F2. * docs(scripts): fix issue ref + add enabled filter in list_universal_plans.sql Two minor fixes per CR #743 review: - F3: header comment referenced PR #739 (the now-closed account-filter fix) but the operator-driven cleanup is tracked by issue #742, not #739. Update the reference so future readers find the right issue. - F4: the fan-out cleanup example (option b) attached every cloud account matching the plan's provider, including disabled accounts. Add `ca.enabled = true` to the JOIN so disabled accounts aren't swept into a re-attach pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #705 follow-up (PR #715 made Account filter functional but verification on QA rows 325-326 still failed: plans with no target_account were excluded entirely).
Root cause:
buildListPlansQueryininternal/config/store_postgres.gousedINNER JOIN plan_accounts ... WHERE pa.account_id IN (...). Plans with no rows inplan_accounts(universal plans created by leaving Target Account blank) were silently excluded because the INNER JOIN matched nothing.Fix
Replaced the JOIN with
WHERE pp.id IN (UNION of two subqueries):SELECT pa.plan_id FROM plan_accounts pa WHERE pa.account_id = ANY($1::uuid[])— unchanged semantics.plan_accountsAND whoseservicesJSONB contains at least one service whose provider matches one of the selected accounts' providers. This matches the "Target Account blank = all accounts of this provider" contract from the New Purchase Plan modal.Per-ID placeholder loop replaced with
ANY($1::uuid[])array argument — consistent with the rest of the store.Test plan
TestPGXMock_ListPurchasePlans_AccountFilterverifies array arg and row returnTestPGXMock_ListPurchasePlans_AccountFilterErrorverifies error propagationgo test ./...: 4845 passSummary by CodeRabbit
New Features
Tests