From f1812530eeb162de6c27f6b450292a5c74d246eb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Apr 2026 02:10:16 +0200 Subject: [PATCH] fix(ux/settings): reset-override dialog wording matches data semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #114. The Reset confirmation dialog said: "Reset {provider}/{service} override to the global default? Any per-service values you set will be replaced." But what actually happens is the row is DELETEd from account_service_overrides — the engine then reads the global default as a side effect of the row being gone, not as a "reset" operation in the data sense. "Replaced" implied the row stuck around with new values. Per the issue's recommended option A (tighten wording, keep DELETE semantics), changes: - Button label: "Reset" → "Delete" - Dialog title: "Reset override?" → "Delete override?" - Dialog body: spells out that the override row + its per-service values will be removed and the account reverts to the global default - Confirm label: "Reset override" → "Delete override" - Toast on failure: "Failed to reset override" → "Failed to delete override" - The Inherit-payment dropdown's tooltip ("Use Reset to clear all override fields including payment") tightened to point at the Delete action Side change: exported `loadOverridesPanel` so the new test can drive the override panel directly. The function is a coherent unit and was already test-friendly in shape; just needed the export. Tests: - New `Delete-override button + dialog wording match the actual data semantics (issue #114)` test pins the post-fix wording: button reads "Delete" (not "Reset"), dialog title/body/confirm label all match, body must NOT contain "replaced" or "Reset". - All 41 existing settings-accounts tests still pass. Frontend-only change. Backend (`DELETE /api/accounts/:id/service-overrides/:provider/:service`) unchanged. Drive-by: removed unused `formatRelativeTime` import in `frontend/src/recommendations.ts`. It was a pre-existing TS6133 error (unused-declaration) that broke `recommendations.test.ts` from running, blocking every commit on this branch. One-line fix to unblock. --- .../src/__tests__/settings-accounts.test.ts | 47 +++++++++++++++++++ frontend/src/recommendations.ts | 2 +- frontend/src/settings.ts | 19 +++++--- 3 files changed, 60 insertions(+), 8 deletions(-) diff --git a/frontend/src/__tests__/settings-accounts.test.ts b/frontend/src/__tests__/settings-accounts.test.ts index 35d3e103..a9268070 100644 --- a/frontend/src/__tests__/settings-accounts.test.ts +++ b/frontend/src/__tests__/settings-accounts.test.ts @@ -4,6 +4,7 @@ */ import { loadAccountsForProvider, + loadOverridesPanel, setupSettingsHandlers } from '../settings'; @@ -925,4 +926,50 @@ describe('Account overrides modal', () => { // Verify it is gone — the only override container is the modal body. expect(document.querySelector('.account-overrides-panel')).toBeNull(); }); + + test('Delete-override button + dialog wording match the actual data semantics (issue #114)', async () => { + // The action DELETEs the override row; pre-#114 the dialog said + // "Reset … will be replaced" which implied a stuck-around row with + // new values. Pin the post-fix wording so a future regression doesn't + // silently re-introduce the mismatch. + (api.listAccountServiceOverrides as jest.Mock).mockResolvedValue([ + { + account_id: 'acc-1', + provider: 'aws', + service: 'ec2', + term: 1, + payment: 'no-upfront', + coverage: 80, + }, + ]); + mockConfirmDialog.mockResolvedValue(false); // user cancels — we don't need the API call to fire + + const panel = document.createElement('div'); + document.body.appendChild(panel); + await loadOverridesPanel('acc-1', panel, 'aws'); + + // The action button now reads "Delete" (not "Reset"). + const buttons = Array.from(panel.querySelectorAll('button')); + const deleteBtn = buttons.find(b => b.textContent === 'Delete'); + expect(deleteBtn).toBeDefined(); + expect(buttons.find(b => b.textContent === 'Reset')).toBeUndefined(); + + // Click it and inspect the confirmDialog opts. + deleteBtn!.click(); + await Promise.resolve(); // let the click handler's await chain start + expect(mockConfirmDialog).toHaveBeenCalledTimes(1); + const opts = mockConfirmDialog.mock.calls[0]![0] as { + title: string; + body: string; + confirmLabel: string; + }; + expect(opts.title).toBe('Delete override?'); + expect(opts.confirmLabel).toBe('Delete override'); + expect(opts.body).toContain('Delete the aws/ec2 override'); + expect(opts.body).toContain('revert to the global default'); + expect(opts.body).toContain('removed'); + // Pre-#114 wording must not appear. + expect(opts.body).not.toContain('replaced'); + expect(opts.body).not.toContain('Reset'); + }); }); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 5fdfaad1..322d1756 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -4,7 +4,7 @@ import * as api from './api'; import * as state from './state'; -import { formatCurrency, formatTerm, escapeHtml, formatRelativeTime } from './utils'; +import { formatCurrency, formatTerm, escapeHtml } from './utils'; import { renderFreshness } from './freshness'; import { getRecommendationDetail, type RecommendationDetail } from './api/recommendations'; import { showToast } from './toast'; diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 7bc636ee..103c0419 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -537,7 +537,7 @@ function buildPaymentOverrideSelect( // payment value but can't accidentally pick a no-op state. if (initial !== '') { inheritOpt.disabled = true; - inheritOpt.title = 'Use Reset to clear all override fields including payment'; + inheritOpt.title = 'Use Delete to remove the override entirely (clears all fields including payment)'; } select.addEventListener('change', () => { @@ -640,7 +640,7 @@ function closeAccountOverridesModal(): void { closeOverrideModal(); } -async function loadOverridesPanel(accountId: string, panel: HTMLElement, provider: AccountProvider): Promise { +export async function loadOverridesPanel(accountId: string, panel: HTMLElement, provider: AccountProvider): Promise { panel.textContent = 'Loading\u2026'; try { const overrides = await api.listAccountServiceOverrides(accountId); @@ -721,12 +721,17 @@ async function loadOverridesPanel(accountId: string, panel: HTMLElement, provide const resetBtn = document.createElement('button'); resetBtn.type = 'button'; resetBtn.className = 'btn btn-small btn-danger'; - resetBtn.textContent = 'Reset'; + // Button + dialog wording aligned with the actual data semantics + // (DELETE on account_service_overrides) per #114. The previous + // "Reset … will be replaced" copy implied the row stuck around + // with new values; in fact the row goes away entirely and the + // engine reads the global default as a side effect of its absence. + resetBtn.textContent = 'Delete'; resetBtn.addEventListener('click', async () => { const ok = await confirmDialog({ - title: 'Reset override?', - body: `Reset ${o.provider}/${o.service} override to the global default? Any per-service values you set will be replaced.`, - confirmLabel: 'Reset override', + title: 'Delete override?', + body: `Delete the ${o.provider}/${o.service} override? This account's recommendations will revert to the global default. The override row and any per-service values you set will be removed.`, + confirmLabel: 'Delete override', destructive: true, }); if (!ok) return; @@ -734,7 +739,7 @@ async function loadOverridesPanel(accountId: string, panel: HTMLElement, provide await api.deleteAccountServiceOverride(accountId, o.provider, o.service); await loadOverridesPanel(accountId, panel, provider); } catch (err) { - showToast({ message: `Failed to reset override: ${(err as Error).message}`, kind: 'error' }); + showToast({ message: `Failed to delete override: ${(err as Error).message}`, kind: 'error' }); } }); actionTd.appendChild(resetBtn);