Company form: a rejected identifier still says 'Company added.' and still runs the logo fields, crashing on create #530

New issue

Closed

opened 2026-10-01 10:47:25 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

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

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, following the company form's form_valid chain and posting it in tests.

Checked again on 2026-10-01 against main at 0a5cad587: still present. The code is now at jobs/forms.py:291-313 (CompanyForm.apply_logo); jobs/views.py has not changed, so its lines stand.

When the company form is valid but an identifier row is not, the identifiers mixin re-renders the form without saving the company. Everything around it carries on as if the save had happened: the success message is already queued, and the logo fields are acted on afterwards.

Where: jobs/views.py:394-396 and :406-408 (CompanyCreateView.form_valid and CompanyUpdateView.form_valid add the message before super()); jobs/views.py:350-353 (CompanyIdentifiersMixin.form_valid returns render_to_response on an invalid formset); jobs/views.py:372-379 (LogoFormMixin.form_valid calls form.apply_logo(self.object) whatever the response was); jobs/forms.py:290-312 (CompanyForm.apply_logo).
What happens:
1. A person adds a company with a malformed Wikidata id. The page comes back with the identifier error and, above it, Company added.; no company exists. The same on edit shows Company updated. with nothing changed.
2. The same form with a Logo address filled in answers 500: on create self.object is None, and apply_logo reads company.logo_source_url from it (AttributeError: 'NoneType' object has no attribute 'logo_source_url').
3. On edit, the logo fields are applied to a company whose other changes were refused: with Remove the logo ticked and an invalid identifier, the logo file is deleted and the fields cleared, while the new name is not saved. A logo address or upload is likewise fetched and stored.
Why it matters: a person told their company was added navigates away and loses what they typed; a logo address on a new company turns an ordinary validation error into a crash; and a rejected edit is not a no-op.
Fix: validate the identifiers formset before anything is committed to, for example in post() or at the top of the outermost form_valid (treat an invalid formset as form_invalid), so that the success message and apply_logo are reached only on the saving path. At the least, add the message and call apply_logo only when the response from super().form_valid() is a redirect.

Done when

Tests in tests/test_identifiers.py and tests/test_logos.py that fail today: a create with an invalid identifier answers 200 without Company added.; the same with a logo address answers 200, not 500, and fetches nothing; an edit with an invalid identifier and Remove the logo ticked leaves the logo in place.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, following the company form's `form_valid` chain and posting it in tests. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. The code is now at `jobs/forms.py:291-313` (`CompanyForm.apply_logo`); `jobs/views.py` has not changed, so its lines stand. When the company form is valid but an identifier row is not, the identifiers mixin re-renders the form without saving the company. Everything around it carries on as if the save had happened: the success message is already queued, and the logo fields are acted on afterwards. - **Where:** `jobs/views.py:394-396` and `:406-408` (`CompanyCreateView.form_valid` and `CompanyUpdateView.form_valid` add the message before `super()`); `jobs/views.py:350-353` (`CompanyIdentifiersMixin.form_valid` returns `render_to_response` on an invalid formset); `jobs/views.py:372-379` (`LogoFormMixin.form_valid` calls `form.apply_logo(self.object)` whatever the response was); `jobs/forms.py:290-312` (`CompanyForm.apply_logo`). - **What happens:** 1. A person adds a company with a malformed Wikidata id. The page comes back with the identifier error and, above it, *Company added.*; no company exists. The same on edit shows *Company updated.* with nothing changed. 2. The same form with a *Logo address* filled in answers 500: on create `self.object` is `None`, and `apply_logo` reads `company.logo_source_url` from it (`AttributeError: 'NoneType' object has no attribute 'logo_source_url'`). 3. On edit, the logo fields are applied to a company whose other changes were refused: with *Remove the logo* ticked and an invalid identifier, the logo file is deleted and the fields cleared, while the new name is not saved. A logo address or upload is likewise fetched and stored. - **Why it matters:** a person told their company was added navigates away and loses what they typed; a logo address on a new company turns an ordinary validation error into a crash; and a rejected edit is not a no-op. - **Fix:** validate the identifiers formset before anything is committed to, for example in `post()` or at the top of the outermost `form_valid` (treat an invalid formset as `form_invalid`), so that the success message and `apply_logo` are reached only on the saving path. At the least, add the message and call `apply_logo` only when the response from `super().form_valid()` is a redirect. ## Done when - Tests in `tests/test_identifiers.py` and `tests/test_logos.py` that fail today: a create with an invalid identifier answers 200 without *Company added.*; the same with a logo address answers 200, not 500, and fetches nothing; an edit with an invalid identifier and *Remove the logo* ticked leaves the logo in place.

tiagoagueda added this to the 0.5.0 milestone

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

tiagoagueda added the

tier

bug

labels

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

tiagoagueda referenced this issue from a commit

2026-10-03 18:09:38 +00:00

Stop a refused company identifier from acting as if the company was saved

tiagoagueda closed this issue

2026-10-03 18:09:38 +00:00