From 6cc96c26148fab680bdff90273ba3f75d848805d Mon Sep 17 00:00:00 2001 From: Vaibhavee Singh Date: Tue, 24 Feb 2026 10:17:39 +0530 Subject: [PATCH] Fix #134: Add validation for duplicate profile names - Add validation in create_profile() to check for existing names before insert - Add validation in update_profile() to prevent renaming to duplicate names - Improve error handling in API endpoints with user-friendly messages - Add comprehensive test suite for duplicate name validation - Update CHANGELOG.md with fix details This fix prevents database constraint violations and provides clear error messages when users attempt to create or update profiles with names that already exist in the database. --- CHANGELOG.md | 8 + backend/main.py | 14 +- backend/profiles.py | 37 ++- backend/tests/test_profile_duplicate_names.py | 217 ++++++++++++++++++ 4 files changed, 262 insertions(+), 14 deletions(-) create mode 100644 backend/tests/test_profile_duplicate_names.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b7116d3..165cc89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ All notable changes to Voicebox will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed +- **Profile Name Validation** - Added proper validation to prevent duplicate profile names ([#134](https://github.com/jamiepine/voicebox/issues/134)) + - Users now receive clear error messages when attempting to create or update profiles with duplicate names + - Improved error handling in create and update profile API endpoints + - Added comprehensive test suite for duplicate name validation + ## [0.1.0] - 2026-01-25 ### Added diff --git a/backend/main.py b/backend/main.py index e218d23..c3ad18f 100644 --- a/backend/main.py +++ b/backend/main.py @@ -221,7 +221,10 @@ async def create_profile( """Create a new voice profile.""" try: return await profiles.create_profile(data, db) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) except Exception as e: + # Fallback for unexpected errors raise HTTPException(status_code=400, detail=str(e)) @@ -277,10 +280,13 @@ async def update_profile( db: Session = Depends(get_db), ): """Update a voice profile.""" - profile = await profiles.update_profile(profile_id, data, db) - if not profile: - raise HTTPException(status_code=404, detail="Profile not found") - return profile + try: + profile = await profiles.update_profile(profile_id, data, db) + if not profile: + raise HTTPException(status_code=404, detail="Profile not found") + return profile + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) @app.delete("/profiles/{profile_id}") diff --git a/backend/profiles.py b/backend/profiles.py index cda6bee..7b4cc93 100644 --- a/backend/profiles.py +++ b/backend/profiles.py @@ -38,14 +38,22 @@ async def create_profile( ) -> VoiceProfileResponse: """ Create a new voice profile. - + Args: data: Profile creation data db: Database session - + Returns: Created profile + + Raises: + ValueError: If a profile with the same name already exists """ + # Check if profile name already exists + existing_profile = db.query(DBVoiceProfile).filter_by(name=data.name).first() + if existing_profile: + raise ValueError(f"A profile with the name '{data.name}' already exists. Please choose a different name.") + # Create profile in database db_profile = DBVoiceProfile( id=str(uuid.uuid4()), @@ -55,15 +63,15 @@ async def create_profile( created_at=datetime.utcnow(), updated_at=datetime.utcnow(), ) - + db.add(db_profile) db.commit() db.refresh(db_profile) - + # Create profile directory profile_dir = _get_profiles_dir() / db_profile.id profile_dir.mkdir(parents=True, exist_ok=True) - + return VoiceProfileResponse.model_validate(db_profile) @@ -191,28 +199,37 @@ async def update_profile( ) -> Optional[VoiceProfileResponse]: """ Update a voice profile. - + Args: profile_id: Profile ID data: Updated profile data db: Database session - + Returns: Updated profile or None if not found + + Raises: + ValueError: If a profile with the same name already exists (different profile) """ profile = db.query(DBVoiceProfile).filter_by(id=profile_id).first() if not profile: return None - + + # Check if the new name conflicts with another profile + if profile.name != data.name: + existing_profile = db.query(DBVoiceProfile).filter_by(name=data.name).first() + if existing_profile: + raise ValueError(f"A profile with the name '{data.name}' already exists. Please choose a different name.") + # Update fields profile.name = data.name profile.description = data.description profile.language = data.language profile.updated_at = datetime.utcnow() - + db.commit() db.refresh(profile) - + return VoiceProfileResponse.model_validate(profile) diff --git a/backend/tests/test_profile_duplicate_names.py b/backend/tests/test_profile_duplicate_names.py new file mode 100644 index 0000000..81d4f7e --- /dev/null +++ b/backend/tests/test_profile_duplicate_names.py @@ -0,0 +1,217 @@ +""" +Tests for profile duplicate name validation. + +This test suite verifies that the application correctly handles +duplicate profile names and provides user-friendly error messages. +""" + +import pytest +import tempfile +import shutil +from pathlib import Path +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker + +# Add parent directory to path to import backend modules +import sys +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from database import Base, VoiceProfile as DBVoiceProfile +from models import VoiceProfileCreate +from profiles import create_profile, update_profile + + +@pytest.fixture +def test_db(): + """Create a temporary test database.""" + # Create temporary directory for test database + temp_dir = tempfile.mkdtemp() + db_path = Path(temp_dir) / "test.db" + + # Create engine and session + engine = create_engine(f"sqlite:///{db_path}") + Base.metadata.create_all(bind=engine) + SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) + + db = SessionLocal() + + yield db + + # Cleanup + db.close() + shutil.rmtree(temp_dir) + + +@pytest.fixture +def mock_profiles_dir(monkeypatch, tmp_path): + """Mock the profiles directory to use a temporary path.""" + import profiles + monkeypatch.setattr(profiles, '_get_profiles_dir', lambda: tmp_path) + return tmp_path + + +@pytest.mark.asyncio +async def test_create_profile_duplicate_name_raises_error(test_db, mock_profiles_dir): + """Test that creating a profile with a duplicate name raises a ValueError.""" + # Create first profile + profile_data_1 = VoiceProfileCreate( + name="Test Profile", + description="First profile", + language="en" + ) + + profile_1 = await create_profile(profile_data_1, test_db) + assert profile_1.name == "Test Profile" + + # Try to create second profile with same name + profile_data_2 = VoiceProfileCreate( + name="Test Profile", + description="Second profile", + language="en" + ) + + with pytest.raises(ValueError) as exc_info: + await create_profile(profile_data_2, test_db) + + # Verify error message is user-friendly + assert "already exists" in str(exc_info.value) + assert "Test Profile" in str(exc_info.value) + assert "choose a different name" in str(exc_info.value).lower() + + +@pytest.mark.asyncio +async def test_create_profile_different_names_succeeds(test_db, mock_profiles_dir): + """Test that creating profiles with different names succeeds.""" + # Create first profile + profile_data_1 = VoiceProfileCreate( + name="Profile One", + description="First profile", + language="en" + ) + + profile_1 = await create_profile(profile_data_1, test_db) + assert profile_1.name == "Profile One" + + # Create second profile with different name + profile_data_2 = VoiceProfileCreate( + name="Profile Two", + description="Second profile", + language="en" + ) + + profile_2 = await create_profile(profile_data_2, test_db) + assert profile_2.name == "Profile Two" + + # Verify both profiles exist + assert profile_1.id != profile_2.id + + +@pytest.mark.asyncio +async def test_update_profile_to_duplicate_name_raises_error(test_db, mock_profiles_dir): + """Test that updating a profile to a duplicate name raises a ValueError.""" + # Create two profiles with different names + profile_data_1 = VoiceProfileCreate( + name="Profile A", + description="First profile", + language="en" + ) + profile_1 = await create_profile(profile_data_1, test_db) + + profile_data_2 = VoiceProfileCreate( + name="Profile B", + description="Second profile", + language="en" + ) + profile_2 = await create_profile(profile_data_2, test_db) + + # Try to update profile_2 to use profile_1's name + update_data = VoiceProfileCreate( + name="Profile A", # Duplicate name + description="Updated description", + language="en" + ) + + with pytest.raises(ValueError) as exc_info: + await update_profile(profile_2.id, update_data, test_db) + + # Verify error message is user-friendly + assert "already exists" in str(exc_info.value) + assert "Profile A" in str(exc_info.value) + + +@pytest.mark.asyncio +async def test_update_profile_keep_same_name_succeeds(test_db, mock_profiles_dir): + """Test that updating a profile while keeping the same name succeeds.""" + # Create profile + profile_data = VoiceProfileCreate( + name="My Profile", + description="Original description", + language="en" + ) + profile = await create_profile(profile_data, test_db) + + # Update profile with same name but different description + update_data = VoiceProfileCreate( + name="My Profile", # Same name + description="Updated description", + language="en" + ) + + updated_profile = await update_profile(profile.id, update_data, test_db) + + # Verify update succeeded + assert updated_profile is not None + assert updated_profile.id == profile.id + assert updated_profile.name == "My Profile" + assert updated_profile.description == "Updated description" + + +@pytest.mark.asyncio +async def test_update_profile_to_new_unique_name_succeeds(test_db, mock_profiles_dir): + """Test that updating a profile to a new unique name succeeds.""" + # Create profile + profile_data = VoiceProfileCreate( + name="Original Name", + description="Profile description", + language="en" + ) + profile = await create_profile(profile_data, test_db) + + # Update profile with new unique name + update_data = VoiceProfileCreate( + name="New Unique Name", + description="Updated description", + language="en" + ) + + updated_profile = await update_profile(profile.id, update_data, test_db) + + # Verify update succeeded + assert updated_profile is not None + assert updated_profile.id == profile.id + assert updated_profile.name == "New Unique Name" + + +@pytest.mark.asyncio +async def test_case_sensitive_names_allowed(test_db, mock_profiles_dir): + """Test that profile names are case-sensitive (e.g., 'Test' and 'test' are different).""" + # Create profile with lowercase name + profile_data_1 = VoiceProfileCreate( + name="test profile", + description="Lowercase", + language="en" + ) + profile_1 = await create_profile(profile_data_1, test_db) + + # Create profile with different case + profile_data_2 = VoiceProfileCreate( + name="Test Profile", + description="Title case", + language="en" + ) + profile_2 = await create_profile(profile_data_2, test_db) + + # Both should succeed since SQLite unique constraint is case-sensitive by default + assert profile_1.name == "test profile" + assert profile_2.name == "Test Profile" + assert profile_1.id != profile_2.id