fix(account): one form and one Save for your account; the same for an account's page in Users

Operator review of the Account page: two sections with two different
save buttons, one stuck to the next card and one beside a squeezed hint.
- Account: time zone and Email are one form; POST /account/ saves both,
  nothing unless all of it is valid. /account/email is gone.
- Users › account: Name and Email are one form; the rename route also
  saves the email switches. /settings/users/<id>/email is gone.
- The Email section is a plain section now (email_section); its hint sits
  under the switches, names the zone as the picker does, and links
  Notifications only for an admin.
- apply_email_form leaves a switch the form doesn't carry as it is.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

steward/account/routes.py

@@ -1,5 +1,6 @@
 """The signed-in user's own account (milestone 490): the time zone every
-page writes its times in. Any role reaches it, for themselves only."""
+page writes its times in, and their email (milestone 495). Any role reaches
+it, for themselves only."""
 from __future__ import annotations
 
 from quart import Blueprint, abort, current_app, redirect, render_template, request, session
@@ -26,48 +27,34 @@ async def _me(db) -> User:
 
 @account_bp.get("/")
 @require_role(UserRole.viewer)
-async def account(error: str | None = None, status: int = 200, email_error: str | None = None):
+async def account(error: str | None = None, status: int = 200):
     from steward.core.mail import role_defaults
     from steward.core.settings import get_all_settings
     async with current_app.db_sessionmaker() as db:
         user = await _me(db)
         defaults = role_defaults(user, await get_all_settings(db))
     tz = formatting.zone(user.time_zone)
+    zone_name = user.time_zone or "UTC"
     return await render_template(
         "account/index.html", user=user, source=_SOURCES.get(user.auth_source, user.auth_source),
-        zone_name=user.time_zone or "UTC", zone_label=formatting.tz_label(tz),
+        zone_name=zone_name, zone_title=zone_name.split("/", 1)[-1].replace("_", " "),
-        zones=formatting.zones_by_region(), defaults=defaults, error=error,
+        zone_label=formatting.tz_label(tz), zones=formatting.zones_by_region(),
-        email_error=email_error), status
+        defaults=defaults, error=error), status
 
 
 @account_bp.post("/")
 @require_role(UserRole.viewer)
 async def account_save():
-    from steward.core.audit import log_audit
+    """The page's one form: your time zone, your address and whether you're
-    name = ((await request.form).get("time_zone") or "").strip()
+    emailed alerts and the weekly report (milestone 495). Nothing is saved
-    if name not in formatting.zone_names():
+    unless all of it is valid."""
-        return await account(f"{name or 'That'} is not a time zone Steward knows.", 400)
-    async with current_app.db_sessionmaker() as db:
-        async with db.begin():
-            user = await _me(db)
-            before = user.time_zone
-            user.time_zone = name
-    session["tz"] = name
-    if before != name:
-        await log_audit(current_app, user.id, user.username, "account.time_zone",
-                        detail={"time_zone": name, "was": before or "UTC"})
-    return redirect("/account/")
-
-
-@account_bp.post("/email")
-@require_role(UserRole.viewer)
-async def account_email():
-    """Your address and whether you're emailed alerts and the weekly report
-    (milestone 495)."""
     from sqlalchemy.exc import IntegrityError
     from steward.core.audit import log_audit
     from steward.core.mail import apply_email_form
     form = await request.form
+    name = (form.get("time_zone") or "").strip()
+    if name not in formatting.zone_names():
+        return await account(f"{name or 'That'} is not a time zone Steward knows.", 400)
     try:
         async with current_app.db_sessionmaker() as db:
             async with db.begin():
@@ -75,13 +62,19 @@ async def account_email():
                 changes, error = apply_email_form(user, form)
                 if error:
                     raise ValueError(error)
+                before = user.time_zone
+                user.time_zone = name
     except ValueError as exc:
-        return await account(status=400, email_error=str(exc))
+        return await account(str(exc), 400)
     except IntegrityError:
-        return await account(status=400, email_error="Another account already uses that address.")
+        return await account("Another account already uses that address.", 400)
+    session["tz"] = name
+    if before != name:
+        await log_audit(current_app, user.id, user.username, "account.time_zone",
+                        detail={"time_zone": name, "was": before or "UTC"})
     if changes:
         await log_audit(current_app, user.id, user.username, "account.email", detail=changes)
-    return redirect("/account/#email")
+    return redirect("/account/")
 
 
 @account_bp.post("/time-zone/adopt")

steward/core/mail.py

@@ -113,8 +113,8 @@ def role_defaults(user, settings: dict) -> dict[str, bool]:
 
 
 def apply_email_form(user, form) -> tuple[dict, str | None]:
-    """Apply an email form to `user`: "email" (when the form has it) and
+    """Apply an email form to `user`: "email" and "notify_alerts" /
-    "notify_alerts" / "notify_report" ("", "on" or "off"). Returns
+    "notify_report" ("", "on" or "off"), each only when the form has it. Returns
     ({field: [before, after]} for what changed, None), or ({}, why) having
     changed nothing."""
     wanted = {}
@@ -124,7 +124,9 @@ def apply_email_form(user, form) -> tuple[dict, str | None]:
             return {}, "That isn't an email address mail can reach."
         wanted["email"] = address
     for kind in KINDS:
-        value = form.get(f"notify_{kind}", "")
+        if f"notify_{kind}" not in form:
+            continue
+        value = form.get(f"notify_{kind}")
         if value not in CHOICES:
             return {}, "Pick On, Off or the role's default."
         wanted[f"notify_{kind}"] = CHOICES[value]

steward/settings/users.py

@@ -156,7 +156,6 @@ async def user_create():
 
 
 _DONE = {"created": "Account added.", "saved": "Saved.", "role": "Role changed.",
-         "email": "Email settings saved.",
          "password": "Password set. It works from their next sign-in.",
          "disabled": "Account disabled. They are signed out at their next click.",
          "enabled": "Account enabled."}
@@ -190,18 +189,27 @@ async def user_detail(user_id: str):
 @users_bp.post("/<user_id>")
 @require_role(UserRole.admin)
 async def user_save(user_id: str):
-    """Rename: the username and email, for any account."""
+    """The page's one form, for any account: its username and email, and
+    whether it is emailed alerts and the weekly report (milestone 495). The
+    address may be a provider's placeholder, so it isn't held to mail's test
+    here; the account just gets no email until it is real."""
     form = await request.form
     username = (form.get("username") or "").strip()
     email = (form.get("email") or "").strip()
     if not username or not email or len(username) > 64 or len(email) > 255:
         return await _user_page(user_id, "A username (up to 64 characters) and an email are both needed.", 400)
+    switches = {k: v for k, v in form.items() if k != "email"}
     try:
         async with current_app.db_sessionmaker() as db:
             async with db.begin():
                 user = await _target(db, user_id)
+                emailed, error = mail.apply_email_form(user, switches)
+                if error:
+                    raise ValueError(error)
                 before = {"username": user.username, "email": user.email}
                 user.username, user.email = username, email
+    except ValueError as exc:
+        return await _user_page(user_id, str(exc), 400)
     except IntegrityError:
         return await _user_page(user_id, f"Another account already uses the username {username} or that email.", 400)
     after = {"username": username, "email": email}
@@ -211,6 +219,8 @@ async def user_save(user_id: str):
                             | {f"old_{k}": before[k] for k in after if before[k] != after[k]})
         if user_id == session.get("user_id"):
             session["username"] = username
+    if emailed:
+        await audit_request("user.email_settings", "user", user_id, {"username": username} | emailed)
     return redirect(f"/settings/users/{user_id}?done=saved")
 
 
@@ -240,23 +250,6 @@ async def user_role(user_id: str):
     return redirect(f"/settings/users/{user_id}?done=role")
 
 
