Skip to content

feature: DJango Ninja and drf cleanup #1646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

abhayymishraa
Copy link
Collaborator

Resolves #1601

  • Migrated from drf to django ninja

Add the PR description here.

Copy link
Contributor

coderabbitai bot commented Jun 21, 2025

Summary by CodeRabbit

  • New Features

    • All API endpoints now use a streamlined interface for data retrieval, offering consistent and efficient responses.
  • Refactor

    • The backend API framework has been migrated from Django REST Framework to Django Ninja, resulting in simplified endpoints and improved schema validation.
    • URL routing for APIs has been updated for better modularity and clarity.
  • Chores

    • Dependencies have been updated to remove Django REST Framework and add Django Ninja.
    • Unused linter suppression comments have been removed.
  • Tests

    • All tests have been updated to use the new schema-based validation approach, ensuring continued reliability.

Summary by CodeRabbit

  • New Features

    • API endpoints have been migrated to use the Ninja framework with Pydantic schemas, providing consistent and streamlined responses for GitHub and OWASP resources.
  • Refactor

    • All previous Django REST Framework-based API endpoints have been replaced with Ninja-based routers and Pydantic models.
    • URL routing has been updated to use Ninja routers for both GitHub and OWASP APIs.
  • Chores

    • Added the django-ninja package as a dependency.
    • Updated linter configuration to ignore specific argument warnings.
    • Removed # noqa: ARG001 linter suppression comments from Slack app and event handler code.
  • Tests

    • Tests were updated to replace DRF serializer usage with Pydantic schema instantiation and attribute assertions.
    • Router registration tests were refactored to validate sub-router instances instead of route names and viewset classes.

Walkthrough

The codebase migrates its REST API implementation from Django REST Framework (DRF) to Django Ninja. This involves replacing DRF serializers and viewsets with Pydantic schemas and Ninja routers, updating URL routing, and refactoring tests to use Pydantic schemas. Linter and dependency configurations are also updated accordingly. Minor unrelated changes include removal of linter suppression comments in Slack app code.

Changes

Files/Groups Change Summary
backend/apps/github/api/issue.py, .../label.py, .../organization.py, .../release.py, .../repository.py, .../user.py Replaced DRF serializers/viewsets with Pydantic schemas and Ninja routers/endpoints for each resource.
backend/apps/github/api/urls.py, backend/apps/owasp/api/urls.py, backend/settings/urls.py Replaced DRF router setup with Ninja router setup and updated API route registration.
backend/apps/owasp/api/chapter.py, .../committee.py, .../event.py, .../project.py Replaced DRF serializers/viewsets with Pydantic schemas and Ninja routers/endpoints for each resource.
backend/pyproject.toml Added django-ninja dependency; updated Ruff linter config to ignore ARG001.
backend/apps/slack/apps.py, backend/apps/slack/events/member_joined_channel/catch_all.py Removed # noqa: ARG001 linter suppression comments from function definitions.
backend/tests/apps/github/api/issue_test.py, .../label_test.py, .../organization_test.py, .../release_test.py, .../repository_test.py, .../urls_test.py, .../user_test.py Refactored tests to replace DRF serializer validation with Pydantic schema instantiation and attribute assertions.
backend/tests/apps/owasp/api/chapter_test.py, .../committee_test.py, .../event_test.py, .../project_test.py, .../urls_test.py Refactored tests to replace DRF serializer validation with Pydantic schema instantiation and attribute assertions.

Assessment against linked issues

