Merge branch 'support-curd-operation-for-flag-scopes' into 'master'

Support CURD operation for feature flag scopes (GitLab API)

See merge request gitlab-org/gitlab-ee!9182

ee/app/controllers/projects/feature_flags_controller.rb

@@ -3,6 +3,7 @@
 class Projects::FeatureFlagsController < Projects::ApplicationController
   respond_to :html
 
+  before_action :push_feature_flag_to_frontend!
   before_action :authorize_read_feature_flag!
   before_action :authorize_create_feature_flag!, only: [:new, :create]
   before_action :authorize_update_feature_flag!, only: [:edit, :update]
@@ -91,17 +92,19 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
   protected
 
   def feature_flag
-    @feature_flag ||= project.operations_feature_flags.find(params[:id])
+    @feature_flag ||= project.operations_feature_flags.for_list.find(params[:id])
   end
 
   def create_params
     params.require(:operations_feature_flag)
-          .permit(:name, :description, :active)
+          .permit(:name, :description, :active,
+                  scopes_attributes: [:environment_scope, :active])
   end
 
   def update_params
     params.require(:operations_feature_flag)
-          .permit(:name, :description, :active)
+          .permit(:name, :description, :active,
+                  scopes_attributes: [:id, :environment_scope, :active, :_destroy])
   end
 
   def feature_flag_json
@@ -135,4 +138,8 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
     render json: { message: feature_flag.errors.full_messages },
            status: :bad_request
   end
+
+  def push_feature_flag_to_frontend!
+    push_frontend_feature_flag(:feature_flags_environment_scope, project)
+  end
 end

ee/app/finders/feature_flags_finder.rb

@@ -17,6 +17,8 @@ class FeatureFlagsFinder
 
     items = feature_flags
     items = by_scope(items)
+    items = for_list(items)
+
     items.ordered
   end
 
@@ -32,4 +34,12 @@ class FeatureFlagsFinder
       items
     end
   end
+
+  def for_list(items)
+    if Feature.enabled?(:feature_flags_environment_scope, project)
+      items.for_list
+    else
+      items
+    end
+  end
 end

ee/app/models/operations/feature_flag.rb

@@ -12,6 +12,7 @@ module Operations
     belongs_to :project
 
     has_many :scopes, class_name: 'Operations::FeatureFlagScope'
+    has_one :default_scope, -> { where(environment_scope: '*') }, class_name: 'Operations::FeatureFlagScope'
 
     validates :project, presence: true
     validates :name,
@@ -24,15 +25,39 @@ module Operations
     validates :name, uniqueness: { scope: :project_id }
     validates :description, allow_blank: true, length: 0..255
 
+    before_create :build_default_scope
+    after_update :update_default_scope
+
+    accepts_nested_attributes_for :scopes, allow_destroy: true
+
     scope :ordered, -> { order(:name) }
-    scope :enabled, -> { where(active: true) }
-    scope :disabled, -> { where(active: false) }
+
+    scope :enabled, -> do
+      if Feature.enabled?(:feature_flags_environment_scope)
+        where('EXISTS (?)', join_enabled_scopes)
+      else
+        where(active: true)
+      end
+    end
+
+    scope :disabled, -> do
+      if Feature.enabled?(:feature_flags_environment_scope)
+        where('NOT EXISTS (?)', join_enabled_scopes)
+      else
+        where(active: false)
+      end
+    end
 
     scope :for_environment, -> (environment) do
       select("operations_feature_flags.*" \
              ", (#{actual_active_sql(environment)}) AS active")
     end
 
+    scope :for_list, -> do
+      select("operations_feature_flags.*" \
+             ", COALESCE((#{join_enabled_scopes.to_sql}), FALSE) AS active")
+    end
+
     class << self
       def actual_active_sql(environment)
         Operations::FeatureFlagScope
@@ -42,6 +67,16 @@ module Operations
           .select('active')
           .to_sql
       end
+
+      def join_enabled_scopes
+        Operations::FeatureFlagScope
+          .where('operations_feature_flags.id = feature_flag_id')
+          .enabled.limit(1).select('TRUE')
+      end
+
+      def preload_relations
+        preload(:scopes)
+      end
     end
 
     def strategies
@@ -49,5 +84,15 @@ module Operations
         { name: 'default' }
       ]
     end
+
+    private
+
+    def build_default_scope
+      scopes.build(environment_scope: '*', active: self.active)
+    end
+
+    def update_default_scope
+      default_scope.update(active: self.active) if self.active_changed?
+    end
   end
 end

ee/app/models/operations/feature_flag_scope.rb

@@ -13,7 +13,24 @@ module Operations
       message: "(%{value}) has already been taken"
     }
 
+    validates :environment_scope,
+      if: :default_scope?, on: :update,
+      inclusion: { in: %w(*), message: 'cannot be changed from default scope' }
+
+    before_destroy :prevent_destroy_default_scope, if: :default_scope?
+
+    scope :ordered, -> { order(:id) }
     scope :enabled, -> { where(active: true) }
     scope :disabled, -> { where(active: false) }
+
+    private
+
+    def default_scope?
+      environment_scope_was == '*'
+    end
+
+    def prevent_destroy_default_scope
+      raise ActiveRecord::ReadOnlyRecord, "default scope cannot be destroyed"
+    end
   end
 end

ee/app/serializers/feature_flag_entity.rb

@@ -18,6 +18,10 @@ class FeatureFlagEntity < Grape::Entity
     project_feature_flag_path(feature_flag.project, feature_flag)
   end
 
+  expose :scopes, with: FeatureFlagScopeEntity do |feature_flag|
+    feature_flag.scopes.sort_by(&:id)
+  end
+
   private
 
   def can_update?(feature_flag)

ee/app/serializers/feature_flag_scope_entity.rb

+# frozen_string_literal: true
+
+class FeatureFlagScopeEntity < Grape::Entity
+  include RequestAwareEntity
+
+  expose :id
+  expose :active
+  expose :environment_scope
+  expose :created_at
+  expose :updated_at
+end

ee/app/serializers/feature_flag_serializer.rb

@@ -3,4 +3,12 @@
 class FeatureFlagSerializer < BaseSerializer
   include WithPagination
   entity FeatureFlagEntity
+
+  def represent(resource, opts = {})
+    if resource.is_a?(ActiveRecord::Relation)
+      resource = resource.preload_relations
+    end
+
+    super(resource, opts)
+  end
 end

ee/changelogs/unreleased/support-curd-operation-for-flag-scopes.yml

+---
+title: Support CURD operation for feature flag scopes
+merge_request: 9182
+author:
+type: added

ee/db/migrate/20190111183834_create_default_scope_to_feature_flags.rb

+# frozen_string_literal: true
+
+class CreateDefaultScopeToFeatureFlags < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    execute <<~SQL
+      INSERT INTO operations_feature_flag_scopes (feature_flag_id, environment_scope, active, created_at, updated_at)
+      SELECT id, '*', active, created_at, updated_at
+      FROM operations_feature_flags
+      WHERE NOT EXISTS (
+        SELECT 1
+        FROM operations_feature_flag_scopes
+        WHERE operations_feature_flags.id = operations_feature_flag_scopes.feature_flag_id AND
+          environment_scope = '*'
+      );
+    SQL
+  end
+
+  def down
+    execute <<~SQL
+      DELETE FROM operations_feature_flag_scopes WHERE environment_scope = '*';
+    SQL
+  end
+end

ee/spec/finders/feature_flags_finder_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe FeatureFlagsFinder do
+  include FeatureFlagHelpers
+
   let(:finder) { described_class.new(project, user, params) }
   let(:project) { create(:project) }
   let(:user) { developer }
@@ -55,5 +57,26 @@ describe FeatureFlagsFinder do
         end
       end
     end
+
+    context 'when it is presented for list' do
+      let!(:feature_flag_1) { create(:operations_feature_flag, project: project, active: false) }
+      let!(:feature_flag_2) { create(:operations_feature_flag, project: project, active: false) }
+
+      context 'when there is an active scope' do
+        before do
+          create_scope(feature_flag_1, 'review/*', true)
+        end
+
+        it 'presents a virtual active value' do
+          expect(subject.map(&:active)).to eq([true, false])
+        end
+      end
+
+      context 'when there are no active scopes' do
+        it 'presents a virtual active value' do
+          expect(subject.map(&:active)).to eq([false, false])
+        end
+      end
+    end
   end
 end

ee/spec/fixtures/api/schemas/feature_flag.json

@@ -12,7 +12,8 @@
     "active": { "type": "boolean" },
     "description": { "type": ["string", "null"] },
     "edit_path": { "type": ["string", "null"] },
-    "destroy_path": { "type": ["string", "null"] }
+    "destroy_path": { "type": ["string", "null"] },
+    "scopes": { "type": "array", "items": { "$ref": "feature_flag_scope.json" } }
   },
   "additionalProperties": false
 }

ee/spec/fixtures/api/schemas/feature_flag_scope.json

+{
+  "type": "object",
+  "required" : [
+    "id",
+    "environment_scope",
+    "active"
+  ],
+  "properties" : {
+    "id": { "type": "integer" },
+    "environment_scope": { "type": "string" },
+    "active": { "type": "boolean" },
+    "created_at": { "type": "date" },
+    "updated_at": { "type": "date" }
+  },
+  "additionalProperties": false
+}

ee/spec/migrations/create_default_scope_to_feature_flags_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'migrate', '20190111183834_create_default_scope_to_feature_flags.rb')
+
+describe CreateDefaultScopeToFeatureFlags, :migration do
+  let(:namespaces) { table(:namespaces) }
+  let(:namespace) { namespaces.create(name: 'test', path: 'test') }
+  let(:projects) { table(:projects) }
+  let(:project) { projects.create(name: 'test', namespace_id: namespace.id) }
+  let(:feature_flags) { table(:operations_feature_flags) }
+  let(:feature_flag_scopes) { table(:operations_feature_flag_scopes) }
+
+  let!(:feature_flag_1) do
+    feature_flags.create!(
+      project_id: project.id,
+      name: 'ci_live_trace',
+      active: true,
+      description: 'For live trace',
+      created_at: "'2017-10-17 20:24:02'",
+      updated_at: "'2017-10-17 20:24:02'"
+    )
+  end
+
+  let!(:feature_flag_2) do
+    feature_flags.create!(
+      project_id: project.id,
+      name: 'ci_merge_request',
+      active: false,
+      description: 'For merge request',
+      created_at: "'2017-10-17 20:24:02'",
+      updated_at: "'2017-10-17 20:24:02'"
+    )
+  end
+
+  describe '#up' do
+    subject { described_class.new.up }
+
+    let(:scope_1) do
+      feature_flag_scopes.find_by_feature_flag_id(feature_flag_1.id)
+    end
+
+    let(:scope_2) do
+      feature_flag_scopes.find_by_feature_flag_id(feature_flag_2.id)
+    end
+
+    it 'creates default scopes for existing rows' do
+      subject
+
+      expect(scope_1).to be_active
+      expect(scope_1.environment_scope).to eq('*')
+      expect(scope_2).not_to be_active
+      expect(scope_2.environment_scope).to eq('*')
+    end
+
+    context 'when a feature flag has already had default scope' do
+      let!(:feature_flag_scope_1) do
+        feature_flag_scopes.create!(
+          feature_flag_id: feature_flag_1.id,
+          environment_scope: '*',
+          active: true,
+          created_at: "'2017-10-17 20:24:02'",
+          updated_at: "'2017-10-17 20:24:02'"
+        )
+      end
+
+      it 'does not raise an error' do
+        expect { subject }.not_to raise_error
+      end
+
+      it 'creates default scopes for a feature flag' do
+        subject
+
+        expect(scope_2).not_to be_active
+        expect(scope_2.environment_scope).to eq('*')
+      end
+    end
+
+    context 'when there are no feature flags' do
+      let!(:feature_flag_1) { }
+      let!(:feature_flag_2) { }
+
+      it 'does not create scopes' do
+        expect { subject }.not_to change { feature_flag_scopes.count }
+      end
+    end
+  end
+
+  describe '#down' do
+    subject { described_class.new.down }
+
+    let!(:feature_flag_scope_1) do
+      feature_flag_scopes.create!(
+        feature_flag_id: feature_flag_1.id,
+        environment_scope: '*',
+        active: true,
+        created_at: "'2017-10-17 20:24:02'",
+        updated_at: "'2017-10-17 20:24:02'"
+      )
+    end
+
+    it 'deletes default scopes' do
+      expect { subject }.to change { feature_flag_scopes.count }.by(-1)
+    end
+
+    it 'does not affect feature flags' do
+      expect { subject }.not_to change { feature_flags.count }
+    end
+  end
+end

ee/spec/models/operations/feature_flag_scope_spec.rb

@@ -27,6 +27,27 @@ describe Operations::FeatureFlagScope do
                       " has already been taken")
       end
     end
+
+    context 'when environment scope of a default scope is updated' do
+      let!(:feature_flag) { create(:operations_feature_flag) }
+      let!(:default_scope) { feature_flag.default_scope }
+
+      it 'keeps default scope intact' do
+        default_scope.update(environment_scope: 'review/*')
+
+        expect(default_scope.errors[:environment_scope])
+          .to include("cannot be changed from default scope")
+      end
+    end
+
+    context 'when a default scope is destroyed' do
+      let!(:feature_flag) { create(:operations_feature_flag) }
+      let!(:default_scope) { feature_flag.default_scope }
+
+      it 'prevents from destroying the default scope' do
+        expect { default_scope.destroy! }.to raise_error(ActiveRecord::ReadOnlyRecord)
+      end
+    end
   end
 
   describe '.enabled' do
@@ -40,7 +61,7 @@ describe Operations::FeatureFlagScope do
       let(:active) { true }
 
       it 'returns the scope' do
-        is_expected.to eq([feature_flag_scope])
+        is_expected.to include(feature_flag_scope)
       end
     end
 
@@ -48,7 +69,7 @@ describe Operations::FeatureFlagScope do
       let(:active) { false }
 
       it 'returns an empty array' do
