Add projects-by-org endpoint and pagination for organizations list#727
Merged
Add projects-by-org endpoint and pagination for organizations list#727
Conversation
- Add GET /projects/organization/{org_id} endpoint with org validation
- Add has_more pagination to organizations list endpoint
- Add Swagger docs for list_by_org
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded pagination metadata ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as API Router
participant Auth as Permission check
participant CRUD as Organization CRUD
participant DB as Database
Client->>Router: GET /projects/organization/{org_id}
Router->>Auth: enforce Permission.SUPERUSER
Auth-->>Router: allowed
Router->>CRUD: validate_organization(session, org_id)
CRUD->>DB: SELECT organization WHERE id=org_id
DB-->>CRUD: organization record (or none)
alt organization not found
CRUD-->>Router: HTTPException 404
Router-->>Client: 404 Not Found
else organization inactive
CRUD-->>Router: HTTPException 403
Router-->>Client: 403 Forbidden
else valid organization
Router->>DB: get_projects_by_organization(session, org_id)
DB-->>Router: list of projects
Router-->>Client: 200 OK with APIResponse(data=projects, metadata?)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/app/api/routes/organization.py (1)
30-34: Add return type hint to the function.The function is missing a return type annotation. As per coding guidelines, all function parameters and return values should have type hints.
Suggested fix
def read_organizations( session: SessionDep, skip: int = Query(0, ge=0), limit: int = Query(100, ge=1, le=100), -): +) -> APIResponse[List[OrganizationPublic]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/organization.py` around lines 30 - 34, Add an explicit return type annotation to the read_organizations function signature (def read_organizations(...)) — e.g. annotate it as returning a list of your organization response model like -> List[Organization] or -> list[OrganizationRead] (match your project's schema/model name), and add the necessary import from typing (List) or from your schemas/models module so the signature is fully typed.backend/app/api/routes/project.py (1)
126-129: Add return type hint to the function.The function is missing a return type annotation.
Suggested fix
-def read_projects_by_organization(session: SessionDep, org_id: int): +def read_projects_by_organization(session: SessionDep, org_id: int) -> APIResponse[List[ProjectPublic]]: validate_organization(session=session, org_id=org_id)As per coding guidelines: "Always add type hints to all function parameters and return values in Python code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/project.py` around lines 126 - 129, Add a return type annotation to the function read_projects_by_organization so it follows the codebase typing guidelines; update its signature to include the expected return type (the type returned by APIResponse.success_response, e.g., APIResponse or a more specific generic/DTO type if available) while keeping the existing parameters (session: SessionDep, org_id: int) unchanged and ensure any imports for that return type are added if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/routes/project.py`:
- Around line 126-129: Fix two issues: in validate_organization replace the
incorrect raise HTTPException("Organization is not active") with a proper
HTTPException call that supplies an integer status_code and a detail string
(e.g., raise HTTPException(status_code=503, detail="Organization is not
active")) so the exception is constructed correctly; then update the endpoint
signature for read_projects_by_organization to include the return type hint ->
APIResponse[List[ProjectPublic]] (and add any needed imports for List and
ProjectPublic) so the function is properly typed.
---
Nitpick comments:
In `@backend/app/api/routes/organization.py`:
- Around line 30-34: Add an explicit return type annotation to the
read_organizations function signature (def read_organizations(...)) — e.g.
annotate it as returning a list of your organization response model like ->
List[Organization] or -> list[OrganizationRead] (match your project's
schema/model name), and add the necessary import from typing (List) or from your
schemas/models module so the signature is fully typed.
In `@backend/app/api/routes/project.py`:
- Around line 126-129: Add a return type annotation to the function
read_projects_by_organization so it follows the codebase typing guidelines;
update its signature to include the expected return type (the type returned by
APIResponse.success_response, e.g., APIResponse or a more specific generic/DTO
type if available) while keeping the existing parameters (session: SessionDep,
org_id: int) unchanged and ensure any imports for that return type are added if
needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a558bf72-494b-40cd-bbd0-2cb53c220a22
📒 Files selected for processing (4)
backend/app/api/docs/organization/list.mdbackend/app/api/docs/projects/list_by_org.mdbackend/app/api/routes/organization.pybackend/app/api/routes/project.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…organizations; update read_projects_by_organization response type
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/crud/organization.py (1)
51-55:⚠️ Potential issue | 🟠 MajorUse a semantically correct HTTP status for inactive organizations.
Line 55 uses
503 Service Unavailablefor an inactive organization, but 503 indicates transient infrastructure failure and may trigger client-side retries. An inactive organization is a business-state denial—use403 Forbiddeninstead.Suggested change
- raise HTTPException(status_code=503, detail="Organization is not active") + raise HTTPException(status_code=403, detail="Organization is not active")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/organization.py` around lines 51 - 55, Change the HTTP response for inactive organizations from a transient infrastructure code to a business-state denial: in the validate_organization flow (the block that checks organization.is_active, e.g., in validate_organization or where org_id is logged) replace the HTTPException status_code 503 with 403 (Forbidden) so clients receive the correct semantic status for an inactive organization; keep the existing log message but ensure the raised HTTPException uses status_code=403 and an appropriate detail string like "Organization is not active".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/app/crud/organization.py`:
- Around line 51-55: Change the HTTP response for inactive organizations from a
transient infrastructure code to a business-state denial: in the
validate_organization flow (the block that checks organization.is_active, e.g.,
in validate_organization or where org_id is logged) replace the HTTPException
status_code 503 with 403 (Forbidden) so clients receive the correct semantic
status for an inactive organization; keep the existing log message but ensure
the raised HTTPException uses status_code=403 and an appropriate detail string
like "Organization is not active".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5490e757-e95f-447c-b44d-26cc9d0873c8
📒 Files selected for processing (2)
backend/app/api/routes/project.pybackend/app/crud/organization.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/routes/project.py
Ayush8923
assigned
Ayush8923,
Prajna1999 and
vprashrex and unassigned
Ayush8923 and
Prajna1999
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_project.py (1)
100-142: Add a permission-denial test for this new route.Since this endpoint is protected, include at least one
401/403test (e.g., no token or non-superuser token) to prevent permission regressions.Based on learnings: Implement organization and project-level permissions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/api/routes/test_project.py` around lines 100 - 142, Add a permission-denial test for the protected route by creating a test function (e.g., test_read_projects_by_organization_unauthorized) that calls client.get(f"{settings.API_V1_STR}/projects/organization/{project.organization_id}") without auth headers to assert a 401, and another variant that uses a non-superuser token fixture (e.g., non_superuser_token_headers) to assert a 403; reuse create_test_project(db) to create the project/organization and reference client and settings.API_V1_STR so the test verifies both unauthenticated and insufficient-permission responses for the endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/tests/api/routes/test_project.py`:
- Around line 118-125: In test_read_projects_by_organization_not_found, avoid
the hardcoded 999999 and the unused db param by computing a
guaranteed-nonexistent organization ID from the database (e.g., use the db
Session to query the maximum Organization.id and set non_existent_id = (max_id
or 0) + 1), then call
client.get(f"{settings.API_V1_STR}/projects/organization/{non_existent_id}",
headers=superuser_token_headers) and assert 404; update the function body of
test_read_projects_by_organization_not_found to use this computed id so the db
parameter is used and the test is deterministic.
In `@backend/app/tests/crud/test_org.py`:
- Around line 47-63: The tests test_validate_organization_not_found and
test_validate_organization_inactive currently only match exception text; update
them to also assert the HTTPException.status_code to prevent silent behavior
drift: use pytest.raises as a context manager to capture the exception from
validate_organization and add an assertion that excinfo.value.status_code == 404
for the "Organization not found" case and == 503 for the "Organization is not
active" case, referring to validate_organization and HTTPException to locate the
code.
---
Nitpick comments:
In `@backend/app/tests/api/routes/test_project.py`:
- Around line 100-142: Add a permission-denial test for the protected route by
creating a test function (e.g., test_read_projects_by_organization_unauthorized)
that calls
client.get(f"{settings.API_V1_STR}/projects/organization/{project.organization_id}")
without auth headers to assert a 401, and another variant that uses a
non-superuser token fixture (e.g., non_superuser_token_headers) to assert a 403;
reuse create_test_project(db) to create the project/organization and reference
client and settings.API_V1_STR so the test verifies both unauthenticated and
insufficient-permission responses for the endpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d06caaa2-0482-4814-82f9-c8a517311875
📒 Files selected for processing (3)
backend/app/tests/api/routes/test_org.pybackend/app/tests/api/routes/test_project.pybackend/app/tests/crud/test_org.py
Comment on lines
+118
to
+125
| def test_read_projects_by_organization_not_found( | ||
| db: Session, superuser_token_headers: dict[str, str] | ||
| ) -> None: | ||
| response = client.get( | ||
| f"{settings.API_V1_STR}/projects/organization/999999", | ||
| headers=superuser_token_headers, | ||
| ) | ||
| assert response.status_code == 404 |
There was a problem hiding this comment.
Replace hardcoded missing-org ID and fix unused db (Line 119).
Using 999999 is brittle; generate a guaranteed non-existent ID from the DB so the test stays deterministic and resolves Ruff ARG001.
Proposed fix
+from app.tests.utils.utils import get_non_existent_id
@@
def test_read_projects_by_organization_not_found(
db: Session, superuser_token_headers: dict[str, str]
) -> None:
+ non_existent_org_id = get_non_existent_id(db, Organization)
response = client.get(
- f"{settings.API_V1_STR}/projects/organization/999999",
+ f"{settings.API_V1_STR}/projects/organization/{non_existent_org_id}",
headers=superuser_token_headers,
)
assert response.status_code == 404🧰 Tools
🪛 Ruff (0.15.7)
[warning] 119-119: Unused function argument: db
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/tests/api/routes/test_project.py` around lines 118 - 125, In
test_read_projects_by_organization_not_found, avoid the hardcoded 999999 and the
unused db param by computing a guaranteed-nonexistent organization ID from the
database (e.g., use the db Session to query the maximum Organization.id and set
non_existent_id = (max_id or 0) + 1), then call
client.get(f"{settings.API_V1_STR}/projects/organization/{non_existent_id}",
headers=superuser_token_headers) and assert 404; update the function body of
test_read_projects_by_organization_not_found to use this computed id so the db
parameter is used and the test is deterministic.
Comment on lines
+47
to
+63
| def test_validate_organization_not_found(db: Session) -> None: | ||
| """Test that validation fails when organization does not exist.""" | ||
| non_existent_org_id = get_non_existent_id(db, Organization) | ||
| with pytest.raises(HTTPException, match="Organization not found"): | ||
| validate_organization(session=db, org_id=non_existent_org_id) | ||
|
|
||
|
|
||
| def test_validate_organization_inactive(db: Session) -> None: | ||
| """Test that validation fails when organization is inactive.""" | ||
| organization = create_test_organization(db) | ||
| organization.is_active = False | ||
| db.add(organization) | ||
| db.commit() | ||
| db.refresh(organization) | ||
|
|
||
| with pytest.raises(HTTPException, match="Organization is not active"): | ||
| validate_organization(session=db, org_id=organization.id) |
There was a problem hiding this comment.
Assert HTTP status codes, not only error text.
These tests currently pass on message match alone; they should also lock in 404 and 503 to prevent silent behavior drift.
Proposed test hardening
def test_validate_organization_not_found(db: Session) -> None:
"""Test that validation fails when organization does not exist."""
non_existent_org_id = get_non_existent_id(db, Organization)
- with pytest.raises(HTTPException, match="Organization not found"):
+ with pytest.raises(HTTPException) as exc_info:
validate_organization(session=db, org_id=non_existent_org_id)
+ assert exc_info.value.status_code == 404
+ assert exc_info.value.detail == "Organization not found"
@@
def test_validate_organization_inactive(db: Session) -> None:
@@
- with pytest.raises(HTTPException, match="Organization is not active"):
+ with pytest.raises(HTTPException) as exc_info:
validate_organization(session=db, org_id=organization.id)
+ assert exc_info.value.status_code == 503
+ assert exc_info.value.detail == "Organization is not active"📝 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
| def test_validate_organization_not_found(db: Session) -> None: | |
| """Test that validation fails when organization does not exist.""" | |
| non_existent_org_id = get_non_existent_id(db, Organization) | |
| with pytest.raises(HTTPException, match="Organization not found"): | |
| validate_organization(session=db, org_id=non_existent_org_id) | |
| def test_validate_organization_inactive(db: Session) -> None: | |
| """Test that validation fails when organization is inactive.""" | |
| organization = create_test_organization(db) | |
| organization.is_active = False | |
| db.add(organization) | |
| db.commit() | |
| db.refresh(organization) | |
| with pytest.raises(HTTPException, match="Organization is not active"): | |
| validate_organization(session=db, org_id=organization.id) | |
| def test_validate_organization_not_found(db: Session) -> None: | |
| """Test that validation fails when organization does not exist.""" | |
| non_existent_org_id = get_non_existent_id(db, Organization) | |
| with pytest.raises(HTTPException) as exc_info: | |
| validate_organization(session=db, org_id=non_existent_org_id) | |
| assert exc_info.value.status_code == 404 | |
| assert exc_info.value.detail == "Organization not found" | |
| def test_validate_organization_inactive(db: Session) -> None: | |
| """Test that validation fails when organization is inactive.""" | |
| organization = create_test_organization(db) | |
| organization.is_active = False | |
| db.add(organization) | |
| db.commit() | |
| db.refresh(organization) | |
| with pytest.raises(HTTPException) as exc_info: | |
| validate_organization(session=db, org_id=organization.id) | |
| assert exc_info.value.status_code == 503 | |
| assert exc_info.value.detail == "Organization is not active" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/tests/crud/test_org.py` around lines 47 - 63, The tests
test_validate_organization_not_found and test_validate_organization_inactive
currently only match exception text; update them to also assert the
HTTPException.status_code to prevent silent behavior drift: use pytest.raises as
a context manager to capture the exception from validate_organization and add an
assertion that excinfo.value.status_code == 404 for the "Organization not found"
case and == 503 for the "Organization is not active" case, referring to
validate_organization and HTTPException to locate the code.
Ayush8923
approved these changes
Apr 1, 2026
nishika26
requested changes
Apr 1, 2026
|
|
||
| return APIResponse.success_response(organizations) | ||
| has_more = (skip + limit) < count | ||
| return APIResponse.success_response(organizations, metadata={"has_more": has_more}) |
Collaborator
There was a problem hiding this comment.
this kind of support is not there in read projects endpoint as well so why are we only adding it to this one
Collaborator
Author
There was a problem hiding this comment.
since this is requried for frontend that's why
Collaborator
There was a problem hiding this comment.
but how do you know that we wont require this for project in future
Collaborator
Author
There was a problem hiding this comment.
but do u think organization would have lot of projects? .. for consistency I added the has_more metadata in projects endpoint as well
AkhileshNegi
approved these changes
Apr 1, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/app/tests/api/routes/test_project.py (1)
117-124:⚠️ Potential issue | 🟡 MinorReplace hardcoded org ID and use
dbparameter in not-found test.At Line 121,
999999is brittle and Line 118 leavesdbunused (Ruff ARG001). Prefer generating a guaranteed non-existent organization ID from DB state so the test remains deterministic.Proposed fix
+from app.models import Organization +from app.tests.utils.utils import get_non_existent_id @@ def test_read_projects_by_organization_not_found( db: Session, superuser_token_headers: dict[str, str] ) -> None: + non_existent_org_id = get_non_existent_id(db, Organization) response = client.get( - f"{settings.API_V1_STR}/projects/organization/999999", + f"{settings.API_V1_STR}/projects/organization/{non_existent_org_id}", headers=superuser_token_headers, ) assert response.status_code == 404🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/api/routes/test_project.py` around lines 117 - 124, In test_read_projects_by_organization_not_found replace the hardcoded 999999 and the unused db param by deriving a guaranteed non-existent organization id from the current DB state: query the Organization id max via the test DB session (db), compute non_existing_id = max_id + 1 (treat missing rows as 0), then use that id in the GET URL and remove the unused hardcoded literal so db is actually used; locate this logic in the test_read_projects_by_organization_not_found function and update imports if needed to reference the Organization model or its id column.
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_project.py (1)
99-141: Add an authorization regression test for non-superuser access.The endpoint is security-sensitive and currently only exercised with
superuser_token_headers. Add a test that a non-superuser token is denied (expected403) to prevent permission regressions.Based on learnings: "Implement organization and project-level permissions"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/api/routes/test_project.py` around lines 99 - 141, Add a new regression test verifying non-superuser access is denied: create a test named e.g. test_read_projects_by_organization_non_superuser_denied that mirrors test_read_projects_by_organization but uses a non-superuser token header (e.g., normal_user_token_headers) and calls the same endpoint for project.organization_id (or an organization created via create_test_organization/create_test_project), then assert response.status_code == 403; place it next to the existing tests (test_read_projects_by_organization, test_read_projects_by_organization_not_found, test_read_projects_by_inactive_organization) so permissions regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/app/tests/api/routes/test_project.py`:
- Around line 117-124: In test_read_projects_by_organization_not_found replace
the hardcoded 999999 and the unused db param by deriving a guaranteed
non-existent organization id from the current DB state: query the Organization
id max via the test DB session (db), compute non_existing_id = max_id + 1 (treat
missing rows as 0), then use that id in the GET URL and remove the unused
hardcoded literal so db is actually used; locate this logic in the
test_read_projects_by_organization_not_found function and update imports if
needed to reference the Organization model or its id column.
---
Nitpick comments:
In `@backend/app/tests/api/routes/test_project.py`:
- Around line 99-141: Add a new regression test verifying non-superuser access
is denied: create a test named e.g.
test_read_projects_by_organization_non_superuser_denied that mirrors
test_read_projects_by_organization but uses a non-superuser token header (e.g.,
normal_user_token_headers) and calls the same endpoint for
project.organization_id (or an organization created via
create_test_organization/create_test_project), then assert response.status_code
== 403; place it next to the existing tests (test_read_projects_by_organization,
test_read_projects_by_organization_not_found,
test_read_projects_by_inactive_organization) so permissions regressions are
caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ac11f2a-002b-45c2-a983-27a18d9d1430
📒 Files selected for processing (3)
backend/app/api/routes/organization.pybackend/app/crud/organization.pybackend/app/tests/api/routes/test_project.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/crud/organization.py
- backend/app/api/routes/organization.py
nishika26
approved these changes
Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: #733
Summary
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests