From 57795ee4871bb0bb0727037a889542bf46a8bb9e Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 23 Mar 2026 14:39:53 +0000 Subject: [PATCH] chore: simplify and fix naming for save settings endpoints - Renamed `save_dropbox_settings` inside `app/api/google_drive.py` to `save_google_drive_settings` to fix a copy-paste naming error. - Extracted duplicate `.env` file updating logic from `app/api/google_drive.py`, `app/api/onedrive.py`, and `app/api/dropbox.py` into a new reusable helper function `update_env_file` inside `app/utils/settings_service.py`. - Refactored the three API endpoints to use the new helper function, significantly reducing complexity and code duplication. - Updated relevant test files (`tests/test_api_google_drive_final.py`) to reflect the new function name. Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- app/api/dropbox.py | 48 +++++------------------ app/api/google_drive.py | 46 ++-------------------- app/api/onedrive.py | 52 ++++++------------------- app/utils/settings_service.py | 58 ++++++++++++++++++++++++++++ tests/test_api_google_drive_final.py | 16 +++----- 5 files changed, 90 insertions(+), 130 deletions(-) diff --git a/app/api/dropbox.py b/app/api/dropbox.py index f9bf10e5..ced23cf2 100644 --- a/app/api/dropbox.py +++ b/app/api/dropbox.py @@ -14,7 +14,7 @@ from app.auth import require_login from app.config import settings from app.database import get_db from app.utils.oauth_helper import exchange_oauth_token -from app.utils.settings_service import save_setting_to_db +from app.utils.settings_service import save_setting_to_db, update_env_file from app.utils.settings_sync import notify_settings_updated # Set up logging @@ -249,44 +249,16 @@ async def save_dropbox_settings( # Best-effort .env file write try: env_path = os.path.join(os.path.dirname(os.path.dirname(os.path.dirname(__file__))), ".env") - if not os.path.exists(env_path): - logger.warning(f".env file not found at {env_path}, skipping file write") - else: - logger.info(f"Updating Dropbox settings in {env_path}") + dropbox_settings = {"DROPBOX_REFRESH_TOKEN": refresh_token} + if app_key: + dropbox_settings["DROPBOX_APP_KEY"] = app_key + if app_secret: + dropbox_settings["DROPBOX_APP_SECRET"] = app_secret + if folder_path: + dropbox_settings["DROPBOX_FOLDER"] = folder_path - with open(env_path, "r") as f: - env_lines = f.readlines() - - dropbox_settings = {"DROPBOX_REFRESH_TOKEN": refresh_token} - if app_key: - dropbox_settings["DROPBOX_APP_KEY"] = app_key - if app_secret: - dropbox_settings["DROPBOX_APP_SECRET"] = app_secret - if folder_path: - dropbox_settings["DROPBOX_FOLDER"] = folder_path - - updated = set() - new_env_lines = [] - for line in env_lines: - stripped_line = line.rstrip() - is_updated = False - for key, value in dropbox_settings.items(): - if stripped_line.startswith(f"{key}=") or stripped_line.startswith(f"# {key}="): - new_env_lines.append(f"{key}={value}") - updated.add(key) - is_updated = True - break - if not is_updated: - new_env_lines.append(stripped_line) - - for key, value in dropbox_settings.items(): - if key not in updated: - new_env_lines.append(f"{key}={value}") - - with open(env_path, "w") as f: - f.write("\n".join(new_env_lines) + "\n") - - logger.info("Successfully updated Dropbox settings in .env file") + if not update_env_file(env_path, dropbox_settings): + logger.info("Continuing with in-memory update despite .env file update failure or skip") except Exception as env_err: logger.warning(f"Failed to write .env file (non-fatal): {env_err}") diff --git a/app/api/google_drive.py b/app/api/google_drive.py index f9eda756..58622894 100644 --- a/app/api/google_drive.py +++ b/app/api/google_drive.py @@ -14,7 +14,7 @@ from app.auth import require_login from app.config import settings from app.database import get_db from app.utils.oauth_helper import exchange_oauth_token -from app.utils.settings_service import save_setting_to_db +from app.utils.settings_service import save_setting_to_db, update_env_file from app.utils.settings_sync import notify_settings_updated # Set up logging @@ -363,7 +363,7 @@ def format_time_remaining(time_delta): @router.post("/google-drive/save-settings") @require_login -async def save_dropbox_settings( +async def save_google_drive_settings( request: Request, refresh_token: Annotated[str, Form(...)], client_id: Annotated[Optional[str], Form()] = None, @@ -404,46 +404,8 @@ async def save_dropbox_settings( drive_settings["GOOGLE_DRIVE_FOLDER_ID"] = folder_id # Try to update the .env file, but don't fail if it doesn't exist (for Docker containers) - if os.path.exists(env_path): - try: - logger.info(f"Updating Google Drive settings in {env_path}") - - # Read the current .env file - with open(env_path, "r") as f: - env_lines = f.readlines() - - # Process each line and update or add settings - updated = set() - new_env_lines = [] - for line in env_lines: - stripped_line = line.rstrip() - is_updated = False - for key, value in drive_settings.items(): - if stripped_line.startswith(f"{key}=") or stripped_line.startswith(f"# {key}="): - # Uncomment if commented out - check the original stripped line - new_env_lines.append(f"{key}={value}") - updated.add(key) - is_updated = True - break - if not is_updated: - new_env_lines.append(stripped_line) - - # Add any settings that weren't updated (they weren't in the file) - for key, value in drive_settings.items(): - if key not in updated: - new_env_lines.append(f"{key}={value}") - - # Write the updated .env file - with open(env_path, "w") as f: - f.write("\n".join(new_env_lines) + "\n") - - logger.info("Successfully updated Google Drive settings in .env file") - except Exception as e: - logger.warning(f"Failed to update .env file: {str(e)}, but will continue with in-memory update") - else: - logger.warning( - f".env file not found at {env_path}, skipping file update but continuing with in-memory update" - ) + if not update_env_file(env_path, drive_settings): + logger.info("Continuing with in-memory update despite .env file update failure or skip") # Update the settings in memory (this always happens) if refresh_token: diff --git a/app/api/onedrive.py b/app/api/onedrive.py index e9f8328d..a211a740 100644 --- a/app/api/onedrive.py +++ b/app/api/onedrive.py @@ -15,7 +15,7 @@ from app.auth import require_login from app.config import settings from app.database import get_db from app.utils.oauth_helper import exchange_oauth_token -from app.utils.settings_service import save_setting_to_db +from app.utils.settings_service import save_setting_to_db, update_env_file from app.utils.settings_sync import notify_settings_updated # Set up logging @@ -249,46 +249,18 @@ async def save_onedrive_settings( # Best-effort .env file write try: env_path = os.path.join(os.path.dirname(os.path.dirname(os.path.dirname(__file__))), ".env") - if not os.path.exists(env_path): - logger.warning(f".env file not found at {env_path}, skipping file write") - else: - logger.info(f"Updating OneDrive settings in {env_path}") + onedrive_settings = {"ONEDRIVE_REFRESH_TOKEN": refresh_token} + if client_id: + onedrive_settings["ONEDRIVE_CLIENT_ID"] = client_id + if client_secret: + onedrive_settings["ONEDRIVE_CLIENT_SECRET"] = client_secret + if tenant_id: + onedrive_settings["ONEDRIVE_TENANT_ID"] = tenant_id + if folder_path: + onedrive_settings["ONEDRIVE_FOLDER_PATH"] = folder_path - with open(env_path, "r") as f: - env_lines = f.readlines() - - onedrive_settings = {"ONEDRIVE_REFRESH_TOKEN": refresh_token} - if client_id: - onedrive_settings["ONEDRIVE_CLIENT_ID"] = client_id - if client_secret: - onedrive_settings["ONEDRIVE_CLIENT_SECRET"] = client_secret - if tenant_id: - onedrive_settings["ONEDRIVE_TENANT_ID"] = tenant_id - if folder_path: - onedrive_settings["ONEDRIVE_FOLDER_PATH"] = folder_path - - updated = set() - new_env_lines = [] - for line in env_lines: - stripped_line = line.rstrip() - is_updated = False - for key, value in onedrive_settings.items(): - if stripped_line.startswith(f"{key}=") or stripped_line.startswith(f"# {key}="): - new_env_lines.append(f"{key}={value}") - updated.add(key) - is_updated = True - break - if not is_updated: - new_env_lines.append(stripped_line) - - for key, value in onedrive_settings.items(): - if key not in updated: - new_env_lines.append(f"{key}={value}") - - with open(env_path, "w") as f: - f.write("\n".join(new_env_lines) + "\n") - - logger.info("Successfully updated OneDrive settings in .env file") + if not update_env_file(env_path, onedrive_settings): + logger.info("Continuing with in-memory update despite .env file update failure or skip") except Exception as env_err: logger.warning(f"Failed to write .env file (non-fatal): {env_err}") diff --git a/app/utils/settings_service.py b/app/utils/settings_service.py index dceaa943..4d50a66a 100644 --- a/app/utils/settings_service.py +++ b/app/utils/settings_service.py @@ -8,6 +8,7 @@ This module provides functionality to: """ import logging +import os from typing import Any, Dict, List, Optional, Tuple from sqlalchemy.exc import SQLAlchemyError @@ -3120,3 +3121,60 @@ def get_settings_for_export(db: Session, source: str = "db") -> Dict[str, str]: # DB only db_settings = get_all_settings_from_db(db) return {k.upper(): v for k, v in sorted(db_settings.items()) if v is not None} + + +def update_env_file(env_path: str, settings_to_update: Dict[str, str]) -> bool: + """ + Update an .env file with new settings. + + Reads the file, updates matching settings (even if commented), + appends any that weren't found, and writes the result back. + + Args: + env_path: Path to the .env file + settings_to_update: Dictionary mapping setting names (e.g. 'GOOGLE_DRIVE_USE_OAUTH') to string values + + Returns: + True if the file was successfully updated, False otherwise (e.g. file not found or write error) + """ + if not os.path.exists(env_path): + logger.warning(f".env file not found at {env_path}, skipping file update") + return False + + try: + logger.info(f"Updating settings in {env_path}") + + # Read the current .env file + with open(env_path, "r") as f: + env_lines = f.readlines() + + # Process each line and update or add settings + updated = set() + new_env_lines = [] + for line in env_lines: + stripped_line = line.rstrip() + is_updated = False + for key, value in settings_to_update.items(): + if stripped_line.startswith(f"{key}=") or stripped_line.startswith(f"# {key}="): + # Uncomment if commented out - check the original stripped line + new_env_lines.append(f"{key}={value}") + updated.add(key) + is_updated = True + break + if not is_updated: + new_env_lines.append(stripped_line) + + # Add any settings that weren't updated (they weren't in the file) + for key, value in settings_to_update.items(): + if key not in updated: + new_env_lines.append(f"{key}={value}") + + # Write the updated .env file + with open(env_path, "w") as f: + f.write("\n".join(new_env_lines) + "\n") + + logger.info(f"Successfully updated settings in {env_path}") + return True + except Exception as e: + logger.warning(f"Failed to update {env_path}: {str(e)}") + return False diff --git a/tests/test_api_google_drive_final.py b/tests/test_api_google_drive_final.py index 38ede9e1..0ad93318 100644 --- a/tests/test_api_google_drive_final.py +++ b/tests/test_api_google_drive_final.py @@ -7,9 +7,9 @@ Targets the remaining uncovered branches from the 97.03% baseline: - 214 : test_google_drive_token — generic connection error (not token-related) - 302->306: get_google_drive_token_info — credentials already valid (no refresh) - 307->318: get_google_drive_token_info — credentials have no expiry - - 395->397: save_dropbox_settings — refresh_token falsy inside use_oauth block - - 449->451: save_dropbox_settings — refresh_token falsy in in-memory update - - 468->470: save_dropbox_settings — folder_id falsy in db-persist block + - 395->397: save_google_drive_settings — refresh_token falsy inside use_oauth block + - 449->451: save_google_drive_settings — refresh_token falsy in in-memory update + - 468->470: save_google_drive_settings — folder_id falsy in db-persist block """ from datetime import datetime, timedelta @@ -152,11 +152,7 @@ class TestGetTokenInfoCredentialsBranches: @pytest.mark.unit class TestSaveGoogleDriveSettingsFalsyFields: - """Cover branches 395->397, 449->451, 468->470 in save_dropbox_settings. - - Note: the Google Drive save endpoint is named save_dropbox_settings in the - source (app/api/google_drive.py) due to an existing naming inconsistency. - """ + """Cover branches 395->397, 449->451, 468->470 in save_google_drive_settings.""" @patch("app.api.google_drive.settings") @patch("os.path.exists", return_value=False) @@ -167,7 +163,7 @@ class TestSaveGoogleDriveSettingsFalsyFields: from starlette.requests import Request as StarletteRequest - from app.api.google_drive import save_dropbox_settings + from app.api.google_drive import save_google_drive_settings mock_request = MagicMock(spec=StarletteRequest) mock_request.session = {} @@ -175,7 +171,7 @@ class TestSaveGoogleDriveSettingsFalsyFields: with patch("app.api.google_drive.save_setting_to_db"): with patch("app.api.google_drive.notify_settings_updated"): - result = await save_dropbox_settings( + result = await save_google_drive_settings( request=mock_request, refresh_token="", # falsy → branches 395->397 and 449->451 client_id="cid",