Add database-backed feature flags with super admin UI (#130) #144
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "130-feature-flags"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements a database-backed feature flag system that allows toggling features on/off in production without redeploying. Includes a super admin UI at
/platform/feature_flagsfor managing flags, cached lookups with 1-minute TTL, and an idempotent rake task for registering flags on deploy.Changes
db/migrate/20260606010000_create_feature_flags.rb-- migration forfeature_flagstable with unique name indexapp/models/feature_flag.rb-- model withenabled?,enable,disableclass methods andRails.cache.fetchcachingapp/controllers/platform/feature_flags_controller.rb-- index and toggle actions gated tosuper_adminrole (returns 404 for non-super-admin)app/views/platform/feature_flags/index.html.erb-- table UI with ON/OFF status badges and toggle buttonsapp/controllers/application_controller.rb-- addedfeature_enabled?helper method exposed to viewsconfig/routes.rb-- addedplatformnamespace withfeature_flagsresourceapp/views/layouts/application.html.erb-- Platform link under Person icon for super_admin users onlyapp/assets/stylesheets/application.css-- styles for PlatformNav and FlagsTable components (design tokens, component comments, no Tailwind)lib/tasks/feature_flags.rake-- idempotentfeature_flags:synctask withstripeandclient_requestsflagsspec/support/feature_flags.rb--with_feature_disabled/with_feature_enabledtest helpers andbefore(:each)hookspec/models/feature_flag_spec.rb-- model validations,enabled?,enable,disable, cache behaviorspec/requests/platform/feature_flags_spec.rb-- super admin access, toggle, 404 for admin/memberspec/requests/feature_flag_gating_spec.rb-- demonstrates controller-level flag gating patternTest Plan
enabled?returns correct boolean, caching works with MemoryStore,enable/disablebust cachewith_feature_disabled/with_feature_enabledblock helpers work correctlyReview Checklist
!important, no#idselectors)docs/feature-flags.mdnot modified (implementation follows the doc as spec)Related Notes
docs/feature-flags.md(used as implementation guide, not modified)Related
Closes #130
Self-Review Findings
Reviewed the diff against issue #130 spec and
docs/feature-flags.mdarchitecture doc.Acceptance Criteria Checklist
feature_flagstable with correct columns, constraints, and unique indexFeatureFlagmodel with validations,enabled?,enable,disableclass methodsenabled?usesRails.cache.fetchwith 1-minute TTL;enable/disablebust cachepick(:enabled)for minimal DB overheadfeature_enabled?helper inApplicationController, exposed viahelper_methodfeature_flags:syncrake task withstripeandclient_requestsflags (disabled by default)find_or_create_by!/platform/feature_flagswith table and toggle buttonsrequire_super_adminreturns 404 (not 403) for non-super-adminwith_feature_disabledandwith_feature_enabledbefore(:each)hook enables all flags and clears cache (withtable_exists?guard)Notes
Controller cache bust is intentional duplication: The
updateaction manually callsRails.cache.deleteafterflag.update!rather than delegating toFeatureFlag.enable/FeatureFlag.disable. This matches the architecture doc exactly -- the controller toggles via!flag.enabled?which the model class methods don't support (they're explicit enable/disable). Acceptable.Cache test uses MemoryStore swap: Test env uses
NullStore, so the cache spec swaps inMemoryStorewith proper cleanup viaensure. Correct approach.CSS follows conventions: Design tokens used throughout, component comments present, no Tailwind, no inline styles, no
!important, no#idselectors.docs/feature-flags.mdnot modified: Confirmed.Schema version unchanged at
2026_06_07_011440: Correct -- the existingcreate_versionsmigration timestamp is higher than the new migration's.No Issues Found
VERDICT: APPROVE
PR #144 Review
DOMAIN REVIEW
Stack: Ruby on Rails 8.1, RSpec, Hotwire/Turbo, plain CSS with design tokens. Reviewing against Rails conventions, OWASP top 10, the
~/ror-css-guideCSS conventions, and thedocs/feature-flags.mdarchitecture spec.Model (
app/models/feature_flag.rb)validates :name, presence: true, uniqueness: true,CACHE_TTL = 1.minute,Rails.cache.fetchwithpick(:enabled),find_or_create_by!inenable/disable, cache busting on write. Clean.Controller (
app/controllers/platform/feature_flags_controller.rb)current_user_has_role?("super_admin")-- correct. This calls into the session hash viacurrent_user[:roles].include?, not a method on a user object. Matches the AC requirement.head :not_found(404) for non-super-admin -- correct per spec. Does not leak information about the route's existence.updateaction toggles via!flag.enabled?, busts cache, redirects with flash notice. Straightforward.ApplicationControllerviaActionController::Base-- toggle is a PATCH withbutton_to, which generates a form with an authenticity token. Correct.ApplicationController helper
feature_enabled?delegates toFeatureFlag.enabled?and is added to thehelper_methodlist. Correct pattern -- available in both controllers and views.Routes (
config/routes.rb)namespace :platform do resources :feature_flags, only: [:index, :update] end-- properly namespaced. Only the two needed actions are exposed. No extraneous routes.Migration
name(string, not null),enabled(boolean, not null, default false),description(string, nullable), timestamps, unique index onname. Clean.Rake task (
lib/tasks/feature_flags.rake)feature_flags:syncwithfind_or_create_by!pattern. Registersstripeandclient_requestsflags withenabled: false. Idempotent -- existing flags keep their currentenabledstate. NOT in seeds. Matches spec.Layout (
app/views/layouts/application.html.erb)logged_in? && current_user_has_role?("super_admin"). Uses a gear/settings SVG icon.is-activeclass based oncontroller_name == 'feature_flags'. Correct placement per architecture doc.View (
app/views/platform/feature_flags/index.html.erb)button_towithmethod: :patch-- standard Turbo form, no JS needed. Output is properly escaped via ERB<%= %>tags (noraworhtml_safeusage). No XSS risk.CSS (
application.css)PlatformNavandFlagsTablewith view paths. Follows~/ror-css-guidepattern.var(--color-muted),var(--color-border),var(--color-success),var(--color-danger), etc.var(--spacing-xs),var(--spacing-sm),var(--spacing-md).var(--radius)for border-radius.color: whiteon hover states for.btn-flag-enable:hoverand.btn-flag-disable:hover. This is a contrast-on-solid-bg pattern (white text on green/red background). There is no--color-surface-on-accenttoken defined, sowhiteis the pragmatic choice here. Acceptable given the token system does not have a "text on colored background" token.!important, no#idselectors. Compliant.border-radius: 1remon.flag-status(pill shape) andborder-radius: 4pxon.flags-table code. The pill radius is intentional (large value = pill), but4pxoncodeshould ideally usevar(--radius)or a smaller token. Minor nit.Test support (
spec/support/feature_flags.rb)with_feature_disabled/with_feature_enabledblock helpers withensurefor cleanup.before(:each)hook withtable_exists?guard enables all flags and clears cache. Matches the doc exactly.Model spec (
spec/models/feature_flag_spec.rb).enabled?(true/false/nonexistent/string names/caching),.enable(toggle/create/cache bust),.disable(toggle/create/cache bust),.cache_key. The caching test swaps inMemoryStoreand restores inensure-- thorough. Good coverage.Request specs (
spec/requests/platform/feature_flags_spec.rb)Gating spec (
spec/requests/feature_flag_gating_spec.rb)with_feature_disabledandwith_feature_enabledblock behavior. Testsfeature_enabled?for enabled/disabled/nonexistent flags. Good pattern documentation.BLOCKERS
None.
NITS
CSS magic number:
.flags-table codeusesborder-radius: 4pxinstead ofvar(--radius)(which is8px). If the intent is a smaller radius, consider defining a--radius-smtoken. Very minor.with_feature_enabledensure block: Theensureclause callsFeatureFlag.enable(name)-- this means it restores to "enabled" regardless of the flag's state before the block was entered. The comment says "Reset to default-on state" which is correct for the test environment (wherebefore(:each)enables everything), but the asymmetry withwith_feature_disabled(which re-enables, not "restores original") is worth noting. Not a bug given the test env contract, but could be surprising if used outside tests. Fine for now.with_keycloakduplication: Thewith_keycloakhelper is defined inline in bothfeature_flags_spec.rbandfeature_flag_gating_spec.rb(and already exists increw_spec.rb). This is a pre-existing pattern in the codebase -- the PR is following the established convention, not introducing new duplication. Worth extracting to a shared helper inspec/support/in a future cleanup ticket, but not a blocker for this PR.descriptioncolumn output escaping: The view uses<%= flag.description %>which is auto-escaped by ERB. Currently descriptions are only set via the rake task (developer-controlled strings), so there is no user-input XSS vector. If descriptions ever become editable via UI, the escaping is already correct. No issue.SOP COMPLIANCE
130-feature-flags(issue #130).envfiles, no credentials in codedocs/feature-flags.mdnot modified (confirmed: not in the diff)~/ror-css-guideconventions (design tokens, component comments, no Tailwind, no inline styles, no!important, no#idselectors)current_user_has_role?("super_admin")-- notcurrent_user&.super_admin?find_or_create_by!, not seedspick(:enabled)for hot pathRails.cache.fetchwith 1-minute TTLbefore(:each)withtable_exists?guard in test supportPROCESS OBSERVATIONS
docs/feature-flags.md) faithfully. The doc served as the spec and the code matches it line-for-line in the critical paths (model, controller, rake task, test support). This is how doc-first development should work.feature_enabled?to the helper_method list, which has no effect until code actually calls it.VERDICT: APPROVED