Remove Legacy Flag from Feature Flag Enum

The beginning of deleting all the legacy feature flags code involves
removing the enum value for legacy flags. Some changes need to be made
to keep the checks (basically checking that everything is always a
new_version_flag).

app/controllers/projects/feature_flags_controller.rb

@@ -10,9 +10,6 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
 
   before_action :feature_flag, only: [:edit, :update, :destroy]
 
-  before_action :ensure_flag_writable!, only: [:update]
-  before_action :exclude_legacy_flags_check, only: [:edit]
-
   feature_category :feature_flags
 
   def index
@@ -98,18 +95,6 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
     @feature_flag ||= @noteable = project.operations_feature_flags.find_by_iid!(params[:iid])
   end
 
-  def ensure_flag_writable!
-    if feature_flag.legacy_flag?
-      render_error_json(['Legacy feature flags are read-only'])
-    end
-  end
-
-  def exclude_legacy_flags_check
-    if feature_flag.legacy_flag?
-      not_found
-    end
-  end
-
   def create_params
     params.require(:operations_feature_flag)
       .permit(:name, :description, :active, :version,

app/models/operations/feature_flag.rb

@@ -17,6 +17,7 @@ module Operations
     has_internal_id :iid, scope: :project
 
     default_value_for :active, true
+    default_value_for :version, :new_version_flag
 
     # scopes exists only for the first version
     has_many :scopes, class_name: 'Operations::FeatureFlagScope'
@@ -39,8 +40,6 @@ module Operations
     validate :first_default_scope, on: :create, if: :has_scopes?
     validate :version_associations
 
-    before_create :build_default_scope, if: -> { legacy_flag? && scopes.none? }
-
     accepts_nested_attributes_for :scopes, allow_destroy: true
     accepts_nested_attributes_for :strategies, allow_destroy: true
 
@@ -52,7 +51,6 @@ module Operations
     scope :new_version_only, -> { where(version: :new_version_flag)}
 
     enum version: {
-      legacy_flag: 1,
       new_version_flag: 2
     }
 
@@ -127,8 +125,6 @@ module Operations
     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
 

app/models/operations/feature_flag_scope.rb

 # frozen_string_literal: true
 
+# All of the legacy flags have been removed in 14.1, including all of the
+# `operations_feature_flag_scopes` rows.  Therefore, this model and the database
+# table are unused and should be removed.
+
 module Operations
   class FeatureFlagScope < ApplicationRecord
     prepend HasEnvironmentScope

lib/api/feature_flags.rb

@@ -118,7 +118,7 @@ module API
         put do
           authorize_update_feature_flag!
           exclude_legacy_flags_check!
-          render_api_error!('PUT operations are not supported for legacy feature flags', :unprocessable_entity) if feature_flag.legacy_flag?
+          render_api_error!('PUT operations are not supported for legacy feature flags', :unprocessable_entity) unless feature_flag.new_version_flag?
 
           attrs = declared_params(include_missing: false)
 
@@ -207,7 +207,7 @@ module API
       end
 
       def exclude_legacy_flags_check!
-        if feature_flag.legacy_flag?
+        unless feature_flag.new_version_flag?
           not_found!
         end
       end

spec/factories/operations/feature_flag_scopes.rb

-# frozen_string_literal: true
-
-FactoryBot.define do
-  factory :operations_feature_flag_scope, class: 'Operations::FeatureFlagScope' do
-    association :feature_flag, factory: [:operations_feature_flag, :legacy_flag]
-    active { true }
-    strategies { [{ name: "default", parameters: {} }] }
-    sequence(:environment_scope) { |n| "review/patch-#{n}" }
-  end
-end

spec/factories/operations/feature_flags.rb

@@ -6,13 +6,5 @@ FactoryBot.define do
     project
     active { true }
     version { :new_version_flag }
-
-    trait :legacy_flag do
-      version { Operations::FeatureFlag.versions['legacy_flag'] }
-    end
-
-    trait :new_version_flag do
-      version { Operations::FeatureFlag.versions['new_version_flag'] }
-    end
   end
 end

spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb

@@ -66,20 +66,4 @@ RSpec.describe 'User updates feature flag', :js do
       end
     end
   end
-
-  context 'with a legacy feature flag' do
-    let!(:feature_flag) do
-      create_flag(project, 'ci_live_trace', true,
-                  description: 'For live trace feature',
-                  version: :legacy_flag)
-    end
-
-    let!(:scope) { create_scope(feature_flag, 'review/*', true) }
-
-    it 'shows not found error' do
-      visit(edit_project_feature_flag_path(project, feature_flag))
-
-      expect(page).to have_text 'Page Not Found'
-    end
-  end
 end

spec/finders/feature_flags_finder_spec.rb

@@ -72,13 +72,5 @@ RSpec.describe FeatureFlagsFinder do
         subject
       end
     end
-
-    context 'with a legacy flag' do
-      let!(:feature_flag_3) { create(:operations_feature_flag, :legacy_flag, name: 'flag-c', project: project) }
-
-      it 'returns new flags' do
-        is_expected.to eq([feature_flag_1, feature_flag_2])
-      end
-    end
   end
 end

spec/fixtures/api/schemas/feature_flag.json

 {
   "type": "object",
-  "required" : [
-    "id",
-    "name"
-  ],
-  "properties" : {
+  "required": ["id", "name"],
+  "properties": {
     "id": { "type": "integer" },
     "iid": { "type": ["integer", "null"] },
     "version": { "type": "string" },

spec/models/operations/feature_flag_spec.rb

@@ -49,28 +49,7 @@ RSpec.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 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.new({ name: 'test', project: project, version: 1,
-                                 strategies_attributes: [{ name: 'default', parameters: {} }] })
-
-        expect(feature_flag.valid?).to eq(false)
-        expect(feature_flag.errors.messages).to eq({
-          version_associations: ["version 1 feature flags may not have strategies"]
-        })
-      end
-    end
+    it { is_expected.to define_enum_for(:version).with_values(new_version_flag: 2) }
 
     context 'a version 2 feature flag' do
       it 'is invalid if associated with Operations::FeatureFlagScope models' do
@@ -102,64 +81,9 @@ RSpec.describe Operations::FeatureFlag do
     end
   end
 
-  describe 'feature flag version' do
-    it 'defaults to 1 if unspecified' do
-      project = create(:project)
-
-      feature_flag = described_class.create!(name: 'my_flag', project: project, active: true)
-
-      expect(feature_flag).to be_valid
-      expect(feature_flag.version_before_type_cast).to eq(1)
-    end
-  end
-
-  describe 'Scope creation' do
-    subject { described_class.new(**params) }
-
-    let(:project) { create(:project) }
-
-    let(:params) do
-      { name: 'test', project: project, scopes_attributes: scopes_attributes }
-    end
-
-    let(:scopes_attributes) do
-      [{ environment_scope: '*', active: false },
-       { environment_scope: 'review/*', active: true }]
-    end
-
-    it { is_expected.to be_valid }
-
-    context 'when the first scope is not wildcard' do
-      let(:scopes_attributes) do
-        [{ environment_scope: 'review/*', active: true },
-         { environment_scope: '*', active: false }]
-      end
-
-      it { is_expected.not_to be_valid }
-    end
-  end
-
   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
-        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 })
@@ -180,16 +104,6 @@ RSpec.describe Operations::FeatureFlag do
       end
     end
 
-    context 'when the feature flag is active and all scopes are inactive' do
-      let!(:feature_flag) { create(:operations_feature_flag, :legacy_flag, active: true) }
-
-      it 'returns the flag' do
-        feature_flag.default_scope.update!(active: false)
-
-        is_expected.to eq([feature_flag])
-      end
-    end
-
     context 'when the feature flag is inactive' do
       let!(:feature_flag) { create(:operations_feature_flag, active: false) }
 
@@ -197,16 +111,6 @@ RSpec.describe Operations::FeatureFlag do
         is_expected.to be_empty
       end
     end
-
-    context 'when the feature flag is inactive and all scopes are active' do
-      let!(:feature_flag) { create(:operations_feature_flag, :legacy_flag, active: false) }
-
-      it 'does not return the flag' do
-        feature_flag.default_scope.update!(active: true)
-
-        is_expected.to be_empty
-      end
-    end
   end
 
   describe '.disabled' do
@@ -220,16 +124,6 @@ RSpec.describe Operations::FeatureFlag do
       end
     end
 
-    context 'when the feature flag is active and all scopes are inactive' do
-      let!(:feature_flag) { create(:operations_feature_flag, :legacy_flag, active: true) }
-
-      it 'does not return the flag' do
-        feature_flag.default_scope.update!(active: false)
-
-        is_expected.to be_empty
-      end
-    end
-
     context 'when the feature flag is inactive' do
       let!(:feature_flag) { create(:operations_feature_flag, active: false) }
 
@@ -237,16 +131,6 @@ RSpec.describe Operations::FeatureFlag do
         is_expected.to eq([feature_flag])
       end
     end
-
-    context 'when the feature flag is inactive and all scopes are active' do
-      let!(:feature_flag) { create(:operations_feature_flag, :legacy_flag, active: false) }
-
-      it 'returns the flag' do
-        feature_flag.default_scope.update!(active: true)
-
-        is_expected.to eq([feature_flag])
-      end
-    end
   end
 
   describe '.for_unleash_client' do

spec/requests/api/feature_flags_spec.rb

@@ -116,21 +116,6 @@ RSpec.describe API::FeatureFlags do
         }])
       end
     end
