pal-e-docs

Review: Multi-tenancy: Business model, property scoping, and test tenant

review-1662-2026-07-04 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Feature Flag
  • [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
  • [ ] story:auth is a loose fit -- multi-tenancy is about data isolation and business scoping, not authentication. [SCOPE] Consider creating a dedicated "multi-tenancy" user story entry on project-landscaping-assistant.
  • [x] arch:rails-app label
  • [ ] arch note MISSING -- [SCOPE] Create architecture note arch-rails-app for the Rails app component. No matching note found via search.
  • [x] Forgejo issue -- ldraney/landscaping-assistant#282, open

File Targets

  • [x] app/models/property.rb -- verified exists, has no current business association
  • [x] app/controllers/application_controller.rb -- verified exists, contains feature_enabled? and find_property_or_reject (unscoped Property.find_by)
  • [x] app/controllers/properties_controller.rb -- verified exists, has unscoped Property.by_proximity, Property.find, Property.create!
  • [x] app/controllers/work_queue_items_controller.rb -- verified exists, has unscoped Property.active, Property.create!, Property.nearest_to
  • [x] app/controllers/weeks_controller.rb -- verified exists, has unscoped Property.by_proximity(Property.active)
  • [x] lib/tasks/feature_flags.rake -- verified exists, no multi_tenancy flag yet
  • [x] config/initializers/omniauth.rb -- verified exists (do-not-touch, correct)
  • [x] docs/app-architecture.md -- verified exists, already describes Business and UserBusiness under "Planned" section (do-not-touch, correct)
  • [ ] app/controllers/days_controller.rb -- MISSING from file targets. Has 3 unscoped Property queries: Property.active.order(:client_name), .joins(:property).merge(Property.active), Property.active.where(id:). [BODY] Add to file targets.
  • [ ] app/controllers/property_comments_controller.rb -- MISSING from file targets. Has unscoped Property.find(params[:property_id]). [BODY] Add to file targets.
  • [ ] New files (migrations, models, seed task) -- cannot verify, will be created. Paths look correct for Rails conventions.

Repo Placement

OK. Issue filed on ldraney/landscaping-assistant, all file targets are in that repo. PR 2 (Keycloak role migration) is deferred to a separate ticket, which is appropriate since it may touch Keycloak config in pal-e-services.

Dependencies

  • #118 (Onboarding flow, solo-to-business transition) -- backlog, story:auth, phase:4. This ticket is a prerequisite for #118. Not documented in the issue's Related section as a dependency direction (only says "Related to #118").
  • #262 (Crew/team grouping) -- backlog, story:auth. Related but not a hard dependency. Crew grouping may need to integrate with the Business model. Documented in issue.
  • No items in in_progress or next_up that block or are blocked by this ticket.
  • PR 2 (UserBusiness roles) is an explicit follow-up dependency, correctly deferred to a separate ticket.

Acceptance Criteria

PR 1 has 12 acceptance criteria (7 model/scoping + 5 seed data). PR 2 has 3 AC but is deferred to a separate ticket. Individual AC items are testable and specific. The test expectations section lists 6 concrete specs with a run command. However:
  • Missing test for days_controller scoping (not in file targets, not in test expectations)
  • Missing test for property_comments_controller scoping
  • The AC "Property queries scoped to current_business when multi_tenancy flag is ON" is broad -- which controllers specifically?

Blast Radius

  • days_controller.rb -- 3 unscoped Property queries, not listed in file targets. Day detail pages would show cross-business properties without scoping.
  • property_comments_controller.rb -- unscoped Property.find, not listed. Could allow commenting on another business's property.
  • profile_controller.rb -- uses owner_sub scoping, not business scoping. Likely acceptable for PR 1 since owner_sub uniquely identifies the user, but account deletion (Property.where(owner_sub:).destroy_all) might need business-awareness in PR 2.
  • find_property_or_reject in application_controller.rb -- used by weeks_controller toggle_assign. Unscoped Property.find_by(id:). Listed in targets, but the description only says "current_business helper" -- should also note scoping this method.

Decomposition Assessment

NEEDS DECOMPOSITION -- route to skill-decompose-ticket.
  • 12 file targets in 1 repo -- below the cross-repo threshold but high file count
  • 12 AC items for PR 1 -- exceeds the 5 AC threshold
  • Estimated agent work: 15-20 minutes (3 migrations, 2 new models, 1 new rake task, scoping across 5+ controllers, feature flag, 6+ test files)
Suggested decomposition:
  • Sub-ticket A: Business model + migrations + feature flag -- Create Business, UserBusiness models, add business_id FK to properties, data migration, register multi_tenancy feature flag. ~5 AC.
  • Sub-ticket B: Property scoping + controller updates -- current_business helper, scope all Property queries in properties/work_queue_items/weeks/days/property_comments controllers behind flag. ~4 AC.
  • Sub-ticket C: Test business seed data -- Seed rake task for Greenview Landscaping with realistic properties and work_queue_items. ~5 AC.

Recommendation

  • [BODY] Add missing file target: app/controllers/days_controller.rb (3 unscoped Property queries)
  • [BODY] Add missing file target: app/controllers/property_comments_controller.rb (unscoped Property.find)
  • [BODY] Clarify which controllers are covered by "Property queries scoped to current_business" AC
  • [SCOPE] Create architecture note arch-rails-app for the Rails app component
  • [SCOPE] Consider creating a dedicated "multi-tenancy" user story entry on project-landscaping-assistant (story:auth is a loose fit for data isolation work)
  • [DECOMPOSE] 12 AC across model/scoping/seed, estimated 15-20 min agent work. Split into 3 sub-tickets: (A) models+migrations+flag, (B) controller scoping, (C) seed data. Route to skill-decompose-ticket.