Merge pull request #602 from cbcoutinho/fix/contacts-vcard-dict-format-601

fix: handle pythonvCard4 dict-format fields and missing phones (#601)
2026-03-03 09:33:27 +01:00
parent 184415eca1 fdb7b87baf
commit 0f8b1c6325
3 changed files with 218 additions and 15 deletions
@@ -274,6 +274,7 @@ class ContactsClient(BaseNextcloudClient):
                        "nickname": contact.nickname,
                        "birthday": contact.bday,
                        "email": contact.email,
+                        "tel": contact.tel,
                    },
                    "addressdata": addressdata,
                }
@@ -18,23 +18,60 @@ from nextcloud_mcp_server.observability.metrics import instrument_tool
 logger = logging.getLogger(__name__)


+def _parse_vcard_fields(
+    raw_values: str | dict | list | None, field_type: str
+) -> list[ContactField]:
+    """Parse polymorphic vCard field data into a list of ContactField.
+
+    pythonvCard4 returns field values in several shapes:
+    - ``str``  – plain value, e.g. ``"alice@example.com"``
+    - ``dict`` – ``{'value': '...', 'type': ['HOME', 'PREF']}``
+    - ``list`` – a list whose items are any of the above
+
+    The ``PREF`` type parameter is treated as a *preferred* flag rather than a
+    label.  All other type values are lowercased and joined with ``", "``.
+    """
+    if raw_values is None:
+        return []
+
+    items: list[str | dict] = (
+        raw_values if isinstance(raw_values, list) else [raw_values]
+    )
+
+    fields: list[ContactField] = []
+    for item in items:
+        if isinstance(item, dict):
+            value = str(item.get("value", ""))
+            if not value:
+                continue
+            raw_types: list[str] = item.get("type") or []
+            preferred = any(t.upper() == "PREF" for t in raw_types)
+            labels = [t.lower() for t in raw_types if t.upper() != "PREF"]
+            fields.append(
+                ContactField(
+                    type=field_type,
+                    value=value,
+                    label=", ".join(labels) if labels else None,
+                    preferred=preferred,
+                )
+            )
+        elif isinstance(item, str) and item:
+            fields.append(ContactField(type=field_type, value=item))
+
+    return fields
+
+
 def _raw_contact_to_model(raw: dict) -> Contact:
    """Convert a raw contact dict from the contacts client to a Contact model.

-    Only maps fields the client's list_contacts() currently returns:
-    fullname, nickname, birthday, and email. Additional Contact model fields
-    (phones, addresses, organization, etc.) require expanding the client's
-    vCard parsing in ContactsClient.list_contacts().
+    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`.
    """
    contact_info = raw.get("contact", {})

-    # Convert email field (str, list, or None) to list[ContactField]
-    raw_email = contact_info.get("email")
-    emails: list[ContactField] = []
-    if isinstance(raw_email, list):
-        emails = [ContactField(type="email", value=e) for e in raw_email if e]
-    elif isinstance(raw_email, str) and raw_email:
-        emails = [ContactField(type="email", value=raw_email)]
+    emails = _parse_vcard_fields(contact_info.get("email"), "email")
+    phones = _parse_vcard_fields(contact_info.get("tel"), "phone")

    # Nickname goes into custom_fields (no dedicated model field)
    custom_fields: dict[str, Any] = {}
@@ -48,6 +85,7 @@ def _raw_contact_to_model(raw: dict) -> Contact:
        etag=raw.get("getetag"),
        birthday=contact_info.get("birthday"),
        emails=emails,
+        phones=phones,
        custom_fields=custom_fields,
    )

@@ -87,7 +125,13 @@ def configure_contacts_tools(mcp: FastMCP):
    async def nc_contacts_list_contacts(
        ctx: Context, *, addressbook: str
    ) -> ListContactsResponse:
-        """List all contacts in the specified addressbook."""
+        """List all contacts in the specified addressbook.
+
+        Args:
+            addressbook: The URI slug of the addressbook (e.g. "contacts"),
+                not the display name. Use nc_contacts_list_addressbooks to
+                find available URI slugs.
+        """
        client = await get_client(ctx)
        contacts_data = await client.contacts.list_contacts(addressbook=addressbook)
        contacts = [_raw_contact_to_model(c) for c in contacts_data]
@@ -140,7 +184,9 @@ def configure_contacts_tools(mcp: FastMCP):
        """Create a new contact.

        Args:
-            addressbook: The name of the addressbook to create the contact in.
+            addressbook: The URI slug of the addressbook (e.g. "contacts"),
+                not the display name. Use nc_contacts_list_addressbooks to
+                find available URI slugs.
            uid: The unique ID for the contact.
            contact_data: A dictionary with the contact's details, e.g. {"fn": "John Doe", "email": "john.doe@example.com"}.
        """
@@ -158,7 +204,14 @@ def configure_contacts_tools(mcp: FastMCP):
    @require_scopes("contacts:write")
    @instrument_tool
    async def nc_contacts_delete_contact(ctx: Context, *, addressbook: str, uid: str):
-        """Delete a contact."""
+        """Delete a contact.
+
+        Args:
+            addressbook: The URI slug of the addressbook (e.g. "contacts"),
+                not the display name. Use nc_contacts_list_addressbooks to
+                find available URI slugs.
+            uid: The unique ID of the contact to delete.
+        """
        client = await get_client(ctx)
        return await client.contacts.delete_contact(addressbook=addressbook, uid=uid)

