Contacts sync: a card's number that is taken elsewhere is dropped without a note, and a removed TEL deletes the number here

tiagoagueda commented

2026-10-01 10:47:54 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo-dav at 97abc9779, reading apply_card against the rules #1 set for telephone numbers and running both cases against the suite's in-memory server.

Related to postulo/postulo#333.

When a pulled card carries a number that some other row on the instance already holds, the number is skipped and nothing says so; the link is then stored as if the card had been applied, so the two sides stay different and a later edit here writes the old number over the phone's. When a pulled card has no TEL at all, the contact's primary number here is deleted. Related to #1 (closed), which asked for the opposite of both: "A number refused as taken elsewhere is a note in the report, never a failed sync", and "A number removed from a card must not remove the number in Postulo".

Where: src/postulo_dav/sync.py:159-162 (apply_card), src/postulo_dav/sync.py:491-496 (the pull in _sync_contacts), src/postulo_dav/sync.py:540-549 (adoption, the same call). In the core, core/phone_numbers.py:269-274 (save_only_number deletes the primary row when given an empty string).
What happens:
1. Taken number. sync.py:161 is if not number or not phone_number_is_taken(number, exclude_pk=primary.pk if primary else None):. A taken number skips save_phone_number and adds nothing to report.notes. The caller then sets link.etag = resource.etag and link.local_hash = digest(card_for(contact).digest_source()), computed from the contact as it now is, with the old number. Run against the fake server: the report was 1 pulled with no note, the primary stayed +1 555 0100, the next run said nothing to do while the card still held the other number, and after a change of role here the push put +1 555 0100 back on the card. The push is conditional on the ETag just stored, so nothing notices. taken_elsewhere counts the same account's rows too, so two contacts at one company who share a switchboard number are enough.
2. Removed TEL. With number == "" the first half of the condition is true and save_phone_number(contact, contact.owner, "") runs, which deletes the primary row. Run: a card with its TEL line removed gave 1 pulled and primary_for(recruiter) was None afterwards.
Why it matters: a number changed on the phone never arrives and is later overwritten there, with no word in the report; a number removed on the phone is destroyed here, which the plugin's own issue ruled out because the core keeps numbers behind a plugin that never deletes them.
Fix: in apply_card, call save_phone_number only when the card has a number and it is not taken. When it is taken, append a note that names the contact and says no more than the core's own sentence does (ALREADY_IN_USE, core/phone_numbers.py:33), and have the caller leave link.etag as it was, so the card is still seen as changed: the note repeats until the two sides agree, and a later edit here meets the conflict rule at sync.py:472 (the losing version kept as a note) instead of a silent overwrite. apply_card needs the report, or a return value, for that. When the card has no TEL, leave the number here alone. save_web_link(..., card.url[:500]) on sync.py:163 has the same shape and removes the primary social link when a card has no URL; decide it the same way.

Done when

tests/test_dav.py has a test in which a pulled card carries a number another contact holds: the report carries a note, the contact keeps its number, the next run does not say nothing to do, and a later change here does not put the old number on the card without a note.
tests/test_dav.py has a test in which a pulled card has no TEL: the contact's primary number is still there afterwards.
New strings go through uv run postulo-messages extract, with fr-fr and pt-pt filled.

