diff --git a/ui/litellm-dashboard/src/components/DefaultUserSettings.test.tsx b/ui/litellm-dashboard/src/components/DefaultUserSettings.test.tsx index 78d50fa7f31..6fc83d79d98 100644 --- a/ui/litellm-dashboard/src/components/DefaultUserSettings.test.tsx +++ b/ui/litellm-dashboard/src/components/DefaultUserSettings.test.tsx @@ -1,6 +1,6 @@ import { act, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { beforeEach, describe, expect, it, vi } from "vitest"; -import DefaultUserSettings from "./DefaultUserSettings"; +import DefaultUserSettings, { computeSettingsDiff } from "./DefaultUserSettings"; import * as networking from "./networking"; vi.mock("./networking", () => ({ @@ -20,6 +20,10 @@ vi.mock("./common_components/budget_duration_dropdown", () => ({ getBudgetDurationLabel: (value: string) => value, })); +vi.mock("@/utils/dataUtils", () => ({ + formatNumberWithCommas: (n: number) => String(n), +})); + vi.mock("./key_team_helpers/fetch_available_models_team_key", () => ({ getModelDisplayName: (model: string) => model, })); @@ -115,13 +119,11 @@ describe("DefaultUserSettings", () => { expect(screen.queryByText("Edit Settings")).not.toBeInTheDocument(); }); - it("should save settings when save button is clicked", async () => { + it("should save settings directly when no destructive changes", async () => { + // No changes at all → direct save (no modal) mockGetInternalUserSettings.mockResolvedValue(mockSettings); mockUpdateInternalUserSettings.mockResolvedValue({ - settings: { - ...mockSettings.values, - max_budget: 2000, - }, + settings: mockSettings.values, }); render(); @@ -148,6 +150,375 @@ describe("DefaultUserSettings", () => { expect(mockUpdateInternalUserSettings).toHaveBeenCalled(); }); + // No modal should appear + expect(screen.queryByText("Review Changes")).not.toBeInTheDocument(); expect(screen.getByText("Edit Settings")).toBeInTheDocument(); }); + + it("should show confirmation modal when a team is removed", async () => { + const settingsWithTeams = { + ...mockSettings, + values: { + ...mockSettings.values, + teams: [ + { team_id: "team-alpha", max_budget_in_team: 50, user_role: "user" }, + { team_id: "team-beta", max_budget_in_team: 25, user_role: "admin" }, + ], + }, + }; + mockGetInternalUserSettings.mockResolvedValue(settingsWithTeams); + + render(); + + await waitFor(() => { + expect(screen.getByText("Edit Settings")).toBeInTheDocument(); + }); + + // Enter edit mode + act(() => { + fireEvent.click(screen.getByText("Edit Settings")); + }); + + // Remove the first team + const removeButtons = screen.getAllByText("Remove"); + act(() => { + fireEvent.click(removeButtons[0]); + }); + + // Click Save Changes + act(() => { + fireEvent.click(screen.getByText("Save Changes")); + }); + + // Modal should appear + await waitFor(() => { + expect(screen.getByText("Review Changes")).toBeInTheDocument(); + }); + + // Should mention the removed team + expect(screen.getByText(/team-alpha/)).toBeInTheDocument(); + + // Save should NOT have been called yet + expect(mockUpdateInternalUserSettings).not.toHaveBeenCalled(); + }); + + it("should save after confirming in the modal", async () => { + const settingsWithTeams = { + ...mockSettings, + values: { + ...mockSettings.values, + teams: [ + { team_id: "team-alpha", max_budget_in_team: 50, user_role: "user" }, + { team_id: "team-beta", max_budget_in_team: 25, user_role: "admin" }, + ], + }, + }; + mockGetInternalUserSettings.mockResolvedValue(settingsWithTeams); + mockUpdateInternalUserSettings.mockResolvedValue({ + settings: { + ...settingsWithTeams.values, + teams: [{ team_id: "team-beta", max_budget_in_team: 25, user_role: "admin" }], + }, + }); + + render(); + + await waitFor(() => { + expect(screen.getByText("Edit Settings")).toBeInTheDocument(); + }); + + // Enter edit mode, remove first team, click Save + act(() => { + fireEvent.click(screen.getByText("Edit Settings")); + }); + const removeButtons = screen.getAllByText("Remove"); + act(() => { + fireEvent.click(removeButtons[0]); + }); + act(() => { + fireEvent.click(screen.getByText("Save Changes")); + }); + + // Wait for modal + await waitFor(() => { + expect(screen.getByText("Review Changes")).toBeInTheDocument(); + }); + + // Click Confirm + await act(async () => { + fireEvent.click(screen.getByText("Confirm Changes")); + }); + + // Save should have been called + await waitFor(() => { + expect(mockUpdateInternalUserSettings).toHaveBeenCalledTimes(1); + }); + + // The saved teams should not include team-alpha + const sentValues = mockUpdateInternalUserSettings.mock.calls[0][1]; + const sentTeamIds = sentValues.teams.map((t: any) => t.team_id); + expect(sentTeamIds).not.toContain("team-alpha"); + expect(sentTeamIds).toContain("team-beta"); + }); + + it("should NOT save when modal is cancelled", async () => { + const settingsWithTeams = { + ...mockSettings, + values: { + ...mockSettings.values, + teams: [ + { team_id: "team-alpha", max_budget_in_team: 50, user_role: "user" }, + ], + }, + }; + mockGetInternalUserSettings.mockResolvedValue(settingsWithTeams); + + render(); + + await waitFor(() => { + expect(screen.getByText("Edit Settings")).toBeInTheDocument(); + }); + + // Enter edit mode, remove team, click Save + act(() => { + fireEvent.click(screen.getByText("Edit Settings")); + }); + const removeButtons = screen.getAllByText("Remove"); + act(() => { + fireEvent.click(removeButtons[0]); + }); + act(() => { + fireEvent.click(screen.getByText("Save Changes")); + }); + + // Wait for modal + await waitFor(() => { + expect(screen.getByText("Review Changes")).toBeInTheDocument(); + }); + + // Click Cancel inside the modal (there are two Cancel buttons: one in the + // edit header and one in the modal footer). The modal's Cancel is the last + // one rendered. + const cancelButtons = screen.getAllByText("Cancel"); + await act(async () => { + fireEvent.click(cancelButtons[cancelButtons.length - 1]); + }); + + // Save should NOT have been called + expect(mockUpdateInternalUserSettings).not.toHaveBeenCalled(); + + // Should still be in edit mode (Save Changes button visible) + expect(screen.getByText("Save Changes")).toBeInTheDocument(); + }); + + it("should keep modal open when API save fails", async () => { + const settingsWithTeams = { + ...mockSettings, + values: { + ...mockSettings.values, + teams: [ + { team_id: "team-alpha", max_budget_in_team: 50, user_role: "user" }, + ], + }, + }; + mockGetInternalUserSettings.mockResolvedValue(settingsWithTeams); + mockUpdateInternalUserSettings.mockRejectedValue(new Error("Server error")); + + render(); + + await waitFor(() => { + expect(screen.getByText("Edit Settings")).toBeInTheDocument(); + }); + + // Enter edit mode, remove team, click Save → modal opens + act(() => { + fireEvent.click(screen.getByText("Edit Settings")); + }); + act(() => { + fireEvent.click(screen.getAllByText("Remove")[0]); + }); + act(() => { + fireEvent.click(screen.getByText("Save Changes")); + }); + + await waitFor(() => { + expect(screen.getByText("Review Changes")).toBeInTheDocument(); + }); + + // Click Confirm — API will reject + await act(async () => { + fireEvent.click(screen.getByText("Confirm Changes")); + }); + + // Modal should still be open (not cleared on failure) + await waitFor(() => { + expect(screen.getByText("Review Changes")).toBeInTheDocument(); + expect(screen.getByText("Confirm Changes")).toBeInTheDocument(); + }); + }); + + it("should reflect API response values after successful save", async () => { + mockGetInternalUserSettings.mockResolvedValue(mockSettings); + // API returns different max_budget than what was sent + mockUpdateInternalUserSettings.mockResolvedValue({ + settings: { + ...mockSettings.values, + max_budget: 2000, + }, + }); + + render(); + + await waitFor(() => { + expect(screen.getByText("Edit Settings")).toBeInTheDocument(); + }); + + act(() => { + fireEvent.click(screen.getByText("Edit Settings")); + }); + + const saveButton = screen.getByText("Save Changes"); + act(() => { + fireEvent.click(saveButton); + }); + + await waitFor(() => { + expect(mockUpdateInternalUserSettings).toHaveBeenCalled(); + }); + + // Should exit edit mode and show the API response value + expect(screen.getByText("Edit Settings")).toBeInTheDocument(); + expect(screen.getByText("2000")).toBeInTheDocument(); + }); +}); + +// --------------------------------------------------------------------------- +// Tests: computeSettingsDiff (pure function) +// --------------------------------------------------------------------------- + +describe("computeSettingsDiff", () => { + it("returns no changes when values are identical", () => { + const values = { max_budget: 100, models: ["gpt-4"], teams: [] }; + const { changes, hasDestructiveChanges } = computeSettingsDiff(values, { ...values }); + expect(changes).toHaveLength(0); + expect(hasDestructiveChanges).toBe(false); + }); + + it("detects team removal as destructive", () => { + const original = { + teams: [ + { team_id: "team-alpha", user_role: "user" }, + { team_id: "team-beta", user_role: "admin" }, + ], + }; + const edited = { + teams: [{ team_id: "team-alpha", user_role: "user" }], + }; + + const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited); + expect(hasDestructiveChanges).toBe(true); + expect(changes).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + type: "removed", + details: expect.stringContaining("team-beta"), + }), + ]), + ); + }); + + it("detects team addition as non-destructive", () => { + const original = { teams: [] }; + const edited = { teams: [{ team_id: "new-team", user_role: "user" }] }; + + const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited); + expect(hasDestructiveChanges).toBe(false); + expect(changes).toEqual( + expect.arrayContaining([expect.objectContaining({ type: "added" })]), + ); + }); + + it("detects team budget change as destructive", () => { + const original = { teams: [{ team_id: "t1", max_budget_in_team: 100, user_role: "user" }] }; + const edited = { teams: [{ team_id: "t1", max_budget_in_team: 50, user_role: "user" }] }; + + const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited); + expect(hasDestructiveChanges).toBe(true); + expect(changes[0].type).toBe("changed"); + expect(changes[0].details).toContain("$100"); + expect(changes[0].details).toContain("$50"); + }); + + it("detects model removal as destructive", () => { + const original = { models: ["gpt-4", "gpt-3.5-turbo"] }; + const edited = { models: ["gpt-4"] }; + + const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited); + expect(hasDestructiveChanges).toBe(true); + expect(changes).toEqual( + expect.arrayContaining([ + expect.objectContaining({ type: "removed", details: expect.stringContaining("gpt-3.5-turbo") }), + ]), + ); + }); + + it("detects model addition as non-destructive", () => { + const original = { models: ["gpt-4"] }; + const edited = { models: ["gpt-4", "claude-3"] }; + + const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited); + expect(hasDestructiveChanges).toBe(false); + expect(changes).toEqual( + expect.arrayContaining([expect.objectContaining({ type: "added" })]), + ); + }); + + it("detects scalar field cleared as destructive", () => { + const original = { max_budget: 100 }; + const edited = { max_budget: null }; + + const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited); + expect(hasDestructiveChanges).toBe(true); + expect(changes[0].type).toBe("removed"); + expect(changes[0].details).toContain("Cleared"); + }); + + it("detects scalar field set as non-destructive", () => { + const original = { max_budget: null }; + const edited = { max_budget: 200 }; + + const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited); + expect(hasDestructiveChanges).toBe(false); + expect(changes[0].type).toBe("added"); + }); + + it("detects scalar value change as destructive", () => { + const original = { user_role: "internal_user" }; + const edited = { user_role: "internal_user_view_only" }; + + const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited); + expect(hasDestructiveChanges).toBe(true); + expect(changes[0].type).toBe("changed"); + }); + + it("handles string team_ids in original", () => { + const original = { teams: ["team-alpha", "team-beta"] }; + const edited = { teams: [{ team_id: "team-alpha", user_role: "user" }] }; + + const { changes, hasDestructiveChanges } = computeSettingsDiff(original, edited); + expect(hasDestructiveChanges).toBe(true); + expect(changes).toEqual( + expect.arrayContaining([ + expect.objectContaining({ type: "removed", details: expect.stringContaining("team-beta") }), + ]), + ); + }); + + it("treats null and undefined as equal for scalars", () => { + const original = { max_budget: null }; + const edited = { max_budget: undefined }; + + const { changes } = computeSettingsDiff(original, edited); + expect(changes).toHaveLength(0); + }); }); diff --git a/ui/litellm-dashboard/src/components/DefaultUserSettings.tsx b/ui/litellm-dashboard/src/components/DefaultUserSettings.tsx index 314946c520b..bab56fc1095 100644 --- a/ui/litellm-dashboard/src/components/DefaultUserSettings.tsx +++ b/ui/litellm-dashboard/src/components/DefaultUserSettings.tsx @@ -4,6 +4,7 @@ import { Button, Typography, Spin, Switch, Select, InputNumber } from "antd"; import { PlusOutlined, DeleteOutlined } from "@ant-design/icons"; import { getInternalUserSettings, updateInternalUserSettings, modelAvailableCall } from "./networking"; import BudgetDurationDropdown, { getBudgetDurationLabel } from "./common_components/budget_duration_dropdown"; +import ConfirmSettingsChangeModal, { SettingsChange } from "./common_components/ConfirmSettingsChangeModal"; import { getModelDisplayName } from "./key_team_helpers/fetch_available_models_team_key"; import { formatNumberWithCommas } from "@/utils/dataUtils"; import NotificationManager from "./molecules/notifications_manager"; @@ -21,6 +22,98 @@ interface TeamEntry { user_role: "user" | "admin"; } +/** Shape returned by the GET endpoint and sent back on PATCH. */ +interface DefaultUserSettingsValues { + user_role?: string | null; + max_budget?: number | null; + budget_duration?: string | null; + models?: string[] | null; + teams?: (string | TeamEntry)[] | null; +} + +interface SettingsResponse { + values: DefaultUserSettingsValues; + field_schema: { + description?: string; + properties: Record; + }; +} + +/** Normalize the backend's union type (string | TeamEntry) to TeamEntry[]. */ +function normalizeTeams(teams: (string | TeamEntry)[]): TeamEntry[] { + return teams.map((team) => + typeof team === "string" + ? { team_id: team, user_role: "user" as const } + : { team_id: team.team_id, max_budget_in_team: team.max_budget_in_team, user_role: team.user_role || "user" }, + ); +} + +/** + * Compare original settings values against edited values and return a list of + * human-readable changes. A change is "destructive" if something was removed, + * cleared, or reduced. + */ +export function computeSettingsDiff( + original: DefaultUserSettingsValues, + edited: DefaultUserSettingsValues, +): { changes: SettingsChange[]; hasDestructiveChanges: boolean } { + const changes: SettingsChange[] = []; + + // --- Teams --- + const oldTeams = normalizeTeams(original.teams || []); + const newTeams = normalizeTeams(edited.teams || []); + const oldTeamMap = new Map(oldTeams.map((t) => [t.team_id, t])); + const newTeamIds = new Set(newTeams.map((t) => t.team_id)); + + for (const t of oldTeams) { + if (!newTeamIds.has(t.team_id)) { + changes.push({ field: "Teams", type: "removed", details: `Team "${t.team_id}" removed` }); + } + } + for (const t of newTeams) { + if (!t.team_id) continue; + const old = oldTeamMap.get(t.team_id); + if (!old) { + changes.push({ field: "Teams", type: "added", details: `Team "${t.team_id}" added` }); + continue; + } + if (old.max_budget_in_team !== t.max_budget_in_team) { + const fmt = (v?: number) => (v !== undefined ? `$${v}` : "No limit"); + changes.push({ field: "Teams", type: "changed", details: `Team "${t.team_id}" budget: ${fmt(old.max_budget_in_team)} → ${fmt(t.max_budget_in_team)}` }); + } + if (old.user_role !== t.user_role) { + changes.push({ field: "Teams", type: "changed", details: `Team "${t.team_id}" role: ${old.user_role} → ${t.user_role}` }); + } + } + + // --- Models --- + const oldModels = new Set(original.models || []); + const newModels = new Set(edited.models || []); + const removedModels = [...oldModels].filter((m) => !newModels.has(m)); + const addedModels = [...newModels].filter((m) => !oldModels.has(m)); + if (removedModels.length) changes.push({ field: "Models", type: "removed", details: `${removedModels.join(", ")} removed` }); + if (addedModels.length) changes.push({ field: "Models", type: "added", details: `${addedModels.join(", ")} added` }); + + // --- Scalars --- + const scalarKeys = ["user_role", "max_budget", "budget_duration"] as const; + for (const key of scalarKeys) { + const oldVal = original[key] ?? null; + const newVal = edited[key] ?? null; + if (oldVal === newVal) continue; + + const displayName = key.replace(/_/g, " ").replace(/\b\w/g, (l) => l.toUpperCase()); + if (oldVal != null && newVal == null) { + changes.push({ field: displayName, type: "removed", details: `Cleared (was "${oldVal}")` }); + } else if (oldVal == null && newVal != null) { + changes.push({ field: displayName, type: "added", details: `Set to "${newVal}"` }); + } else { + changes.push({ field: displayName, type: "changed", details: `"${oldVal}" → "${newVal}"` }); + } + } + + return { changes, hasDestructiveChanges: changes.some((c) => c.type === "removed" || c.type === "changed") }; +} + const DefaultUserSettings: React.FC = ({ accessToken, possibleUIRoles, @@ -28,11 +121,14 @@ const DefaultUserSettings: React.FC = ({ userRole, }) => { const [loading, setLoading] = useState(true); - const [settings, setSettings] = useState(null); + const [settings, setSettings] = useState(null); const [isEditing, setIsEditing] = useState(false); - const [editedValues, setEditedValues] = useState({}); + const [editedValues, setEditedValues] = useState({}); const [saving, setSaving] = useState(false); const [availableModels, setAvailableModels] = useState([]); + const [showConfirmModal, setShowConfirmModal] = useState(false); + const [pendingChanges, setPendingChanges] = useState([]); + const [pendingProcessedValues, setPendingProcessedValues] = useState(null); const { Paragraph } = Typography; const { Option } = Select; @@ -71,60 +167,63 @@ const DefaultUserSettings: React.FC = ({ fetchSSOSettings(); }, [accessToken]); - const handleSaveSettings = async () => { - if (!accessToken) return; + /** Perform the actual API save. Returns true on success, false on failure. */ + const executeSave = async (processedValues: DefaultUserSettingsValues): Promise => { + if (!accessToken) return false; setSaving(true); try { - // Convert empty strings to null - const processedValues = Object.entries(editedValues).reduce( - (acc, [key, value]) => { - acc[key] = value === "" ? null : value; - return acc; - }, - {} as Record, - ); - const updatedSettings = await updateInternalUserSettings(accessToken, processedValues); - setSettings({ ...settings, values: updatedSettings.settings }); + setSettings((prev) => prev ? { ...prev, values: updatedSettings.settings } : null); setIsEditing(false); + return true; } catch (error) { console.error("Error updating SSO settings:", error); NotificationManager.fromBackend("Failed to update settings: " + error); + return false; } finally { setSaving(false); } }; - const handleTextInputChange = (key: string, value: any) => { - setEditedValues((prev: Record) => ({ - ...prev, - [key]: value, - })); + /** Called when user clicks "Save Changes". Shows modal if destructive. */ + const handleSaveSettings = async () => { + if (!accessToken) return; + + // Convert empty strings to null + const processedValues = Object.fromEntries( + Object.entries(editedValues).map(([key, value]) => [key, value === "" ? null : value]), + ) as DefaultUserSettingsValues; + + const { changes, hasDestructiveChanges } = computeSettingsDiff( + settings?.values || {}, + processedValues, + ); + + if (hasDestructiveChanges) { + setPendingChanges(changes); + setPendingProcessedValues(processedValues); + setShowConfirmModal(true); + return; + } + + // No destructive changes — save directly + await executeSave(processedValues); }; - // Helper function to normalize teams array to consistent format - const normalizeTeams = (teams: any[]): TeamEntry[] => { - if (!teams || !Array.isArray(teams)) return []; - - return teams.map((team) => { - if (typeof team === "string") { - return { - team_id: team, - user_role: "user" as const, - }; - } else if (typeof team === "object" && team.team_id) { - return { - team_id: team.team_id, - max_budget_in_team: team.max_budget_in_team, - user_role: team.user_role || "user", - }; - } - return { - team_id: "", - user_role: "user" as const, - }; - }); + /** Called when user confirms changes in the modal. Only clears modal on success. */ + const handleConfirmSave = async () => { + if (!pendingProcessedValues) return; + const success = await executeSave(pendingProcessedValues); + if (success) { + setShowConfirmModal(false); + setPendingChanges([]); + setPendingProcessedValues(null); + } + }; + + const handleTextInputChange = (key: string, value: any) => { + setEditedValues((prev) => ({ ...prev, [key]: value })); }; // Teams editor component @@ -291,6 +390,19 @@ const DefaultUserSettings: React.FC = ({ ))} ); + } else if (type === "number") { + return ( + handleTextInputChange(key, value)} + placeholder={property.description || ""} + className="mt-2" + min={0} + step={0.01} + precision={2} + /> + ); } else if (type === "string" && property.enum) { return (