From a89a19cd31a509f093163e7a979fa44b5d18f014 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 24 Oct 2026 00:41:35 +0000 Subject: [PATCH] fix(settings): report a fully denied Settings Save as one failure, not 17 Since #614 gated per-service config PUTs on unrestricted account scope, a scoped admin's Save fires all 17 service PUTs and every one returns 403. Promise.allSettled kept the save going, and the toast read "Settings saved, but 17 services failed: " even though nothing was saved. Only say "Settings saved, but ..." when something did save (the global PUT went out or at least one service PUT succeeded). When nothing saved and every service PUT was permission-denied, show the backend's single reason in the same "Failed to save settings: permission denied: ..." form the global PUT already uses. When nothing saved for other reasons, say the save failed and list the services. The dirty-snapshot handling is unchanged: failed services keep their fields dirty for a retry. Refs #617 Co-Authored-By: Claude Opus 5.5 --- .../settings-service-payment.test.ts | 57 +++++++++++++++++++ frontend/src/settings.ts | 33 +++++++++-- 2 files changed, 85 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/settings-service-payment.test.ts b/frontend/src/__tests__/settings-service-payment.test.ts index 1f2b8fe37..0b6d22df4 100644 --- a/frontend/src/__tests__/settings-service-payment.test.ts +++ b/frontend/src/__tests__/settings-service-payment.test.ts @@ -170,6 +170,63 @@ describe('Settings Save payment tokens (issue #545)', () => { expect(awsPayment.classList.contains('dirty')).toBe(false); consoleError.mockRestore(); }); + // Issue #617: after #614 a scoped admin's service PUTs all 403. With no + // global change and no service saved, the toast must not claim "Settings + // saved" or repeat the same denial 17 times. + function denied(): Error { + return Object.assign(new Error('permission denied: global configuration requires unrestricted account access'), { status: 403 }); + } + + it('scoped admin: every service PUT denied shows one permission-denied failure toast', async () => { + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => undefined); + (api.updateServiceConfig as jest.Mock).mockRejectedValue(denied()); + await loadGlobalSettings(); + const awsPayment = document.getElementById('aws-ec2-payment') as HTMLSelectElement; + awsPayment.value = 'no-upfront'; + + await saveGlobalSettings(new Event('submit')); + + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledTimes(1); + expect(mockShowToast).toHaveBeenCalledWith({ + message: 'Failed to save settings: permission denied: global configuration requires unrestricted account access', + kind: 'error', + }); + // Nothing saved, so the edited card stays dirty for a retry. + expect(awsPayment.classList.contains('dirty')).toBe(true); + consoleError.mockRestore(); + }); + + it('every service PUT failing for other reasons says the save failed, listing each service', async () => { + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => undefined); + (api.updateServiceConfig as jest.Mock).mockImplementation(async (provider: string, service: string) => { + throw provider === 'aws' && service === 'ec2' ? new Error('boom') : denied(); + }); + await loadGlobalSettings(); + + await saveGlobalSettings(new Event('submit')); + + const { message } = mockShowToast.mock.calls[0]![0] as { message: string }; + expect(message).toMatch(/^Failed to save settings: 17 services failed: /); + expect(message).toContain('aws/ec2: boom'); + consoleError.mockRestore(); + }); + + it('a saved global change still reports "Settings saved, but" when every service PUT fails', async () => { + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => undefined); + (api.updateServiceConfig as jest.Mock).mockRejectedValue(new Error('unavailable')); + await loadGlobalSettings(); + const notify = document.getElementById('setting-notification-days') as HTMLInputElement; + notify.value = '9'; + + await saveGlobalSettings(new Event('submit')); + + expect(api.updateConfig).toHaveBeenCalledTimes(1); + const { message } = mockShowToast.mock.calls[0]![0] as { message: string }; + expect(message).toMatch(/^Settings saved, but 17 services failed: /); + consoleError.mockRestore(); + }); + it.each([ ['no-upfront', 'monthly'], ['partial-upfront', 'monthly'], diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index efbed45af..7725a70d9 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3558,7 +3558,8 @@ export async function saveGlobalSettings(e: Event): Promise { for (const [key, ids] of GLOBAL_CONFIG_FIELDS) { if (ids.every(id => getFieldValue(id) === savedSnapshot[id])) delete changedGlobal[key]; } - if (Object.keys(changedGlobal).length > 0) { + const globalSent = Object.keys(changedGlobal).length > 0; + if (globalSent) { await api.updateConfig(changedGlobal); // Persisted: advance the snapshot now, so a later validation failure or // service-save error does not leave these fields looking unsaved. @@ -3678,10 +3679,7 @@ export async function saveGlobalSettings(e: Event): Promise { if (failures.length > 0) { console.error('Failed to save service configs:', failures); - showToast({ - message: `Settings saved, but ${failures.length} service${failures.length === 1 ? '' : 's'} failed: ${failures.join('; ')}`, - kind: 'error', - }); + showToast({ message: serviceSaveFailureMessage(results, failures, globalSent), kind: 'error' }); } else { showToast({ message: 'Settings saved successfully', kind: 'success', timeout: 5_000 }); } @@ -3695,6 +3693,31 @@ export async function saveGlobalSettings(e: Event): Promise { } } +/** + * Toast text for a Save where at least one service PUT failed. Only claim + * "Settings saved" when something was: the global PUT went out or a service + * PUT succeeded. When nothing saved and every service PUT was denied (a + * scoped admin after #614 gated service config on unrestricted account + * scope), show the single backend reason, matching the global PUT's + * "Failed to save settings: permission denied: ..." toast instead of + * repeating it once per service (#617). + */ +function serviceSaveFailureMessage( + results: PromiseSettledResult[], + failures: string[], + globalSent: boolean, +): string { + const savedSomething = globalSent || results.some(r => r.status === 'fulfilled'); + if (savedSomething) { + return `Settings saved, but ${failures.length} service${failures.length === 1 ? '' : 's'} failed: ${failures.join('; ')}`; + } + const reasons = results.map(r => (r.status === 'rejected' ? r.reason : null)); + if (reasons.every(isPermissionDeniedError)) { + return `Failed to save settings: ${(reasons[0] as Error).message}`; + } + return `Failed to save settings: ${failures.length} service${failures.length === 1 ? '' : 's'} failed: ${failures.join('; ')}`; +} + /** * Reset settings to defaults */