From 8bcad0828e65593a93e79ff2b8f3ce3a44fb1d18 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 19:38:43 +0100 Subject: [PATCH 1/4] fix(purchase): drop internal issue ref from empty plan step message The empty-plan-step refusal and the retry 409 are shown to users and named an internal issue number. Drop the suffix from both. Failure reasons already stored carry the old text, so the stored-note allowlist accepts both forms. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 2 +- .../api/handler_purchases_retry_dedupe_test.go | 16 ++++++++++++++++ internal/purchase/errnotes.go | 2 +- internal/purchase/errnotes_test.go | 10 ++++++++++ internal/purchase/execution.go | 7 +++++-- 5 files changed, 33 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 0fbe3328d..7cc4348c1 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2461,7 +2461,7 @@ func (h *Handler) retryPurchase(ctx context.Context, req *events.LambdaFunctionU // successor that fails the same way. if failedExec.PlanID != "" && len(failedExec.Recommendations) == 0 { return nil, NewClientError(409, - "this plan step has no recommendations attached, so retrying it would buy nothing (platform#631: plan steps cannot yet carry recommendations)") + "this plan step has no recommendations attached, so retrying it would buy nothing") } totalUpfront, totalSavings, err := validateAndTotalRecommendations(failedExec.Recommendations) diff --git a/internal/api/handler_purchases_retry_dedupe_test.go b/internal/api/handler_purchases_retry_dedupe_test.go index eb9321987..81ad0871f 100644 --- a/internal/api/handler_purchases_retry_dedupe_test.go +++ b/internal/api/handler_purchases_retry_dedupe_test.go @@ -110,3 +110,19 @@ func TestPersistExecution_RecentSubmitsErrorIsSurfaced(t *testing.T) { assert.Nil(t, dup) mockConfig.AssertNotCalled(t, "SavePurchaseExecution") } + +// The empty-plan-step 409 is shown to users, so it must not name an internal +// issue number (#1708). +func TestHandler_retryPurchase_EmptyPlanStepMessageHasNoInternalRef(t *testing.T) { + failed, handler, _ := retryDedupeFixture(t, false) + failed.PlanID = "plan-1" + failed.Recommendations = nil + + _, err := handler.retryPurchase(context.Background(), sessionRetryReq(), retryExecID) + + ce, ok := IsClientError(err) + require.True(t, ok, "%v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "no recommendations attached") + assert.NotContains(t, ce.message, "#") +} diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index e7b4d070b..e0e407a42 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -21,7 +21,7 @@ var serverNotePatterns = []string{ `execution was approved but its purchase run was interrupted before completing and never finalized; failed by the recovery sweep so it is not silently stuck \(issue #632\)\. (?:Verify on the cloud provider that no commitment was created, then Retry\.|Retry is refused for this purchase because a repeat could buy a second commitment\. Verify on the cloud provider whether the commitment was created; if it was not, start a new purchase\.)`, `refusing to execute retry attempt \d+: (?:` + redriveReasonPattern + `)`, `stale Azure recommendation pricing; refresh recommendations and submit a new purchase(?:: the latest collection state is missing, so Azure prices cannot be confirmed current)?`, - `(?:execution [^:\s]+: )?plan step has no recommendations attached, so it cannot buy anything \(platform#631: plan steps cannot yet carry recommendations\)`, + `(?:execution [^:\s]+: )?plan step has no recommendations attached, so it cannot buy anything(?: \(platform#631: plan steps cannot yet carry recommendations\))?`, `plan step refused by commitment cap(?:: plan has no max_step_commitment set \(auto-purchase of steps with recommendations is refused until one is set\)|: step total commitment \d+\.\d{2} exceeds max_step_commitment \d+\.\d{2})?`, `scheduled purchase skipped: it was due too long ago(?:: it was due at \d{4}-\d{2}-\d{2}T[0-9:.]+Z, [0-9a-z.]+ ago, which is more than the [0-9a-z.]+ limit \(SCHEDULED_PURCHASE_MAX_OVERDUE\); nothing was submitted, retry it if the purchase is still wanted)?`, `purchase refused after claim: the claimed execution no longer passes the authorization checks`, diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index a8e366396..e192b7239 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -30,6 +30,8 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { armedRedriveRefusal(gcpRetry).Error(), ErrStaleAzurePricing.Error(), ErrPlanStepNoRecommendations.Error(), + legacyPlanStepNoRecommendationsNote, + "execution 00000000-0000-0000-0000-000000000000: " + legacyPlanStepNoRecommendationsNote, fmt.Errorf("execution %s: %w", "00000000-0000-0000-0000-000000000000", ErrPlanStepNoRecommendations).Error(), fmt.Errorf("%w: plan has no max_step_commitment set (auto-purchase of steps with recommendations is refused until one is set)", ErrStepCommitmentCap).Error(), fmt.Errorf("%w: step total commitment 10.00 exceeds max_step_commitment 5.00", ErrStepCommitmentCap).Error(), @@ -77,3 +79,11 @@ func TestSanitizeStoredError_UnknownTextFailsClosed(t *testing.T) { assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), out, in) } } + +// Rows stored before #1708 carry the old text that named an internal issue; +// it must keep passing the allowlist, and the live text must not name one. +const legacyPlanStepNoRecommendationsNote = "plan step has no recommendations attached, so it cannot buy anything (platform#631: plan steps cannot yet carry recommendations)" + +func TestErrPlanStepNoRecommendations_NamesNoInternalIssue(t *testing.T) { + assert.NotContains(t, ErrPlanStepNoRecommendations.Error(), "#") +} diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index fc62ef7e6..56a821888 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -25,8 +25,11 @@ import ( // ErrPlanStepNoRecommendations marks a plan-step execution that carries no // recommendations. Nothing attaches recommendations to a plan step yet // (platform#609), so such a step can only "succeed" by buying nothing, which -// would complete it and advance the ramp. It is refused instead. -var ErrPlanStepNoRecommendations = errors.New("plan step has no recommendations attached, so it cannot buy anything (platform#631: plan steps cannot yet carry recommendations)") +// would complete it and advance the ramp. It is refused instead. The text is +// user-facing, so it names no internal issue (tracked in platform#631); rows +// stored before that carry a "(platform#631: ...)" suffix, which errnotes.go +// still recognizes. +var ErrPlanStepNoRecommendations = errors.New("plan step has no recommendations attached, so it cannot buy anything") // executePurchase performs the actual purchase. // When the plan has associated cloud accounts and a credential store is configured, From 1fd5aab8e77cb783f0631a78d41b8af4ac5fafa8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 19:38:57 +0100 Subject: [PATCH 2/4] fix(plans): polish Planned Purchases table and ramp wording Keep the empty-step Run now focusable with aria-disabled so keyboard users can read why it cannot run, let the plan name use available width, render empty numeric cells as one muted dash, add Plan and Scheduled Date column filters through the existing plans filter mechanism, and say the ramp step is a percent of the target. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/plans-column-filters.test.ts | 20 +++++++++++++++- .../src/__tests__/plans-ramp-label.test.ts | 16 +++++++++++++ frontend/src/__tests__/plans.test.ts | 22 ++++++++++++++--- frontend/src/index.html | 6 ++--- frontend/src/plans.ts | 24 +++++++++++++------ frontend/src/state.ts | 2 +- frontend/src/styles/tables.css | 22 +++++++++++++++++ 7 files changed, 97 insertions(+), 15 deletions(-) diff --git a/frontend/src/__tests__/plans-column-filters.test.ts b/frontend/src/__tests__/plans-column-filters.test.ts index e385833cd..e9c2385e3 100644 --- a/frontend/src/__tests__/plans-column-filters.test.ts +++ b/frontend/src/__tests__/plans-column-filters.test.ts @@ -141,10 +141,28 @@ describe('Planned Purchases column filters (issue #166 follow-up)', () => { ); const cols = Array.from(buttons).map((b) => b.dataset['column']); expect(cols.sort()).toEqual( - ['count', 'estimated_savings', 'payment', 'provider', 'resource_type', 'service', 'status', 'term', 'upfront_cost'].sort(), + ['count', 'estimated_savings', 'payment', 'plan_name', 'provider', 'resource_type', 'scheduled_date', 'service', 'status', 'term', 'upfront_cost'].sort(), ); }); + test('plan name and scheduled date filter narrow rows (#1680)', async () => { + await loadPlans(); + const uncheck = (column: string, value: string): void => { + document.querySelector( + `#planned-purchases-list th .column-filter-btn[data-column="${column}"]`, + )?.click(); + const cb = Array.from(document.querySelectorAll('.column-filter-popover input[type="checkbox"]')) + .find((i) => i.dataset['value'] === value); + expect(cb).toBeDefined(); + cb!.checked = false; + cb!.dispatchEvent(new Event('change')); + }; + uncheck('plan_name', 'Plan B'); + expect(countRows()).toBe(2); + uncheck('scheduled_date', '2026-06-02'); + expect(countRows()).toBe(1); + }); + test('clicking a trigger opens a popover detached to document.body', async () => { await loadPlans(); const providerBtn = document.querySelector( diff --git a/frontend/src/__tests__/plans-ramp-label.test.ts b/frontend/src/__tests__/plans-ramp-label.test.ts index e76fe2136..946a20d10 100644 --- a/frontend/src/__tests__/plans-ramp-label.test.ts +++ b/frontend/src/__tests__/plans-ramp-label.test.ts @@ -180,3 +180,19 @@ describe('custom ramp inputs when Edit is the first plan modal (#1536)', () => { expect(intervalInput().disabled).toBe(true); }); }); + +describe('ramp step wording says percent of target (#1680)', () => { + beforeEach(() => { document.body.innerHTML = loadRealIndexBody(); }); + + test('custom step field label names the target', () => { + const label = document.getElementById('ramp-step-percent')?.closest('label')?.textContent ?? ''; + expect(label).toContain('% of target'); + }); + + test('preset descriptions say each step is a share of the target', () => { + const desc = (value: string): string => + document.querySelector(`input[value="${value}"]`)?.closest('label')?.querySelector('.ramp-desc')?.textContent ?? ''; + expect(desc('weekly-25pct')).toContain('25% of target each week'); + expect(desc('monthly-10pct')).toContain('10% of target each month'); + }); +}); diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index 94735b070..42caa2512 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -816,17 +816,33 @@ describe('Plans Module', () => { await loadPlans(); }); - test('renders the run button disabled with an explanatory title', () => { + test('renders the run button aria-disabled (still focusable) with an explanatory title', () => { const runBtn = document.querySelector('[data-action="run"]') as HTMLButtonElement; - expect(runBtn.disabled).toBe(true); + expect(runBtn.disabled).toBe(false); + expect(runBtn.getAttribute('aria-disabled')).toBe('true'); + expect(runBtn.classList.contains('is-disabled')).toBe(true); expect(runBtn.title).toBe('Run now: this step has no recommendations, so there is nothing to run'); expect(runBtn.getAttribute('aria-label')).toBe('Run now: this step has no recommendations, so there is nothing to run'); expect(runBtn.getAttribute('data-id')).toBe('purchase-bare'); }); + test('placeholder dashes share one muted style in the numeric columns (#1680)', () => { + const cells = Array.from(document.querySelectorAll('tr.planned-purchase-row td.num')); + expect(cells).toHaveLength(3); + cells.forEach(td => { + expect(td.innerHTML.trim()).toBe('\u2014'); + }); + }); + + test('the plan name is not clamped to the small truncate width (#1680)', () => { + const name = document.querySelector('.plan-name') as HTMLElement; + expect(name.classList.contains('cell-truncate--sm')).toBe(false); + expect(name.classList.contains('plan-name-cell')).toBe(true); + expect(name.title).toBe('Azure compute 80% weekly'); + }); + test('the run action never calls the API or opens a confirm', async () => { const runBtn = document.querySelector('[data-action="run"]') as HTMLButtonElement; - runBtn.disabled = false; runBtn.click(); await flushPromises(); diff --git a/frontend/src/index.html b/frontend/src/index.html index 4c34fcfe7..1e316e598 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -1075,12 +1075,12 @@

Ramp-up Schedule