diff --git a/authentication/tests.py b/authentication/tests.py index 11b421e..a7806be 100644 --- a/authentication/tests.py +++ b/authentication/tests.py @@ -1,5 +1,6 @@ import re import uuid +from types import SimpleNamespace from urllib.parse import parse_qs, urlparse from allauth.core import context @@ -7,8 +8,10 @@ from allauth.mfa.models import Authenticator from allauth.mfa.recovery_codes.internal.auth import RecoveryCodes from django.contrib.auth import get_user_model from django.contrib.auth.models import AnonymousUser +from django.core import mail from django.db import IntegrityError from django.http import HttpResponse +from django.template.loader import render_to_string from django.test import RequestFactory, TestCase, override_settings from django.urls import reverse @@ -262,6 +265,65 @@ class AuthFormRenderingTests(TestCase): self.assertNotContains(response, 'name="password1"', status_code=403) +@override_settings( + ROSTERCHIEF_BASE_DOMAIN="rosterchief.app", + ALLOWED_HOSTS=["rosterchief.app", "ajax-united.rosterchief.app", "testserver"], +) +class PasswordResetEmailTests(TestCase): + """allauth auto-attaches templates/account/email/password_reset_key_message.html + as an HTML alternative next to its own .txt body -- see + allauth.account.adapter.DefaultAccountAdapter.render_mail, which looks for + "_message." for ext in [TEMPLATE_EXTENSION ("html", unset here), "txt"]. + No Python override needed; this only exercises the template.""" + + def test_the_html_email_carries_the_clubs_branding_on_a_club_subdomain(self): + Club.objects.create(name="Ajax United", slug="ajax-united") + User.objects.create_user(email="parent@example.com", password="pw-secret-123") + + response = self.client.post(reverse("account_reset_password"), {"email": "parent@example.com"}, HTTP_HOST="ajax-united.rosterchief.app") + + self.assertEqual(response.status_code, 302) + self.assertEqual(len(mail.outbox), 1) + [(html_body, mimetype)] = mail.outbox[0].alternatives + self.assertEqual(mimetype, "text/html") + self.assertIn("Ajax United", html_body) + self.assertIn("/accounts/password/reset/key/", html_body) + + def test_the_html_email_falls_back_to_rosterchief_branding_off_a_club_subdomain(self): + # The base domain has no tenant, e.g. a platform control-panel user + # resetting their own password -- club.context_processors.branding + # leaves `club` unset there, so the template must not assume one. + User.objects.create_user(email="admin@example.com", password="pw-secret-123") + + self.client.post(reverse("account_reset_password"), {"email": "admin@example.com"}) + + self.assertEqual(len(mail.outbox), 1) + [(html_body, mimetype)] = mail.outbox[0].alternatives + self.assertEqual(mimetype, "text/html") + # The wordmark splits "Chief" into its own for the sky-blue accent + # (matching templates/_platform_base.html), so the two halves aren't + # contiguous text in the raw HTML -- check for both rather than the + # combined word. + self.assertIn("Roster", html_body) + self.assertIn("Chief", html_body) + + def test_the_password_reset_key_html_template_renders_directly(self): + # Mirrors authentication.tests.AuthFormRenderingTests' direct-render + # style: exercises the template's own branching (club vs. none, logo + # vs. initials) without going through the full request/email pipeline. + club = Club.objects.create(name="Ajax United", slug="ajax-united", primary_color="#1e40af") + + with_club = render_to_string("account/email/password_reset_key_message.html", {"club": club, "password_reset_url": "https://ajax-united.rosterchief.app/accounts/password/reset/key/abc-def/"}) + self.assertIn("Ajax United", with_club) + self.assertIn("AU", with_club) # initials fallback: no logo set + self.assertIn("https://ajax-united.rosterchief.app/accounts/password/reset/key/abc-def/", with_club) + + without_club = render_to_string("account/email/password_reset_key_message.html", {"club": None, "password_reset_url": "https://rosterchief.app/accounts/password/reset/key/abc-def/", "current_site": SimpleNamespace(name="rosterchief.app")}) + self.assertIn("Roster", without_club) + self.assertIn("Chief", without_club) + self.assertIn("https://rosterchief.app/accounts/password/reset/key/abc-def/", without_club) + + class TwoFactorPageTests(TestCase): @classmethod def setUpTestData(cls): diff --git a/club/templatetags/__init__.py b/club/templatetags/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/club/templatetags/club_email.py b/club/templatetags/club_email.py new file mode 100644 index 0000000..d9e75f8 --- /dev/null +++ b/club/templatetags/club_email.py @@ -0,0 +1,25 @@ +"""Template helper shared by every HTML email that shows a club's logo. + +An in an email has no page to resolve a relative /media/... URL against +the way a browser tab would -- the inbox fetches it cold. Storage backends that +already return an absolute URL (e.g. S3 in production) are unaffected; this +only matters for the local FileSystemStorage used in dev, where FieldFile.url +is relative. +""" + +from django import template + +register = template.Library() + + +@register.simple_tag(takes_context=True) +def absolute_media_url(context, file_field): + """An absolute URL for ``file_field`` (e.g. ``club.logo``), for use in an + email. Falls back to the field's own (possibly relative) ``.url`` when + there's no request in the template context to build an absolute one from -- + better a relative URL than a hard error while rendering the email.""" + if not file_field: + return "" + + request = context.get("request") + return request.build_absolute_uri(file_field.url) if request is not None else file_field.url diff --git a/management/tests.py b/management/tests.py index b883c3b..80a5358 100644 --- a/management/tests.py +++ b/management/tests.py @@ -1501,6 +1501,21 @@ class ParentClaimViewTests(ManagementTestBase): self.assertEqual(response.status_code, 200) self.assertContains(response, "password1") + def test_the_email_carries_an_html_alternative_alongside_the_plain_text(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) + + sent = mail.outbox[0] + [(html_body, mimetype)] = sent.alternatives + self.assertEqual(mimetype, "text/html") + self.assertIn("Jamie", html_body) + self.assertIn(self.club.name, html_body) + [reset_path] = [line for line in sent.body.splitlines() if "/accounts/password/reset/key/" in line] + self.assertIn(reset_path.strip(), html_body) + 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"]) @@ -1517,7 +1532,7 @@ class ParentClaimViewTests(ManagementTestBase): 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")): + with mock.patch("members.services.claims.EmailMultiAlternatives.send", 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")) diff --git a/members/services/claims.py b/members/services/claims.py index 921e69b..8b7f39b 100644 --- a/members/services/claims.py +++ b/members/services/claims.py @@ -16,7 +16,7 @@ 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.core.mail import EmailMultiAlternatives from django.db import transaction from django.db.models import Exists, OuterRef, Q from django.template.loader import render_to_string @@ -170,12 +170,20 @@ def send_claim_approved_email(claim, *, child, request=None): 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} + # "request" rides along in the context (not used by the .txt template, only + # by the .html one's {% absolute_media_url %} tag) so the club's logo -- if + # it has one -- renders as an absolute URL an inbox can actually fetch, the + # same way set_password_url above is made absolute. + context = {"club": claim.club, "child": child, "parent_first_name": claim.parent_first_name, "set_password_url": set_password_url, "request": request} 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" + text_body = render_to_string("members/email/claim_approved.txt", context).strip() + "\n" + html_body = render_to_string("members/email/claim_approved.html", context) + + message = EmailMultiAlternatives(subject, text_body, settings.DEFAULT_FROM_EMAIL, [claim.parent_email]) + message.attach_alternative(html_body, "text/html") try: - send_mail(subject, body, settings.DEFAULT_FROM_EMAIL, [claim.parent_email], fail_silently=False) + message.send(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. diff --git a/members/templates/members/email/claim_approved.html b/members/templates/members/email/claim_approved.html new file mode 100644 index 0000000..af13020 --- /dev/null +++ b/members/templates/members/email/claim_approved.html @@ -0,0 +1,58 @@ +{% extends "email/_base.html" %} +{% load i18n club_email %} + +{% comment %} + HTML sibling of claim_approved.txt -- same content, same context (club, + child, parent_first_name, set_password_url), just laid out for an inbox + instead of a terminal. Kept in lockstep with the .txt version by hand: + there's no single source the two are generated from, so a change to one + reads as incomplete without the other. +{% endcomment %} + +{% block title %}{% blocktrans with club=club.name %}Your {{ club }} account is ready{% endblocktrans %}{% endblock title %} + +{% block preheader %}{% blocktrans with club=club.name %}{{ club }} has confirmed you're a parent or guardian -- set your password to sign in.{% endblocktrans %}{% endblock preheader %} + +{% block header %} + {% absolute_media_url club.logo as logo_url %} + + + + + +
+ {% if club.logo %} + {{ club.name }} + {% else %} + + + + +
+ {{ club.initials }} +
+ {% endif %} + 		+ {{ club.name }} +
+{% endblock header %} + +{% block content %} +

