fix(contacts): surface ORG/TITLE/NOTE/URL/CATEGORIES/PHOTO on read (refs #716)
PR #719 fixed the contact-create path so all documented fields persist to the vCard, but the read path (list/search via MCP) still returned ``organization: null`` / ``note: null`` / ``title: null`` because pythonvCard4 has no typed parser for ORG/TITLE — they land in ``Contact.custom`` — and the server-side mapper never read ``note``/``urls``/``categories``/``photo`` even when present. Reads now surface what the write side persisted: - ``client/contacts.py``: new ``_first_custom`` helper pulls raw values from ``Contact.custom`` for ORG/TITLE/unencoded PHOTO. ``list_contacts`` extends its per-contact dict with org/title/note/url/categories/photo. - ``server/contacts.py``: ``_raw_contact_to_model`` maps the new keys onto ``Contact.organization`` / ``.title`` / ``.note`` / ``.urls`` / ``.categories`` / ``.photo``. URL accepts both list and plain-string shapes; categories accepts comma-separated strings for forward-compat. Coverage: - Unit: ``TestFirstCustom`` (five cases incl. bare-string library shape) and three new ``_raw_contact_to_model`` cases covering the full field set, plain-string URL, and comma-string categories. - Integration: ``test_mcp_contacts_workflow`` now decodes the ``nc_contacts_search_contacts`` response and asserts ``organization`` / ``note`` round-trip — direct regression coverage for elvisdragonmao's report on issue #716. Verified end-to-end against the local single-user docker stack: creating a contact with ``{organization, title, note, url, categories}`` and reading it back via ``nc_contacts_search_contacts`` returns every field populated. 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
04a6294cc5
commit
688a03f00a
@@ -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(
|
||||
|
||||
@@ -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."""
|
||||
|
||||
|
||||
@@ -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."""
|
||||
|
||||
Reference in New Issue
Block a user