diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 8721133..71db2eb 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -450,6 +450,22 @@ ClubMembership(ClubScopedModel) # -> carries `club` afterwards; approving a claim is not the place to decide it. They then get a password-reset link and a minimal "my family" page (`members/views.py::MyFamilyView`) — the seam a real parent portal would grow from. +- **`Club.contact_email`** *(built)* — the club's own public address, set from the control + panel next to `legal_name`. Shown to the parent both in the submission flash and in the + approval email, as somewhere to write if something's wrong — falls back to nothing shown at + all when unset, same pattern as `legal_name`/`official_name`. +- **The flash after submitting is worded and timed to reveal nothing.** Sent *before* any + lookup happens, from a fixed string that never varies with whether a matching child was + actually found (`members/views.py::ParentClaimView.form_valid`) — a message that differed + would be exactly the enumeration channel free-text matching was built to avoid. +- **Approval emails a real one-time set-password link**, built with allauth's own token + generator (`default_token_generator`/`user_pk_to_url_str`) so it lands in the same flow the + login page's own reset would send them to, rather than a second, parallel one that could + drift out of step with it. Sending is never allowed to fail the approval — the family link + and the guardian row are real either way, and a briefly unreachable mail server must not + cost the parent their place in the queue; the admin sees a distinct warning message + (`management/views.py::ParentClaimApproveView`) telling them the email didn't go and to have + the parent use "Forgot your password?" instead. - **Why a field and not a separate model.** Everything that answers "is this person attached to this club" already reads through `ClubMembership` — tenancy scoping, group membership, the club-wide event audience — and a second kind of link would need a parallel path through diff --git a/club/migrations/0024_club_contact_email.py b/club/migrations/0024_club_contact_email.py new file mode 100644 index 0000000..b30641b --- /dev/null +++ b/club/migrations/0024_club_contact_email.py @@ -0,0 +1,18 @@ +# Generated by Django 6.0.6 on 2026-08-11 16:26 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('club', '0023_backfill_guardian_memberships'), + ] + + operations = [ + migrations.AddField( + model_name='club', + name='contact_email', + field=models.EmailField(blank=True, help_text="The club's public address, shown to people the club writes to or asks to get in touch -- e.g. a parent claiming a child. Falls back to nothing being shown at all, so it's worth setting.", max_length=254, verbose_name='contact email'), + ), + ] diff --git a/club/models.py b/club/models.py index 99f32d5..2aed459 100644 --- a/club/models.py +++ b/club/models.py @@ -41,6 +41,11 @@ class Club(UUIDModel): name = models.CharField(_("name"), max_length=255) legal_name = models.CharField(_("legal name"), max_length=255, blank=True, help_text=_("Full registered name (e.g. including a legal form like VZW/ASBL), used on official documents. Falls back to club name if blank.")) slug = models.SlugField(_("slug"), max_length=255, unique=True, blank=True, help_text=_("Drives subdomain / path resolution (e.g. ajax-united.rosterchief.app).")) + contact_email = models.EmailField( + _("contact email"), + blank=True, + help_text=_("The club's public address, shown to people the club writes to or asks to get in touch -- e.g. a parent claiming a child. Falls back to nothing being shown at all, so it's worth setting."), + ) logo = models.FileField( _("logo"), diff --git a/controlpanel/forms.py b/controlpanel/forms.py index 3ba8a81..188e134 100644 --- a/controlpanel/forms.py +++ b/controlpanel/forms.py @@ -14,7 +14,7 @@ from .services.admins import find_member_by_email class ClubForm(forms.ModelForm): class Meta: model = Club - fields = ["name", "legal_name", "slug", "sport_type", "logo", "primary_color", "secondary_color", "season_start", "season_duration_months"] + fields = ["name", "legal_name", "contact_email", "slug", "sport_type", "logo", "primary_color", "secondary_color", "season_start", "season_duration_months"] help_texts = {"slug": _("Drives the club's subdomain. Left blank, it is derived from the name.")} # Deliberately a text input, not : a colour picker cannot # express "no colour" -- it would submit #000000 for every club that never diff --git a/controlpanel/templates/controlpanel/club_form.html b/controlpanel/templates/controlpanel/club_form.html index ada8eb5..077c0fb 100644 --- a/controlpanel/templates/controlpanel/club_form.html +++ b/controlpanel/templates/controlpanel/club_form.html @@ -18,6 +18,7 @@
{% form_field form.name %} {% form_field form.legal_name %} + {% form_field form.contact_email %} {% form_field form.slug %} {% form_field form.sport_type %}
diff --git a/management/tests.py b/management/tests.py index 0216631..6963400 100644 --- a/management/tests.py +++ b/management/tests.py @@ -8,6 +8,7 @@ from unittest import mock import openpyxl from allauth.mfa.models import Authenticator from django.contrib.auth import get_user_model +from django.core import mail from django.core.cache import cache from django.core.files.uploadedfile import SimpleUploadedFile from django.template.loader import render_to_string @@ -1389,27 +1390,38 @@ class ParentClaimViewTests(ManagementTestBase): payload.update(overrides) return payload - def submit(self, **overrides): - return self.client.post(reverse("members:parent_claim"), self.claim_payload(**overrides), HTTP_HOST="ajax-united.rosterchief.app") + def submit(self, *, follow=False, **overrides): + return self.client.post(reverse("members:parent_claim"), self.claim_payload(**overrides), HTTP_HOST="ajax-united.rosterchief.app", follow=follow) def test_the_claim_form_is_reachable_without_signing_in(self): response = self.client.get(reverse("members:parent_claim"), HTTP_HOST="ajax-united.rosterchief.app") self.assertEqual(response.status_code, 200) - def test_submitting_records_a_pending_claim(self): + def test_submitting_records_a_pending_claim_and_redirects_back_with_a_flash(self): response = self.submit() - self.assertEqual(response.status_code, 200) + # fetch_redirect_response=False: assertRedirects' own probe GET would + # otherwise consume the one-shot flash before the assertion below gets to see it. + self.assertRedirects(response, reverse("members:parent_claim"), fetch_redirect_response=False) self.assertEqual(ParentClaim.objects.filter(club=self.club, status=ParentClaim.Status.PENDING).count(), 1) + page = self.client.get(reverse("members:parent_claim"), HTTP_HOST="ajax-united.rosterchief.app") + self.assertContains(page, "Request received") - def test_an_unmatched_claim_looks_exactly_like_a_matched_one(self): + def test_an_unmatched_claim_flashes_the_same_message_as_a_matched_one(self): # The page must not tell an anonymous submitter which children exist. - matched = self.submit() - unmatched = self.submit(child_first_name="Nobody", child_last_name="Here", parent_email="other@example.com") + matched = self.submit(follow=True) + unmatched = self.submit(child_first_name="Nobody", child_last_name="Here", parent_email="other@example.com", follow=True) - self.assertEqual(matched.status_code, unmatched.status_code) - self.assertEqual(matched.content, unmatched.content) + self.assertEqual([str(m) for m in matched.context["messages"]], [str(m) for m in unmatched.context["messages"]]) + + def test_the_flash_mentions_the_clubs_contact_email_when_set(self): + self.club.contact_email = "info@ajax-united.example.com" + self.club.save(update_fields=["contact_email"]) + + response = self.submit(follow=True) + + self.assertContains(response, "info@ajax-united.example.com") def test_submitting_creates_no_account(self): # A public form that made a User per submission would be a spam magnet; @@ -1454,6 +1466,49 @@ class ParentClaimViewTests(ManagementTestBase): claim.refresh_from_db() self.assertEqual(claim.status, ParentClaim.Status.APPROVED) + def test_approving_emails_the_parent_a_working_set_password_link(self): + self.submit() + claim = ParentClaim.objects.get(club=self.club) + self.client.force_login(self.admin_user) + + self.club_post("parent_claim_approve", {"child": str(self.child.pk)}, claim.pk) + + self.assertEqual(len(mail.outbox), 1) + sent = mail.outbox[0] + self.assertEqual(sent.to, ["taylor.doe@example.com"]) + self.assertIn("Jamie", sent.body) + + [reset_path] = [line for line in sent.body.splitlines() if "/accounts/password/reset/key/" in line] + parent = Member.objects.get(user__email="taylor.doe@example.com") + self.assertFalse(parent.user.has_usable_password()) + response = self.client.get(reset_path.strip(), HTTP_HOST="ajax-united.rosterchief.app", follow=True) + self.assertEqual(response.status_code, 200) + self.assertContains(response, "password1") + + def test_the_email_mentions_the_clubs_contact_email_when_set(self): + self.club.contact_email = "info@ajax-united.example.com" + self.club.save(update_fields=["contact_email"]) + self.submit() + claim = ParentClaim.objects.get(club=self.club) + self.client.force_login(self.admin_user) + + self.club_post("parent_claim_approve", {"child": str(self.child.pk)}, claim.pk) + + self.assertIn("info@ajax-united.example.com", mail.outbox[0].body) + + def test_a_send_failure_still_leaves_the_claim_linked(self): + self.submit() + claim = ParentClaim.objects.get(club=self.club) + self.client.force_login(self.admin_user) + + with mock.patch("members.services.claims.send_mail", side_effect=OSError("smtp down")): + response = self.club_post("parent_claim_approve", {"child": str(self.child.pk)}, claim.pk) + + self.assertRedirects(response, reverse("management:parent_claim_list")) + claim.refresh_from_db() + self.assertEqual(claim.status, ParentClaim.Status.APPROVED) + self.assertTrue(Member.objects.filter(user__email="taylor.doe@example.com").exists()) + def test_approving_without_choosing_a_child_changes_nothing(self): self.submit() claim = ParentClaim.objects.get(club=self.club) diff --git a/management/views.py b/management/views.py index 4bdfbe1..b104ba1 100644 --- a/management/views.py +++ b/management/views.py @@ -40,7 +40,7 @@ from formbuilder.models import Form as FormBuilderForm from formbuilder.models import Submission from members.forms import ClaimReviewForm from members.models import Family, FamilyMembership, Group, GroupMembership, Member, ParentClaim -from members.services.claims import ClaimError, approve_claim, children_awaiting_a_parent, reject_claim, suggested_children +from members.services.claims import ClaimError, approve_claim, children_awaiting_a_parent, reject_claim, send_claim_approved_email, suggested_children from members.services.family import add_child_to_family, add_parent_to_family, attach_to_family, detach_from_family, get_or_create_login_user, grant_login, register_family from news.models import News, NewsPhoto from shop.models import Discount, Invoice, Order, Product @@ -1487,8 +1487,16 @@ class ParentClaimApproveView(ClubAdminRequiredMixin, View): except ClaimError as error: notify(request, f"e|{_('Could not approve')}|{error}") else: - body = _("“%(parent)s” is now linked to %(child)s and can set up their login.") % {"parent": claim.parent_name, "child": form.cleaned_data["child"]} - notify(request, f"s|{_('Claim approved')}|{body}") + child = form.cleaned_data["child"] + emailed = send_claim_approved_email(claim, child=child, request=request) + if emailed: + body = _("“%(parent)s” is now linked to %(child)s. They've been emailed a link to set their password.") % {"parent": claim.parent_name, "child": child} + notify(request, f"s|{_('Claim approved')}|{body}") + else: + # The link is made either way -- say so plainly rather than letting + # the club assume the parent has been told. + body = _("“%(parent)s” is now linked to %(child)s, but the email could not be sent. Ask them to use “Forgot your password?” on the sign-in page.") % {"parent": claim.parent_name, "child": child} + notify(request, f"w|{_('Claim approved, email not sent')}|{body}") return redirect("management:parent_claim_list") diff --git a/members/services/claims.py b/members/services/claims.py index df4ef80..921e69b 100644 --- a/members/services/claims.py +++ b/members/services/claims.py @@ -12,14 +12,23 @@ must never confirm whether a given child exists -- so it takes free text and matches nothing itself. """ +from allauth.account.forms import default_token_generator +from allauth.account.utils import user_pk_to_url_str +from django.conf import settings +from django.contrib.auth import get_user_model +from django.core.mail import send_mail from django.db import transaction from django.db.models import Exists, OuterRef, Q +from django.template.loader import render_to_string +from django.urls import reverse from django.utils import timezone from club.models import ClubMembership from members.models import Family, FamilyMembership, Member, ParentClaim from members.services.family import add_parent_to_family +User = get_user_model() + #: Suggestions are ranked, never auto-applied -- an exact name-and-birthday match #: is still only a suggestion, because the whole point of the queue is that a #: human confirms it. @@ -139,3 +148,36 @@ def reject_claim(claim, *, reviewed_by=None, note=""): claim.note = note claim.save(update_fields=["status", "reviewed_by", "reviewed_at", "note"]) return claim + + +def send_claim_approved_email(claim, *, child, request=None): + """Tell the parent their account is ready, with a link that sets their password. + + A real one-time link rather than "go to the reset page and type your email": + the account was created for them with no usable password, so being told to + "reset" something they never had reads as an error. Built with allauth's own + token generator so it lands in the same flow the login page would send them + to, rather than a second, parallel one that could drift out of step with it. + + Never fatal: an approved claim is a real link in the database whether or not + the mail leaves the building, and losing that link because a mail server was + briefly unreachable would be far worse than a parent needing a nudge. + """ + user = User.objects.filter(email__iexact=claim.parent_email).first() + if user is None: + return False + + path = reverse("account_reset_password_from_key", kwargs={"uidb36": user_pk_to_url_str(user), "key": default_token_generator.make_token(user)}) + set_password_url = request.build_absolute_uri(path) if request is not None else path + + context = {"club": claim.club, "child": child, "parent_first_name": claim.parent_first_name, "set_password_url": set_password_url} + subject = " ".join(render_to_string("members/email/claim_approved_subject.txt", context).split()) + body = render_to_string("members/email/claim_approved.txt", context).strip() + "\n" + + try: + send_mail(subject, body, settings.DEFAULT_FROM_EMAIL, [claim.parent_email], fail_silently=False) + except OSError: + # Anything the mail backend raises for an unreachable server or a refused + # connection. The link stands; the club can resend from the queue. + return False + return True diff --git a/members/templates/members/email/claim_approved.txt b/members/templates/members/email/claim_approved.txt new file mode 100644 index 0000000..3a54474 --- /dev/null +++ b/members/templates/members/email/claim_approved.txt @@ -0,0 +1,13 @@ +{% load i18n %}{% blocktrans with name=parent_first_name %}Hello {{ name }},{% endblocktrans %} + +{% blocktrans with club=club.name child=child %}{{ club }} has confirmed that you're {{ child }}'s parent or guardian, and your account is ready.{% endblocktrans %} + +{% trans "Set your password here:" %} +{{ set_password_url }} +{% blocktrans %}That link is for you alone — please don't forward it.{% endblocktrans %} + +{% blocktrans %}Once you're signed in you'll see the children linked to you.{% endblocktrans %} +{% if club.contact_email %} +{% blocktrans with email=club.contact_email %}Something not right? Reply to this note or write to {{ email }}.{% endblocktrans %} +{% endif %} +{% blocktrans with club=club.name %}— {{ club }}{% endblocktrans %} diff --git a/members/templates/members/email/claim_approved_subject.txt b/members/templates/members/email/claim_approved_subject.txt new file mode 100644 index 0000000..50337f9 --- /dev/null +++ b/members/templates/members/email/claim_approved_subject.txt @@ -0,0 +1 @@ +{% load i18n %}{% blocktrans with club=club.name %}Your {{ club }} account is ready{% endblocktrans %} diff --git a/members/templates/members/parent_claim_submitted.html b/members/templates/members/parent_claim_submitted.html deleted file mode 100644 index 5cebfb1..0000000 --- a/members/templates/members/parent_claim_submitted.html +++ /dev/null @@ -1,20 +0,0 @@ -{% extends "_club_base.html" %} -{% load i18n lucide %} - -{% block head_title %}{% trans "Request sent" %}{% endblock head_title %} - -{% block main %} - {% comment %} - Says the same thing whether or not the child was found: this page is public, - so confirming a match would let anyone test which children the club has. - {% endcomment %} -
-
-
-

