From f165c47b341999e4ba2163b072aa5e562eefabab Mon Sep 17 00:00:00 2001 From: Josh Puetz Date: Mon, 1 Jun 2020 09:23:19 -0500 Subject: [PATCH] Add feature flag support to Pages (#8149) * Add 'feature_flag' field to pages table, form * Add 'exist?' to FeatureFlag * Test for feature flag when loading custom pages * Refactor FeatureFlag.enabled? to check both boolean and user gates * Ugly test fixes * PR feedback and refactoring * PR refactor: don't require database table! --- app/controllers/pages_controller.rb | 2 + app/controllers/stories_controller.rb | 8 ++- app/helpers/feature_flag.rb | 8 +-- app/models/page.rb | 4 ++ app/views/internal/pages/_form.html.erb | 20 +++++++- spec/helpers/feature_flag_spec.rb | 66 +++++++++++++++++++++++++ 6 files changed, 102 insertions(+), 6 deletions(-) create mode 100644 spec/helpers/feature_flag_spec.rb diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index 0d6b7105c..36fb9d682 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -4,6 +4,8 @@ class PagesController < ApplicationController def show @page = Page.find_by!(slug: params[:slug]) + not_found unless FeatureFlag.accessible?(@page.feature_flag_name, current_user) + set_surrogate_key_header "show-page-#{params[:slug]}" end diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 44fa510a7..02fe7313a 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -111,8 +111,12 @@ class StoriesController < ApplicationController Honeycomb.add_field("stories_route", "org") handle_organization_index elsif @page - Honeycomb.add_field("stories_route", "page") - handle_page_display + if FeatureFlag.accessible?(@page.feature_flag_name, current_user) + Honeycomb.add_field("stories_route", "page") + handle_page_display + else + not_found + end else Honeycomb.add_field("stories_route", "user") handle_user_index diff --git a/app/helpers/feature_flag.rb b/app/helpers/feature_flag.rb index 6e7d9d559..737ce0c91 100644 --- a/app/helpers/feature_flag.rb +++ b/app/helpers/feature_flag.rb @@ -1,7 +1,9 @@ module FeatureFlag - extend self # rubocop:disable Style/ModuleFunction + class << self + delegate :enabled?, :exist?, to: Flipper - def enabled?(feature_name, *args) - Flipper[feature_name].enabled?(*args) + def accessible?(feature_flag_name, *args) + feature_flag_name.blank? || !exist?(feature_flag_name) || enabled?(feature_flag_name, *args) + end end end diff --git a/app/models/page.rb b/app/models/page.rb index 95f656f82..9bddc3107 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -18,6 +18,10 @@ class Page < ApplicationRecord is_top_level_path ? "/#{slug}" : "/page/#{slug}" end + def feature_flag_name + "page_#{slug}" + end + private def evaluate_markdown diff --git a/app/views/internal/pages/_form.html.erb b/app/views/internal/pages/_form.html.erb index 534f10e0f..085f5b57a 100644 --- a/app/views/internal/pages/_form.html.erb +++ b/app/views/internal/pages/_form.html.erb @@ -23,7 +23,7 @@
<% if @page.social_image_url %> - + <% end %> <%= form.label :social_image %> <%= form.file_field :social_image, class: "form-control" %> @@ -38,6 +38,24 @@ <%= form.check_box :is_top_level_path %>

(Determines if it is accessible by /page-slug vs /page/page-slug) Be careful! ⚠️

+
+

+ <%= link_to "Feature Flag", "/internal/feature_flags" %> + "> + <%= FeatureFlag.exist?(@page.feature_flag_name) ? "Present" : "Not Present" %> + +
+ <% if FeatureFlag.exist?(@page.feature_flag_name) %> + Access to this page is being guarded by the feature flag <%= @page.feature_flag_name %>. + <%= link_to "Modify flag here", "/internal/feature_flags/features/#{@page.feature_flag_name}" %> + <% else %> + Everyone has access. Optionally guard access to this page by creating feature <%= @page.feature_flag_name %> + <%= link_to "here", "/internal/feature_flags/features/" %> + <% end %> +
+ +

+
<%= form.submit class: "btn btn-primary float-right" %> <% end %> diff --git a/spec/helpers/feature_flag_spec.rb b/spec/helpers/feature_flag_spec.rb new file mode 100644 index 000000000..78d07147f --- /dev/null +++ b/spec/helpers/feature_flag_spec.rb @@ -0,0 +1,66 @@ +require "rails_helper" + +UserStruct = Struct.new(:flipper_id) + +describe FeatureFlag, type: :helper do + describe ".enabled?" do + it "calls Flipper's enabled? method" do + allow(Flipper).to receive(:enabled?).with("foo") + + described_class.enabled?("foo") + + expect(Flipper).to have_received(:enabled?).with("foo") + end + end + + describe ".exist?" do + it "calls Flipper's exist? method" do + allow(Flipper).to receive(:exist?).with("foo") + + described_class.exist?("foo") + + expect(Flipper).to have_received(:exist?).with("foo") + end + end + + describe ".accessible?" do + let(:user) { UserStruct.new(flipper_id: 1) } + + it "returns false when flag doesn't exist" do + expect(described_class.accessible?("missing_flag")).to be_truthy # rubocop:disable Rspec/PredicateMatcher + end + + it "returns true when flag is empty" do + expect(described_class.accessible?("")).to be_truthy # rubocop:disable Rspec/PredicateMatcher + end + + it "returns true when flag is nil" do + expect(described_class.accessible?(nil)).to be_truthy # rubocop:disable Rspec/PredicateMatcher + end + + context "when flag exists and is set to off" do + before { Flipper.disable("flag") } + + it "returns false" do + expect(described_class.accessible?("flag")).to be_falsy # rubocop:disable Rspec/PredicateMatcher + end + + it "returns true when flag is on for user" do + Flipper.enable_actor("flag", user) + expect(described_class.accessible?("flag", user)).to be_truthy # rubocop:disable Rspec/PredicateMatcher + end + end + + context "when flag exists and is set to on" do + before { Flipper.enable("flag") } + + it "returns true" do + expect(described_class.accessible?("flag")).to be_truthy # rubocop:disable Rspec/PredicateMatcher + end + + it "returns true for a user" do + expect(described_class.accessible?("flag", user)).to be_truthy # rubocop:disable Rspec/PredicateMatcher + end + end + end +end