Skip to content
Merged
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
2 changes: 2 additions & 0 deletions backend/apps/api/rest/v0/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from apps.api.rest.v0.event import router as event_router
from apps.api.rest.v0.issue import router as issue_router
from apps.api.rest.v0.member import router as member_router
from apps.api.rest.v0.milestone import router as milestone_router
from apps.api.rest.v0.organization import router as organization_router
from apps.api.rest.v0.project import router as project_router
from apps.api.rest.v0.release import router as release_router
Expand All @@ -23,6 +24,7 @@
"/events": event_router,
"/issues": issue_router,
"/members": member_router,
"/milestones": milestone_router,
"/organizations": organization_router,
"/projects": project_router,
"/releases": release_router,
Expand Down
124 changes: 124 additions & 0 deletions backend/apps/api/rest/v0/milestone.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
"""Milestone API."""

from datetime import datetime
from http import HTTPStatus
from typing import Literal

from django.http import HttpRequest
from ninja import Field, FilterSchema, Path, Query, Schema
from ninja.decorators import decorate_view
from ninja.pagination import RouterPaginated
from ninja.responses import Response

from apps.api.decorators.cache import cache_response
from apps.github.models.generic_issue_model import GenericIssueModel
from apps.github.models.milestone import Milestone as MilestoneModel

router = RouterPaginated(tags=["Milestones"])


class MilestoneBase(Schema):
"""Base schema for Milestone (used in list endpoints)."""

created_at: datetime
number: int
state: GenericIssueModel.State
title: str
updated_at: datetime
url: str


class Milestone(MilestoneBase):
"""Schema for Milestone (minimal fields for list display)."""

Comment on lines +31 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Remove redundant schema or add distinguishing fields.

The Milestone class is currently empty and identical to MilestoneBase. This is code duplication. Either:

  • Remove Milestone and use MilestoneBase directly in the list endpoint, or
  • Add specific fields or behavior that distinguishes Milestone from MilestoneBase.

Apply this diff if no distinction is needed:

-class Milestone(MilestoneBase):
-    """Schema for Milestone (minimal fields for list display)."""
-
-
 class MilestoneDetail(MilestoneBase):
     """Detail schema for Milestone (used in single item endpoints)."""
 
@@ -73,7 +70,7 @@
     "/",
     description="Retrieve a paginated list of GitHub milestones.",
     operation_id="list_milestones",
-    response=list[Milestone],
+    response=list[MilestoneBase],
     summary="List milestones",
 )
 @decorate_view(cache_response())
 def list_milestones(
     request: HttpRequest,
     filters: MilestoneFilter = Query(...),
     ordering: Literal["created_at", "-created_at", "updated_at", "-updated_at"] | None = None,
-) -> list[Milestone]:
+) -> list[MilestoneBase]:

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In backend/apps/api/rest/v0/milestone.py around lines 31 to 33, the Milestone
class is a direct duplicate of MilestoneBase and should be removed or
differentiated; either delete the Milestone class and update call sites to use
MilestoneBase for list endpoints, or add the extra fields/validation/metadata
that make Milestone distinct (e.g., trimmed fields for list display or
additional read-only properties) and update tests/docs accordingly.


class MilestoneDetail(MilestoneBase):
"""Detail schema for Milestone (used in single item endpoints)."""

body: str
closed_issues_count: int
due_on: datetime | None
open_issues_count: int


class MilestoneError(Schema):
"""Milestone error schema."""

message: str


class MilestoneFilter(FilterSchema):
"""Filter for Milestone."""

organization: str | None = Field(
None,
description="Organization that milestones belong to (filtered by repository owner)",
example="OWASP",
)
repository: str | None = Field(
None,
description="Repository that milestones belong to",
example="Nest",
)
state: GenericIssueModel.State | None = Field(
None,
description="Milestone state",
)


