Merge branch 'user-lists-strategies-unleash' into 'master'

Return User List Strategy to Unleash Clients

See merge request gitlab-org/gitlab!30650

ee/app/models/operations/feature_flag.rb

@@ -53,7 +53,7 @@ module Operations
       end
 
       def for_unleash_client(project, environment)
-        includes(strategies: :scopes)
+        includes(strategies: [:scopes, :user_list])
           .where(project: project)
           .merge(Operations::FeatureFlags::Scope.on_environment(environment))
           .reorder(:id)

ee/app/models/operations/feature_flags/strategy.rb

@@ -4,10 +4,12 @@ module Operations
   module FeatureFlags
     class Strategy < ApplicationRecord
       STRATEGY_DEFAULT = 'default'
+      STRATEGY_GITLABUSERLIST = 'gitlabUserList'
       STRATEGY_GRADUALROLLOUTUSERID = 'gradualRolloutUserId'
       STRATEGY_USERWITHID = 'userWithId'
       STRATEGIES = {
         STRATEGY_DEFAULT => [].freeze,
+        STRATEGY_GITLABUSERLIST => [].freeze,
         STRATEGY_GRADUALROLLOUTUSERID => %w[groupId percentage].freeze,
         STRATEGY_USERWITHID => ['userIds'].freeze
       }.freeze
@@ -17,6 +19,8 @@ module Operations
 
       belongs_to :feature_flag
       has_many :scopes, class_name: 'Operations::FeatureFlags::Scope'
+      has_one :strategy_user_list
+      has_one :user_list, through: :strategy_user_list
 
       validates :name,
         inclusion: {
@@ -25,11 +29,20 @@ module Operations
       }
 
       validate :parameters_validations, if: -> { errors[:name].blank? }
+      validates :user_list, presence: true, if: -> { name == STRATEGY_GITLABUSERLIST }
+      validates :user_list, absence: true, if: -> { name != STRATEGY_GITLABUSERLIST }
+      validate :same_project_validation, if: -> { user_list.present? }
 
       accepts_nested_attributes_for :scopes, allow_destroy: true
 
       private
 
+      def same_project_validation
+        unless user_list.project_id == feature_flag.project_id
+          errors.add(:user_list, 'must belong to the same project')
+        end
+      end
+
       def parameters_validations
         validate_parameters_type &&
           validate_parameters_keys &&

ee/app/models/operations/feature_flags/strategy_user_list.rb

+# frozen_string_literal: true
+
+module Operations
+  module FeatureFlags
+    class StrategyUserList < ApplicationRecord
+      self.table_name = 'operations_strategies_user_lists'
+
+      belongs_to :strategy
+      belongs_to :user_list
+    end
+  end
+end

ee/lib/ee/api/entities/unleash_feature.rb

@@ -7,7 +7,27 @@ module EE
         expose :name
         expose :description, unless: ->(feature) { feature.description.nil? }
         expose :active, as: :enabled
-        expose :strategies, using: UnleashStrategy
+        expose :strategies do |flag|
+          flag.strategies.map do |strategy|
+            if legacy_strategy?(strategy)
+              UnleashLegacyStrategy.represent(strategy)
+            elsif gitlab_user_list_strategy?(strategy)
+              UnleashGitlabUserListStrategy.represent(strategy)
+            else
+              UnleashStrategy.represent(strategy)
+            end
+          end
+        end
+
+        private
+
+        def legacy_strategy?(strategy)
+          !strategy.respond_to?(:name)
+        end
+
+        def gitlab_user_list_strategy?(strategy)
+          strategy.name == ::Operations::FeatureFlags::Strategy::STRATEGY_GITLABUSERLIST
+        end
       end
     end
   end

ee/lib/ee/api/entities/unleash_gitlab_user_list_strategy.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Entities
+      class UnleashGitlabUserListStrategy < Grape::Entity
+        expose :name do |_strategy|
+          ::Operations::FeatureFlags::Strategy::STRATEGY_USERWITHID
+        end
+        expose :parameters do |strategy|
+          { userIds: strategy.user_list.user_xids }
+        end
+      end
+    end
+  end
+end

ee/lib/ee/api/entities/unleash_legacy_strategy.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Entities
+      class UnleashLegacyStrategy < Grape::Entity
+        expose :name do |strategy|
+          strategy['name']
+        end
+        expose :parameters do |strategy|
+          strategy['parameters']
+        end
+      end
+    end
+  end
+end

ee/lib/ee/api/entities/unleash_strategy.rb

@@ -4,20 +4,8 @@ module EE
   module API
     module Entities
       class UnleashStrategy < Grape::Entity
-        expose :name do |strategy|
-          if strategy.respond_to?(:name)
-            strategy.name
-          else
-            strategy['name']
-          end
-        end
-        expose :parameters do |strategy|
-          if strategy.respond_to?(:parameters)
-            strategy.parameters
-          else
-            strategy['parameters']
-          end
-        end
+        expose :name
+        expose :parameters
       end
     end
   end

ee/spec/models/operations/feature_flags/strategy_spec.rb

@@ -3,10 +3,12 @@
 require 'spec_helper'
 
 describe Operations::FeatureFlags::Strategy do
+  let_it_be(:project) { create(:project) }
+
   describe 'validations' do
     it do
       is_expected.to validate_inclusion_of(:name)
-        .in_array(%w[default gradualRolloutUserId userWithId])
+        .in_array(%w[default gradualRolloutUserId userWithId gitlabUserList])
         .with_message('strategy name is invalid')
     end
 
@@ -17,7 +19,7 @@ describe Operations::FeatureFlags::Strategy do
         end
         with_them do
           it 'skips parameters validation' do
-            feature_flag = create(:operations_feature_flag)
+            feature_flag = create(:operations_feature_flag, project: project)
             strategy = described_class.create(feature_flag: feature_flag,
                                               name: invalid_name, parameters: { bad: 'params' })
 
@@ -34,7 +36,7 @@ describe Operations::FeatureFlags::Strategy do
         end
         with_them do
           it 'must have valid parameters for the strategy' do
-            feature_flag = create(:operations_feature_flag)
+            feature_flag = create(:operations_feature_flag, project: project)
             strategy = described_class.create(feature_flag: feature_flag,
                                               name: 'gradualRolloutUserId', parameters: invalid_parameters)
 
@@ -43,7 +45,7 @@ describe Operations::FeatureFlags::Strategy do
         end
 
         it 'allows the parameters in any order' do
-          feature_flag = create(:operations_feature_flag)
+          feature_flag = create(:operations_feature_flag, project: project)
           strategy = described_class.create(feature_flag: feature_flag,
                                             name: 'gradualRolloutUserId',
                                             parameters: { percentage: '10', groupId: 'mygroup' })
@@ -59,7 +61,7 @@ describe Operations::FeatureFlags::Strategy do
           end
           with_them do
             it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do
-              feature_flag = create(:operations_feature_flag)
+              feature_flag = create(:operations_feature_flag, project: project)
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'gradualRolloutUserId',
                                                 parameters: { groupId: 'mygroup', percentage: invalid_value })
@@ -73,7 +75,7 @@ describe Operations::FeatureFlags::Strategy do
           end
           with_them do
             it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do
-              feature_flag = create(:operations_feature_flag)
+              feature_flag = create(:operations_feature_flag, project: project)
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'gradualRolloutUserId',
                                                 parameters: { groupId: 'mygroup', percentage: valid_value })
@@ -90,7 +92,7 @@ describe Operations::FeatureFlags::Strategy do
           end
           with_them do
             it 'must be a string value of up to 32 lowercase characters' do
-              feature_flag = create(:operations_feature_flag)
+              feature_flag = create(:operations_feature_flag, project: project)
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'gradualRolloutUserId',
                                                 parameters: { groupId: invalid_value, percentage: '40' })
@@ -104,7 +106,7 @@ describe Operations::FeatureFlags::Strategy do
           end
           with_them do
             it 'must be a string value of up to 32 lowercase characters' do
-              feature_flag = create(:operations_feature_flag)
+              feature_flag = create(:operations_feature_flag, project: project)
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'gradualRolloutUserId',
                                                 parameters: { groupId: valid_value, percentage: '40' })
@@ -121,7 +123,7 @@ describe Operations::FeatureFlags::Strategy do
         end
         with_them do
           it 'must have valid parameters for the strategy' do
-            feature_flag = create(:operations_feature_flag)
+            feature_flag = create(:operations_feature_flag, project: project)
             strategy = described_class.create(feature_flag: feature_flag,
                                               name: 'userWithId', parameters: invalid_parameters)
 
@@ -138,7 +140,7 @@ describe Operations::FeatureFlags::Strategy do
           end
           with_them do
             it 'is valid with a string of comma separated values' do
-              feature_flag = create(:operations_feature_flag)
+              feature_flag = create(:operations_feature_flag, project: project)
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'userWithId', parameters: { userIds: valid_value })
 
@@ -153,7 +155,7 @@ describe Operations::FeatureFlags::Strategy do
           end
           with_them do
             it 'is invalid' do
-              feature_flag = create(:operations_feature_flag)
+              feature_flag = create(:operations_feature_flag, project: project)
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'userWithId', parameters: { userIds: invalid_value })
 
@@ -171,7 +173,7 @@ describe Operations::FeatureFlags::Strategy do
         end
         with_them do
           it 'must be empty' do
-            feature_flag = create(:operations_feature_flag)
+            feature_flag = create(:operations_feature_flag, project: project)
             strategy = described_class.create(feature_flag: feature_flag,
                                               name: 'default',
                                               parameters: invalid_value)
@@ -181,7 +183,7 @@ describe Operations::FeatureFlags::Strategy do
         end
 
         it 'must be empty' do
-          feature_flag = create(:operations_feature_flag)
+          feature_flag = create(:operations_feature_flag, project: project)
           strategy = described_class.create(feature_flag: feature_flag,
                                             name: 'default',
                                             parameters: {})
@@ -189,6 +191,133 @@ describe Operations::FeatureFlags::Strategy do
           expect(strategy.errors[:parameters]).to be_empty
         end
       end
+
+      context 'when the strategy name is gitlabUserList' do
+        where(:invalid_value) do
+          [{ groupId: "default", percentage: "7" }, "", "nothing", 7, nil, [], 2.5, { userIds: 'user1' }]
+        end
+        with_them do
+          it 'must be empty' do
+            feature_flag = create(:operations_feature_flag, project: project)
+            strategy = described_class.create(feature_flag: feature_flag,
+                                              name: 'gitlabUserList',
+                                              parameters: invalid_value)
+
+            expect(strategy.errors[:parameters]).to eq(['parameters are invalid'])
+          end
+        end
+
+        it 'must be empty' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'gitlabUserList',
+                                            parameters: {})
+
+          expect(strategy.errors[:parameters]).to be_empty
+        end
+      end
+    end
+
+    describe 'associations' do
+      context 'when name is gitlabUserList' do
+        it 'is valid when associated with a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          user_list = create(:operations_feature_flag_user_list, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'gitlabUserList',
+                                            user_list: user_list,
+                                            parameters: {})
+
+          expect(strategy.errors[:user_list]).to be_empty
+        end
+
+        it 'is invalid without a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'gitlabUserList',
+                                            parameters: {})
+
+          expect(strategy.errors[:user_list]).to eq(["can't be blank"])
+        end
+
+        it 'is invalid when associated with a user list from another project' do
+          other_project = create(:project)
+          feature_flag = create(:operations_feature_flag, project: project)
+          user_list = create(:operations_feature_flag_user_list, project: other_project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'gitlabUserList',
+                                            user_list: user_list,
+                                            parameters: {})
+
+          expect(strategy.errors[:user_list]).to eq(['must belong to the same project'])
+        end
+      end
+
+      context 'when name is default' do
+        it 'is invalid when associated with a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          user_list = create(:operations_feature_flag_user_list, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'default',
+                                            user_list: user_list,
+                                            parameters: {})
+
+          expect(strategy.errors[:user_list]).to eq(['must be blank'])
+        end
+
+        it 'is valid without a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'default',
+                                            parameters: {})
+
+          expect(strategy.errors[:user_list]).to be_empty
+        end
+      end
+
+      context 'when name is userWithId' do
+        it 'is invalid when associated with a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          user_list = create(:operations_feature_flag_user_list, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'userWithId',
+                                            user_list: user_list,
+                                            parameters: { userIds: 'user1' })
+
+          expect(strategy.errors[:user_list]).to eq(['must be blank'])
+        end
+
+        it 'is valid without a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'userWithId',
+                                            parameters: { userIds: 'user1' })
+
+          expect(strategy.errors[:user_list]).to be_empty
+        end
+      end
+
+      context 'when name is gradualRolloutUserId' do
+        it 'is invalid when associated with a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          user_list = create(:operations_feature_flag_user_list, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'gradualRolloutUserId',
+                                            user_list: user_list,
+                                            parameters: { groupId: 'default', percentage: '10' })
+
+          expect(strategy.errors[:user_list]).to eq(['must be blank'])
+        end
+
+        it 'is valid without a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'gradualRolloutUserId',
+                                            parameters: { groupId: 'default', percentage: '10' })
+
+          expect(strategy.errors[:user_list]).to be_empty
+        end
+      end
     end
   end
 end

ee/spec/requests/api/unleash_spec.rb

@@ -494,6 +494,28 @@ describe API::Unleash do
             }]
           }])
         end
+
+        it 'returns a userWithId strategy for a gitlabUserList strategy' do
+          feature_flag = create(:operations_feature_flag, :new_version_flag, project: project,
+                                name: 'myfeature', active: true)
+          user_list = create(:operations_feature_flag_user_list, project: project,
+                             name: 'My List', user_xids: 'user1,user2')
+          strategy = create(:operations_strategy, feature_flag: feature_flag,
+                            name: 'gitlabUserList', parameters: {}, user_list: user_list)
+          create(:operations_scope, strategy: strategy, environment_scope: 'production')
+
+          get api(features_url), headers: { 'UNLEASH-INSTANCEID' => client.token, 'UNLEASH-APPNAME' => 'production' }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['features']).to eq([{
+            'name' => 'myfeature',
+            'enabled' => true,
+            'strategies' => [{
+              'name' => 'userWithId',
+              'parameters' => { 'userIds' => 'user1,user2' }
+            }]
+          }])
+        end
       end
 
       context 'when mixing version 1 and version 2 feature flags' do