pal-e-docs

Review: Client request UI: My Property view with package and request form

review-1348-2026-06-07 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type
  • [x] Lineage
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Feature Flag (none -- appropriate for this ticket)
  • [x] Acceptance Criteria
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related

Traceability

  • [x] story:auth label -- "Log in with Keycloak, see role-appropriate tabs and features"
  • [x] story note verified -- found in project-landscaping-assistant user-stories section (key: auth)
  • [x] arch:rails-app label -- Rails application component
  • [ ] arch note MISSING -- [SCOPE] No arch-rails-app note exists in pal-e-docs. Search returned zero results. Create architecture note arch-rails-app for the Rails app component.
  • [x] Forgejo issue -- #123, open

File Targets

  • [x] app/views/person/index.html.erb -- verified: exists, has placeholder "My Property" section at lines 46-53 ready for expansion
  • [x] app/controllers/service_requests_controller.rb -- verified: does not exist yet, marked as new. Correct.
  • [x] app/views/service_requests/_client_form.html.erb -- verified: does not exist yet, marked as new. Correct.
  • [x] app/views/service_requests/_client_request_list.html.erb -- verified: does not exist yet, marked as new. Correct.
  • [x] app/assets/stylesheets/application.css -- verified: exists (1630 lines), has Person component section and design tokens.
  • [x] config/routes.rb -- verified: exists, currently has resources :person, only: [:index] at line 40. Adding service_requests resource is correct.
  • [x] spec/requests/service_requests_spec.rb -- verified: does not exist yet, marked as new. Correct.

Repo Placement

OK. Issue filed on ldraney/landscaping-assistant, all file targets are in the same repo. Single-repo change.

Dependencies

  • [x] #122 (ServiceRequest model + migration) -- MERGED (commit 5391056), Forgejo issue closed. Dependency satisfied.
  • [x] #176 (Admin property projects display) -- sibling ticket in backlog, no blocking relationship. Correct.
  • [x] #179 (Admin project workflow) -- sibling ticket in backlog, no blocking relationship. Correct.
  • [ ] docs/service-requests.md -- MISSING. The issue body references this doc in Context ("See docs/service-requests.md for the full architecture, status lifecycle, and data model") and Constraints ("Reference docs/service-requests.md for status values, transitions, and role access matrix"). This file does not exist in the repo at HEAD. The implementing agent will not be able to follow these references. This is a blocking scope gap.

Acceptance Criteria

  • [ ] Status name mismatch: AC #2 lists statuses "requested, quoted, accepted, paid, scheduled" but the merged ServiceRequest model defines statuses as requested, quoted, paid, scheduled, completed, declined. There is no "accepted" status. The "Accept" button in AC #9 actually triggers quoted -> paid transition per the model. The AC text will confuse the implementing agent.
  • [ ] crew_member_id resolution unclear: AC #6 says "creates ServiceRequest with crew_member_id = current user" but current_user is a Keycloak session hash ({username:, email:, roles:}), not a CrewMember record. The CrewMember model has a keycloak_username field, so lookup is possible, but the ticket doesn't specify how to resolve this. Furthermore, clients may not have a CrewMember record at all (CrewMember.ROLES includes "client" but there's no guarantee every logged-in client has been provisioned as a CrewMember).
  • [x] AC #8 (owner_sub check) -- Property model has owner_sub column and owned_by scope. Implementable, though the ticket doesn't specify how to get the current user's sub from the session (it's not stored in current_user hash currently -- only username, email, roles).
  • [x] AC #10 (mobile touch targets) -- testable visually.
  • [x] Remaining ACs are clear and testable.

Blast Radius

Low. This is a new controller and new views. Touches existing person/index.html.erb (additive, expands placeholder section), routes.rb (additive), and application.css (additive). No destructive changes to existing functionality. The service_requests resource is new and won't conflict with existing routes.

Decomposition Assessment

7 file targets in 1 repo, 10 acceptance criteria. Borderline on the 5-minute rule. However, all targets are in one repo, the model layer is already merged, and the work is a focused controller+views build. A single agent pass is feasible if scope issues are resolved. No decomposition needed.

Recommendations

  • [SCOPE] Create docs/service-requests.md with the architecture, status lifecycle, and role access matrix referenced by this ticket. Without it, the implementing agent has no reference for status transitions and role access. This is the most critical gap.
  • [BODY] Fix AC #2 status list: replace "requested, quoted, accepted, paid, scheduled" with "requested, quoted, paid, scheduled, completed, declined" to match the merged ServiceRequest model.
  • [BODY] Fix AC #6: clarify how crew_member_id is resolved from the current Keycloak session. Specify whether clients are expected to have a CrewMember record, or whether the FK should be something else (e.g., store the Keycloak sub directly instead).
  • [BODY] Fix AC #9: clarify that "Accept" maps to status transition quoted -> paid and "Decline" maps to quoted -> declined. The word "accepted" does not correspond to any model status.
  • [BODY] Clarify AC #8: specify how to obtain the current user's sub for the owner_sub check. The current session hash stores username/email/roles but not the Keycloak sub.
  • [SCOPE] Create architecture note arch-rails-app for the Rails application component in pal-e-docs.