Merge pull request #719 from cbcoutinho/fix/contacts-create-dropped-fields-716

fix(contacts): persist all documented fields on create (fixes #716)

tests/client/contacts/test_contacts_operations.py

@@ -86,3 +86,47 @@ async def test_full_contact_workflow(
     contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name)
     contact_uids = [c["vcard_id"] for c in contacts]
     assert contact_uid not in contact_uids
+
+
+async def test_create_contact_persists_all_documented_fields(
+    nc_client: NextcloudClient, temporary_addressbook: str
+):
+    """Regression for issue #716: org/note/phone/organization must persist to the vCard.
+
+    Historically ``create_contact`` only handled fn/email/tel and silently dropped every
+    other key. Inspect the raw server-side vCard (not just the parsed list response) to
+    confirm each documented field round-trips.
+    """
+    addressbook_name = temporary_addressbook
+    contact_uid = f"test-full-{uuid.uuid4().hex[:8]}"
+    contact_data = {
+        "fn": "Full Field User",
+        "email": "full@example.com",
+        "phone": "555-0716",  # alias for tel
+        "organization": "Acme Corp",  # alias for org
+        "note": "Issue 716 regression",
+        "title": "Engineer",
+        "url": "https://example.com",
+    }
+
+    await nc_client.contacts.create_contact(
+        addressbook=addressbook_name,
+        uid=contact_uid,
+        contact_data=contact_data,
+    )
+    try:
+        raw_vcard, _etag = await nc_client.contacts._get_raw_vcard(
+            addressbook_name, contact_uid
+        )
+        assert "FN:Full Field User" in raw_vcard
+        assert "EMAIL" in raw_vcard and "full@example.com" in raw_vcard
+        assert "TEL" in raw_vcard and "555-0716" in raw_vcard
+        assert "ORG:Acme Corp" in raw_vcard
+        assert "NOTE:Issue 716 regression" in raw_vcard
+        assert "TITLE:Engineer" in raw_vcard
+        # Sabre rewrites bare URL: to URL;VALUE=URI: on PUT
+        assert "URL" in raw_vcard and "https://example.com" in raw_vcard
+    finally:
+        await nc_client.contacts.delete_contact(
+            addressbook=addressbook_name, uid=contact_uid
+        )

tests/server/test_contacts_mcp.py

@@ -24,6 +24,9 @@ async def test_mcp_contacts_workflow(
         "fn": f"MCP Contact {unique_suffix}",
         "email": f"mcp.contact.{unique_suffix}@example.com",
         "tel": "1234567890",
+        # Regression for issue #716 — these were silently dropped before
+        "organization": "MCP Test Corp",
+        "note": f"Created by test {unique_suffix}",
     }
 
     try:
@@ -51,9 +54,31 @@ async def test_mcp_contacts_workflow(
         )
         assert create_c_result.isError is False
 
-        # 4. Verify contact creation
+        # 4. Verify contact creation (and that all fields — #716 — actually persisted)
         contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name)
-        assert any(c["vcard_id"] == contact_uid for c in contacts)
+        created = next((c for c in contacts if c["vcard_id"] == contact_uid), None)
+        assert created is not None
+        raw_vcard = created.get("addressdata", "")
+        assert "ORG:MCP Test Corp" in raw_vcard
+        assert f"NOTE:Created by test {unique_suffix}" in raw_vcard
+
+        # 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(
+            "nc_contacts_update_contact",
+            {
+                "addressbook": addressbook_name,
+                "uid": contact_uid,
+                "contact_data": {"url": "https://mcp-test.example.com"},
+            },
+        )
+        assert update_result.isError is False
+        contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name)
+        updated = next(c for c in contacts if c["vcard_id"] == contact_uid)
+        updated_vcard = updated.get("addressdata", "")
+        assert "mcp-test.example.com" in updated_vcard
+        # Prior properties must not have been clobbered by the merge.
+        assert "ORG:MCP Test Corp" in updated_vcard
 
         # 5. Delete contact via MCP
         logger.info("Deleting contact %s via MCP", contact_uid)

