From cb16060b1b450e5803fb32daa78d98a1d73baea8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 27 Mar 2026 11:26:26 +0100 Subject: [PATCH] fix: address PR review feedback (round 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix inconsistent error code in set_collective_emoji (400 → -32603) - Allow clearing emoji via set_collective_emoji(emoji=None) - Remove destructiveHint from trash operations (soft deletes are recoverable) - Change delete_collective to idempotentHint=False (requires trash precondition) - Add restore_collective and get_trashed_collectives tools - Add unit tests for ValueError guard, clear-emoji path, and new tools - Add integration test for full trash/restore/delete lifecycle - Verify move_page returns new title in response message Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 20 ++++++ nextcloud_mcp_server/models/collectives.py | 7 ++ nextcloud_mcp_server/server/collectives.py | 70 ++++++++++++++++--- .../collectives/test_collectives_api.py | 64 +++++++++++++++++ tests/server/test_annotations.py | 6 +- tests/server/test_collectives_mcp.py | 68 ++++++++++++++++++ 6 files changed, 223 insertions(+), 12 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 6fd140bb..4df636fb 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -118,6 +118,26 @@ class CollectivesClient(BaseNextcloudClient): ) self._unwrap_ocs(response.json()) + # Trash (collectives) + + async def get_trashed_collectives(self) -> list[dict[str, Any]]: + """List trashed collectives.""" + response = await self._make_request( + "GET", f"{API_BASE}/collectives/trash", headers=self._OCS_HEADERS + ) + data = self._unwrap_ocs(response.json()) + return data["collectives"] + + async def restore_collective(self, collective_id: int) -> dict[str, Any]: + """Restore a collective from trash.""" + response = await self._make_request( + "PATCH", + f"{API_BASE}/collectives/trash/{collective_id}", + headers=self._OCS_HEADERS, + ) + data = self._unwrap_ocs(response.json()) + return data["collective"] + # Pages async def get_pages(self, collective_id: int) -> list[dict[str, Any]]: diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py index 25259407..538b366e 100644 --- a/nextcloud_mcp_server/models/collectives.py +++ b/nextcloud_mcp_server/models/collectives.py @@ -140,6 +140,13 @@ class ListTrashedPagesResponse(ListPagesResponse): ) +class ListTrashedCollectivesResponse(BaseResponse): + """Response for listing trashed collectives.""" + + collectives: list[Collective] = Field(description="List of trashed collectives") + total: int = Field(description="Total number of trashed collectives") + + class ListTagsResponse(BaseResponse): """Response for listing tags in a collective.""" diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 85e8f194..6d216022 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -21,6 +21,7 @@ from nextcloud_mcp_server.models.collectives import ( ListCollectivesResponse, ListPagesResponse, ListTagsResponse, + ListTrashedCollectivesResponse, ListTrashedPagesResponse, PageInfo, PageOperationResponse, @@ -241,21 +242,22 @@ def configure_collectives_tools(mcp: FastMCP): @require_scopes("collectives:write") @instrument_tool async def collectives_set_collective_emoji( - ctx: Context, collective_id: int, emoji: str + ctx: Context, collective_id: int, emoji: str | None = None ) -> CollectiveOperationResponse: - """Set the emoji on a Nextcloud Collective. + """Set or clear the emoji on a Nextcloud Collective. Setting the same emoji twice produces the same result (idempotent). + Pass emoji=None to clear the emoji. Args: collective_id: ID of the collective - emoji: Emoji to set on the collective + emoji: Emoji to set, or None to clear """ client = await get_client(ctx) try: raw = await client.collectives.update_collective(collective_id, emoji) except ValueError as e: - raise McpError(ErrorData(code=400, message=str(e))) from e + raise McpError(ErrorData(code=-32603, message=str(e))) from e except (OCSError, HTTPStatusError) as e: raise _handle_collectives_error(e) from e collective = Collective(**raw) @@ -267,9 +269,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Trash Collective", - annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=False, openWorldHint=True - ), + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), ) @require_scopes("collectives:write") @instrument_tool @@ -297,7 +297,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Delete Collective", annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=True, openWorldHint=True + destructiveHint=True, idempotentHint=False, openWorldHint=True ), ) @require_scopes("collectives:write") @@ -325,6 +325,56 @@ def configure_collectives_tools(mcp: FastMCP): message="Collective permanently deleted", ) + @mcp.tool( + title="List Trashed Collectives", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("collectives:read") + @instrument_tool + async def collectives_get_trashed_collectives( + ctx: Context, + ) -> ListTrashedCollectivesResponse: + """List all trashed Nextcloud Collectives. + + Returns collectives that have been soft-deleted and can be restored + or permanently deleted. + """ + client = await get_client(ctx) + try: + raw = await client.collectives.get_trashed_collectives() + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e + collectives = [Collective(**c) for c in raw] + return ListTrashedCollectivesResponse( + collectives=collectives, total=len(collectives) + ) + + @mcp.tool( + title="Restore Collective", + annotations=ToolAnnotations(idempotentHint=True, openWorldHint=True), + ) + @require_scopes("collectives:write") + @instrument_tool + async def collectives_restore_collective( + ctx: Context, collective_id: int + ) -> CollectiveOperationResponse: + """Restore a Nextcloud Collective from trash. + + Args: + collective_id: ID of the trashed collective to restore + """ + client = await get_client(ctx) + try: + raw = await client.collectives.restore_collective(collective_id) + except (OCSError, HTTPStatusError) as e: + raise _handle_collectives_error(e) from e + collective = Collective(**raw) + return CollectiveOperationResponse( + collective_id=collective.id, + status_code=200, + message=f"Collective '{collective.name}' restored from trash", + ) + @mcp.tool( title="Create Collective Page", annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), @@ -401,9 +451,7 @@ def configure_collectives_tools(mcp: FastMCP): @mcp.tool( title="Trash Collective Page", - annotations=ToolAnnotations( - destructiveHint=True, idempotentHint=False, openWorldHint=True - ), + annotations=ToolAnnotations(idempotentHint=False, openWorldHint=True), ) @require_scopes("collectives:write") @instrument_tool diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index e6865c59..04ae40cf 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -453,3 +453,67 @@ async def test_get_page_404(mocker): client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") with pytest.raises(httpx.HTTPStatusError): await client.get_page(collective_id=1, page_id=999) + + +# --- Additional coverage --- + + +async def test_update_collective_no_fields_raises_value_error(mocker): + """Test that update_collective raises ValueError when called with no fields.""" + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(ValueError, match="At least one field"): + await client.update_collective(collective_id=1) + + +async def test_set_page_emoji_clear(mocker): + """Test that set_page_emoji sends null emoji to clear it.""" + page = _sample_page(10, "Test Page") + page["emoji"] = None + mock_response = _ocs_response({"page": page}) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.set_page_emoji(collective_id=1, page_id=10, emoji=None) + + assert result["emoji"] is None + call_args = mock_request.call_args + assert call_args[1]["json"] == {"emoji": None} + + +async def test_get_trashed_collectives(mocker): + """Test listing trashed collectives.""" + mock_response = _ocs_response( + {"collectives": [_sample_collective(1, "Trashed Wiki")]} + ) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.get_trashed_collectives() + + assert len(result) == 1 + assert result[0]["name"] == "Trashed Wiki" + call_args = mock_request.call_args + assert "/collectives/trash" in call_args[0][1] + assert call_args[0][0] == "GET" + + +async def test_restore_collective(mocker): + """Test restoring a collective from trash.""" + mock_response = _ocs_response( + {"collective": _sample_collective(5, "Restored Wiki")} + ) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.restore_collective(collective_id=5) + + assert result["name"] == "Restored Wiki" + call_args = mock_request.call_args + assert call_args[0][0] == "PATCH" + assert "/collectives/trash/5" in call_args[0][1] diff --git a/tests/server/test_annotations.py b/tests/server/test_annotations.py index 4ef1acd7..9424eb71 100644 --- a/tests/server/test_annotations.py +++ b/tests/server/test_annotations.py @@ -78,8 +78,12 @@ async def test_delete_operations_are_idempotent(nc_mcp_client: ClientSession): """Verify delete operations are marked as idempotent (ADR-017 decision).""" tools = await nc_mcp_client.list_tools() + # Exceptions: delete operations that require a precondition (e.g. must be + # trashed first), so calling twice produces an error on the second call. + non_idempotent_deletes = {"collectives_delete_collective"} + for tool in tools.tools: - if "delete" in tool.name.lower(): + if "delete" in tool.name.lower() and tool.name not in non_idempotent_deletes: assert tool.annotations is not None, f"Tool {tool.name} missing annotations" assert tool.annotations.idempotentHint is True, ( f"Delete tool {tool.name} should be idempotent (same end state)" diff --git a/tests/server/test_collectives_mcp.py b/tests/server/test_collectives_mcp.py index c69272ba..aae68c07 100644 --- a/tests/server/test_collectives_mcp.py +++ b/tests/server/test_collectives_mcp.py @@ -87,6 +87,8 @@ async def test_collectives_tools_available(nc_mcp_client: ClientSession): "collectives_assign_tag", "collectives_remove_tag", "collectives_get_trashed_pages", + "collectives_get_trashed_collectives", + "collectives_restore_collective", ] for expected in expected_tools: @@ -280,6 +282,7 @@ async def test_collectives_move_page( data = json.loads(move_result.content[0].text) assert data["page_id"] == page_id assert "moved" in data["message"] + assert new_title in data["message"] logger.info(f"Page renamed to: {new_title}") # Cleanup @@ -382,6 +385,71 @@ async def test_collectives_search( logger.info(f"Search returned {data['total']} results for 'Welcome'") +# --- Collective Trash / Restore / Delete --- + + +async def test_collectives_trash_restore_delete_workflow( + nc_mcp_client: ClientSession, +): + """Test the full collective lifecycle: create, trash, list trashed, restore, trash, delete.""" + # Create a throwaway collective + name = f"Lifecycle Test {uuid.uuid4().hex[:8]}" + create_result = await nc_mcp_client.call_tool( + "collectives_create_collective", + {"name": name}, + ) + assert create_result.isError is False + created = json.loads(create_result.content[0].text) + cid = created["id"] + logger.info(f"Created collective {name} (ID: {cid})") + + # Trash the collective + trash_result = await nc_mcp_client.call_tool( + "collectives_trash_collective", + {"collective_id": cid}, + ) + assert trash_result.isError is False + logger.info("Collective moved to trash") + + # List trashed collectives — should include ours + list_trash_result = await nc_mcp_client.call_tool( + "collectives_get_trashed_collectives", + {}, + ) + assert list_trash_result.isError is False + trash_data = json.loads(list_trash_result.content[0].text) + trashed_ids = [c["id"] for c in trash_data["collectives"]] + assert cid in trashed_ids + logger.info(f"Found {trash_data['total']} trashed collectives") + + # Restore the collective + restore_result = await nc_mcp_client.call_tool( + "collectives_restore_collective", + {"collective_id": cid}, + ) + assert restore_result.isError is False + restore_data = json.loads(restore_result.content[0].text) + assert restore_data["collective_id"] == cid + assert "restored" in restore_data["message"].lower() + logger.info("Collective restored from trash") + + # Trash again, then permanently delete + trash_result2 = await nc_mcp_client.call_tool( + "collectives_trash_collective", + {"collective_id": cid}, + ) + assert trash_result2.isError is False + + delete_result = await nc_mcp_client.call_tool( + "collectives_delete_collective", + {"collective_id": cid}, + ) + assert delete_result.isError is False + delete_data = json.loads(delete_result.content[0].text) + assert "permanently deleted" in delete_data["message"].lower() + logger.info("Collective permanently deleted") + + # --- Error Handling ---