Skip to content

Commit 848856f

Browse files
committed
Fix get_app_info returning null fields, tighten test assertions
- Remove enabled/type from _format_app (getAppInfo endpoint doesn't return active/type fields, so they were always null) - Use ADDITIVE_IDEMPOTENT annotation for enable_app (idempotent op) - Fix test_disable_returns_confirmation to actually assert the message - Tighten user-permission tests: ToolError instead of bare Exception, regex matches actual Nextcloud error messages - Update PROGRESS.md with app management and collectives entries
1 parent 526a1fa commit 848856f

4 files changed

Lines changed: 24 additions & 17 deletions

File tree

PROGRESS.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@
3030
- [x] Files Trashbin tools: list_trash, restore_trash_item, empty_trash (2026-03-27)
3131
- [x] Files Versions tools: list_versions, restore_version (2026-03-27)
3232
- [x] Mail tools: list_mail_accounts, list_mailboxes, list_mail_messages, get_mail_message, send_mail (2026-03-28)
33+
- [x] Collectives tools: list_collectives, get_collective_pages, get_collective_page (2026-03-29)
34+
- [x] App Management tools: list_apps, get_app_info, enable_app, disable_app (2026-03-30)
35+
- [x] User-permission integration tests: non-admin error handling validation (2026-03-30)
3336

3437
### In Progress
3538

@@ -66,10 +69,13 @@
6669
| Shares | 5 | 40 |
6770
| System Tags | 6 | 22 |
6871
| Mail | 5 | 29 |
72+
| Collectives | 3 | 22 |
73+
| App Management | 4 | 14 |
74+
| User Permissions || 15 |
6975
| Server || 7 |
7076
| Permissions || 34 |
7177
| Errors || 16 |
7278
| Client || 29 |
7379
| Config || 17 |
7480
| State || 2 |
75-
| **Total** | **60** | **491** |
81+
| **Total** | **67** | **542** |

src/nc_mcp_server/tools/app_management.py

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
from mcp.server.fastmcp import FastMCP
77

8-
from ..annotations import ADDITIVE, DESTRUCTIVE, READONLY
8+
from ..annotations import ADDITIVE_IDEMPOTENT, DESTRUCTIVE, READONLY
99
from ..permissions import PermissionLevel, require_permission
1010
from ..state import get_client
1111

@@ -18,8 +18,6 @@ def _format_app(a: dict[str, Any]) -> dict[str, Any]:
1818
"name": a.get("name"),
1919
"summary": a.get("summary"),
2020
"version": a.get("version"),
21-
"enabled": a.get("active"),
22-
"type": a.get("type"),
2321
"author": a.get("author"),
2422
}
2523

@@ -54,22 +52,22 @@ async def list_apps(app_filter: str = "enabled") -> str:
5452
async def get_app_info(app_id: str) -> str:
5553
"""Get detailed information about an installed Nextcloud app. Requires admin privileges.
5654
57-
Returns app metadata including name, version, description, author, and status.
55+
Returns app metadata including name, version, description, and author.
5856
5957
Args:
6058
app_id: The app identifier (e.g. "spreed", "mail", "collectives").
6159
Use list_apps to find available app IDs.
6260
6361
Returns:
64-
JSON object with app details: id, name, summary, version, enabled, type, author.
62+
JSON object with app details: id, name, summary, version, author.
6563
"""
6664
client = get_client()
6765
data = await client.ocs_get(f"{APPS_API}/{app_id}")
6866
return json.dumps(_format_app(data), indent=2, default=str)
6967

7068

