Skip to content

feat(billing): move plan writes to AdminService and add ListAllPlans - #1830

Merged
rohilsurana merged 34 commits into
mainfrom
feat/billing-plan-api
Aug 7, 2026
Merged

feat(billing): move plan writes to AdminService and add ListAllPlans#1830
rohilsurana merged 34 commits into
mainfrom
feat/billing-plan-api

Conversation

@rohilsurana

@rohilsurana rohilsurana commented Jul 31, 2026

Copy link
Copy Markdown
Member

What

Wire the frontier side of the plan admin APIs from raystack/proton#496.

  • Move CreatePlan and UpdatePlan to AdminService (super user only). They were on FrontierService before.
  • Add ListAllPlans on AdminService. It lists every plan, including disabled ones. An empty state returns all plans; a set state filters to it.
  • UpdatePlan is implemented (it returned Unimplemented before) and takes a dedicated UpdatePlanRequestBody (title, description, on_start_credits, trial_days, state, metadata). It is a full replacement of those fields. It does not touch interval, products, or name. Products are managed through CreatePlan's upsert.
  • CreatePlan carries the plan state.
  • ListPlans and GetPlan stay on FrontierService, unchanged. ListPlans returns active plans only.
  • Plan state is required on the write APIs (active or disabled), so a plan can never be stored with an empty state.

How

  • Pin proton to the branch commit that carries these proto changes and regenerate.
  • The same ConnectHandler serves both services, so the existing CreatePlan method now satisfies AdminService. UpdatePlan and ListAllPlans are added to it.
  • The authz interceptor gates the three AdminService plan RPCs behind IsSuperUser, and the old FrontierService plan entries are removed. Any endpoint not listed is denied by default, so these entries are required.
  • ListAllPlans passes a plan.StateAll sentinel to the repository so an empty state lists plans in every state. ListPlans still defaults to active only.

Review fixes (from an independent review)

  • UpdatePlan no longer reuses the create body, so a plan update no longer requires a decorative interval that the handler ignored.
  • The plan repository no longer coerces an empty state to active on update, so an update can no longer silently reactivate a disabled plan. UpdatePlanRequest.body is required.
  • UpdatePlan maps a missing plan to NotFound and an invalid id/name to InvalidArgument instead of an opaque 500.
  • New plans default to active on create, so the create and seed paths can never store an empty state.

Depends on

Related fixes

Testing

  • Unit tests for the UpdatePlan and ListAllPlans handlers.
  • e2e coverage: create, update (disable), missing-plan 404, and ListAllPlans vs ListPlans visibility of a disabled plan.
  • build, vet, gofmt, and golangci-lint are green.

@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview Aug 7, 2026 5:43am

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Added plan states for active and inactive plans.
    • Added plan update functionality and administrative listing of all plans with optional state filters.
    • Plan creation now defaults to active when no state is provided.
  • Bug Fixes
    • Prevented new checkouts and plan changes to inactive plans, with clearer errors.
    • Improved plan state preservation during updates.
    • Corrected product behavior data returned with plans.
  • Access Control
    • Restricted plan creation, updates, and all-plan listings to administrators.

Walkthrough

Changes

Plan lifecycle management