-
-    context 'with version 1 and 2 feature flags' do
-      it 'returns both versions of flags ordered by name' do
-        create(:operations_feature_flag, project: project, name: 'legacy_flag')
-        feature_flag = create(:operations_feature_flag, :new_version_flag, project: project, name: 'new_version_flag')
-        strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
-        create(:operations_scope, strategy: strategy, environment_scope: 'production')
-
-        subject
-
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(response).to match_response_schema('public_api/v4/feature_flags')
-        expect(json_response.map { |f| f['name'] }).to eq(%w[legacy_flag new_version_flag])
-      end
-    end
   end
 
   describe 'GET /projects/:id/feature_flags/:name' do
@@ -185,22 +170,13 @@ RSpec.describe API::FeatureFlags do
   end
 
   describe 'POST /projects/:id/feature_flags' do
-    def scope_default
-      {
-        environment_scope: '*',
-        active: false,
-        strategies: [{ name: 'default', parameters: {} }].to_json
-      }
-    end
-
     subject do
       post api("/projects/#{project.id}/feature_flags", user), params: params
     end
 
     let(:params) do
       {
-        name: 'awesome-feature',
-        scopes: [scope_default]
+        name: 'awesome-feature'
       }
     end
 
@@ -215,14 +191,14 @@ RSpec.describe API::FeatureFlags do
       expect(feature_flag.description).to eq(params[:description])
     end
 
