pal-e-docs

Review: ServiceRequest model + migration

review-1347-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
  • [ ] Feature Flag -- MISSING. Template requires a Feature Flag section. This ticket is an internal model change with no UI, so the correct value is "none", but the section must be present.
  • [x] Acceptance Criteria
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related

Traceability

  • [x] story:auth label -- auth story
  • [x] story note verified -- "auth" entry found in project-landscaping-assistant user-stories table
  • [x] arch:rails-app label
  • [ ] arch note MISSING -- [SCOPE] No arch-rails-app note exists in pal-e-docs. Create architecture note arch-rails-app for the Rails application component.
  • [x] Forgejo issue -- ldraney/landscaping-assistant#122, state: open

File Targets

  • [x] db/migrate/*_create_service_requests.rb (new) -- OK, no existing ServiceRequest migration found
  • [x] db/migrate/*_add_monthly_price_to_services.rb (new) -- OK, services table exists (schema verified), no monthly_price column present. ERD in docs/user-stories-auth.md confirms this addition.
  • [x] app/models/service_request.rb (new) -- OK, no existing model. No references to ServiceRequest anywhere in codebase.
  • [x] app/models/property.rb -- verified exists. Currently has has_many :property_services, has_many :services, has_many :work_queue_items. Adding has_many :service_requests is straightforward.
  • [x] db/seeds.rb -- verified exists. Currently seeds three services without prices. Adding monthly_price values is straightforward.
  • [x] spec/models/service_request_spec.rb (new) -- OK, spec/models/ directory exists with 8 existing model specs. Pattern is consistent.

Repo Placement

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

Dependencies

  • #121 (parent spike) -- done (board item 1346 in done column). No blocker.
  • user_id FK problem -- The ticket specifies user_id as a column on ServiceRequest, and the ERD in docs/user-stories-auth.md shows bigint user_id FK "Client who submitted". However, there is no users table in the database. The app uses Keycloak for auth and stores identity as keycloak_username on crew_members. The User model is described in the ERD as a Phase 4 concept (multi-tenancy). The ticket needs to clarify: should user_id be a string column storing a Keycloak sub/username (matching the current crew_members.keycloak_username pattern), or is a User model a prerequisite? This is a blocking ambiguity.
  • No Stripe gem or config -- The stripe_payment_link column is fine as a plain string (just a URL), but there are zero Stripe references in the codebase. The ticket correctly says "No UI" and the Stripe integration is a separate ticket (#125). No dependency issue here.
  • Downstream consumers -- #123 (Client request UI) and #124 (Admin request management) both depend on this model. Both are in backlog, not blocked.

Acceptance Criteria

  • AC1 (ServiceRequest model with listed columns) -- testable, columns clearly specified.
  • AC2 (Service gains monthly_price) -- testable, clear.
  • AC3 (Seeds updated with prices) -- testable, three service names match existing seeds.
  • AC4 (Status transitions validated, no skipping steps) -- Underspecified. The valid transitions are not enumerated. For example: can a request go directly from requested to declined? Can quoted go to declined? The status values are listed (requested, quoted, paid, scheduled, completed, declined), but the allowed transition graph is not. The flow diagram in docs/user-stories-auth.md implies declined branches from the admin decision point (after requested), but also after quoted if the client doesn't pay? This needs explicit transition rules for an agent to implement correctly.
  • AC5 (Model specs) -- testable, clear.

Blast Radius

  • Property model -- adding has_many :service_requests is low risk. No existing associations conflict.
  • Service model -- adding monthly_price column is low risk. No existing code reads a price from services.
  • Seeds -- uses find_or_create_by!, so updating to include prices needs care (may need to use find_or_create_by!(name:).update!(monthly_price:) or similar to be idempotent).
  • No downstream breakage risk -- this is a new model with no UI. Existing tests should be unaffected.

Decomposition Assessment

6 file targets in 1 repo, 5 acceptance criteria. All files are in one repo and the work is cohesive (one model + one column addition + seeds + specs). Estimated agent work: under 5 minutes. No decomposition needed.

Recommendations

  • [BODY] Add ### Feature Flag section with value "none" (internal model change, no UI).
  • [BODY] Clarify user_id column: since there is no users table, specify whether this should be a string column storing the Keycloak sub/username (matching crew_members.keycloak_username pattern) or a bigint FK to crew_members. The ERD shows a User entity that does not yet exist -- the ticket must resolve this ambiguity before implementation.
  • [BODY] Add explicit status transition rules to AC4. Enumerate which transitions are valid (e.g., requested -> quoted, requested -> declined, quoted -> declined, quoted -> paid, paid -> scheduled, scheduled -> completed). Without this, an agent cannot implement "no skipping steps" correctly.
  • [SCOPE] Create architecture note arch-rails-app for the Rails application component in pal-e-docs.