Phone numbers and web links: removing a row and adding the same value back in one save is refused #461

New issue

Closed

opened 2026-10-01 10:46:52 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:46:52 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading the two formsets' clean methods and binding them with a deleted row and a new row holding the same value.

Related to #303.

Checked again on 2026-10-01 against main at 0a5cad587: still present. The code is now at core/phone_numbers.py:310 (BasePhoneNumberFormSet.clean, with the call to taken_elsewhere at line 326), core/phone_numbers.py:123 (taken_elsewhere) and core/phone_numbers.py:149 (collision_message); core/web_links.py has not moved, and both formsets give the same refusals.

Both formsets compare what is typed against the rows still in the database, including the rows the same submission is deleting. For telephone numbers the refusal is the instance-wide sentence, which tells the person the number "may belong to somebody else's records" when the only holder is the row they are removing, and each refusal spends one of the account's collision answers.

Where: core/phone_numbers.py:333 (BasePhoneNumberFormSet.clean), which calls taken_elsewhere(typed, exclude_pk=form.instance.pk) at line 349; taken_elsewhere at core/phone_numbers.py:146 excludes only the row's own key; collision_message at core/phone_numbers.py:172 calls collision_noticed, which spends the allowance. core/web_links.py:308 (BaseWebLinkFormSet.clean), where already at line 313 holds every stored address of the holder, including those marked for deletion.
What happens: a person removes the Work row holding +351912345678 and adds the same number as a Mobile row in one save. The new row is refused with "That number is already recorded on this instance. Telephone numbers are kept unique across every account here, so it may belong to somebody else's records." The same move in a web link block is refused with "This address is already listed." (Both reproduced by binding the formsets.) Swapping the values of two rows is refused the same way.
Why it matters: the message is false and points at other people's records; the person has to save twice to do one thing, and repeated attempts count towards the enumeration limit meant for somebody sweeping numbers.
Fix: in both clean methods, first collect the primary keys of the forms marked DELETE. Pass them to taken_elsewhere (an exclude_pks argument alongside exclude_pk) and drop their stored values from already. BaseModelFormSet.save deletes existing rows before it inserts new ones, so the per-instance unique constraint on PhoneNumber still holds at save.

Done when

tests/test_phone_numbers.py and tests/test_web_links.py each bind their formset with a stored row marked DELETE and a new row with the same value, and asserts that the formset is valid, that saving leaves one row, and that no collision answer was spent.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading the two formsets' `clean` methods and binding them with a deleted row and a new row holding the same value. Related to #303. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. The code is now at `core/phone_numbers.py:310` (`BasePhoneNumberFormSet.clean`, with the call to `taken_elsewhere` at line 326), `core/phone_numbers.py:123` (`taken_elsewhere`) and `core/phone_numbers.py:149` (`collision_message`); `core/web_links.py` has not moved, and both formsets give the same refusals. Both formsets compare what is typed against the rows still in the database, including the rows the same submission is deleting. For telephone numbers the refusal is the instance-wide sentence, which tells the person the number "may belong to somebody else's records" when the only holder is the row they are removing, and each refusal spends one of the account's collision answers. - **Where:** `core/phone_numbers.py:333` (`BasePhoneNumberFormSet.clean`), which calls `taken_elsewhere(typed, exclude_pk=form.instance.pk)` at line 349; `taken_elsewhere` at `core/phone_numbers.py:146` excludes only the row's own key; `collision_message` at `core/phone_numbers.py:172` calls `collision_noticed`, which spends the allowance. `core/web_links.py:308` (`BaseWebLinkFormSet.clean`), where `already` at line 313 holds every stored address of the holder, including those marked for deletion. - **What happens:** a person removes the *Work* row holding `+351912345678` and adds the same number as a *Mobile* row in one save. The new row is refused with "That number is already recorded on this instance. Telephone numbers are kept unique across every account here, so it may belong to somebody else's records." The same move in a web link block is refused with "This address is already listed." (Both reproduced by binding the formsets.) Swapping the values of two rows is refused the same way. - **Why it matters:** the message is false and points at other people's records; the person has to save twice to do one thing, and repeated attempts count towards the enumeration limit meant for somebody sweeping numbers. - **Fix:** in both `clean` methods, first collect the primary keys of the forms marked `DELETE`. Pass them to `taken_elsewhere` (an `exclude_pks` argument alongside `exclude_pk`) and drop their stored values from `already`. `BaseModelFormSet.save` deletes existing rows before it inserts new ones, so the per-instance unique constraint on `PhoneNumber` still holds at save. ## Done when - `tests/test_phone_numbers.py` and `tests/test_web_links.py` each bind their formset with a stored row marked `DELETE` and a new row with the same value, and asserts that the formset is valid, that saving leaves one row, and that no collision answer was spent.

tiagoagueda added this to the 0.6.0 milestone

2026-10-01 10:46:52 +00:00

tiagoagueda added the

tier

bug

labels

2026-10-01 10:46:52 +00:00

tiagoagueda referenced this issue from a commit

2026-10-01 16:03:11 +00:00

Give a new number, link or address its owner where the row is made

tiagoagueda referenced this issue from a commit

2026-10-01 23:12:36 +00:00