From 746abba18c24e6b6af7a7e5c82b8246188557b3d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Mon, 8 Jun 2026 01:52:54 +0200 Subject: [PATCH] fix(contacts): address PR #876 round-2 nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop the redundant `.rstrip("/")` in `_list_object_names`; the `endswith("/")` guard already excludes the collection entry. - Remove the now-unused `_get_raw_vcard` (update_contact resolves the name itself and calls `_fetch_raw_vcard` directly). Its only remaining caller — the create→read integration test — now calls `_fetch_raw_vcard` with the deterministic `.vcf`, saving a redundant PROPFIND. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/client/contacts.py | 21 ++++++++----------- .../contacts/test_contacts_operations.py | 6 ++++-- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index f4eb1678..0a0c9e31 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -249,10 +249,11 @@ class ContactsClient(BaseNextcloudClient): if href is None or not href.text: continue # The collection itself is reported with a trailing slash; skip it - # so only contact objects remain. + # so only contact objects remain. The guard means href.text never + # ends with "/" below, so the bare split is sufficient. if href.text.endswith("/"): continue - names.append(href.text.rstrip("/").split("/")[-1]) + names.append(href.text.split("/")[-1]) return names async def _resolve_object_name(self, addressbook: str, uid: str) -> str | None: @@ -578,19 +579,15 @@ class ContactsClient(BaseNextcloudClient): logger.debug("Found %s contacts", len(contacts)) return contacts - async def _get_raw_vcard(self, addressbook: str, uid: str) -> tuple[str, str]: - """Get raw vCard content for a contact without parsing. - - Resolves the real object filename first (it may not be ``.vcf`` — - issue #874) before fetching. - """ - object_name = await self._resolve_object_name(addressbook, uid) or f"{uid}.vcf" - return await self._fetch_raw_vcard(addressbook, object_name) - async def _fetch_raw_vcard( self, addressbook: str, object_name: str ) -> tuple[str, str]: - """Fetch raw vCard content + etag for an already-resolved object name.""" + """Fetch raw vCard content + etag for an already-resolved object name. + + Callers that only have a surfaced ``uid`` (not the real object name) + must resolve it first via ``_resolve_object_name`` — see issue #874. + ``update_contact`` does exactly that and passes the resolved name here. + """ carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{addressbook}/{object_name}" diff --git a/tests/client/contacts/test_contacts_operations.py b/tests/client/contacts/test_contacts_operations.py index d55f01a0..687772d2 100644 --- a/tests/client/contacts/test_contacts_operations.py +++ b/tests/client/contacts/test_contacts_operations.py @@ -167,8 +167,10 @@ async def test_create_contact_persists_all_documented_fields( contact_data=contact_data, ) try: - raw_vcard, _etag = await nc_client.contacts._get_raw_vcard( - addressbook_name, contact_uid + # create_contact always writes .vcf, so fetch that object directly + # (no PROPFIND resolution needed for a contact we just created). + raw_vcard, _etag = await nc_client.contacts._fetch_raw_vcard( + addressbook_name, f"{contact_uid}.vcf" ) assert "FN:Full Field User" in raw_vcard assert "EMAIL" in raw_vcard and "full@example.com" in raw_vcard