@router.get(
"/",
description="Retrieve a paginated list of GitHub milestones.",
operation_id="list_milestones",
response=list[Milestone],
summary="List milestones",
)
@decorate_view(cache_response())
def list_milestones(
request: HttpRequest,
filters: MilestoneFilter = Query(...),
ordering: Literal["created_at", "-created_at", "updated_at", "-updated_at"] | None = None,
) -> list[Milestone]:
"""Get all milestones."""
milestones = MilestoneModel.objects.select_related("repository", "repository__organization")

if filters.organization:
milestones = milestones.filter(
repository__organization__login__iexact=filters.organization
)
if filters.repository:
milestones = milestones.filter(repository__name__iexact=filters.repository)
if filters.state:
milestones = milestones.filter(state=filters.state)

return milestones.order_by(ordering or "-created_at", "-updated_at")
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix contradictory ordering logic.

When ordering is provided (e.g., "updated_at"), the code produces order_by("updated_at", "-updated_at"), which contradicts itself. The secondary sort on "-updated_at" should only be added when it's not already the primary sort field.

Apply this diff to fix the ordering:

-    return milestones.order_by(ordering or "-created_at", "-updated_at")
+    order_fields = [ordering] if ordering else ["-created_at"]
+    # Add secondary sort on updated_at if not already sorting by it
+    if ordering not in ("updated_at", "-updated_at"):
+        order_fields.append("-updated_at")
+    return milestones.order_by(*order_fields)
🤖 Prompt for AI Agents
In backend/apps/api/rest/v0/milestone.py around line 94, the current return
builds order_by(ordering or "-created_at", "-updated_at") which can produce
contradictory pairs like ("updated_at", "-updated_at"); change it so the
secondary "-updated_at" is only appended when the primary ordering (stripped of
any leading "-") is not "updated_at". Implement by determining primary =
ordering or "-created_at", check if (ordering or "").lstrip("-") !=
"updated_at", and call milestones.order_by(primary, "-updated_at") only in that
case, otherwise call milestones.order_by(primary).



@router.get(
"/{str:organization_id}/{str:repository_id}/{int:milestone_id}",
description=(
"Retrieve a specific GitHub milestone by organization, repository, and milestone number."
),
operation_id="get_milestone",
response={
HTTPStatus.NOT_FOUND: MilestoneError,
HTTPStatus.OK: MilestoneDetail,
},
summary="Get milestone",
)
@decorate_view(cache_response())
def get_milestone(
request: HttpRequest,
organization_id: str = Path(example="OWASP"),
repository_id: str = Path(example="Nest"),
milestone_id: int = Path(example=1),
) -> MilestoneDetail | MilestoneError:
"""Get milestone."""
try:
return MilestoneModel.objects.get(
repository__organization__login__iexact=organization_id,
repository__name__iexact=repository_id,
number=milestone_id,
)
except MilestoneModel.DoesNotExist:
return Response({"message": "Milestone not found"}, status=HTTPStatus.NOT_FOUND)
Comment on lines +123 to +124
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Return schema instance instead of Response object.

The function signature declares it returns MilestoneDetail | MilestoneError, but line 124 returns a Response object. Django Ninja automatically handles status code mapping based on return type—you should return an instance of MilestoneError schema instead.

Apply this diff:

     except MilestoneModel.DoesNotExist:
-        return Response({"message": "Milestone not found"}, status=HTTPStatus.NOT_FOUND)
+        return HTTPStatus.NOT_FOUND, MilestoneError(message="Milestone not found")
📝 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
except MilestoneModel.DoesNotExist:
return Response({"message": "Milestone not found"}, status=HTTPStatus.NOT_FOUND)
except MilestoneModel.DoesNotExist:
return HTTPStatus.NOT_FOUND, MilestoneError(message="Milestone not found")
🤖 Prompt for AI Agents
In backend/apps/api/rest/v0/milestone.py around lines 123-124, the except block
currently returns a Django Response object which violates the declared return
types; replace the Response return with an instance of the MilestoneError schema
(e.g., MilestoneError(message="Milestone not found")) so Django Ninja can map
the status correctly, and ensure MilestoneError is imported at the top of the
file.