@@ -174,7 +227,9 @@ def configure_contacts_tools(mcp: FastMCP):
        """Update an existing contact while preserving all existing properties.

        Args:
-            addressbook: The name of the addressbook containing the contact.
+            addressbook: The URI slug of the addressbook (e.g. "contacts"),
+                not the display name. Use nc_contacts_list_addressbooks to
+                find available URI slugs.
            uid: The unique ID of the contact to update.
            contact_data: A dictionary with the contact's updated details, e.g. {"fn": "Jane Doe", "email": "jane.doe@example.com"}.
            etag: Optional ETag for optimistic concurrency control.
@@ -376,6 +376,153 @@ def test_list_contacts_response_wraps_contacts():
    assert c["custom_fields"]["nickname"] == "Ali"


+@pytest.mark.unit
+def test_contact_mapping_dict_format_emails():
+    """Regression for #601: pythonvCard4 returns dicts, not plain strings."""
+    raw_contact = {
+        "vcard_id": "dict-email-1",
+        "contact": {
+            "fullname": "Evrim Yilmaz",
+            "email": [
+                {"value": "evrim@example.com", "type": ["HOME"]},
+                {"value": "evrim@work.com", "type": ["WORK"]},
+            ],
+        },
+    }
+
+    contact = _map_contact(raw_contact)
+
+    assert len(contact.emails) == 2
+    assert contact.emails[0].value == "evrim@example.com"
+    assert contact.emails[0].label == "home"
+    assert contact.emails[1].value == "evrim@work.com"
+    assert contact.emails[1].label == "work"
+
+
+@pytest.mark.unit
+def test_contact_mapping_dict_format_phones():
+    """Phones from dict-format tel field are parsed into Contact.phones."""
+    raw_contact = {
+        "vcard_id": "dict-tel-1",
+        "contact": {
+            "fullname": "Phone User",
+            "tel": [
+                {"value": "+1-555-0100", "type": ["CELL"]},
+                {"value": "+1-555-0200", "type": ["WORK", "VOICE"]},
+            ],
+        },
+    }
+
+    contact = _map_contact(raw_contact)
+
+    assert len(contact.phones) == 2
+    assert contact.phones[0].value == "+1-555-0100"
+    assert contact.phones[0].type == "phone"
+    assert contact.phones[0].label == "cell"
+    assert contact.phones[1].value == "+1-555-0200"
+    assert contact.phones[1].label == "work, voice"
+
+
+@pytest.mark.unit
+def test_contact_mapping_pref_flag_extraction():
+    """PREF type is extracted as preferred=True, not included in labels."""
+    raw_contact = {
+        "vcard_id": "pref-1",
+        "contact": {
+            "fullname": "Pref User",
+            "email": [
+                {"value": "pref@example.com", "type": ["HOME", "PREF"]},
+                {"value": "other@example.com", "type": ["WORK"]},
+            ],
+            "tel": [
+                {"value": "+1-555-0001", "type": ["pref", "CELL"]},
+            ],
+        },
+    }
+
+    contact = _map_contact(raw_contact)
+
+    assert contact.emails[0].preferred is True
+    assert contact.emails[0].label == "home"  # PREF stripped from label
+    assert contact.emails[1].preferred is False
+    assert contact.primary_email == "pref@example.com"
+
+    assert contact.phones[0].preferred is True
+    assert contact.phones[0].label == "cell"
+    assert contact.primary_phone == "+1-555-0001"
+
+
+@pytest.mark.unit
+def test_contact_mapping_backward_compat_plain_strings():
+    """Plain string emails/phones still work (backward compatibility)."""
+    raw_contact = {
+        "vcard_id": "compat-1",
+        "contact": {
+            "fullname": "Plain String",
+            "email": "plain@example.com",
+            "tel": "+1-555-9999",
+        },
+    }
+
+    contact = _map_contact(raw_contact)
+
+    assert len(contact.emails) == 1
+    assert contact.emails[0].value == "plain@example.com"
+    assert contact.emails[0].label is None
+    assert contact.emails[0].preferred is False
+
+    assert len(contact.phones) == 1
+    assert contact.phones[0].value == "+1-555-9999"
+
+
+@pytest.mark.unit
+def test_contact_mapping_empty_type_list():
+    """Dict with empty or missing type list produces no label."""
+    raw_contact = {
+        "vcard_id": "empty-type-1",
+        "contact": {
+            "fullname": "No Type",
+            "email": {"value": "notype@example.com", "type": []},
+        },
+    }
+
+    contact = _map_contact(raw_contact)
+
+    assert len(contact.emails) == 1
+    assert contact.emails[0].value == "notype@example.com"
+    assert contact.emails[0].label is None
+    assert contact.emails[0].preferred is False
+
+
+@pytest.mark.unit
+def test_contact_mapping_multiple_dict_emails_with_labels():
+    """Multiple dict-format emails preserve individual labels."""
+    raw_contact = {
+        "vcard_id": "multi-label-1",
+        "contact": {
+            "fullname": "Multi Label",
+            "email": [
+                {"value": "home@example.com", "type": ["HOME", "PREF"]},
+                {"value": "work@example.com", "type": ["WORK"]},
+                {"value": "other@example.com"},
+            ],
+        },
+    }
+
+    contact = _map_contact(raw_contact)
+
+    assert len(contact.emails) == 3
+    assert contact.emails[0].value == "home@example.com"
+    assert contact.emails[0].label == "home"
+    assert contact.emails[0].preferred is True
+    assert contact.emails[1].value == "work@example.com"
+    assert contact.emails[1].label == "work"
+    assert contact.emails[1].preferred is False
+    assert contact.emails[2].value == "other@example.com"
+    assert contact.emails[2].label is None
+    assert contact.primary_email == "home@example.com"
+
+
 # ============= _event_dict_to_summary tests =============