Found on 2026-09-29 in a read-only review of postulo-dav at 97abc9779, reading `apply_card` against the rules #1 set for telephone numbers and running both cases against the suite's in-memory server. Related to postulo/postulo#333. When a pulled card carries a number that some other row on the instance already holds, the number is skipped and nothing says so; the link is then stored as if the card had been applied, so the two sides stay different and a later edit here writes the old number over the phone's. When a pulled card has no `TEL` at all, the contact's primary number here is deleted. Related to #1 (closed), which asked for the opposite of both: "A number refused as taken elsewhere is a note in the report, never a failed sync", and "A number removed from a card must not remove the number in Postulo". - **Where:** `src/postulo_dav/sync.py:159-162` (`apply_card`), `src/postulo_dav/sync.py:491-496` (the pull in `_sync_contacts`), `src/postulo_dav/sync.py:540-549` (adoption, the same call). In the core, `core/phone_numbers.py:269-274` (`save_only_number` deletes the primary row when given an empty string). - **What happens:** 1. *Taken number.* `sync.py:161` is `if not number or not phone_number_is_taken(number, exclude_pk=primary.pk if primary else None):`. A taken number skips `save_phone_number` and adds nothing to `report.notes`. The caller then sets `link.etag = resource.etag` and `link.local_hash = digest(card_for(contact).digest_source())`, computed from the contact as it now is, with the old number. Run against the fake server: the report was `1 pulled` with no note, the primary stayed `+1 555 0100`, the next run said `nothing to do` while the card still held the other number, and after a change of role here the push put `+1 555 0100` back on the card. The push is conditional on the ETag just stored, so nothing notices. `taken_elsewhere` counts the same account's rows too, so two contacts at one company who share a switchboard number are enough. 2. *Removed `TEL`.* With `number == ""` the first half of the condition is true and `save_phone_number(contact, contact.owner, "")` runs, which deletes the primary row. Run: a card with its `TEL` line removed gave `1 pulled` and `primary_for(recruiter)` was `None` afterwards. - **Why it matters:** a number changed on the phone never arrives and is later overwritten there, with no word in the report; a number removed on the phone is destroyed here, which the plugin's own issue ruled out because the core keeps numbers behind a plugin that never deletes them. - **Fix:** in `apply_card`, call `save_phone_number` only when the card has a number and it is not taken. When it is taken, append a note that names the contact and says no more than the core's own sentence does (`ALREADY_IN_USE`, `core/phone_numbers.py:33`), and have the caller leave `link.etag` as it was, so the card is still seen as changed: the note repeats until the two sides agree, and a later edit here meets the conflict rule at `sync.py:472` (the losing version kept as a note) instead of a silent overwrite. `apply_card` needs the report, or a return value, for that. When the card has no `TEL`, leave the number here alone. `save_web_link(..., card.url[:500])` on `sync.py:163` has the same shape and removes the primary social link when a card has no `URL`; decide it the same way. ## Done when - `tests/test_dav.py` has a test in which a pulled card carries a number another contact holds: the report carries a note, the contact keeps its number, the next run does not say `nothing to do`, and a later change here does not put the old number on the card without a note. - `tests/test_dav.py` has a test in which a pulled card has no `TEL`: the contact's primary number is still there afterwards. - New strings go through `uv run postulo-messages extract`, with fr-fr and pt-pt filled.

tiagoagueda added this to the 0.5.0 milestone

2026-10-01 10:47:54 +00:00

tiagoagueda added the

bug

label

2026-10-01 10:47:54 +00:00

tiagoagueda commented

2026-10-04 11:15:35 +00:00

Author

Owner

Note from postulo/postulo#333 (the core side is done; this is the plugin's half).

apply_card calls phone_number_is_taken(number, exclude_pk=...) without saying who is asking. In core that bare form is now deprecated: it still answers, but uncharged, with a DeprecationWarning, and it will be removed in a later release. Whoever fixes this issue should also:

Call phone_number_is_taken(number, exclude_pk=..., asked_by=<the account being synced>). A yes then spends one of that account's answers, and once POSTULO_NUMBER_RATE is spent every number answers yes, held or not, so a sync can no longer map which numbers the instance holds.
Treat that yes as what it is: say in the run report which cards had their number left off (the note this issue already asks for), without implying the number is held by somebody else.
Pin postulo-dav to a core that has asked_by (the surface test lists it as phone_number_is_taken(number, asked_by=account)).

Once this lands, postulo/postulo#333 can be closed.

Note from postulo/postulo#333 (the core side is done; this is the plugin's half). `apply_card` calls `phone_number_is_taken(number, exclude_pk=...)` without saying who is asking. In core that bare form is now deprecated: it still answers, but uncharged, with a `DeprecationWarning`, and it will be removed in a later release. Whoever fixes this issue should also: 1. Call `phone_number_is_taken(number, exclude_pk=..., asked_by=<the account being synced>)`. A *yes* then spends one of that account's answers, and once `POSTULO_NUMBER_RATE` is spent every number answers yes, held or not, so a sync can no longer map which numbers the instance holds. 2. Treat that *yes* as what it is: say in the run report which cards had their number left off (the note this issue already asks for), without implying the number is held by somebody else. 3. Pin postulo-dav to a core that has `asked_by` (the surface test lists it as `phone_number_is_taken(number, asked_by=account)`). Once this lands, postulo/postulo#333 can be closed.

tiagoagueda referenced this issue from Postulo/postulo

2026-10-04 11:15:36 +00:00

Telephone numbers: a sync plugin learns which numbers are held on the instance without spending the #142 allowance #333

tiagoagueda referenced this issue from a commit

2026-10-19 07:13:10 +00:00

Review fixes: pairing by UID, bounded reads, per-resource tolerance, notes per reader, and a calendar that carries no attendee

tiagoagueda referenced this issue

2026-10-19 07:13:15 +00:00

Review fixes: pairing by UID, bounded reads, notes per reader, per-resource tolerance #35

tiagoagueda commented

2026-10-19 09:48:30 +00:00

Author