diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index 88553f11..75359f27 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -119,6 +119,24 @@ def _parse_bday(value: str | date | None) -> date | None: return None +def _first_custom(custom: dict[str, str | list[str]], key: str) -> str | None: + """Return the first raw value pythonvCard4 stashed in ``custom[key]``. + + The library has no typed parser for ORG / TITLE / unencoded PHOTO, so they + end up in ``Contact.custom`` keyed by property name. The library's typeshed + declares the values as ``str | list[str]`` even though the current parser + always appends to a list — accept both shapes so we don't break on a future + library version that switches to bare strings. Returns ``None`` when the + key is absent or the value is empty. + """ + values = custom.get(key) + if isinstance(values, list): + return values[0] if values else None + if isinstance(values, str): + return values or None + return None + + def _safe_vcard_value(value: Any) -> Any: """Escape newlines in a value so it can't inject additional vCard properties. @@ -446,6 +464,16 @@ class ContactsClient(BaseNextcloudClient): contact = Contact.from_vcard(addressdata) + # pythonvCard4's parser has no branch for ORG / TITLE — they fall + # through into ``contact.custom`` as a list of raw values. PHOTO + # only gets typed-parsed when the line carries an ``ENCODING=`` + # parameter; otherwise it lands in ``custom`` too. Pull them out + # here so the read side surfaces what the write side persisted + # (issue #716 follow-up). + org_value = _first_custom(contact.custom, "ORG") + title_value = _first_custom(contact.custom, "TITLE") + photo_value = contact.photo_data or _first_custom(contact.custom, "PHOTO") + contacts.append( { "vcard_id": vcard_id, @@ -458,6 +486,12 @@ class ContactsClient(BaseNextcloudClient): else contact.bday, "email": contact.email, "tel": contact.tel, + "org": org_value, + "title": title_value, + "note": contact.note, + "url": contact.url, + "categories": contact.categories, + "photo": photo_value, }, "addressdata": addressdata, } diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index d74b88db..7a9fef3e 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -65,15 +65,35 @@ def _parse_vcard_fields( def _raw_contact_to_model(raw: dict) -> Contact: """Convert a raw contact dict from the contacts client to a Contact model. - Maps fullname, nickname, birthday, email, and tel fields. - Email/tel values may be plain strings, dicts with ``value``/``type`` keys, - or lists of either – see :func:`_parse_vcard_fields`. + Maps fullname, nickname, birthday, email, tel, org, title, note, url, + categories, and photo fields. Email/tel values may be plain strings, dicts + with ``value``/``type`` keys, or lists of either – see + :func:`_parse_vcard_fields`. """ contact_info = raw.get("contact", {}) emails = _parse_vcard_fields(contact_info.get("email"), "email") phones = _parse_vcard_fields(contact_info.get("tel"), "phone") + # URL is parsed by pythonvCard4 into a plain ``list[str]``. Single-string + # inputs surface as such too. Either way wrap each into a ContactField. + raw_urls = contact_info.get("url") + if isinstance(raw_urls, str): + raw_urls = [raw_urls] if raw_urls else [] + urls = [ + ContactField(type="url", value=u) + for u in (raw_urls or []) + if isinstance(u, str) and u + ] + + # CATEGORIES is parsed as ``list[str]``. Accept a comma-separated string + # too for forward-compat with library updates that might change shape. + raw_categories = contact_info.get("categories") or [] + if isinstance(raw_categories, str): + categories = [c.strip() for c in raw_categories.split(",") if c.strip()] + else: + categories = [c for c in raw_categories if isinstance(c, str) and c] + # Nickname goes into custom_fields (no dedicated model field) custom_fields: dict[str, Any] = {} nickname = contact_info.get("nickname") @@ -84,11 +104,17 @@ def _raw_contact_to_model(raw: dict) -> Contact: uid=raw["vcard_id"], fn=contact_info.get("fullname", ""), etag=raw.get("getetag"), + organization=contact_info.get("org"), + title=contact_info.get("title"), + note=contact_info.get("note"), + photo=contact_info.get("photo"), birthday=contact_info["birthday"].isoformat() if isinstance(contact_info.get("birthday"), date) else contact_info.get("birthday"), emails=emails, phones=phones, + urls=urls, + categories=categories, custom_fields=custom_fields, ) diff --git a/tests/server/test_contacts_mcp.py b/tests/server/test_contacts_mcp.py index 6f842d33..e0f66f7b 100644 --- a/tests/server/test_contacts_mcp.py +++ b/tests/server/test_contacts_mcp.py @@ -1,5 +1,6 @@ """Integration tests for Contacts MCP tools.""" +import json import logging import uuid @@ -12,6 +13,11 @@ logger = logging.getLogger(__name__) pytestmark = pytest.mark.integration +def _extract_payload(tool_result) -> dict: + """Return the JSON-decoded text content of an MCP tool result.""" + return json.loads(tool_result.content[0].text) + + async def test_mcp_contacts_workflow( nc_mcp_client: ClientSession, nc_client: NextcloudClient ): @@ -62,6 +68,23 @@ async def test_mcp_contacts_workflow( assert "ORG:MCP Test Corp" in raw_vcard assert f"NOTE:Created by test {unique_suffix}" in raw_vcard + # 4a. Read-side round-trip — issue #716 follow-up. The write side has + # been correct since PR #719, but the MCP list/search tools returned + # ``organization: null`` / ``note: null`` because pythonvCard4 stashes + # ORG/TITLE in ``custom`` and the server's _raw_contact_to_model never + # surfaced ``note`` / ``urls`` either. + search_result = await nc_mcp_client.call_tool( + "nc_contacts_search_contacts", + {"query": unique_suffix, "addressbook": addressbook_name}, + ) + assert search_result.isError is False + search_payload = _extract_payload(search_result) + assert search_payload["total_count"] == 1 + searched = search_payload["contacts"][0] + assert searched["uid"] == contact_uid + assert searched["organization"] == "MCP Test Corp" + assert searched["note"] == f"Created by test {unique_suffix}" + # 4b. Update with a URL — regression guard for PR #719 review: # _merge_vcard_properties previously had no URL handler, silently dropping it. update_result = await nc_mcp_client.call_tool( diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py index a5deebb0..d9db7641 100644 --- a/tests/unit/client/test_contacts.py +++ b/tests/unit/client/test_contacts.py @@ -11,6 +11,7 @@ import pytest from nextcloud_mcp_server.client.contacts import ( _build_contact_from_data, + _first_custom, _normalize_contact_data, _wrap_contact_field, ) @@ -201,6 +202,31 @@ def test_missing_fn_logs_warning(caplog): assert any("fn" in r.message.lower() for r in caplog.records) +class TestFirstCustom: + """``_first_custom`` is the read-side companion to PR #719 — it pulls + ORG / TITLE / unencoded PHOTO out of pythonvCard4's ``custom`` dict because + the library has no typed parser for them. + """ + + def test_returns_first_value_from_list(self): + assert _first_custom({"ORG": ["Acme Corp"]}, "ORG") == "Acme Corp" + + def test_returns_first_value_when_library_uses_bare_string(self): + """The library's typeshed allows ``str`` as a value shape too. Accept it + so we don't break if the parser changes shape upstream. + """ + assert _first_custom({"TITLE": "Engineer"}, "TITLE") == "Engineer" + + def test_returns_none_for_missing_key(self): + assert _first_custom({"ORG": ["Acme"]}, "TITLE") is None + + def test_returns_none_for_empty_list(self): + assert _first_custom({"ORG": []}, "ORG") is None + + def test_returns_none_for_empty_string(self): + assert _first_custom({"ORG": ""}, "ORG") is None + + class TestNormalizeContactData: """Direct tests for the alias helper — it's load-bearing for update_contact too.""" diff --git a/tests/unit/test_response_models.py b/tests/unit/test_response_models.py index 856d4192..fd9dc84e 100644 --- a/tests/unit/test_response_models.py +++ b/tests/unit/test_response_models.py @@ -388,6 +388,73 @@ def test_contact_mapping_missing_optional_fields(): assert contact.custom_fields == {} +@pytest.mark.unit +def test_contact_mapping_surfaces_org_title_note_url_categories_photo(): + """Issue #716 follow-up: ORG / TITLE / NOTE / URL / CATEGORIES / PHOTO must + round-trip from the raw client dict onto the response model. + + Before this fix, the server-side mapper ignored these keys even when the + client supplied them, so MCP responses returned ``organization: null`` / + ``note: null`` for contacts that did have the fields set in their vCard. + """ + raw_contact = { + "vcard_id": "full-1", + "getetag": '"etag"', + "contact": { + "fullname": "Alice", + "org": "Acme Corp", + "title": "Engineer", + "note": "Met at conference", + "url": ["https://acme.example.com"], + "categories": ["vip", "customer"], + "photo": "https://photos.example.com/alice.jpg", + }, + } + + contact = _map_contact(raw_contact) + + assert contact.organization == "Acme Corp" + assert contact.title == "Engineer" + assert contact.note == "Met at conference" + assert len(contact.urls) == 1 + assert contact.urls[0].value == "https://acme.example.com" + assert contact.urls[0].type == "url" + assert contact.categories == ["vip", "customer"] + assert contact.photo == "https://photos.example.com/alice.jpg" + + +@pytest.mark.unit +def test_contact_mapping_accepts_url_as_plain_string(): + """The client dict normally carries ``url`` as a list, but accept a plain + string for forward-compat with library changes that might collapse single + entries. + """ + raw_contact = { + "vcard_id": "url-str-1", + "contact": {"fullname": "Bob", "url": "https://bob.example.com"}, + } + + contact = _map_contact(raw_contact) + + assert len(contact.urls) == 1 + assert contact.urls[0].value == "https://bob.example.com" + + +@pytest.mark.unit +def test_contact_mapping_categories_accepts_comma_string(): + """A comma-separated string is split into discrete categories on the read + side, matching how the write side parses ``categories``. + """ + raw_contact = { + "vcard_id": "cat-1", + "contact": {"fullname": "Carol", "categories": "vip, customer, archived"}, + } + + contact = _map_contact(raw_contact) + + assert contact.categories == ["vip", "customer", "archived"] + + @pytest.mark.unit def test_list_contacts_response_wraps_contacts(): """Test ListContactsResponse wraps contacts correctly for MCP output."""