Auth prereqs: session sub, accepted status, CrewMember auto-create #196

Merged

ldraney merged 1 commit from auth-session-prereqs into main 2026-06-11 03:12:29 +00:00

Conversation 1 Commits 1 Files changed 4 +57 -5

ldraney commented 2026-06-10 01:52:03 +00:00

Owner

Copy link

Summary

• Adds Keycloak sub (UUID) to session hash for owner_sub matching
• Adds accepted status to ServiceRequest (quoted → accepted → paid/scheduled)
• Auto-creates CrewMember record on login so all users have FK-ready records
• Adds current_crew_member helper to ApplicationController

Changes

• app/controllers/sessions_controller.rb: sub in session, CrewMember find_or_create on login
• app/controllers/application_controller.rb: current_crew_member helper method
• app/models/service_request.rb: accepted status, updated transitions (quoted → accepted, not quoted → paid)
• spec/models/service_request_spec.rb: new transition specs for accepted state

Test Plan

• 298 specs pass locally
• quoted → paid is now invalid (must go through accepted)
• quoted → accepted → paid works
• accepted → scheduled works (work before payment)
• CrewMember created on first login

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive
• Feature flag needed? No — internal plumbing

Related Notes

• Closes #195
• Prereq for #123, #176, #179, #180

## Summary - Adds Keycloak `sub` (UUID) to session hash for owner_sub matching - Adds `accepted` status to ServiceRequest (quoted → accepted → paid/scheduled) - Auto-creates CrewMember record on login so all users have FK-ready records - Adds `current_crew_member` helper to ApplicationController ## Changes - `app/controllers/sessions_controller.rb`: sub in session, CrewMember find_or_create on login - `app/controllers/application_controller.rb`: current_crew_member helper method - `app/models/service_request.rb`: accepted status, updated transitions (quoted → accepted, not quoted → paid) - `spec/models/service_request_spec.rb`: new transition specs for accepted state ## Test Plan - [ ] 298 specs pass locally - [ ] quoted → paid is now invalid (must go through accepted) - [ ] quoted → accepted → paid works - [ ] accepted → scheduled works (work before payment) - [ ] CrewMember created on first login ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No — internal plumbing ## Related Notes - Closes #195 - Prereq for #123, #176, #179, #180

ldraney added 1 commit 2026-06-10 01:52:03 +00:00

Auth session prereqs: accepted status, crew member auto-create, current_crew_member helper ... b1dda1fa6f

- Add "accepted" status to ServiceRequest between quoted and paid, since
  work often begins after bid acceptance before payment is received.
  Update transition rules: quoted->accepted->paid is the new path.
- Find-or-create CrewMember on login so every authenticated user has a
  record for FK references on ServiceRequest and PropertyComment.
- Add current_crew_member helper to ApplicationController for convenient
  access to the logged-in user's CrewMember record.
- sub already present in session hash (done in prior PR).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-10 01:54:31 +00:00

Author

Owner

Copy link

PR #196 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, RSpec, OmniAuth/Keycloak, PostgreSQL, PaperTrail.

1. Session hash change -- sub addition

The sub: auth.uid addition to session[:user] in sessions_controller.rb:18 is critical and correct. Two existing consumers already read current_user[:sub]:

• PropertiesController#create (line 21) -- sets owner_sub on new properties
• PropertiesController#authorize_property_access (line 115) -- gates show/edit access

Without this PR, those paths would silently get nil for sub, breaking owner-based property matching. The OmniAuth test mock already provides uid: "test-uid-123", and profile_spec.rb tests the full chain (auth.uid -> session[:user][:sub] -> current_user[:sub] -> property.owner_sub). No existing code is broken by the addition -- session[:user] is a hash; adding a new key is non-destructive.

2. ServiceRequest transitions -- accepted state

The transition map is correct and complete:

• requested -> quoted -> accepted -> paid -> scheduled -> completed (happy path)
• accepted -> scheduled (work before payment -- intentional business rule)
• quoted -> declined and requested -> declined preserved
• quoted -> paid correctly removed (must go through accepted)
• accepted -> declined correctly excluded (once accepted, can't decline)
• Terminal states (completed, declined) remain locked

No DB migration needed -- status is a VARCHAR column and accepted is just a new string value in the application-level allowlist. The STATUSES constant and VALID_TRANSITIONS hash are the single source of truth.

3. CrewMember auto-create via find_or_create_by!

find_or_create_by! in sessions_controller.rb:24 is safe here:

• Race conditions: The keycloak_username column has a unique index (index_crew_members_on_keycloak_username, db/schema.rb:52). If two concurrent requests for the same user hit this, one will raise ActiveRecord::RecordNotUnique which Rails propagates as a 500. This is acceptable for a login path -- concurrent logins for the same user are an edge case, and the bang method ensures data integrity. No silent corruption.
• Role mapping: The ternary chain (admin > lead > member > client) maps the first matching Keycloak realm role. Order is correct -- most privileged wins. Falls back to client for users with no matching role.
• Block semantics: The block only runs on create, not on find. Existing CrewMember records keep their current role on subsequent logins. This is correct -- role updates should be explicit, not overwritten on every login.

4. current_crew_member helper

• Nil-safe: returns nil when not logged in (guard clause on line 47).
• Memoized with @current_crew_member ||= -- correct per-request caching. Each request gets a fresh controller instance, so no stale data across requests.
• Looks up by keycloak_username, which matches the find_or_create_by! key in the sessions controller. Consistent.
• Registered as a helper_method so views can use it.

5. Test coverage

ServiceRequest transition specs are thorough:

• from accepted block covers all 6 possible transitions (2 valid, 4 rejected) -- good exhaustive coverage
• from quoted block updated to test accepted instead of paid, and adds explicit rejection of quoted -> paid
• The #allowed_transitions test dynamically iterates all statuses, so it automatically covers accepted
• Terminal state tests iterate all STATUSES including accepted

BLOCKERS

None.

NITS

1. Role mapping ternary chain (sessions_controller.rb:26): The nested ternary is functional but hard to scan. Consider extracting to a method or using an array-find pattern for readability:

   cm.role = (%w[admin lead member] & roles).first || "client"
   

   This is semantically equivalent and reads in one pass. Non-blocking.

2. No spec for CrewMember auto-create on login: The sessions request spec (spec/requests/sessions_spec.rb) tests session population but does not assert that a CrewMember record is created during the OmniAuth callback. A test like expect { post "/auth/keycloak"; follow_redirect! }.to change(CrewMember, :count).by(1) would close this gap. The model validations are well-tested, but the integration path through the controller is not. Non-blocking for this prereq PR since the feature is exercised transitively (profile specs call sign_in_as which triggers the callback), but worth adding.

3. No spec for current_crew_member helper: No request spec asserts that current_crew_member returns the correct record. Again non-blocking for a prereq PR -- the downstream service request tickets will exercise this -- but noting the gap.

4. Comment on accepted -> declined exclusion: The business decision to disallow accepted -> declined is intentional but undocumented. A brief code comment on the transition map explaining why (e.g., "once accepted, cancellation goes through a different workflow") would help future readers. Non-blocking.

SOP COMPLIANCE

• PR body has ## Summary, ## Changes, ## Test Plan, ## Related
• No secrets committed
• No unnecessary file changes (4 files, all on-topic)
• Commit messages are descriptive
• No .env files or credentials in diff
• Scope is tight -- four related auth prereqs, no feature creep

PROCESS OBSERVATIONS

• Change failure risk: Low. The sub addition to the session hash is additive (no existing keys changed). The accepted status is application-level only (no migration). The find_or_create_by! is idempotent on subsequent logins.
• Deployment notes: No migration needed. The accepted status will be available immediately on deploy. Existing service_requests in quoted status will now transition to accepted instead of paid -- any UI or API code that attempts quoted -> paid will get a validation error. This is the intended behavior per the PR description.
• Documentation: The four downstream tickets (#123, #176, #179, #180) are referenced. Clear dependency chain.

VERDICT: APPROVED

## PR #196 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8.1, RSpec, OmniAuth/Keycloak, PostgreSQL, PaperTrail. **1. Session hash change -- `sub` addition** The `sub: auth.uid` addition to `session[:user]` in `sessions_controller.rb:18` is critical and correct. Two existing consumers already read `current_user[:sub]`: - `PropertiesController#create` (line 21) -- sets `owner_sub` on new properties - `PropertiesController#authorize_property_access` (line 115) -- gates show/edit access Without this PR, those paths would silently get `nil` for `sub`, breaking owner-based property matching. The OmniAuth test mock already provides `uid: "test-uid-123"`, and `profile_spec.rb` tests the full chain (`auth.uid` -> `session[:user][:sub]` -> `current_user[:sub]` -> `property.owner_sub`). No existing code is broken by the addition -- `session[:user]` is a hash; adding a new key is non-destructive. **2. ServiceRequest transitions -- `accepted` state** The transition map is correct and complete: - `requested -> quoted -> accepted -> paid -> scheduled -> completed` (happy path) - `accepted -> scheduled` (work before payment -- intentional business rule) - `quoted -> declined` and `requested -> declined` preserved - `quoted -> paid` correctly removed (must go through `accepted`) - `accepted -> declined` correctly excluded (once accepted, can't decline) - Terminal states (`completed`, `declined`) remain locked No DB migration needed -- `status` is a `VARCHAR` column and `accepted` is just a new string value in the application-level allowlist. The `STATUSES` constant and `VALID_TRANSITIONS` hash are the single source of truth. **3. CrewMember auto-create via `find_or_create_by!`** `find_or_create_by!` in `sessions_controller.rb:24` is safe here: - **Race conditions:** The `keycloak_username` column has a unique index (`index_crew_members_on_keycloak_username`, `db/schema.rb:52`). If two concurrent requests for the same user hit this, one will raise `ActiveRecord::RecordNotUnique` which Rails propagates as a 500. This is acceptable for a login path -- concurrent logins for the same user are an edge case, and the bang method ensures data integrity. No silent corruption. - **Role mapping:** The ternary chain (`admin > lead > member > client`) maps the first matching Keycloak realm role. Order is correct -- most privileged wins. Falls back to `client` for users with no matching role. - **Block semantics:** The block only runs on create, not on find. Existing CrewMember records keep their current role on subsequent logins. This is correct -- role updates should be explicit, not overwritten on every login. **4. `current_crew_member` helper** - Nil-safe: returns `nil` when not logged in (guard clause on line 47). - Memoized with `@current_crew_member ||=` -- correct per-request caching. Each request gets a fresh controller instance, so no stale data across requests. - Looks up by `keycloak_username`, which matches the `find_or_create_by!` key in the sessions controller. Consistent. - Registered as a `helper_method` so views can use it. **5. Test coverage** ServiceRequest transition specs are thorough: - `from accepted` block covers all 6 possible transitions (2 valid, 4 rejected) -- good exhaustive coverage - `from quoted` block updated to test `accepted` instead of `paid`, and adds explicit rejection of `quoted -> paid` - The `#allowed_transitions` test dynamically iterates all statuses, so it automatically covers `accepted` - Terminal state tests iterate all `STATUSES` including `accepted` ### BLOCKERS None. ### NITS 1. **Role mapping ternary chain** (`sessions_controller.rb:26`): The nested ternary is functional but hard to scan. Consider extracting to a method or using an array-find pattern for readability: ```ruby cm.role = (%w[admin lead member] & roles).first || "client" ``` This is semantically equivalent and reads in one pass. Non-blocking. 2. **No spec for CrewMember auto-create on login**: The sessions request spec (`spec/requests/sessions_spec.rb`) tests session population but does not assert that a `CrewMember` record is created during the OmniAuth callback. A test like `expect { post "/auth/keycloak"; follow_redirect! }.to change(CrewMember, :count).by(1)` would close this gap. The model validations are well-tested, but the integration path through the controller is not. Non-blocking for this prereq PR since the feature is exercised transitively (profile specs call `sign_in_as` which triggers the callback), but worth adding. 3. **No spec for `current_crew_member` helper**: No request spec asserts that `current_crew_member` returns the correct record. Again non-blocking for a prereq PR -- the downstream service request tickets will exercise this -- but noting the gap. 4. **Comment on `accepted -> declined` exclusion**: The business decision to disallow `accepted -> declined` is intentional but undocumented. A brief code comment on the transition map explaining why (e.g., "once accepted, cancellation goes through a different workflow") would help future readers. Non-blocking. ### SOP COMPLIANCE - [x] PR body has ## Summary, ## Changes, ## Test Plan, ## Related - [x] No secrets committed - [x] No unnecessary file changes (4 files, all on-topic) - [x] Commit messages are descriptive - [x] No .env files or credentials in diff - [x] Scope is tight -- four related auth prereqs, no feature creep ### PROCESS OBSERVATIONS - **Change failure risk:** Low. The `sub` addition to the session hash is additive (no existing keys changed). The `accepted` status is application-level only (no migration). The `find_or_create_by!` is idempotent on subsequent logins. - **Deployment notes:** No migration needed. The `accepted` status will be available immediately on deploy. Existing `service_requests` in `quoted` status will now transition to `accepted` instead of `paid` -- any UI or API code that attempts `quoted -> paid` will get a validation error. This is the intended behavior per the PR description. - **Documentation:** The four downstream tickets (#123, #176, #179, #180) are referenced. Clear dependency chain. ### VERDICT: APPROVED

ldraney force-pushed auth-session-prereqs from b1dda1fa6f to cd6b00c79b 2026-06-11 03:12:23 +00:00 Compare

ldraney merged commit a0d6773ddf into main 2026-06-11 03:12:29 +00:00

ldraney deleted branch auth-session-prereqs 2026-06-11 03:12:29 +00:00

ldraney referenced this pull request from a commit 2026-06-11 03:12:30 +00:00

Auth prereqs: session sub, accepted status, CrewMember auto-create (#196)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant!196

No description provided.