-    it 'defaults to a version 1 (legacy) feature flag' do
+    it 'defaults to a version 2 (new) feature flag' do
       subject
 
       expect(response).to have_gitlab_http_status(:created)
       expect(response).to match_response_schema('public_api/v4/feature_flag')
 
       feature_flag = project.operations_feature_flags.last
-      expect(feature_flag.version).to eq('legacy_flag')
+      expect(feature_flag.version).to eq('new_version_flag')
     end
 
     it_behaves_like 'check user permission'
@@ -232,38 +208,7 @@ RSpec.describe API::FeatureFlags do
 
       expect(response).to have_gitlab_http_status(:created)
       expect(response).to match_response_schema('public_api/v4/feature_flag')
-      expect(json_response['version']).to eq('legacy_flag')
-    end
-
-    context 'with active set to false in the params for a legacy flag' do
-      let(:params) do
-        {
-          name: 'awesome-feature',
-          version: 'legacy_flag',
-          active: 'false',
-          scopes: [scope_default]
-        }
-      end
-
-      it 'creates an inactive feature flag' do
-        subject
-
-        expect(response).to have_gitlab_http_status(:created)
-        expect(response).to match_response_schema('public_api/v4/feature_flag')
-        expect(json_response['active']).to eq(false)
-      end
-    end
-
-    context 'when no scopes passed in parameters' do
-      let(:params) { { name: 'awesome-feature' } }
-
-      it 'creates a new feature flag with active default scope' do
-        subject
-
-        expect(response).to have_gitlab_http_status(:created)
-        feature_flag = project.operations_feature_flags.last
-        expect(feature_flag.default_scope).to be_active
-      end
+      expect(json_response['version']).to eq('new_version_flag')
     end
 
     context 'when there is a feature flag with the same name already' do