Layer / File(s) Summary
Plan state model and persistence
billing/plan/*, internal/store/postgres/billing_plan_repository.go
Adds active and inactive states, inactive-state detection, plan updates, state preservation, state-aware queries, and product behavior selection.
Inactive-plan billing validation
billing/checkout/*, billing/subscription/*
Rejects inactive plans for new checkout sessions and target plan changes while preserving inactive-plan resolution and direct application paths.
Administrative plan API and authorization
internal/api/v1beta1connect/*, pkg/server/connect_interceptors/authorization.go
Adds plan updates, all-state listing, state propagation, error mapping, mocks, tests, and administrative authorization.
Lifecycle regression coverage
test/e2e/regression/billing_test.go
Covers plan creation, updates, disabling, not-found handling, state-filtered listings, and active plans used by entitlement tests.

Proto dependency update

Layer / File(s) Summary
Proto target commit
Makefile
Updates the PROTON_COMMIT value used by the proto target.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested reviewers: whoabhisheksah, amangit07

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

coveralls commented Aug 3, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 31151483622

Coverage increased (+0.2%) to 47.945%

Details

  • Coverage increased (+0.2%) from the base build.
  • Patch coverage: 48 uncovered changes across 5 files (77 of 125 lines covered, 61.6%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
billing/plan/service.go 24 0 0.0%
internal/store/postgres/billing_plan_repository.go 21 12 57.14%
pkg/server/connect_interceptors/authorization.go 9 0 0.0%
billing/subscription/service.go 7 3 42.86%
internal/api/v1beta1connect/billing_plan.go 50 48 96.0%
Total (9 files) 125 77 61.6%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 39541
Covered Lines: 18958
Line Coverage: 47.95%
Coverage Strength: 15.41 hits per line

💛 - Coveralls

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 36ed8dac-defb-4d0b-89e4-deba6b82920a

📥 Commits

Reviewing files that changed from the base of the PR and between ad5e9a5 and 56fc929.

⛔ Files ignored due to path filters (4)
  • proto/v1beta1/admin.pb.go is excluded by !**/*.pb.go, !proto/**
  • proto/v1beta1/frontier.pb.go is excluded by !**/*.pb.go, !proto/**
  • proto/v1beta1/frontierv1beta1connect/admin.connect.go is excluded by !proto/**
  • proto/v1beta1/frontierv1beta1connect/frontier.connect.go is excluded by !proto/**
📒 Files selected for processing (19)
  • Makefile
  • billing/checkout/checkout.go
  • billing/checkout/service.go
  • billing/plan/plan.go
  • billing/plan/plan_test.go
  • billing/plan/service.go
  • billing/subscription/service.go
  • billing/subscription/service_test.go
  • billing/subscription/subscription.go
  • internal/api/v1beta1connect/billing_checkout.go
  • internal/api/v1beta1connect/billing_plan.go
  • internal/api/v1beta1connect/billing_plan_test.go
  • internal/api/v1beta1connect/billing_subscription.go
  • internal/api/v1beta1connect/errors.go
  • internal/api/v1beta1connect/interfaces.go
  • internal/api/v1beta1connect/mocks/plan_service.go
  • internal/store/postgres/billing_plan_repository.go
  • pkg/server/connect_interceptors/authorization.go
  • test/e2e/regression/billing_test.go

Comment thread test/e2e/regression/billing_test.go Outdated
Comment thread test/e2e/regression/billing_test.go
@AmanGIT07

AmanGIT07 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

A few findings from a review pass, most severe first:

  1. checkout.Service.Apply skips the new inactive-plan check. Create (billing/checkout/service.go:253) and subscription ChangePlan now reject inactive plans, but Apply fetches the plan (billing/checkout/service.go:883) and subscribes without the check. Two paths reach it: AdminService/DelegatedCheckout, and org auto-onboarding when the billing default_plan points at a now-inactive plan (core/event/service.go:152-166). Also, DelegatedCheckout wraps every Apply error as CodeInternal, so it needs an ErrPlanInactive mapping once the guard is added.

  2. Rows with state='' can no longer heal. Before this PR, plans created through the API got state='' — the old handler never set state, and the repo insert always writes the column, so the SQL default never applies. The ''→active coercion in UpdateByName was the only thing that fixed those rows on re-seed, and it is removed here; the new UpsertPlans fallback copies the existing '' back on every seed run. Such a plan is hidden from ListPlans, but IsInactive() is false, so it can still be subscribed to. A one-line data migration would close this: UPDATE billing_plans SET state='active' WHERE state='';

  3. UpdatePlan wipes omitted fields. on_start_credits, trial_days, and metadata are plain proto3 scalars with no presence, so leaving one out writes zero/empty to the DB. The full-write behavior is documented, but the PR's own e2e update step omits these fields and would clear them on any plan that had real values (the fixture happens to have them at zero). Marking the fields optional in proton would keep partial updates safe while the API is new.

  4. plan.StateAll puts a trap in front of every internal caller. Empty state means "active" in the repository (defaulted separately in List and ListWithProducts, which can drift apart) but means "all" in the ListAllPlans handler. Any planService.List lookup by ID must remember to pass State: plan.StateAll or it silently drops retired plans — this PR had to patch findPlanByStripeSubscription and findPlanByStripePhase for exactly that reason. Cleaner: drop the repository default and have each caller pass the state it wants, so Filter.State == "" genuinely means "no filter". There are only four List call sites today.

  5. Three behavior changes ship without tests: the checkout inactive-plan rejection (the same check in subscription ChangePlan has a test; the checkout path does not), the UpsertPlans state-preservation fallback, and the behavior column fix in ListWithProducts.

  6. UpdatePlan re-fetches what it already has. UpdateByName returns the updated row (.Returning(&Plan{})), but the service discards it and calls GetByID again, which re-runs product enrichment (two queries per product). The update never touches products, so the returned row plus the already-loaded existing.Products avoids the second fetch.

  7. ErrPlanInactive is declared three times with the same text (billing/checkout/checkout.go:28, billing/subscription/subscription.go:16, internal/api/v1beta1connect/errors.go:52), and each copy needs its own errors.Is branch in the handlers. One sentinel in billing/plan would cover all paths. The wrap message also repeats the sentinel's text, so the final error reads the same sentence twice.

  8. ListAllPlans duplicates the ListPlans handler body except for the filter; a shared helper would keep the two endpoints from drifting.

@rohilsurana
rohilsurana force-pushed the feat/billing-plan-api branch from 0d9441a to b5228df Compare August 6, 2026 07:23
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@rohilsurana
rohilsurana merged commit 2486677 into main Aug 7, 2026
8 checks passed
@rohilsurana
rohilsurana deleted the feat/billing-plan-api branch August 7, 2026 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(billing): plan product listings return the product title as its behavior

4 participants