mirror of
https://github.com/jamiepine/voicebox.git
synced 2026-10-03 09:05:17 -07:00
Merge pull request #175 from Vaibhavee89/fix/profile-duplicate-name-validation
Fix #134: Add validation for duplicate profile names
This commit is contained in:
@@ -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/),
|
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).
|
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
|
## [0.1.0] - 2026-01-25
|
||||||
|
|
||||||
### Added
|
### Added
|
||||||
|
|||||||
+10
-4
@@ -222,7 +222,10 @@ async def create_profile(
|
|||||||
"""Create a new voice profile."""
|
"""Create a new voice profile."""
|
||||||
try:
|
try:
|
||||||
return await profiles.create_profile(data, db)
|
return await profiles.create_profile(data, db)
|
||||||
|
except ValueError as e:
|
||||||
|
raise HTTPException(status_code=400, detail=str(e))
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
|
# Fallback for unexpected errors
|
||||||
raise HTTPException(status_code=400, detail=str(e))
|
raise HTTPException(status_code=400, detail=str(e))
|
||||||
|
|
||||||
|
|
||||||
@@ -278,10 +281,13 @@ async def update_profile(
|
|||||||
db: Session = Depends(get_db),
|
db: Session = Depends(get_db),
|
||||||
):
|
):
|
||||||
"""Update a voice profile."""
|
"""Update a voice profile."""
|
||||||
profile = await profiles.update_profile(profile_id, data, db)
|
try:
|
||||||
if not profile:
|
profile = await profiles.update_profile(profile_id, data, db)
|
||||||
raise HTTPException(status_code=404, detail="Profile not found")
|
if not profile:
|
||||||
return 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}")
|
@app.delete("/profiles/{profile_id}")
|
||||||
|
|||||||
@@ -45,7 +45,15 @@ async def create_profile(
|
|||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
Created profile
|
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
|
# Create profile in database
|
||||||
db_profile = DBVoiceProfile(
|
db_profile = DBVoiceProfile(
|
||||||
id=str(uuid.uuid4()),
|
id=str(uuid.uuid4()),
|
||||||
@@ -199,11 +207,20 @@ async def update_profile(
|
|||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
Updated profile or None if not found
|
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()
|
profile = db.query(DBVoiceProfile).filter_by(id=profile_id).first()
|
||||||
if not profile:
|
if not profile:
|
||||||
return None
|
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
|
# Update fields
|
||||||
profile.name = data.name
|
profile.name = data.name
|
||||||
profile.description = data.description
|
profile.description = data.description
|
||||||
|
|||||||
@@ -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
|
||||||
Reference in New Issue
Block a user