fix(api): backend save-side guard for account service override saves (#107 criterion 3)#240
Conversation
Wire checkCommitmentOptionCombo into saveAccountServiceOverride so that a direct PUT /api/accounts/:id/service-overrides/aws/rds with an invalid (term, payment) tuple (e.g. 3yr no-upfront for RDS) returns HTTP 400 instead of persisting silently. When commitmentOpts is nil or probe data is unavailable (ErrNoData), the guard is permissive — the frontend's hardcoded rules remain the primary gate. Adds derefInt/derefString helpers to bridge *int/*string override fields to the value-typed config.ServiceConfig shim expected by checkCommitmentOptionCombo. Tests: valid-pass, invalid-reject (400), nil-service permissive, ErrNoData permissive. Completes criterion 3 of #107.
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a pre-persist validation to saveAccountServiceOverride: constructs a nil-safe ChangesValidation Logic Addition
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant CommitmentOpts
participant ConfigStore
Client->>Handler: POST saveAccountServiceOverride(override)
Handler->>CommitmentOpts: Build ServiceConfig and call checkCommitmentOptionCombo
alt validation fails
CommitmentOpts-->>Handler: error (validation failure)
Handler-->>Client: 400 ClientError (do not call ConfigStore)
else validation succeeds or permissive
CommitmentOpts-->>Handler: ok
Handler->>ConfigStore: SaveAccountServiceOverride(override)
ConfigStore-->>Handler: success (ID)
Handler-->>Client: 200 OK (override ID)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 49 minutes and 59 seconds. Comment |
cristim
added
priority/p2
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/handler_accounts_test.go (1)
418-448: ⚡ Quick winAssert that invalid combos are not persisted.
Nice 400 assertion; add a “save not called” assertion to lock down the core guard contract during regressions.
Suggested test hardening
func TestSaveAccountServiceOverride_InvalidCombo_Returns400(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) setupAdminAuth(ctx, mockAuth) store := setupAdminMock(ctx) + saveCalled := false + store.SaveAccountServiceOverrideFn = func(_ context.Context, _ *config.AccountServiceOverride) error { + saveCalled = true + return nil + } handler := &Handler{ auth: mockAuth, config: store, commitmentOpts: &stubCommitmentOpts{ validateFn: func(_ context.Context, provider, service string, term int, payment string) (bool, error) { @@ ce, ok := IsClientError(err) require.True(t, ok, "expected ClientError, got %T: %v", err, err) assert.Equal(t, 400, ce.code) assert.Contains(t, ce.message, "3yr no-upfront") + assert.False(t, saveCalled, "override must not be persisted when combo is invalid") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler_accounts_test.go` around lines 418 - 448, The test TestSaveAccountServiceOverride_InvalidCombo_Returns400 should also assert that no persistence was attempted: extend the mock config/store returned by setupAdminMock (or add an expectation on the mock used as Handler.config) to verify the save method for service overrides (the mock method backing saveAccountServiceOverride in the store) was not called when handler.saveAccountServiceOverride returns the 400 ClientError; add an assertion/expectation on that mock (e.g., call count == 0 or ExpectNotCalled on the store's SaveAccountServiceOverride/SaveOverride method) immediately after the error assertions to lock down that invalid combos are not persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/api/handler_accounts_test.go`:
- Around line 418-448: The test
TestSaveAccountServiceOverride_InvalidCombo_Returns400 should also assert that
no persistence was attempted: extend the mock config/store returned by
setupAdminMock (or add an expectation on the mock used as Handler.config) to
verify the save method for service overrides (the mock method backing
saveAccountServiceOverride in the store) was not called when
handler.saveAccountServiceOverride returns the 400 ClientError; add an
assertion/expectation on that mock (e.g., call count == 0 or ExpectNotCalled on
the store's SaveAccountServiceOverride/SaveOverride method) immediately after
the error assertions to lock down that invalid combos are not persisted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd42da54-2c3d-4268-8bb6-d066a11c13f6
📒 Files selected for processing (2)
internal/api/handler_accounts.gointernal/api/handler_accounts_test.go
|
Thanks for the review @coderabbitai. Docstring coverage warning: The 50% docstring coverage is a pre-existing project-wide baseline — not introduced by this PR. Both new helper functions ( No other actionable items found. The change is minimal and complete. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] ✏️ Learnings added
|
…ick) Add SaveAccountServiceOverrideFn hook to MockConfigStore (mirrors the SetPlanAccountsFn pattern already in the file) so tests can intercept and assert the persist path. Wire it in TestSaveAccountServiceOverride_InvalidCombo_Returns400 to confirm the override is never written to the store when checkCommitmentOptionCombo returns a 400 error.
|
CR pass 1 triage:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering the follow-up review now. [review] ✏️ Learnings added
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
checkCommitmentOptionCombointosaveAccountServiceOverrideso that a directPUT /api/accounts/:id/service-overrides/aws/rdswith an invalid(term, payment)tuple (e.g.3yr no-upfrontfor RDS) returns HTTP 400 instead of persisting silently.derefInt/derefStringhelpers to bridge the sparse*int/*stringoverride fields to the value-typedconfig.ServiceConfigshim expected bycheckCommitmentOptionCombo.commitmentOptsis nil or probe data is unavailable (ErrNoData), the guard is permissive — the frontend's hardcoded rules remain the primary gate (same contract as the global-config path).Completes criterion 3 of #107. See also the tracking issue filed for this criterion (backend save-side guard).
Test plan
go test ./internal/api/... -count=1 -run TestSaveAccountServiceOverride— 6 tests pass (existing success + 4 new cases: invalid-combo-400, valid-combo-saves, nil-commitmentOpts-permissive, ErrNoData-permissive)go test ./internal/api/... -count=1— full api package (1054 tests) passesgo vet ./internal/api/...— no issuesgofmt -l internal/api/handler_accounts.go internal/api/handler_accounts_test.go— no output (clean)Summary by CodeRabbit
Bug Fixes
Tests
Chores