diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 82439eb2..49e048b6 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -40,7 +40,7 @@ class CollectivesClient(BaseNextcloudClient): if status_code >= 400: message = meta.get("message", "OCS error") raise OCSError(status_code, message) - return ocs["data"] + return ocs.get("data", {}) # Collectives @@ -215,19 +215,21 @@ class CollectivesClient(BaseNextcloudClient): async def assign_tag(self, collective_id: int, page_id: int, tag_id: int) -> None: """Assign a tag to a page.""" - await self._make_request( + response = await self._make_request( "PUT", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", headers=self._get_ocs_headers(), ) + self._unwrap_ocs(response.json()) async def remove_tag(self, collective_id: int, page_id: int, tag_id: int) -> None: """Remove a tag from a page.""" - await self._make_request( + response = await self._make_request( "DELETE", f"{API_BASE}/collectives/{collective_id}/pages/{page_id}/tags/{tag_id}", headers=self._get_ocs_headers(), ) + self._unwrap_ocs(response.json()) # Trash diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py index 3e377f67..03eafafe 100644 --- a/nextcloud_mcp_server/models/collectives.py +++ b/nextcloud_mcp_server/models/collectives.py @@ -1,6 +1,8 @@ """Pydantic models for Nextcloud Collectives app.""" -from pydantic import BaseModel, Field +import re + +from pydantic import BaseModel, Field, field_validator from .base import BaseResponse, StatusResponse @@ -53,7 +55,14 @@ class CollectiveTag(BaseModel): id: int = Field(description="Tag ID") collectiveId: int = Field(description="Parent collective ID") name: str = Field(description="Tag name") - color: str = Field(description="Hex color code") + color: str = Field(description="Hex color code (e.g. 'FF0000')") + + @field_validator("color") + @classmethod + def validate_hex_color(cls, v: str) -> str: + if not re.fullmatch(r"[0-9A-Fa-f]{3,8}", v): + raise ValueError(f"Invalid hex color: {v!r}") + return v # Response Models diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index d98811e0..f3a346cc 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -34,8 +34,8 @@ logger = logging.getLogger(__name__) def _handle_collectives_error(e: OCSError | HTTPStatusError) -> McpError: """Convert OCS or HTTP errors to McpError.""" if isinstance(e, OCSError): - return McpError(ErrorData(code=e.status_code, message=e.message)) - return McpError(ErrorData(code=e.response.status_code, message=str(e))) + return McpError(ErrorData(code=-1, message=e.message)) + return McpError(ErrorData(code=-1, message=str(e))) def configure_collectives_tools(mcp: FastMCP): @@ -120,7 +120,7 @@ def configure_collectives_tools(mcp: FastMCP): if page.filePath: parts.append(page.filePath) parts.append(page.fileName) - webdav_path = "/".join(parts) + webdav_path = "/".join(p.strip("/") for p in parts) try: file_bytes, _ = await client.webdav.read_file(webdav_path) content = file_bytes.decode("utf-8") @@ -243,11 +243,13 @@ def configure_collectives_tools(mcp: FastMCP): async def collectives_update_collective( ctx: Context, collective_id: int, emoji: str | None = None ) -> CollectiveOperationResponse: - """Update a Nextcloud Collective (emoji) + """Update a Nextcloud Collective (emoji). + + At least one field must be provided. Args: collective_id: ID of the collective - emoji: New emoji for the collective + emoji: New emoji for the collective (required) """ client = await get_client(ctx) try: @@ -340,7 +342,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Trash Collective Page", annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=True, openWorldHint=True + destructiveHint=True, idempotentHint=False, openWorldHint=True ), ) @require_scopes("collectives:write") diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index 5bdc3ca4..9207b35d 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -263,7 +263,7 @@ async def test_create_tag(mocker): async def test_assign_tag(mocker): """Test assigning a tag to a page.""" - mock_response = create_mock_response(status_code=200, json_data={}) + mock_response = _ocs_response({}) mock_request = mocker.patch.object( CollectivesClient, "_make_request", return_value=mock_response ) @@ -278,7 +278,7 @@ async def test_assign_tag(mocker): async def test_remove_tag(mocker): """Test removing a tag from a page.""" - mock_response = create_mock_response(status_code=200, json_data={}) + mock_response = _ocs_response({}) mock_request = mocker.patch.object( CollectivesClient, "_make_request", return_value=mock_response ) @@ -327,6 +327,25 @@ async def test_restore_page(mocker): # --- Error Handling --- +async def test_ocs_missing_data_returns_empty(mocker): + """Test that OCS envelope without 'data' key returns empty dict.""" + mock_response = create_mock_response( + status_code=200, + json_data={ + "ocs": { + "meta": {"status": "ok", "statuscode": 200}, + } + }, + ) + mocker.patch.object(CollectivesClient, "_make_request", return_value=mock_response) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + # get_collectives accesses data["collectives"], which will KeyError on empty dict + # This tests that _unwrap_ocs itself doesn't crash — it returns {} + with pytest.raises(KeyError): + await client.get_collectives() + + async def test_ocs_error_status_raises(mocker): """Test that OCS envelope with error statuscode raises OCSError.""" mock_response = create_mock_response(