ldraney/landscaping-assistant
1
0
Fork 0
Code Issues 32 Pull requests 1 Projects Releases Packages Wiki Activity Actions 4

Fix stale references in feature flags doc #141

Merged
ldraney merged 2 commits from fix/feature-flags-doc-accuracy into main 2026-06-07 01:55:03 +00:00
Conversation 1 Commits 2 Files changed 1 +3 -6
ldraney commented 2026-06-07 01:48:21 +00:00
Owner
Copy link

Summary

Feature flags architecture doc had 3 stale references from before auth shipped. Fixes them so the doc matches current implementation before #130 work begins.

Changes

  • Removed keycloak_login flag from rake task and planned flags table — Keycloak login shipped ungated (PR #134), gating it retroactively makes no sense
  • Fixed current_user.admin? → current_user_has_role?("admin") — current_user is a session hash, not an ActiveRecord model
  • Fixed current_user&.super_admin? → current_user_has_role?("super_admin") — same reason

Test Plan

  • Doc-only change, no code impact

Review Checklist

  • No unrelated changes
  • Doc accuracy verified against PR #136 implementation

Related Notes

  • Prerequisite accuracy fix before #130 implementation begins
  • docs/feature-flags.md is the spec that the #130 dev agent will read
  • Fixes #141
## Summary Feature flags architecture doc had 3 stale references from before auth shipped. Fixes them so the doc matches current implementation before #130 work begins. ## Changes - Removed `keycloak_login` flag from rake task and planned flags table — Keycloak login shipped ungated (PR #134), gating it retroactively makes no sense - Fixed `current_user.admin?` → `current_user_has_role?("admin")` — `current_user` is a session hash, not an ActiveRecord model - Fixed `current_user&.super_admin?` → `current_user_has_role?("super_admin")` — same reason ## Test Plan - [ ] Doc-only change, no code impact ## Review Checklist - [x] No unrelated changes - [x] Doc accuracy verified against PR #136 implementation ## Related Notes - Prerequisite accuracy fix before #130 implementation begins - `docs/feature-flags.md` is the spec that the #130 dev agent will read - Fixes #141
Fix stale references in feature flags doc
Some checks are pending
CI / scan_ruby (pull_request) Waiting to run
Details
CI / scan_js (pull_request) Waiting to run
Details
CI / lint (pull_request) Waiting to run
Details
ci/woodpecker/push/woodpecker Pipeline was successful
Details
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
6be911e65e 
- Remove keycloak_login flag (Keycloak login shipped ungated in PR #134)
- Fix current_user.admin? and current_user&.super_admin? to use
  current_user_has_role?() since current_user is a session hash

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-07 01:51:24 +00:00
Author
Owner
Copy link

PR #141 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails documentation (Markdown). No executable code changed -- doc-only diff touching docs/feature-flags.md.

Correctness of the three changes:

  1. Removed keycloak_login from rake seed and planned flags table -- Correct. Keycloak login shipped ungated in PR #134. Retroactively gating an already-shipped feature makes no sense. Both removal sites (rake task array at former line 155, planned flags table at former line 453) are addressed.

  2. current_user.admin? to current_user_has_role?("admin") (lines 117, 219) -- Correct. Verified in app/controllers/application_controller.rb: current_user returns session[:user]&.with_indifferent_access (a hash), not an ActiveRecord model. The .admin? method does not exist on it. current_user_has_role? is the real helper (line 56 of ApplicationController).

  3. current_user&.super_admin? to current_user_has_role?("super_admin") (line 274) -- Correct. Same reason. The safe navigation operator &. was also misleading -- current_user_has_role? already handles the nil case internally (return false unless logged_in?).

Completeness check: No remaining current_user.admin?, current_user&.super_admin?, or keycloak_login references remain in the code examples after this diff. The doc reads coherently.

BLOCKERS

None. This is a doc-only accuracy fix with no code impact.

NITS

  1. Stale reference on line 12 -- The Philosophy section still lists "Keycloak login switchover (#115) -- external auth service swap" as an example of what gets flagged. This is now misleading: the PR removes keycloak_login from both the rake seed and the planned flags table specifically because Keycloak login shipped ungated. Keeping it as a "what gets flagged" example contradicts the decision. Consider either removing it or rewording to past tense (e.g., "Keycloak login switchover (#115) -- would have been flagged, but shipped ungated").

  2. PR self-references as its own issue -- The PR body says "Fixes #141" but #141 is this PR itself (Forgejo shares issue/PR numbering). There is no standalone issue for this fix. This is fine for a trivial doc fix, but worth noting.

  3. Branch naming -- Branch is fix/feature-flags-doc-accuracy which uses slash-delimited prefix style rather than the {issue-number}-{kebab-case-purpose} convention. Minor for a doc-only fix with no backing issue.

SOP COMPLIANCE

  • Branch named after issue -- fix/feature-flags-doc-accuracy does not follow {issue-number}-{kebab-case} convention (no backing issue exists)
  • PR body follows template -- Summary, Changes, Test Plan, Review Checklist all present
  • Related section references plan slug -- No ## Related section with plan slug
  • No secrets committed
  • No unnecessary file changes -- single file, all changes on-topic
  • Commit messages are descriptive

PROCESS OBSERVATIONS

Low-risk, high-value change. Fixing doc accuracy before the #130 implementation begins prevents the dev agent from writing code against stale examples. The three corrections are all verifiable against the actual codebase. Change failure risk is zero (no executable code).

VERDICT: APPROVED

## PR #141 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails documentation (Markdown). No executable code changed -- doc-only diff touching `docs/feature-flags.md`. **Correctness of the three changes:** 1. **Removed `keycloak_login` from rake seed and planned flags table** -- Correct. Keycloak login shipped ungated in PR #134. Retroactively gating an already-shipped feature makes no sense. Both removal sites (rake task array at former line 155, planned flags table at former line 453) are addressed. 2. **`current_user.admin?` to `current_user_has_role?("admin")`** (lines 117, 219) -- Correct. Verified in `app/controllers/application_controller.rb`: `current_user` returns `session[:user]&.with_indifferent_access` (a hash), not an ActiveRecord model. The `.admin?` method does not exist on it. `current_user_has_role?` is the real helper (line 56 of ApplicationController). 3. **`current_user&.super_admin?` to `current_user_has_role?("super_admin")`** (line 274) -- Correct. Same reason. The safe navigation operator `&.` was also misleading -- `current_user_has_role?` already handles the nil case internally (`return false unless logged_in?`). **Completeness check**: No remaining `current_user.admin?`, `current_user&.super_admin?`, or `keycloak_login` references remain in the code examples after this diff. The doc reads coherently. ### BLOCKERS None. This is a doc-only accuracy fix with no code impact. ### NITS 1. **Stale reference on line 12** -- The Philosophy section still lists "Keycloak login switchover (#115) -- external auth service swap" as an example of what gets flagged. This is now misleading: the PR removes `keycloak_login` from both the rake seed and the planned flags table specifically because Keycloak login shipped ungated. Keeping it as a "what gets flagged" example contradicts the decision. Consider either removing it or rewording to past tense (e.g., "Keycloak login switchover (#115) -- would have been flagged, but shipped ungated"). 2. **PR self-references as its own issue** -- The PR body says "Fixes #141" but #141 is this PR itself (Forgejo shares issue/PR numbering). There is no standalone issue for this fix. This is fine for a trivial doc fix, but worth noting. 3. **Branch naming** -- Branch is `fix/feature-flags-doc-accuracy` which uses slash-delimited prefix style rather than the `{issue-number}-{kebab-case-purpose}` convention. Minor for a doc-only fix with no backing issue. ### SOP COMPLIANCE - [ ] Branch named after issue -- `fix/feature-flags-doc-accuracy` does not follow `{issue-number}-{kebab-case}` convention (no backing issue exists) - [x] PR body follows template -- Summary, Changes, Test Plan, Review Checklist all present - [ ] Related section references plan slug -- No `## Related` section with plan slug - [x] No secrets committed - [x] No unnecessary file changes -- single file, all changes on-topic - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS Low-risk, high-value change. Fixing doc accuracy before the #130 implementation begins prevents the dev agent from writing code against stale examples. The three corrections are all verifiable against the actual codebase. Change failure risk is zero (no executable code). ### VERDICT: APPROVED
Remove remaining keycloak_login reference from examples
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
ci/woodpecker/push/woodpecker Pipeline was successful
Details
CI / scan_ruby (pull_request) Has been cancelled
Details
CI / scan_js (pull_request) Has been cancelled
Details
CI / lint (pull_request) Has been cancelled
Details
98d8055e3d 
QA review caught stale "Keycloak login switchover" in the flagging
examples list.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney deleted branch fix/feature-flags-doc-accuracy 2026-06-07 01:55:03 +00:00
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
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!141
No description provided.