Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions app/api/projects/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ async def list_projects(
projects = await project_service.list_projects_for_user(
db, user, str(organization_id) if organization_id else None, language_id
)
return [ProjectResponse.model_validate(p) for p in projects]
return await project_service.serialize_projects(db, projects)


@router.post("", response_model=ProjectResponse, status_code=status.HTTP_201_CREATED)
Expand All @@ -47,7 +47,7 @@ async def create_project(
location_display_name=payload.location_display_name,
creator_user_id=str(user.id),
)
return ProjectResponse.model_validate(project)
return await project_service.serialize_project(db, project)


@router.get("/{project_id}", response_model=ProjectResponse)
Expand All @@ -58,7 +58,7 @@ async def get_project(
) -> ProjectResponse:
project = await project_service.get_project_or_404(db, project_id)
await assert_project_access(db, user, project_id)
return ProjectResponse.model_validate(project)
return await project_service.serialize_project(db, project)


@router.patch("/{project_id}", response_model=ProjectResponse)
Expand All @@ -76,7 +76,7 @@ async def update_project(
description=payload.description,
language_id=payload.language_id,
)
return ProjectResponse.model_validate(project)
return await project_service.serialize_project(db, project)


@router.patch("/{project_id}/location", response_model=ProjectResponse)
Expand All @@ -94,4 +94,4 @@ async def update_project_location(
longitude=payload.longitude,
location_display_name=payload.location_display_name,
)
return ProjectResponse.model_validate(project)
return await project_service.serialize_project(db, project)
1 change: 1 addition & 0 deletions app/models/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class ProjectResponse(BaseModel):
location_display_name: str | None
created_at: datetime
updated_at: datetime
team_size: int = 0

model_config = {"from_attributes": True}

Expand Down
8 changes: 8 additions & 0 deletions app/services/project/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from app.services.project.can_access_project import can_access_project
from app.services.project.count_project_team_sizes import count_project_team_sizes
from app.services.project.create_project import create_project
from app.services.project.get_project_by_id import get_project_by_id
from app.services.project.get_project_or_404 import get_project_or_404
Expand All @@ -19,12 +20,17 @@
from app.services.project.list_user_project_roles import list_user_project_roles
from app.services.project.revoke_organization_access import revoke_organization_access
from app.services.project.revoke_user_access import revoke_user_access
from app.services.project.serialize_project_responses import (
serialize_project,
serialize_projects,
)
from app.services.project.update_project import update_project
from app.services.project.update_project_location import update_project_location
from app.services.project.update_user_access_role import update_user_access_role

__all__ = [
"can_access_project",
"count_project_team_sizes",
"create_project",
"get_project_by_id",
"get_project_or_404",
Expand All @@ -39,6 +45,8 @@
"list_user_project_roles",
"revoke_organization_access",
"revoke_user_access",
"serialize_project",
"serialize_projects",
"update_project",
"update_project_location",
"update_user_access_role",
Expand Down
20 changes: 20 additions & 0 deletions app/services/project/count_project_team_sizes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
from sqlalchemy import distinct, func, select
from sqlalchemy.ext.asyncio import AsyncSession

from app.db.models.project import ProjectUserAccess


async def count_project_team_sizes(db: AsyncSession, project_ids: list[str]) -> dict[str, int]:
if not project_ids:
return {}
stmt = (
select(
ProjectUserAccess.project_id,
func.count(distinct(ProjectUserAccess.user_id)),
)
.where(ProjectUserAccess.project_id.in_(project_ids))
.group_by(ProjectUserAccess.project_id)
)
result = await db.execute(stmt)
counts: dict[str, int] = {row[0]: row[1] for row in result.all()}
return {project_id: counts.get(project_id, 0) for project_id in project_ids}
18 changes: 18 additions & 0 deletions app/services/project/serialize_project_responses.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
from sqlalchemy.ext.asyncio import AsyncSession

from app.db.models.project import Project
from app.models.project import ProjectResponse
from app.services.project.count_project_team_sizes import count_project_team_sizes


async def serialize_projects(db: AsyncSession, projects: list[Project]) -> list[ProjectResponse]:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every other file here has one public function named after the file (create_project.py, count_project_team_sizes.py, and so on). This one has two and the name matches neither. Please verify that on the folder and if it is really the majority, split it and adapt the __init__.

team_sizes = await count_project_team_sizes(db, [p.id for p in projects])
return [
ProjectResponse.model_validate(p).model_copy(update={"team_size": team_sizes.get(p.id, 0)})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ setting the field with a string key. model_copy(update=...) does not validate, and team_size has = 0 as default on the schema, so a rename/typo on this key returns 0 silently instead of failing. For me it is relevant to keep this typed — could you explore the feasibility of it?

for p in projects
]


async def serialize_project(db: AsyncSession, project: Project) -> ProjectResponse:
(response,) = await serialize_projects(db, [project])
return response
Comment on lines +8 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add concise docstrings to the exported service functions.

serialize_projects and serialize_project are public service functions and should document the response-enrichment contract, including the computed team_size.

As per coding guidelines, app/services/**/*.py requires concise docstrings on public service functions.

Suggested documentation
 async def serialize_projects(db: AsyncSession, projects: list[Project]) -> list[ProjectResponse]:
+    """Serialize projects and attach distinct direct-access team sizes."""
     team_sizes = await count_project_team_sizes(db, [p.id for p in projects])

 async def serialize_project(db: AsyncSession, project: Project) -> ProjectResponse:
+    """Serialize one project and attach its distinct direct-access team size."""
     (response,) = await serialize_projects(db, [project])
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def serialize_projects(db: AsyncSession, projects: list[Project]) -> list[ProjectResponse]:
team_sizes = await count_project_team_sizes(db, [p.id for p in projects])
return [
ProjectResponse.model_validate(p).model_copy(update={"team_size": team_sizes.get(p.id, 0)})
for p in projects
]
async def serialize_project(db: AsyncSession, project: Project) -> ProjectResponse:
(response,) = await serialize_projects(db, [project])
return response
async def serialize_projects(db: AsyncSession, projects: list[Project]) -> list[ProjectResponse]:
"""Serialize projects and attach distinct direct-access team sizes."""
team_sizes = await count_project_team_sizes(db, [p.id for p in projects])
return [
ProjectResponse.model_validate(p).model_copy(update={"team_size": team_sizes.get(p.id, 0)})
for p in projects
]
async def serialize_project(db: AsyncSession, project: Project) -> ProjectResponse:
"""Serialize one project and attach its distinct direct-access team size."""
(response,) = await serialize_projects(db, [project])
return response
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/services/project/serialize_project_responses.py` around lines 8 - 18, Add
concise docstrings to the public functions serialize_projects and
serialize_project describing that they serialize projects into responses and
enrich each response with the computed team_size.

Source: Coding guidelines

51 changes: 51 additions & 0 deletions tests/test_project_team_size.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import pytest

from app.services import project_service
from tests.baker import (
make_language,
make_project,
make_project_user_access,
make_user,
)


@pytest.mark.asyncio
async def test_count_team_sizes_counts_distinct_users(db_session) -> None:
lang = await make_language(db_session, code="tsa")
project = await make_project(db_session, language_id=lang.id)
u1 = await make_user(db_session, email="[email protected]")
u2 = await make_user(db_session, email="[email protected]")
await make_project_user_access(db_session, project.id, u1.id)
await make_project_user_access(db_session, project.id, u2.id)

sizes = await project_service.count_project_team_sizes(db_session, [project.id])
assert sizes == {project.id: 2}


@pytest.mark.asyncio
async def test_count_team_sizes_zero_for_project_without_access(db_session) -> None:
lang = await make_language(db_session, code="tsb")
project = await make_project(db_session, language_id=lang.id)

sizes = await project_service.count_project_team_sizes(db_session, [project.id])
assert sizes == {project.id: 0}


@pytest.mark.asyncio
async def test_count_team_sizes_isolates_projects(db_session) -> None:
lang = await make_language(db_session, code="tsc")
p1 = await make_project(db_session, language_id=lang.id, name="P1")
p2 = await make_project(db_session, language_id=lang.id, name="P2")
u1 = await make_user(db_session, email="[email protected]")
u2 = await make_user(db_session, email="[email protected]")
await make_project_user_access(db_session, p1.id, u1.id)
await make_project_user_access(db_session, p1.id, u2.id)
await make_project_user_access(db_session, p2.id, u1.id)

sizes = await project_service.count_project_team_sizes(db_session, [p1.id, p2.id])
assert sizes == {p1.id: 2, p2.id: 1}


@pytest.mark.asyncio
async def test_count_team_sizes_empty_input(db_session) -> None:
assert await project_service.count_project_team_sizes(db_session, []) == {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the tests are on count_project_team_sizes, but what the 5 endpoints call now is serialize_projects, and the team_size merge itself has no test. Could you add one here? @levigtri

Loading