diff --git a/ui/litellm-dashboard/src/components/DefaultUserSettings.test.tsx b/ui/litellm-dashboard/src/components/DefaultUserSettings.test.tsx index 78d50fa7f31..4a49f91058a 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,295 @@ 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(); + }); +}); + +// --------------------------------------------------------------------------- +// 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 empty string same as null for scalars", () => { + const original = { max_budget: "" }; + const edited = { max_budget: null }; + + 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..dd1ae1ef3db 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,162 @@ interface TeamEntry { user_role: "user" | "admin"; } +/** Normalize heterogeneous team arrays (strings or objects) to TeamEntry[]. */ +function 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 }; + }); +} + +/** + * 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: Record, + edited: Record, +): { changes: SettingsChange[]; hasDestructiveChanges: boolean } { + const changes: SettingsChange[] = []; + + const allKeys = new Set([...Object.keys(original), ...Object.keys(edited)]); + + for (const key of allKeys) { + const oldVal = original[key]; + const newVal = edited[key]; + + const displayName = key.replace(/_/g, " ").replace(/\b\w/g, (l) => l.toUpperCase()); + + if (key === "teams") { + const oldTeams = normalizeTeams(oldVal || []); + const newTeams = normalizeTeams(newVal || []); + + const oldIds = new Set(oldTeams.map((t) => t.team_id)); + const newIds = new Set(newTeams.map((t) => t.team_id)); + + // Removed teams + for (const t of oldTeams) { + if (!newIds.has(t.team_id)) { + changes.push({ + field: displayName, + type: "removed", + details: `Team "${t.team_id}" removed`, + }); + } + } + + // Added teams + for (const t of newTeams) { + if (t.team_id && !oldIds.has(t.team_id)) { + changes.push({ + field: displayName, + type: "added", + details: `Team "${t.team_id}" added`, + }); + } + } + + // Budget/role changes for teams that still exist + for (const newTeam of newTeams) { + if (!oldIds.has(newTeam.team_id)) continue; + const oldTeam = oldTeams.find((t) => t.team_id === newTeam.team_id); + if (!oldTeam) continue; + + if (oldTeam.max_budget_in_team !== newTeam.max_budget_in_team) { + const oldBudget = oldTeam.max_budget_in_team !== undefined ? `$${oldTeam.max_budget_in_team}` : "No limit"; + const newBudget = newTeam.max_budget_in_team !== undefined ? `$${newTeam.max_budget_in_team}` : "No limit"; + changes.push({ + field: displayName, + type: "changed", + details: `Team "${newTeam.team_id}" budget: ${oldBudget} → ${newBudget}`, + }); + } + + if (oldTeam.user_role !== newTeam.user_role) { + changes.push({ + field: displayName, + type: "changed", + details: `Team "${newTeam.team_id}" role: ${oldTeam.user_role} → ${newTeam.user_role}`, + }); + } + } + + continue; + } + + if (key === "models") { + const oldModels: string[] = Array.isArray(oldVal) ? oldVal : []; + const newModels: string[] = Array.isArray(newVal) ? newVal : []; + const oldSet = new Set(oldModels); + const newSet = new Set(newModels); + + const removed = oldModels.filter((m) => !newSet.has(m)); + const added = newModels.filter((m) => !oldSet.has(m)); + + if (removed.length > 0) { + changes.push({ + field: displayName, + type: "removed", + details: `${removed.join(", ")} removed`, + }); + } + if (added.length > 0) { + changes.push({ + field: displayName, + type: "added", + details: `${added.join(", ")} added`, + }); + } + continue; + } + + // Scalar fields + const oldNorm = oldVal === "" ? null : oldVal; + const newNorm = newVal === "" ? null : newVal; + + if (oldNorm === newNorm) continue; + // Both null/undefined — no change + if (oldNorm == null && newNorm == null) continue; + + if (oldNorm != null && newNorm == null) { + // Value cleared + changes.push({ + field: displayName, + type: "removed", + details: `Cleared (was "${oldNorm}")`, + }); + } else if (oldNorm == null && newNorm != null) { + // Value set + changes.push({ + field: displayName, + type: "added", + details: `Set to "${newNorm}"`, + }); + } else if (oldNorm !== newNorm) { + changes.push({ + field: displayName, + type: "changed", + details: `"${oldNorm}" → "${newNorm}"`, + }); + } + } + + const hasDestructiveChanges = changes.some((c) => c.type === "removed" || c.type === "changed"); + + return { changes, hasDestructiveChanges }; +} + const DefaultUserSettings: React.FC = ({ accessToken, possibleUIRoles, @@ -33,6 +190,9 @@ const DefaultUserSettings: React.FC = ({ 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>(null); const { Paragraph } = Typography; const { Option } = Select; @@ -71,20 +231,12 @@ const DefaultUserSettings: React.FC = ({ fetchSSOSettings(); }, [accessToken]); - const handleSaveSettings = async () => { + /** Perform the actual API save with the given processed values. */ + const executeSave = async (processedValues: Record) => { if (!accessToken) return; 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 }); setIsEditing(false); @@ -96,6 +248,44 @@ const DefaultUserSettings: React.FC = ({ } }; + /** Called when user clicks "Save Changes". Shows modal if destructive. */ + const handleSaveSettings = async () => { + if (!accessToken) return; + + // Convert empty strings to null + const processedValues = Object.entries(editedValues).reduce( + (acc, [key, value]) => { + acc[key] = value === "" ? null : value; + return acc; + }, + {} as Record, + ); + + const { changes, hasDestructiveChanges } = computeSettingsDiff( + settings?.values || {}, + processedValues, + ); + + if (hasDestructiveChanges) { + setPendingChanges(changes); + setPendingProcessedValues(processedValues); + setShowConfirmModal(true); + return; + } + + // No destructive changes — save directly + await executeSave(processedValues); + }; + + /** Called when user confirms changes in the modal. */ + const handleConfirmSave = async () => { + if (!pendingProcessedValues) return; + await executeSave(pendingProcessedValues); + setShowConfirmModal(false); + setPendingChanges([]); + setPendingProcessedValues(null); + }; + const handleTextInputChange = (key: string, value: any) => { setEditedValues((prev: Record) => ({ ...prev, @@ -103,30 +293,6 @@ const DefaultUserSettings: React.FC = ({ })); }; - // 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, - }; - }); - }; - // Teams editor component const renderTeamsEditor = (teams: any[]) => { const normalizedTeams = normalizeTeams(teams); @@ -484,6 +650,18 @@ const DefaultUserSettings: React.FC = ({ {renderSettings()} + + { + setShowConfirmModal(false); + setPendingChanges([]); + setPendingProcessedValues(null); + }} + confirmLoading={saving} + /> ); }; diff --git a/ui/litellm-dashboard/src/components/common_components/ConfirmSettingsChangeModal.tsx b/ui/litellm-dashboard/src/components/common_components/ConfirmSettingsChangeModal.tsx new file mode 100644 index 00000000000..df26c611b0e --- /dev/null +++ b/ui/litellm-dashboard/src/components/common_components/ConfirmSettingsChangeModal.tsx @@ -0,0 +1,97 @@ +import React from "react"; +import { Alert, Modal, Typography } from "antd"; +import { + MinusCircleOutlined, + PlusCircleOutlined, + EditOutlined, +} from "@ant-design/icons"; + +export interface SettingsChange { + field: string; + type: "removed" | "added" | "changed"; + details: string; +} + +interface ConfirmSettingsChangeModalProps { + isOpen: boolean; + changes: SettingsChange[]; + onConfirm: () => void; + onCancel: () => void; + confirmLoading: boolean; +} + +const CHANGE_CONFIG: Record< + SettingsChange["type"], + { color: string; icon: React.ReactNode; label: string } +> = { + removed: { + color: "#cf1322", + icon: , + label: "Removed", + }, + added: { + color: "#389e0d", + icon: , + label: "Added", + }, + changed: { + color: "#d48806", + icon: , + label: "Changed", + }, +}; + +export default function ConfirmSettingsChangeModal({ + isOpen, + changes, + onConfirm, + onCancel, + confirmLoading, +}: ConfirmSettingsChangeModalProps) { + const { Text } = Typography; + + return ( + + + + + + {changes.map((change, index) => { + const config = CHANGE_CONFIG[change.type]; + return ( + + {config.icon} + + + {change.field} + + + — {change.details} + + + + ); + })} + + + + ); +}