{% 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" as set_password_label %} + {% include "email/_button.html" with href=set_password_url label=set_password_label bg=club.primary_color|default:"#4f46e5" fg=club.primary_content_color|default:"#ffffff" %} + +

{% trans "That link is for you alone — please don't forward it." %}

+ +

{% trans "Once you're signed in you'll see the children linked to you." %}

+{% endblock content %} + +{% block footer %} + {% 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 %}

+{% endblock footer %} diff --git a/templates/account/email/password_reset_key_message.html b/templates/account/email/password_reset_key_message.html new file mode 100644 index 0000000..866a186 --- /dev/null +++ b/templates/account/email/password_reset_key_message.html @@ -0,0 +1,78 @@ +{% extends "email/_base.html" %} +{% load i18n club_email %} + +{% comment %} + allauth's DefaultAccountAdapter.render_mail looks for + "_message." (TEMPLATE_EXTENSION + defaults to "html", unset in this project) next to the .txt template it + already renders, and attaches it as the HTML alternative automatically -- + no Python override needed, see allauth.account.adapter.render_mail. + + Context comes from allauth.account.internal.flows.password_reset + .request_password_reset() plus DefaultAccountAdapter.send_mail: user, + password_reset_url, uid, key, request, current_site, email, and username + only when ACCOUNT_LOGIN_METHODS includes "username" (it doesn't here, see + rosterchief/settings.py -- login is email-only -- but the check is kept for + parity with the .txt template in case that ever changes). + + render_to_string is called with the real `request` + (allauth.account.adapter.render_mail passes `context.request`), so this + picks up club.context_processors.branding like any other page: `club` is + set when the reset was requested on a club subdomain, None on the base + domain (e.g. a platform control-panel user). Both cases must render + something sensible -- a password reset is exactly the kind of email that + still has to work with no club in play. +{% endcomment %} + +{% block title %}{% trans "Reset your password" %}{% endblock title %} + +{% block preheader %}{% trans "Use this link to choose a new password. If you didn't request this, you can ignore this email." %}{% endblock preheader %} + +{% block header %} + {% if club %} + {% absolute_media_url club.logo as logo_url %} + + + + + +
+ {% if club.logo %} + {{ club.name }} + {% else %} + + + + +
+ {{ club.initials }} +
+ {% endif %} + 		+ {{ club.name }} +
+ {% else %} + RosterChief + {% endif %} +{% endblock header %} + +{% block content %} +

{% trans "You're receiving this email because you or someone else has requested a password reset for your user account. It can be safely ignored if you did not request one." %}

+ + {% trans "Reset your password" as reset_label %} + {% include "email/_button.html" with href=password_reset_url label=reset_label bg=club.primary_color|default:"#0ea5e9" fg=club.primary_content_color|default:"#ffffff" %} + + {% if username %} +

{% blocktrans %}In case you forgot, your username is {{ username }}.{% endblocktrans %}

+ {% endif %} +{% endblock content %} + +{% block footer %} +

+ {% if club %} + {% blocktrans with club=club.name %}Sent by {{ club }}, powered by RosterChief.{% endblocktrans %} + {% else %} + {% blocktrans with site_name=current_site.name %}Sent by {{ site_name }}.{% endblocktrans %} + {% endif %} +

+{% endblock footer %} diff --git a/templates/email/_base.html b/templates/email/_base.html new file mode 100644 index 0000000..67f7202 --- /dev/null +++ b/templates/email/_base.html @@ -0,0 +1,52 @@ +{% load i18n %} +{% comment %} + Shared shell for every transactional HTML email (the claim-approved parent + email, allauth's password-reset override, and anything after it). Deliberately + not a responsive email framework: a single ~600px table with generous padding + reads fine on a phone without media queries, and media queries are one of the + first things Outlook desktop and a lot of webmail strip anyway. + + Everything is inline styles on table-based markup on purpose -- no