mirror of
https://github.com/Significant-Gravitas/AutoGPT.git
synced 2026-04-08 03:00:28 -04:00
fix(backend): address PR review round 2 — TDD for 3 bugs + docstring fix
Bugs found by sentry, coderabbit, and Pwuts. Each had a failing xfail test written first, then fixed, then xfail removed. Fixes: 1. update_org_member: bare next() replaced with next(..., None) + NotFoundError — prevents StopIteration crash if member disappears concurrently (sentry) 2. add_workspace_member: now verifies workspace belongs to the claimed org — prevents adding members to workspaces in other orgs (coderabbit) 3. test_assign_seat: fixed to assert requested seat type not mock default (coderabbit) 4. Docstring format: RST 'Usage::' blocks replaced with plain 'Example::' to match codebase style (Pwuts) 3 new tests + 3 existing tests updated. Total: 127 route tests + 15 credit tests passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -265,11 +265,11 @@ async def get_request_context(
|
||||
def requires_org_permission(
|
||||
*actions: OrgAction,
|
||||
):
|
||||
"""
|
||||
Factory that returns a FastAPI dependency enforcing one or more org-level
|
||||
permissions. The request is allowed if the user holds **all** listed actions.
|
||||
"""Factory returning a FastAPI dependency that enforces org-level permissions.
|
||||
|
||||
Usage::
|
||||
The request is allowed only if the user holds **all** listed actions.
|
||||
|
||||
Example::
|
||||
|
||||
@router.delete("/org/{org_id}")
|
||||
async def delete_org(
|
||||
@@ -295,12 +295,12 @@ def requires_org_permission(
|
||||
def requires_workspace_permission(
|
||||
*actions: WorkspaceAction,
|
||||
):
|
||||
"""
|
||||
Factory that returns a FastAPI dependency enforcing one or more
|
||||
workspace-level permissions. The user must be in a workspace context
|
||||
(workspace_id is set) and hold **all** listed actions.
|
||||
"""Factory returning a FastAPI dependency that enforces workspace-level permissions.
|
||||
|
||||
Usage::
|
||||
The user must be in a workspace context (workspace_id is set) and
|
||||
hold **all** listed actions.
|
||||
|
||||
Example::
|
||||
|
||||
@router.post("/workspace/{ws_id}/agents")
|
||||
async def create_agent(
|
||||
|
||||
@@ -422,7 +422,10 @@ async def update_org_member(
|
||||
data=update_data,
|
||||
)
|
||||
members = await list_org_members(org_id)
|
||||
return next(m for m in members if m.user_id == user_id)
|
||||
match = next((m for m in members if m.user_id == user_id), None)
|
||||
if match is None:
|
||||
raise NotFoundError(f"Member {user_id} not found in org {org_id} after update")
|
||||
return match
|
||||
|
||||
|
||||
async def remove_org_member(org_id: str, user_id: str, requesting_user_id: str) -> None:
|
||||
|
||||
@@ -977,6 +977,10 @@ class TestWorkspaceDbMembers:
|
||||
async def test_add_workspace_member_requires_org_membership(self):
|
||||
from backend.api.features.orgs.workspace_db import add_workspace_member
|
||||
|
||||
# Workspace belongs to the org
|
||||
self.prisma.orgworkspace.find_unique = AsyncMock(
|
||||
return_value=_make_workspace(orgId=ORG_ID)
|
||||
)
|
||||
self.prisma.orgmember.find_unique = AsyncMock(return_value=None)
|
||||
|
||||
with pytest.raises(ValueError, match="not a member of the organization"):
|
||||
@@ -990,6 +994,10 @@ class TestWorkspaceDbMembers:
|
||||
async def test_add_workspace_member_success(self):
|
||||
from backend.api.features.orgs.workspace_db import add_workspace_member
|
||||
|
||||
# Workspace belongs to the org
|
||||
self.prisma.orgworkspace.find_unique = AsyncMock(
|
||||
return_value=_make_workspace(orgId=ORG_ID)
|
||||
)
|
||||
org_mem = _make_member(userId=OTHER_USER_ID)
|
||||
self.prisma.orgmember.find_unique = AsyncMock(return_value=org_mem)
|
||||
ws_mem = _make_ws_member(
|
||||
@@ -2490,3 +2498,75 @@ class TestPRReviewBugs:
|
||||
with pytest.raises(fastapi.HTTPException) as exc_info:
|
||||
await create_workspace(org_id="org-B", request=request, ctx=ctx)
|
||||
assert exc_info.value.status_code == 403
|
||||
|
||||
|
||||
class TestPRReviewBugsRound2:
|
||||
"""Second round of PR review bugs — TDD: xfail first, fix, remove xfail."""
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def setup(self, mocker):
|
||||
self.prisma = MagicMock()
|
||||
mocker.patch("backend.api.features.orgs.db.prisma", self.prisma)
|
||||
mocker.patch("backend.api.features.orgs.workspace_db.prisma", self.prisma)
|
||||
|
||||
# --- Bug: update_org_member bare next() raises StopIteration ---
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_org_member_missing_member_after_update_raises_not_found(self):
|
||||
"""If the member disappears between update and re-fetch, should raise
|
||||
NotFoundError not StopIteration."""
|
||||
from backend.api.features.orgs.db import update_org_member
|
||||
|
||||
member = _make_member(userId=OTHER_USER_ID, isOwner=False)
|
||||
self.prisma.orgmember.find_unique = AsyncMock(return_value=member)
|
||||
self.prisma.orgmember.update = AsyncMock()
|
||||
# list_org_members returns empty — member was deleted concurrently
|
||||
self.prisma.orgmember.find_many = AsyncMock(return_value=[])
|
||||
|
||||
with pytest.raises(NotFoundError):
|
||||
await update_org_member(ORG_ID, OTHER_USER_ID, is_admin=True, is_billing_manager=None)
|
||||
|
||||
# --- Bug: test_assign_seat asserts wrong seat type ---
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_assign_seat_returns_requested_type(self):
|
||||
"""assign_seat should return the seat type that was requested, not the mock default."""
|
||||
from backend.data.org_credit import assign_seat
|
||||
|
||||
# Mock returns the requested values
|
||||
mock_prisma = MagicMock()
|
||||
mock_prisma.organizationseatassignment.upsert = AsyncMock(
|
||||
return_value=MagicMock(userId="u1", seatType="PAID", status="ACTIVE")
|
||||
)
|
||||
|
||||
import backend.data.org_credit
|
||||
|
||||
original = backend.data.org_credit.prisma
|
||||
backend.data.org_credit.prisma = mock_prisma
|
||||
try:
|
||||
result = await assign_seat("org-1", "u1", seat_type="PAID")
|
||||
assert result["seatType"] == "PAID"
|
||||
finally:
|
||||
backend.data.org_credit.prisma = original
|
||||
|
||||
# --- Bug: add_workspace_member doesn't verify workspace belongs to org ---
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_add_workspace_member_verifies_workspace_in_org(self):
|
||||
"""add_workspace_member should verify the workspace actually belongs to
|
||||
the claimed org, not just that the user is in the org."""
|
||||
from backend.api.features.orgs.workspace_db import add_workspace_member
|
||||
|
||||
# User is an org member of org-A
|
||||
self.prisma.orgmember.find_unique = AsyncMock(
|
||||
return_value=_make_member(orgId="org-A")
|
||||
)
|
||||
# But the workspace belongs to org-B
|
||||
self.prisma.orgworkspace.find_unique = AsyncMock(
|
||||
return_value=_make_workspace(orgId="org-B")
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError, match="does not belong"):
|
||||
await add_workspace_member(
|
||||
ws_id=WS_ID, user_id=OTHER_USER_ID, org_id="org-A"
|
||||
)
|
||||
|
||||
@@ -160,7 +160,12 @@ async def add_workspace_member(
|
||||
is_billing_manager: bool = False,
|
||||
invited_by: str | None = None,
|
||||
) -> WorkspaceMemberResponse:
|
||||
"""Add a member to a workspace. Must be an org member."""
|
||||
"""Add a member to a workspace. Must be an org member, workspace must belong to org."""
|
||||
# Verify workspace belongs to the org
|
||||
ws = await prisma.orgworkspace.find_unique(where={"id": ws_id})
|
||||
if ws is None or ws.orgId != org_id:
|
||||
raise ValueError(f"Workspace {ws_id} does not belong to org {org_id}")
|
||||
|
||||
# Verify user is in the org
|
||||
org_member = await prisma.orgmember.find_unique(
|
||||
where={"orgId_userId": {"orgId": org_id, "userId": user_id}}
|
||||
|
||||
@@ -180,8 +180,11 @@ class TestSeatManagement:
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_assign_seat(self, mock_prisma):
|
||||
mock_prisma.organizationseatassignment.upsert = AsyncMock(
|
||||
return_value=MagicMock(userId="user-1", seatType="PAID", status="ACTIVE")
|
||||
)
|
||||
result = await assign_seat("org-1", "user-1", seat_type="PAID")
|
||||
assert result["seatType"] == "FREE" # From the mock default
|
||||
assert result["seatType"] == "PAID"
|
||||
mock_prisma.organizationseatassignment.upsert.assert_called_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
Reference in New Issue
Block a user