fix(contacts): address PR #876 round-1 review
- Use str.removesuffix(".vcf") instead of str.replace(".vcf", "") in both
_resolve_object_name and list_contacts so a filename like "alice.vcf.backup"
isn't mangled; the two transforms stay consistent to preserve the
surface-then-resolve round-trip.
- Add update_contact resolution tests mirroring the delete coverage:
targets the real no-extension path, and falls back to <uid>.vcf when
resolution finds nothing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
854ef349cd
commit
011356cca2
@@ -258,7 +258,7 @@ class ContactsClient(BaseNextcloudClient):
|
|||||||
async def _resolve_object_name(self, addressbook: str, uid: str) -> str | None:
|
async def _resolve_object_name(self, addressbook: str, uid: str) -> str | None:
|
||||||
"""Map a surfaced contact id back to its real CardDAV object filename.
|
"""Map a surfaced contact id back to its real CardDAV object filename.
|
||||||
|
|
||||||
``list_contacts`` surfaces ``vcard_id`` with any ``.vcf`` suffix
|
``list_contacts`` surfaces ``vcard_id`` with a trailing ``.vcf`` suffix
|
||||||
stripped, so reverse that transform here: return the object whose
|
stripped, so reverse that transform here: return the object whose
|
||||||
filename reduces to ``uid``. The conventional ``<uid>.vcf`` is
|
filename reduces to ``uid``. The conventional ``<uid>.vcf`` is
|
||||||
preferred when present (deterministic for the common case), otherwise
|
preferred when present (deterministic for the common case), otherwise
|
||||||
@@ -267,7 +267,7 @@ class ContactsClient(BaseNextcloudClient):
|
|||||||
candidates = [
|
candidates = [
|
||||||
name
|
name
|
||||||
for name in await self._list_object_names(addressbook)
|
for name in await self._list_object_names(addressbook)
|
||||||
if name.replace(".vcf", "") == uid
|
if name.removesuffix(".vcf") == uid
|
||||||
]
|
]
|
||||||
if not candidates:
|
if not candidates:
|
||||||
return None
|
return None
|
||||||
@@ -512,7 +512,9 @@ class ContactsClient(BaseNextcloudClient):
|
|||||||
continue
|
continue
|
||||||
# ``vcard_id`` keeps the historical ``.vcf``-stripped form for
|
# ``vcard_id`` keeps the historical ``.vcf``-stripped form for
|
||||||
# backward compatibility with callers that use it as the contact id.
|
# backward compatibility with callers that use it as the contact id.
|
||||||
vcard_id = object_name.replace(".vcf", "")
|
# Must use the same trailing-suffix strip as ``_resolve_object_name``
|
||||||
|
# so the surface-then-resolve round-trip stays lossless (issue #874).
|
||||||
|
vcard_id = object_name.removesuffix(".vcf")
|
||||||
|
|
||||||
# Get properties
|
# Get properties
|
||||||
propstat = response_elem.find(".//d:propstat", ns)
|
propstat = response_elem.find(".//d:propstat", ns)
|
||||||
|
|||||||
@@ -296,6 +296,55 @@ class TestObjectNameResolution:
|
|||||||
"/remote.php/dav/addressbooks/users/testuser/contacts/ghost.vcf",
|
"/remote.php/dav/addressbooks/users/testuser/contacts/ghost.vcf",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
async def test_update_targets_real_no_extension_path(self, mocker):
|
||||||
|
"""Regression for #874: update must PUT to ``.../default`` not
|
||||||
|
``.../default.vcf`` (parallels the delete coverage).
|
||||||
|
"""
|
||||||
|
client = ContactsClient.__new__(ContactsClient)
|
||||||
|
client.username = "testuser"
|
||||||
|
mocker.patch.object(
|
||||||
|
client, "_resolve_object_name", mocker.AsyncMock(return_value="default")
|
||||||
|
)
|
||||||
|
mocker.patch.object(
|
||||||
|
client,
|
||||||
|
"_fetch_raw_vcard",
|
||||||
|
mocker.AsyncMock(
|
||||||
|
return_value=(
|
||||||
|
"BEGIN:VCARD\nVERSION:3.0\nUID:default\nFN:No Ext\nEND:VCARD\n",
|
||||||
|
'"etag"',
|
||||||
|
)
|
||||||
|
),
|
||||||
|
)
|
||||||
|
make_request = mocker.patch.object(client, "_make_request", mocker.AsyncMock())
|
||||||
|
await client.update_contact(
|
||||||
|
addressbook="contacts", uid="default", contact_data={"fn": "Updated"}
|
||||||
|
)
|
||||||
|
method, url = make_request.await_args.args[0], make_request.await_args.args[1]
|
||||||
|
assert method == "PUT"
|
||||||
|
assert url == "/remote.php/dav/addressbooks/users/testuser/contacts/default"
|
||||||
|
|
||||||
|
async def test_update_falls_back_to_vcf_when_unresolved(self, mocker):
|
||||||
|
"""When resolution finds nothing, update falls back to ``<uid>.vcf`` so
|
||||||
|
the caller still gets a clean 404 from the PUT (mirrors delete).
|
||||||
|
"""
|
||||||
|
client = ContactsClient.__new__(ContactsClient)
|
||||||
|
client.username = "testuser"
|
||||||
|
mocker.patch.object(
|
||||||
|
client, "_resolve_object_name", mocker.AsyncMock(return_value=None)
|
||||||
|
)
|
||||||
|
make_request = mocker.patch.object(client, "_make_request", mocker.AsyncMock())
|
||||||
|
# Supplying an etag skips the existing-vCard fetch; update builds a fresh
|
||||||
|
# vCard and PUTs it to the fallback path.
|
||||||
|
await client.update_contact(
|
||||||
|
addressbook="contacts",
|
||||||
|
uid="ghost",
|
||||||
|
contact_data={"fn": "Ghost"},
|
||||||
|
etag='"x"',
|
||||||
|
)
|
||||||
|
method, url = make_request.await_args.args[0], make_request.await_args.args[1]
|
||||||
|
assert method == "PUT"
|
||||||
|
assert url == "/remote.php/dav/addressbooks/users/testuser/contacts/ghost.vcf"
|
||||||
|
|
||||||
|
|
||||||
class TestFirstCustom:
|
class TestFirstCustom:
|
||||||
"""``_first_custom`` is the read-side companion to PR #719 — it pulls
|
"""``_first_custom`` is the read-side companion to PR #719 — it pulls
|
||||||
|
|||||||
Reference in New Issue
Block a user