Skip to content

Generic form system: generic_forms app foundation (PR 1/5) - #4705

Open
marcoacierno wants to merge 10 commits into
mainfrom
generic-forms/01-app
Open

Generic form system: generic_forms app foundation (PR 1/5)#4705
marcoacierno wants to merge 10 commits into
mainfrom
generic-forms/01-app

Conversation

@marcoacierno

Copy link
Copy Markdown
Member

Summary

First PR of the generic form system stack (spec: specs/generic-form-system.md, plan: tasks/generic-forms/plan.md). Adds the generic_forms Django app — no consumers wired yet; grants integration, GraphQL and frontend come in the next PRs of the stack.

  • Models: Form (conference + purpose, one per (conference, purpose) except generic), FormQuestion (6 types: text/textarea/select/multi_select/boolean/url; options as [{id, label}] JSON; validated at authoring), FormAnswer (one per (form, user), versioned JSON envelope {"version": 1, "answers": {...}})
  • Freeze-on-answer: once a form has any answer, question type/options/required/form and form conference/purpose become immutable; question deletion blocked (pre_delete signal, covers queryset deletes). Labels/description/order/active stay editable; deactivate replaces delete
  • validate_answers service: single source of truth for answer validation (required, per-type shape, option membership, URL, max length); never crashes on malformed client JSON
  • Admin: form authoring with inline questions; FormAnswer fully read-only (add/change/delete blocked — deleting answers would unfreeze questions and destroy submissions)

Test plan

  • 48 new tests (generic_forms/tests/): constraints, freeze rules incl. queryset-delete bypass, validation accept/reject per question type, envelope round-trip, admin permissions
  • Full suite green: 1191 passed
  • ruff (0.12.5) + format clean; adversarially reviewed (3-lens workflow), all findings fixed
  • Manual: create a form with all 6 question types in local Django admin

Stack: PR1 (this) → PR2 GraphQL query → PR3 grants backend → PR4 admin display/export → PR5 frontend.

Foundation for the generic form system: conference-scoped forms with
admin-authorable questions and JSON answers (versioned envelope).
DB constraints: one answer per (form, user); at most one form per
(conference, purpose) except for generic-purpose forms.
question_type, options, required and the parent form become immutable
(and deletion is blocked) as soon as any FormAnswer exists, so stored
answers always match their questions. Label, description, order and
active stay editable; deactivation replaces deletion.
validate_answers checks a flat {question_id: value} map against a
form's active questions: required, per-type value shape, option
membership (every multi-select item), URL format and max length.
wrap_answers/unwrap_answers implement the {version: 1, answers: {...}}
storage envelope with version dispatch.
FormAdmin with inline questions (freeze rule surfaces via model
validation so new questions can still be added to answered forms;
inline deletion is blocked once answers exist). FormAnswerAdmin is
read-only.
- validate_answers: reject non-dict answers and non-string multi-select
  items with errors instead of crashing (client-controlled JSON input)
- validate options shape at authoring time (list of unique {id, label}
  string pairs, required for select types, forbidden otherwise) so a
  malformed options blob can't 500 every submission
- unwrap_answers: ValueError on malformed envelopes, empty dict allowed
- block FormAnswer deletion in admin (deleting answers would unfreeze
  questions and silently destroy submissions)
- freeze Form.conference/purpose once answered (model + admin readonly)
- question delete guard moved to pre_delete signal so queryset deletes
  are covered; explicit-pk saves no longer crash (_state.adding)
@vercel

vercel Bot commented Aug 6, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
pycon Error Error Aug 7, 2026 2:40am

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Adds the generic_forms app foundation (models, admin, validation service) with no consumers wired in yet. Well-tested; a couple of points worth a look before the next PRs in the stack build on top of it.

Correctness

  • Form.save() / FormQuestion.save() re-implement the freeze/options checks manually but never call full_clean()/clean_fields(), so Django's choices validation for purpose and question_type is only enforced through the admin ModelForm path — not on direct .save()/.objects.create() calls. There's no DB CHECK constraint backing it either. Since PR2 (GraphQL mutations) and PR3 (grants integration) will very likely create/update these models outside the admin, invalid purpose/question_type values could slip through unless those code paths also call full_clean() or the choice is re-validated explicitly. Worth deciding now whether save() should also enforce field choices, since it's already positioned as the single place doing model-level validation.

Minor

  • FormAdmin.list_display = ("name", "conference", "purpose") has no list_select_related, so rendering the changelist does one query per row to fetch conference. FormAnswerAdmin right below it already sets list_select_related for the same reason — worth applying the same fix to FormAdmin for consistency.

@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.49112% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.45%. Comparing base (d249997) to head (d2b4b60).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4705      +/-   ##
==========================================
+ Coverage   92.43%   92.45%   +0.01%     
==========================================
  Files         355      358       +3     
  Lines       10719    10888     +169     
  Branches      818      852      +34     
==========================================
+ Hits         9908    10066     +158     
- Misses        698      706       +8     
- Partials      113      116       +3     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Replace substring checks (e.g. asserting "5" appears somewhere in the
error) with exact error-dict equality and distinctive pytest.raises
match phrases, so tests assert the actual behavior and catch stray
extra errors.
- validate_answers: plain dict instead of defaultdict + trailing
  filter comprehension (empty entries no longer created then dropped)
- extract _is_valid_option predicate from the option-shape mega-boolean
- reuse a module-level URLValidator; drop redundant map(str, ...) on
  already-validated strings; idiomatic empty-dict check in unwrap
- FormAnswer admin: select related form__conference/user (Form.__str__
  renders the conference name)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant