From 00e7430ef1ae663e0210cc46b75fd430358adfe8 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 13:57:09 +0100 Subject: [PATCH] fix(api): keep an equivalent ramp when editing a non-preset weekly/monthly plan The UI can send only the two presets or "custom", so Edit resubmits a stored non-preset weekly or monthly ramp (e.g. weekly 20%/7d, created through the API) as the equal custom one. rampScheduleShapeUnchanged compared the type label, so that counted as a ramp change: an in-progress plan was refused with 409 even for a bare rename, and a not-started one had its type rewritten to custom and its start and next-execution dates rebuilt. weekly, monthly and custom are labels for the same step-based behavior; only "immediate" is branched on downstream. Treat them as equivalent when percent, interval and step count match, and keep the stored type along with the progress fields. "immediate" and unknown labels still have to match exactly, and a real change of percent, interval or step count is still refused once a step has run. Closes #1184 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_plans.go | 31 +++++- internal/api/handler_plans_ramp_shape_test.go | 96 +++++++++++++++++++ 2 files changed, 123 insertions(+), 4 deletions(-) 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") + }) + } +}