586 lines
18 KiB
Markdown
586 lines
18 KiB
Markdown
# Profile Form Validation Implementation Plan
|
|
|
|
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.
|
|
|
|
**Goal:** Replace inline profile validation with a shared Pydantic model using `EmailStr` and `pydantic-extra-types` `PhoneNumberValidator` for E.164 phone validation.
|
|
|
|
**Architecture:** New `src/porchlight/validation.py` module with a `ProfileUpdate` Pydantic model. Both manage and admin route handlers parse form data through this model, catch `ValidationError`, and return user-friendly errors via the existing htmx pattern.
|
|
|
|
**Tech Stack:** Pydantic v2 (`EmailStr`), `pydantic-extra-types[phonenumbers]` (Google's libphonenumber), FastAPI form handling.
|
|
|
|
---
|
|
|
|
### Task 1: Add `pydantic-extra-types[phonenumbers]` dependency
|
|
|
|
**Files:**
|
|
- Modify: `pyproject.toml:7-22`
|
|
|
|
**Step 1: Add the dependency**
|
|
|
|
In `pyproject.toml`, add `"pydantic-extra-types[phonenumbers]>=2.0"` to the `dependencies` list (after `pydantic-settings`).
|
|
|
|
```toml
|
|
"pydantic-settings>=2.7",
|
|
"pydantic-extra-types[phonenumbers]>=2.0",
|
|
```
|
|
|
|
**Step 2: Install**
|
|
|
|
Run: `uv sync`
|
|
|
|
**Step 3: Verify import works**
|
|
|
|
Run: `uv run python -c "from pydantic_extra_types.phone_numbers import PhoneNumberValidator; print('OK')"`
|
|
Expected: `OK`
|
|
|
|
**Step 4: Commit**
|
|
|
|
```bash
|
|
git add pyproject.toml uv.lock
|
|
git commit -m "build: add pydantic-extra-types[phonenumbers] dependency"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 2: Create `ProfileUpdate` validation model with tests
|
|
|
|
**Files:**
|
|
- Create: `src/porchlight/validation.py`
|
|
- Create: `tests/test_validation.py`
|
|
|
|
**Step 1: Write the failing tests**
|
|
|
|
Create `tests/test_validation.py`:
|
|
|
|
```python
|
|
import pytest
|
|
from pydantic import ValidationError
|
|
|
|
from porchlight.validation import ProfileUpdate
|
|
|
|
|
|
class TestProfileUpdateEmail:
|
|
def test_valid_email(self) -> None:
|
|
p = ProfileUpdate(email="user@example.com")
|
|
assert p.email == "user@example.com"
|
|
|
|
def test_empty_email_becomes_none(self) -> None:
|
|
p = ProfileUpdate(email="")
|
|
assert p.email is None
|
|
|
|
def test_invalid_email_no_at(self) -> None:
|
|
with pytest.raises(ValidationError, match="email"):
|
|
ProfileUpdate(email="not-an-email")
|
|
|
|
def test_invalid_email_no_domain(self) -> None:
|
|
with pytest.raises(ValidationError, match="email"):
|
|
ProfileUpdate(email="user@")
|
|
|
|
|
|
class TestProfileUpdatePhone:
|
|
def test_valid_e164(self) -> None:
|
|
p = ProfileUpdate(phone_number="+46701234567")
|
|
assert p.phone_number == "+46701234567"
|
|
|
|
def test_valid_e164_with_spaces_normalized(self) -> None:
|
|
p = ProfileUpdate(phone_number="+46 70 123 45 67")
|
|
assert p.phone_number == "+46701234567"
|
|
|
|
def test_empty_phone_becomes_none(self) -> None:
|
|
p = ProfileUpdate(phone_number="")
|
|
assert p.phone_number is None
|
|
|
|
def test_invalid_phone(self) -> None:
|
|
with pytest.raises(ValidationError, match="phone"):
|
|
ProfileUpdate(phone_number="not-a-phone")
|
|
|
|
def test_invalid_phone_no_plus(self) -> None:
|
|
with pytest.raises(ValidationError, match="phone"):
|
|
ProfileUpdate(phone_number="46701234567")
|
|
|
|
|
|
class TestProfileUpdatePicture:
|
|
def test_valid_https_url(self) -> None:
|
|
p = ProfileUpdate(picture="https://example.com/pic.jpg")
|
|
assert p.picture == "https://example.com/pic.jpg"
|
|
|
|
def test_valid_http_url(self) -> None:
|
|
p = ProfileUpdate(picture="http://example.com/pic.jpg")
|
|
assert p.picture == "http://example.com/pic.jpg"
|
|
|
|
def test_empty_picture_becomes_none(self) -> None:
|
|
p = ProfileUpdate(picture="")
|
|
assert p.picture is None
|
|
|
|
def test_invalid_picture_not_url(self) -> None:
|
|
with pytest.raises(ValidationError, match="picture"):
|
|
ProfileUpdate(picture="not-a-url")
|
|
|
|
def test_invalid_picture_ftp_scheme(self) -> None:
|
|
with pytest.raises(ValidationError, match="picture"):
|
|
ProfileUpdate(picture="ftp://example.com/pic.jpg")
|
|
|
|
|
|
class TestProfileUpdateFieldLengths:
|
|
def test_given_name_too_long(self) -> None:
|
|
with pytest.raises(ValidationError, match="given_name"):
|
|
ProfileUpdate(given_name="x" * 256)
|
|
|
|
def test_phone_number_max_length(self) -> None:
|
|
"""E.164 max is 15 digits + plus sign = 16 chars."""
|
|
p = ProfileUpdate(phone_number="+123456789012345")
|
|
assert p.phone_number == "+123456789012345"
|
|
|
|
def test_locale_too_long(self) -> None:
|
|
with pytest.raises(ValidationError, match="locale"):
|
|
ProfileUpdate(locale="x" * 21)
|
|
|
|
|
|
class TestProfileUpdateDefaults:
|
|
def test_all_defaults(self) -> None:
|
|
p = ProfileUpdate()
|
|
assert p.given_name == ""
|
|
assert p.family_name == ""
|
|
assert p.preferred_username == ""
|
|
assert p.email is None
|
|
assert p.phone_number is None
|
|
assert p.picture is None
|
|
assert p.locale == ""
|
|
```
|
|
|
|
**Step 2: Run tests to verify they fail**
|
|
|
|
Run: `uv run python -m pytest tests/test_validation.py -v`
|
|
Expected: FAIL — `ModuleNotFoundError: No module named 'porchlight.validation'`
|
|
|
|
**Step 3: Write the implementation**
|
|
|
|
Create `src/porchlight/validation.py`:
|
|
|
|
```python
|
|
from typing import Annotated
|
|
from urllib.parse import urlparse
|
|
|
|
from pydantic import BaseModel, EmailStr, Field, field_validator
|
|
from pydantic_extra_types.phone_numbers import PhoneNumberValidator
|
|
|
|
E164Phone = Annotated[str, PhoneNumberValidator(number_format="E164")]
|
|
|
|
|
|
class ProfileUpdate(BaseModel):
|
|
given_name: str = Field(default="", max_length=255)
|
|
family_name: str = Field(default="", max_length=255)
|
|
preferred_username: str = Field(default="", max_length=255)
|
|
email: EmailStr | None = None
|
|
phone_number: E164Phone | None = None
|
|
picture: str | None = Field(default=None, max_length=2048)
|
|
locale: str = Field(default="", max_length=20)
|
|
|
|
@field_validator("email", mode="before")
|
|
@classmethod
|
|
def empty_email_to_none(cls, v: str) -> str | None:
|
|
if isinstance(v, str) and v.strip() == "":
|
|
return None
|
|
return v
|
|
|
|
@field_validator("phone_number", mode="before")
|
|
@classmethod
|
|
def empty_phone_to_none(cls, v: str) -> str | None:
|
|
if isinstance(v, str) and v.strip() == "":
|
|
return None
|
|
return v
|
|
|
|
@field_validator("picture", mode="before")
|
|
@classmethod
|
|
def validate_picture_url(cls, v: str) -> str | None:
|
|
if isinstance(v, str) and v.strip() == "":
|
|
return None
|
|
if isinstance(v, str):
|
|
parsed = urlparse(v)
|
|
if parsed.scheme not in ("http", "https") or not parsed.netloc:
|
|
raise ValueError("Picture URL must be a valid HTTP or HTTPS URL")
|
|
return v
|
|
```
|
|
|
|
**Step 4: Run tests to verify they pass**
|
|
|
|
Run: `uv run python -m pytest tests/test_validation.py -v`
|
|
Expected: All PASS
|
|
|
|
**Step 5: Commit**
|
|
|
|
```bash
|
|
git add src/porchlight/validation.py tests/test_validation.py
|
|
git commit -m "feat: add ProfileUpdate pydantic model with email and phone validation"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 3: Wire `ProfileUpdate` into manage routes
|
|
|
|
**Files:**
|
|
- Modify: `src/porchlight/manage/routes.py:199-255`
|
|
- Create: `tests/test_manage_profile.py`
|
|
|
|
**Step 1: Write the failing integration tests**
|
|
|
|
Create `tests/test_manage_profile.py`:
|
|
|
|
```python
|
|
from datetime import UTC, datetime
|
|
|
|
from argon2 import PasswordHasher
|
|
from httpx import AsyncClient
|
|
|
|
from porchlight.authn.password import PasswordService
|
|
from porchlight.models import PasswordCredential, User
|
|
from tests.conftest import get_csrf_token
|
|
|
|
|
|
async def _login(client: AsyncClient, username: str = "alice", userid: str = "user-01") -> None:
|
|
app = client._transport.app # type: ignore[union-attr]
|
|
user_repo = app.state.user_repo
|
|
cred_repo = app.state.credential_repo
|
|
|
|
user = await user_repo.get_by_username(username)
|
|
if user is None:
|
|
user = User(
|
|
userid=userid,
|
|
username=username,
|
|
created_at=datetime.now(UTC),
|
|
updated_at=datetime.now(UTC),
|
|
)
|
|
await user_repo.create(user)
|
|
|
|
svc = PasswordService(hasher=PasswordHasher(time_cost=1, memory_cost=8192))
|
|
existing = await cred_repo.get_password_by_user(user.userid)
|
|
if existing is None:
|
|
await cred_repo.create_password(PasswordCredential(user_id=user.userid, password_hash=svc.hash("password")))
|
|
|
|
token = await get_csrf_token(client)
|
|
await client.post(
|
|
"/login/password",
|
|
data={"username": username, "password": "password"},
|
|
headers={"HX-Request": "true", "X-CSRF-Token": token},
|
|
)
|
|
|
|
|
|
async def test_update_profile_success(client: AsyncClient) -> None:
|
|
await _login(client)
|
|
token = await get_csrf_token(client)
|
|
response = await client.post(
|
|
"/manage/profile",
|
|
data={
|
|
"given_name": "Alice",
|
|
"family_name": "Smith",
|
|
"preferred_username": "ally",
|
|
"email": "alice@example.com",
|
|
"phone_number": "+46701234567",
|
|
"picture": "https://example.com/alice.jpg",
|
|
"locale": "en",
|
|
},
|
|
headers={"X-CSRF-Token": token},
|
|
)
|
|
assert response.status_code == 200
|
|
assert "Profile updated" in response.text
|
|
|
|
app = client._transport.app # type: ignore[union-attr]
|
|
user = await app.state.user_repo.get_by_userid("user-01")
|
|
assert user is not None
|
|
assert user.given_name == "Alice"
|
|
assert user.email == "alice@example.com"
|
|
assert user.phone_number == "+46701234567"
|
|
|
|
|
|
async def test_update_profile_invalid_email(client: AsyncClient) -> None:
|
|
await _login(client)
|
|
token = await get_csrf_token(client)
|
|
response = await client.post(
|
|
"/manage/profile",
|
|
data={"email": "not-an-email"},
|
|
headers={"X-CSRF-Token": token},
|
|
)
|
|
assert response.status_code == 200
|
|
assert "alert" in response.text
|
|
assert "email" in response.text.lower()
|
|
|
|
|
|
async def test_update_profile_invalid_phone(client: AsyncClient) -> None:
|
|
await _login(client)
|
|
token = await get_csrf_token(client)
|
|
response = await client.post(
|
|
"/manage/profile",
|
|
data={"phone_number": "not-a-phone"},
|
|
headers={"X-CSRF-Token": token},
|
|
)
|
|
assert response.status_code == 200
|
|
assert "alert" in response.text
|
|
assert "phone" in response.text.lower()
|
|
|
|
|
|
async def test_update_profile_phone_normalized(client: AsyncClient) -> None:
|
|
await _login(client)
|
|
token = await get_csrf_token(client)
|
|
response = await client.post(
|
|
"/manage/profile",
|
|
data={"phone_number": "+46 70 123 45 67"},
|
|
headers={"X-CSRF-Token": token},
|
|
)
|
|
assert response.status_code == 200
|
|
assert "Profile updated" in response.text
|
|
|
|
app = client._transport.app # type: ignore[union-attr]
|
|
user = await app.state.user_repo.get_by_userid("user-01")
|
|
assert user is not None
|
|
assert user.phone_number == "+46701234567"
|
|
|
|
|
|
async def test_update_profile_invalid_picture(client: AsyncClient) -> None:
|
|
await _login(client)
|
|
token = await get_csrf_token(client)
|
|
response = await client.post(
|
|
"/manage/profile",
|
|
data={"picture": "not-a-url"},
|
|
headers={"X-CSRF-Token": token},
|
|
)
|
|
assert response.status_code == 200
|
|
assert "alert" in response.text
|
|
assert "Picture URL" in response.text
|
|
|
|
|
|
async def test_update_profile_field_too_long(client: AsyncClient) -> None:
|
|
await _login(client)
|
|
token = await get_csrf_token(client)
|
|
response = await client.post(
|
|
"/manage/profile",
|
|
data={"given_name": "x" * 256},
|
|
headers={"X-CSRF-Token": token},
|
|
)
|
|
assert response.status_code == 200
|
|
assert "alert" in response.text
|
|
```
|
|
|
|
**Step 2: Run tests to verify they fail**
|
|
|
|
Run: `uv run python -m pytest tests/test_manage_profile.py -v`
|
|
Expected: Some tests fail (e.g. phone validation won't reject invalid, no normalization)
|
|
|
|
**Step 3: Update the manage route handler**
|
|
|
|
Replace `src/porchlight/manage/routes.py` lines 199-255 with:
|
|
|
|
```python
|
|
@router.post("/profile", response_class=HTMLResponse)
|
|
async def update_profile(
|
|
request: Request,
|
|
given_name: str = Form(""),
|
|
family_name: str = Form(""),
|
|
preferred_username: str = Form(""),
|
|
email: str = Form(""),
|
|
phone_number: str = Form(""),
|
|
picture: str = Form(""),
|
|
locale: str = Form(""),
|
|
) -> Response:
|
|
session_user = get_session_user(request)
|
|
if session_user is None:
|
|
return RedirectResponse("/login", status_code=303)
|
|
|
|
userid, _username = session_user
|
|
|
|
try:
|
|
profile = ProfileUpdate(
|
|
given_name=given_name,
|
|
family_name=family_name,
|
|
preferred_username=preferred_username,
|
|
email=email,
|
|
phone_number=phone_number,
|
|
picture=picture,
|
|
locale=locale,
|
|
)
|
|
except ValidationError as exc:
|
|
error = exc.errors()[0]
|
|
field = error["loc"][-1] if error["loc"] else "input"
|
|
msg = error["msg"]
|
|
# Produce user-friendly field labels
|
|
labels = {
|
|
"given_name": "Given name",
|
|
"family_name": "Family name",
|
|
"preferred_username": "Display name",
|
|
"email": "Email",
|
|
"phone_number": "Phone number",
|
|
"picture": "Picture URL",
|
|
"locale": "Locale",
|
|
}
|
|
label = labels.get(str(field), str(field))
|
|
# Use custom message for picture URL, generic pydantic message otherwise
|
|
if "Picture URL" in msg:
|
|
display_msg = msg
|
|
else:
|
|
display_msg = f"{label}: {msg}"
|
|
return HTMLResponse(f'<div role="alert">{display_msg}</div>')
|
|
|
|
user_repo = request.app.state.user_repo
|
|
user = await user_repo.get_by_userid(userid)
|
|
|
|
updated = user.model_copy(
|
|
update={
|
|
"given_name": profile.given_name or None,
|
|
"family_name": profile.family_name or None,
|
|
"preferred_username": profile.preferred_username or None,
|
|
"email": profile.email,
|
|
"phone_number": profile.phone_number,
|
|
"picture": profile.picture,
|
|
"locale": profile.locale or None,
|
|
}
|
|
)
|
|
await user_repo.update(updated)
|
|
|
|
return HTMLResponse('<div role="status">Profile updated</div>')
|
|
```
|
|
|
|
Add imports at top of file:
|
|
```python
|
|
from pydantic import ValidationError
|
|
|
|
from porchlight.validation import ProfileUpdate
|
|
```
|
|
|
|
Remove the `from urllib.parse import urlparse` import (no longer needed).
|
|
|
|
**Step 4: Run tests to verify they pass**
|
|
|
|
Run: `uv run python -m pytest tests/test_manage_profile.py tests/test_validation.py -v`
|
|
Expected: All PASS
|
|
|
|
**Step 5: Run full test suite**
|
|
|
|
Run: `uv run python -m pytest -v`
|
|
Expected: All PASS
|
|
|
|
**Step 6: Commit**
|
|
|
|
```bash
|
|
git add src/porchlight/manage/routes.py tests/test_manage_profile.py
|
|
git commit -m "feat: wire ProfileUpdate validation into manage profile route"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 4: Wire `ProfileUpdate` into admin routes
|
|
|
|
**Files:**
|
|
- Modify: `src/porchlight/admin/routes.py:126-170`
|
|
- Modify: `tests/test_admin/test_admin_routes.py` (update existing tests)
|
|
|
|
**Step 1: Update the admin route handler**
|
|
|
|
Replace `src/porchlight/admin/routes.py` lines 126-170 with the same `ProfileUpdate` pattern used in manage routes. Add imports:
|
|
|
|
```python
|
|
from pydantic import ValidationError
|
|
|
|
from porchlight.validation import ProfileUpdate
|
|
```
|
|
|
|
Replace the inline validation block (lines 145-151) with:
|
|
|
|
```python
|
|
try:
|
|
profile = ProfileUpdate(
|
|
given_name=given_name,
|
|
family_name=family_name,
|
|
preferred_username=preferred_username,
|
|
email=email,
|
|
phone_number=phone_number,
|
|
picture=picture,
|
|
locale=locale,
|
|
)
|
|
except ValidationError as exc:
|
|
error = exc.errors()[0]
|
|
field = error["loc"][-1] if error["loc"] else "input"
|
|
msg = error["msg"]
|
|
labels = {
|
|
"given_name": "Given name",
|
|
"family_name": "Family name",
|
|
"preferred_username": "Display name",
|
|
"email": "Email",
|
|
"phone_number": "Phone number",
|
|
"picture": "Picture URL",
|
|
"locale": "Locale",
|
|
}
|
|
label = labels.get(str(field), str(field))
|
|
if "Picture URL" in msg:
|
|
display_msg = msg
|
|
else:
|
|
display_msg = f"{label}: {msg}"
|
|
return HTMLResponse(f'<div role="alert">{display_msg}</div>')
|
|
```
|
|
|
|
Use `profile.email`, `profile.phone_number`, `profile.picture` in the `model_copy` call.
|
|
|
|
Remove the `from urllib.parse import urlparse` import.
|
|
|
|
**Step 2: Update admin tests for new error messages**
|
|
|
|
The existing `test_update_profile_invalid_email` checks for `"Invalid email"` which may change. Update assertions to match new pydantic-driven messages (check for `"alert"` and `"email"` in text, similar to manage tests).
|
|
|
|
The existing `test_update_profile` sends `phone_number="+1234567890"` which should still be valid E.164.
|
|
|
|
**Step 3: Run admin tests**
|
|
|
|
Run: `uv run python -m pytest tests/test_admin/ -v`
|
|
Expected: All PASS
|
|
|
|
**Step 4: Run full test suite**
|
|
|
|
Run: `uv run python -m pytest -v`
|
|
Expected: All PASS
|
|
|
|
**Step 5: Commit**
|
|
|
|
```bash
|
|
git add src/porchlight/admin/routes.py tests/test_admin/test_admin_routes.py
|
|
git commit -m "refactor: use shared ProfileUpdate validation in admin routes"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 5: Update templates with phone input hints
|
|
|
|
**Files:**
|
|
- Modify: `src/porchlight/templates/manage/profile.html:35-36`
|
|
- Modify: `src/porchlight/templates/admin/user_detail.html:31-32`
|
|
|
|
**Step 1: Update manage template phone input**
|
|
|
|
Change the phone input to add `pattern` and `title` attributes:
|
|
|
|
```html
|
|
<input type="tel" id="phone_number" name="phone_number" value="{{ user.phone_number or '' }}" maxlength="50" autocomplete="tel" pattern="\+[0-9 ]+" title="International format: +46701234567">
|
|
```
|
|
|
|
**Step 2: Update admin template phone input**
|
|
|
|
Same change for the admin template.
|
|
|
|
**Step 3: Commit**
|
|
|
|
```bash
|
|
git add src/porchlight/templates/manage/profile.html src/porchlight/templates/admin/user_detail.html
|
|
git commit -m "style: add E.164 format hint to phone number inputs"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 6: Lint, type-check, final verification
|
|
|
|
**Step 1: Format and lint**
|
|
|
|
Run: `uv run ruff format src/ tests/ && uv run ruff check src/ tests/ --fix`
|
|
|
|
**Step 2: Run full test suite**
|
|
|
|
Run: `uv run python -m pytest -v`
|
|
Expected: All PASS
|
|
|
|
**Step 3: Fix any issues and commit if needed**
|