Add "shadow" property to feature flags

This allows a feature flag to remain hidden, even after being enabled. Only Site Admins will be able to see and opt in/out of shadow features. The generated API documentation is not changed, as the shadow property is not shown to non-Site Admins and is meant for internal use only. This also enables the shadow property for the the `send_usage_metrics` feature flag. Closes FOO-3277 flag=none Test Plan: * Set `shadow: true` on a feature * Verify it's shown in the UI if you're a site admin. * Verify you can't see it as a root account admin, even when enabled. Change-Id: I5d764c50014b1bb1d2064a22cb5f88022d752e3a Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/309190 Tested-by: Service Cloud Jenkins <[email protected]> QA-Review: Jason Perry <[email protected]> Reviewed-by: Jeremy Stanley <[email protected]> Product-Review: Jesse Poulos <[email protected]>
Yehuda-Binik · Feb 9, 2023 · f988fdd · f988fdd
1 parent 85747c2
commit f988fdd
Show file tree

Hide file tree

Showing 11 changed files with 132 additions and 6 deletions.
diff --git a/app/controllers/feature_flags_controller.rb b/app/controllers/feature_flags_controller.rb
@@ -154,7 +154,8 @@ def index
 
       flags = features.filter_map do |fd|
         @context.lookup_feature_flag(fd.feature,
-                                     override_hidden: Account.site_admin.grants_right?(@current_user, session, :read),
+                                     override_hidden: can_read_site_admin?,
+                                     include_shadowed: can_read_site_admin?,
                                      skip_cache: skip_cache,
                                      # Hide flags that are forced ON at a higher level
                                      # Undocumented flag for frontend use only
@@ -227,7 +228,8 @@ def show
       raise ActiveRecord::RecordNotFound unless Feature.definitions.key?(feature.to_s)
 
       flag = @context.lookup_feature_flag(feature,
-                                          override_hidden: Account.site_admin.grants_right?(@current_user, session, :read),
+                                          override_hidden: can_read_site_admin?,
+                                          include_shadowed: can_read_site_admin?,
                                           skip_cache: @context.grants_right?(@current_user, session, :manage_feature_flags))
       raise ActiveRecord::RecordNotFound unless flag
 
@@ -331,6 +333,10 @@ def delete
 
   private
 
+  def can_read_site_admin?
+    @can_read_site_admin ||= Account.site_admin.grants_right?(@current_user, session, :read)
+  end
+
   def create_or_update_feature_flag(attributes, current_flag = nil)
     FeatureFlag.unique_constraint_retry do
       new_flag = @context.feature_flags.find(current_flag.id) if current_flag &&

diff --git a/config/feature_flags/00_standard.yml b/config/feature_flags/00_standard.yml
@@ -435,6 +435,7 @@ send_usage_metrics:
   description: Send usage metrics
   applies_to: RootAccount
   visible_on: usage_metrics_allowed_hook
+  shadow: true
 new_quiz_public_api:
   applies_to: SiteAdmin
   state: hidden

diff --git a/lib/api/v1/feature_flag.rb b/lib/api/v1/feature_flag.rb
@@ -25,6 +25,8 @@ def feature_json(feature, _current_user, _session)
     # this isn't an AR object, so api_json doesn't work
     hash = feature.as_json.slice("feature", "applies_to", "root_opt_in", "beta",
                                  "release_notes_url", "autoexpand", "type")
+    # Only show the shdadow attribute if it's true, non-site-admin users don't need to see it exists
+    hash["shadow"] = true if feature.shadow?
     add_localized_attr(hash, feature, "display_name")
     add_localized_attr(hash, feature, "description")
     hash

diff --git a/lib/feature.rb b/lib/feature.rb
@@ -20,7 +20,7 @@
 
 class Feature
   ATTRS = %i[feature display_name description applies_to state
-             root_opt_in beta type
+             root_opt_in beta type shadow
              release_notes_url custom_transition_proc visible_on
              after_state_change_proc autoexpand touch_context].freeze
   attr_reader(*ATTRS)
@@ -61,6 +61,10 @@ def hidden?
     @state == "hidden"
   end
 
+  def shadow?
+    @shadow || false
+  end
+
   def self.environment
     if Rails.env.development?
       :development

diff --git a/lib/feature_flags.rb b/lib/feature_flags.rb
@@ -131,11 +131,12 @@ def feature_flag_account_ids
 
   # find the feature flag setting that applies to this object
   # it may be defined on the object or inherited
-  def lookup_feature_flag(feature, override_hidden: false, skip_cache: false, hide_inherited_enabled: false, inherited_only: false)
+  def lookup_feature_flag(feature, override_hidden: false, skip_cache: false, hide_inherited_enabled: false, inherited_only: false, include_shadowed: true)
     feature = feature.to_s
     feature_def = Feature.definitions[feature]
     raise "no such feature - #{feature}" unless feature_def
     return nil unless feature_def.applies_to_object(self)
+    return nil if feature_def.shadow? && !include_shadowed
 
     return nil if feature_def.visible_on.is_a?(Proc) && !feature_def.visible_on.call(self)
     return return_flag(feature_def, hide_inherited_enabled) unless feature_def.can_override? || feature_def.hidden?

diff --git a/spec/apis/v1/feature_flags_api_spec.rb b/spec/apis/v1/feature_flags_api_spec.rb
@@ -206,6 +206,26 @@
       end
     end
 
+    describe "shadow" do
+      before do
+        allow(Feature).to receive(:definitions).and_return({ "shadow_feature" => Feature.new(feature: "shadow_feature", applies_to: "Account", state: "allowed", shadow: true), })
+      end
+
+      it "shows shadow flag to a site admin user" do
+        json = api_call_as_user(site_admin_user, :get, "/api/v1/accounts/#{t_root_account.id}/features",
+                                { controller: "feature_flags", action: "index", format: "json", account_id: t_root_account.to_param })
+        feature = json.find { |f| f["feature"] == "shadow_feature" }
+        expect(feature).to have_key("shadow")
+        expect(feature["shadow"]).to eq true
+      end
+
+      it "does not show shadow feature at all to a non-site-admin user" do
+        json = api_call_as_user(t_root_admin, :get, "/api/v1/accounts/#{t_root_account.id}/features",
+                                { controller: "feature_flags", action: "index", format: "json", account_id: t_root_account.to_param })
+        expect(json.find { |f| f["feature"] == "shadow_feature" }).to be_nil
+      end
+    end
+
     it "operates on a course" do
       allow(Feature).to receive(:definitions).and_return({
                                                            "granular_permissions_manage_courses" => granular_permissions_feature,

diff --git a/spec/lib/feature_flags_spec.rb b/spec/lib/feature_flags_spec.rb
@@ -45,6 +45,7 @@
                                                          "hidden_feature" => Feature.new(feature: "hidden_feature", applies_to: "Course", state: "hidden"),
                                                          "hidden_root_opt_in_feature" => Feature.new(feature: "hidden_feature", applies_to: "Course", state: "hidden", root_opt_in: true),
                                                          "hidden_user_feature" => Feature.new(feature: "hidden_user_feature", applies_to: "User", state: "hidden"),
+                                                         "shadow_feature" => Feature.new(feature: "shadow_feature", applies_to: "Course", state: "on", shadow: true),
                                                          "disabled_feature" => Feature::DISABLED_FEATURE
                                                        })
     allow(analytics_service).to receive(:persist_feature_evaluation)
@@ -102,7 +103,8 @@
         "Feature double",
         feature: "some_feature",
         visible_on: ->(_) { return false },
-        state: "allowed"
+        state: "allowed",
+        shadow?: false
       )
       expect(feature).to receive(:applies_to_object).and_return(true)
       allow(Feature.definitions).to receive(:[]).and_call_original
