From 76e305006ceddec833fb16f900f1f98245826598 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 20 Feb 2026 09:39:46 +0100 Subject: [PATCH] fix: address PR #574 review comments Restore contact email/birthday/nickname data and per-event calendar source that were silently dropped during response model wrapping. Remove dead elif branches in OAuth deck tests, add regression tests. Co-Authored-By: Claude Opus 4.6 --- nextcloud_mcp_server/models/calendar.py | 6 + nextcloud_mcp_server/server/calendar.py | 5 + nextcloud_mcp_server/server/contacts.py | 35 ++++- .../oauth/test_oauth_deck_permissions.py | 32 ++--- tests/unit/test_response_models.py | 127 ++++++++++++++++++ 5 files changed, 176 insertions(+), 29 deletions(-) diff --git a/nextcloud_mcp_server/models/calendar.py b/nextcloud_mcp_server/models/calendar.py index fb1bf8f..3884b50 100644 --- a/nextcloud_mcp_server/models/calendar.py +++ b/nextcloud_mcp_server/models/calendar.py @@ -34,6 +34,12 @@ class CalendarEventSummary(BaseModel): status: Optional[str] = Field( None, description="Event status (CONFIRMED, TENTATIVE, CANCELLED)" ) + calendar_name: Optional[str] = Field( + None, description="Calendar containing this event" + ) + calendar_display_name: Optional[str] = Field( + None, description="Display name of calendar containing this event" + ) class CalendarEvent(CalendarEventSummary): diff --git a/nextcloud_mcp_server/server/calendar.py b/nextcloud_mcp_server/server/calendar.py index 5974264..8bf4033 100644 --- a/nextcloud_mcp_server/server/calendar.py +++ b/nextcloud_mcp_server/server/calendar.py @@ -39,6 +39,8 @@ def _event_dict_to_summary(event: dict) -> CalendarEventSummary: description=event.get("description") or None, categories=categories, status=event.get("status"), + calendar_name=event.get("calendar_name"), + calendar_display_name=event.get("calendar_display_name"), ) @@ -470,6 +472,9 @@ def configure_calendar_tools(mcp: FastMCP): end_datetime=end_datetime, limit=limit, ) + for event in cal_events: + event["calendar_name"] = calendar["name"] + event["calendar_display_name"] = calendar["display_name"] all_events.extend(cal_events) except Exception as e: logger.warning( diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index 593c913..bd51734 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -8,6 +8,7 @@ from nextcloud_mcp_server.context import get_client from nextcloud_mcp_server.models.contacts import ( AddressBook, Contact, + ContactField, ListAddressBooksResponse, ListContactsResponse, ) @@ -52,14 +53,34 @@ def configure_contacts_tools(mcp: FastMCP): """List all contacts in the specified addressbook.""" client = await get_client(ctx) contacts_data = await client.contacts.list_contacts(addressbook=addressbook) - contacts = [ - Contact( - uid=c["vcard_id"], - fn=c.get("contact", {}).get("fullname", ""), - etag=c.get("getetag"), + contacts = [] + for c in contacts_data: + contact_info = c.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)] + + # Nickname goes into custom_fields (no dedicated model field) + custom_fields: dict[str, str] = {} + nickname = contact_info.get("nickname") + if nickname: + custom_fields["nickname"] = nickname + + contacts.append( + Contact( + uid=c["vcard_id"], + fn=contact_info.get("fullname", ""), + etag=c.get("getetag"), + birthday=contact_info.get("birthday"), + emails=emails, + custom_fields=custom_fields, + ) ) - for c in contacts_data - ] return ListContactsResponse( contacts=contacts, addressbook=addressbook, total_count=len(contacts) ) diff --git a/tests/server/oauth/test_oauth_deck_permissions.py b/tests/server/oauth/test_oauth_deck_permissions.py index b5ade0e..ee444aa 100644 --- a/tests/server/oauth/test_oauth_deck_permissions.py +++ b/tests/server/oauth/test_oauth_deck_permissions.py @@ -79,12 +79,9 @@ async def test_deck_board_view_permissions( if not result.isError: response_data = json.loads(result.content[0].text) # Response is a ListBoardsResponse with a "boards" field - if isinstance(response_data, dict) and "boards" in response_data: - response_data = response_data["boards"] - elif not isinstance(response_data, list): - response_data = [response_data] if response_data else [] - board_ids = [b["id"] for b in response_data] - logger.info(f"Bob can see {len(response_data)} boards: {board_ids}") + board_list = response_data.get("boards", []) + board_ids = [b["id"] for b in board_list] + logger.info(f"Bob can see {len(board_list)} boards: {board_ids}") # Bob should see the shared board if board_id in board_ids: @@ -101,12 +98,9 @@ async def test_deck_board_view_permissions( if not result.isError: response_data = json.loads(result.content[0].text) # Response is a ListBoardsResponse with a "boards" field - if isinstance(response_data, dict) and "boards" in response_data: - response_data = response_data["boards"] - elif not isinstance(response_data, list): - response_data = [response_data] if response_data else [] - board_ids = [b["id"] for b in response_data] - logger.info(f"Diana can see {len(response_data)} boards") + board_list = response_data.get("boards", []) + board_ids = [b["id"] for b in board_list] + logger.info(f"Diana can see {len(board_list)} boards") # Diana should NOT see the board assert board_id not in board_ids, "Diana should not see board without ACL" @@ -318,11 +312,8 @@ async def test_deck_user_isolation(nc_client, alice_mcp_client, bob_mcp_client): if not result.isError: response_data = json.loads(result.content[0].text) # Response is a ListBoardsResponse with a "boards" field - if isinstance(response_data, dict) and "boards" in response_data: - response_data = response_data["boards"] - elif not isinstance(response_data, list): - response_data = [response_data] if response_data else [] - board_ids = [b["id"] for b in response_data] + board_list = response_data.get("boards", []) + board_ids = [b["id"] for b in board_list] logger.info(f"Alice can see boards: {board_ids}") # Alice should NOT see Bob's board @@ -339,11 +330,8 @@ async def test_deck_user_isolation(nc_client, alice_mcp_client, bob_mcp_client): if not result.isError: response_data = json.loads(result.content[0].text) # Response is a ListBoardsResponse with a "boards" field - if isinstance(response_data, dict) and "boards" in response_data: - response_data = response_data["boards"] - elif not isinstance(response_data, list): - response_data = [response_data] if response_data else [] - board_ids = [b["id"] for b in response_data] + board_list = response_data.get("boards", []) + board_ids = [b["id"] for b in board_list] logger.info(f"Bob can see boards: {board_ids}") # Bob should NOT see Alice's board diff --git a/tests/unit/test_response_models.py b/tests/unit/test_response_models.py index bbe44dc..272901e 100644 --- a/tests/unit/test_response_models.py +++ b/tests/unit/test_response_models.py @@ -2,6 +2,11 @@ import pytest +from nextcloud_mcp_server.models.contacts import ( + Contact, + ContactField, + ListContactsResponse, +) from nextcloud_mcp_server.models.notes import ( CreateNoteResponse, Note, @@ -267,3 +272,125 @@ def test_sampling_search_response_serialization(): assert data["model_used"] == "claude-3-5-sonnet" assert data["stop_reason"] == "maxTokens" assert data["success"] is True + + +def _map_contact(raw: dict) -> Contact: + """Replicate the mapping logic from server/contacts.py for testing.""" + contact_info = raw.get("contact", {}) + + 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)] + + custom_fields: dict[str, str] = {} + nickname = contact_info.get("nickname") + if nickname: + custom_fields["nickname"] = nickname + + return Contact( + uid=raw["vcard_id"], + fn=contact_info.get("fullname", ""), + etag=raw.get("getetag"), + birthday=contact_info.get("birthday"), + emails=emails, + custom_fields=custom_fields, + ) + + +@pytest.mark.unit +def test_contact_mapping_preserves_email_birthday_nickname(): + """Test that list_contacts mapping preserves email, birthday, and nickname. + + Regression test for PR #574: the original mapping only kept uid, fn, etag + and silently dropped email, birthday, and nickname. + """ + raw_contact = { + "vcard_id": "abc-123", + "getetag": '"etag-val"', + "contact": { + "fullname": "Jane Doe", + "email": "jane@example.com", + "birthday": "1990-05-15", + "nickname": "JD", + }, + } + + contact = _map_contact(raw_contact) + + assert contact.uid == "abc-123" + assert contact.fn == "Jane Doe" + assert contact.etag == '"etag-val"' + assert contact.birthday == "1990-05-15" + assert len(contact.emails) == 1 + assert contact.emails[0].value == "jane@example.com" + assert contact.emails[0].type == "email" + assert contact.custom_fields["nickname"] == "JD" + + +@pytest.mark.unit +def test_contact_mapping_multiple_emails(): + """Test that multiple emails are mapped correctly.""" + raw_contact = { + "vcard_id": "def-456", + "contact": { + "fullname": "John Smith", + "email": ["john@work.com", "john@home.com"], + }, + } + + contact = _map_contact(raw_contact) + + assert len(contact.emails) == 2 + assert contact.emails[0].value == "john@work.com" + assert contact.emails[1].value == "john@home.com" + + +@pytest.mark.unit +def test_contact_mapping_missing_optional_fields(): + """Test mapping when email, birthday, and nickname are absent.""" + raw_contact = { + "vcard_id": "ghi-789", + "contact": {"fullname": "No Details"}, + } + + contact = _map_contact(raw_contact) + + assert contact.uid == "ghi-789" + assert contact.fn == "No Details" + assert contact.birthday is None + assert contact.emails == [] + assert contact.custom_fields == {} + + +@pytest.mark.unit +def test_list_contacts_response_wraps_contacts(): + """Test ListContactsResponse wraps contacts correctly for MCP output.""" + contacts = [ + _map_contact( + { + "vcard_id": "a", + "getetag": '"e1"', + "contact": { + "fullname": "Alice", + "email": "alice@test.com", + "birthday": "2000-01-01", + "nickname": "Ali", + }, + } + ), + ] + + response = ListContactsResponse( + contacts=contacts, addressbook="personal", total_count=1 + ) + + data = response.model_dump() + assert data["total_count"] == 1 + assert len(data["contacts"]) == 1 + c = data["contacts"][0] + assert c["birthday"] == "2000-01-01" + assert c["emails"][0]["value"] == "alice@test.com" + assert c["custom_fields"]["nickname"] == "Ali"