7169
def _register_write_tools(mcp: FastMCP) -> None:
72-
@mcp.tool(annotations=ADDITIVE)
70+
@mcp.tool(annotations=ADDITIVE_IDEMPOTENT)
7371
@require_permission(PermissionLevel.WRITE)
7472
async def enable_app(app_id: str) -> str:
7573
"""Enable a Nextcloud app. Requires admin privileges.

tests/integration/test_app_management.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,9 @@ async def test_get_known_app(self, nc_mcp: McpTestHelper) -> None:
5858
async def test_get_app_has_fields(self, nc_mcp: McpTestHelper) -> None:
5959
result = await nc_mcp.call("get_app_info", app_id="dav")
6060
app = json.loads(result)
61-
for field in ["id", "name", "version", "enabled"]:
61+
for field in ["id", "name", "version", "author"]:
6262
assert field in app, f"Missing field: {field}"
63+
assert app[field] is not None, f"Field '{field}' should not be None"
6364

6465
@pytest.mark.asyncio
6566
async def test_get_nonexistent_app_raises(self, nc_mcp: McpTestHelper) -> None:
@@ -87,9 +88,9 @@ async def test_enable_already_enabled(self, nc_mcp: McpTestHelper) -> None:
8788

8889
@pytest.mark.asyncio
8990
async def test_disable_returns_confirmation(self, nc_mcp: McpTestHelper) -> None:
90-
await nc_mcp.call("disable_app", app_id=SAFE_APP)
9191
try:
92-
pass
92+
result = await nc_mcp.call("disable_app", app_id=SAFE_APP)
93+
assert "disabled" in result.lower()
9394
finally:
9495
await nc_mcp.call("enable_app", app_id=SAFE_APP)
9596

tests/integration/test_user_permissions.py

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111

1212
import pytest
1313

14+
from mcp.server.fastmcp.exceptions import ToolError
15+
1416
from nc_mcp_server.client import NextcloudClient, NextcloudError
1517
from nc_mcp_server.config import Config
1618
from nc_mcp_server.permissions import PermissionLevel
@@ -112,35 +114,35 @@ async def test_list_notifications(self, user_mcp: McpTestHelper) -> None:
112114
class TestAdminOnlyToolsReturnErrors:
113115
@pytest.mark.asyncio
114116
async def test_list_users_forbidden(self, user_mcp: McpTestHelper) -> None:
115-
with pytest.raises(Exception, match=r"403|Forbidden|admin"):
117+
with pytest.raises(ToolError, match=r"must be.*admin|403|[Ff]orbidden"):
116118
await user_mcp.call("list_users")
117119

118120
@pytest.mark.asyncio
119121
async def test_create_user_forbidden(self, user_mcp: McpTestHelper) -> None:
120-
with pytest.raises(Exception, match=r"403|Forbidden|admin|sub admin"):
122+
with pytest.raises(ToolError, match=r"must be.*admin|403|[Ff]orbidden"):
121123
await user_mcp.call("create_user", user_id="hacker", password=TEST_PASS)
122124

123125
@pytest.mark.asyncio
124126
async def test_delete_user_forbidden(self, user_mcp: McpTestHelper) -> None:
125-
with pytest.raises(Exception, match=r"403|Forbidden|admin"):
127+
with pytest.raises(ToolError, match=r"must be.*admin|403|[Ff]orbidden"):
126128
await user_mcp.call("delete_user", user_id="admin")
127129

128130
@pytest.mark.asyncio
129131
async def test_list_apps_forbidden(self, user_mcp: McpTestHelper) -> None:
130-
with pytest.raises(Exception, match=r"403|Forbidden|admin"):
132+
with pytest.raises(ToolError, match=r"must be.*admin|403|[Ff]orbidden"):
131133
await user_mcp.call("list_apps")
132134

133135
@pytest.mark.asyncio
134136
async def test_enable_app_forbidden(self, user_mcp: McpTestHelper) -> None:
135-
with pytest.raises(Exception, match=r"403|Forbidden|admin"):
137+
with pytest.raises(ToolError, match=r"must be.*admin|403|[Ff]orbidden"):
136138
await user_mcp.call("enable_app", app_id="weather_status")
137139

138140
@pytest.mark.asyncio
139141
async def test_disable_app_forbidden(self, user_mcp: McpTestHelper) -> None:
140-
with pytest.raises(Exception, match=r"403|Forbidden|admin"):
142+
with pytest.raises(ToolError, match=r"must be.*admin|403|[Ff]orbidden"):
141143
await user_mcp.call("disable_app", app_id="weather_status")
142144

143145
@pytest.mark.asyncio
144146
async def test_get_app_info_forbidden(self, user_mcp: McpTestHelper) -> None:
145-
with pytest.raises(Exception, match=r"403|Forbidden|admin"):
147+
with pytest.raises(ToolError, match=r"must be.*admin|403|[Ff]orbidden"):
146148
await user_mcp.call("get_app_info", app_id="files")

0 commit comments

Comments
 (0)