[cristim]

Part of #539. This follows up on #1487 with the points both of its reviews raised.

Changes:

• Repair GCP rows that already hold an AWS payment. Before fix(settings): save only the service configs and keys the user changed #1487, a change to the global default payment could write an AWS value such as all-upfront onto GCP service rows. GCP validation accepts only monthly, and GCP cards have no payment control, so every later save of such a row failed with a 400. When Settings loads a GCP row whose stored payment isn't monthly, it now treats the row like the fix(settings): Save sends Azure/GCP payment values the service-config handler rejects #545 normalised-payment case: the next PUT of that row carries payment: "monthly".
• Pin the AWS-only payment propagation. A new integration test sets the global default payment to monthly, which is valid globally but not for AWS, and checks that AWS rows are left unchanged. Without the "AWS accepts it" check in propagateChangedDefaults, the test fails.
• Inline coverage error. An edited Savings Plan coverage that's been cleared now shows an inline required error as well as the toast.
• Empty cache window refused. An emptied cache-stale-hours input used to be accepted as 0, which turns caching off. It's now refused with the existing validation error. An explicit 0 is still accepted, because the backend allows 0 to 8760 and treats 0 as off (validation.go:383).

Tests:

• jest covers:
  • a GCP row stored as all-upfront sends {term, payment:"monthly"};
  • a monthly GCP row sends only {term};
  • an unedited card is never flagged;
  • an empty cache window is refused.
• A real-Postgres integration test covers the AWS-only propagation.
• Mutation probes: dropping the GCP normalisation, the unedited-card skip, the empty cache check, or the inline required each fails a test.

Local:

• The full jest suite passes (4536, 1 skipped). tsc is clean and lint shows 0 errors.
• go test -race and the integration test pass on config.
• pre-commit passes.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Required settings fields now show validation errors when left empty, while a cache-staleness value of 0 remains valid.
  • Unchanged Savings Plan coverage no longer blocks saving other settings.
  • GCP service saves retain the displayed monthly payment when updating a term, and service-specific AWS payment choices remain unchanged when the global default is updated.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/settings-gcp-payment-repair
• File: .coderabbit.yaml
• Commit: f424efe
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: 118c698f-c09a-4409-9734-638346219f59

Commits

Reviewing files that changed from the base of the PR and between fe11124 and f424efe.

Files selected for processing: 4

• frontend/src/__tests__/settings-service-payment.test.ts
• frontend/src/__tests__/settings.test.ts
• frontend/src/settings.ts
• internal/config/store_postgres_payment_propagation_integration_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Walkthrough

Settings validation now rejects empty cache-staleness and Savings Plan coverage values. GCP service saves adjust payment fields when needed. Tests cover these changes and AWS payment preservation during global-default updates.

Changes: Settings save behavior

Layer / File(s)	Summary
Required settings validation frontend/src/settings.ts, frontend/src/__tests__/settings.test.ts, frontend/src/__tests__/settings-service-payment.test.ts	Cache-staleness and Savings Plan coverage inputs reject empty values. Save validation rejects an empty stale-hours value but accepts 0. Tests verify that untouched stored Savings Plan coverage does not block saving another setting.
Service payment updates frontend/src/settings.ts, frontend/src/__tests__/settings-service-payment.test.ts, internal/config/store_postgres_payment_propagation_integration_test.go	GCP service saves repair stored payment values when no payment control is present. Tests check the partial PUT payload and verify that a global default-payment update preserves an AWS service’s existing no-upfront value.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

• LeanerCloud/cloud-commitments-platform#1487: This PR extends the changed-service-field payment handling introduced in that PR to GCP rows without a payment control.

Suggested reviewers: iamjsd

Merge Risk: Minimal · up to f424e

No actionable issue remains from the supplied evidence; the change is ready to merge after normal checks.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the two main changes: repairing invalid payment values on GCP rows and rejecting an empty cache-stale-hours field.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

Independent review 1 (cold, fresh context):

VERDICT: APPROVE f424efe

