From beae8a33f38a0bcc80a0d74b483fa9cc4bf73c05 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 8 Oct 2026 16:05:41 -0400 Subject: [PATCH] fix(account): one form and one Save for your account; the same for an account's page in Users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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//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 --- steward/account/routes.py | 51 +++++++-------- steward/core/mail.py | 8 ++- steward/settings/users.py | 31 ++++----- steward/templates/account/_email_choices.html | 64 ++++++++++--------- steward/templates/account/index.html | 17 +++-- steward/templates/settings/user.html | 11 ++-- tests/integration/test_account_time_zone.py | 2 +- tests/integration/test_users_page.py | 24 +++---- 8 files changed, 102 insertions(+), 106 deletions(-) diff --git a/steward/account/routes.py b/steward/account/routes.py index d59b536..557ee8e 100644 --- a/steward/account/routes.py +++ b/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), - zones=formatting.zones_by_region(), defaults=defaults, error=error, - email_error=email_error), status + zone_name=zone_name, zone_title=zone_name.split("/", 1)[-1].replace("_", " "), + zone_label=formatting.tz_label(tz), zones=formatting.zones_by_region(), + defaults=defaults, error=error), status @account_bp.post("/") @require_role(UserRole.viewer) async def account_save(): - from steward.core.audit import log_audit - name = ((await request.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) - 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).""" + """The page's one form: your time zone, your address and whether you're + emailed alerts and the weekly report (milestone 495). Nothing is saved + unless all of it is valid.""" 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") diff --git a/steward/core/mail.py b/steward/core/mail.py index ea91f66..f8dcb51 100644 --- a/steward/core/mail.py +++ b/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 - "notify_alerts" / "notify_report" ("", "on" or "off"). Returns + """Apply an email form to `user`: "email" and "notify_alerts" / + "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] diff --git a/steward/settings/users.py b/steward/settings/users.py index 2571b7f..e91af09 100644 --- a/steward/settings/users.py +++ b/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("/") @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("//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("//password") @require_role(UserRole.admin) async def user_password(user_id: str): diff --git a/steward/templates/account/_email_choices.html b/steward/templates/account/_email_choices.html index 6c3cb9e..57d0976 100644 --- a/steward/templates/account/_email_choices.html +++ b/steward/templates/account/_email_choices.html @@ -1,34 +1,36 @@ -{# The email form for one account (milestone 495): its address, and whether - it gets alert emails and the weekly report. Used by the Account page (your - own) and an account's page in Settings › Users (an admin's view). - `defaults` is {kind: bool}, the account's role defaults. #} -{% macro email_choices(user, defaults, action, error=none, with_address=true) -%} -
{{ csrf_field() }} -
-

Email

- {% if error %}{% endif %} - {% if with_address %} +{# The Email section for one account (milestone 495): its address, and whether + it gets alert emails and the weekly report. A section of the page's one + form, saved by that form's Save: the Account page (your own) and an + account's page in Settings › Users (an admin's view, whose address sits + with its name). `defaults` is {kind: bool}, the account's role defaults; + `zone_note` says whose zone the emails' times are in. #} +{% macro email_section(user, defaults, zone_note, with_address=true, unreachable=false) -%} +
+

Email

+ {% if with_address %} +
+ + + {% if user.auth_source != "local" %}Your sign-in provider set this first; change it if mail doesn't reach it.{% endif %} +
+ {% elif unreachable %} +

{{ user.email }} isn't an address mail can reach, so this account gets no email. Set a real one above.

+ {% endif %} +
+ {% for kind, label in (("alerts", "Alert emails"), ("report", "Weekly report")) %} + {% set choice = user["notify_" ~ kind] %}
- - - {% if user.auth_source != "local" %}Your sign-in provider set this first; change it if mail doesn't reach it.{% endif %} + +
- {% endif %} -
- {% for kind, label in (("alerts", "Alert emails"), ("report", "Weekly report")) %} - {% set choice = user["notify_" ~ kind] %} -
- - -
- {% endfor %} -
- Times in these emails are in {{ user.time_zone or "UTC" }}. A role's default is set in Settings › Notifications. - -
- + {% endfor %} + +

Times in these emails are in {{ zone_note }}. + {% if session.user_role == "admin" %}Each role's default is set in Settings › Notifications. + {%- else %}An admin sets each role's default.{% endif %}

+
{%- endmacro %} diff --git a/steward/templates/account/index.html b/steward/templates/account/index.html index 1669304..852c3d9 100644 --- a/steward/templates/account/index.html +++ b/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") }}

Signed in as {{ user.username }} ({{ user.role.value }}) with {{ source }} - · times show in {{ zone_name }} ({{ zone_label }}) + · times show in {{ zone_title }} ({{ zone_label }})

+{% if error %}{% endif %} +
{{ csrf_field() }}

Time zone

- {% if error %}{% endif %}
{{ zone_select("time_zone", "acct-tz", zone_name, zones) }} Every page writes its times in this zone, on a 24-hour clock.
- -
-{{ email_choices(user, defaults, "/account/email", email_error) }} + {{ email_section(user, defaults, "your time zone") }} + +
+ {% endblock %} diff --git a/steward/templates/settings/user.html b/steward/templates/settings/user.html index 8fb52ee..91e57d6 100644 --- a/steward/templates/settings/user.html +++ b/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 @@ {% if not local %}

{{ source }} finds this account by its own id, not by name, so renaming it here does not stop it signing in.

{% endif %} - - -{% if not reachable %}

{{ user.email }} isn't an address mail can reach, so this account gets no email. Set a real one above.

{% endif %} -{{ email_choices(user, defaults, "/settings/users/" ~ user.id ~ "/email", with_address=false) }} + {% set zone = (user.time_zone or "UTC").split("/", 1)[-1]|replace("_", " ") %} + {{ email_section(user, defaults, "this account's own zone (" ~ zone ~ ")", with_address=false, unreachable=not reachable) }} + +
+

Role

diff --git a/tests/integration/test_account_time_zone.py b/tests/integration/test_account_time_zone.py index db799c0..3297b13 100644 --- a/tests/integration/test_account_time_zone.py +++ b/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 America/New_York (E" in page # EDT or EST + assert "times show in New York (E" in page # EDT or EST assert '' 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" diff --git a/tests/integration/test_users_page.py b/tests/integration/test_users_page.py index 0d72716..5e4fa1c 100644 --- a/tests/integration/test_users_page.py +++ b/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("