pal-e-docs

Review: Remove password rendering (basketball-api#168)

review-353-2026-03-27 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Discovered during QA review of PR #167
  • [x] Repo — forgejo_admin/basketball-api
  • [x] User Story — present
  • [x] Context — present
  • [x] File Targets — 3 files listed
  • [x] Acceptance Criteria — 4 items
  • [x] Test Expectations — present with run command
  • [x] Constraints — present
  • [x] Checklist — present
  • [x] Related — present

Traceability

  • [x] story:WS-S21 label — present on board item
  • [x] arch:auth-flow label — present on board item
  • [x] Forgejo issue — basketball-api#168, open

File Targets

  • [x] src/basketball_api/routes/register.py — verified: line 1357 returns result["password"] = keycloak_credentials["password"]; lines 314-335 render password in confirmation HTML
  • [x] src/basketball_api/services/email.py — verified: lines 88-93 render password in plaintext email; lines 172-188 render password in HTML credential block
  • [x] src/basketball_api/services/keycloak.py — verified: line 330 returns {"email": parent_email, "password": ""} (already empty string for backward compat)

Repo Placement

Issue filed on basketball-api — correct for the API changes. However, westside-app reads result.password at src/routes/(app)/register/+page.svelte:156. The frontend stores result.password into confirmPassword. Removing the password key from the API response requires either a coordinated westside-app change, or acceptance that the optional chaining (result?.password) handles the missing field gracefully (it won't crash, but confirmation UI may show stale/empty credentials section).

Dependencies

  • Depends on understanding what westside-app does with confirmPassword after line 157 — if it renders it to the user, removing the key silently makes the confirmation page show nothing where credentials used to be. The frontend needs updated messaging too.
  • No board item blockers found in in_progress.

Acceptance Criteria

  • Criteria are testable but incomplete. Criterion 4 ("westside-app frontend doesn't break") needs more specificity — the frontend DOES read the password field. The agent needs to know whether to also create a westside-app PR or just verify the optional chaining handles it.
  • Missing: acceptance criterion for the email.py HTML template credential block (lines 172-188) which is separate from the plaintext section.

Blast Radius

  • westside-app register page — reads result.password (line 156). Won't crash (optional chaining) but confirmation UI will be incomplete without a frontend update.
  • email.py has TWO password rendering paths: plaintext (lines 88-93) and HTML (lines 172-188). Both need updating.
  • register.py has credential rendering in the HTML confirmation page (lines 314-335) AND in the JSON response (line 1357).

Recommendation

Two issues before READY:
  • Cross-repo scope: The ticket must explicitly state whether a westside-app PR is also needed. If yes, this should be split into two Forgejo issues (one per repo). If the optional chaining is sufficient, the ticket should state "westside-app change deferred — optional chaining handles missing key."
  • Missing file target: The email.py HTML credential block (_render_credentials_section, lines 172-188) is a separate function from the plaintext section (lines 88-93). The ticket should list both rendering paths explicitly so the agent doesn't miss one.