From 5a3eca2a2acda26ff27fd34f86d8917165b62cc9 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Tue, 25 Mar 2025 18:34:55 -0400 Subject: [PATCH] [Refactor]: Create dedicated reset settings route (#7472) Co-authored-by: openhands --- frontend/__tests__/routes/settings.test.tsx | 84 +++++------ frontend/src/api/open-hands.ts | 14 +- .../components/features/sidebar/sidebar.tsx | 5 +- frontend/src/hooks/mutation/use-logout.ts | 16 ++- .../src/hooks/mutation/use-save-settings.ts | 35 +++-- frontend/src/hooks/query/use-github-user.ts | 10 +- frontend/src/hooks/use-app-logout.ts | 7 +- frontend/src/mocks/handlers.ts | 11 +- frontend/src/routes/account-settings.tsx | 18 +-- frontend/src/types/settings.ts | 2 - openhands/server/routes/settings.py | 134 +++++++++++++----- openhands/server/settings.py | 1 - tests/unit/test_settings_api.py | 47 ------ 13 files changed, 193 insertions(+), 191 deletions(-) diff --git a/frontend/__tests__/routes/settings.test.tsx b/frontend/__tests__/routes/settings.test.tsx index e05b8cf4d3..408cb92e4d 100644 --- a/frontend/__tests__/routes/settings.test.tsx +++ b/frontend/__tests__/routes/settings.test.tsx @@ -8,7 +8,6 @@ import { AuthProvider } from "#/context/auth-context"; import SettingsScreen from "#/routes/settings"; import * as AdvancedSettingsUtlls from "#/utils/has-advanced-settings-set"; import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers"; -import { PostApiSettings } from "#/types/settings"; import * as ConsentHandlers from "#/utils/handle-capture-consent"; import AccountSettings from "#/routes/account-settings"; @@ -20,6 +19,7 @@ const toggleAdvancedSettings = async (user: UserEvent) => { describe("Settings Screen", () => { const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + const resetSettingsSpy = vi.spyOn(OpenHands, "resetSettings"); const getConfigSpy = vi.spyOn(OpenHands, "getConfig"); const { handleLogoutMock } = vi.hoisted(() => ({ @@ -583,6 +583,11 @@ describe("Settings Screen", () => { test("resetting settings with no changes but having advanced enabled should hide the advanced items", async () => { const user = userEvent.setup(); + + getSettingsSpy.mockResolvedValueOnce({ + ...MOCK_DEFAULT_USER_SETTINGS, + }); + renderSettingsScreen(); await toggleAdvancedSettings(user); @@ -594,6 +599,15 @@ describe("Settings Screen", () => { const modal = await screen.findByTestId("reset-modal"); expect(modal).toBeInTheDocument(); + // Mock the settings that will be returned after reset + // This should be the default settings with no advanced settings enabled + getSettingsSpy.mockResolvedValueOnce({ + ...MOCK_DEFAULT_USER_SETTINGS, + llm_base_url: "", + confirmation_mode: false, + security_analyzer: "", + }); + // confirm reset const confirmButton = within(modal).getByText("Reset"); await user.click(confirmButton); @@ -817,19 +831,27 @@ describe("Settings Screen", () => { const confirmButton = within(modal).getByText("Reset"); await user.click(confirmButton); - const mockCopy: Partial = { - ...MOCK_DEFAULT_USER_SETTINGS, - }; - delete mockCopy.github_token_is_set; - delete mockCopy.unset_github_token; - delete mockCopy.user_consents_to_analytics; - - expect(saveSettingsSpy).toHaveBeenCalledWith({ - ...mockCopy, - provider_tokens: undefined, // not set - llm_api_key: "", // reset as well + await waitFor(() => { + expect(resetSettingsSpy).toHaveBeenCalled(); + }); + + // Mock the settings response after reset + getSettingsSpy.mockResolvedValueOnce({ + ...MOCK_DEFAULT_USER_SETTINGS, + llm_base_url: "", + confirmation_mode: false, + security_analyzer: "", + }); + + // Wait for the mutation to complete and the modal to be removed + await waitFor(() => { + expect(screen.queryByTestId("reset-modal")).not.toBeInTheDocument(); + expect(screen.queryByTestId("llm-custom-model-input")).not.toBeInTheDocument(); + expect(screen.queryByTestId("base-url-input")).not.toBeInTheDocument(); + expect(screen.queryByTestId("agent-input")).not.toBeInTheDocument(); + expect(screen.queryByTestId("security-analyzer-input")).not.toBeInTheDocument(); + expect(screen.queryByTestId("enable-confirmation-mode-switch")).not.toBeInTheDocument(); }); - expect(screen.queryByTestId("reset-modal")).not.toBeInTheDocument(); }); it("should cancel the reset when the 'Cancel' button is clicked", async () => { @@ -887,32 +909,6 @@ describe("Settings Screen", () => { expect(handleCaptureConsentSpy).toHaveBeenCalledWith(false); }); - it("should not reset analytics consent when resetting to defaults", async () => { - const user = userEvent.setup(); - getSettingsSpy.mockResolvedValue({ - ...MOCK_DEFAULT_USER_SETTINGS, - user_consents_to_analytics: true, - }); - - renderSettingsScreen(); - - const analyticsConsentInput = await screen.findByTestId( - "enable-analytics-switch", - ); - expect(analyticsConsentInput).toBeChecked(); - - const resetButton = await screen.findByText("Reset to defaults"); - await user.click(resetButton); - - const modal = await screen.findByTestId("reset-modal"); - const confirmButton = within(modal).getByText("Reset"); - await user.click(confirmButton); - - expect(saveSettingsSpy).toHaveBeenCalledWith( - expect.objectContaining({ user_consents_to_analytics: undefined }), - ); - }); - it("should render the security analyzer input if the confirmation mode is enabled", async () => { const user = userEvent.setup(); renderSettingsScreen(); @@ -1094,14 +1090,8 @@ describe("Settings Screen", () => { const modal = await screen.findByTestId("reset-modal"); const confirmButton = within(modal).getByText("Reset"); await user.click(confirmButton); - - expect(saveSettingsSpy).toHaveBeenCalledWith( - expect.objectContaining({ - llm_api_key: undefined, - llm_base_url: undefined, - llm_model: undefined, - }), - ); + expect(saveSettingsSpy).not.toHaveBeenCalled(); + expect(resetSettingsSpy).toHaveBeenCalled(); }); }); }); diff --git a/frontend/src/api/open-hands.ts b/frontend/src/api/open-hands.ts index 084c3a3a1c..c56736e8c9 100644 --- a/frontend/src/api/open-hands.ts +++ b/frontend/src/api/open-hands.ts @@ -273,6 +273,14 @@ class OpenHands { return data.status === 200; } + /** + * Reset user settings in server + */ + static async resetSettings(): Promise { + const response = await openHands.post("/api/reset-settings"); + return response.status === 200; + } + static async createCheckoutSession(amount: number): Promise { const { data } = await openHands.post( "/api/billing/create-checkout-session", @@ -345,8 +353,10 @@ class OpenHands { return data; } - static async logout(): Promise { - await openHands.post("/api/logout"); + static async logout(appMode: GetConfigResponse["APP_MODE"]): Promise { + const endpoint = + appMode === "saas" ? "/api/logout" : "/api/unset-settings-tokens"; + await openHands.post(endpoint); } } diff --git a/frontend/src/components/features/sidebar/sidebar.tsx b/frontend/src/components/features/sidebar/sidebar.tsx index 983e02f403..8d75eb0b09 100644 --- a/frontend/src/components/features/sidebar/sidebar.tsx +++ b/frontend/src/components/features/sidebar/sidebar.tsx @@ -21,7 +21,6 @@ import { useLogout } from "#/hooks/mutation/use-logout"; import { useConfig } from "#/hooks/query/use-config"; import { cn } from "#/utils/utils"; import { displayErrorToast } from "#/utils/custom-toast-handlers"; -import { useSaveSettings } from "#/hooks/mutation/use-save-settings"; export function Sidebar() { const location = useLocation(); @@ -36,7 +35,6 @@ export function Sidebar() { isFetching: isFetchingSettings, } = useSettings(); const { mutateAsync: logout } = useLogout(); - const { mutate: saveUserSettings } = useSaveSettings(); const [settingsModalIsOpen, setSettingsModalIsOpen] = React.useState(false); @@ -78,8 +76,7 @@ export function Sidebar() { }; const handleLogout = async () => { - if (config?.APP_MODE === "saas") await logout(); - else saveUserSettings({ unset_github_token: true }); + await logout(); posthog.reset(); }; diff --git a/frontend/src/hooks/mutation/use-logout.ts b/frontend/src/hooks/mutation/use-logout.ts index 4207c1dd19..d10bd307e2 100644 --- a/frontend/src/hooks/mutation/use-logout.ts +++ b/frontend/src/hooks/mutation/use-logout.ts @@ -1,16 +1,26 @@ import { useMutation, useQueryClient } from "@tanstack/react-query"; import OpenHands from "#/api/open-hands"; import { useAuth } from "#/context/auth-context"; +import { useConfig } from "../query/use-config"; export const useLogout = () => { const { setGitHubTokenIsSet } = useAuth(); const queryClient = useQueryClient(); + const { data: config } = useConfig(); return useMutation({ - mutationFn: OpenHands.logout, - onSuccess: async () => { + mutationFn: async () => { + // Pause all queries that depend on githubTokenIsSet + queryClient.setQueryData(["user"], null); + + // Call logout endpoint + await OpenHands.logout(config?.APP_MODE ?? "oss"); + + // Remove settings from cache so it will be refetched with new token state + queryClient.removeQueries({ queryKey: ["settings"] }); + + // Update token state - this will trigger a settings refetch since it's part of the query key setGitHubTokenIsSet(false); - await queryClient.invalidateQueries(); }, }); }; diff --git a/frontend/src/hooks/mutation/use-save-settings.ts b/frontend/src/hooks/mutation/use-save-settings.ts index a93adfb14b..53297b3530 100644 --- a/frontend/src/hooks/mutation/use-save-settings.ts +++ b/frontend/src/hooks/mutation/use-save-settings.ts @@ -4,8 +4,14 @@ import OpenHands from "#/api/open-hands"; import { PostSettings, PostApiSettings } from "#/types/settings"; import { useSettings } from "../query/use-settings"; -const saveSettingsMutationFn = async (settings: Partial) => { - const resetLlmApiKey = settings.LLM_API_KEY === ""; +const saveSettingsMutationFn = async ( + settings: Partial | null, +) => { + // If settings is null, we're resetting + if (settings === null) { + await OpenHands.resetSettings(); + return; + } const apiSettings: Partial = { llm_model: settings.LLM_MODEL, @@ -14,12 +20,12 @@ const saveSettingsMutationFn = async (settings: Partial) => { language: settings.LANGUAGE || DEFAULT_SETTINGS.LANGUAGE, confirmation_mode: settings.CONFIRMATION_MODE, security_analyzer: settings.SECURITY_ANALYZER, - llm_api_key: resetLlmApiKey - ? "" - : settings.LLM_API_KEY?.trim() || undefined, + llm_api_key: + settings.LLM_API_KEY === "" + ? "" + : settings.LLM_API_KEY?.trim() || undefined, remote_runtime_resource_factor: settings.REMOTE_RUNTIME_RESOURCE_FACTOR, provider_tokens: settings.provider_tokens, - unset_github_token: settings.unset_github_token, enable_default_condenser: settings.ENABLE_DEFAULT_CONDENSER, enable_sound_notifications: settings.ENABLE_SOUND_NOTIFICATIONS, user_consents_to_analytics: settings.user_consents_to_analytics, @@ -33,20 +39,13 @@ export const useSaveSettings = () => { const { data: currentSettings } = useSettings(); return useMutation({ - mutationFn: async (settings: Partial) => { - const newSettings = { ...currentSettings, ...settings }; - - // Temp hack for reset logic - if ( - settings.LLM_API_KEY === undefined && - settings.LLM_BASE_URL === undefined && - settings.LLM_MODEL === undefined - ) { - delete newSettings.LLM_API_KEY; - delete newSettings.LLM_BASE_URL; - delete newSettings.LLM_MODEL; + mutationFn: async (settings: Partial | null) => { + if (settings === null) { + await saveSettingsMutationFn(null); + return; } + const newSettings = { ...currentSettings, ...settings }; await saveSettingsMutationFn(newSettings); }, onSuccess: async () => { diff --git a/frontend/src/hooks/query/use-github-user.ts b/frontend/src/hooks/query/use-github-user.ts index 330c1a55ab..7781e07187 100644 --- a/frontend/src/hooks/query/use-github-user.ts +++ b/frontend/src/hooks/query/use-github-user.ts @@ -5,13 +5,11 @@ import { useConfig } from "./use-config"; import OpenHands from "#/api/open-hands"; import { useAuth } from "#/context/auth-context"; import { useLogout } from "../mutation/use-logout"; -import { useSaveSettings } from "../mutation/use-save-settings"; export const useGitHubUser = () => { const { githubTokenIsSet } = useAuth(); - const { setGitHubTokenIsSet } = useAuth(); const { mutateAsync: logout } = useLogout(); - const { mutate: saveUserSettings } = useSaveSettings(); + const { data: config } = useConfig(); const user = useQuery({ @@ -36,11 +34,7 @@ export const useGitHubUser = () => { }, [user.data]); const handleLogout = async () => { - if (config?.APP_MODE === "saas") await logout(); - else { - saveUserSettings({ unset_github_token: true }); - setGitHubTokenIsSet(false); - } + await logout(); posthog.reset(); }; diff --git a/frontend/src/hooks/use-app-logout.ts b/frontend/src/hooks/use-app-logout.ts index c9ba01c360..87c299a5a5 100644 --- a/frontend/src/hooks/use-app-logout.ts +++ b/frontend/src/hooks/use-app-logout.ts @@ -1,15 +1,10 @@ import { useLogout } from "./mutation/use-logout"; -import { useSaveSettings } from "./mutation/use-save-settings"; -import { useConfig } from "./query/use-config"; export const useAppLogout = () => { - const { data: config } = useConfig(); const { mutateAsync: logout } = useLogout(); - const { mutate: saveUserSettings } = useSaveSettings(); const handleLogout = async () => { - if (config?.APP_MODE === "saas") await logout(); - else saveUserSettings({ unset_github_token: true }); + await logout(); }; return { handleLogout }; diff --git a/frontend/src/mocks/handlers.ts b/frontend/src/mocks/handlers.ts index e1d170f1fe..2571b9ff70 100644 --- a/frontend/src/mocks/handlers.ts +++ b/frontend/src/mocks/handlers.ts @@ -206,11 +206,6 @@ export const handlers = [ let newSettings: Partial = {}; if (typeof body === "object") { newSettings = { ...body }; - if (newSettings.unset_github_token) { - newSettings.provider_tokens = { github: "", gitlab: "" }; - newSettings.github_token_is_set = false; - delete newSettings.unset_github_token; - } } const fullSettings = { @@ -306,4 +301,10 @@ export const handlers = [ }), http.post("/api/logout", () => HttpResponse.json(null, { status: 200 })), + + http.post("/api/reset-settings", async () => { + await delay(); + MOCK_USER_PREFERENCES.settings = { ...MOCK_DEFAULT_USER_SETTINGS }; + return HttpResponse.json(null, { status: 200 }); + }), ]; diff --git a/frontend/src/routes/account-settings.tsx b/frontend/src/routes/account-settings.tsx index 24bca6c35d..a97f4cae37 100644 --- a/frontend/src/routes/account-settings.tsx +++ b/frontend/src/routes/account-settings.tsx @@ -25,7 +25,6 @@ import { displayErrorToast, displaySuccessToast, } from "#/utils/custom-toast-handlers"; -import { PostSettings } from "#/types/settings"; const REMOTE_RUNTIME_OPTIONS = [ { key: 1, label: "1x (2 core, 8G)" }, @@ -170,24 +169,11 @@ function AccountSettings() { }; const handleReset = () => { - const newSettings: Partial = { - ...DEFAULT_SETTINGS, - LLM_API_KEY: "", // reset LLM API key - }; - - // we don't want the user to be able to modify these settings in SaaS - // and we should make sure they aren't included in the reset - if (shouldHandleSpecialSaasCase) { - delete newSettings.LLM_API_KEY; - delete newSettings.LLM_BASE_URL; - delete newSettings.LLM_MODEL; - } - - saveSettings(newSettings, { + saveSettings(null, { onSuccess: () => { displaySuccessToast("Settings reset"); setResetSettingsModalIsOpen(false); - setLlmConfigMode(isAdvancedSettingsSet ? "advanced" : "basic"); + setLlmConfigMode("basic"); }, }); }; diff --git a/frontend/src/types/settings.ts b/frontend/src/types/settings.ts index 6e13a2ec50..9fcec7d7ef 100644 --- a/frontend/src/types/settings.ts +++ b/frontend/src/types/settings.ts @@ -35,12 +35,10 @@ export type ApiSettings = { export type PostSettings = Settings & { provider_tokens: Record; - unset_github_token: boolean; user_consents_to_analytics: boolean | null; }; export type PostApiSettings = ApiSettings & { provider_tokens: Record; - unset_github_token: boolean; user_consents_to_analytics: boolean | null; }; diff --git a/openhands/server/routes/settings.py b/openhands/server/routes/settings.py index 8b682d793c..ceee072c87 100644 --- a/openhands/server/routes/settings.py +++ b/openhands/server/routes/settings.py @@ -7,7 +7,8 @@ from openhands.integrations.provider import ProviderToken, ProviderType, SecretS from openhands.integrations.utils import validate_provider_token from openhands.server.auth import get_provider_tokens, get_user_id from openhands.server.settings import GETSettingsModel, POSTSettingsModel, Settings -from openhands.server.shared import SettingsStoreImpl, config +from openhands.server.shared import SettingsStoreImpl, config, server_config +from openhands.server.types import AppMode app = APIRouter(prefix='/api') @@ -40,6 +41,79 @@ async def load_settings(request: Request) -> GETSettingsModel | JSONResponse: ) +@app.post('/unset-settings-tokens', response_model=dict[str, str]) +async def unset_settings_tokens( + request: Request +) -> JSONResponse: + try: + settings_store = await SettingsStoreImpl.get_instance( + config, get_user_id(request) + ) + + existing_settings = await settings_store.load() + if existing_settings: + settings = existing_settings.model_copy(update={'secrets_store': SecretStore()}) + await settings_store.store(settings) + + return JSONResponse( + status_code=status.HTTP_200_OK, + content={'message': 'Settings stored'}, + ) + + except Exception as e: + logger.warning(f'Something went wrong unsetting tokens: {e}') + return JSONResponse( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + content={'error': 'Something went wrong unsetting tokens'}, + ) +@app.post('/reset-settings', response_model=dict[str, str]) +async def reset_settings( + request: Request +) -> JSONResponse: + """ + Resets user settings. + """ + try: + settings_store = await SettingsStoreImpl.get_instance( + config, get_user_id(request) + ) + + existing_settings = await settings_store.load() + settings = Settings(language="en", + agent="CodeActAgent", + security_analyzer="", + confirmation_mode=False, + llm_model="anthropic/claude-3-5-sonnet-20241022", + llm_api_key="", + llm_base_url="", + remote_runtime_resource_factor=1, + enable_default_condenser=True, + enable_sound_notifications=False, + user_consents_to_analytics=existing_settings.user_consents_to_analytics if existing_settings else False + ) + + server_config_values = server_config.get_config() + is_hide_llm_settings_enabled = server_config_values.get("FEATURE_FLAGS", {}).get("HIDE_LLM_SETTINGS", False) + # We don't want the user to be able to modify these settings in SaaS + if (server_config.app_mode == AppMode.SAAS and is_hide_llm_settings_enabled): + if existing_settings: + settings.llm_api_key = existing_settings.llm_api_key + settings.llm_base_url = existing_settings.llm_base_url + settings.llm_model = existing_settings.llm_model + + await settings_store.store(settings) + return JSONResponse( + status_code=status.HTTP_200_OK, + content={'message': 'Settings stored'}, + ) + + except Exception as e: + logger.warning(f'Something went wrong resetting settings: {e}') + return JSONResponse( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + content={'error': 'Something went wrong resetting settings'}, + ) + @app.post('/settings', response_model=dict[str, str]) async def store_settings( request: Request, @@ -89,39 +163,35 @@ async def store_settings( existing_settings.user_consents_to_analytics ) - # Handle token updates immutably - if settings.unset_github_token: - settings = settings.model_copy(update={'secrets_store': SecretStore()}) + # Only merge if not unsetting tokens + if settings.provider_tokens: + if existing_settings.secrets_store: + existing_providers = [ + provider.value + for provider in existing_settings.secrets_store.provider_tokens + ] - else: # Only merge if not unsetting tokens - if settings.provider_tokens: - if existing_settings.secrets_store: - existing_providers = [ - provider.value - for provider in existing_settings.secrets_store.provider_tokens - ] - - # Merge incoming settings store with the existing one - for provider, token_value in settings.provider_tokens.items(): - if provider in existing_providers and not token_value: - provider_type = ProviderType(provider) - existing_token = ( - existing_settings.secrets_store.provider_tokens.get( - provider_type - ) + # Merge incoming settings store with the existing one + for provider, token_value in settings.provider_tokens.items(): + if provider in existing_providers and not token_value: + provider_type = ProviderType(provider) + existing_token = ( + existing_settings.secrets_store.provider_tokens.get( + provider_type ) - if existing_token and existing_token.token: - settings.provider_tokens[provider] = ( - existing_token.token.get_secret_value() - ) - else: # nothing passed in means keep current settings - provider_tokens = existing_settings.secrets_store.provider_tokens - settings.provider_tokens = { - provider.value: data.token.get_secret_value() - if data.token - else None - for provider, data in provider_tokens.items() - } + ) + if existing_token and existing_token.token: + settings.provider_tokens[provider] = ( + existing_token.token.get_secret_value() + ) + else: # nothing passed in means keep current settings + provider_tokens = existing_settings.secrets_store.provider_tokens + settings.provider_tokens = { + provider.value: data.token.get_secret_value() + if data.token + else None + for provider, data in provider_tokens.items() + } # Update sandbox config with new settings if settings.remote_runtime_resource_factor is not None: diff --git a/openhands/server/settings.py b/openhands/server/settings.py index bc6929ca43..ac267ada25 100644 --- a/openhands/server/settings.py +++ b/openhands/server/settings.py @@ -107,7 +107,6 @@ class POSTSettingsModel(Settings): Settings for POST requests """ - unset_github_token: bool | None = None # Override provider_tokens to accept string tokens from frontend provider_tokens: dict[str, str] = {} diff --git a/tests/unit/test_settings_api.py b/tests/unit/test_settings_api.py index cda91dcf48..2d45d5c4bc 100644 --- a/tests/unit/test_settings_api.py +++ b/tests/unit/test_settings_api.py @@ -209,53 +209,6 @@ async def test_settings_api_set_github_token( assert data['token_is_set'] is True -@pytest.mark.skip( - reason='Mock middleware does not seem to properly set the github_token' -) -async def test_settings_unset_github_token( - mock_github_service, - test_client, - mock_settings_store, - mock_get_user_id, - mock_validate_provider_token, -): - # Test data with unset_token set to True - settings_data = { - 'language': 'en', - 'agent': 'test-agent', - 'max_iterations': 100, - 'security_analyzer': 'default', - 'confirmation_mode': True, - 'llm_model': 'test-model', - 'llm_api_key': 'test-key', - 'llm_base_url': 'https://test.com', - 'provider_tokens': {'github': 'test-token'}, - } - - # Mock settings store to return our settings for the GET request - mock_settings_store.load.return_value = Settings(**settings_data) - - response = test_client.get('/api/settings') - assert response.status_code == 200 - assert response.json()['token_is_set'] is True - - settings_data['unset_token'] = True - - # Make the POST request to store settings - response = test_client.post('/api/settings', json=settings_data) - assert response.status_code == 200 - - # Verify the settings were stored with the provider token unset - stored_settings = mock_settings_store.store.call_args[0][0] - assert not stored_settings.secrets_store.provider_tokens - mock_settings_store.load.return_value = Settings(**stored_settings.dict()) - - # Make a GET request to retrieve settings - response = test_client.get('/api/settings') - assert response.status_code == 200 - assert response.json()['token_is_set'] is False - - @pytest.mark.asyncio async def test_settings_preserve_llm_fields_when_none(test_client, mock_settings_store): # Setup initial settings with LLM fields populated