Your details: changing one postal address into another that is being removed crashes the save #458

New issue

Closed

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

tiagoagueda commented

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

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, by reading BasePostalAddressFormSet against the model's constraints and running the case through accounts:profile in a test.

Related to #329.

Checked again on 2026-10-01 against main at 0a5cad587: still present.

Postal addresses are unique per owner (address_unique_per_owner), but the address formset compares a submission only with itself. It checks nothing against the database, and it saves the rows in page order. Two ordinary edits therefore get past validation and then fail at the constraint with a 500.

Where: core/postal.py:253-264 (BasePostalAddressFormSet.clean, which skips removed rows and compares only the rows kept in this submission); core/postal.py:266-285 (save, which calls Django's save_existing_objects, and that updates and deletes the existing rows in index order); core/models.py:564-568 (address_unique_per_owner on (owner, comparable)); accounts/views.py:186-187.
What happens:
1. Somebody has Home: Rua A 1 (first row) and Postal: Rua B 2. They move to Rua B 2, so they change the first row to Rua B 2, tick Remove on the second and save. The formset is valid. The first row's UPDATE runs while the second row still exists, and the save raises IntegrityError: UNIQUE constraint failed: core_postaladdress.owner_id, core_postaladdress.comparable. Reproduced against accounts:profile. The same edit with the rows in the other order saves.
2. The constraint is per owner, not per holder, and a contact can hold addresses (from an archive, core/importer.py:523). If somebody edits their own address into one that a contact of theirs already has, the formset again accepts it and the save fails the same way. Reproduced with postal.formset_for.
The telephone and web link formsets do not crash on the first case, because they compare against the saved rows in clean().
Why it matters: a normal move-house edit fails, and everything else typed on Your details is lost with it. Nothing on the page suggests that splitting the change over two saves would work.
Fix:
- In BasePostalAddressFormSet.save, delete the rows marked for removal before updating the changed ones (delete self.deleted_forms first, then call the parent save).
- In clean(), compare each kept row with the owner's saved addresses (PostalAddress.objects.filter(owner=...)), leaving out the row itself and the rows being removed, and report "This address is already listed." instead of letting the constraint answer.

Done when

A test in tests/test_postal_addresses.py edits the first of two addresses into the second, removes the second in the same POST to accounts:profile, and finds one address left and a redirect. Today it raises IntegrityError.
A test gives a contact an address, types the same address on the profile, and gets the form back with "This address is already listed."

Found on 2026-09-29 in a read-only review of postulo at 95348908d, by reading `BasePostalAddressFormSet` against the model's constraints and running the case through `accounts:profile` in a test. Related to #329. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. Postal addresses are unique per owner (`address_unique_per_owner`), but the address formset compares a submission only with itself. It checks nothing against the database, and it saves the rows in page order. Two ordinary edits therefore get past validation and then fail at the constraint with a 500. - **Where:** `core/postal.py:253-264` (`BasePostalAddressFormSet.clean`, which skips removed rows and compares only the rows kept in this submission); `core/postal.py:266-285` (`save`, which calls Django's `save_existing_objects`, and that updates and deletes the existing rows in index order); `core/models.py:564-568` (`address_unique_per_owner` on `(owner, comparable)`); `accounts/views.py:186-187`. - **What happens:** 1. Somebody has *Home: Rua A 1* (first row) and *Postal: Rua B 2*. They move to Rua B 2, so they change the first row to Rua B 2, tick *Remove* on the second and save. The formset is valid. The first row's UPDATE runs while the second row still exists, and the save raises `IntegrityError: UNIQUE constraint failed: core_postaladdress.owner_id, core_postaladdress.comparable`. Reproduced against `accounts:profile`. The same edit with the rows in the other order saves. 2. The constraint is per owner, not per holder, and a contact can hold addresses (from an archive, `core/importer.py:523`). If somebody edits their own address into one that a contact of theirs already has, the formset again accepts it and the save fails the same way. Reproduced with `postal.formset_for`. The telephone and web link formsets do not crash on the first case, because they compare against the saved rows in `clean()`. - **Why it matters:** a normal move-house edit fails, and everything else typed on *Your details* is lost with it. Nothing on the page suggests that splitting the change over two saves would work. - **Fix:** - In `BasePostalAddressFormSet.save`, delete the rows marked for removal before updating the changed ones (delete `self.deleted_forms` first, then call the parent `save`). - In `clean()`, compare each kept row with the owner's saved addresses (`PostalAddress.objects.filter(owner=...)`), leaving out the row itself and the rows being removed, and report "This address is already listed." instead of letting the constraint answer. ## Done when - A test in `tests/test_postal_addresses.py` edits the first of two addresses into the second, removes the second in the same POST to `accounts:profile`, and finds one address left and a redirect. Today it raises `IntegrityError`. - A test gives a contact an address, types the same address on the profile, and gets the form back with "This address is already listed."

tiagoagueda added this to the 0.5.0 milestone

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

tiagoagueda added the

bug

tier

labels

2026-10-01 10:46:51 +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-02 00:40:22 +00:00