-        is_expected.to be_empty
+        is_expected.not_to include(feature_flag_scope)
       end
     end
   end
@@ -64,7 +85,7 @@ describe Operations::FeatureFlagScope do
       let(:active) { true }
 
       it 'returns an empty array' do
-        is_expected.to be_empty
+        is_expected.not_to include(feature_flag_scope)
       end
     end
 
@@ -72,7 +93,7 @@ describe Operations::FeatureFlagScope do
       let(:active) { false }
 
       it 'returns the scope' do
-        is_expected.to eq([feature_flag_scope])
+        is_expected.to include(feature_flag_scope)
       end
     end
   end

ee/spec/models/operations/feature_flag_spec.rb

@@ -16,6 +16,46 @@ describe Operations::FeatureFlag do
     it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) }
   end
 
+  describe '.enabled' do
+    subject { described_class.enabled }
+
+    context 'when the feature flag has an active scope' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: true) }
+
+      it 'returns the flag' do
+        is_expected.to eq([feature_flag])
+      end
+    end
+
+    context 'when the feature flag does not have an active scope' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: false) }
+
+      it 'does not return the flag' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
+  describe '.disabled' do
+    subject { described_class.disabled }
+
+    context 'when the feature flag has an active scope' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: true) }
+
+      it 'does not return the flag' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when the feature flag does not have an active scope' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: false) }
+
+      it 'returns the flag' do
+        is_expected.to eq([feature_flag])
+      end
+    end
+  end
+
   describe '.for_environment' do
     subject { described_class.for_environment(environment_name) }
 
@@ -25,8 +65,7 @@ describe Operations::FeatureFlag do
 
     context 'when feature flag is off on production' do
       before do
-        feature_flag = create(:operations_feature_flag)
-        create_scope(feature_flag, '*', true)
+        feature_flag = create(:operations_feature_flag, active: true)
         create_scope(feature_flag, 'production', false)
       end
 
@@ -49,8 +88,7 @@ describe Operations::FeatureFlag do
 
     context 'when feature flag is default disabled but enabled for review apps' do
       before do
-        feature_flag = create(:operations_feature_flag)
-        create_scope(feature_flag, '*', false)
+        feature_flag = create(:operations_feature_flag, active: false)
         create_scope(feature_flag, 'review/*', true)
       end
 
@@ -72,13 +110,11 @@ describe Operations::FeatureFlag do
     end
 
     context 'when there are two flags' do
-      let!(:feature_flag_1) { create(:operations_feature_flag) }
-      let!(:feature_flag_2) { create(:operations_feature_flag) }
+      let!(:feature_flag_1) { create(:operations_feature_flag, active: true) }
+      let!(:feature_flag_2) { create(:operations_feature_flag, active: true) }
 
       before do
-        create_scope(feature_flag_1, '*', true)
         create_scope(feature_flag_1, 'production', false)
-        create_scope(feature_flag_2, '*', true)
       end
 
       context 'when environment is production' do
@@ -90,4 +126,39 @@ describe Operations::FeatureFlag do
       end
     end
   end
+
+  describe '.for_list' do
+    subject { described_class.for_list }
+
+    before do
+      stub_feature_flags(feature_flags_environment_scope: true)
+    end
+
+    context 'when all scopes are active' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: true) }
+      let!(:scope) { create_scope(feature_flag, 'production', true) }
+
+      it 'returns virtual active value' do
+        expect(subject.first.active).to be_truthy
+      end
+    end
+
+    context 'when all scopes are inactive' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: false) }
+      let!(:scope) { create_scope(feature_flag, 'production', false) }
+
+      it 'returns virtual active value' do
+        expect(subject.first.active).to be_falsy
+      end
+    end
+
+    context 'when one scopes is active' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: false) }
+      let!(:scope) { create_scope(feature_flag, 'production', true) }
+
+      it 'returns virtual active value' do
+        expect(subject.first.active).to be_truthy
+      end
+    end
+  end
 end

ee/spec/requests/api/unleash_spec.rb

@@ -74,18 +74,16 @@ describe API::Unleash do
     let(:headers) { base_headers.merge({ "UNLEASH-APPNAME" => "test" }) }
 
     let!(:feature_flag_1) do
-      create(:operations_feature_flag, project: project)
+      create(:operations_feature_flag, project: project, active: true)
     end
 
     let!(:feature_flag_2) do
-      create(:operations_feature_flag, project: project)
+      create(:operations_feature_flag, project: project, active: false)
     end
 
     before do
       stub_feature_flags(feature_flags_environment_scope: true)
-      create_scope(feature_flag_1, '*', true)
       create_scope(feature_flag_1, 'production', false)
-      create_scope(feature_flag_2, '*', false)
       create_scope(feature_flag_2, 'review/*', true)
     end