Skip to content

Commit a86bd4d

Browse files
authored
fix(ux/settings): reset-override dialog wording matches data semantics (#174)
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.
1 parent cedcade commit a86bd4d

2 files changed

Lines changed: 59 additions & 7 deletions

File tree

frontend/src/__tests__/settings-accounts.test.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55
import {
66
loadAccountsForProvider,
7+
loadOverridesPanel,
78
setupSettingsHandlers
89
} from '../settings';
910

@@ -989,4 +990,50 @@ describe('Account overrides modal', () => {
989990
// Verify it is gone — the only override container is the modal body.
990991
expect(document.querySelector('.account-overrides-panel')).toBeNull();
991992
});
993+
994+
test('Delete-override button + dialog wording match the actual data semantics (issue #114)', async () => {
995+
// The action DELETEs the override row; pre-#114 the dialog said
996+
// "Reset … will be replaced" which implied a stuck-around row with
997+
// new values. Pin the post-fix wording so a future regression doesn't
998+
// silently re-introduce the mismatch.
999+
(api.listAccountServiceOverrides as jest.Mock).mockResolvedValue([
1000+
{
1001+
account_id: 'acc-1',
1002+
provider: 'aws',
1003+
service: 'ec2',
1004+
term: 1,
1005+
payment: 'no-upfront',
1006+
coverage: 80,
1007+
},
1008+
]);
1009+
mockConfirmDialog.mockResolvedValue(false); // user cancels — we don't need the API call to fire
1010+
1011+
const panel = document.createElement('div');
1012+
document.body.appendChild(panel);
1013+
await loadOverridesPanel('acc-1', panel, 'aws');
1014+
1015+
// The action button now reads "Delete" (not "Reset").
1016+
const buttons = Array.from(panel.querySelectorAll('button'));
1017+
const deleteBtn = buttons.find(b => b.textContent === 'Delete');
1018+
expect(deleteBtn).toBeDefined();
1019+
expect(buttons.find(b => b.textContent === 'Reset')).toBeUndefined();
1020+
1021+
// Click it and inspect the confirmDialog opts.
1022+
deleteBtn!.click();
1023+
await Promise.resolve(); // let the click handler's await chain start
1024+
expect(mockConfirmDialog).toHaveBeenCalledTimes(1);
1025+
const opts = mockConfirmDialog.mock.calls[0]![0] as {
1026+
title: string;
1027+
body: string;
1028+
confirmLabel: string;
1029+
};
1030+
expect(opts.title).toBe('Delete override?');
1031+
expect(opts.confirmLabel).toBe('Delete override');
1032+
expect(opts.body).toContain('Delete the aws/ec2 override');
1033+
expect(opts.body).toContain('revert to the global default');
1034+
expect(opts.body).toContain('removed');
1035+
// Pre-#114 wording must not appear.
1036+
expect(opts.body).not.toContain('replaced');
1037+
expect(opts.body).not.toContain('Reset');
1038+
});
9921039
});

frontend/src/settings.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -556,7 +556,7 @@ function buildPaymentOverrideSelect(
556556
// payment value but can't accidentally pick a no-op state.
557557
if (initial !== '') {
558558
inheritOpt.disabled = true;
559-
inheritOpt.title = 'Use Reset to clear all override fields including payment';
559+
inheritOpt.title = 'Use Delete to remove the override entirely (clears all fields including payment)';
560560
}
561561

562562
select.addEventListener('change', () => {
@@ -660,7 +660,7 @@ function closeAccountOverridesModal(): void {
660660
closeOverrideModal();
661661
}
662662

663-
async function loadOverridesPanel(accountId: string, panel: HTMLElement, provider: AccountProvider): Promise<void> {
663+
export async function loadOverridesPanel(accountId: string, panel: HTMLElement, provider: AccountProvider): Promise<void> {
664664
panel.textContent = 'Loading\u2026';
665665
try {
666666
const overrides = await api.listAccountServiceOverrides(accountId);
@@ -741,12 +741,17 @@ async function loadOverridesPanel(accountId: string, panel: HTMLElement, provide
741741
const resetBtn = document.createElement('button');
742742
resetBtn.type = 'button';
743743
resetBtn.className = 'btn btn-small btn-danger';
744-
resetBtn.textContent = 'Reset';
744+
// Button + dialog wording aligned with the actual data semantics
745+
// (DELETE on account_service_overrides) per #114. The previous
746+
// "Reset … will be replaced" copy implied the row stuck around
747+
// with new values; in fact the row goes away entirely and the
748+
// engine reads the global default as a side effect of its absence.
749+
resetBtn.textContent = 'Delete';
745750
resetBtn.addEventListener('click', async () => {
746751
const ok = await confirmDialog({
747-
title: 'Reset override?',
748-
body: `Reset ${o.provider}/${o.service} override to the global default? Any per-service values you set will be replaced.`,
749-
confirmLabel: 'Reset override',
752+
title: 'Delete override?',
753+
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.`,
754+
confirmLabel: 'Delete override',
750755
destructive: true,
751756
});
752757
if (!ok) return;
@@ -755,7 +760,7 @@ async function loadOverridesPanel(accountId: string, panel: HTMLElement, provide
755760
await loadOverridesPanel(accountId, panel, provider);
756761
await refreshRecommendationsAfterOverrideChange();
757762
} catch (err) {
758-
showToast({ message: `Failed to reset override: ${(err as Error).message}`, kind: 'error' });
763+
showToast({ message: `Failed to delete override: ${(err as Error).message}`, kind: 'error' });
759764
}
760765
});
761766
actionTd.appendChild(resetBtn);

0 commit comments

Comments
 (0)