Objective (Issue #) Addressed Explanation
Migrate REST API to Django ninja (#1601)
Remove DRF serializers and viewsets (#1601)
Replace DRF routers with Ninja routers (#1601)
Update dependency and linter config for Ninja (#1601)

Assessment against linked issues: Out-of-scope changes

Code Change (file_path) Explanation
Removed # noqa: ARG001 comments (backend/apps/slack/apps.py, backend/apps/slack/events/member_joined_channel/catch_all.py) These linter suppression comment removals are unrelated to the migration to Django Ninja.

Suggested labels

backend

Suggested reviewers

  • kasya
  • arkid15r

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 441d6e3 and d8e089e.

📒 Files selected for processing (1)
  • backend/apps/github/api/urls.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/apps/github/api/urls.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Run frontend e2e tests
  • GitHub Check: Run backend tests
  • GitHub Check: Run frontend unit tests
  • GitHub Check: CodeQL (javascript-typescript)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate Unit Tests
  • Create PR with Unit Tests
  • Post Copyable Unit Tests in Comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai auto-generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🔭 Outside diff range comments (5)
backend/tests/apps/github/api/issue_test.py (1)

1-39: Disabled DRF IssueSerializer tests without coverage replacements
The entire TestIssueSerializer class is commented out, leaving no tests for the new Pydantic IssueSchema or the Ninja get_issue endpoint. This creates a significant test coverage gap for your GitHub API. Please migrate or write new tests to validate schema serialization, input validation, and endpoint behavior.

backend/tests/apps/github/api/label_test.py (1)

1-29: All DRF LabelSerializer tests commented out
Disabling TestLabelSerializer removes validation checks for the new LabelSchema and its corresponding Ninja route. Introduce tests verifying correct schema parsing, field constraints, and endpoint responses to maintain coverage.

backend/tests/apps/owasp/api/event_test.py (1)

1-32: Event serializer tests fully disabled
The DRF-based EventSerializer tests have been commented out, with no new tests for EventSchema or the list_events Ninja endpoint. Please implement equivalent tests to ensure event data validation and response formatting are covered.

backend/tests/apps/owasp/api/committee_test.py (1)

1-34: Committee serializer tests removed without replacements
Disabling tests for CommitteeSerializer leaves the OWASP API’s validation untested. Add new unit tests for the Pydantic CommitteeSchema and its Ninja router to preserve test quality and coverage.

backend/tests/apps/owasp/api/project_test.py (1)

1-36: Address the loss of test coverage from commenting out all tests.

While commenting out DRF-based tests during the Django Ninja migration is understandable, this creates a significant gap in test coverage for the Project API functionality.

The current approach leaves the new Django Ninja implementation completely untested, which poses risks for:

  • API functionality regressions
  • Data validation issues
  • Serialization/deserialization problems

Consider implementing equivalent tests for the new Pydantic schemas and Ninja endpoints rather than leaving tests commented out indefinitely.

Would you like me to help generate test templates for the new Django Ninja Project API implementation?

♻️ Duplicate comments (2)
backend/tests/apps/github/api/organization_test.py (1)

1-55: Critical test coverage gap for Organization API.

This test file contained comprehensive validation including parameterized serialization tests and mocked database operations. Commenting out all tests creates a significant risk for the Organization API functionality.

The original tests covered important scenarios like the get_logins() method and serialization validation that should be preserved in the new Django Ninja implementation.

backend/tests/apps/github/api/repository_test.py (1)

1-36: Test coverage loss continues the concerning pattern.

Like other test files in this migration, all repository API tests have been commented out, eliminating validation for date field serialization and other critical functionality.

🧹 Nitpick comments (11)
backend/tests/apps/owasp/api/urls_test.py (1)

1-32: Address the test coverage gap after DRF to Ninja migration.

While commenting out DRF-specific tests during migration is understandable, this leaves the OWASP API routes without test coverage. The disabled tests were validating important routing behaviors that should still be tested in the Ninja implementation.

Consider creating equivalent tests for the new Ninja router setup. The tests should verify:

  • Route registration and availability
  • URL path patterns
  • Endpoint functionality

Would you like me to help generate new test cases for the Django Ninja implementation?

backend/tests/apps/github/api/urls_test.py (1)

1-37: Restore test coverage for GitHub API routes after Ninja migration.

The commented-out tests covered 6 critical GitHub API endpoints (issues, labels, organizations, releases, repositories, users). This creates a significant gap in route validation testing.

The new Ninja router implementation needs equivalent test coverage to ensure:

  • All API endpoints are properly registered
  • URL patterns are correct
  • Route accessibility and functionality

Would you like me to help create new test cases compatible with the Django Ninja architecture?

backend/tests/apps/github/api/release_test.py (1)

1-38: Adapt serialization tests for Pydantic schemas.

The commented-out tests provided valuable validation coverage for release data serialization, including date formatting logic. While the migration to Django Ninja uses Pydantic instead of DRF serializers, similar validation testing is still needed.

The parameterized test data can be reused to test the new ReleaseSchema Pydantic model. Consider creating tests that verify:

  • Schema validation with valid/invalid data
  • Date field parsing and formatting
  • Field requirement enforcement

Would you like me to help convert these tests to work with the new Pydantic implementation?

backend/tests/apps/owasp/api/chapter_test.py (1)

1-36: Replace serializer tests with Pydantic schema validation tests.

The commented-out test provided important validation coverage for chapter data serialization. The migration to Django Ninja with Pydantic schemas still requires equivalent validation testing.

The test data and validation scenarios can be adapted to test the new ChapterSchema. Consider creating tests that verify:

  • Pydantic model validation with the provided test data
  • Field validation rules (required fields, data types)
  • Schema serialization/deserialization

Would you like me to help create new tests for the Pydantic schema implementation?

backend/tests/apps/github/api/user_test.py (1)

1-64: Address the test coverage gap from commenting out DRF tests.

While commenting out the DRF-based tests is appropriate during the migration to Django Ninja, this creates a significant test coverage gap. The underlying functionality for user serialization and the is_indexable logic still needs testing with the new Pydantic schema implementation.

I can help generate equivalent tests for the new Django Ninja + Pydantic implementation. Would you like me to create new test cases that validate the UserSchema serialization and the user indexability logic?

backend/apps/owasp/api/urls.py (1)

1-1: Fix the misleading docstring.

The docstring incorrectly mentions "GitHub API URLs" but this file handles OWASP API URLs.

-"""GitHub API URLs."""
+"""OWASP API URLs."""
backend/apps/owasp/api/committee.py (1)

25-28: Consider adding pagination for scalability.

The endpoint returns all committees without pagination, which could become a performance concern as the dataset grows.

Consider adding pagination support:

+from ninja import Query
+from ninja.pagination import paginate

-@router.get("/", response=list[CommitteeSchema])
-def list_committees(request: HttpRequest) -> list[CommitteeSchema]:
+@router.get("/", response=list[CommitteeSchema])
+@paginate
+def list_committees(request: HttpRequest) -> list[CommitteeSchema]:
     """Get all committees."""
     return Committee.objects.all()
backend/apps/owasp/api/project.py (1)

26-29: Consider adding pagination and error handling for production readiness.

The endpoint implementation is functionally correct but may benefit from pagination for large datasets and error handling.

Consider enhancing the endpoint:

@router.get("/", response=list[ProjectSchema])
-def list_projects(request: HttpRequest) -> list[ProjectSchema]:
+def list_projects(request: HttpRequest, skip: int = 0, limit: int = 100) -> list[ProjectSchema]:
    """Get all projects."""
-    return Project.objects.all()
+    try:
+        return list(Project.objects.all()[skip:skip + limit])
+    except Exception as e:
+        # Log error and handle appropriately
+        raise
backend/apps/github/api/release.py (1)

27-27: Consider renaming function for clarity.

The function name get_release (singular) is misleading since it returns all releases. Consider renaming for consistency with the project API.

-def get_release(request: HttpRequest) -> list[ReleaseSchema]:
+def list_releases(request: HttpRequest) -> list[ReleaseSchema]:
backend/apps/github/api/repository.py (2)

26-26: Function naming inconsistency.

Similar to the release API, the function name get_repository (singular) doesn't match the fact that it returns all repositories.

-def get_repository(request: HttpRequest) -> list[RepositorySchema]:
+def list_repositories(request: HttpRequest) -> list[RepositorySchema]:

25-28: Consider architectural consistency across the migration.

All three migrated APIs follow the same pattern but have inconsistent function naming (list_projects vs get_release vs get_repository). Consider standardizing the naming convention across all endpoints.

For consistency across the codebase, consider establishing a naming convention like:

  • list_{resource}s for endpoints returning multiple items
  • get_{resource} for endpoints returning single items
  • create_{resource}, update_{resource}, delete_{resource} for other operations
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ff8f279 and 51248a0.

⛔ Files ignored due to path filters (1)
  • backend/poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (28)
  • backend/apps/github/api/issue.py (1 hunks)
  • backend/apps/github/api/label.py (1 hunks)
  • backend/apps/github/api/organization.py (1 hunks)
  • backend/apps/github/api/release.py (1 hunks)
  • backend/apps/github/api/repository.py (1 hunks)
  • backend/apps/github/api/urls.py (1 hunks)
  • backend/apps/github/api/user.py (1 hunks)
  • backend/apps/owasp/api/chapter.py (1 hunks)
  • backend/apps/owasp/api/committee.py (1 hunks)
  • backend/apps/owasp/api/event.py (1 hunks)
  • backend/apps/owasp/api/project.py (1 hunks)
  • backend/apps/owasp/api/urls.py (1 hunks)
  • backend/apps/slack/apps.py (2 hunks)
  • backend/apps/slack/events/member_joined_channel/catch_all.py (1 hunks)
  • backend/pyproject.toml (2 hunks)
  • backend/settings/urls.py (2 hunks)
  • backend/tests/apps/github/api/issue_test.py (1 hunks)
  • backend/tests/apps/github/api/label_test.py (1 hunks)
  • backend/tests/apps/github/api/organization_test.py (1 hunks)
  • backend/tests/apps/github/api/release_test.py (1 hunks)
  • backend/tests/apps/github/api/repository_test.py (1 hunks)
  • backend/tests/apps/github/api/urls_test.py (1 hunks)
  • backend/tests/apps/github/api/user_test.py (1 hunks)
  • backend/tests/apps/owasp/api/chapter_test.py (1 hunks)
  • backend/tests/apps/owasp/api/committee_test.py (1 hunks)
  • backend/tests/apps/owasp/api/event_test.py (1 hunks)
  • backend/tests/apps/owasp/api/project_test.py (1 hunks)
  • backend/tests/apps/owasp/api/urls_test.py (1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/owasp/api/event.py

[refactor] 12-12: Too few public methods (0/2)

(R0903)

backend/apps/github/api/label.py

[refactor] 12-12: Too few public methods (0/2)

(R0903)

backend/apps/owasp/api/committee.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

backend/apps/github/api/issue.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

backend/apps/github/api/organization.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

backend/apps/owasp/api/chapter.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

backend/apps/github/api/user.py

[refactor] 15-15: Too few public methods (0/2)

(R0903)

backend/apps/owasp/api/project.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

backend/apps/github/api/release.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

backend/apps/github/api/repository.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Run frontend e2e tests
  • GitHub Check: Run backend tests
  • GitHub Check: Run frontend unit tests
  • GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (28)
backend/pyproject.toml (2)

30-30: New django-ninja dependency added as expected
Adding django-ninja = "^1.4.3" correctly supports the migration from DRF to Ninja. Ensure that all newly refactored API modules import and utilize Ninja’s Router and Pydantic schemas accordingly.


123-123: Confirm broad suppression of unused-argument warnings
Ignoring ARG001 globally hides unused-argument errors across the codebase. Verify that this won’t mask legitimate issues in other modules and that all dead arguments are cleaned up where possible.

backend/apps/slack/apps.py (1)

28-28: LGTM! Clean linting improvement.

The removal of inline # noqa: ARG001 comments aligns with the global linting configuration changes. The function signatures and logic remain unchanged.

Also applies to: 42-42

backend/apps/slack/events/member_joined_channel/catch_all.py (1)

9-9: Verify removal of linter suppression doesn't introduce failures.

The # noqa: ARG001 comment was removed, but the event and client parameters appear to be unused in the function body. This may cause linting failures if ARG001 (unused function arguments) warnings are enabled.

Run the following script to check if this change introduces linting issues:

#!/bin/bash
# Description: Check for unused argument warnings in the modified file
# Expected: Verify if ARG001 warnings are triggered for unused parameters

# Check if ruff is being used and what rules are enabled
if [ -f "pyproject.toml" ]; then
    echo "=== Checking ruff configuration ==="
    grep -A 10 -B 5 "ARG001\|unused.*arg" pyproject.toml || echo "No ARG001 configuration found"
fi

if [ -f ".flake8" ]; then
    echo "=== Checking flake8 configuration ==="
    grep -A 5 -B 5 "ARG001\|unused.*arg" .flake8 || echo "No ARG001 configuration found"
fi

# Run linting on the specific file
echo "=== Running linter on the modified file ==="
if command -v ruff &> /dev/null; then
    ruff check backend/apps/slack/events/member_joined_channel/catch_all.py
elif command -v flake8 &> /dev/null; then
    flake8 backend/apps/slack/events/member_joined_channel/catch_all.py
else
    echo "No compatible linter found (ruff/flake8)"
fi
backend/apps/github/api/urls.py (1)

3-20: Excellent Django Ninja routing implementation.

The migration from DRF's SimpleRouter to Django Ninja's Router is well-executed:

  • Clean composition pattern using individual resource routers
  • Logical URL structure with appropriate nesting
  • Proper separation of concerns with modular router imports
  • Follows Django Ninja best practices for router organization

This demonstrates a thoughtful approach to the framework migration.

backend/apps/owasp/api/urls.py (1)

3-16: LGTM! Clean migration to Ninja router composition.

The migration from DRF's SimpleRouter to Ninja's modular router composition is well-executed. The approach of importing individual routers and composing them with add_router provides good separation of concerns and maintainability.

backend/settings/urls.py (1)

10-30: LGTM! Proper migration to Django Ninja API.

The migration from DRF's DefaultRouter to Django Ninja's NinjaAPI is correctly implemented. The modular composition of github_router and owasp_router under their respective prefixes maintains the API structure while leveraging Ninja's capabilities.

backend/apps/owasp/api/committee.py (1)

14-23: LGTM! Proper Pydantic schema definition.

The CommitteeSchema is correctly configured with from_attributes=True for ORM integration and includes all necessary fields with proper type annotations.

Note: The static analysis warning about "too few public methods" is a false positive - Pydantic models are data containers, not traditional classes requiring multiple methods.

backend/apps/github/api/label.py (1)

12-20: LGTM! Correct Pydantic schema implementation.

The LabelSchema is properly configured for ORM integration with appropriate field definitions.

The static analysis warning about "too few public methods" is a false positive for Pydantic models.

backend/apps/owasp/api/chapter.py (3)

3-7: LGTM: Clean migration to Django Ninja.

The imports are correctly updated to use Django Ninja and Pydantic instead of DRF components.


14-23: LGTM: Well-structured Pydantic schema.

The ChapterSchema properly defines all necessary fields with appropriate types and configures from_attributes=True for ORM integration.


26-29: LGTM: Proper Ninja router implementation.

The endpoint correctly uses the router decorator with proper response typing and returns the QuerySet directly for automatic serialization.

backend/apps/github/api/user.py (1)

15-34: LGTM: Comprehensive user schema.

The UserSchema includes all relevant user fields with proper typing and ORM configuration.

backend/apps/github/api/issue.py (3)

3-7: LGTM: Clean imports for Django Ninja.

The imports correctly use Django Ninja and Pydantic components without any DRF dependencies.


14-24: LGTM: Appropriate schema for Issue model.

The IssueSchema includes relevant fields for issue tracking with proper types and ORM configuration.


27-30: LGTM: Straightforward endpoint implementation.

The endpoint correctly implements the GET route with proper typing and direct QuerySet return.

backend/apps/owasp/api/event.py (3)

3-5: LGTM: Clean imports for Django Ninja.

The imports are correctly updated to use Django Ninja and Pydantic instead of DRF.


22-25: LGTM: Proper endpoint implementation.

The endpoint correctly uses the router decorator and returns the QuerySet for automatic serialization.


12-19: ```shell
#!/bin/bash

Show beginning of Event model code

sed -n '1,50p' backend/apps/owasp/models/event.py


</details>
<details>
<summary>backend/apps/github/api/organization.py (3)</summary>

`3-7`: **LGTM: Clean migration imports.**

The imports are correctly updated to use Django Ninja and Pydantic components.

---

`14-24`: **LGTM: Complete organization schema.**

The OrganizationSchema includes all relevant organization fields with proper typing and ORM configuration.

---

`27-30`: **LGTM: Business logic preserved in migration.**

The endpoint correctly maintains the existing business logic of filtering for OWASP organizations while migrating to Django Ninja.

</details>
<details>
<summary>backend/apps/owasp/api/project.py (2)</summary>

`3-7`: **Clean migration to Django Ninja imports.**

The import changes correctly replace DRF dependencies with Django Ninja and Pydantic equivalents.

---

`14-23`: **Well-structured Pydantic schema definition.**

The `ProjectSchema` correctly uses `BaseModel` with `from_attributes` configuration for Django model serialization. The field types are appropriate for the Project model.

</details>
<details>
<summary>backend/apps/github/api/release.py (2)</summary>

`3-7`: **Consistent migration to Django Ninja.**

The import structure matches the migration pattern and correctly replaces DRF dependencies.

---

`14-23`: **Comprehensive schema for Release model.**

The `ReleaseSchema` includes all relevant fields with proper typing, including `published_at` and `tag_name` specific to releases.

</details>
<details>
<summary>backend/apps/github/api/repository.py (2)</summary>

`3-7`: **Proper Django Ninja migration setup.**

The imports correctly establish the Django Ninja framework dependencies and remove DRF references.

---

`14-22`: **Appropriate schema for Repository model.**

The `RepositorySchema` covers the essential repository fields with correct types and Pydantic configuration.

</details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (4)
backend/apps/github/api/user.py (1)

43-49: Fix type annotation inconsistency and error handling approach.

The function signature indicates it returns UserSchema | None but the except block returns a Response object, creating a type annotation mismatch. Additionally, returning status 200 for "User not found" is semantically incorrect - it should be 404.

This issue was previously identified in past review comments. The recommended approach is to use Ninja's error handling mechanism.

backend/apps/owasp/api/committee.py (1)

25-28: Same pagination concern as other endpoints.

Consider implementing pagination for consistency and performance, similar to the chapter endpoint.

backend/apps/owasp/api/event.py (1)

22-25: Consider pagination for events endpoint.

Events could potentially be numerous, so pagination would be beneficial here as well.

backend/apps/owasp/api/project.py (1)

26-29: Add pagination for projects endpoint.

Projects are likely to be numerous, making pagination essential for performance.

🧹 Nitpick comments (3)
backend/apps/owasp/api/chapter.py (1)

14-23: Consider field ordering and potential performance implications.

The ChapterSchema is well-implemented with proper Pydantic configuration. However, consider:

  1. Field ordering: Group related fields together (e.g., timestamps at the end)
  2. Performance concern: The endpoint returns all chapters without pagination
 class ChapterSchema(BaseModel):
     """Schema for Chapter."""
 
     model_config = {"from_attributes": True}
 
+    name: str
     country: str
-    created_at: datetime
-    name: str
     region: str
+    created_at: datetime
-    updated_at: datetime
+    updated_at: datetime
backend/apps/owasp/api/event.py (1)

21-21: Remove extra blank line for consistency.

-
-
+
 @router.get("/", response=list[EventSchema])
backend/apps/owasp/api/project.py (1)

14-23: Improve field ordering for better readability.

Consider grouping business fields together and placing timestamps at the end.

 class ProjectSchema(BaseModel):
     """Schema for Project."""
 
     model_config = {"from_attributes": True}
 
-    created_at: datetime
+    name: str
     description: str
     level: str
-    name: str
+    created_at: datetime
     updated_at: datetime
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2cd9a71 and bc4f8b5.

📒 Files selected for processing (5)
  • backend/apps/github/api/user.py (1 hunks)
  • backend/apps/owasp/api/chapter.py (1 hunks)
  • backend/apps/owasp/api/committee.py (1 hunks)
  • backend/apps/owasp/api/event.py (1 hunks)
  • backend/apps/owasp/api/project.py (1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/github/api/user.py

[refactor] 15-15: Too few public methods (0/2)

(R0903)

backend/apps/owasp/api/chapter.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

backend/apps/owasp/api/committee.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

backend/apps/owasp/api/event.py

[refactor] 12-12: Too few public methods (0/2)

(R0903)

backend/apps/owasp/api/project.py

[refactor] 14-14: Too few public methods (0/2)

(R0903)

⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Run frontend e2e tests
  • GitHub Check: Run backend tests
  • GitHub Check: Run frontend unit tests
  • GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (10)
backend/apps/github/api/user.py (4)

3-8: LGTM! Imports correctly migrated to Django Ninja.

The imports have been properly updated for the Django Ninja migration, using Ninja's Router and Response instead of DRF equivalents.


15-34: Well-implemented Pydantic schema for Django Ninja migration.

The UserSchema properly replaces the DRF serializer with comprehensive field definitions and correct ORM integration via from_attributes. The static analysis hint about "too few public methods" is a false positive for Pydantic models.


37-40: Clean function-based endpoint implementation.

The get_user endpoint is well-implemented with proper type hints and response schema for the Django Ninja migration.


12-12: Standard Ninja router setup.

Router instantiation follows Django Ninja conventions correctly.

backend/apps/owasp/api/chapter.py (1)

3-7: Excellent migration to Django Ninja with proper imports.

The import structure correctly brings in all necessary components for the Django Ninja + Pydantic implementation.

backend/apps/owasp/api/committee.py (2)

3-7: Consistent import pattern with Django Ninja migration.

The imports align well with the migration strategy across all OWASP API modules.


14-22: Good Pydantic schema implementation with logical field ordering.

The field ordering (name, description, then timestamps) is more logical than in other schemas.

backend/apps/owasp/api/event.py (2)

3-5: Correct import selection for schema requirements.

Good choice to omit the datetime import since EventSchema doesn't include timestamp fields.


12-19: Clean schema implementation with relevant fields only.

The EventSchema appropriately includes only the essential fields without unnecessary timestamps.

backend/apps/owasp/api/project.py (1)

3-7: Complete import set for project schema with timestamps.

All necessary imports are properly included for the ProjectSchema implementation.

Comment on lines +26 to +29
@router.get("/", response=list[ChapterSchema])
def get_chapters(request: HttpRequest) -> list[ChapterSchema]:
"""Get all chapters."""
return Chapter.objects.all()
Copy link
Contributor

@coderabbitai coderabbitai bot Jun 22, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add pagination to prevent performance issues with large datasets.

Returning all chapters could cause performance problems as the dataset grows. Consider implementing pagination.

-@router.get("/", response=list[ChapterSchema])
-def get_chapters(request: HttpRequest) -> list[ChapterSchema]:
+from ninja.pagination import paginate, PageNumberPagination
+
+@router.get("/", response=list[ChapterSchema])
+@paginate(PageNumberPagination)
+def get_chapters(request: HttpRequest):
     """Get all chapters."""
     return Chapter.objects.all()
📝 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
@router.get("/", response=list[ChapterSchema])
def get_chapters(request: HttpRequest) -> list[ChapterSchema]:
"""Get all chapters."""
return Chapter.objects.all()
from ninja.pagination import paginate, PageNumberPagination
@router.get("/", response=list[ChapterSchema])
@paginate(PageNumberPagination)
def get_chapters(request: HttpRequest):
"""Get all chapters."""
return Chapter.objects.all()
🤖 Prompt for AI Agents
In backend/apps/owasp/api/chapter.py around lines 26 to 29, the get_chapters
function returns all Chapter objects without pagination, which can cause
performance issues with large datasets. Modify the function to implement
pagination by accepting query parameters for page number and page size, then
return only the corresponding subset of Chapter objects. Use Django's pagination
utilities or manually slice the queryset accordingly to limit the number of
chapters returned per request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@arkid15r should we apply pagination ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@abhayymishraa abhayymishraa marked this pull request as ready for review June 22, 2025 06:00
@abhayymishraa abhayymishraa requested a review from arkid15r as a code owner June 22, 2025 06:00
Copy link

@arkid15r arkid15r requested a review from aramattamara June 22, 2025 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate REST API to Django ninja
1 participant