Skip to content

Commit

Permalink
Improve the UX on email management
Browse files Browse the repository at this point in the history
The most-frequent actions exposed on this UI make no sense:

- You can't make the only email primary (it already is)
- You can't remove your only email
- Re-sending email to a verified email does nothing

Let's try to mediate this a bit by just controlling what buttons are
available for end users. This is a simple display-only fix that doesn't
require any changes to the `django-allauth` backend.
  • Loading branch information
DavidCain committed Jun 25, 2024
1 parent 46e74b1 commit 715be57
Show file tree
Hide file tree
Showing 3 changed files with 205 additions and 4 deletions.
39 changes: 36 additions & 3 deletions ws/templates/account/email.html
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{% extends "base.html" %}
{% load crispy_forms_tags %}
{% load email_tags %}

{% block head_title %}Emails{% endblock %}

Expand Down Expand Up @@ -33,9 +34,41 @@ <h3>E-mail Addresses</h3>
</div>

<div class="btn-group" role="group">
<button class="btn btn-default" type="submit" name="action_primary">Make Primary</button>
<button class="btn btn-default" type="submit" name="action_send">Re-send Verification</button>
<button class="btn btn-default" type="submit" name="action_remove">Remove</button>
{# Disable if there's only one email #}
{# Always display the button to self-document multi-email support #}
<button
class="btn btn-default"
type="submit"
{% if user.emailaddress_set.all|length < 2 %}
disabled
title="Email address is already primary."
{% endif %}
name="action_primary">
Make Primary
</button>

{# Without using JavaScript, there's no way to disable this per-email #}
{# But we can handle the usual case of *all* emails being verified #}
{% if user.emailaddress_set.all|has_unverified_email %}
<button
class="btn btn-default"
type="submit"
name="action_send">
Re-send Verification
</button>
{% endif %}

{# Always display the button to self-document multi-email support #}
<button
class="btn btn-default"
type="submit"
{% if user.emailaddress_set.all|length < 2 %}
disabled
title="Cannot remove your only email address."
{% endif %}
name="action_remove">
Remove
</button>
</div>
</form>

Expand Down
10 changes: 9 additions & 1 deletion ws/templatetags/email_tags.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
from allauth.account.models import EmailAddress
from django import template
from django.db.models import QuerySet

from ws import enums
from ws.models import Trip

register = template.Library()


def _conditional_rendering(trip):
@register.filter
def has_unverified_email(emails: QuerySet[EmailAddress]) -> bool:
return any(not email.verified for email in emails)


def _conditional_rendering(trip: Trip) -> dict[str, bool]:
return {
"show_program": trip.program_enum != enums.Program.NONE,
"show_trip_type": trip.trip_type_enum != enums.TripType.NONE,
Expand Down
160 changes: 160 additions & 0 deletions ws/tests/views/test_account.py
Original file line number Diff line number Diff line change
Expand Up @@ -270,3 +270,163 @@ def test_attempt_changing_to_insecure_password(self):
form_group = soup.find(class_="has-error")
self.assertTrue(form_group)
self.assertTrue(form_group.find(string="This password is too common."))


class ManageEmailsTest(TestCase):
maxDiff = None

def _get_buttons(self, user):
self.client.force_login(user)
response = self.client.get("/accounts/email/")
soup = BeautifulSoup(response.content, "html.parser")
form = soup.find("form", action="/accounts/email/")
return {btn.text.strip(): btn.attrs for btn in form.find_all("button")}

def test_one_primary_email(self):
"""Test the usual case -- just one verified primary email."""
user = factories.UserFactory(
emailaddress__primary=True,
emailaddress__verified=True,
)

self.assertEqual(
self._get_buttons(user),
{
"Make Primary": {
"class": ["btn", "btn-default"],
"type": "submit",
"disabled": "",
"title": "Email address is already primary.",
"name": "action_primary",
},
# Notably "Re-send Verification" is missing entirely
"Remove": {
"class": ["btn", "btn-default"],
"type": "submit",
"disabled": "",
"title": "Cannot remove your only email address.",
"name": "action_remove",
},
},
)

def test_one_unverified_email(self):
user = factories.UserFactory(emailaddress__verified=False)

self.assertEqual(
self._get_buttons(user),
{
"Make Primary": {
"class": ["btn", "btn-default"],
"type": "submit",
"disabled": "",
"title": "Email address is already primary.",
"name": "action_primary",
},
"Re-send Verification": {
"class": ["btn", "btn-default"],
"type": "submit",
"name": "action_send",
},
"Remove": {
"class": ["btn", "btn-default"],
"type": "submit",
"disabled": "",
"title": "Cannot remove your only email address.",
"name": "action_remove",
},
},
)

def test_two_verified_emails(self):
"""With two emails, we can remove either or make either primary."""
user = factories.UserFactory()
factories.EmailAddressFactory(
user=user,
email="[email protected]",
primary=False,
verified=True,
)

self.assertEqual(
self._get_buttons(user),
{
"Make Primary": {
"class": ["btn", "btn-default"],
"type": "submit",
"name": "action_primary",
},
# Notably "Re-send Verification" is missing entirely -- both are verified!
"Remove": {
"class": ["btn", "btn-default"],
"type": "submit",
"name": "action_remove",
},
},
)

def test_two_emails_but_one_unverified(self):
"""Test the case of a newly-added but still unverified email."""
user = factories.UserFactory(
emailaddress__verified=True,
)
factories.EmailAddressFactory(
user=user,
email="[email protected]",
primary=False,
verified=False,
)

self.assertEqual(
self._get_buttons(user),
{
"Make Primary": {
"class": ["btn", "btn-default"],
"type": "submit",
"name": "action_primary",
},
# Because there's at least one unverified email, we allow re-sending
"Re-send Verification": {
"class": ["btn", "btn-default"],
"type": "submit",
"name": "action_send",
},
"Remove": {
"class": ["btn", "btn-default"],
"type": "submit",
"name": "action_remove",
},
},
)

def test_multiple_unverified(self):
"""You can still add an email even if one is unverified!"""
user = factories.UserFactory()
factories.EmailAddressFactory(
user=user,
email="[email protected]",
primary=False,
verified=False,
)

self.assertEqual(
self._get_buttons(user),
{
"Make Primary": {
"class": ["btn", "btn-default"],
"type": "submit",
"name": "action_primary",
},
# Because there's at least one unverified email, we allow re-sending
"Re-send Verification": {
"class": ["btn", "btn-default"],
"type": "submit",
"name": "action_send",
},
"Remove": {
"class": ["btn", "btn-default"],
"type": "submit",
"name": "action_remove",
},
},
)

0 comments on commit 715be57

Please sign in to comment.