-@users_bp.post("/<user_id>/email")
-@require_role(UserRole.admin)
-async def user_email(user_id: str):
-    """Whether this account is emailed alerts and the weekly report (milestone
-    495); its address is edited with its name."""
-    form = {k: v for k, v in (await request.form).items() if k != "email"}
-    async with current_app.db_sessionmaker() as db:
-        async with db.begin():
-            user = await _target(db, user_id)
-            changes, error = mail.apply_email_form(user, form)
-    if error:
-        return await _user_page(user_id, error, 400)
-    if changes:
-        await audit_request("user.email_settings", "user", user_id, {"username": user.username} | changes)
-    return redirect(f"/settings/users/{user_id}?done=email")
-
-
 @users_bp.post("/<user_id>/password")
 @require_role(UserRole.admin)
 async def user_password(user_id: str):

steward/templates/account/_email_choices.html

@@ -1,34 +1,36 @@
-{# The email form for one account (milestone 495): its address, and whether
+{# The Email section for one account (milestone 495): its address, and whether
-   it gets alert emails and the weekly report. Used by the Account page (your
+   it gets alert emails and the weekly report. A section of the page's one
-   own) and an account's page in Settings › Users (an admin's view).
+   form, saved by that form's Save: the Account page (your own) and an
-   `defaults` is {kind: bool}, the account's role defaults. #}
+   account's page in Settings › Users (an admin's view, whose address sits
-{% macro email_choices(user, defaults, action, error=none, with_address=true) -%}
+   with its name). `defaults` is {kind: bool}, the account's role defaults;
-<form method="post" action="{{ action }}" class="form-page" id="email">{{ csrf_field() }}
+   `zone_note` says whose zone the emails' times are in. #}
-  <section class="card form-section" aria-labelledby="email-title">
+{% macro email_section(user, defaults, zone_note, with_address=true, unreachable=false) -%}
-    <h2 id="email-title" class="section-title">Email</h2>
+<section class="card form-section" aria-labelledby="email-title" id="email">
-    {% if error %}<p class="field-error" role="alert">{{ error }}</p>{% endif %}
+  <h2 id="email-title" class="section-title">Email</h2>
-    {% if with_address %}
+  {% if with_address %}
+  <div class="field">
+    <label class="field-label" for="email-address">Address</label>
+    <input class="input" id="email-address" type="email" name="email" maxlength="255" value="{{ user.email }}" required>
+    {% if user.auth_source != "local" %}<span class="field-hint">Your sign-in provider set this first; change it if mail doesn't reach it.</span>{% endif %}
+  </div>
+  {% elif unreachable %}
+  <p class="field-hint tone-warn">{{ user.email }} isn't an address mail can reach, so this account gets no email. Set a real one above.</p>
+  {% endif %}
+  <div class="form-pair">
+    {% for kind, label in (("alerts", "Alert emails"), ("report", "Weekly report")) %}
+    {% set choice = user["notify_" ~ kind] %}
     <div class="field">
-      <label class="field-label" for="email-address">Address</label>
+      <label class="field-label" for="email-{{ kind }}">{{ label }}</label>
-      <input class="input" id="email-address" type="email" name="email" maxlength="255" value="{{ user.email }}" required>
+      <select class="input" id="email-{{ kind }}" name="notify_{{ kind }}">
-      {% if user.auth_source != "local" %}<span class="field-hint">Your sign-in provider set this first; change it if mail doesn't reach it.</span>{% endif %}
+        <option value=""{% if choice is none %} selected{% endif %}>As for {{ user.role.value }}s ({{ "on" if defaults[kind] else "off" }})</option>
+        <option value="on"{% if choice is sameas true %} selected{% endif %}>On</option>
+        <option value="off"{% if choice is sameas false %} selected{% endif %}>Off</option>
+      </select>
     </div>
