From 52470ea713c422c769c7976a29215861905d1777 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 28 Mar 2026 09:19:53 +0100 Subject: [PATCH] fix: address PR review feedback (round 9) - Fix emoji clearing bug: use _UNSET sentinel in update_collective so emoji=None sends {"emoji": null} instead of raising ValueError - Move collectives_get_trashed_collectives to Read Tools section - Remove redundant is_trash field from ListTrashedPagesResponse - Add page lifecycle note to collectives_trash_page docstring - Add unit test for clearing collective emoji via update_collective - Add integration test for clearing collective emoji via MCP tool Co-Authored-By: Claude Opus 4.6 (1M context) --- nextcloud_mcp_server/client/collectives.py | 10 +++- nextcloud_mcp_server/models/collectives.py | 4 -- nextcloud_mcp_server/server/collectives.py | 54 ++++++++++--------- .../collectives/test_collectives_api.py | 18 +++++++ tests/server/test_collectives_mcp.py | 24 +++++++++ 5 files changed, 79 insertions(+), 31 deletions(-) diff --git a/nextcloud_mcp_server/client/collectives.py b/nextcloud_mcp_server/client/collectives.py index 4df636fb..ec2569a2 100644 --- a/nextcloud_mcp_server/client/collectives.py +++ b/nextcloud_mcp_server/client/collectives.py @@ -9,6 +9,9 @@ logger = logging.getLogger(__name__) API_BASE = "/ocs/v2.php/apps/collectives/api/v1.0" +_UNSET = object() +"""Sentinel to distinguish 'not provided' from an explicit None.""" + class OCSError(Exception): """Error returned in the OCS response envelope.""" @@ -75,15 +78,18 @@ class CollectivesClient(BaseNextcloudClient): return data["collective"] async def update_collective( - self, collective_id: int, emoji: str | None = None + self, collective_id: int, emoji: str | None | object = _UNSET ) -> dict[str, Any]: """Update a collective (emoji). + Pass emoji=None to clear the emoji. Omit emoji entirely to leave + it unchanged. + Raises: ValueError: If no fields are provided to update. """ json_data: dict[str, Any] = {} - if emoji is not None: + if emoji is not _UNSET: json_data["emoji"] = emoji if not json_data: raise ValueError("At least one field must be provided to update") diff --git a/nextcloud_mcp_server/models/collectives.py b/nextcloud_mcp_server/models/collectives.py index 538b366e..b4c0f6e0 100644 --- a/nextcloud_mcp_server/models/collectives.py +++ b/nextcloud_mcp_server/models/collectives.py @@ -135,10 +135,6 @@ class SearchPagesResponse(BaseResponse): class ListTrashedPagesResponse(ListPagesResponse): """Response for listing trashed pages in a collective.""" - is_trash: bool = Field( - default=True, description="Indicates these are trashed pages" - ) - class ListTrashedCollectivesResponse(BaseResponse): """Response for listing trashed collectives.""" diff --git a/nextcloud_mcp_server/server/collectives.py b/nextcloud_mcp_server/server/collectives.py index 6d216022..cd0dbd66 100644 --- a/nextcloud_mcp_server/server/collectives.py +++ b/nextcloud_mcp_server/server/collectives.py @@ -208,6 +208,30 @@ def configure_collectives_tools(mcp: FastMCP): pages=pages, total=len(pages), collective_id=collective_id ) + @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) + ) + # --- Write Tools --- @mcp.tool( @@ -325,30 +349,6 @@ 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), @@ -458,7 +458,11 @@ def configure_collectives_tools(mcp: FastMCP): async def collectives_trash_page( ctx: Context, collective_id: int, page_id: int ) -> PageOperationResponse: - """Move a page to trash in a Nextcloud Collective (soft delete) + """Move a page to trash in a Nextcloud Collective (soft delete). + + Trashed pages can be restored with collectives_restore_page. The + Collectives API does not support permanent page deletion; trashed + pages are cleaned up by Nextcloud's retention policy. Args: collective_id: ID of the collective diff --git a/tests/client/collectives/test_collectives_api.py b/tests/client/collectives/test_collectives_api.py index 04ae40cf..46460bc4 100644 --- a/tests/client/collectives/test_collectives_api.py +++ b/tests/client/collectives/test_collectives_api.py @@ -461,10 +461,28 @@ async def test_get_page_404(mocker): 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") + # Omitting emoji entirely (not passing it) should raise with pytest.raises(ValueError, match="At least one field"): await client.update_collective(collective_id=1) +async def test_update_collective_clear_emoji(mocker): + """Test that update_collective sends null emoji to clear it.""" + mock_response = _ocs_response( + {"collective": _sample_collective(1, "Test Wiki", emoji=None)} + ) + mock_request = mocker.patch.object( + CollectivesClient, "_make_request", return_value=mock_response + ) + + client = CollectivesClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + result = await client.update_collective(collective_id=1, emoji=None) + + assert result["emoji"] is None + call_args = mock_request.call_args + assert call_args[1]["json"] == {"emoji": None} + + 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") diff --git a/tests/server/test_collectives_mcp.py b/tests/server/test_collectives_mcp.py index aae68c07..96ebd635 100644 --- a/tests/server/test_collectives_mcp.py +++ b/tests/server/test_collectives_mcp.py @@ -134,6 +134,30 @@ async def test_collectives_set_collective_emoji( logger.info("Collective emoji updated") +async def test_collectives_clear_collective_emoji( + nc_mcp_client: ClientSession, temporary_collective: dict +): + """Test clearing a collective's emoji by passing null.""" + cid = temporary_collective["id"] + + # Set an emoji first + set_result = await nc_mcp_client.call_tool( + "collectives_set_collective_emoji", + {"collective_id": cid, "emoji": "🔬"}, + ) + assert set_result.isError is False + + # Clear the emoji by passing null + clear_result = await nc_mcp_client.call_tool( + "collectives_set_collective_emoji", + {"collective_id": cid, "emoji": None}, + ) + assert clear_result.isError is False + data = json.loads(clear_result.content[0].text) + assert data["collective_id"] == cid + logger.info("Collective emoji cleared") + + # --- Page CRUD ---