diff --git a/internal/entitlements/plan_act_runner.go b/internal/entitlements/plan_act_runner.go index 005ef29a..87ae302e 100644 --- a/internal/entitlements/plan_act_runner.go +++ b/internal/entitlements/plan_act_runner.go @@ -239,16 +239,29 @@ func nullString(s string) sql.NullString { // type defaults to. It returns the sentence that refuses the deletion of // productID's tier when that tier is the ladder's only one and // defaultingTypes, the display names of the organization types whose default -// ladder it is, is not empty; otherwise it returns "". It reads no row, so the -// plain delete and the removal apply the one rule: new signups land on the -// ladder's rank-0 tier, and a ladder with no tier leaves them without a -// starting plan. Emptying a ladder no type defaults to stays legal. +// ladder it is, is not empty; otherwise it returns "". The sentence says that +// new organizations of those types start on this tier, the only one in their +// default plan, and names the way past the refusal: add another tier first, +// or choose a new default (for the type when one is named, for each when +// several are). It reads no row, so the plain delete and the removal apply the +// one rule: new signups land on the ladder's rank-0 tier, and a ladder with no +// tier leaves them without a starting plan. Emptying a ladder no type defaults +// to stays legal. func LastTierRefusal(tiers []billing.PlanLadderTier, productID string, defaultingTypes []string) string { if len(tiers) != 1 || tiers[0].ProductID != productID || len(defaultingTypes) == 0 { return "" } - return "This ladder's only remaining tier; " + strings.Join(defaultingTypes, ", ") + - " defaults new signups to it. Deleting it would leave those signups without a starting plan, so the removal is refused." + n := len(defaultingTypes) + names := defaultingTypes[0] + if n > 1 { + names = strings.Join(defaultingTypes[:n-1], ", ") + " and " + defaultingTypes[n-1] + } + newDefault := "for each" + if n == 1 { + newDefault = "for " + defaultingTypes[0] + } + return "New " + names + " organizations start on this tier, the only one in their default plan. " + + "Add another tier first, or choose a new default " + newDefault + "." } // orgTypesDefaultingTo returns the display names of the organization types diff --git a/internal/entitlements/plan_act_runner_test.go b/internal/entitlements/plan_act_runner_test.go index fc5378eb..8f4ba344 100644 --- a/internal/entitlements/plan_act_runner_test.go +++ b/internal/entitlements/plan_act_runner_test.go @@ -143,8 +143,11 @@ func TestRunPlanActNamesAFailedCommit(t *testing.T) { func TestLastTierRefusal(t *testing.T) { only := []billing.PlanLadderTier{{ProductID: "basic"}} two := []billing.PlanLadderTier{{ProductID: "basic"}, {ProductID: "plus"}} - const sentence = "This ladder's only remaining tier; Personal, Team defaults new signups to it. " + - "Deleting it would leave those signups without a starting plan, so the removal is refused." + const ( + one = "New Personal organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for Personal." + two2 = "New Personal and Team organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for each." + three = "New Personal, Team and Club organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for each." + ) for _, tc := range []struct { name string tiers []billing.PlanLadderTier @@ -152,7 +155,10 @@ func TestLastTierRefusal(t *testing.T) { defaults []string want string }{ - {"the only tier of a default ladder", only, "basic", []string{"Personal", "Team"}, sentence}, + {"the only tier of a ladder one type defaults to", only, "basic", []string{"Personal"}, one}, + {"the only tier of a ladder two types default to", only, "basic", []string{"Personal", "Team"}, two2}, + {"the only tier of a ladder three types default to", only, "basic", []string{"Personal", "Team", "Club"}, three}, + {"the only tier of a ladder with an empty list of types", only, "basic", []string{}, ""}, {"the only tier of a ladder no type defaults to", only, "basic", nil, ""}, {"a tier of a ladder with another tier", two, "basic", []string{"Personal"}, ""}, {"a product that is not the only tier", only, "plus", []string{"Personal"}, ""}, @@ -175,7 +181,7 @@ func TestTierRemovalRefusal(t *testing.T) { viaType = " 2 of them hold it through their org type's default; " change = "changing that default with Migrate moves them in the background." reorder = "changing that default, or reordering this ladder, with Migrate moves them in the background." - lastTier = "This ladder's only remaining tier; Personal defaults new signups to it. Deleting it would leave those signups without a starting plan, so the removal is refused." + lastTier = "New Personal organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for Personal." ) personal := []string{"Personal"} for _, tc := range []struct { diff --git a/internal/server/operator_plan_ladder_delete_guard_test.go b/internal/server/operator_plan_ladder_delete_guard_test.go index 34db8588..652fe0f7 100644 --- a/internal/server/operator_plan_ladder_delete_guard_test.go +++ b/internal/server/operator_plan_ladder_delete_guard_test.go @@ -4,7 +4,7 @@ package server_test // Tests for DeletePlanLadderTier's last-tier guard (schema-hardening tasks -// 2.3 + 2.5, design D4a): deleting a ladder's only remaining tier is refused +// 2.3 + 2.5, design D4a): deleting a ladder's last tier is refused // when some org type's default_plan_ladder_id references that ladder (new // signups would land on a plan-less ladder), but stays allowed for a ladder // no org type defaults to (the 2026-07-03 remediation's narrow guard). @@ -54,7 +54,7 @@ func TestDeletePlanLadderTier_RefusesLastTierOfDefaultLadder(t *testing.T) { if code != http.StatusOK { t.Fatalf("status=%d body=%s", code, body) } - if !strings.Contains(body, "only remaining tier") { + if !strings.Contains(body, "the only one in their default plan") { t.Errorf("refusal copy missing; body:\n%s", body) } // The fixture's org type display name is "OTC "+suffix (newOtcFixture). diff --git a/internal/server/operator_plan_ladder_name_guard_test.go b/internal/server/operator_plan_ladder_name_guard_test.go index 268108d6..72f36e79 100644 --- a/internal/server/operator_plan_ladder_name_guard_test.go +++ b/internal/server/operator_plan_ladder_name_guard_test.go @@ -17,6 +17,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "regexp" "strings" "testing" @@ -113,11 +114,7 @@ func TestCreatePlanLadder_DuplicateNameRenders422OnTheNameField(t *testing.T) { if !strings.Contains(body, "is-invalid") || !strings.Contains(body, "invalid-feedback") { t.Errorf("expected the message to render on the name field, got: %s", body) } - for _, leak := range []string{"uq_plan_ladders_name_ci", "SQLSTATE", "23505"} { - if strings.Contains(body, leak) { - t.Errorf("response leaked driver text %q: %s", leak, body) - } - } + requireNoDriverText(t, body, "uq_plan_ladders_name_ci") } // TestUpdatePlanLadder_DuplicateNameRenders422OnTheNameField covers the same @@ -172,11 +169,7 @@ func TestUpdatePlanLadder_DuplicateNameRenders422OnTheNameField(t *testing.T) { if !strings.Contains(body, "is-invalid") || !strings.Contains(body, "invalid-feedback") { t.Errorf("expected the message to render on the name field, got: %s", body) } - for _, leak := range []string{"uq_plan_ladders_name_ci", "SQLSTATE", "23505"} { - if strings.Contains(body, leak) { - t.Errorf("response leaked driver text %q: %s", leak, body) - } - } + requireNoDriverText(t, body, "uq_plan_ladders_name_ci") reloaded, err := bq.GetPlanLadderByID(ctx, ladder.PlanLadderID) if err != nil { @@ -190,3 +183,22 @@ func TestUpdatePlanLadder_DuplicateNameRenders422OnTheNameField(t *testing.T) { func uniqueLadderSuffix() string { return uuid.New().String()[:8] } + +// uniqueViolationCode matches the unique-violation SQLSTATE as a word of its +// own. A substring match would also find it among the hex digits of an id the +// page renders, and fail a page that leaks nothing. +var uniqueViolationCode = regexp.MustCompile(`\b23505\b`) + +// requireNoDriverText fails when body carries the constraint's name, the word +// SQLSTATE, or the unique-violation code: the driver text of a duplicate name. +func requireNoDriverText(t *testing.T, body, constraint string) { + t.Helper() + for _, leak := range []string{constraint, "SQLSTATE"} { + if strings.Contains(body, leak) { + t.Errorf("response leaked driver text %q: %s", leak, body) + } + } + if uniqueViolationCode.MatchString(body) { + t.Errorf("response leaked driver text %q: %s", "23505", body) + } +} diff --git a/internal/server/operator_tier_removal_guards_test.go b/internal/server/operator_tier_removal_guards_test.go index 9274e603..c2fd127f 100644 --- a/internal/server/operator_tier_removal_guards_test.go +++ b/internal/server/operator_tier_removal_guards_test.go @@ -308,7 +308,7 @@ func TestTierRemoval_CommitRefusesWhatThePreviewRefuses(t *testing.T) { rec := r.commit(h, r.basic, disposition) - if got := htmltest.Parse(t, rec.Body.String()).Text(); !strings.Contains(got, "This ladder's only remaining tier;") || !strings.Contains(got, "so the removal is refused.") { + if got := htmltest.Parse(t, rec.Body.String()).Text(); !strings.Contains(got, "start on this tier, the only one in their default plan.") || !strings.Contains(got, "Add another tier first") { t.Errorf("the answer lacks the last-tier sentence: %.500s", got) } if !r.tierExists(t, r.basic) || r.acts(t) != 0 { diff --git a/internal/server/testdata/plan_acts/delete-last-tier-of-a-default.json b/internal/server/testdata/plan_acts/delete-last-tier-of-a-default.json index eb4e8536..fda7cbb3 100644 --- a/internal/server/testdata/plan_acts/delete-last-tier-of-a-default.json +++ b/internal/server/testdata/plan_acts/delete-last-tier-of-a-default.json @@ -1,8 +1,8 @@ { "answer": [ "status 200", - "Hx-Trigger showErrorToast: This ladder's only remaining tier; T defaults new signups to it. Deleting it would leave those signups without a starting plan, so the removal is refused.", - "[role=\"alert\"]: This ladder's only remaining tier; T defaults new signups to it. Deleting it would leave those signups without a starting plan, so the removal is refused." + "Hx-Trigger showErrorToast: New T organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for T.", + "[role=\"alert\"]: New T organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for T." ], "checked": [], "logs": [], diff --git a/internal/server/testdata/plan_acts/remove-last-tier-keep.json b/internal/server/testdata/plan_acts/remove-last-tier-keep.json index f3eee8db..72ff62a1 100644 --- a/internal/server/testdata/plan_acts/remove-last-tier-keep.json +++ b/internal/server/testdata/plan_acts/remove-last-tier-keep.json @@ -1,8 +1,8 @@ { "answer": [ "status 200", - "Hx-Trigger showErrorToast: This ladder's only remaining tier; T defaults new signups to it. Deleting it would leave those signups without a starting plan, so the removal is refused.", - "[role=\"alert\"]: This ladder's only remaining tier; T defaults new signups to it. Deleting it would leave those signups without a starting plan, so the removal is refused." + "Hx-Trigger showErrorToast: New T organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for T.", + "[role=\"alert\"]: New T organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for T." ], "checked": [], "logs": [], diff --git a/internal/server/testdata/plan_acts/remove-last-tier-migrate.json b/internal/server/testdata/plan_acts/remove-last-tier-migrate.json index f3eee8db..72ff62a1 100644 --- a/internal/server/testdata/plan_acts/remove-last-tier-migrate.json +++ b/internal/server/testdata/plan_acts/remove-last-tier-migrate.json @@ -1,8 +1,8 @@ { "answer": [ "status 200", - "Hx-Trigger showErrorToast: This ladder's only remaining tier; T defaults new signups to it. Deleting it would leave those signups without a starting plan, so the removal is refused.", - "[role=\"alert\"]: This ladder's only remaining tier; T defaults new signups to it. Deleting it would leave those signups without a starting plan, so the removal is refused." + "Hx-Trigger showErrorToast: New T organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for T.", + "[role=\"alert\"]: New T organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for T." ], "checked": [], "logs": [], diff --git a/internal/server/workspace_create_guard_test.go b/internal/server/workspace_create_guard_test.go index b6ee3620..42923cfd 100644 --- a/internal/server/workspace_create_guard_test.go +++ b/internal/server/workspace_create_guard_test.go @@ -219,11 +219,7 @@ func TestCreateWorkspace_DuplicateNameRenders422OnTheNameField(t *testing.T) { if !strings.Contains(body, "is-invalid") || !strings.Contains(body, "invalid-feedback") { t.Errorf("expected the message to render on the name field, got: %s", body) } - for _, leak := range []string{"uq_workspaces_org_id_name_ci", "SQLSTATE", "23505"} { - if strings.Contains(body, leak) { - t.Errorf("response leaked driver text %q: %s", leak, body) - } - } + requireNoDriverText(t, body, "uq_workspaces_org_id_name_ci") workspaces, err := organization.New(database).GetWorkspacesByOrgID(context.Background(), orgID) if err != nil { diff --git a/openspec/changes/plan-act-obligations/design.md b/openspec/changes/plan-act-obligations/design.md index c99669ef..841b07a9 100644 --- a/openspec/changes/plan-act-obligations/design.md +++ b/openspec/changes/plan-act-obligations/design.md @@ -819,7 +819,7 @@ Every sentence a person reads that this change adds, changes, reuses or removes. | # | Sentence | New place | Since the previous draft | |---|---|---|---| -| 21 | "This ladder's only remaining tier; {types} defaults new signups to it. Deleting it would leave those signups without a starting plan, so the removal is refused." | the plain delete's commit refusal, the removal's under either disposition, the refused removal preview, in place of its outcome lines and disposition choice, with Discard alone, and the reason of the tier card's disabled Remove or Preview removal on that tier | "Where" changed: Discard alone, the disabled Remove | +| 21 | "New {type} organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for {type}." With more than one type: "New {A} and {B} organizations start on this tier, the only one in their default plan. Add another tier first, or choose a new default for each." (three or more: "{A}, {B} and {C}") | the plain delete's commit refusal, the removal's under either disposition, the refused removal preview, in place of its outcome lines and disposition choice, with Discard alone, and the reason of the tier card's disabled Remove or Preview removal on that tier | "Where" changed: Discard alone, the disabled Remove | | 22 | "Already the default." | the default change's card error when a concurrent commit saved the same default first | | | 23 | "Another change collided with this one; try again." | tier add, tier delete, tier removal, the reorder, the default change's card and Retry, and the rule change's Apply changes, on a lock wait past the timeout or a deadlock (#216); today the tier add's field error for a rank collision | | | 24 | withdrawn | was the tier add's "Failed to add tier. Reference: {request id}." on its new failure paths | withdrawn: the tier add answers copy 50 on every failure path | diff --git a/openspec/changes/plan-act-obligations/specs/plan-ladder-management/spec.md b/openspec/changes/plan-act-obligations/specs/plan-ladder-management/spec.md index 4faa6286..07c98d47 100644 --- a/openspec/changes/plan-act-obligations/specs/plan-ladder-management/spec.md +++ b/openspec/changes/plan-act-obligations/specs/plan-ladder-management/spec.md @@ -389,7 +389,7 @@ attaches to a product whose set is empty is unfinished rather than stranded. ### Requirement: The last tier of a live default ladder cannot be deleted -Deleting a plan ladder tier, whether by the plain delete of a tier no organization holds or by the removal of a tier with holders under either disposition, SHALL be refused when it is the ladder's only remaining tier AND any org type's default plan ladder references that ladder, with copy explaining that new signups land on this ladder's bottom tier. The guard, and the plain delete's count of live holders, SHALL be read inside the transaction that would delete the tier, and a failed read of the org types SHALL refuse the deletion rather than allow it. The tier card SHALL render that tier's Remove or Preview removal disabled with the explanation as its reason, because the deletion is certain to be refused. Deleting the last tier of a ladder no org type defaults to SHALL remain allowed, because emptying a non-default ladder to rebuild it is a legitimate workflow. +Deleting a plan ladder tier, whether by the plain delete of a tier no organization holds or by the removal of a tier with holders under either disposition, SHALL be refused when it is the ladder's only remaining tier AND any org type's default plan ladder references that ladder, with copy explaining that new organizations of those types start on this tier and naming the way past the refusal: add another tier first, or choose a new default. The guard, and the plain delete's count of live holders, SHALL be read inside the transaction that would delete the tier, and a failed read of the org types SHALL refuse the deletion rather than allow it. The tier card SHALL render that tier's Remove or Preview removal disabled with the explanation as its reason, because the deletion is certain to be refused. Deleting the last tier of a ladder no org type defaults to SHALL remain allowed, because emptying a non-default ladder to rebuild it is a legitimate workflow. #### Scenario: Last tier of the signup default is protected