From 40255805c3f2c51f6f9a473d1b1aa8bd2d22f061 Mon Sep 17 00:00:00 2001 From: Bernard Siebens Date: Thu, 16 Jul 2026 18:33:56 +0200 Subject: [PATCH] Modularize confirmation modal for destructive POST actions and add `notify` helper for concise message handling across the UI. --- controlpanel/messages.py | 35 +++++ controlpanel/mixins.py | 5 +- .../controlpanel/_club_admins_card.html | 22 +-- .../controlpanel/_confirm_modal.html | 28 ++++ .../templates/controlpanel/_nav_items.html | 2 +- .../templates/controlpanel/admin_form.html | 22 --- .../templates/controlpanel/admins.html | 29 ++-- .../controlpanel/club_admin_form.html | 22 --- .../templates/controlpanel/features.html | 19 ++- .../templates/controlpanel/flag_form.html | 19 --- controlpanel/templatetags/ui.py | 7 +- controlpanel/tests.py | 91 ++++++++++--- controlpanel/views.py | 127 ++++++++++-------- formbuilder/services/submission.py | 57 +++----- formbuilder/tests.py | 18 +++ members/services/member_csv_importer.py | 15 ++- members/tests.py | 45 ++++++- shop/models.py | 22 ++- shop/tests.py | 5 +- 19 files changed, 369 insertions(+), 221 deletions(-) create mode 100644 controlpanel/messages.py create mode 100644 controlpanel/templates/controlpanel/_confirm_modal.html delete mode 100644 controlpanel/templates/controlpanel/admin_form.html delete mode 100644 controlpanel/templates/controlpanel/club_admin_form.html delete mode 100644 controlpanel/templates/controlpanel/flag_form.html diff --git a/controlpanel/messages.py b/controlpanel/messages.py new file mode 100644 index 0000000..f7f4efb --- /dev/null +++ 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: ``"||<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) diff --git a/controlpanel/mixins.py b/controlpanel/mixins.py index 31b438b..9e8f326 100644 --- 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()) diff --git a/controlpanel/templates/controlpanel/_club_admins_card.html b/controlpanel/templates/controlpanel/_club_admins_card.html index 56c59ad..d88fa87 100644 --- 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 %} diff --git a/controlpanel/templates/controlpanel/_confirm_modal.html b/controlpanel/templates/controlpanel/_confirm_modal.html new file mode 100644 index 0000000..dc55f65 --- /dev/null +++ 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> diff --git a/controlpanel/templates/controlpanel/_nav_items.html b/controlpanel/templates/controlpanel/_nav_items.html index fb638c6..c3d9465 100644 --- 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 %} diff --git a/controlpanel/templates/controlpanel/admin_form.html b/controlpanel/templates/controlpanel/admin_form.html deleted file mode 100644 index 88112ad..0000000 --- a/controlpanel/templates/controlpanel/admin_form.html +++ /dev/null @@ -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 %} diff --git a/controlpanel/templates/controlpanel/admins.html b/controlpanel/templates/controlpanel/admins.html index 70472d0..2d2d9ba 100644 --- 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 %} diff --git a/controlpanel/templates/controlpanel/club_admin_form.html b/controlpanel/templates/controlpanel/club_admin_form.html deleted file mode 100644 index 8921561..0000000 --- a/controlpanel/templates/controlpanel/club_admin_form.html +++ /dev/null @@ -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 %} diff --git a/controlpanel/templates/controlpanel/features.html b/controlpanel/templates/controlpanel/features.html index 4410bb5..64fa957 100644 --- 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 %} diff --git a/controlpanel/templates/controlpanel/flag_form.html b/controlpanel/templates/controlpanel/flag_form.html deleted file mode 100644 index 5b9c833..0000000 --- a/controlpanel/templates/controlpanel/flag_form.html +++ /dev/null @@ -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 %} diff --git a/controlpanel/templatetags/ui.py b/controlpanel/templatetags/ui.py index 0d79141..ec17cbb 100644 --- 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 diff --git a/controlpanel/tests.py b/controlpanel/tests.py index f58423d..c7fe062 100644 --- 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 diff --git a/controlpanel/views.py b/controlpanel/views.py index 25b3538..e5f10f3 100644 --- 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") diff --git a/formbuilder/services/submission.py b/formbuilder/services/submission.py index eaac775..48aedd8 100644 --- 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)] diff --git a/formbuilder/tests.py b/formbuilder/tests.py index cfa884a..0305e3c 100644 --- 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"]) diff --git a/members/services/member_csv_importer.py b/members/services/member_csv_importer.py index 45f37b6..fdadee3 100644 --- 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. diff --git a/members/tests.py b/members/tests.py index fa21870..c794f48 100644 --- 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): diff --git a/shop/models.py b/shop/models.py index 7331d28..28bf493 100644 --- 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): diff --git a/shop/tests.py b/shop/tests.py index f0de9f5..92d8c7c 100644 --- 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")