@@ -278,43 +223,6 @@ RSpec.describe API::FeatureFlags do
       end
     end
 
-    context 'when create a feature flag with two scopes' do
-      let(:params) do
-        {
-          name: 'awesome-feature',
-          description: 'this is awesome',
-          scopes: [
-            scope_default,
-            scope_with_user_with_id
-          ]
-        }
-      end
-
-      let(:scope_with_user_with_id) do
-        {
-          environment_scope: 'production',
-          active: true,
-          strategies: [{
-            name: 'userWithId',
-            parameters: { userIds: 'user:1' }
-          }].to_json
-        }
-      end
-
-      it 'creates a new feature flag with two scopes' do
-        subject
-
-        expect(response).to have_gitlab_http_status(:created)
-
-        feature_flag = project.operations_feature_flags.last
-        feature_flag.scopes.ordered.each_with_index do |scope, index|
-          expect(scope.environment_scope).to eq(params[:scopes][index][:environment_scope])
-          expect(scope.active).to eq(params[:scopes][index][:active])
-          expect(scope.strategies).to eq(Gitlab::Json.parse(params[:scopes][index][:strategies]))
-        end
-      end
-    end
-
     context 'when creating a version 2 feature flag' do
       it 'creates a new feature flag' do
         params = {
@@ -455,23 +363,6 @@ RSpec.describe API::FeatureFlags do
   end
 
   describe 'PUT /projects/:id/feature_flags/:name' do
-    context 'with a legacy feature flag' do
-      let!(:feature_flag) do
-        create(:operations_feature_flag, :legacy_flag, project: project,
-               name: 'feature1', description: 'old description')
-      end
-
-      it 'returns a 404' do
-        params = { description: 'new description' }
-
-        put api("/projects/#{project.id}/feature_flags/feature1", user), params: params
-
-        expect(response).to have_gitlab_http_status(:not_found)
-        expect(json_response).to eq({ 'message' => '404 Not Found' })
-        expect(feature_flag.reload.description).to eq('old description')
-      end
-    end
-
     context 'with a version 2 feature flag' do
       let!(:feature_flag) do
         create(:operations_feature_flag, :new_version_flag, project: project, active: true,
@@ -781,7 +672,7 @@ RSpec.describe API::FeatureFlags do
              params: params
     end
 
-    let!(:feature_flag) { create(:operations_feature_flag, :legacy_flag, project: project) }
+    let!(:feature_flag) { create(:operations_feature_flag, project: project) }
     let(:params) { {} }
 
     it 'destroys the feature flag' do
@@ -794,7 +685,7 @@ RSpec.describe API::FeatureFlags do
       subject
 
       expect(response).to have_gitlab_http_status(:ok)
-      expect(json_response['version']).to eq('legacy_flag')
+      expect(json_response['version']).to eq('new_version_flag')
     end
 
     context 'with a version 2 feature flag' do

spec/requests/api/unleash_spec.rb

@@ -176,25 +176,6 @@ RSpec.describe API::Unleash do
 
       it_behaves_like 'authenticated request'
 
-      context 'with version 1 (legacy) feature flags' do
-        let(:feature_flag) { create(:operations_feature_flag, :legacy_flag, project: project, name: 'feature1', active: true, version: 1) }
-
-        it 'does not return a legacy feature flag' do
-          create(:operations_feature_flag_scope,
-                 feature_flag: feature_flag,
-                 environment_scope: 'sandbox',
-                 active: true,
-                 strategies: [{ name: "gradualRolloutUserId",
-                                parameters: { groupId: "default", percentage: "50" } }])
-          headers = { "UNLEASH-INSTANCEID" => client.token, "UNLEASH-APPNAME" => "sandbox" }
-
-          get api(features_url), headers: headers
-
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response['features']).to be_empty
-        end
-      end
-
       context 'with version 2 feature flags' do
         it 'does not return a flag without any strategies' do
           create(:operations_feature_flag, project: project,