Fix crew specs: expect /login redirect (matches PR #151 hotfix) #163

Merged

ldraney merged 1 commit from fix-crew-spec-redirect-targets into main 2026-06-07 17:38:13 +00:00

Conversation 1 Commits 1 Files changed 2 +2 -2

ldraney commented 2026-06-07 17:29:31 +00:00

Owner

Copy link

Summary

Fix two failing specs that still expected redirect to /auth/keycloak instead of /login after PR #151 changed the unauthenticated redirect target.

Changes

• spec/requests/crew_spec.rb line 95: changed redirect_to("/auth/keycloak") to redirect_to("/login")
• spec/requests/role_access_spec.rb line 49: changed redirect_to("/auth/keycloak") to redirect_to("/login")

Test Plan

• Pipeline #348 should pass with these fixes
• All other redirect expectations in both files already use /login and are unaffected

Related

• Root cause: PR #151 hotfix changed redirect target but missed these two assertions

## Summary Fix two failing specs that still expected redirect to `/auth/keycloak` instead of `/login` after PR #151 changed the unauthenticated redirect target. ## Changes - `spec/requests/crew_spec.rb` line 95: changed `redirect_to("/auth/keycloak")` to `redirect_to("/login")` - `spec/requests/role_access_spec.rb` line 49: changed `redirect_to("/auth/keycloak")` to `redirect_to("/login")` ## Test Plan - Pipeline #348 should pass with these fixes - All other redirect expectations in both files already use `/login` and are unaffected ## Related - Root cause: PR #151 hotfix changed redirect target but missed these two assertions

ldraney added 1 commit 2026-06-07 17:29:31 +00:00

Fix crew specs: expect /login redirect (matches PR #151 hotfix) ... a99954e738

Two specs still expected redirect to /auth/keycloak, but PR #151 changed
the unauthenticated redirect target to /login. Pipeline #348 failure.

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

ldraney commented 2026-06-07 17:35:41 +00:00

Author

Owner

Copy link

PR #163 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails request specs (RSpec)

Correctness analysis:

The application controller (app/controllers/application_controller.rb, line 52) defines:

def authenticate_user!
  return if logged_in?
  redirect_to login_path
end

And config/routes.rb line 8 maps:

get "/login", to: "sessions#new", as: :login

The redirect target /login in the specs is correct. This matches the behavior introduced in PR #151.

Completeness analysis:

I searched every request spec file in the repository:

• spec/requests/crew_spec.rb -- uses /login (after this fix)
• spec/requests/role_access_spec.rb -- uses /login (after this fix)
• spec/requests/sessions_spec.rb -- uses post "/auth/keycloak" (correct -- this is the OmniAuth route, not a redirect target)
• spec/requests/person_spec.rb -- already uses /login
• spec/requests/platform/feature_flags_spec.rb -- already uses /login
• spec/requests/properties_spec.rb -- no auth redirect tests
• spec/requests/work_queue_items_spec.rb -- no auth redirect tests
• spec/requests/weeks_spec.rb -- no auth redirect tests
• spec/requests/uploads_spec.rb -- no auth redirect tests
• spec/requests/client_errors_spec.rb -- no auth redirect tests

No remaining occurrences of redirect_to("/auth/keycloak") exist anywhere in the spec directory. The fix is complete.

BLOCKERS

None.

NITS

1. The it description on crew_spec.rb line 93 still reads "redirects unauthenticated users to Keycloak" and role_access_spec.rb line 48 reads "redirects /crew/:id to Keycloak login". These are slightly misleading now that the redirect goes to /login (an intermediate login page) rather than directly to the OmniAuth endpoint. A more accurate description would be "redirects unauthenticated users to login". Non-blocking since the actual assertion is correct.

SOP COMPLIANCE

• Branch named after issue -- branch is fix-crew-spec-redirect-targets (no issue number prefix; appears to be a hotfix without a standalone issue)
• PR body follows template -- has Summary, Changes, Test Plan, Related sections
• Related references root cause -- references PR #151 as root cause
• No secrets committed
• No unnecessary file changes -- exactly two assertions changed, nothing else

PROCESS OBSERVATIONS

This is a targeted fix for a pipeline failure. The scope is minimal and correct. The two-line change aligns perfectly with the application's actual behavior. No deployment risk -- this only affects test assertions.

VERDICT: APPROVED

## PR #163 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails request specs (RSpec) **Correctness analysis:** The application controller (`app/controllers/application_controller.rb`, line 52) defines: ```ruby def authenticate_user! return if logged_in? redirect_to login_path end ``` And `config/routes.rb` line 8 maps: ```ruby get "/login", to: "sessions#new", as: :login ``` The redirect target `/login` in the specs is correct. This matches the behavior introduced in PR #151. **Completeness analysis:** I searched every request spec file in the repository: - `spec/requests/crew_spec.rb` -- uses `/login` (after this fix) - `spec/requests/role_access_spec.rb` -- uses `/login` (after this fix) - `spec/requests/sessions_spec.rb` -- uses `post "/auth/keycloak"` (correct -- this is the OmniAuth route, not a redirect target) - `spec/requests/person_spec.rb` -- already uses `/login` - `spec/requests/platform/feature_flags_spec.rb` -- already uses `/login` - `spec/requests/properties_spec.rb` -- no auth redirect tests - `spec/requests/work_queue_items_spec.rb` -- no auth redirect tests - `spec/requests/weeks_spec.rb` -- no auth redirect tests - `spec/requests/uploads_spec.rb` -- no auth redirect tests - `spec/requests/client_errors_spec.rb` -- no auth redirect tests No remaining occurrences of `redirect_to("/auth/keycloak")` exist anywhere in the spec directory. The fix is complete. ### BLOCKERS None. ### NITS 1. The `it` description on crew_spec.rb line 93 still reads "redirects unauthenticated users to Keycloak" and role_access_spec.rb line 48 reads "redirects /crew/:id to Keycloak login". These are slightly misleading now that the redirect goes to `/login` (an intermediate login page) rather than directly to the OmniAuth endpoint. A more accurate description would be "redirects unauthenticated users to login". Non-blocking since the actual assertion is correct. ### SOP COMPLIANCE - [ ] Branch named after issue -- branch is `fix-crew-spec-redirect-targets` (no issue number prefix; appears to be a hotfix without a standalone issue) - [x] PR body follows template -- has Summary, Changes, Test Plan, Related sections - [x] Related references root cause -- references PR #151 as root cause - [x] No secrets committed - [x] No unnecessary file changes -- exactly two assertions changed, nothing else ### PROCESS OBSERVATIONS This is a targeted fix for a pipeline failure. The scope is minimal and correct. The two-line change aligns perfectly with the application's actual behavior. No deployment risk -- this only affects test assertions. ### VERDICT: APPROVED

ldraney merged commit ab5f7281f5 into main 2026-06-07 17:38:13 +00:00

ldraney deleted branch fix-crew-spec-redirect-targets 2026-06-07 17:38:13 +00:00

ldraney referenced this pull request from a commit 2026-06-07 17:38:13 +00:00

Fix crew specs: expect /login redirect (matches PR #151 hotfix) (#163)

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!163

No description provided.