refactor(contacts): address PR #719 follow-up review

Pulls the remaining review feedback into one commit:

- Remove the double _normalize_contact_data call: the helper now assumes
  canonical keys, and create_contact normalises before calling it
  (update_contact already did). Docstring states the invariant.
- Drop phone/organization from _SUPPORTED_CONTACT_KEYS; they never reach
  the unknown-key check post-normalisation.
- Tighten generics to dict[str, Any] / list[str] across helpers and
  ContactsClient signatures.
- Comment both URL-merge sites noting only the first URL is written.
- Log a warning when fn is missing from contact_data.
- Test coverage for _wrap_contact_field dropping value-less dicts and
  for the fn-missing warning; _vcard helper now mirrors the real call
  chain (normalise → build).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: cbcoutinho

nextcloud_mcp_server/client/contacts.py

@@ -3,6 +3,7 @@
 import logging
 import xml.etree.ElementTree as ET
 from datetime import date
+from typing import Any
 
 from pythonvCard4.vcard import Contact
 
@@ -11,16 +12,15 @@
 logger = logging.getLogger(__name__)
 
 
-# Canonical keys that _build_contact_from_data consumes. Aliases (``phone``, ``organization``)
-# are normalised to their canonical form by _normalize_contact_data before lookup.
+# Canonical keys accepted by _build_contact_from_data. Callers normalise aliases
+# (``phone``→``tel``, ``organization``→``org``) via _normalize_contact_data beforehand
+# so the set never needs to list them.
 _SUPPORTED_CONTACT_KEYS = frozenset(
     {
         "fn",
         "email",
         "tel",
-        "phone",
         "org",
-        "organization",
         "note",
         "title",
         "nickname",
@@ -31,7 +31,7 @@
 )
 
 
-def _normalize_contact_data(contact_data: dict) -> dict:
+def _normalize_contact_data(contact_data: dict[str, Any]) -> dict[str, Any]:
     """Map documented aliases to canonical keys.
 
     ``phone`` → ``tel``, ``organization`` → ``org``. The canonical key wins if both
@@ -50,16 +50,19 @@ def _normalize_contact_data(contact_data: dict) -> dict:
     return normalised
 
 
-def _wrap_contact_field(value: str | dict | list | None) -> list[dict]:
+def _wrap_contact_field(
+    value: str | dict[str, Any] | list[str | dict[str, Any]] | None,
+) -> list[dict[str, Any]]:
     """Normalize an email/tel input into pythonvCard4's list-of-dicts shape.
 
     Accepts a plain string, a dict already in ``{value, type}`` form, or a list of
-    either. Empty strings are dropped. Always returns a list (possibly empty).
+    either. Empty strings and dicts without a ``value`` key are dropped. Always
+    returns a list (possibly empty).
     """
     if value is None or value == "":
         return []
     items = value if isinstance(value, list) else [value]
-    out: list[dict] = []
+    out: list[dict[str, Any]] = []
     for item in items:
         if isinstance(item, dict) and item.get("value"):
             types = item.get("type") or ["HOME"]
@@ -69,7 +72,7 @@ def _wrap_contact_field(value: str | dict | list | None) -> list[dict]:
     return out
 
 
-def _as_str_list(value: str | list) -> list[str]:
+def _as_str_list(value: str | list[str]) -> list[str]:
     """Wrap a bare string in a list. Does NOT split on commas.
 
     Used for ORG/NICKNAME/URL where commas are part of the value (e.g.
@@ -79,7 +82,7 @@ def _as_str_list(value: str | list) -> list[str]:
     return value if isinstance(value, list) else [value]
 
 
-def _split_categories(value: str | list) -> list[str]:
+def _split_categories(value: str | list[str]) -> list[str]:
     """Normalise CATEGORIES input: a comma-separated string is split into a list.
 
     Unlike ORG/NICKNAME, CATEGORIES is canonically comma-separated in vCards
@@ -92,16 +95,25 @@ def _split_categories(value: str | list) -> list[str]:
     return [v.strip() for v in value.split(",") if v.strip()]
 
 
-def _build_contact_from_data(contact_data: dict, uid: str) -> Contact:
+def _build_contact_from_data(contact_data: dict[str, Any], uid: str) -> Contact:
     """Build a pythonvCard4 Contact from an MCP ``contact_data`` dict.
 
     Maps every key documented on ``nc_contacts_create_contact`` onto the underlying
     library, normalising shapes (list/str) to avoid pythonvCard4's char-by-char
     iteration of bare strings — see issue #716.
+
+    Callers must pre-normalise aliases via ``_normalize_contact_data`` before
+    invoking this helper; it assumes canonical keys only.
     """
-    data = _normalize_contact_data(contact_data)
+    data = contact_data
+
+    if not data.get("fn"):
+        logger.warning(
+            "contact_data missing required 'fn' field; pythonvCard4 may reject or "
+            "produce an invalid vCard"
+        )
 
-    kwargs: dict = {"fn": data.get("fn"), "uid": uid}
+    kwargs: dict[str, Any] = {"fn": data.get("fn"), "uid": uid}
 
     emails = _wrap_contact_field(data.get("email"))
     if emails:
@@ -259,11 +271,15 @@ async def delete_addressbook(self, *, name: str):
         url = f"{carddav_path}/{name}/"
         await self._make_request("DELETE", url)
 
-    async def create_contact(self, *, addressbook: str, uid: str, contact_data: dict):
+    async def create_contact(
+        self, *, addressbook: str, uid: str, contact_data: dict[str, Any]
+    ):
         """Create a new contact."""
         carddav_path = self._get_carddav_base_path()
         url = f"{carddav_path}/{addressbook}/{uid}.vcf"
 
+        # Normalise aliases here so the helper's invariant (canonical keys only) holds.
+        contact_data = _normalize_contact_data(contact_data)
         vcard = _build_contact_from_data(contact_data, uid).to_vcard()
 
         headers = {
@@ -280,7 +296,12 @@ async def delete_contact(self, *, addressbook: str, uid: str):
         await self._make_request("DELETE", url)
 
     async def update_contact(
-        self, *, addressbook: str, uid: str, contact_data: dict, etag: str = ""
+        self,
+        *,
+        addressbook: str,
+        uid: str,
+        contact_data: dict[str, Any],
+        etag: str = "",
     ):
         """Update an existing contact while preserving all existing properties."""
         carddav_path = self._get_carddav_base_path()
@@ -429,7 +450,7 @@ async def _get_raw_vcard(self, addressbook: str, uid: str) -> tuple[str, str]:
             raise
 
     def _merge_vcard_properties(
-        self, raw_vcard: str, contact_data: dict, uid: str
+        self, raw_vcard: str, contact_data: dict[str, Any], uid: str
     ) -> str:
         """Merge new contact data into existing raw vCard while preserving all properties."""
         try:
@@ -521,6 +542,9 @@ def _merge_vcard_properties(
                 elif property_name == "URL" and "url" in contact_data:
                     if "url" not in updated_properties:
                         url_value = contact_data["url"]
+                        # Only the first URL from a list is written; multi-URL
+                        # contacts are rare and this text merge doesn't attempt
+                        # position-stable mapping to existing URL lines.
                         if isinstance(url_value, list):
                             url_value = url_value[0] if url_value else ""
                         if url_value:
@@ -561,6 +585,8 @@ def _merge_vcard_properties(
                     elif key == "title":
                         updated_lines.append(f"TITLE:{value}")
                     elif key == "url":
+                        # Only the first URL is written on add-new; see note in the
+                        # update-existing branch above.
                         url_value = (
                             value[0] if isinstance(value, list) and value else value
                         )

tests/unit/client/test_contacts.py

@@ -12,14 +12,20 @@
 from nextcloud_mcp_server.client.contacts import (
     _build_contact_from_data,
     _normalize_contact_data,
+    _wrap_contact_field,
 )
 
 pytestmark = pytest.mark.unit
 
 
 def _vcard(**kwargs) -> str:
-    """Build a vCard from ``contact_data`` with a fixed uid, return the serialised text."""
-    return _build_contact_from_data(kwargs, uid="unit-test-uid").to_vcard()
+    """Build a vCard from ``contact_data`` with a fixed uid, return the serialised text.
+
+    Mirrors ``create_contact``'s real call chain: normalise aliases first, then hand
+    canonical keys to ``_build_contact_from_data``.
+    """
+    data = _normalize_contact_data(kwargs)
+    return _build_contact_from_data(data, uid="unit-test-uid").to_vcard()
 
 
 def test_issue_716_minimal_payload_keeps_all_fields():
@@ -154,6 +160,33 @@ def test_dict_form_email_preserves_custom_type():
     assert "EMAIL;TYPE=WORK:work@example.com" in vcard
 
 
+def test_wrap_field_dict_without_value_is_dropped():
+    """Dict inputs lacking the ``value`` key are silently dropped so malformed
+    payloads don't emit an EMAIL/TEL line pointing at nothing.
+    """
+    assert _wrap_contact_field({"type": ["WORK"]}) == []
+    # Mixed list: the valid entry survives, the value-less dict is omitted.
+    out = _wrap_contact_field(
+        [{"value": "ok@example.com", "type": ["WORK"]}, {"type": ["HOME"]}]
+    )
+    assert out == [{"value": "ok@example.com", "type": ["WORK"]}]
+
+
+def test_missing_fn_logs_warning(caplog):
+    """A missing ``fn`` should log a warning so operators notice malformed payloads."""
+    import logging
+
+    with caplog.at_level(
+        logging.WARNING, logger="nextcloud_mcp_server.client.contacts"
+    ):
+        try:
+            _build_contact_from_data({"email": "x@example.com"}, uid="no-fn-uid")
+        except Exception:
+            # pythonvCard4 may raise on missing fn; we only care about the warning log.
+            pass
+    assert any("fn" in r.message.lower() for r in caplog.records)
+
+
 class TestNormalizeContactData:
     """Direct tests for the alias helper — it's load-bearing for update_contact too."""