feat: implement external payment endpoints#4158
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds an external settlement update feature for credit grants: new HTTP handler, request/response conversions and errors, service interface and implementation, routing gating by feature flag, noop backing implementation, and unit tests for conversion and error encoding. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as HTTP Handler
participant Service as CreditGrant Service
participant ChargesService as Charges Service
participant DB as Database/State
Client->>Handler: POST /external-settlement (namespace, customerID, chargeID, status)
Handler->>Handler: Parse JSON -> fromAPIUpdateCreditGrantExternalSettlementRequest
Handler->>Service: UpdateExternalSettlement(ctx, input)
Service->>Service: Validate input
Service->>DB: Get charge & verify customer/grant
DB-->>Service: Charge data
Service->>Service: Ensure settlement type == external
Service->>ChargesService: HandleCreditPurchaseExternalPaymentStateTransition(charge, targetStatus)
ChargesService->>DB: Persist payment state transition
DB-->>ChargesService: Updated charge
ChargesService-->>Service: Updated creditpurchase.Charge
Service-->>Handler: creditpurchase.Charge
Handler->>Handler: toAPIBillingCreditGrant(charge)
Handler-->>Client: 200 OK + BillingCreditGrant
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/v3/handlers/customers/credits/errors.go (1)
13-30: The package-levelerrCreditGrantExternalSettlementStatusInvalidvar is unused.Small thing: the
newCreditGrantExternalSettlementStatusInvalidfunction constructs a brand-newValidationIssuefrom scratch, so the static var above it is effectively dead code today. The nearbyapi/v3/handlers/meters/query/errors.gopattern builds the base issue once and then calls.WithAttr("value", duration)to add dynamic fields — that pattern removes the duplication and keeps the message template in one place.♻️ Possible refactor
-var errCreditGrantExternalSettlementStatusInvalid = models.NewValidationIssue( - errCodeCreditGrantExternalSettlementStatusInvalid, - "unsupported credit grant settlement status", - models.WithCriticalSeverity(), - commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest), - models.WithFieldString("status"), -) - -func newCreditGrantExternalSettlementStatusInvalid(status string) error { - return models.NewValidationIssue( - errCodeCreditGrantExternalSettlementStatusInvalid, - fmt.Sprintf("unsupported credit grant settlement status: %s", status), - models.WithCriticalSeverity(), - commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest), - models.WithFieldString("status"), - models.WithAttribute("status", status), - ) -} +var errCreditGrantExternalSettlementStatusInvalid = models.NewValidationIssue( + errCodeCreditGrantExternalSettlementStatusInvalid, + "unsupported credit grant settlement status", + models.WithCriticalSeverity(), + commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest), + models.WithFieldString("status"), +) + +func newCreditGrantExternalSettlementStatusInvalid(status string) error { + return errCreditGrantExternalSettlementStatusInvalid.WithAttr("status", status) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/credits/errors.go` around lines 13 - 30, Remove the dead duplicate by reusing the package-level template errCreditGrantExternalSettlementStatusInvalid inside newCreditGrantExternalSettlementStatusInvalid: build the base ValidationIssue once with models.NewValidationIssue (errCreditGrantExternalSettlementStatusInvalid) and in newCreditGrantExternalSettlementStatusInvalid return that template augmented with the dynamic attribute (call the instance method to add the "status" attribute, e.g. errCreditGrantExternalSettlementStatusInvalid.WithAttribute("status", status) or the equivalent .WithAttr API), so you keep the single message template and only add the runtime "status" value.api/v3/handlers/customers/credits/externalsettlement_test.go (1)
49-54: Prefert.Context()overcontext.Background()in tests.Small one: since you already have
tin scope, usingt.Context()ties cancellation to the test lifecycle and matches the repo convention.♻️ Proposed tweak
- handled := apierrors.GenericErrorEncoder()(context.Background(), err, rec, req) + handled := apierrors.GenericErrorEncoder()(t.Context(), err, rec, req)And drop the now-unused
"context"import.As per coding guidelines: "Use
t.Context()when atesting.Tortesting.TBis available instead of introducingcontext.Background()to keep cancellation and test-scoped lifecycle tied to the test harness".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/credits/externalsettlement_test.go` around lines 49 - 54, Replace the use of context.Background() with t.Context() when calling apierrors.GenericErrorEncoder() in the test (the call that assigns handled := apierrors.GenericErrorEncoder()(context.Background(), err, rec, req)), and remove the now-unused "context" import; this ensures the request context is tied to the test lifecycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/customers/credits/create_grant.go`:
- Around line 46-52: Add a short inline comment near the GenericNotFoundError
handling in the Create handler explaining the current assumption:
models.IsGenericNotFoundError(err) originates from creditGrantService.Create's
customer lookup (see Create in creditGrantService/service.go), and that if the
service later validates other entities (features, tax codes, etc.) this mapping
should be revisited; place the comment adjacent to the error branch that returns
apierrors.NewNotFoundError(ctx, err, "customer") to guide future maintainers.
---
Nitpick comments:
In `@api/v3/handlers/customers/credits/errors.go`:
- Around line 13-30: Remove the dead duplicate by reusing the package-level
template errCreditGrantExternalSettlementStatusInvalid inside
newCreditGrantExternalSettlementStatusInvalid: build the base ValidationIssue
once with models.NewValidationIssue
(errCreditGrantExternalSettlementStatusInvalid) and in
newCreditGrantExternalSettlementStatusInvalid return that template augmented
with the dynamic attribute (call the instance method to add the "status"
attribute, e.g.
errCreditGrantExternalSettlementStatusInvalid.WithAttribute("status", status) or
the equivalent .WithAttr API), so you keep the single message template and only
add the runtime "status" value.
In `@api/v3/handlers/customers/credits/externalsettlement_test.go`:
- Around line 49-54: Replace the use of context.Background() with t.Context()
when calling apierrors.GenericErrorEncoder() in the test (the call that assigns
handled := apierrors.GenericErrorEncoder()(context.Background(), err, rec,
req)), and remove the now-unused "context" import; this ensures the request
context is tied to the test lifecycle.
🪄 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: e9965c45-2a93-42bd-a6f9-0d8cd460e8a4
📒 Files selected for processing (11)
api/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/create_grant.goapi/v3/handlers/customers/credits/errors.goapi/v3/handlers/customers/credits/externalsettlement.goapi/v3/handlers/customers/credits/externalsettlement_test.goapi/v3/handlers/customers/credits/get_grant.goapi/v3/handlers/customers/credits/handler.goapi/v3/server/routes.goopenmeter/billing/creditgrant/noop.goopenmeter/billing/creditgrant/service.goopenmeter/billing/creditgrant/service/service.go
…ntion Rename all conversion functions in the customers/credits handler to follow the FromAPI/ToAPI naming convention and remove unused error var. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Overview
Implement the external payment handling endpoints.
Notes for reviewer
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests