Merge branch 'afontaine/flexible-rollout-stickiness' into 'master'

Fix Stickiness to Match Unleash API

See merge request gitlab-org/gitlab!66676

app/assets/javascripts/feature_flags/components/strategies/flexible_rollout.vue

@@ -25,19 +25,19 @@ export default {
   },
   stickinessOptions: [
     {
-      value: 'DEFAULT',
+      value: 'default',
       text: __('Available ID'),
     },
     {
-      value: 'USERID',
+      value: 'userId',
       text: __('User ID'),
     },
     {
-      value: 'SESSIONID',
+      value: 'sessionId',
       text: __('Session ID'),
     },
     {
-      value: 'RANDOM',
+      value: 'random',
       text: __('Random'),
     },
   ],

app/models/operations/feature_flags/strategy.rb

@@ -16,7 +16,7 @@ module Operations
         STRATEGY_USERWITHID => ['userIds'].freeze
       }.freeze
       USERID_MAX_LENGTH = 256
-      STICKINESS_SETTINGS = %w[DEFAULT USERID SESSIONID RANDOM].freeze
+      STICKINESS_SETTINGS = %w[default userId sessionId random].freeze
 
       self.table_name = 'operations_strategies'
 

db/migrate/20210722150102_operations_feature_flags_correct_flexible_rollout_values.rb

+# frozen_string_literal: true
+
+class OperationsFeatureFlagsCorrectFlexibleRolloutValues < ActiveRecord::Migration[6.1]
+  STICKINESS = { "USERID" => "userId", "RANDOM" => "random", "SESSIONID" => "sessionId", "DEFAULT" => "default" }.freeze
+
+  def up
+    STICKINESS.each do |before, after|
+      update_statement = <<-SQL
+        UPDATE operations_strategies
+        SET parameters = parameters || jsonb_build_object('stickiness', '#{quote_string(after)}')
+        WHERE name = 'flexibleRollout' AND parameters->>'stickiness' = '#{quote_string(before)}'
+      SQL
+
+      execute(update_statement)
+    end
+  end
+
+  def down
+    STICKINESS.each do |before, after|
+      update_statement = <<-SQL
+        UPDATE operations_strategies
+        SET parameters = parameters || jsonb_build_object('stickiness', '#{quote_string(before)}')
+        WHERE name = 'flexibleRollout' AND parameters->>'stickiness' = '#{quote_string(after)}'
+      SQL
+
+      execute(update_statement)
+    end
+  end
+end

db/schema_migrations/20210722150102

+cea8e51f6917be9ad43280fba9f8e7d9b9db1f508e249d9f5df792e43c0b8313
\ No newline at end of file

spec/controllers/projects/feature_flags_controller_spec.rb

@@ -652,7 +652,7 @@ RSpec.describe Projects::FeatureFlagsController do
             version: 'new_version_flag',
             strategies_attributes: [{
               name: 'flexibleRollout',
-              parameters: { groupId: 'default', rollout: '15', stickiness: 'DEFAULT' },
+              parameters: { groupId: 'default', rollout: '15', stickiness: 'default' },
               scopes_attributes: [{ environment_scope: 'production' }]
             }]
           }
@@ -666,7 +666,7 @@ RSpec.describe Projects::FeatureFlagsController do
 
         strategy_json = json_response['strategies'].first
         expect(strategy_json['name']).to eq('flexibleRollout')
-        expect(strategy_json['parameters']).to eq({ 'groupId' => 'default', 'rollout' => '15', 'stickiness' => 'DEFAULT' })
+        expect(strategy_json['parameters']).to eq({ 'groupId' => 'default', 'rollout' => '15', 'stickiness' => 'default' })
         expect(strategy_json['scopes'].count).to eq(1)
 
         scope_json = strategy_json['scopes'].first
@@ -938,7 +938,7 @@ RSpec.describe Projects::FeatureFlagsController do
       it 'creates a flexibleRollout strategy' do
         put_request(new_version_flag, strategies_attributes: [{
           name: 'flexibleRollout',
-          parameters: { groupId: 'default', rollout: '30', stickiness: 'DEFAULT' }
+          parameters: { groupId: 'default', rollout: '30', stickiness: 'default' }
         }])
 
         expect(response).to have_gitlab_http_status(:ok)
@@ -948,7 +948,7 @@ RSpec.describe Projects::FeatureFlagsController do
         expect(strategy_json['parameters']).to eq({
           'groupId' => 'default',
           'rollout' => '30',
-          'stickiness' => 'DEFAULT'
+          'stickiness' => 'default'
         })
         expect(strategy_json['scopes']).to eq([])
       end

spec/frontend/feature_flags/components/strategies/flexible_rollout_spec.js

@@ -66,15 +66,14 @@ describe('feature_flags/components/strategies/flexible_rollout.vue', () => {
     });
 
     it('emits a change when the stickiness value changes', async () => {
-      stickinessSelect.setValue('USERID');
-      await wrapper.vm.$nextTick();
+      await stickinessSelect.setValue('userId');
       expect(wrapper.emitted('change')).toEqual([
         [
           {
             parameters: {
               rollout: flexibleRolloutStrategy.parameters.rollout,
               groupId: PERCENT_ROLLOUT_GROUP_ID,
-              stickiness: 'USERID',
+              stickiness: 'userId',
             },
           },
         ],

spec/frontend/feature_flags/mock_data.js

@@ -76,7 +76,7 @@ export const percentRolloutStrategy = {
 
 export const flexibleRolloutStrategy = {
   name: ROLLOUT_STRATEGY_FLEXIBLE_ROLLOUT,
-  parameters: { rollout: '50', groupId: 'default', stickiness: 'DEFAULT' },
+  parameters: { rollout: '50', groupId: 'default', stickiness: 'default' },
   scopes: [],
 };
 

spec/migrations/20210722150102_operations_feature_flags_correct_flexible_rollout_values_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require_migration!('operations_feature_flags_correct_flexible_rollout_values')
+
+RSpec.describe OperationsFeatureFlagsCorrectFlexibleRolloutValues, :migration do
+  let_it_be(:strategies) { table(:operations_strategies) }
+
+  let(:namespace) { table(:namespaces).create!(name: 'feature_flag', path: 'feature_flag') }
+  let(:project) { table(:projects).create!(namespace_id: namespace.id) }
+  let(:feature_flag) { table(:operations_feature_flags).create!(project_id: project.id, active: true, name: 'foo', iid: 1) }
+
+  describe "#up" do
+    described_class::STICKINESS.each do |old, new|
+      it "corrects parameters for flexible rollout stickiness #{old}" do
+        reversible_migration do |migration|
+          parameters = { groupId: "default", rollout: "100", stickiness: old }
+          strategy = create_strategy(parameters)
+
+          migration.before -> {
+            expect(strategy.reload.parameters).to eq({ "groupId" => "default", "rollout" => "100", "stickiness" => old })
+          }
+
+          migration.after -> {
+            expect(strategy.reload.parameters).to eq({ "groupId" => "default", "rollout" => "100", "stickiness" => new })
+          }
+        end
+      end
+    end
+
+    it 'ignores other strategies' do
+      reversible_migration do |migration|
+        parameters = { "groupId" => "default", "rollout" => "100", "stickiness" => "USERID" }
+        strategy = create_strategy(parameters, name: 'default')
+
+        migration.before -> {
+          expect(strategy.reload.parameters).to eq(parameters)
+        }
+
+        migration.after -> {
+          expect(strategy.reload.parameters).to eq(parameters)
+        }
+      end
+    end
+
+    it 'ignores other stickiness' do
+      reversible_migration do |migration|
+        parameters = { "groupId" => "default", "rollout" => "100", "stickiness" => "FOO" }
+        strategy = create_strategy(parameters)
+
+        migration.before -> {
+          expect(strategy.reload.parameters).to eq(parameters)
+        }
+
+        migration.after -> {
+          expect(strategy.reload.parameters).to eq(parameters)
+        }
+      end
+    end
+  end
+
+  def create_strategy(params, name: 'flexibleRollout')
+    strategies.create!(name: name, parameters: params, feature_flag_id: feature_flag.id)
+  end
+end

spec/models/operations/feature_flags/strategy_spec.rb

@@ -112,7 +112,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
       end
 
       context 'when the strategy name is flexibleRollout' do
-        valid_parameters = { rollout: '40', groupId: 'mygroup', stickiness: 'DEFAULT' }
+        valid_parameters = { rollout: '40', groupId: 'mygroup', stickiness: 'default' }
         where(invalid_parameters: [
           nil,
           {},
@@ -133,7 +133,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
 
         [
           [:rollout, '10'],
-          [:stickiness, 'DEFAULT'],
+          [:stickiness, 'default'],
           [:groupId, 'mygroup']
         ].permutation(3).each do |parameters|
           it "allows the parameters in the order #{parameters.map { |p| p.first }.join(', ')}" do
@@ -151,7 +151,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
                                 "\n", "\t", "\n10", "20\n", "\n100", "100\n", "\n  ", nil])
           with_them do
             it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do
-              parameters = { stickiness: 'DEFAULT', groupId: 'mygroup', rollout: invalid_value }
+              parameters = { stickiness: 'default', groupId: 'mygroup', rollout: invalid_value }
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'flexibleRollout',
                                                 parameters: parameters)
@@ -165,7 +165,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
           where(valid_value: %w[0 1 10 38 100 93])
           with_them do
             it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do
-              parameters = { stickiness: 'DEFAULT', groupId: 'mygroup', rollout: valid_value }
+              parameters = { stickiness: 'default', groupId: 'mygroup', rollout: valid_value }
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'flexibleRollout',
                                                 parameters: parameters)
@@ -180,7 +180,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
                                 '!bad', '.bad', 'Bad', 'bad1', "", " ", "b" * 33, "ba_d", "ba\nd"])
           with_them do
             it 'must be a string value of up to 32 lowercase characters' do
-              parameters = { stickiness: 'DEFAULT', groupId: invalid_value, rollout: '40' }
+              parameters = { stickiness: 'default', groupId: invalid_value, rollout: '40' }
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'flexibleRollout',
                                                 parameters: parameters)
@@ -192,7 +192,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
           where(valid_value: ["somegroup", "anothergroup", "okay", "g", "a" * 32])
           with_them do
             it 'must be a string value of up to 32 lowercase characters' do
-              parameters = { stickiness: 'DEFAULT', groupId: valid_value, rollout: '40' }
+              parameters = { stickiness: 'default', groupId: valid_value, rollout: '40' }
               strategy = described_class.create(feature_flag: feature_flag,
                                                 name: 'flexibleRollout',
                                                 parameters: parameters)
@@ -203,7 +203,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
         end
 
         describe 'stickiness' do
-          where(invalid_value: [nil, " ", "default", "DEFAULT\n", "UserId", "USER", "USERID "])
+          where(invalid_value: [nil, " ", "DEFAULT", "DEFAULT\n", "UserId", "USER", "USERID "])
           with_them do
             it 'must be a string representing a supported stickiness setting' do
               parameters = { stickiness: invalid_value, groupId: 'mygroup', rollout: '40' }
@@ -212,12 +212,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do
                                                 parameters: parameters)
 
               expect(strategy.errors[:parameters]).to eq([
-                'stickiness parameter must be DEFAULT, USERID, SESSIONID, or RANDOM'
+                'stickiness parameter must be default, userId, sessionId, or random'
               ])
             end
           end
 
-          where(valid_value: %w[DEFAULT USERID SESSIONID RANDOM])
+          where(valid_value: %w[default userId sessionId random])
           with_them do
             it 'must be a string representing a supported stickiness setting' do
               parameters = { stickiness: valid_value, groupId: 'mygroup', rollout: '40' }
@@ -425,7 +425,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
                                             user_list: user_list,
                                             parameters: { groupId: 'default',
                                                           rollout: '10',
-                                                          stickiness: 'DEFAULT' })
+                                                          stickiness: 'default' })
 
           expect(strategy.errors[:user_list]).to eq(['must be blank'])
         end
@@ -435,7 +435,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
                                             name: 'flexibleRollout',
                                             parameters: { groupId: 'default',
                                                           rollout: '10',
-                                                          stickiness: 'DEFAULT' })
+                                                          stickiness: 'default' })
 
           expect(strategy.errors[:user_list]).to be_empty
         end

spec/requests/api/feature_flags_spec.rb

@@ -417,7 +417,7 @@ RSpec.describe API::FeatureFlags do
           version: 'new_version_flag',
           strategies: [{
             name: 'flexibleRollout',
-            parameters: { groupId: 'default', rollout: '50', stickiness: 'DEFAULT' },
+            parameters: { groupId: 'default', rollout: '50', stickiness: 'default' },
             scopes: [{
               environment_scope: 'staging'
             }]
@@ -434,7 +434,7 @@ RSpec.describe API::FeatureFlags do
         expect(feature_flag.version).to eq('new_version_flag')
         expect(feature_flag.strategies.map { |s| s.slice(:name, :parameters).deep_symbolize_keys }).to eq([{
           name: 'flexibleRollout',
-          parameters: { groupId: 'default', rollout: '50', stickiness: 'DEFAULT' }
+          parameters: { groupId: 'default', rollout: '50', stickiness: 'default' }
         }])
         expect(feature_flag.strategies.first.scopes.map { |s| s.slice(:environment_scope).deep_symbolize_keys }).to eq([{
           environment_scope: 'staging'
@@ -630,7 +630,7 @@ RSpec.describe API::FeatureFlags do
           strategies: [{
             id: strategy.id,
             name: 'flexibleRollout',
-            parameters: { groupId: 'default', rollout: '10', stickiness: 'DEFAULT' }
+            parameters: { groupId: 'default', rollout: '10', stickiness: 'default' }
           }]
         }
 
@@ -642,7 +642,7 @@ RSpec.describe API::FeatureFlags do
         expect(result).to eq([{
           id: strategy.id,
           name: 'flexibleRollout',
-          parameters: { groupId: 'default', rollout: '10', stickiness: 'DEFAULT' }
+          parameters: { groupId: 'default', rollout: '10', stickiness: 'default' }
         }])
       end
 
@@ -677,7 +677,7 @@ RSpec.describe API::FeatureFlags do
         params = {
           strategies: [{
             name: 'flexibleRollout',
-            parameters: { groupId: 'default', rollout: '10', stickiness: 'DEFAULT' }
+            parameters: { groupId: 'default', rollout: '10', stickiness: 'default' }
           }]
         }
 
@@ -694,7 +694,7 @@ RSpec.describe API::FeatureFlags do
           parameters: {}
         }, {
           name: 'flexibleRollout',
-          parameters: { groupId: 'default', rollout: '10', stickiness: 'DEFAULT' }
+          parameters: { groupId: 'default', rollout: '10', stickiness: 'default' }
         }])
       end