Merge branch 'afontaine/remove-legacy-flag-enum' into 'master'

Remove Legacy Flag from Feature Flag Enum

See merge request gitlab-org/gitlab!68408

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

app/services/feature_flags/base_service.rb

@@ -48,10 +48,12 @@ module FeatureFlags
       end
     end
 
-    def created_scope_message(scope)
-      "Created rule #{scope.environment_scope} "\
-      "and set it as #{scope.active ? "active" : "inactive"} "\
-      "with strategies #{scope.strategies}."
+    def created_strategy_message(strategy)
+      scopes = strategy.scopes
+                 .map { |scope| %Q("#{scope.environment_scope}") }
+                 .join(', ')
+      %Q(Created strategy \"#{strategy.name}\" )\
+      "with scopes #{scopes}."
     end
 
     def feature_flag_by_name

app/services/feature_flags/create_service.rb

@@ -24,8 +24,8 @@ module FeatureFlags
     def audit_message(feature_flag)
       message_parts = ["Created feature flag #{feature_flag.name} with description \"#{feature_flag.description}\"."]
 
-      message_parts += feature_flag.scopes.map do |scope|
-        created_scope_message(scope)
+      message_parts += feature_flag.strategies.map do |strategy|
+        created_strategy_message(strategy)
       end
 
       message_parts.join(" ")

app/services/feature_flags/update_service.rb

@@ -2,10 +2,9 @@
 
 module FeatureFlags
   class UpdateService < FeatureFlags::BaseService
-    AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES = {
-      'active' => 'active state',
-      'environment_scope' => 'environment scope',
-      'strategies' => 'strategies'
+    AUDITABLE_STRATEGY_ATTRIBUTES_HUMAN_NAMES = {
+      'scopes' => 'environment scopes',
+      'parameters' => 'parameters'
     }.freeze
 
     def execute(feature_flag)
@@ -41,7 +40,7 @@ module FeatureFlags
 
     def audit_message(feature_flag)
       changes = changed_attributes_messages(feature_flag)
-      changes += changed_scopes_messages(feature_flag)
+      changes += changed_strategies_messages(feature_flag)
 
       return if changes.empty?
 
@@ -56,29 +55,30 @@ module FeatureFlags
       end
     end
 
-    def changed_scopes_messages(feature_flag)
-      feature_flag.scopes.map do |scope|
-        if scope.new_record?
-          created_scope_message(scope)
-        elsif scope.marked_for_destruction?
-          deleted_scope_message(scope)
+    def changed_strategies_messages(feature_flag)
+      feature_flag.strategies.map do |strategy|
+        if strategy.new_record?
+          created_strategy_message(strategy)
+        elsif strategy.marked_for_destruction?
+          deleted_strategy_message(strategy)
         else
-          updated_scope_message(scope)
+          updated_strategy_message(strategy)
         end
-      end.compact # updated_scope_message can return nil if nothing has been changed
+      end.compact # updated_strategy_message can return nil if nothing has been changed
     end
 
-    def deleted_scope_message(scope)
-      "Deleted rule #{scope.environment_scope}."
+    def deleted_strategy_message(strategy)
+      scopes = strategy.scopes.map { |scope| scope.environment_scope }.join(', ')
+      "Deleted strategy #{strategy.name} with environment scopes #{scopes}."
     end
 
-    def updated_scope_message(scope)
-      changes = scope.changes.slice(*AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES.keys)
+    def updated_strategy_message(strategy)
+      changes = strategy.changes.slice(*AUDITABLE_STRATEGY_ATTRIBUTES_HUMAN_NAMES.keys)
       return if changes.empty?
 
-      message = "Updated rule #{scope.environment_scope} "
+      message = "Updated strategy #{strategy.name} "
       message += changes.map do |attribute_name, change|
-        name = AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES[attribute_name]
+        name = AUDITABLE_STRATEGY_ATTRIBUTES_HUMAN_NAMES[attribute_name]
         "#{name} from #{change.first} to #{change.second}"
       end.join(' ')
 

doc/administration/audit_events.md

@@ -126,6 +126,7 @@ From there, you can see the following actions:
 - User password required for approvals was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/336211) in GitLab 14.2)
 - Permission to modify merge requests approval rules in merge requests was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/336211) in GitLab 14.2)
 - New approvals requirement when new commits are added to an MR was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/336211) in GitLab 14.2)
+- When [strategies for feature flags](../operations/feature_flags.md#feature-flag-strategies) are changed ([introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68408) in GitLab 14.3)
 
 Project events can also be accessed via the [Project Audit Events API](../api/audit_events.md#project-audit-events).
 

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,

spec/services/feature_flags/create_service_spec.rb

@@ -48,8 +48,9 @@ RSpec.describe FeatureFlags::CreateService do
         {
           name: 'feature_flag',
           description: 'description',
-          scopes_attributes: [{ environment_scope: '*', active: true },
-                              { environment_scope: 'production', active: false }]
+          version: 'new_version_flag',
+          strategies_attributes: [{ name: 'default', scopes_attributes: [{ environment_scope: '*' }], parameters: {} },
+                                  { name: 'default', parameters: {}, scopes_attributes: [{ environment_scope: 'production' }] }]
         }
       end
 
@@ -70,10 +71,10 @@ RSpec.describe FeatureFlags::CreateService do
       it 'creates audit event' do
         expected_message = 'Created feature flag feature_flag '\
                            'with description "description". '\
-                           'Created rule * and set it as active '\
-                           'with strategies [{"name"=>"default", "parameters"=>{}}]. '\
-                           'Created rule production and set it as inactive '\
-                           'with strategies [{"name"=>"default", "parameters"=>{}}].'
+                           'Created strategy "default" with scopes '\
+                           '"*". '\
+                           'Created strategy "default" with scopes '\
+                           '"production".'
 
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(AuditEvent.last.details[:custom_message]).to eq(expected_message)