fix: wire CreateDefaultBillingProfile through on v1 marketplace app install - #4783
Open
vsengar-79 wants to merge 5 commits into
Open
fix: wire CreateDefaultBillingProfile through on v1 marketplace app install#4783vsengar-79 wants to merge 5 commits into
vsengar-79 wants to merge 5 commits into
Conversation
Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Refactor billing profile creation to be transactional and remove the createBillingProfile function. Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Implement default billing profile creation for installed apps, including Stripe and Sandbox types. Add error handling for unknown app types and incomplete implementations. Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughDefault billing profile provisioning is centralized in ChangesBilling profile provisioning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InstallHandler
participant billingprofile.CreateDefault
participant billing.Service
participant StripeAppService
InstallHandler->>billingprofile.CreateDefault: provision installed app
billingprofile.CreateDefault->>billing.Service: provision or inspect default profile
billingprofile.CreateDefault->>StripeAppService: retrieve supplier contract for Stripe
billingprofile.CreateDefault->>billing.Service: create billing profile
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+36
to
+38
| // TODO: Implement custom invoicing billing profile creation | ||
| return nil, nil | ||
| default: |
Contributor
There was a problem hiding this comment.
When a Custom Invoicing app is installed with createBillingProfile enabled or omitted, this branch returns success without creating a billing profile, causing the installation to complete while the documented flag remains ineffective and the response reports no default capabilities.
Knowledge Base Used: App Framework (Marketplace Integrations)
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/app/billingprofile/provision.go
Line: 36-38
Comment:
**Custom invoicing skips provisioning**
When a Custom Invoicing app is installed with `createBillingProfile` enabled or omitted, this branch returns success without creating a billing profile, causing the installation to complete while the documented flag remains ineffective and the response reports no default capabilities.
**Knowledge Base Used:** [App Framework (Marketplace Integrations)](https://app.greptile.com/openmeter/-/custom-context/knowledge-base/openmeterio/openmeter/-/docs/app.md)
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/app/httpdriver/marketplace.go (1)
132-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: the same three-line closure appears three times, and the comment oversells it a bit.
The closure is pure partial application — it doesn't itself open a transaction; the install service does, and it just runs inside that boundary. Something like "provisioning runs inside the install transaction" reads more accurately, and a small shared binder in the
billingprofilepackage would collapse the triplication. Totally fine to leave as-is if you'd rather keep the wiring explicit at each call site.
openmeter/app/httpdriver/marketplace.go#L132-L136: reword the comment and optionally swap in the shared binder.openmeter/app/httpdriver/marketplace.go#L204-L208: apply the same change.api/v3/handlers/apps/install_app.go#L115-L117: apply the same change.♻️ Sketch of the shared binder
// openmeter/app/billingprofile/provision.go // DefaultFn binds the services needed by CreateDefault so handlers can hand the // result straight to app.InstallAppV3Input.CreateDefaultBillingProfileFn. func DefaultFn(billingService billing.Service, stripeAppService appstripe.Service) func(context.Context, app.App) ([]app.CapabilityType, error) { return func(ctx context.Context, installedApp app.App) ([]app.CapabilityType, error) { return CreateDefault(ctx, billingService, stripeAppService, installedApp) } }- // make the billing profile provisioning transactional - request.CreateDefaultBillingProfileFn = func(ctx context.Context, installedApp app.App) ([]app.CapabilityType, error) { - return billingprofile.CreateDefault(ctx, h.billingService, h.stripeAppService, installedApp) - } + // Runs inside the install transaction opened by the app service. + request.CreateDefaultBillingProfileFn = billingprofile.DefaultFn(h.billingService, h.stripeAppService)🤖 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 `@openmeter/app/httpdriver/marketplace.go` around lines 132 - 136, The repeated billing-profile closure should be centralized as an optional shared binder, and its comments should accurately state that provisioning runs inside the install transaction. Add the binder in the billingprofile package, then replace the closures at openmeter/app/httpdriver/marketplace.go lines 132-136 and 204-208 and api/v3/handlers/apps/install_app.go lines 115-117 with it; update each associated comment accordingly.
🤖 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.
Nitpick comments:
In `@openmeter/app/httpdriver/marketplace.go`:
- Around line 132-136: The repeated billing-profile closure should be
centralized as an optional shared binder, and its comments should accurately
state that provisioning runs inside the install transaction. Add the binder in
the billingprofile package, then replace the closures at
openmeter/app/httpdriver/marketplace.go lines 132-136 and 204-208 and
api/v3/handlers/apps/install_app.go lines 115-117 with it; update each
associated comment accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b495aca-6013-42c6-a764-5585fa6c11c4
📒 Files selected for processing (3)
api/v3/handlers/apps/install_app.goopenmeter/app/billingprofile/provision.goopenmeter/app/httpdriver/marketplace.go
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.
POST /api/v1/marketplace/listings/{type}/install (and the API-key variant) accepted a createBillingProfile flag but silently ignored it — the flag was decoded into a dead field on the HTTP request wrapper struct instead of InstallAppV3Input.CreateDefaultBillingProfile, and CreateDefaultBillingProfileFn was never set at all, so no billing profile was ever provisioned regardless of the flag's value.
Changes:
Extracted the billing-profile provisioning logic (createBillingProfile/makeStripeDefaultBillingApp) out of api/v3/handlers/apps/install_app.go into a new shared package, openmeter/app/billingprofile, so both the v1 and v3 install handlers use identical provisioning behavior instead of duplicating it.
Fixed openmeter/app/httpdriver/marketplace.go (MarketplaceAppInstall and MarketplaceAppAPIKeyInstall) to set CreateDefaultBillingProfile on InstallAppV3Input and wire CreateDefaultBillingProfileFn via the new shared package, matching the v3 handler's pattern.
Removed the now-redundant CreateBillingProfile field from the v1 request wrapper structs.
No API contract changes — this only fixes existing, already-documented request/response behavior that wasn't functioning.
Summary by CodeRabbit
Greptile Summary
This PR activates billing-profile provisioning for v1 marketplace installs and shares the existing provisioning implementation across API versions.
openmeter/app/billingprofile.createBillingProfileintoInstallAppV3Inputfor both v1 installation methods.Confidence Score: 3/5
The Custom Invoicing path should be fixed before merging because an enabled or defaulted createBillingProfile request still succeeds without creating a billing profile.
The newly activated v1 callback reaches a supported Custom Invoicing branch that returns success without performing the requested provisioning, leaving the endpoint's documented flag silently ineffective for that app type.
Files Needing Attention: openmeter/app/billingprofile/provision.go, openmeter/app/httpdriver/marketplace.go
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[v1 marketplace install] --> B[InstallAppV3Input] B --> C[AppService.InstallApp transaction] C --> D{Create billing profile?} D -->|No| E[Return installed app] D -->|Yes| F[billingprofile.CreateDefault] F -->|Stripe| G[Create Stripe profile] F -->|Sandbox| H[Provision default profile] F -->|Custom Invoicing| I[Return success without profile]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Add default billing profile provisioning..." | Re-trigger Greptile
Context used: