Merge branch 'read-only-legacy-feature-flags' into 'master'

Read Only Legacy Feature Flags

See merge request gitlab-org/gitlab!38353

ee/app/assets/javascripts/feature_flags/components/edit_feature_flag.vue

@@ -49,6 +49,9 @@ export default {
     legacyFlagAlert: s__(
       'FeatureFlags|GitLab is moving to a new way of managing feature flags, and in 13.4, this feature flag will become read-only. Please create a new feature flag.',
     ),
+    legacyReadOnlyFlagAlert: s__(
+      'FeatureFlags|GitLab is moving to a new way of managing feature flags. This feature flag is read-only, and it will be removed in 14.0. Please create a new feature flag.',
+    ),
     newFlagAlert: NEW_FLAG_ALERT,
   },
   computed: {
@@ -72,9 +75,18 @@ export default {
     deprecated() {
       return this.hasNewVersionFlags && this.version === LEGACY_FLAG;
     },
+    deprecatedAndEditable() {
+      return this.deprecated && !this.hasLegacyReadOnlyFlags;
+    },
+    deprecatedAndReadOnly() {
+      return this.deprecated && this.hasLegacyReadOnlyFlags;
+    },
     hasNewVersionFlags() {
       return this.glFeatures.featureFlagsNewVersion;
     },
+    hasLegacyReadOnlyFlags() {
+      return this.glFeatures.featureFlagsLegacyReadOnly;
+    },
     shouldShowNewFlagAlert() {
       return !(this.hasNewVersionFlags || this.userDidDismissNewFlagAlert);
     },
@@ -107,9 +119,12 @@ export default {
     <gl-loading-icon v-if="isLoading" />
 
     <template v-else-if="!isLoading && !hasError">
-      <gl-alert v-if="deprecated" variant="warning" :dismissible="false" class="gl-my-5">
+      <gl-alert v-if="deprecatedAndEditable" variant="warning" :dismissible="false" class="gl-my-5">
         {{ $options.translations.legacyFlagAlert }}
       </gl-alert>
+      <gl-alert v-if="deprecatedAndReadOnly" variant="warning" :dismissible="false" class="gl-my-5">
+        {{ $options.translations.legacyReadOnlyFlagAlert }}
+      </gl-alert>
       <div class="d-flex align-items-center mb-3 mt-3">
         <gl-toggle :value="active" class="m-0 mr-3 js-feature-flag-status" @change="toggleActive" />
         <h3 class="page-title m-0">{{ title }}</h3>

ee/app/assets/javascripts/feature_flags/components/feature_flags_table.vue

@@ -2,7 +2,7 @@
 import { GlBadge, GlButton, GlTooltipDirective, GlModal, GlToggle, GlIcon } from '@gitlab/ui';
 import { sprintf, s__ } from '~/locale';
 import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
-import { ROLLOUT_STRATEGY_PERCENT_ROLLOUT, NEW_VERSION_FLAG } from '../constants';
+import { ROLLOUT_STRATEGY_PERCENT_ROLLOUT, NEW_VERSION_FLAG, LEGACY_FLAG } from '../constants';
 import labelForStrategy from '../utils';
 
 export default {
@@ -35,6 +35,7 @@ export default {
   },
   translations: {
     legacyFlagAlert: s__('FeatureFlags|Flag becomes read only soon'),
+    legacyFlagReadOnlyAlert: s__('FeatureFlags|Flag is read-only'),
   },
   computed: {
     permissions() {
@@ -43,6 +44,9 @@ export default {
     isNewVersionFlagsEnabled() {
       return this.glFeatures.featureFlagsNewVersion;
     },
+    isLegacyReadOnlyFlagsEnabled() {
+      return this.glFeatures.featureFlagsLegacyReadOnly;
+    },
     modalTitle() {
       return sprintf(s__('FeatureFlags|Delete %{name}?'), {
         name: this.deleteFeatureFlagName,
@@ -56,11 +60,19 @@ export default {
     modalId() {
       return 'delete-feature-flag';
     },
+    legacyFlagToolTipText() {
+      const { legacyFlagReadOnlyAlert, legacyFlagAlert } = this.$options.translations;
+
+      return this.isLegacyReadOnlyFlagsEnabled ? legacyFlagReadOnlyAlert : legacyFlagAlert;
+    },
   },
   methods: {
     isLegacyFlag(flag) {
       return !this.isNewVersionFlagsEnabled || flag.version !== NEW_VERSION_FLAG;
     },
+    statusToggleDisabled(flag) {
+      return this.isLegacyReadOnlyFlagsEnabled && flag.version === LEGACY_FLAG;
+    },
     scopeTooltipText(scope) {
       return !scope.active
         ? sprintf(s__('FeatureFlags|Inactive flag for %{scope}'), {
@@ -142,6 +154,7 @@ export default {
             <gl-toggle
               v-if="featureFlag.update_path"
               :value="featureFlag.active"
+              :disabled="statusToggleDisabled(featureFlag)"
               @change="toggleFeatureFlag(featureFlag)"
             />
             <gl-badge v-else-if="featureFlag.active" variant="success">
@@ -162,7 +175,7 @@ export default {
               </div>
               <gl-icon
                 v-if="isLegacyFlag(featureFlag)"
-                v-gl-tooltip.hover="$options.translations.legacyFlagAlert"
+                v-gl-tooltip.hover="legacyFlagToolTipText"
                 class="gl-ml-3"
                 name="information-o"
               />

ee/app/assets/javascripts/feature_flags/components/form.vue

@@ -153,6 +153,13 @@ export default {
     showRelatedIssues() {
       return this.featureFlagIssuesEndpoint.length > 0;
     },
+    readOnly() {
+      return (
+        this.glFeatures.featureFlagsNewVersion &&
+        this.glFeatures.featureFlagsLegacyReadOnly &&
+        this.version === LEGACY_FLAG
+      );
+    },
   },
   mounted() {
     if (this.supportsStrategies) {
@@ -587,6 +594,7 @@ export default {
     <div class="form-actions">
       <gl-deprecated-button
         ref="submitButton"
+        :disabled="readOnly"
         type="button"
         variant="success"
         class="js-ff-submit col-xs-12"

ee/app/controllers/projects/feature_flags_controller.rb

@@ -10,9 +10,12 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
 
   before_action :feature_flag, only: [:edit, :update, :destroy]
 
+  before_action :ensure_legacy_flags_writable!, only: [:update]
+
   before_action do
     push_frontend_feature_flag(:feature_flag_permissions)
     push_frontend_feature_flag(:feature_flags_new_version, project, default_enabled: true)
+    push_frontend_feature_flag(:feature_flags_legacy_read_only, project)
   end
 
   def index
@@ -106,6 +109,12 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
     ::Feature.enabled?(:feature_flags_new_version, project, default_enabled: true)
   end
 
+  def ensure_legacy_flags_writable!
+    if ::Feature.enabled?(:feature_flags_legacy_read_only, project) && feature_flag.legacy_flag?
+      render_error_json(['Legacy feature flags are read-only'])
+    end
+  end
+
   def create_params
     params.require(:operations_feature_flag)
       .permit(:name, :description, :active, :version,

ee/spec/controllers/projects/feature_flags_controller_spec.rb

@@ -914,13 +914,18 @@ RSpec.describe Projects::FeatureFlagsController do
       put(:update, params: params, format: :json, as: :json)
     end
 
+    before do
+      stub_feature_flags(feature_flags_legacy_read_only: false)
+    end
+
     subject { put(:update, params: params, format: :json) }
 
     let!(:feature_flag) do
       create(:operations_feature_flag,
-        name: 'ci_live_trace',
-        active: true,
-        project: project)
+             :legacy_flag,
+             name: 'ci_live_trace',
+             active: true,
+             project: project)
     end
 
     let(:params) do
@@ -1292,6 +1297,17 @@ RSpec.describe Projects::FeatureFlagsController do
       end
     end
 
+    context 'when legacy feature flags are set to be read only' do
+      it 'does not update the flag' do
+        stub_feature_flags(feature_flags_legacy_read_only: true)
+
+        put_request(feature_flag, name: 'ci_new_live_trace')
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['message']).to eq(["Legacy feature flags are read-only"])
+      end
+    end
+
     context 'with a version 2 feature flag' do
       let!(:new_version_flag) do
         create(:operations_feature_flag,
@@ -1512,6 +1528,15 @@ RSpec.describe Projects::FeatureFlagsController do
         expect(response).to have_gitlab_http_status(:not_found)
         expect(new_version_flag.reload.name).to eq('new-feature')
       end
+
+      it 'updates the flag when legacy feature flags are set to be read only' do
+        stub_feature_flags(feature_flags_legacy_read_only: true)
+
+        put_request(new_version_flag, name: 'some-other-name')
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(new_version_flag.reload.name).to eq('some-other-name')
+      end
     end
   end
 

ee/spec/features/projects/feature_flags/user_sees_feature_flag_list_spec.rb

@@ -5,16 +5,19 @@ require 'spec_helper'
 RSpec.describe 'User sees feature flag list', :js do
   include FeatureFlagHelpers
 
-  let(:user) { create(:user) }
-  let(:project) { create(:project, namespace: user.namespace) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, namespace: user.namespace) }
 
-  before do
+  before_all do
     project.add_developer(user)
+  end
+
+  before do
     stub_licensed_features(feature_flags: true)
     sign_in(user)
   end
 
-  context 'when there are feature flags and scopes' do
+  context 'with legacy feature flags' do
     before do
       create_flag(project, 'ci_live_trace', false).tap do |feature_flag|
         create_scope(feature_flag, 'review/*', true)
@@ -23,11 +26,11 @@ RSpec.describe 'User sees feature flag list', :js do
       create_flag(project, 'mr_train', true).tap do |feature_flag|
         create_scope(feature_flag, 'production', false)
       end
-
-      visit(project_feature_flags_path(project))
     end
 
     it 'user sees the first flag' do
+      visit(project_feature_flags_path(project))
+
       within_feature_flag_row(1) do
         expect(page.find('.js-feature-flag-id')).to have_content('^1')
         expect(page.find('.feature-flag-name')).to have_content('ci_live_trace')
@@ -41,6 +44,8 @@ RSpec.describe 'User sees feature flag list', :js do
     end
 
     it 'user sees the second flag' do
+      visit(project_feature_flags_path(project))
+
       within_feature_flag_row(2) do
         expect(page.find('.js-feature-flag-id')).to have_content('^2')
         expect(page.find('.feature-flag-name')).to have_content('drop_legacy_artifacts')
@@ -53,6 +58,8 @@ RSpec.describe 'User sees feature flag list', :js do
     end
 
     it 'user sees the third flag' do
+      visit(project_feature_flags_path(project))
+
       within_feature_flag_row(3) do
         expect(page.find('.js-feature-flag-id')).to have_content('^3')
         expect(page.find('.feature-flag-name')).to have_content('mr_train')
@@ -65,9 +72,48 @@ RSpec.describe 'User sees feature flag list', :js do
       end
     end
 
+    it 'user sees the status toggle disabled' do
+      visit(project_feature_flags_path(project))
+
+      within_feature_flag_row(1) do
+        expect(page).to have_css('.js-feature-flag-status button.is-disabled')
+      end
+    end
+
+    context 'when legacy feature flags are not read-only' do
+      before do
+        stub_feature_flags(feature_flags_legacy_read_only: false)
+      end
+
+      it 'user updates the status toggle' do
+        visit(project_feature_flags_path(project))
+
+        within_feature_flag_row(1) do
+          page.find('.js-feature-flag-status button').click
+
+          expect(page).to have_css('.js-feature-flag-status button.is-checked')
+        end
+
+        visit(project_audit_events_path(project))
+
+        expect(page).to(
+          have_text('Updated feature flag ci_live_trace. Updated active from "false" to "true".')
+        )
+      end
+    end
+  end
+
+  context 'with new version flags' do
+    before do
+      create(:operations_feature_flag, :new_version_flag, project: project,
+             name: 'my_flag', active: false)
+    end
+
     it 'user updates the status toggle' do
+      visit(project_feature_flags_path(project))
+
       within_feature_flag_row(1) do
-        page.find('.js-feature-flag-status button').click
+        status_toggle_button.click
 
         expect(page).to have_css('.js-feature-flag-status button.is-checked')
       end
@@ -75,7 +121,7 @@ RSpec.describe 'User sees feature flag list', :js do
       visit(project_audit_events_path(project))
 
       expect(page).to(
-        have_text('Updated feature flag ci_live_trace. Updated active from "false" to "true".')
+        have_text('Updated feature flag my_flag. Updated active from "false" to "true".')
       )
     end
   end

ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb

@@ -5,11 +5,14 @@ require 'spec_helper'
 RSpec.describe 'User updates feature flag', :js do
   include FeatureFlagHelpers
 
-  let(:user) { create(:user) }
-  let(:project) { create(:project, namespace: user.namespace) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, namespace: user.namespace) }
 
-  before do
+  before_all do
     project.add_developer(user)
+  end
+
+  before do
     stub_licensed_features(feature_flags: true)
     stub_feature_flags(feature_flag_permissions: false)
     sign_in(user)
@@ -74,109 +77,121 @@ RSpec.describe 'User updates feature flag', :js do
 
     let!(:scope) { create_scope(feature_flag, 'review/*', true) }
 
-    before do
-      visit(edit_project_feature_flag_path(project, feature_flag))
-    end
+    context 'when legacy flags are editable' do
+      before do
+        stub_feature_flags(feature_flags_legacy_read_only: false)
 
-    it 'user sees persisted default scope' do
-      within_scope_row(1) do
-        within_environment_spec do
-          expect(page).to have_content('* (All Environments)')
-        end
+        visit(edit_project_feature_flag_path(project, feature_flag))
+      end
 
-        within_status do
-          expect(find('.project-feature-toggle')['aria-label'])
-            .to eq('Toggle Status: ON')
+      it 'user sees persisted default scope' do
+        within_scope_row(1) do
+          within_environment_spec do
+            expect(page).to have_content('* (All Environments)')
+          end
+
+          within_status do
+            expect(find('.project-feature-toggle')['aria-label'])
+              .to eq('Toggle Status: ON')
+          end
         end
       end
-    end
 
-    context 'when user updates a status of a scope' do
-      before do
-        within_scope_row(2) do
-          within_status { find('.project-feature-toggle').click }
+      context 'when user updates a status of a scope' do
+        before do
+          within_scope_row(2) do
+            within_status { find('.project-feature-toggle').click }
+          end
+
+          click_button 'Save changes'
+          expect(page).to have_current_path(project_feature_flags_path(project))
         end
 
-        click_button 'Save changes'
-        expect(page).to have_current_path(project_feature_flags_path(project))
-      end
+        it 'shows the updated feature flag' do
+          within_feature_flag_row(1) do
+            expect(page.find('.feature-flag-name')).to have_content('ci_live_trace')
+            expect(page).to have_css('.js-feature-flag-status button.is-checked')
+
+            within_feature_flag_scopes do
+              expect(page.find('.badge:nth-child(1)')).to have_content('*')
+              expect(page.find('.badge:nth-child(1)')['class']).to include('badge-info')
+              expect(page.find('.badge:nth-child(2)')).to have_content('review/*')
+              expect(page.find('.badge:nth-child(2)')['class']).to include('badge-muted')
+            end
+          end
+        end
 
-      it 'shows the updated feature flag' do
-        within_feature_flag_row(1) do
-          expect(page.find('.feature-flag-name')).to have_content('ci_live_trace')
-          expect(page).to have_css('.js-feature-flag-status button.is-checked')
+        it 'records audit event' do
+          visit(project_audit_events_path(project))
 
-          within_feature_flag_scopes do
-            expect(page.find('[data-qa-selector="feature-flag-scope-info-badge"]:nth-child(1)')).to have_content('*')
-            expect(page.find('[data-qa-selector="feature-flag-scope-muted-badge"]:nth-child(2)')).to have_content('review/*')
-          end
+          expect(page).to(
+            have_text("Updated feature flag ci_live_trace. Updated rule review/* active state from true to false.")
+          )
         end
       end
 
-      it 'records audit event' do
-        visit(project_audit_events_path(project))
+      context 'when user adds a new scope' do
+        before do
+          within_scope_row(3) do
+            within_environment_spec do
+              find('.js-env-input').set('production')
+              find('.js-create-button').click
+            end
+          end
 
-        expect(page).to(
-          have_text("Updated feature flag ci_live_trace. Updated rule review/* active state from true to false.")
-        )
-      end
-    end
+          click_button 'Save changes'
+          expect(page).to have_current_path(project_feature_flags_path(project))
+        end
 
-    context 'when user adds a new scope' do
-      before do
-        within_scope_row(3) do
-          within_environment_spec do
-            find('.js-env-input').set('production')
-            find('.js-create-button').click
+        it 'shows the newly created scope' do
+          within_feature_flag_row(1) do
+            within_feature_flag_scopes do
+              expect(page.find('.badge:nth-child(3)')).to have_content('production')
+              expect(page.find('.badge:nth-child(3)')['class']).to include('badge-muted')
+            end
           end
         end
 
-        click_button 'Save changes'
-        expect(page).to have_current_path(project_feature_flags_path(project))
-      end
+        it 'records audit event' do
+          visit(project_audit_events_path(project))
 
-      it 'shows the newly created scope' do
-        within_feature_flag_row(1) do
-          within_feature_flag_scopes do
-            expect(page.find('[data-qa-selector="feature-flag-scope-muted-badge"]:nth-child(3)')).to have_content('production')
-          end
+          expect(page).to have_text "Updated feature flag ci_live_trace"
         end
       end
 
-      it 'records audit event' do
-        visit(project_audit_events_path(project))
+      context 'when user deletes a scope' do
+        before do
+          within_scope_row(2) do
+            within_delete { find('.js-delete-scope').click }
+          end
 
-        expect(page).to(
-          have_text("Updated feature flag ci_live_trace")
-        )
-      end
-    end
+          click_button 'Save changes'
+          expect(page).to have_current_path(project_feature_flags_path(project))
+        end
 
-    context 'when user deletes a scope' do
-      before do
-        within_scope_row(2) do
-          within_delete { find('.js-delete-scope').click }
+        it 'shows the updated feature flag' do
+          within_feature_flag_row(1) do
+            within_feature_flag_scopes do
+              expect(page).to have_css('.badge:nth-child(1)')
+              expect(page).not_to have_css('.badge:nth-child(2)')
+            end
+          end
         end
 
-        click_button 'Save changes'
-        expect(page).to have_current_path(project_feature_flags_path(project))
-      end
+        it 'records audit event' do
+          visit(project_audit_events_path(project))
 
-      it 'shows the updated feature flag' do
-        within_feature_flag_row(1) do
-          within_feature_flag_scopes do
-            expect(page).to have_css('[data-qa-selector="feature-flag-scope-info-badge"]:nth-child(1)')
-            expect(page).not_to have_css('[data-qa-selector="feature-flag-scope-info-badge"]:nth-child(2)')
-          end
+          expect(page).to have_text "Updated feature flag ci_live_trace"
         end
       end
+    end
 
-      it 'records audit event' do
-        visit(project_audit_events_path(project))
+    context 'when legacy flags are read-only' do
+      it 'the user cannot edit the flag' do
+        visit(edit_project_feature_flag_path(project, feature_flag))
 
-        expect(page).to(
-          have_text("Updated feature flag ci_live_trace")
-        )
+        expect(page).to have_text 'This feature flag is read-only, and it will be removed in 14.0.'
+        expect(page).to have_css('button.js-ff-submit.disabled')
       end
     end
   end

locale/gitlab.pot

@@ -10369,12 +10369,18 @@ msgstr ""
 msgid "FeatureFlags|Flag becomes read only soon"
 msgstr ""
 
+msgid "FeatureFlags|Flag is read-only"
+msgstr ""
+
 msgid "FeatureFlags|Get started with feature flags"
 msgstr ""
 
 msgid "FeatureFlags|GitLab is moving to a new way of managing feature flags, and in 13.4, this feature flag will become read-only. Please create a new feature flag."
 msgstr ""
 
+msgid "FeatureFlags|GitLab is moving to a new way of managing feature flags. This feature flag is read-only, and it will be removed in 14.0. Please create a new feature flag."
+msgstr ""
+
 msgid "FeatureFlags|ID"
 msgstr ""