-    {% endif %}
+    {% endfor %}
-    <div class="form-pair">
+  </div>
-      {% for kind, label in (("alerts", "Alert emails"), ("report", "Weekly report")) %}
+  <p class="field-hint">Times in these emails are in {{ zone_note }}.
-      {% set choice = user["notify_" ~ kind] %}
+    {% if session.user_role == "admin" %}Each role's default is set in Settings › <a href="/settings/notifications/">Notifications</a>.
-      <div class="field">
+    {%- else %}An admin sets each role's default.{% endif %}</p>
-        <label class="field-label" for="email-{{ kind }}">{{ label }}</label>
+</section>
-        <select class="input" id="email-{{ kind }}" name="notify_{{ kind }}">
-          <option value=""{% if choice is none %} selected{% endif %}>As for {{ user.role.value }}s ({{ "on" if defaults[kind] else "off" }})</option>
-          <option value="on"{% if choice is sameas true %} selected{% endif %}>On</option>
-          <option value="off"{% if choice is sameas false %} selected{% endif %}>Off</option>
-        </select>
-      </div>
-      {% endfor %}
-    </div>
-    <span class="field-hint">Times in these emails are in {{ user.time_zone or "UTC" }}. A role's default is set in Settings › Notifications.</span>
-    <button type="submit" class="btn btn-ghost">Save email settings</button>
-  </section>
-</form>
 {%- endmacro %}

steward/templates/account/index.html

