From 3172036bccc8095b42ebbbf1ea308ff43db19f85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20Bl=C3=A5man?= Date: Sat, 22 Aug 2026 22:07:32 +0000 Subject: [PATCH 1/2] Song and category edit fixes + tests --- routes/song_category_router.py | 18 ++++- routes/song_router.py | 14 ++-- tests/basic_factories.py | 15 ++++ tests/basic_fixtures.py | 18 +++++ tests/test_song_categories.py | 115 ++++++++++++++++++++++++++++ tests/test_songs.py | 136 +++++++++++++++++++++++++++++++++ 6 files changed, 309 insertions(+), 7 deletions(-) create mode 100644 tests/test_song_categories.py create mode 100644 tests/test_songs.py diff --git a/routes/song_category_router.py b/routes/song_category_router.py index c787dc69..f262ab0c 100644 --- a/routes/song_category_router.py +++ b/routes/song_category_router.py @@ -1,3 +1,5 @@ +from sqlalchemy import func + from database import DB_dependency from api_schemas.song_category_schemas import SongCategoryCreate, SongCategoryRead from db_models.song_category_model import SongCategory_DB @@ -23,8 +25,12 @@ def get_song_category(category_id: int, db: DB_dependency): @song_category_router.post("/", response_model=SongCategoryRead, dependencies=[Permission.require("manage", "Song")]) def create_song_category(song_category_data: SongCategoryCreate, db: DB_dependency): - num_existing = db.query(SongCategory_DB).filter(SongCategory_DB.name == song_category_data.name).count() - if num_existing > 0: + same_title = ( + db.query(SongCategory_DB.id) + .filter(func.lower(SongCategory_DB.name) == func.lower(song_category_data.name)) + .first() + ) + if same_title is not None: raise HTTPException(status.HTTP_400_BAD_REQUEST, detail="This song category already exists") songcategory = SongCategory_DB(name=song_category_data.name) db.add(songcategory) @@ -56,6 +62,14 @@ def update_song_category(category_id: int, category_data: SongCategoryCreate, db category = db.query(SongCategory_DB).filter_by(id=category_id).one_or_none() if category is None: raise HTTPException(status.HTTP_404_NOT_FOUND) + same_title = ( + db.query(SongCategory_DB.id) + .filter(func.lower(SongCategory_DB.name) == func.lower(category_data.name), SongCategory_DB.id != category_id) + .first() + ) + if same_title is not None: + raise HTTPException(status.HTTP_400_BAD_REQUEST, detail="This song category already exists") + category.name = category_data.name db.commit() return category diff --git a/routes/song_router.py b/routes/song_router.py index 79fb9997..ad42609f 100644 --- a/routes/song_router.py +++ b/routes/song_router.py @@ -1,10 +1,10 @@ from fastapi import APIRouter, HTTPException, status +from sqlalchemy import func from api_schemas.song_schemas import SongCreate, SongRead from database import DB_dependency from db_models.song_model import Song_DB from user.permission import Permission - song_router = APIRouter() @@ -29,8 +29,8 @@ def get_song(song_id: int, db: DB_dependency): @song_router.post("/", response_model=SongRead, dependencies=[Permission.require("manage", "Song")]) def create_song(song_data: SongCreate, db: DB_dependency): - num_existing = db.query(Song_DB).filter(Song_DB.title == song_data.title).count() - if num_existing > 0: + same_title = db.query(Song_DB.id).filter(func.lower(Song_DB.title) == func.lower(song_data.title)).first() + if same_title is not None: raise HTTPException(status.HTTP_400_BAD_REQUEST, detail="This song already exists") song = Song_DB( title=song_data.title, @@ -61,8 +61,12 @@ def update_song(song_id: int, song_data: SongCreate, db: DB_dependency): raise HTTPException(status.HTTP_404_NOT_FOUND) # simply check if the title is being changed to an already existing title, if so, throw an error. - num_existing = db.query(Song_DB).filter(Song_DB.title == song_data.title).count() - if num_existing > 0: + same_title = ( + db.query(Song_DB.id) + .filter(func.lower(Song_DB.title) == func.lower(song_data.title), Song_DB.id != song_id) + .first() + ) + if same_title is not None: raise HTTPException(status.HTTP_400_BAD_REQUEST, detail="This song already exists") # Allow for partial updates (kinda). We handle melody separately since we want to allow for None for var, value in vars(song_data).items(): diff --git a/tests/basic_factories.py b/tests/basic_factories.py index f01a4834..9f18c3be 100644 --- a/tests/basic_factories.py +++ b/tests/basic_factories.py @@ -50,6 +50,21 @@ def auth_headers(token): return {"Authorization": f"Bearer {token}"} +def category_data(name="Testkategori"): + return {"name": name} + + +def song_data(category_id, **kwargs): + data = { + "title": "Testsång", + "author": "Testförfattare", + "melody": "Testmelodi", + "content": "Testsångtext", + "category_id": category_id, + } + return {**data, **kwargs} + + def council_data_factory(**kwargs): """Factory for council create/update payloads.""" default_data = { diff --git a/tests/basic_fixtures.py b/tests/basic_fixtures.py index f7393f45..639e9784 100644 --- a/tests/basic_fixtures.py +++ b/tests/basic_fixtures.py @@ -296,6 +296,24 @@ def event(client, admin_token, admin_council_id): return response.json() +@pytest.fixture() +def song_category(client, admin_token): + """Create and return a song category.""" + + response = client.post("/songs-category/", json=category_data(), headers=auth_headers(admin_token)) + assert response.status_code in (200, 201), response.text + return response.json() + + +@pytest.fixture() +def song(client, admin_token, song_category): + """Create and return a song in the shared song category.""" + + response = client.post("/songs/", json=song_data(song_category["id"]), headers=auth_headers(admin_token)) + assert response.status_code in (200, 201), response.text + return response.json() + + @pytest.fixture() def nollning_event(client, admin_token, admin_council_id): """Create and return a nollning event which only accepts groups of type "Mentor".""" diff --git a/tests/test_song_categories.py b/tests/test_song_categories.py new file mode 100644 index 00000000..b3b53948 --- /dev/null +++ b/tests/test_song_categories.py @@ -0,0 +1,115 @@ +# type: ignore +import pytest +from .basic_factories import auth_headers, category_data + + +class TestCreateSongCategory: + """Test POST /songs-category/ endpoint""" + + def test_create_song_category_success(self, client, admin_token): + response = client.post( + "/songs-category/", + json=category_data("Visor"), + headers=auth_headers(admin_token), + ) + + assert response.status_code in (200, 201), response.text + assert response.json()["name"] == "Visor" + + def test_create_song_category_duplicate_is_case_insensitive(self, client, admin_token, song_category): + response = client.post( + "/songs-category/", + json=category_data(song_category["name"].upper()), + headers=auth_headers(admin_token), + ) + + assert response.status_code == 400 + + @pytest.mark.parametrize("token_fixture", ["member_token", "non_member_token"]) + def test_create_song_category_forbidden(self, client, request, token_fixture): + response = client.post( + "/songs-category/", + json=category_data("Forbidden"), + headers=auth_headers(request.getfixturevalue(token_fixture)), + ) + + assert response.status_code == 403 + + def test_create_song_category_unauthenticated(self, client): + response = client.post("/songs-category/", json=category_data("Unauthenticated")) + + assert response.status_code == 401 + + +class TestGetSongCategories: + """Test GET /songs-category/ and GET /songs-category/{category_id} endpoints""" + + def test_get_all_song_categories(self, client, song_category): + response = client.get("/songs-category/") + + assert response.status_code == 200 + assert song_category["id"] in [category["id"] for category in response.json()] + + def test_get_single_song_category(self, client, song_category): + response = client.get(f"/songs-category/{song_category['id']}") + + assert response.status_code == 200 + assert response.json() == song_category + + def test_get_missing_song_category(self, client): + response = client.get("/songs-category/999999") + + assert response.status_code == 404 + + +class TestUpdateSongCategory: + """Test PATCH /songs-category/{category_id} endpoint""" + + def test_update_song_category_success(self, client, admin_token, song_category): + response = client.patch( + f"/songs-category/{song_category['id']}", + json=category_data("Updated Category"), + headers=auth_headers(admin_token), + ) + + assert response.status_code == 200 + assert response.json()["name"] == "Updated Category" + + def test_update_song_category_duplicate_is_case_insensitive(self, client, admin_token, song_category): + other = client.post( + "/songs-category/", + json=category_data("Other Category"), + headers=auth_headers(admin_token), + ).json() + + response = client.patch( + f"/songs-category/{other['id']}", + json=category_data(song_category["name"].lower()), + headers=auth_headers(admin_token), + ) + + assert response.status_code == 400 + + def test_update_song_category_forbidden(self, client, member_token, song_category): + response = client.patch( + f"/songs-category/{song_category['id']}", + json=category_data("Forbidden Update"), + headers=auth_headers(member_token), + ) + + assert response.status_code == 403 + + +class TestDeleteSongCategory: + """Test DELETE /songs-category/{category_id} endpoint""" + + def test_delete_song_category_success(self, client, admin_token, song_category): + response = client.delete(f"/songs-category/{song_category['id']}", headers=auth_headers(admin_token)) + + assert response.status_code == 200 + assert client.get(f"/songs-category/{song_category['id']}").status_code == 404 + + def test_delete_song_category_forbidden(self, client, member_token, song_category): + response = client.delete(f"/songs-category/{song_category['id']}", headers=auth_headers(member_token)) + + assert response.status_code == 403 diff --git a/tests/test_songs.py b/tests/test_songs.py new file mode 100644 index 00000000..62d68e4d --- /dev/null +++ b/tests/test_songs.py @@ -0,0 +1,136 @@ +# type: ignore +import pytest +from .basic_factories import auth_headers, song_data + + +class TestCreateSong: + """Test POST /songs/ endpoint""" + + def test_create_song_success(self, client, admin_token, song_category): + data = song_data(song_category["id"]) + response = client.post("/songs/", json=data, headers=auth_headers(admin_token)) + + assert response.status_code in (200, 201), response.text + created = response.json() + assert created["title"] == data["title"] + assert created["author"] == data["author"] + assert created["content"] == data["content"] + assert created["category"]["id"] == song_category["id"] + assert created["views"] == 0 + + def test_create_song_duplicate_title_is_rejected(self, client, admin_token, song): + response = client.post( + "/songs/", + json=song_data(song["category"]["id"]), + headers=auth_headers(admin_token), + ) + + assert response.status_code == 400 + + @pytest.mark.parametrize("token_fixture", ["member_token", "non_member_token"]) + def test_create_song_forbidden(self, client, request, song_category, token_fixture): + response = client.post( + "/songs/", + json=song_data(song_category["id"]), + headers=auth_headers(request.getfixturevalue(token_fixture)), + ) + + assert response.status_code == 403 + + def test_create_song_unauthenticated(self, client, song_category): + response = client.post("/songs/", json=song_data(song_category["id"])) + + assert response.status_code == 401 + + +class TestGetSongs: + """Test GET /songs/ and GET /songs/{song_id} endpoints""" + + def test_get_all_songs(self, client, song): + response = client.get("/songs/") + + assert response.status_code == 200 + assert song["id"] in [listed["id"] for listed in response.json()] + + def test_get_single_song_increments_views(self, client, song): + response = client.get(f"/songs/{song['id']}") + + assert response.status_code == 200 + assert response.json()["title"] == song["title"] + assert response.json()["views"] == 1 + + def test_get_missing_song(self, client): + response = client.get("/songs/999999") + + assert response.status_code == 404 + + +class TestUpdateSong: + """Test PATCH /songs/{song_id} endpoint""" + + def test_update_song_success(self, client, admin_token, song): + response = client.patch( + f"/songs/{song['id']}", + json=song_data( + song["category"]["id"], + title="Updated Song", + author=None, + melody=None, + content="Updated lyrics", + ), + headers=auth_headers(admin_token), + ) + + assert response.status_code == 200 + updated = response.json() + assert updated["title"] == "Updated Song" + assert updated["content"] == "Updated lyrics" + assert updated["author"] == song["author"] + assert updated["melody"] is None + + def test_update_song_duplicate_title_is_rejected(self, client, admin_token, song, song_category): + other = client.post( + "/songs/", + json=song_data(song_category["id"], title="Other Song"), + headers=auth_headers(admin_token), + ).json() + + response = client.patch( + f"/songs/{other['id']}", + json=song_data(song_category["id"], title=song["title"]), + headers=auth_headers(admin_token), + ) + + assert response.status_code == 400 + + def test_update_song_forbidden(self, client, member_token, song): + response = client.patch( + f"/songs/{song['id']}", + json=song_data(song["category"]["id"], title="Forbidden Update"), + headers=auth_headers(member_token), + ) + + assert response.status_code == 403 + + +class TestDeleteSong: + """Test DELETE /songs/{song_id} endpoint""" + + def test_delete_song_success(self, client, admin_token, song): + response = client.delete(f"/songs/{song['id']}", headers=auth_headers(admin_token)) + + assert response.status_code == 200 + assert client.get(f"/songs/{song['id']}").status_code == 404 + + def test_delete_song_forbidden(self, client, member_token, song): + response = client.delete(f"/songs/{song['id']}", headers=auth_headers(member_token)) + + assert response.status_code == 403 + + def test_delete_category_detaches_song(self, client, admin_token, song, song_category): + response = client.delete(f"/songs-category/{song_category['id']}", headers=auth_headers(admin_token)) + + assert response.status_code == 200 + song_response = client.get(f"/songs/{song['id']}") + assert song_response.status_code == 200 + assert song_response.json()["category"] is None From acb8e52c172bcb45af31afb87edb1255fc1418d8 Mon Sep 17 00:00:00 2001 From: georgelgeback Date: Fri, 28 Aug 2026 11:09:42 +0000 Subject: [PATCH 2/2] Add regression tests for fix --- tests/test_songs.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/tests/test_songs.py b/tests/test_songs.py index 62d68e4d..8e885328 100644 --- a/tests/test_songs.py +++ b/tests/test_songs.py @@ -27,6 +27,15 @@ def test_create_song_duplicate_title_is_rejected(self, client, admin_token, song assert response.status_code == 400 + def test_create_song_duplicate_title_is_case_insensitive(self, client, admin_token, song): + response = client.post( + "/songs/", + json=song_data(song["category"]["id"], title=song["title"].upper()), + headers=auth_headers(admin_token), + ) + + assert response.status_code == 400 + @pytest.mark.parametrize("token_fixture", ["member_token", "non_member_token"]) def test_create_song_forbidden(self, client, request, song_category, token_fixture): response = client.post( @@ -88,6 +97,17 @@ def test_update_song_success(self, client, admin_token, song): assert updated["author"] == song["author"] assert updated["melody"] is None + def test_update_song_keeping_own_title(self, client, admin_token, song): + response = client.patch( + f"/songs/{song['id']}", + json=song_data(song["category"]["id"], title=song["title"], content="Updated lyrics"), + headers=auth_headers(admin_token), + ) + + assert response.status_code == 200, response.text + assert response.json()["title"] == song["title"] + assert response.json()["content"] == "Updated lyrics" + def test_update_song_duplicate_title_is_rejected(self, client, admin_token, song, song_category): other = client.post( "/songs/", @@ -97,7 +117,7 @@ def test_update_song_duplicate_title_is_rejected(self, client, admin_token, song response = client.patch( f"/songs/{other['id']}", - json=song_data(song_category["id"], title=song["title"]), + json=song_data(song_category["id"], title=song["title"].upper()), headers=auth_headers(admin_token), )