89 changes: 89 additions & 0 deletions backend/tests/apps/api/rest/v0/milestone_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
from datetime import datetime

import pytest

from apps.api.rest.v0.milestone import MilestoneDetail


class TestMilestoneSchema:
@pytest.mark.parametrize(
"milestone_data",
[
{
"body": "This is a test milestone for Q1",
"closed_issues_count": 5,
"created_at": "2024-01-01T00:00:00Z",
"due_on": "2024-03-31T23:59:59Z",
"number": 1,
"open_issues_count": 3,
"state": "open",
"title": "Q1 2024 Release",
"updated_at": "2024-01-15T00:00:00Z",
"url": "https://github.com/OWASP/Nest/milestone/1",
},
{
"body": "Completed milestone for Q4 2023",
"closed_issues_count": 15,
"created_at": "2023-10-01T00:00:00Z",
"due_on": "2023-12-31T23:59:59Z",
"number": 2,
"open_issues_count": 0,
"state": "closed",
"title": "Q4 2023 Release",
"updated_at": "2024-01-05T00:00:00Z",
"url": "https://github.com/OWASP/Nest/milestone/2",
},
{
"body": "Milestone without due date",
"closed_issues_count": 2,
"created_at": "2024-02-01T00:00:00Z",
"due_on": None,
"number": 3,
"open_issues_count": 8,
"state": "open",
"title": "Backlog",
"updated_at": "2024-02-15T00:00:00Z",
"url": "https://github.com/OWASP/Nest/milestone/3",
},
],
)
def test_milestone_schema(self, milestone_data):
milestone = MilestoneDetail(**milestone_data)

assert milestone.body == milestone_data["body"]
assert milestone.closed_issues_count == milestone_data["closed_issues_count"]
assert milestone.created_at == datetime.fromisoformat(milestone_data["created_at"])
if milestone_data["due_on"]:
assert milestone.due_on == datetime.fromisoformat(milestone_data["due_on"])
else:
assert milestone.due_on is None
assert milestone.number == milestone_data["number"]
assert milestone.open_issues_count == milestone_data["open_issues_count"]
assert milestone.state == milestone_data["state"]
assert milestone.title == milestone_data["title"]
assert milestone.updated_at == datetime.fromisoformat(milestone_data["updated_at"])
assert milestone.url == milestone_data["url"]

def test_milestone_schema_with_minimal_data(self):
"""Test milestone schema with minimal required fields."""
minimal_data = {
"body": "",
"closed_issues_count": 0,
"created_at": "2024-01-01T00:00:00Z",
"due_on": None,
"number": 1,
"open_issues_count": 0,
"state": "open",
"title": "Test Milestone",
"updated_at": "2024-01-01T00:00:00Z",
"url": "https://github.com/test/repo/milestone/1",
}
milestone = MilestoneDetail(**minimal_data)

assert milestone.body == ""
assert milestone.closed_issues_count == 0
assert milestone.due_on is None
assert milestone.number == 1
assert milestone.open_issues_count == 0
assert milestone.title == "Test Milestone"
assert milestone.state == "open"
2 changes: 2 additions & 0 deletions backend/tests/apps/api/rest/v0/urls_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from apps.api.rest.v0.event import router as event_router
from apps.api.rest.v0.issue import router as issue_router
from apps.api.rest.v0.member import router as member_router
from apps.api.rest.v0.milestone import router as milestone_router
from apps.api.rest.v0.organization import router as organization_router
from apps.api.rest.v0.project import router as project_router
from apps.api.rest.v0.release import router as release_router
Expand All @@ -24,6 +25,7 @@ class TestRouterRegistration:
"/events": event_router,
"/issues": issue_router,
"/members": member_router,
"/milestones": milestone_router,
"/organizations": organization_router,
"/projects": project_router,
"/releases": release_router,
Expand Down