@@ -1,29 +1,32 @@
 {# account/index.html — the signed-in user's own account (milestone 490): who
-   they are, and the time zone every page writes its times in. #}
+   they are, the time zone every page writes its times in, and their email
+   (milestone 495). One form, one Save. #}
 {% extends "base.html" %}
 {% from "_macros.html" import page_header, zone_select %}
-{% from "account/_email_choices.html" import email_choices with context %}
+{% from "account/_email_choices.html" import email_section with context %}
 {% block title %}Your account — Steward{% endblock %}
 {% block content %}
 {{ page_header("Your account") }}
 
 <p class="page-verdict">
   Signed in as <strong>{{ user.username }}</strong> ({{ user.role.value }}) with {{ source }}
-  · times show in <strong>{{ zone_name }}</strong> ({{ zone_label }})
+  · times show in <strong>{{ zone_title }}</strong> ({{ zone_label }})
 </p>
 
+{% if error %}<div class="alert alert-error" role="alert">{{ error }}</div>{% endif %}
+
 <form method="post" action="/account/" class="form-page">{{ csrf_field() }}
   <section class="card form-section" aria-labelledby="acct-time">
     <h2 id="acct-time" class="section-title">Time zone</h2>
-    {% if error %}<p class="field-error" role="alert">{{ error }}</p>{% endif %}
     <div class="field">
       <label class="field-label" for="acct-tz">Show times in</label>
       {{ zone_select("time_zone", "acct-tz", zone_name, zones) }}
       <span class="field-hint">Every page writes its times in this zone, on a 24-hour clock.</span>
     </div>
   </section>
-  <button type="submit" class="btn">Save</button>
-</form>
 
-{{ email_choices(user, defaults, "/account/email", email_error) }}
+  {{ email_section(user, defaults, "your time zone") }}
+
+  <div class="form-actions"><button type="submit" class="btn">Save</button></div>
+</form>
 {% endblock %}

steward/templates/settings/user.html

@@ -5,7 +5,7 @@
 {% extends "base.html" %}
 {% from "_macros.html" import crumbs, page_header, status_badge %}
 {% from "settings/_tabs.html" import settings_tabs %}
-{% from "account/_email_choices.html" import email_choices with context %}
+{% from "account/_email_choices.html" import email_section with context %}
 {% block title %}{{ user.username }} — Users — Steward{% endblock %}
 {% block section_tabs %}{{ settings_tabs("users") }}{% endblock %}
 {% block breadcrumb %}{{ crumbs([("Settings", "/settings/"), ("Users", "/settings/users/"), (user.username, "")]) }}{% endblock %}
@@ -38,12 +38,13 @@
       </div>
     </div>
     {% if not local %}<p class="field-hint">{{ source }} finds this account by its own id, not by name, so renaming it here does not stop it signing in.</p>{% endif %}
-    <button type="submit" class="btn">Save</button>
   </section>
-</form>
 
-{% if not reachable %}<p class="field-hint tone-warn">{{ user.email }} isn't an address mail can reach, so this account gets no email. Set a real one above.</p>{% endif %}
+  {% set zone = (user.time_zone or "UTC").split("/", 1)[-1]|replace("_", " ") %}
-{{ email_choices(user, defaults, "/settings/users/" ~ user.id ~ "/email", with_address=false) }}
+  {{ email_section(user, defaults, "this account's own zone (" ~ zone ~ ")", with_address=false, unreachable=not reachable) }}
+
+  <div class="form-actions"><button type="submit" class="btn">Save</button></div>
+</form>
 
 <section class="card form-section" aria-labelledby="u-role">
   <h2 id="u-role" class="section-title">Role</h2>

tests/integration/test_account_time_zone.py

@@ -73,7 +73,7 @@ def test_the_browser_zone_is_adopted_once_and_the_account_page_changes_it(app):
     assert out["adopt"] == {"adopted": True} and out["after_adopt"] == "America/New_York"
     assert out["again"] == {"adopted": False}
     page = out["page"]
-    assert "times show in <strong>America/New_York</strong> (E" in page        # EDT or EST
+    assert "times show in <strong>New York</strong> (E" in page               # EDT or EST
     assert '<option value="America/New_York" selected>New York</option>' in page
     assert "/account/time-zone/adopt" not in page                              # nothing left to adopt
     assert out["save"] == (302, "/account/") and out["after_save"] == "Europe/London"

tests/integration/test_users_page.py

@@ -174,25 +174,27 @@ def test_an_account_sets_its_own_address_and_switches_and_an_admin_can_too(app):
     async def _go():
         viewer, vid = await _client(app, role="viewer")
         page = await (await viewer.get("/account/")).get_data(as_text=True)
-        assert 'action="/account/email"' in page and "As for viewers (on)" in page
+        assert page.count("<form") == 1 and 'name="notify_alerts"' in page and "As for viewers (on)" in page
 
-        resp = await viewer.post("/account/email", headers=CSRF, form={
+        mine = {"time_zone": "Europe/Paris", "notify_alerts": "off", "notify_report": ""}
-            "email": "nope@oidc", "notify_alerts": "off", "notify_report": ""})
+        resp = await viewer.post("/account/", headers=CSRF, form=mine | {"email": "nope@oidc"})
         assert resp.status_code == 400 and "mail can reach" in await resp.get_data(as_text=True)
+        assert (await _user(app, vid)).time_zone is None            # nothing saved from a bad form
         address = f"v-{vid[:8]}@example.com"
-        assert (await viewer.post("/account/email", headers=CSRF, form={
+        assert (await viewer.post("/account/", headers=CSRF, form=mine | {"email": address})).status_code == 302
-            "email": address, "notify_alerts": "off", "notify_report": ""})).status_code == 302
         me = await _user(app, vid)
-        assert (me.email, me.notify_alerts, me.notify_report) == (address, False, None)
+        assert (me.email, me.notify_alerts, me.notify_report, me.time_zone) == (address, False, None, "Europe/Paris")
-        assert [a for a, _ in await _audits(app, vid)] == ["account.email"]
+        assert [a for a, _ in await _audits(app, vid)] == ["account.time_zone", "account.email"]
 
         admin, aid = await _client(app)
-        resp = await admin.post(f"/settings/users/{vid}/email", headers=CSRF,
+        page = await (await admin.get(f"/settings/users/{vid}")).get_data(as_text=True)
-                                form={"email": "ignored@example.com", "notify_alerts": "", "notify_report": "off"})
+        assert 'name="notify_report"' in page and "own zone (Paris)" in page
+        resp = await admin.post(f"/settings/users/{vid}", headers=CSRF, form={
+            "username": me.username, "email": address, "notify_alerts": "", "notify_report": "off"})
         assert resp.status_code == 302
         me = await _user(app, vid)
-        assert (me.email, me.notify_alerts, me.notify_report) == (address, None, False)  # address kept
+        assert (me.email, me.notify_alerts, me.notify_report) == (address, None, False)
-        assert [a for a, _ in await _audits(app, aid)] == ["user.email_settings"]
+        assert [a for a, _ in await _audits(app, aid)] == ["user.email_settings"]   # not renamed
 
         page = await (await admin.get("/settings/notifications/")).get_data(as_text=True)
         assert "Who gets email" in page and 'name="notify.alerts.viewer"' in page and "Also send to" in page