@@ -320,6 +322,13 @@
       end
     end
 
+    describe "shadow" do
+      it "does not find the feature unless site admin" do
+        expect(t_root_account.lookup_feature_flag("shadow_feature", include_shadowed: false)).to be_nil
+        expect(t_root_account.lookup_feature_flag("shadow_feature", include_shadowed: true)).to be_default
+      end
+    end
+
     context "cross-sharding" do
       specs_require_sharding
 

diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb
@@ -127,6 +127,34 @@
     end
   end
 
+  describe "Shadow features" do
+    it "is not shadow? by default" do
+      expect(Feature.definitions["SA"].shadow?).to be_falsey
+      expect(Feature.definitions["RA"].shadow?).to be_falsey
+      expect(Feature.definitions["A"].shadow?).to be_falsey
+      expect(Feature.definitions["C"].shadow?).to be_falsey
+      expect(Feature.definitions["U"].shadow?).to be_falsey
+    end
+
+    context "when shadowed" do
+      before do
+        Feature.definitions["SA"].instance_variable_set(:@shadow, true)
+        Feature.definitions["RA"].instance_variable_set(:@shadow, true)
+        Feature.definitions["A"].instance_variable_set(:@shadow, true)
+        Feature.definitions["C"].instance_variable_set(:@shadow, true)
+        Feature.definitions["U"].instance_variable_set(:@shadow, true)
+      end
+
+      it "is shadow?" do
+        expect(Feature.definitions["SA"].shadow?).to be_truthy
+        expect(Feature.definitions["RA"].shadow?).to be_truthy
+        expect(Feature.definitions["A"].shadow?).to be_truthy
+        expect(Feature.definitions["C"].shadow?).to be_truthy
+        expect(Feature.definitions["U"].shadow?).to be_truthy
+      end
+    end
+  end
+
   describe "RootAccount feature" do
     it "implies root_opt_in" do
       expect(Feature.definitions["RA"].root_opt_in).to be_truthy