tests/unit/client/test_contacts.py

@@ -0,0 +1,411 @@
+"""Unit tests for the contacts client vCard builder.
+
+These exercise ``_build_contact_from_data`` in isolation — no HTTP, no fixtures —
+so they cover the issue #716 regression surface and the edge cases flagged in
+PR #719 review without standing up the compose stack.
+"""
+
+from datetime import date
+
+import pytest
+
+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.
+
+    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():
+    """Reporter's exact payload from issue #716: every field must survive."""
+    vcard = _vcard(
+        fn="Repro User",
+        email="repro@example.com",
+        phone="555-0716",
+        organization="Acme Corp",
+        note="Issue 716",
+    )
+    assert "FN:Repro User" in vcard
+    assert "EMAIL" in vcard and "repro@example.com" in vcard
+    assert "TEL" in vcard and "555-0716" in vcard
+    assert "ORG:Acme Corp" in vcard
+    assert "NOTE:Issue 716" in vcard
+
+
+def test_org_preserves_comma_in_company_name():
+    """Regression: ``_as_list`` used to comma-split ORG, mangling names like
+    "Smith, Jones & Associates" into a two-component ORG. After the fix the whole
+    string is a single ORG component (with the comma RFC-6350-escaped as ``\\,``).
+    """
+    vcard = _vcard(fn="Alice", organization="Smith, Jones & Associates")
+    org_line = next(line for line in vcard.splitlines() if line.startswith("ORG"))
+    # Single component: no unescaped semicolon separator.
+    payload = org_line.split(":", 1)[1]
+    assert ";" not in payload
+    # Comma is escaped per RFC 6350 but the logical value is preserved.
+    assert payload.replace(r"\,", ",") == "Smith, Jones & Associates"
+
+
+def test_org_list_input_produces_structured_org():
+    """A list input is the opt-in shape for multi-component ORG (Company;Department)."""
+    vcard = _vcard(fn="Alice", org=["Acme", "Engineering"])
+    assert "ORG:Acme;Engineering" in vcard
+
+
+def test_invalid_bday_is_dropped_not_raised(caplog):
+    """An unparseable BDAY must warn and be omitted, not crash the call."""
+    import logging
+
+    with caplog.at_level(
+        logging.WARNING, logger="nextcloud_mcp_server.client.contacts"
+    ):
+        vcard = _vcard(fn="Alice", bday="not-a-date")
+    assert "BDAY" not in vcard
+    assert any("bday" in r.message.lower() for r in caplog.records)
+
+
+def test_valid_iso_bday_is_persisted():
+    vcard = _vcard(fn="Alice", bday="1990-05-01")
+    assert "BDAY:1990-05-01" in vcard
+
+
+def test_date_object_bday_is_persisted():
+    vcard = _vcard(fn="Alice", bday=date(1985, 12, 24))
+    assert "BDAY:1985-12-24" in vcard
+
+
+def test_tel_takes_precedence_over_phone_alias():
+    """When the caller supplies both canonical and alias, canonical wins. Documents
+    the precedence so future callers aren't surprised.
+    """
+    vcard = _vcard(fn="Alice", tel="111-1111", phone="222-2222")
+    assert "111-1111" in vcard
+    assert "222-2222" not in vcard
+
+
+def test_organization_alias_fills_in_when_org_absent():
+    vcard = _vcard(fn="Alice", organization="Acme")
+    assert "ORG:Acme" in vcard
+
+
+def test_categories_string_is_split_on_commas():
+    vcard = _vcard(fn="Alice", categories="friends,work,vip")
+    cat_line = next(
+        line for line in vcard.splitlines() if line.startswith("CATEGORIES")
+    )
+    assert cat_line == "CATEGORIES:friends,work,vip"
+
+
+def test_categories_list_passes_through_unchanged():
+    """A caller that already supplied a list shouldn't have their entries split again
+    — ``["friends,work"]`` stays as one item (with the comma RFC-6350-escaped), not
+    two categories ``friends`` + ``work``.
+    """
+    vcard = _vcard(fn="Alice", categories=["friends,work"])
+    cat_line = next(
+        line for line in vcard.splitlines() if line.startswith("CATEGORIES")
+    )
+    payload = cat_line.split(":", 1)[1]
+    assert payload == r"friends\,work"  # one item, comma escaped
+
+
+def test_nickname_bare_string_is_not_char_iterated():
+    """Regression: pythonvCard4 iterates bare strings; we wrap to prevent that."""
+    vcard = _vcard(fn="Alice", nickname="Bob")
+    nick_line = next(line for line in vcard.splitlines() if line.startswith("NICKNAME"))
+    assert nick_line == "NICKNAME:Bob"
+
+
+def test_url_bare_string_is_not_char_iterated():
+    vcard = _vcard(fn="Alice", url="https://example.com")
+    # Must appear as a single URL, not one URL: per character.
+    url_lines = [line for line in vcard.splitlines() if line.startswith("URL")]
+    assert url_lines == ["URL:https://example.com"]
+
+
+def test_unknown_keys_are_ignored_without_error(caplog):
+    """Future-compat: callers sending unknown keys shouldn't blow up."""
+    import logging
+
+    with caplog.at_level(logging.DEBUG, logger="nextcloud_mcp_server.client.contacts"):
+        vcard = _vcard(fn="Alice", totally_made_up_field="ignored")
+    assert "FN:Alice" in vcard
+    assert "totally_made_up_field" not in vcard
+    # A debug log is expected but not required — main guarantee is that no exception is raised.
+
+
+def test_empty_email_is_skipped():
+    """An empty string for email must not emit an EMAIL: line."""
+    vcard = _vcard(fn="Alice", email="")
+    assert "EMAIL" not in vcard
+
+
+def test_dict_form_email_preserves_custom_type():
+    vcard = _vcard(
+        fn="Alice",
+        email={"value": "work@example.com", "type": ["WORK"]},
+    )
+    assert "EMAIL;TYPE=WORK:work@example.com" in vcard
+
+
+def test_dict_form_email_with_bare_string_type():
+    """Regression: a dict with ``type="WORK"`` (bare string) used to be
+    char-iterated into ``["W","O","R","K"]`` because of an unguarded ``list()``
+    call inside ``_wrap_contact_field``.
+    """
+    vcard = _vcard(
+        fn="Alice",
+        email={"value": "work@example.com", "type": "WORK"},
+    )
+    assert "EMAIL;TYPE=WORK:work@example.com" in vcard
+    # The bug would emit something like ``EMAIL;TYPE=W,O,R,K:`` — guard against it.
+    assert "TYPE=W," not 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."""
+
+    def test_phone_maps_to_tel(self):
+        assert _normalize_contact_data({"phone": "123"}) == {"tel": "123"}
+
+    def test_organization_maps_to_org(self):
+        assert _normalize_contact_data({"organization": "Acme"}) == {"org": "Acme"}
+
+    def test_canonical_wins_when_both_present(self):
+        """Caller intent: they set ``tel`` deliberately. A stray ``phone`` entry
+        must not clobber the canonical value.
+        """
+        out = _normalize_contact_data({"tel": "canonical", "phone": "alias"})
+        assert out == {"tel": "canonical"}
+
+    def test_does_not_mutate_input(self):
+        original = {"phone": "123", "organization": "Acme"}
+        _normalize_contact_data(original)
+        assert original == {"phone": "123", "organization": "Acme"}
+
+    def test_passthrough_for_unknown_keys(self):
+        assert _normalize_contact_data({"foo": "bar"}) == {"foo": "bar"}
+
+
+class TestMergeVcardProperties:
+    """Direct tests for ``_merge_vcard_properties`` — the primary update path.
+
+    Written in response to PR #719 review claiming NICKNAME/BDAY/CATEGORIES are not
+    updatable via this function. These tests pin the actual behaviour so future
+    regressions (or claims) can be answered in one line.
+    """
+
+    @staticmethod
+    def _merge(raw: str, data: dict) -> str:
+        from nextcloud_mcp_server.client.contacts import ContactsClient
+
+        client = ContactsClient.__new__(ContactsClient)  # no HTTP / no __init__
+        return client._merge_vcard_properties(raw, data, uid="merge-test")
+
+    def test_nickname_overwrites_existing_line(self):
+        """Existing NICKNAME must be replaced with the new value, not preserved."""
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nNICKNAME:Bob\nEND:VCARD\n"
+        result = self._merge(existing, {"nickname": "Robert"})
+        assert "NICKNAME:Robert" in result
+        assert "NICKNAME:Bob" not in result
+
+    def test_bday_overwrites_existing_line(self):
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nBDAY:1990-05-01\nEND:VCARD\n"
+        result = self._merge(existing, {"bday": "1991-06-02"})
+        assert "BDAY:1991-06-02" in result
+        assert "BDAY:1990-05-01" not in result
+
+    def test_categories_overwrites_existing_line(self):
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nCATEGORIES:old,stale\nEND:VCARD\n"
+        result = self._merge(existing, {"categories": ["vip", "new"]})
+        assert "CATEGORIES:vip,new" in result
+        assert "old,stale" not in result
+
+    def test_nickname_added_when_not_in_existing_vcard(self):
+        """If the existing vCard has no NICKNAME line, update must append one."""
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n"
+        result = self._merge(existing, {"nickname": "Bob"})
+        assert "NICKNAME:Bob" in result
+
+    def test_bday_added_when_not_in_existing_vcard(self):
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n"
+        result = self._merge(existing, {"bday": "1990-05-01"})
+        assert "BDAY:1990-05-01" in result
+
+    def test_categories_added_when_not_in_existing_vcard(self):
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n"
+        result = self._merge(existing, {"categories": "a,b,c"})
+        assert "CATEGORIES:a,b,c" in result
+
+    def test_url_update_preserves_unrelated_properties(self):
+        """A URL update must not clobber ORG / NOTE / TEL from the existing vCard."""
+        existing = (
+            "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n"
+            "ORG:Acme\nTEL:555-1234\nNOTE:keep me\nEND:VCARD\n"
+        )
+        result = self._merge(existing, {"url": "https://example.com"})
+        assert "URL:https://example.com" in result
+        assert "ORG:Acme" in result
+        assert "TEL:555-1234" in result
+        assert "NOTE:keep me" in result
+
+    def test_dict_email_input_preserves_existing_line(self):
+        """Regression: a dict-form email on update used to consume the existing
+        EMAIL: line and write nothing, silently deleting the contact's email.
+        Now the original line is preserved when the input shape isn't a plain str.
+        """
+        existing = (
+            "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n"
+            "EMAIL;TYPE=HOME:alice@example.com\nEND:VCARD\n"
+        )
+        result = self._merge(
+            existing, {"email": {"value": "work@example.com", "type": ["WORK"]}}
+        )
+        assert "EMAIL;TYPE=HOME:alice@example.com" in result
+
+    def test_list_tel_input_preserves_existing_line(self):
+        """Same regression as the email branch but for TEL — a list-shaped tel
+        input must not silently drop the existing phone number.
+        """
+        existing = (
+            "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n"
+            "TEL;TYPE=HOME:555-0001\nEND:VCARD\n"
+        )
+        result = self._merge(
+            existing, {"tel": [{"value": "555-9999", "type": ["WORK"]}]}
+        )
+        assert "TEL;TYPE=HOME:555-0001" in result
+
+    def test_invalid_bday_on_update_preserves_existing_line(self):
+        """A non-ISO BDAY string must not produce a malformed vCard line on update.
+        We share validation with the create path; invalid → keep the existing line.
+        """
+        existing = (
+            "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\n"
+            "BDAY:1990-05-01\nEND:VCARD\n"
+        )
+        result = self._merge(existing, {"bday": "not-a-date"})
+        assert "BDAY:1990-05-01" in result
+        assert "BDAY:not-a-date" not in result
+
+    def test_invalid_bday_on_add_new_is_dropped(self):
+        """No existing BDAY + invalid input → no BDAY line appended (vs. raw write)."""
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n"
+        result = self._merge(existing, {"bday": "not-a-date"})
+        assert "BDAY" not in result
+
+    def test_newline_in_note_does_not_inject_property(self):
+        """Regression: a literal newline in a value must not terminate the line
+        and inject a fresh vCard property.
+        """
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n"
+        result = self._merge(
+            existing, {"note": "harmless\nEMAIL:attacker@evil.example"}
+        )
+        # The injected property must not appear as a real EMAIL line.
+        lines = result.splitlines()
+        assert "EMAIL:attacker@evil.example" not in lines
+        # The note value is preserved with newlines escaped per RFC 6350.
+        assert any(line.startswith("NOTE:") and "\\n" in line for line in lines)
+
+    def test_list_org_overwrites_with_semicolon_join(self):
+        """Regression: list-form ORG used to fall through ``_safe_vcard_value``
+        unchanged and emit a Python ``repr`` like ``ORG:['Acme', 'Engineering']``.
+        Per RFC 6350 §6.6.4 components are ``;``-separated.
+        """
+        existing = (
+            "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nORG:OldCo\nEND:VCARD\n"
+        )
+        result = self._merge(existing, {"org": ["Acme", "Engineering"]})
+        assert "ORG:Acme;Engineering" in result
+        assert "ORG:OldCo" not in result
+        assert "[" not in result and "'Acme'" not in result
+
+    def test_list_org_added_with_semicolon_join(self):
+        """Add-new branch: list ORG without an existing line still serialises
+        with ``;`` rather than as a Python list repr.
+        """
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n"
+        result = self._merge(existing, {"org": ["Acme", "Engineering"]})
+        assert "ORG:Acme;Engineering" in result
+        assert "[" not in result
+
+    def test_dict_email_on_no_existing_line_warns(self, caplog):
+        """No existing EMAIL + dict input is a known limitation of the text-merge
+        path. Surface it as a warning so the silent no-op is at least observable.
+        """
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n"
+        with caplog.at_level("WARNING"):
+            result = self._merge(
+                existing, {"email": {"value": "alice@work.com", "type": ["WORK"]}}
+            )
+        # The dict input is not applied; no EMAIL line is added.
+        assert "EMAIL" not in result
+        # A warning specifically calls out the dict/list shape and recommends
+        # plain str / create_contact as alternatives.
+        assert any(
+            "email" in r.message and "dict/list shape" in r.message
+            for r in caplog.records
+        )
+
+    def test_list_tel_on_no_existing_line_warns(self, caplog):
+        """Same warning behaviour for TEL — list input on a contact without an
+        existing TEL line must not silently disappear.
+        """
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n"
+        with caplog.at_level("WARNING"):
+            result = self._merge(
+                existing, {"tel": [{"value": "555-9999", "type": ["WORK"]}]}
+            )
+        assert "TEL" not in result
+        assert any(
+            "tel" in r.message and "dict/list shape" in r.message
+            for r in caplog.records
+        )
+
+    def test_str_email_does_not_warn(self, caplog):
+        """Plain string email is the supported shape — no warning should fire."""
+        existing = "BEGIN:VCARD\nVERSION:3.0\nUID:merge-test\nFN:Alice\nEND:VCARD\n"
+        with caplog.at_level("WARNING"):
+            result = self._merge(existing, {"email": "alice@work.com"})
+        assert "EMAIL:alice@work.com" in result
+        assert not any("dict/list shape" in r.message for r in caplog.records)