Fix/settlement mode api#4470
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional subscription SettlementMode to API/specs and propagates it through handlers and services; introduces a FeatureGateChecker, middleware, config, and wiring; enforces credits gating in workflow/reconciler; updates router/server wiring and tests. ChangesSettlement Mode Override & Feature Gate System
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
rolosp
added
release-note/ignore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
openmeter/productcatalog/subscription/service/create_test.go (1)
56-56: ⚡ Quick winUse
t.Context()in tests instead ofcontext.Background().At Line 56 and Line 109, prefer
ctx := t.Context()so cancellation/lifecycle stays tied to the test harness.As per coding guidelines: “In tests, prefer
t.Context()when atesting.Tortesting.TBis available instead of introducingcontext.Background(), to keep cancellation and test-scoped lifecycle tied to the test harness.”Also applies to: 109-109
🤖 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/productcatalog/subscription/service/create_test.go` at line 56, Replace the use of context.Background() in the test file by tying the context to the test harness: change occurrences of "ctx := context.Background()" to "ctx := t.Context()" inside the test functions (where t is *testing.T) so the test-scoped cancellation/lifecycle is respected; update both places in create_test.go where the ctx variable is created.e2e/productcatalog_test.go (1)
1342-1346: ⚡ Quick winAssert structured 400 payload here too (not only raw body text).
Nice coverage addition. To make failures less brittle, also assert
ApplicationproblemJSON400is non-nil and check.Detail(same pattern used in the nearby settlement-mode tests).Also applies to: 1364-1368
🤖 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 `@e2e/productcatalog_test.go` around lines 1342 - 1346, The test currently only asserts raw 400 body text after calling client.CreateSubscriptionWithResponse(ctx, create); enhance it to also decode/assert the structured error: check that res.JSON400 (or the generated field ApplicationproblemJSON400 from CreateSubscriptionWithResponse) is non-nil and assert its .Detail contains the expected message (mirror the pattern used in the settlement-mode tests); i.e., after verifying StatusCode()==400, assert ApplicationproblemJSON400 != nil and assert ApplicationproblemJSON400.Detail contains "credits are not enabled on this deployment of OpenMeter".openmeter/productcatalog/subscription/service/change_test.go (1)
415-415: ⚡ Quick winPrefer
t.Context()in these new subtests.At Line 415 and Line 464, switch from
context.Background()tot.Context()to keep request lifetime bound to the test.As per coding guidelines: “In tests, prefer
t.Context()when atesting.Tortesting.TBis available instead of introducingcontext.Background(), to keep cancellation and test-scoped lifecycle tied to the test harness.”Also applies to: 464-464
🤖 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/productcatalog/subscription/service/change_test.go` at line 415, Replace uses of context.Background() in the subtests with t.Context() so the test-scoped context is used; specifically update the ctx variable initialization inside the subtests (the lines where ctx := context.Background() appears) to use the provided testing.T's context (t.Context()) so request lifetime is bound to the test harness and cancellation signals propagate. Ensure this change is applied at both occurrences noted in the test (the ctx declarations around line 415 and line 464 in change_test.go).
🤖 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 `@api/v3/handlers/subscriptions/change.go`:
- Around line 54-57: In the ChangeSubscription handler, the error wrap for the
call to h.isCreditsEnabled(ns) incorrectly says "failed to create subscription";
update the fmt.Errorf call to a correct message such as "failed to change
subscription: %w" (or "failed to determine credits enabled for namespace: %w")
where the current fmt.Errorf is invoked returning ChangeSubscriptionRequest{},
referencing the isCreditsEnabled call and the fmt.Errorf location so logs
reflect the actual operation.
In `@openmeter/productcatalog/subscription/http/change.go`:
- Around line 126-129: The error message returned when h.isCreditsEnabled(ns)
fails is inconsistent: it currently says "failed to create subscription" but the
matching branch uses "failed to change subscription"; update the error returned
in the creditEnabled check inside ChangeSubscriptionRequest flow to use the same
message ("failed to change subscription") so both branches are consistent—locate
the call to h.isCreditsEnabled in the change subscription handler (the block
that returns ChangeSubscriptionRequest{}) and replace the fmt.Errorf message
accordingly.
---
Nitpick comments:
In `@e2e/productcatalog_test.go`:
- Around line 1342-1346: The test currently only asserts raw 400 body text after
calling client.CreateSubscriptionWithResponse(ctx, create); enhance it to also
decode/assert the structured error: check that res.JSON400 (or the generated
field ApplicationproblemJSON400 from CreateSubscriptionWithResponse) is non-nil
and assert its .Detail contains the expected message (mirror the pattern used in
the settlement-mode tests); i.e., after verifying StatusCode()==400, assert
ApplicationproblemJSON400 != nil and assert ApplicationproblemJSON400.Detail
contains "credits are not enabled on this deployment of OpenMeter".
In `@openmeter/productcatalog/subscription/service/change_test.go`:
- Line 415: Replace uses of context.Background() in the subtests with
t.Context() so the test-scoped context is used; specifically update the ctx
variable initialization inside the subtests (the lines where ctx :=
context.Background() appears) to use the provided testing.T's context
(t.Context()) so request lifetime is bound to the test harness and cancellation
signals propagate. Ensure this change is applied at both occurrences noted in
the test (the ctx declarations around line 415 and line 464 in change_test.go).
In `@openmeter/productcatalog/subscription/service/create_test.go`:
- Line 56: Replace the use of context.Background() in the test file by tying the
context to the test harness: change occurrences of "ctx := context.Background()"
to "ctx := t.Context()" inside the test functions (where t is *testing.T) so the
test-scoped cancellation/lifecycle is respected; update both places in
create_test.go where the ctx variable is created.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 634b8696-933d-4a24-a9fb-dc556787e23a
⛔ Files ignored due to path filters (7)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlapi/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (17)
api/api.gen.goapi/spec/packages/aip/src/subscriptions/subscription.tspapi/spec/packages/legacy/src/productcatalog/subscription.tspapi/v3/api.gen.goapi/v3/handlers/subscriptions/change.goapi/v3/handlers/subscriptions/create.goapi/v3/handlers/subscriptions/featuregate.goapi/v3/handlers/subscriptions/handler.goapi/v3/server/server.goe2e/productcatalog_test.goopenmeter/productcatalog/subscription/http/change.goopenmeter/productcatalog/subscription/http/create.goopenmeter/productcatalog/subscription/service.goopenmeter/productcatalog/subscription/service/change.goopenmeter/productcatalog/subscription/service/change_test.goopenmeter/productcatalog/subscription/service/create.goopenmeter/productcatalog/subscription/service/create_test.go
Member
|
LGFM please fix the etoe failures |
Member
|
Also did you check if the settmenet mode is exposed on subscription reads for the v3 api? |
GAlexIHU
approved these changes
Jun 2, 2026
GAlexIHU
requested changes
Jun 2, 2026
GAlexIHU
left a comment
GAlexIHU
left a comment
Contributor
There was a problem hiding this comment.
sry, actually if the settlement_mode = credit_only comes from the plan the check is bypassed i think
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/featuregate/featuregate.go (1)
57-59: ⚡ Quick winConsider making the flag registry more maintainable.
The
Keys()method returns a hardcoded list of valid feature flags. When new flags are added to the system, this method needs manual updates, which could be easy to forget. Consider maintaining a package-level registry thatKeys()reads from, or having flag declarations automatically register themselves.That said, the current whitelist approach does force explicit declaration of new flags, which might be intentional.
🤖 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 `@pkg/featuregate/featuregate.go` around lines 57 - 59, The Keys() method on type Flags currently returns a hardcoded slice containing only credits; make the flag registry maintainable by introducing a package-level registry (e.g., a slice variable like registeredFlags) that all flag declarations append to (either via init functions or a RegisterFeatureFlag(func) call) and then have func (f Flags) Keys() return that registry instead of a hardcoded list; update the credits declaration to register itself with registeredFlags or call the RegisterFeatureFlag helper so new flags are automatically included without editing Keys().pkg/featuregate/featuregate_test.go (1)
192-192: 💤 Low valuePrefer
t.Context()overcontext.Background()in tests.These test cases use
context.Background()but run insidet.Run()subtests wheretesting.Tis available. Consider usingt.Context()instead to tie the context lifecycle to the test harness.As per coding guidelines: In tests, prefer
t.Context()when atesting.Tortesting.TBis available instead of introducingcontext.Background().Also applies to: 212-212, 235-235, 247-247, 253-253, 260-260
🤖 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 `@pkg/featuregate/featuregate_test.go` at line 192, Tests call op(context.Background(), ...) inside t.Run subtests; replace those with t.Context() so the test harness manages the context lifecycle. Update the call sites that invoke op(...) (and any other helper functions in the same tests) to pass t.Context() instead of context.Background() — e.g., change op(context.Background(), "req") to op(t.Context(), "req") in the subtests noted (and the similar occurrences at the other line numbers).
🤖 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 `@openmeter/server/router/router.go`:
- Around line 144-146: Update the validation error message to reference the new
field name: change the error returned when checking c.NamespaceDecoder in
router.go (the block that currently returns errors.New("namespace manager is
required")) to a message like "namespace decoder is required" so it matches the
NamespaceDecoder field; locate the conditional that checks NamespaceDecoder and
replace the string literal accordingly.
---
Nitpick comments:
In `@pkg/featuregate/featuregate_test.go`:
- Line 192: Tests call op(context.Background(), ...) inside t.Run subtests;
replace those with t.Context() so the test harness manages the context
lifecycle. Update the call sites that invoke op(...) (and any other helper
functions in the same tests) to pass t.Context() instead of context.Background()
— e.g., change op(context.Background(), "req") to op(t.Context(), "req") in the
subtests noted (and the similar occurrences at the other line numbers).
In `@pkg/featuregate/featuregate.go`:
- Around line 57-59: The Keys() method on type Flags currently returns a
hardcoded slice containing only credits; make the flag registry maintainable by
introducing a package-level registry (e.g., a slice variable like
registeredFlags) that all flag declarations append to (either via init functions
or a RegisterFeatureFlag(func) call) and then have func (f Flags) Keys() return
that registry instead of a hardcoded list; update the credits declaration to
register itself with registeredFlags or call the RegisterFeatureFlag helper so
new flags are automatically included without editing Keys().
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb400b1f-04b0-48ee-b903-808f851feffc
📒 Files selected for processing (43)
api/v3/handlers/subscriptions/change.goapi/v3/handlers/subscriptions/create.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/billing.goapp/common/featuregate.goapp/common/featuregate_test.goapp/config/config.goapp/config/config_test.goapp/config/credits.goapp/config/featuregate.goapp/config/testdata/complete.yamlcmd/billing-worker/wire.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire.gocmd/jobs/internal/wire_gen.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/billing/worker/subscriptionsync/service/base_test.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patch.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patch_test.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/service.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/productcatalog/plan/httpdriver/driver.goopenmeter/productcatalog/plan/httpdriver/featuregate.goopenmeter/productcatalog/plan/httpdriver/plan.goopenmeter/productcatalog/subscription/http/change.goopenmeter/productcatalog/subscription/http/create.goopenmeter/productcatalog/subscription/http/featuregate.goopenmeter/productcatalog/subscription/http/handler.goopenmeter/server/router/plan.goopenmeter/server/router/router.goopenmeter/server/router/subscription.goopenmeter/server/server.goopenmeter/server/server_test.goopenmeter/subscription/workflow/service/subscription.goopenmeter/subscription/workflow/service/subscription_test.gopkg/featuregate/featuregate.gopkg/featuregate/featuregate_test.gotest/billing/subscription_test.gotest/subscription/framework_test.go
💤 Files with no reviewable changes (8)
- openmeter/productcatalog/subscription/http/featuregate.go
- openmeter/productcatalog/plan/httpdriver/featuregate.go
- openmeter/productcatalog/subscription/http/handler.go
- openmeter/productcatalog/plan/httpdriver/driver.go
- api/v3/handlers/subscriptions/create.go
- api/v3/handlers/subscriptions/change.go
- openmeter/productcatalog/subscription/http/change.go
- openmeter/productcatalog/subscription/http/create.go
✅ Files skipped from review due to trivial changes (4)
- app/config/credits.go
- app/common/featuregate_test.go
- cmd/jobs/internal/wire_gen.go
- cmd/server/wire_gen.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/config/config.go (2)
149-151:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate validation is happening for Credits.
Hey, looks like
c.Credits.Validate()is being called twice—once at lines 149-151 (with prefix "credits") and again at lines 173-175 (with prefix "credit"). This will produce duplicate error messages with inconsistent prefixes, which could confuse users. You'll want to remove one of these calls.🔧 Suggested fix
Remove the second validation call:
- if err := c.Credits.Validate(); err != nil { - errs = append(errs, errorsx.WithPrefix(err, "credit")) - } -Also applies to: 173-175
🤖 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 `@app/config/config.go` around lines 149 - 151, The c.Credits.Validate() call is being run twice producing duplicate errors with inconsistent prefixes; remove the redundant call so validation runs only once. Locate the two calls to c.Credits.Validate() and keep the one that uses the correct prefix (errorsx.WithPrefix(..., "credits")), then delete the other call (the one using "credit") to avoid duplicate messages; ensure only the remaining call appends its result to errs.
228-228:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate configuration setup for Credits.
ConfigureCredits(v, "credits")is being called twice (lines 228 and 237). This is redundant and should be removed.🔧 Suggested fix
Remove the duplicate call:
ConfigureCustomer(v, "customer") - ConfigureCredits(v, "credits") ConfigureTaxCode(v)Also applies to: 237-237
🤖 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 `@app/config/config.go` at line 228, Remove the duplicate ConfigureCredits call: locate the repeated ConfigureCredits(v, "credits") invocation in the config initialization (the symbol ConfigureCredits) and delete the redundant occurrence so the credits configuration is only set once; ensure no other setup logic (e.g., surrounding ConfigureX calls) is accidentally removed when cleaning up the duplicate.
🤖 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.
Outside diff comments:
In `@app/config/config.go`:
- Around line 149-151: The c.Credits.Validate() call is being run twice
producing duplicate errors with inconsistent prefixes; remove the redundant call
so validation runs only once. Locate the two calls to c.Credits.Validate() and
keep the one that uses the correct prefix (errorsx.WithPrefix(..., "credits")),
then delete the other call (the one using "credit") to avoid duplicate messages;
ensure only the remaining call appends its result to errs.
- Line 228: Remove the duplicate ConfigureCredits call: locate the repeated
ConfigureCredits(v, "credits") invocation in the config initialization (the
symbol ConfigureCredits) and delete the redundant occurrence so the credits
configuration is only set once; ensure no other setup logic (e.g., surrounding
ConfigureX calls) is accidentally removed when cleaning up the duplicate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9519325-6113-439e-884f-10a4fa876906
📒 Files selected for processing (3)
app/config/config.goapp/config/config_test.goapp/config/credits.go
💤 Files with no reviewable changes (2)
- app/config/config_test.go
- app/config/credits.go
GAlexIHU
approved these changes
Jun 8, 2026
… create and change endpoints
…lows. the reconciler's checker updated but logically unchanged. also, introduce a local cache before the flag checks.
rolosp
force-pushed
the
fix/settlement-mode-api
branch
from
e75769b to
6fbaf20
Compare
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
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 by CodeRabbit
New Features
Refactor