Reviewed git diff origin/main...f424efec (951d718, f424efe). The merge with origin/main is clean (tree 98d47c1e).
Findings (none blocking):
LOW frontend/src/settings.ts:2624 - nothing tests required: true on the setting-recs-stale-hours inline validator. Removing it still passes all 310 settings tests. The save-time refusal is tested (M2 below), so the risk is limited to the inline error UX.
NOTE (pre-existing, out of scope) settings.ts:246 - readGraceInput turns an emptied grace field into 7, the default, without telling the user. An admin who clears it, probably meaning 0, gets 7. This is the same "empty becomes a value" class as the cache field. The other numeric fields are fine: notification-days '' becomes 0 and is refused because the minimum is 1; global coverage and SP coverage already refuse '' (#1487).
Attacked:

• GCP legal payment: config.ValidPaymentOptionsByProvider has "gcp": {"monthly"} (validation.go:53), with no per-service exception. NormalizePaymentOption collapses every GCP token to monthly. "monthly" is the only value the backend can store for any GCP service, so the repair cannot overwrite a deliberate value. A stored "" (inherit) becomes "monthly" only on a PUT the user triggers (probe P2). That value is equivalent and is the one planstep can resolve.
• No extra PUTs: the repair runs after if (Object.keys(changes).length === 0) return null (settings.ts:3422). Probe P1: an unedited GCP row stored as all-upfront, plus an unrelated global edit, sends no GCP PUT (pass). The new it.each test shows a stored monthly row sends no payment key.
• Cache window: '' is refused with the existing 'Cache stale threshold...' toast and no PUT; '0' saves { recommendations_cache_stale_hours: 0 } (new test).
  Mutation probes (each restored with cp and checked identical with cmp):
• M1, GCP repair branch changed to undefined: 1 test fails.
• M2, stale '' check removed: 1 test fails.
• M3, propagateChangedDefaults AWS-accepts guard forced true: TestIntegration_PropagateDefaultPayment_SkipsValueAWSRejects fails.
• M4, SP coverage required:true removed: the inline-error test fails.
• M5, stale-hours required removed: survives (the LOW above).
  Runs on the merged tree:
• go test -race ./internal/config: ok.
• -tags integration ./internal/config (heavy lock): ok.
• Full npx jest: 173 suites, 4536 passed, 1 skipped.
• tsc --noEmit: clean.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE f424efe

Findings:

• frontend/src/settings.ts:3424 - the GCP repair runs only after the early return null at :3422, so a GCP row holding an AWS token is fixed only when the user edits something else on that row. A non-UI client doing a partial PUT of that row is still rejected, because internal/api/handler_config.go:367-375 validates the merged row. The emission boundary (internal/config/validation.go:145 NormalizePaymentOption) keeps scheduling safe in the meantime. The root fix for the bad rows already stored is a one-off data migration (payment='monthly' WHERE provider='gcp' AND payment<>'monthly'), as a follow-up. [non-blocking]
• frontend/src/settings.ts:2624 - nothing tests required: true on setting-recs-stale-hours. Mutation M4 (dropping it) leaves all 310 settings tests green. [non-blocking]
• internal/config/store_postgres_payment_propagation_integration_test.go:15 - the test only fails when the AWS-accept clause is dropped. With propagateChangedDefaults turned into a no-op it still passes. The positive path is covered elsewhere (internal/api/handler_config_propagation_integration_test.go:53, :106), so this is acceptable. A positive control in the same test would make it self-contained. [non-blocking]
• frontend/src/settings.ts:3428 - the literal 'monthly' repeats commitmentOptions.ts:336-338. normalizePaymentValue(base.payment, field.provider) would reuse it. A GCP row stored with an empty payment now gets 'monthly' sent. The backend accepts that (an empty payment also passes validation.go:536), but no test covers the case. [non-blocking]

Attacked:

• GCP-only claim: validation.go:53 "gcp": {"monthly"}, and validatePayment (validation.go:535-552) accepts only "" or "monthly". It applies to every GCP service because the set is per provider. The combo check (handler_config.go:171) skips non-AWS rows.
• Rows without a payment control: SERVICE_FIELDS (settings.ts:121-151) has paymentId null only on the 3 GCP rows, so the undefined branch is unreachable today. That matches the old behavior.
• base is the server row (settings.ts:3184, canonicalized for keys only). Re-keyed and new rows go through newServiceConfigDefaults and normalizePaymentValue, which already yields monthly.
• Stale-hours paths: whitespace-only is refused; " 0 " gives 0 and is saved; "1e2" gives 100; "24.0" gives 24; "-0" passes and serializes as 0. A missing element falls back to '24'. Load always fills the field (:3154, ?? 24), so an unedited form is never refused.
• SP coverage required:true: load always fills every SP card (:3207-3210) before revalidateInlineRanges. A stored 150 was already flagged inline before this change, and the Save check still skips unedited cards (new pin test).
• Mutation probes (head copy, each restored and checked with cmp):
  • M1 dropped the GCP branch: 1 test failed (GCP all-upfront case).
  • M2 dropped required on the SP wiring: 1 test failed (inline error).
  • M3 dropped the trim check: 1 test failed (emptied stale threshold).
  • M4 dropped required on stale-hours: nothing failed (gap above).
  • M5 removed the early return: 21 tests failed.
  • G1 dropped the AWS-accept clause: the integration test failed (expected "no-upfront", got "monthly").
  • G2 made propagation a no-op: the integration test still passed.

Verified locally:

• Fail-before, with the head tests on base sources: 3 failed out of 145 (GCP all-upfront, SP inline error, emptied stale threshold). The unedited-SP pin passes on base, as the commit says.
• npx jest --coverage=false src/__tests__/settings (head): 10 suites, 310 passed.
• npx tsc --noEmit: clean. npx eslint on the 3 changed TS files: no issues.
• GOTOOLCHAIN=go1.26.10:
  • gofmt -l on the new file: clean.
  • go vet ./internal/config/: ok. With -tags integration: ok.
  • go test -count=1 ./internal/config/: ok.
  • go test -tags integration -run TestIntegration_PropagateDefaultPayment_SkipsValueAWSRejects ./internal/config/ (shared testcontainers postgres, per-test DB clone through RequireMigratedDatabase): PASS.
• git merge-tree --write-tree origin/main <head>: clean, tree 98d47c1. The local origin/main is fe11124, the same as the base, so nothing has landed on main since.
• The head export matches the PR head (settings.ts cmp). The PR adds no migration.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification at f424efecb00b6c9557b5fc15362cd41e0ffe9434 merged onto current main (fe111246; merge-tree 98d47c1e; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration passes on real Postgres (testcontainers) for the touched packages: ./internal/config
• Frontend: npm ci, tsc --noEmit and npm run build pass on the merged tree, and the full jest suite has no failures beyond those that fail identically on main in this local environment (CI runs the authoritative jest suite).
• Nothing was applied to any cloud.