diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 97157e494..19755f11c 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -485,6 +485,9 @@ func (h *Handler) updateServiceConfig(ctx context.Context, req *events.LambdaFun // GET response carries them, so a client that echoes it back must not // get them into the merged row; the store takes both from the database. cfg.CreatedAt, cfg.UpdatedAt = nil, nil + // The plan-group fields (#1746) belong only to purchase plan entries; a + // global service setting never carries them, even on a first save. + cfg.GroupID, cfg.BoundIdentities, cfg.MaxStepCommitment = "", nil, nil parts := strings.SplitN(service, "/", 2) if len(parts) == 2 { diff --git a/internal/api/handler_config_stamps_test.go b/internal/api/handler_config_stamps_test.go index aff74bc86..a7b892a10 100644 --- a/internal/api/handler_config_stamps_test.go +++ b/internal/api/handler_config_stamps_test.go @@ -85,3 +85,38 @@ func TestUpdateServiceConfig_IgnoresClientStamps(t *testing.T) { assert.Nil(t, saved.UpdatedAt, "a client-supplied updated_at must not reach the store") assert.Equal(t, 80.0, saved.Coverage, "the editable fields still apply") } + +// TestUpdateServiceConfig_IgnoresClientGroupFields: the plan-group fields +// (issue #1746) belong only to purchase plan entries, so a first save of a +// global service setting must not store a client's group_id, +// bound_identities or max_step_commitment. +func TestUpdateServiceConfig_IgnoresClientGroupFields(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com", + }, nil) + mockAuth.grantAdmin() + + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return(nil, nil) + var saved *config.ServiceConfig + mockStore.On("SaveServiceConfig", ctx, mock.AnythingOfType("*config.ServiceConfig")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.ServiceConfig) }). + Return(nil) + handler := &Handler{config: mockStore, auth: mockAuth} + + _, err := handler.updateServiceConfig(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"enabled": true, "term": 3, "payment": "all-upfront", "coverage": 80, + "group_id": "g1", "bound_identities": ["k1"], "max_step_commitment": 50}`, + }, "aws/rds") + require.NoError(t, err) + require.NotNil(t, saved) + assert.Empty(t, saved.GroupID) + assert.Nil(t, saved.BoundIdentities) + assert.Nil(t, saved.MaxStepCommitment) + assert.Equal(t, 80.0, saved.Coverage, "the editable fields still apply") +} diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 236c94f1d..9434fe58e 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -472,6 +472,11 @@ func mergeUpdatedPlan(existingPlan *config.PurchasePlan, req *PlanRequest, planI func preserveRampProgress(existingPlan, plan *config.PurchasePlan) error { plan.LastExecutionDate = existingPlan.LastExecutionDate plan.LastNotificationSent = existingPlan.LastNotificationSent + // A bound plan keeps its marker across any ramp edit: without it the plan + // would buy by filter instead of its selected recommendations (#1746). + if existingPlan.RampSchedule.AllocationMode == config.RampAllocationBoundGroups { + plan.RampSchedule.AllocationMode = config.RampAllocationBoundGroups + } if rampScheduleShapeUnchanged(existingPlan.RampSchedule, plan.RampSchedule) { plan.RampSchedule.Type = existingPlan.RampSchedule.Type diff --git a/internal/api/handler_plans_bound_marker_test.go b/internal/api/handler_plans_bound_marker_test.go new file mode 100644 index 000000000..e65c0c5d5 --- /dev/null +++ b/internal/api/handler_plans_bound_marker_test.go @@ -0,0 +1,39 @@ +package api + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Fixtures are hand-built plans, not real configuration (issue #1746). + +// A PUT before step 1 may change the ramp shape, but a bound plan must keep +// its bound_groups marker: dropping it would let the plan buy by filter. +func TestUpdatePlan_KeepsBoundGroupsMarker(t *testing.T) { + const planID = "12345678-1234-1234-1234-123456789abc" + weekly := config.PresetRampSchedules[config.RampWeekly25Pct] + for _, tc := range []struct { + name, body string + stored config.RampAllocationMode + want config.RampAllocationMode + }{ + {"shape change keeps bound_groups", `{"name":"p","ramp_schedule":"monthly-10pct"}`, config.RampAllocationBoundGroups, config.RampAllocationBoundGroups}, + // (guard) the same shape already carried every mode. + {"same shape keeps bound_groups", `{"name":"renamed","ramp_schedule":"weekly-25pct"}`, config.RampAllocationBoundGroups, config.RampAllocationBoundGroups}, + // (guard) other modes still reset on a shape change, as today. + {"shape change still drops equal_count", `{"name":"p","ramp_schedule":"monthly-10pct"}`, config.RampAllocationEqualCount, ""}, + } { + t.Run(tc.name, func(t *testing.T) { + rs := weekly + rs.AllocationMode = tc.stored + existing := &config.PurchasePlan{ID: planID, Name: "p", Enabled: true, RampSchedule: rs} + resp, saved := putPlan(t, existing, tc.body) + require.Equal(t, 200, resp.StatusCode, resp.Body) + require.NotNil(t, saved) + assert.Equal(t, tc.want, saved.RampSchedule.AllocationMode) + }) + } +}