diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 1b2343c3f..515131ff8 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -395,9 +395,11 @@ func mergeUpdatedPlan(existingPlan *config.PurchasePlan, req *PlanRequest, planI // carried over; a rebuild has no way to know either and would otherwise // wipe them. CurrentStep, StartDate, and NextExecutionDate are carried // over when the requested ramp schedule has the same shape as the existing -// one (same type/percent/interval/step-count): an unrelated field edit -// (rename, target coverage, ...) resubmits the same schedule and must not -// restart it. A different shape is refused once a step has completed, +// one (see rampScheduleShapeUnchanged): an unrelated field edit (rename, +// target coverage, ...) resubmits the same schedule and must not restart it. +// The stored type label is kept too, so resubmitting a non-preset weekly or +// monthly ramp as the equal custom one leaves the plan as it was (issue +// #1184). A different shape is refused once a step has completed, // because the rebuilt schedule would restart at step zero and forget the // percentage already bought (issue #386); before the first step it is a // plain schedule change. @@ -406,6 +408,7 @@ func preserveRampProgress(existingPlan, plan *config.PurchasePlan) error { plan.LastNotificationSent = existingPlan.LastNotificationSent if rampScheduleShapeUnchanged(existingPlan.RampSchedule, plan.RampSchedule) { + plan.RampSchedule.Type = existingPlan.RampSchedule.Type plan.RampSchedule.CurrentStep = existingPlan.RampSchedule.CurrentStep plan.RampSchedule.StartDate = existingPlan.RampSchedule.StartDate // Editor-owned markers the request cannot express; dropping them hands @@ -428,13 +431,33 @@ func preserveRampProgress(existingPlan, plan *config.PurchasePlan) error { // excluding the progress fields CurrentStep and StartDate, to tell apart // "the update resubmitted the same schedule" from "the update picked a // genuinely different one". +// +// The type must match, except that weekly, monthly and custom are only labels +// for the same step-based behavior (nothing downstream branches on them; only +// "immediate" is special). The UI can send just the presets or "custom", so a +// stored weekly 20%/7d ramp comes back as custom 20%/7d, and treating that as +// a change refused every edit of such a plan once a step had run (issue #1184). func rampScheduleShapeUnchanged(a, b config.RampSchedule) bool { - return a.Type == b.Type && + return rampTypesEquivalent(a.Type, b.Type) && a.PercentPerStep == b.PercentPerStep && a.StepIntervalDays == b.StepIntervalDays && a.TotalSteps == b.TotalSteps } +// rampTypesEquivalent reports whether two ramp type labels describe the same +// behavior: identical labels, or two of the step-based weekly/monthly/custom. +// "immediate" and any unrecognized label must match exactly. +func rampTypesEquivalent(a, b string) bool { + if a == b { + return true + } + return isStepBasedRampType(a) && isStepBasedRampType(b) +} + +func isStepBasedRampType(t string) bool { + return t == "weekly" || t == "monthly" || t == "custom" +} + func (h *Handler) deletePlan(ctx context.Context, req *events.LambdaFunctionURLRequest, planID string) (any, error) { // Validate UUID format to prevent injection attacks if err := validateUUID(planID); err != nil { diff --git a/internal/api/handler_plans_ramp_shape_test.go b/internal/api/handler_plans_ramp_shape_test.go index 499cf8404..4d490d87d 100644 --- a/internal/api/handler_plans_ramp_shape_test.go +++ b/internal/api/handler_plans_ramp_shape_test.go @@ -3,6 +3,7 @@ package api import ( "context" "testing" + "time" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/aws/aws-lambda-go/events" @@ -85,3 +86,98 @@ func TestUpdatePlan_RampShapeChange(t *testing.T) { }) } } + +// TestUpdatePlan_EquivalentCustomRampKeepsStoredSchedule pins issue #1184. +// The UI can send only the presets or "custom", so Edit resubmits a stored +// non-preset weekly/monthly ramp as an equal custom one. The type label is +// cosmetic for step-based ramps (only "immediate" changes behavior), so an +// equal percent, interval and step count is the same schedule: an in-progress +// plan keeps its progress, and the stored type, start date and next execution +// date are kept rather than rebuilt, whether or not a step has completed. +func TestUpdatePlan_EquivalentCustomRampKeepsStoredSchedule(t *testing.T) { + const planID = "12345678-1234-1234-1234-123456789abc" + start := time.Date(2026, 9, 1, 0, 0, 0, 0, time.UTC) + next := time.Date(2026, 9, 22, 0, 0, 0, 0, time.UTC) + weekly20 := config.RampSchedule{Type: "weekly", PercentPerStep: 20, StepIntervalDays: 7, TotalSteps: 5} + monthly5 := config.RampSchedule{Type: "monthly", PercentPerStep: 5, StepIntervalDays: 30, TotalSteps: 20} + custom25 := config.RampSchedule{Type: "custom", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4} + + tests := []struct { + name string + stored config.RampSchedule + step int + body string + }{ + {"in-progress weekly 20%, renamed via custom", weekly20, 3, + `{"name":"renamed","ramp_schedule":"custom","custom_step_percent":20,"custom_interval_days":7}`}, + {"in-progress monthly 5%, renamed via custom", monthly5, 1, + `{"name":"renamed","ramp_schedule":"custom","custom_step_percent":5,"custom_interval_days":30}`}, + {"in-progress custom 25%/7d, renamed via the equal weekly preset", custom25, 2, + `{"name":"renamed","ramp_schedule":"weekly-25pct"}`}, + {"not started weekly 20%, renamed via custom", weekly20, 0, + `{"name":"renamed","ramp_schedule":"custom","custom_step_percent":20,"custom_interval_days":7}`}, + {"not started monthly 5%, renamed via custom", monthly5, 0, + `{"name":"renamed","ramp_schedule":"custom","custom_step_percent":5,"custom_interval_days":30}`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + rs := tc.stored + rs.CurrentStep = tc.step + rs.StartDate = start + nextExec := next + existing := &config.PurchasePlan{ID: planID, Name: "p", Enabled: true, RampSchedule: rs, NextExecutionDate: &nextExec} + + resp, saved := putPlan(t, existing, tc.body) + + require.Equal(t, 200, resp.StatusCode, resp.Body) + require.NotNil(t, saved) + assert.Equal(t, "renamed", saved.Name) + assert.Equal(t, rs, saved.RampSchedule, "the stored schedule (type, progress, start date) must be kept") + require.NotNil(t, saved.NextExecutionDate) + assert.True(t, next.Equal(*saved.NextExecutionDate), "next execution date must be kept, got %v", saved.NextExecutionDate) + }) + } +} + +// TestUpdatePlan_RealRampChangeOnNonPresetStillRefused is the other half of +// #1184: label-agnostic matching must not let a real change through on an +// in-progress plan. +func TestUpdatePlan_RealRampChangeOnNonPresetStillRefused(t *testing.T) { + const planID = "12345678-1234-1234-1234-123456789abc" + weekly20 := config.RampSchedule{Type: "weekly", PercentPerStep: 20, StepIntervalDays: 7, TotalSteps: 5, CurrentStep: 2} + // 30% and 33% both give 3 steps: only the percent differs. + weekly30 := config.RampSchedule{Type: "weekly", PercentPerStep: 30, StepIntervalDays: 7, TotalSteps: 3, CurrentStep: 1} + immediate := config.PresetRampSchedules[config.RampImmediate] + immediate.CurrentStep = 1 + // Same percent, interval and step count as immediate: only the type + // differs, and "immediate" is never interchangeable with a step-based type. + custom100 := config.RampSchedule{Type: "custom", PercentPerStep: 100, StepIntervalDays: 0, TotalSteps: 1, CurrentStep: 1} + + tests := []struct { + name string + stored config.RampSchedule + body string + }{ + {"different percent and step count", weekly20, + `{"name":"p","ramp_schedule":"custom","custom_step_percent":25,"custom_interval_days":7}`}, + {"different interval", weekly20, + `{"name":"p","ramp_schedule":"custom","custom_step_percent":20,"custom_interval_days":14}`}, + {"different percent, same step count", weekly30, + `{"name":"p","ramp_schedule":"custom","custom_step_percent":33,"custom_interval_days":7}`}, + {"weekly to immediate", weekly20, `{"name":"p","ramp_schedule":"immediate"}`}, + {"immediate to custom 100%", immediate, + `{"name":"p","ramp_schedule":"custom","custom_step_percent":100,"custom_interval_days":1}`}, + {"custom 100% to immediate", custom100, `{"name":"p","ramp_schedule":"immediate"}`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + existing := &config.PurchasePlan{ID: planID, Name: "p", Enabled: true, RampSchedule: tc.stored} + + resp, saved := putPlan(t, existing, tc.body) + + assert.Equal(t, 409, resp.StatusCode, resp.Body) + assert.Nil(t, saved, "a refused shape change must not reach the store") + assert.Contains(t, resp.Body, "create a new plan") + }) + } +}