Merge branch 'ff-version-assoc-validations' into 'master'

Validate Feature Flag Associations

See merge request gitlab-org/gitlab!25933

ee/app/models/operations/feature_flag.rb

@@ -12,7 +12,10 @@ module Operations
 
     default_value_for :active, true
 
+    # scopes exists only for the first version
     has_many :scopes, class_name: 'Operations::FeatureFlagScope'
+    # strategies exists only for the second version
+    has_many :strategies, class_name: 'Operations::FeatureFlags::Strategy'
     has_one :default_scope, -> { where(environment_scope: '*') }, class_name: 'Operations::FeatureFlagScope'
 
     validates :project, presence: true
@@ -25,18 +28,24 @@ module Operations
       }
     validates :name, uniqueness: { scope: :project_id }
     validates :description, allow_blank: true, length: 0..255
-    validates :version, inclusion: { in: [1, 2], message: 'must be 1 or 2' }
     validate :first_default_scope, on: :create, if: :has_scopes?
+    validate :version_associations
 
-    before_create :build_default_scope, unless: :has_scopes?
+    before_create :build_default_scope, if: -> { legacy_flag? && scopes.none? }
 
     accepts_nested_attributes_for :scopes, allow_destroy: true
+    accepts_nested_attributes_for :strategies
 
     scope :ordered, -> { order(:name) }
 
     scope :enabled, -> { where(active: true) }
     scope :disabled, -> { where(active: false) }
 
+    enum version: {
+      legacy_flag: 1,
+      new_version_flag: 2
+    }
+
     class << self
       def preload_relations
         preload(:scopes)
@@ -45,6 +54,14 @@ module Operations
 
     private
 
+    def version_associations
+      if new_version_flag? && scopes.any?
+        errors.add(:version_associations, 'version 2 feature flags may not have scopes')
+      elsif legacy_flag? && strategies.any?
+        errors.add(:version_associations, 'version 1 feature flags may not have strategies')
+      end
+    end
+
     def first_default_scope
       unless scopes.first.environment_scope == '*'
         errors.add(:default_scope, 'has to be the first element')

ee/spec/models/operations/feature_flag_spec.rb

@@ -16,7 +16,47 @@ describe Operations::FeatureFlag do
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_presence_of(:name) }
     it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) }
-    it { is_expected.to validate_inclusion_of(:version).in_array([1, 2]).with_message('must be 1 or 2') }
+    it { is_expected.to define_enum_for(:version).with_values(legacy_flag: 1, new_version_flag: 2) }
+
+    context 'a version 1 feature flag' do
+      it 'is valid if associated with Operations::FeatureFlagScope models' do
+        project = create(:project)
+        feature_flag = described_class.create({ name: 'test', project: project, version: 1,
+                                 scopes_attributes: [{ environment_scope: '*', active: false }] })
+
+        expect(feature_flag).to be_valid
+      end
+
+      it 'is invalid if associated with Operations::FeatureFlags::Strategy models' do
+        project = create(:project)
+        feature_flag = described_class.create({ name: 'test', project: project, version: 1,
+                                 strategies_attributes: [{ name: 'default', parameters: {} }] })
+
+        expect(feature_flag.errors.messages).to eq({
+          version_associations: ["version 1 feature flags may not have strategies"]
+        })
+      end
+    end
+
+    context 'a version 2 feature flag' do
+      it 'is invalid if associated with Operations::FeatureFlagScope models' do
+        project = create(:project)
+        feature_flag = described_class.create({ name: 'test', project: project, version: 2,
+                                 scopes_attributes: [{ environment_scope: '*', active: false }] })
+
+        expect(feature_flag.errors.messages).to eq({
+          version_associations: ["version 2 feature flags may not have scopes"]
+        })
+      end
+
+      it 'is valid if associated with Operations::FeatureFlags::Strategy models' do
+        project = create(:project)
+        feature_flag = described_class.create({ name: 'test', project: project, version: 2,
+                                                strategies_attributes: [{ name: 'default', parameters: {} }] })
+
+        expect(feature_flag).to be_valid
+      end
+    end
 
     it_behaves_like 'AtomicInternalId', validate_presence: false do
       let(:internal_id_attribute) { :iid }
@@ -34,7 +74,7 @@ describe Operations::FeatureFlag do
       feature_flag = described_class.create(name: 'my_flag', project: project, active: true)
 
       expect(feature_flag).to be_valid
-      expect(feature_flag.version).to eq(1)
+      expect(feature_flag.version_before_type_cast).to eq(1)
     end
   end
 
@@ -62,14 +102,34 @@ describe Operations::FeatureFlag do
 
       it { is_expected.not_to be_valid }
     end
+  end
 
-    context 'when scope is empty' do
-      let(:scopes_attributes) { [] }
+  describe 'the default scope' do
+    let_it_be(:project) { create(:project) }
 
+    context 'with a version 1 feature flag' do
       it 'creates a default scope' do
-        subject.save
+        feature_flag = described_class.create({ name: 'test', project: project, scopes_attributes: [], version: 1 })
+
+        expect(feature_flag.scopes.count).to eq(1)
+        expect(feature_flag.scopes.first.environment_scope).to eq('*')
+      end
+
+      it 'allows specifying the default scope in the parameters' do
+        feature_flag = described_class.create({ name: 'test', project: project,
+                                                scopes_attributes: [{ environment_scope: '*', active: false },
+                                                                    { environment_scope: 'review/*', active: true }], version: 1 })
+
+        expect(feature_flag.scopes.count).to eq(2)
+        expect(feature_flag.scopes.first.environment_scope).to eq('*')
+      end
+    end
+
+    context 'with a version 2 feature flag' do
+      it 'does not create a default scope' do
+        feature_flag = described_class.create({ name: 'test', project: project, scopes_attributes: [], version: 2 })
 
-        expect(subject.scopes.first.environment_scope).to eq('*')
+        expect(feature_flag.scopes).to eq([])
       end
     end
   end