diff --git a/ui/shared/feature-flags/react/FeatureFlagTable.js b/ui/shared/feature-flags/react/FeatureFlagTable.js
@@ -60,7 +60,7 @@ function FeatureFlagTable({title, rows, disableDefaults}) {
               </Cell>
               <Cell>
                 <>
-                  {feature.feature_flag.hidden && (
+                  {feature.feature_flag.hidden && !feature.shadow && (
                     <Tooltip
                       isShowingContent={feature.feature === visibleTooltip}
                       onShowContent={() => setVisibleTooltip(feature.feature)}
@@ -79,6 +79,22 @@ function FeatureFlagTable({title, rows, disableDefaults}) {
                       <Pill margin="0 x-small">{I18n.t('Hidden')}</Pill>
                     </Tooltip>
                   )}
+                  {feature.shadow && (
+                    <Tooltip
+                      renderTip={
+                        <View as="div" width="600px">
+                          {I18n.t(
+                            `This feature option is only visible to users with Site Admin access. It is similar to
+                          "Hidden", but end users will not see it even if enabled by a Site Admin user.`
+                          )}
+                        </View>
+                      }
+                    >
+                      <Pill color="alert" margin="0 x-small">
+                        {I18n.t('Shadow')}
+                      </Pill>
+                    </Tooltip>
+                  )}
                   {feature.beta && (
                     <Tooltip
                       renderTip={I18n.t(

diff --git a/ui/shared/feature-flags/react/__tests__/FeatureFlagTable.test.js b/ui/shared/feature-flags/react/__tests__/FeatureFlagTable.test.js
@@ -30,6 +30,7 @@ const rows = [
   sampleData.offFeature,
   sampleData.siteAdminOnFeature,
   sampleData.siteAdminOffFeature,
+  sampleData.shadowedRootAccountFeature,
 ]
 const title = 'Section 123'
 
@@ -76,4 +77,15 @@ describe('feature_flags::FeatureFlagTable', () => {
       ).length
     ).toBe(2)
   })
+
+  it('Includes tooltips for shadow features', () => {
+    const {getAllByTestId, getByText} = render(<FeatureFlagTable rows={rows} title={title} />)
+    expect(getAllByTestId('ff-table-row')[7]).toHaveTextContent('Shadow')
+    expect(
+      getByText(
+        'This feature option is only visible to users with Site Admin access. It is similar to "Hidden",' +
+          ' but end users will not see it even if enabled by a Site Admin user.'
+      )
+    ).toBeInTheDocument()
+  })
 })
diff --git a/ui/shared/feature-flags/react/__tests__/sampleData.json b/ui/shared/feature-flags/react/__tests__/sampleData.json
@@ -130,6 +130,33 @@
     },
     "type": "feature_option"
   },
+  "shadowedRootAccountFeature": {
+    "feature": "shadowFeature",
+    "applies_to": "RootAccount",
+    "root_opt_in": true,
+    "display_name": "Shadow Feature",
+    "description": "This does ninja things",
+    "shadow": true,
+    "feature_flag": {
+      "feature": "shadow_feature",
+      "state": "allowed_on",
+      "transitions": {
+        "off": {
+          "locked": false
+        },
+        "on": {
+          "locked": false
+        },
+        "allowed": {
+          "locked": true
+        }
+      },
+      "locked": false,
+      "hidden": true,
+      "parent_state": "allowed_on"
+    },
+    "type": "feature_option"
+  },
   "allowedOnCourseFeature": {
     "feature": "feature6",
     "applies_to": "Course",