fix(settings): rebase grace periods and enabled providers onto the server before saving

frontend/src/__tests__/settings-purchasing-grace.test.ts

@@ -229,16 +229,19 @@ describe('Settings → Purchasing grace-period inputs', () => {
     expect(toastArg.message).not.toMatch(/AZURE/);
   });
 
-  it('empty input defaults to 7 in the save payload', async () => {
-    (document.getElementById('setting-grace-aws') as HTMLInputElement).value = '';
+  it('an emptied grace input refuses the save instead of saving 7', async () => {
+    const aws = document.getElementById('setting-grace-aws') as HTMLInputElement;
+    aws.value = '';
+    aws.dispatchEvent(new Event('input'));
     (document.getElementById('setting-grace-azure') as HTMLInputElement).value = '0';
-    (document.getElementById('setting-grace-gcp') as HTMLInputElement).value = '5';
 
     await saveGlobalSettings(new Event("submit"));
 
-    expect(api.updateConfig).toHaveBeenCalledTimes(1);
-    const payload = (api.updateConfig as jest.Mock).mock.calls[0]![0];
-    expect(payload.grace_period_days).toEqual({ aws: 7, azure: 0, gcp: 5 });
+    expect(api.updateConfig).not.toHaveBeenCalled();
+    expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({
+      kind: 'error',
+      message: expect.stringContaining('(AWS)'),
+    }));
   });
 
   it('resetSettings restores every grace input to 7', async () => {
@@ -313,16 +316,16 @@ describe('Settings → Purchasing grace inputs: inline range validation', () =>
     expect(errorEl('setting-grace-azure')!.textContent).toBe('Must be a whole number between 0 and 30');
   });
 
-  it('clearing the field removes aria-invalid and hides the inline error', () => {
+  it('clearing the field shows the inline error (a grace period is required)', () => {
     const input = document.getElementById('setting-grace-aws') as HTMLInputElement;
     input.value = '99';
     fire(input, 'input');
     expect(input.getAttribute('aria-invalid')).toBe('true');
 
     input.value = '';
     fire(input, 'input');
-    expect(input.getAttribute('aria-invalid')).toBeNull();
-    expect(errorEl('setting-grace-aws')!.classList.contains('hidden')).toBe(true);
+    expect(input.getAttribute('aria-invalid')).toBe('true');
+    expect(errorEl('setting-grace-aws')!.classList.contains('hidden')).toBe(false);
   });
 
   it('typing a valid in-range value removes aria-invalid', () => {
@@ -353,3 +356,175 @@ describe('Settings → Purchasing grace inputs: inline range validation', () =>
     expect(toastArg.kind).toBe('error');
   });
 });
+
+/**
+ * #539: grace_period_days and enabled_providers are replaced whole by the
+ * backend, so Save re-reads the server's value and overlays only the fields
+ * this tab changed.
+ */
+describe('Settings → stale-tab Save rebases multi-field keys onto the server (#539)', () => {
+  const baseGlobal = {
+    default_term: 3, default_payment: 'all-upfront', default_coverage: 80, notification_days_before: 3,
+  };
+  const input = (id: string): HTMLInputElement => document.getElementById(id) as HTMLInputElement;
+  const sent = (): Record<string, unknown> => (api.updateConfig as jest.Mock).mock.calls[0]![0];
+  const submit = (): Promise<void> => saveGlobalSettings(new Event('submit'));
+
+  // Loads the tab's (soon stale) view, then makes the server return `server`.
+  const loadStaleTab = async (loaded: Record<string, unknown>, server: Record<string, unknown>): Promise<void> => {
+    (api.getConfig as jest.Mock).mockResolvedValueOnce({ global: { ...baseGlobal, ...loaded }, services: [] });
+    await loadGlobalSettings();
+    (api.getConfig as jest.Mock).mockClear().mockResolvedValue({ global: { ...baseGlobal, ...server }, services: [] });
+  };
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    seedDOM();
+    (api.updateConfig as jest.Mock).mockResolvedValue(undefined);
+  });
+
+  it('keeps another admin\'s grace value for a provider this tab did not change, and shows it', async () => {
+    await loadStaleTab(
+      { enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } },
+      { enabled_providers: ['aws'], grace_period_days: { aws: 3, azure: 7, gcp: 7 } },
+    );
+    input('setting-grace-azure').value = '9';
+
+    await submit();
+
+    expect(sent()).toEqual({ grace_period_days: { aws: 3, azure: 9, gcp: 7 } });
+    expect(input('setting-grace-aws').value).toBe('3');
+    (api.updateConfig as jest.Mock).mockClear();
+    await submit();
+    expect(api.updateConfig).not.toHaveBeenCalled();
+  });
+
+  it('keeps a typed-in grace edit made while the PUT is in flight', async () => {
+    await loadStaleTab(
+      { enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } },
+      { enabled_providers: ['aws'], grace_period_days: { aws: 3, azure: 7, gcp: 7 } },
+    );
+    input('setting-grace-azure').value = '9';
+    let resolvePut!: () => void;
+    (api.updateConfig as jest.Mock).mockImplementationOnce(() => new Promise<void>(r => { resolvePut = r; }));
+
+    const saving = submit();
+    await new Promise(r => setTimeout(r, 0));
+    input('setting-grace-aws').value = '12';
+    resolvePut();
+    await saving;
+
+    expect(input('setting-grace-aws').value).toBe('12');
+  });
+
+  it('keeps providers another admin added when this tab unchecks one', async () => {
+    await loadStaleTab(
+      { enabled_providers: ['aws', 'azure'] },
+      { enabled_providers: ['aws', 'azure', 'gcp'] },
+    );
+    input('provider-azure').checked = false;
+
+    await submit();
+
+    expect(sent()).toEqual({ enabled_providers: ['aws', 'gcp'] });
+  });
+
+  it('does not duplicate a provider the server already enabled', async () => {
+    await loadStaleTab(
+      { enabled_providers: ['aws'] },
+      { enabled_providers: ['aws', 'gcp'] },
+    );
+    input('provider-gcp').checked = true;
+
+    await submit();
+
+    expect(sent()).toEqual({ enabled_providers: ['aws', 'gcp'] });
+  });
+
+  it.each([
+    ['rejects', () => (api.getConfig as jest.Mock).mockRejectedValue(new Error('boom'))],
+    ['returns no global', () => (api.getConfig as jest.Mock).mockResolvedValue({ services: [] })],
+  ])('aborts the whole save, sending nothing, when the server read %s', async (_name, breakRead) => {
+    await loadStaleTab({ enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, {});
+    breakRead();
+    input('setting-grace-azure').value = '9';
+
+    await submit();
+
+    expect(api.updateConfig).not.toHaveBeenCalled();
+    expect(api.updateServiceConfig).not.toHaveBeenCalled();
+    expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' }));
+    (api.updateConfig as jest.Mock).mockClear();
+    (api.getConfig as jest.Mock).mockResolvedValue({
+      global: { ...baseGlobal, enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, services: [],
+    });
+    await submit();
+    expect(sent()).toEqual({ grace_period_days: { aws: 7, azure: 9, gcp: 7 } });
+  });
+
+  it('does not read the server or send grace when no grace input changed', async () => {
+    await loadStaleTab({ enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, {});
+    input('setting-notification-email').value = 'new@test.com';
+
+    await submit();
+
+    expect(api.getConfig).not.toHaveBeenCalled();
+    expect(sent()).toEqual({ notification_email: 'new@test.com' });
+  });
+
+  // Holds the server read open so the form can change between click and PUT.
+  const holdServerRead = (server: Record<string, unknown>): (() => void) => {
+    let release!: () => void;
+    (api.getConfig as jest.Mock).mockReset().mockImplementation(() => new Promise(r => {
+      release = () => r({ global: { ...baseGlobal, ...server }, services: [] });
+    }));
+    return () => release();
+  };
+
+  it('judges grace changes by the values captured at Save, not the form after the read', async () => {
+    await loadStaleTab({ enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, {});
+    input('setting-grace-azure').value = '9';
+    const release = holdServerRead({ enabled_providers: ['aws'], grace_period_days: { aws: 3, azure: 7, gcp: 7 } });
+
+    const saving = submit();
+    input('setting-grace-aws').value = '12';
+    release();
+    await saving;
+
+    expect(sent()).toEqual({ grace_period_days: { aws: 3, azure: 9, gcp: 7 } });
+  });
+
+  it('judges provider changes by the values captured at Save, not the form after the read', async () => {
+    await loadStaleTab({ enabled_providers: ['aws', 'azure'] }, {});
+    input('provider-azure').checked = false;
+    const release = holdServerRead({ enabled_providers: ['aws', 'azure', 'gcp'] });
+
+    const saving = submit();
+    input('provider-gcp').checked = true;
+    release();
+    await saving;
+
+    expect(sent()).toEqual({ enabled_providers: ['aws', 'gcp'] });
+  });
+
+  it('removes every copy of an unchecked provider the server lists twice', async () => {
+    await loadStaleTab({ enabled_providers: ['aws', 'azure'] }, { enabled_providers: ['aws', 'aws', 'azure'] });
+    input('provider-aws').checked = false;
+
+    await submit();
+
+    expect(sent()).toEqual({ enabled_providers: ['azure'] });
+  });
+
+  it('omits a provider the server has no grace value for when it is unchanged', async () => {
+    await loadStaleTab(
+      { enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } },
+      { enabled_providers: ['aws'], grace_period_days: { azure: 7, gcp: 7 } },
+    );
+    input('setting-grace-gcp').value = '4';
+
+    await submit();
+
+    expect(sent()).toEqual({ grace_period_days: { azure: 7, gcp: 4 } });
+  });
+});

frontend/src/settings.ts

@@ -222,17 +222,17 @@ let saveInFlight = false;
  *   byId<HTMLInputElement>('foo')?.value ?? '';  // read site
  */
 // Grace-period input: 0 is a valid user choice (disables the feature
-// for that provider), so we use empty-string to signal "fall back to
-// default (7)". An explicit 0 must round-trip as 0, not get swallowed.
+// for that provider), so an explicit 0 must round-trip as 0, not get
+// swallowed. A missing server value renders as the default (7).
 function populateGraceInput(id: string, value: number | undefined): void {
   const el = document.getElementById(id) as HTMLInputElement | null;
   if (!el) return;
   el.value = String(value ?? 7);
 }
 
 // readGraceInput parses the grace-period input, returning the numeric
-// value or an error message (out of range, not a number). Empty input
-// defaults to 7.
+// value or an error message (empty, out of range, not a number). Empty is
+// refused, not defaulted: a cleared field most likely means 0.
 //
 // The error wording mirrors what wireInlineRangeValidation renders
 // under the field on input/blur, so the inline indicator and the
@@ -243,9 +243,8 @@ function readGraceInput(id: string): { value: number; err?: undefined } | { valu
   const el = document.getElementById(id) as HTMLInputElement | null;
   if (!el) return { value: 7 };
   const raw = el.value.trim();
-  if (raw === '') return { value: 7 };
   const n = Number(raw);
-  if (!Number.isFinite(n) || !Number.isInteger(n) || n < 0 || n > 30) {
+  if (raw === '' || !Number.isFinite(n) || !Number.isInteger(n) || n < 0 || n > 30) {
     return { value: 0, err: 'Must be a whole number between 0 and 30' };
   }
   return { value: n };
@@ -2623,9 +2622,9 @@ export function setupSettingsHandlers(signal?: AbortSignal): void {
   wireInlineRangeValidation('setting-notification-days', { signal, requireInteger: true });
   wireInlineRangeValidation('setting-recs-stale-hours', { signal, requireInteger: true, required: true });
   wireInlineRangeValidation('setting-default-coverage', { signal, requireInteger: true, required: true });
-  wireInlineRangeValidation('setting-grace-aws', { signal, requireInteger: true });
-  wireInlineRangeValidation('setting-grace-azure', { signal, requireInteger: true });
-  wireInlineRangeValidation('setting-grace-gcp', { signal, requireInteger: true });
+  wireInlineRangeValidation('setting-grace-aws', { signal, requireInteger: true, required: true });
+  wireInlineRangeValidation('setting-grace-azure', { signal, requireInteger: true, required: true });
+  wireInlineRangeValidation('setting-grace-gcp', { signal, requireInteger: true, required: true });
   // Per-SP-plan coverage percentage inputs — must carry the same inline
   // validation as the global default-coverage field so users see range errors
   // on the service cards without waiting for Save. Issue #1411 row 7.13.
@@ -3467,6 +3466,52 @@ function showPropagatedCoverage(coverage: number, sentFields: Record<string, str
   }
 }
 
+const GRACE_INPUTS = [['aws', 'setting-grace-aws'], ['azure', 'setting-grace-azure'], ['gcp', 'setting-grace-gcp']] as const;
+
+
+/**
+ * grace_period_days and enabled_providers are replaced whole by the backend, so
+ * a tab opened before another admin's save would revert that admin's providers
+ * (#539). Re-read the server's current value and overlay only the fields this
+ * user changed, judged by the values captured at Save (the form may change while
+ * the read is pending). Rejects, leaving nothing sent, if the server value is unavailable.
+ */
+async function rebaseMultiFieldKeys(
+  changedGlobal: Partial<api.Config>,
+  isFieldChanged: (id: string) => boolean,
+): Promise<void> {
+  const fresh = await api.getConfig();
+  if (!fresh?.global) throw new Error('could not load the current settings to merge into');
+  if ('grace_period_days' in changedGlobal) {
+    const merged: Record<string, number> = { ...(fresh.global.grace_period_days ?? {}) };
+    for (const [provider, id] of GRACE_INPUTS) {
+      if (isFieldChanged(id)) merged[provider] = changedGlobal.grace_period_days![provider]!;
+    }
+    changedGlobal.grace_period_days = merged;
+  }
+  if ('enabled_providers' in changedGlobal) {
+    let merged: api.Provider[] = [...((fresh.global.enabled_providers ?? []) as api.Provider[])];
+    for (const provider of ['aws', 'azure', 'gcp'] as const) {
+      if (!isFieldChanged(`provider-${provider}`)) continue;
+      const at = merged.indexOf(provider);
+      const checked = changedGlobal.enabled_providers!.includes(provider);
+      if (checked && at < 0) merged.push(provider);
+      if (!checked) merged = merged.filter(p => p !== provider);
+    }
+    changedGlobal.enabled_providers = merged;
+  }
+}
+
+/** Show the persisted grace map, keeping any input the user edited while the save ran. */
+function showSavedGrace(saved: Record<string, number>, sentFields: Record<string, string>): void {
+  for (const [provider, id] of GRACE_INPUTS) {
+    if (getFieldValue(id) !== sentFields[id]) continue;
+    populateGraceInput(id, saved[provider]);
+    savedSnapshot[id] = sentFields[id] = getFieldValue(id);
+  }
+  revalidateInlineRanges();
+}
+
 /**
  * Save global settings
  */
@@ -3504,7 +3549,7 @@ export async function saveGlobalSettings(e: Event): Promise<void> {
   // reasons.
   const gracePeriodDays: Record<string, number> = {};
   const graceErrors: string[] = [];
-  for (const [provider, id] of [['aws', 'setting-grace-aws'], ['azure', 'setting-grace-azure'], ['gcp', 'setting-grace-gcp']] as const) {
+  for (const [provider, id] of GRACE_INPUTS) {
     const v = readGraceInput(id);
     if (v.err) {
       graceErrors.push(provider.toUpperCase());
@@ -3599,19 +3644,24 @@ export async function saveGlobalSettings(e: Event): Promise<void> {
     // Save button are unreachable after a failed first load, so the snapshot is
     // always populated here. laddering_enabled is not part of Save: its toggle
     // saves itself (ladder.ts). Multi-field keys (grace map, enabled_providers)
-    // are sent whole, because the backend replaces them as a unit.
+    // are replaced whole by the backend, so rebaseMultiFieldKeys merges them first.
     const changedGlobal: Partial<api.Config> = { ...settings };
+    const isFieldChanged = (id: string): boolean => sentFields[id] !== savedSnapshot[id];
     for (const [key, ids] of GLOBAL_CONFIG_FIELDS) {
-      if (ids.every(id => getFieldValue(id) === savedSnapshot[id])) delete changedGlobal[key];
+      if (!ids.some(isFieldChanged)) delete changedGlobal[key];
     }
     const globalSent = Object.keys(changedGlobal).length > 0;
     if (globalSent) {
+      if ('grace_period_days' in changedGlobal || 'enabled_providers' in changedGlobal) {
+        await rebaseMultiFieldKeys(changedGlobal, isFieldChanged);
+      }
       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.
       for (const [key, ids] of GLOBAL_CONFIG_FIELDS) {
         if (key in changedGlobal) ids.forEach(id => { savedSnapshot[id] = sentFields[id]!; });
       }
+      if (changedGlobal.grace_period_days) showSavedGrace(changedGlobal.grace_period_days, sentFields);
     }
     if (defaultCoverageChanged) {
       // The default is persisted and propagated; mirror that locally so a retry