fix(deck): address review — notesPath key, scopes, modernize types
PR #781 review round 1: - 🔴 Fix notesPath key: the Notes API returns the folder under camelCase ``notesPath`` (see models/notes.py:43), but `deck_attach_note` was looking up snake_case ``notes_path`` and silently falling back to ``"Notes"``. Users with a non-default notes folder would have produced shares pointing at non-existent files (404 on click in Deck UI). - 🔴 Add wire-through unit test that would have caught the above: extract `_resolve_note_attach_path(client, note_id)` as a testable helper that encapsulates the camelCase-key lookup. Three new tests: custom notesPath honored, missing key falls back to default, null category handled. - 🟡 Modernize new fields on `DeckAttachmentExtendedData` to PEP 604 (`X | None`) per CLAUDE.md. - 🟡 Drop unnecessary string forward reference on `ListAttachmentsResponse.results` — DeckAttachment is defined earlier in the same module. - 🟢 Move `pytestmark = pytest.mark.unit` to module level in test_sharing_client.py to match the convention in test_deck_server.py. Per user request: `deck_attach_file` is now scoped `deck.write` + ``files.read`` (was just `deck.write`) so the generic file-share permission story is consistent — only `deck_attach_note` keeps `notes.read` since it specifically reads from the Notes app. Docstring updated to emphasise the tool is generic over the user's Files (PDFs/images/etc., not just markdown). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
c0a974c498
commit
271904c227
@@ -11,6 +11,8 @@ from httpx import AsyncClient
|
||||
|
||||
from nextcloud_mcp_server.client.sharing import SharingClient
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def sharing_client(mocker):
|
||||
@@ -32,7 +34,6 @@ def _ok_share_response(mocker, share_id: int = 4242, **extra):
|
||||
return response
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
async def test_create_share_deck_type_payload(sharing_client, mocker):
|
||||
"""create_share(share_type=12) must POST exactly what the Deck UI does:
|
||||
{path, shareType: 12, shareWith: "<cardId>"} to /ocs/v2.php/apps/files_sharing/api/v1/shares.
|
||||
@@ -65,7 +66,6 @@ async def test_create_share_deck_type_payload(sharing_client, mocker):
|
||||
assert call.kwargs["headers"]["OCS-APIRequest"] == "true"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
async def test_create_share_raises_on_ocs_failure(sharing_client, mocker):
|
||||
"""OCS error responses (statuscode != 100/200) raise RuntimeError."""
|
||||
response = mocker.Mock()
|
||||
|
||||
@@ -14,6 +14,7 @@ from nextcloud_mcp_server.server.deck import (
|
||||
_apply_board_filters,
|
||||
_apply_card_filters,
|
||||
_apply_stack_filters,
|
||||
_resolve_note_attach_path,
|
||||
_resolve_note_path,
|
||||
_truncate_card_descriptions,
|
||||
_validate_description_max_length,
|
||||
@@ -408,3 +409,62 @@ def test_resolve_note_path_custom_notes_folder():
|
||||
def test_share_type_deck_constant_matches_deck_app():
|
||||
"""Deck UI uses shareType=12 (IShare::TYPE_DECK) — must not drift."""
|
||||
assert _SHARE_TYPE_DECK == 12
|
||||
|
||||
|
||||
# _resolve_note_attach_path (camelCase notesPath guard) ---------------------
|
||||
|
||||
|
||||
async def test_resolve_note_attach_path_honors_camelcase_notes_path(mocker):
|
||||
"""Custom notes folders configured in the Notes app must be honored.
|
||||
|
||||
Regression: the Notes API returns the folder under ``notesPath`` (camelCase,
|
||||
see ``models/notes.py:43``). An earlier draft of this code looked up
|
||||
``notes_path`` (snake_case) and silently fell back to the default ``"Notes"``,
|
||||
producing 404s for users with a non-default folder. This test pins the
|
||||
correct key so that bug can't reappear.
|
||||
"""
|
||||
client = mocker.AsyncMock()
|
||||
client.notes.get_settings.return_value = {"notesPath": "Documents/MyNotes"}
|
||||
client.notes.get_note.return_value = {
|
||||
"id": 42,
|
||||
"title": "Q4 Plan",
|
||||
"category": "Work",
|
||||
}
|
||||
|
||||
path = await _resolve_note_attach_path(client, note_id=42)
|
||||
|
||||
assert path == "/Documents/MyNotes/Work/Q4 Plan.md"
|
||||
client.notes.get_settings.assert_awaited_once()
|
||||
client.notes.get_note.assert_awaited_once_with(42)
|
||||
|
||||
|
||||
async def test_resolve_note_attach_path_falls_back_to_default_when_setting_missing(
|
||||
mocker,
|
||||
):
|
||||
"""Missing/empty ``notesPath`` falls back to the documented default."""
|
||||
client = mocker.AsyncMock()
|
||||
client.notes.get_settings.return_value = {}
|
||||
client.notes.get_note.return_value = {
|
||||
"id": 1,
|
||||
"title": "Idea",
|
||||
"category": "",
|
||||
}
|
||||
|
||||
path = await _resolve_note_attach_path(client, note_id=1)
|
||||
|
||||
assert path == "/Notes/Idea.md"
|
||||
|
||||
|
||||
async def test_resolve_note_attach_path_handles_null_category(mocker):
|
||||
"""A note with ``category=None`` (rather than ``""``) must not crash."""
|
||||
client = mocker.AsyncMock()
|
||||
client.notes.get_settings.return_value = {"notesPath": "Notes"}
|
||||
client.notes.get_note.return_value = {
|
||||
"id": 7,
|
||||
"title": "Bare",
|
||||
"category": None,
|
||||
}
|
||||
|
||||
path = await _resolve_note_attach_path(client, note_id=7)
|
||||
|
||||
assert path == "/Notes/Bare.md"
|
||||
|
||||
Reference in New Issue
Block a user