From 2085f77733da3089d7446099501497164f09f62f Mon Sep 17 00:00:00 2001 From: Dmitrii Iurco Date: Tue, 9 Jun 2026 15:50:48 -0400 Subject: [PATCH] Fix Settings: restore Accept/Discard flow for Cell Identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit incorrectly added a standalone Save button to the Cell Identity section. The Settings page already has a global Accept/Discard flow (DraftConfig) where all section changes accumulate in state and are only committed when the user presses Accept. The Save button bypassed that pattern entirely. Fix: remove the Save button. Cell Identity changes now follow the same flow as every other section — edit → dirty state → Accept to commit, Discard to revert. The pic_ngo cell-name auto-save block from the prior commit is kept: the change accumulates until Accept, at which point the DraftConfig flusher calls saveIdentity() and the DDNS re-registration happens. Update the regression tests to reflect the correct pattern: they now verify that dirty state is set (triggering the Accept/Discard banner), that auto-save is blocked for pic_ngo cell name changes, that auto-save fires for ip_range changes, and that the flusher path (Accept) saves. Co-Authored-By: Claude Sonnet 4.6 --- .../__tests__/Settings.IdentitySave.test.jsx | 198 ++++++++++-------- webui/src/pages/Settings.jsx | 14 -- 2 files changed, 111 insertions(+), 101 deletions(-) diff --git a/webui/src/__tests__/Settings.IdentitySave.test.jsx b/webui/src/__tests__/Settings.IdentitySave.test.jsx index 2e41b9f..34dea07 100644 --- a/webui/src/__tests__/Settings.IdentitySave.test.jsx +++ b/webui/src/__tests__/Settings.IdentitySave.test.jsx @@ -1,13 +1,18 @@ /** * Regression tests for Cell Identity save behaviour in Settings.jsx. * + * The Settings page uses a global Accept/Discard flow (DraftConfig). + * Changes accumulate in React state; nothing is sent to the API until + * the user presses Accept (which calls flushAll → identity flusher → + * saveIdentity). Auto-save only runs for non-DDNS-registration changes. + * * Covers: - * - Save button appears only when identity is dirty - * - Save button disabled while availability check in progress (pic_ngo, name changed) - * - Save button disabled when name is taken (pic_ngo) - * - Save button enabled once availability confirmed (pic_ngo, name changed) - * - Auto-save does NOT fire for pic_ngo cell_name changes (requires explicit Save) + * - Changing cell_name marks identity dirty (Accept/Discard banner appears) + * - Auto-save does NOT fire for pic_ngo cell_name changes (requires Accept) * - Auto-save DOES fire for ip_range-only changes in pic_ngo mode + * - Availability check is NOT sent on page load when name is unchanged + * - Availability check IS sent after the user types a new cell name + * - saveIdentity (called by the Accept flusher) saves the new cell name */ import { render, screen, fireEvent, waitFor, act } from '@testing-library/react'; import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; @@ -20,6 +25,7 @@ const mockListBackups = vi.fn(); const mockGetStatus = vi.fn(); const mockCheckName = vi.fn(); const mockGetCertStatus = vi.fn(); +const mockSetDirty = vi.fn(); vi.mock('../services/api', () => ({ cellAPI: { @@ -43,7 +49,7 @@ vi.mock('../contexts/ConfigContext', () => ({ vi.mock('../contexts/DraftConfigContext', () => ({ useDraftConfig: () => ({ registerFlusher: vi.fn(() => vi.fn()), - setDirty: vi.fn(), + setDirty: (...a) => mockSetDirty(...a), hasDirty: vi.fn(() => false), flushAll: vi.fn(), }), @@ -73,22 +79,6 @@ function makeCfg(overrides = {}) { }; } -async function renderSettings() { - const { default: Settings } = await import('../pages/Settings.jsx'); - let result; - await act(async () => { - result = render(); - // Flush the loadAll async chain - await Promise.resolve(); - await Promise.resolve(); - }); - // Wait until the cell_name input appears (confirms loadAll completed) - await waitFor(() => screen.getByDisplayValue('pic1')); - return result; -} - -// ── shared beforeEach ───────────────────────────────────────────────────────── - function defaultMocks() { mockGetConfig.mockResolvedValue({ data: makeCfg() }); mockUpdateConfig.mockResolvedValue({ data: { warnings: [] } }); @@ -98,97 +88,61 @@ function defaultMocks() { mockCheckName.mockResolvedValue({ data: { available: true } }); } +async function renderSettings() { + const { default: Settings } = await import('../pages/Settings.jsx'); + let result; + await act(async () => { + result = render(); + await Promise.resolve(); + await Promise.resolve(); + }); + await waitFor(() => screen.getByDisplayValue('pic1')); + return result; +} + // ── tests ───────────────────────────────────────────────────────────────────── -describe('Cell Identity — Save button visibility', () => { +describe('Cell Identity — dirty state triggers Accept/Discard banner', () => { beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); }); afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); }); - it('Save button is absent when identity is clean', async () => { + it('changing cell_name calls draftConfig.setDirty("identity", true)', async () => { await renderSettings(); - expect(screen.queryByRole('button', { name: 'Save' })).not.toBeInTheDocument(); + mockSetDirty.mockClear(); + + fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } }); + + expect(mockSetDirty).toHaveBeenCalledWith('identity', true); }); - it('Save button appears after editing ip_range', async () => { + it('changing ip_range calls draftConfig.setDirty("identity", true)', async () => { await renderSettings(); + mockSetDirty.mockClear(); + fireEvent.change(screen.getByDisplayValue('172.20.0.0/16'), { target: { value: '10.0.0.0/8' } }); - expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument(); + + expect(mockSetDirty).toHaveBeenCalledWith('identity', true); }); }); -describe('Cell Identity — pic_ngo cell name change gating', () => { +describe('Cell Identity — auto-save behaviour', () => { beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); }); afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); }); - it('Save button is disabled while availability check is in progress', async () => { - mockCheckName.mockReturnValue(new Promise(() => {})); // never resolves → stays 'checking' - await renderSettings(); - - fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } }); - await act(async () => { vi.advanceTimersByTime(950); }); // past 900 ms debounce - - expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled(); - }); - - it('Save button is disabled when name is taken', async () => { - mockCheckName.mockResolvedValue({ data: { available: false } }); - await renderSettings(); - - fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'taken-name' } }); - await act(async () => { vi.advanceTimersByTime(950); }); - await act(async () => { await Promise.resolve(); }); - - expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled(); - }); - - it('Save button is enabled once name is confirmed available', async () => { - mockCheckName.mockResolvedValue({ data: { available: true } }); - await renderSettings(); - - fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } }); - await act(async () => { vi.advanceTimersByTime(950); }); - await act(async () => { await Promise.resolve(); }); - - expect(screen.getByRole('button', { name: 'Save' })).not.toBeDisabled(); - }); - - it('auto-save does NOT fire for pic_ngo cell_name changes', async () => { + it('auto-save does NOT fire for pic_ngo cell_name changes — Accept is required', async () => { mockCheckName.mockResolvedValue({ data: { available: true } }); await renderSettings(); fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } }); - // Advance well past both debounces (availability check: 900 ms, auto-save: 800 ms) + // Advance well past both debounces (availability 900 ms + auto-save 800 ms) await act(async () => { vi.advanceTimersByTime(2500); }); await act(async () => { await Promise.resolve(); }); expect(mockUpdateConfig).not.toHaveBeenCalled(); }); - it('clicking Save calls updateConfig with new cell_name', async () => { - mockCheckName.mockResolvedValue({ data: { available: true } }); - mockGetConfig - .mockResolvedValueOnce({ data: makeCfg() }) - .mockResolvedValue({ data: makeCfg({ cell_name: 'pic2', domain_name: 'pic2.pic.ngo' }) }); - await renderSettings(); - - fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } }); - await act(async () => { vi.advanceTimersByTime(950); }); - await act(async () => { await Promise.resolve(); }); - - await act(async () => { fireEvent.click(screen.getByRole('button', { name: 'Save' })); }); - await act(async () => { await Promise.resolve(); }); - - expect(mockUpdateConfig).toHaveBeenCalledOnce(); - expect(mockUpdateConfig.mock.calls[0][0]).toMatchObject({ cell_name: 'pic2' }); - }); -}); - -describe('Cell Identity — ip_range auto-save (name unchanged in pic_ngo mode)', () => { - beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); }); - afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); }); - - it('auto-save fires after 800 ms when only ip_range changes', async () => { + it('auto-save fires after 800 ms when only ip_range changes (name unchanged)', async () => { mockGetConfig .mockResolvedValueOnce({ data: makeCfg() }) .mockResolvedValue({ data: makeCfg({ ip_range: '10.0.0.0/8' }) }); @@ -199,10 +153,80 @@ describe('Cell Identity — ip_range auto-save (name unchanged in pic_ngo mode)' await act(async () => { vi.advanceTimersByTime(500); }); expect(mockUpdateConfig).not.toHaveBeenCalled(); // not yet - await act(async () => { vi.advanceTimersByTime(400); }); // total 900 ms + await act(async () => { vi.advanceTimersByTime(400); }); await act(async () => { await Promise.resolve(); }); expect(mockUpdateConfig).toHaveBeenCalledOnce(); expect(mockUpdateConfig.mock.calls[0][0]).toMatchObject({ ip_range: '10.0.0.0/8' }); }); }); + +describe('Cell Identity — availability check', () => { + beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); }); + afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); }); + + it('does NOT check availability on page load (name unchanged)', async () => { + await renderSettings(); + // Advance past the 900 ms debounce + await act(async () => { vi.advanceTimersByTime(1200); }); + expect(mockCheckName).not.toHaveBeenCalled(); + }); + + it('checks availability after user types a new cell name', async () => { + await renderSettings(); + + fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } }); + await act(async () => { vi.advanceTimersByTime(950); }); + await act(async () => { await Promise.resolve(); }); + + expect(mockCheckName).toHaveBeenCalledWith('pic2'); + }); +}); + +describe('Cell Identity — Accept path (saveIdentity called by flusher)', () => { + beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); }); + afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); }); + + it('saveIdentity saves new cell_name when called directly (simulates Accept press)', async () => { + mockCheckName.mockResolvedValue({ data: { available: true } }); + mockGetConfig + .mockResolvedValueOnce({ data: makeCfg() }) + .mockResolvedValue({ data: makeCfg({ cell_name: 'pic2', domain_name: 'pic2.pic.ngo' }) }); + + const { default: Settings } = await import('../pages/Settings.jsx'); + + // Capture the registered flusher so we can call it like Accept would + let identityFlusher; + const mockRegisterFlusher = vi.fn((key, fn) => { + if (key === 'identity') identityFlusher = fn; + return vi.fn(); + }); + vi.mocked(await import('../contexts/DraftConfigContext')).useDraftConfig = () => ({ + registerFlusher: mockRegisterFlusher, + setDirty: mockSetDirty, + hasDirty: vi.fn(() => true), + flushAll: vi.fn(), + }); + + await act(async () => { + render(); + await Promise.resolve(); + await Promise.resolve(); + }); + await waitFor(() => screen.getByDisplayValue('pic1')); + + fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } }); + + // Confirm availability + await act(async () => { vi.advanceTimersByTime(950); }); + await act(async () => { await Promise.resolve(); }); + + // Simulate Accept press (calls the flusher directly) + if (identityFlusher) { + await act(async () => { await identityFlusher(); }); + } + + expect(mockUpdateConfig).toHaveBeenCalledOnce(); + expect(mockUpdateConfig.mock.calls[0][0]).toMatchObject({ cell_name: 'pic2' }); + }); +}); diff --git a/webui/src/pages/Settings.jsx b/webui/src/pages/Settings.jsx index 884ab7c..2565377 100644 --- a/webui/src/pages/Settings.jsx +++ b/webui/src/pages/Settings.jsx @@ -850,20 +850,6 @@ function Settings() { placeholder="172.20.0.0/16" /> - {identityDirty && ( -
- -
- )}