admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address PR review feedback (round 8)

Browse Source 
- 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) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-03-27 11:26:26 +01:00
co-authored by Claude Opus 4.6
parent 7224a2ebe3
commit cb16060b1b
6 changed files with 223 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nextcloud_mcp_server/client/collectives.py
+20
View File
@@ -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]]:
nextcloud_mcp_server/models/collectives.py
+7
View File
@@ -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."""
nextcloud_mcp_server/server/collectives.py
+59 -11
View File
@@ -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
tests/client/collectives/test_collectives_api.py
+64
View File
@@ -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]
tests/server/test_annotations.py
+5 -1
View File
@@ -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)"
tests/server/test_collectives_mcp.py
+68
View File
@@ -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 ---