{% lucide "circle-check" size=20 %} {% trans "Request sent" %}

-

{% blocktrans with club=club.name %}Thanks — {{ club }} will check this against their records.{% endblocktrans %}

-

{% blocktrans %}If it matches, you'll get an email with a link to set your password. If you don't hear anything, get in touch with the club directly.{% endblocktrans %}

-
-
-
-{% endblock main %} diff --git a/members/views.py b/members/views.py index dde2340..c84984a 100644 --- a/members/views.py +++ b/members/views.py @@ -6,9 +6,11 @@ staff, and ClubStaffRequiredMixin would (rightly) turn them away. from django.contrib.auth.mixins import LoginRequiredMixin from django.http import Http404 -from django.shortcuts import render +from django.shortcuts import redirect +from django.utils.translation import gettext_lazy as _ from django.views.generic import FormView, TemplateView +from controlpanel.messages import notify from members.forms import ParentClaimForm from members.models import FamilyMembership, Member from members.services.claims import submit_claim @@ -39,7 +41,17 @@ class ParentClaimView(ClubScopedPublicMixin, FormView): def form_valid(self, form): submit_claim(self.request.club, **form.cleaned_data) - return render(self.request, "members/parent_claim_submitted.html", {"club": self.request.club}) + + club = self.request.club + title = _("Request received") + body = _("%(club)s will check this against their records. If it matches, you'll get an email with a link to set your password.") % {"club": club.name} + if club.contact_email: + body += " " + _("Any questions, write to %(email)s.") % {"email": club.contact_email} + # Worded identically whether or not that child exists, and sent before any + # lookup happens at all -- a message that differed would tell an anonymous + # submitter which children the club has. + notify(self.request, f"s|{title}|{body}") + return redirect("members:parent_claim") class MyFamilyView(ClubScopedPublicMixin, LoginRequiredMixin, TemplateView):