pal-e-docs

Review: Phase 2: Contact form (business name, name, logo, email) + calendar redirect

review-1179-2026-05-09 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- "Decomposed from #1 -- Phase 2 of 4. Depends on #2 (scaffold)."
  • [x] Repo -- present but wrong (see Repo Placement below)
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Acceptance Criteria
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related
All required sections are present per template-issue-feature.

Traceability

  • [x] story:landing-page label -- "Visitor: Can understand services offered and submit contact form"
  • [x] story note verified -- found in project-pal-enterprises user-stories section
  • [ ] arch:rails-app label -- present on board item
  • [ ] arch note MISSING -- [SCOPE] Create architecture note arch-rails-app for the Rails application component
  • [x] Forgejo issue -- ldraney/pal-enterprises#3, state: open

File Targets

Major discrepancy: the issue lists files "to create" but several already exist from a prior implementation pass. The existing implementation uses a DB-backed Lead model, which contradicts the issue's "no database model" constraint.
  • [ ] app/controllers/contacts_controller.rb -- ISSUE: Listed as "to create" but already exists. Uses Lead model (DB-backed), contradicts spec's "no database model" constraint.
  • [ ] app/views/contacts/new.html.erb -- ISSUE: Listed as "to create" but already exists.
  • [ ] app/views/contacts/thank_you.html.erb -- Does not exist. Controller redirects to root_path instead of rendering a thank_you page. Spec and implementation disagree.
  • [ ] app/mailers/contact_mailer.rb -- Does not exist. Only application_mailer.rb present. This is genuinely needed.
  • [ ] app/views/contact_mailer/new_inquiry.html.erb -- Does not exist. Genuinely needed.
  • [x] config/routes.rb -- Verified: already has resources :contacts, only: [:new, :create]
  • [x] app/views/pages/home.html.erb -- Verified: already renders the contact form partial inline.
  • [x] app/views/layouts/application.html.erb -- Verified: already has "Contact" nav link at line 23.
Additional file not in spec: app/views/contacts/_form.html.erb (partial) and app/models/lead.rb (DB model with validations) already exist.

Repo Placement

MISMATCH. The issue body says forgejo_admin/pal-enterprises but the actual repo is ldraney/pal-enterprises. The forgejo_admin/pal-enterprises path returns no data from the API. The board item's Forgejo URL also uses the forgejo_admin path. This needs correction.

Dependencies

  • Phase 1 (#1178, "Rails scaffold, landing page, health check") is in done -- dependency satisfied.
  • Phase 3 (#1180, "Keycloak OIDC authentication") is in done -- no blocker.
  • Board item #1188 ("Remove Tailwind, implement plain CSS design system") is in backlog. The current issue spec references Tailwind styling ("contact form with Tailwind styling"). If #1188 lands first, Tailwind references become invalid. If this ticket lands first, the Tailwind work will need to update these views. Order dependency is undocumented.
  • No items currently block this ticket.

Acceptance Criteria

Mixed testability:
  • "Contact form at /contact renders with all fields" -- testable, but spec says name/email/message_type/message_body while codebase has name/email/message (no message_type select). Criteria ambiguous about which fields.
  • "Form validates presence of name, email, and message" -- testable. Already implemented in Lead model.
  • "Successful submission delivers email to admin" -- testable, but mailer does not exist yet. This is the real remaining work.
  • "Turbo-powered submit shows confirmation without full page reload" -- testable but NOT implemented. No turbo_frame or turbo_stream usage in the contact form.
  • "Invalid submission re-renders form with errors" -- testable. Already implemented.
  • "Landing page links to contact form" -- testable. Already implemented (form rendered inline on home page, plus nav link).
Missing AC: The board item title says "business name, name, logo, email + calendar redirect" but none of these extra fields (business_name, logo upload, calendar redirect) appear in the Forgejo issue or the codebase. The board title and spec are misaligned.

Blast Radius

Low. The contact form is self-contained. Adding Action Mailer touches SMTP configuration which could affect any future mailers but has no current downstream consumers. The Tailwind dependency is the main cross-cutting concern (see Dependencies).

Decomposition Assessment

6 acceptance criteria (borderline), but all within a single repo and a small number of files. The real remaining work is: (1) add mailer, (2) add message_type select, (3) add Turbo submit, (4) decide DB vs stateless. This fits in a single agent pass if the scope contradictions are resolved first. No decomposition needed.

Recommendations

  • [BODY] Fix Repo field: forgejo_admin/pal-enterprises should be ldraney/pal-enterprises.
  • [BODY] Update File Targets to reflect current codebase state: mark existing files as "to modify" not "to create." Add app/models/lead.rb and app/views/contacts/_form.html.erb to the target list.
  • [BODY] Resolve DB vs stateless contradiction: the Constraints section says "No database model -- contact form is stateless" but the codebase already has a Lead model backed by a leads table. Either update the constraint to accept the DB model, or rewrite the controller to use a plain PORO + mailer.
  • [BODY] Resolve board title vs issue scope mismatch: board says "business name, name, logo, email + calendar redirect" but issue spec has "name, email, message type, message body" with no logo upload or calendar redirect. Either update the board title or add the missing fields to the issue.
  • [BODY] Add message_type select dropdown to file targets and form spec (currently missing from implementation).
  • [BODY] Clarify Tailwind dependency: note that views use Tailwind classes, and document ordering relative to board item #1188 (Tailwind removal).
  • [SCOPE] Create architecture note arch-rails-app for the Rails application component.