Merge branch 'remove-feature_flags_legacy_read_only-feature-flag' into 'master'

Remove `feature_flags_legacy_read_only` feature flag

See merge request gitlab-org/gitlab!48441

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

@@ -30,9 +30,6 @@ export default {
     };
   },
   translations: {
-    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.',
     ),
@@ -59,18 +56,6 @@ export default {
     deprecated() {
       return this.version === LEGACY_FLAG;
     },
-    deprecatedAndEditable() {
-      return this.deprecated && !this.hasLegacyReadOnlyFlags;
-    },
-    deprecatedAndReadOnly() {
-      return this.deprecated && this.hasLegacyReadOnlyFlags;
-    },
-    hasLegacyReadOnlyFlags() {
-      return (
-        this.glFeatures.featureFlagsLegacyReadOnly &&
-        !this.glFeatures.featureFlagsLegacyReadOnlyOverride
-      );
-    },
   },
   created() {
     return this.fetchFeatureFlag();
@@ -91,12 +76,9 @@ export default {
     <gl-loading-icon v-if="isLoading" size="xl" class="gl-mt-7" />
 
     <template v-else-if="!isLoading && !hasError">
-      <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>
+      <gl-alert v-if="deprecated" variant="warning" :dismissible="false" class="gl-my-5">{{
+        $options.translations.legacyReadOnlyFlagAlert
+      }}</gl-alert>
       <div class="gl-display-flex gl-align-items-center gl-mb-4 gl-mt-4">
         <gl-toggle
           :value="active"

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

@@ -31,19 +31,12 @@ export default {
     };
   },
   translations: {
-    legacyFlagAlert: s__('FeatureFlags|Flag becomes read only soon'),
     legacyFlagReadOnlyAlert: s__('FeatureFlags|Flag is read-only'),
   },
   computed: {
     permissions() {
       return this.glFeatures.featureFlagPermissions;
     },
-    isLegacyReadOnlyFlagsEnabled() {
-      return (
-        this.glFeatures.featureFlagsLegacyReadOnly &&
-        !this.glFeatures.featureFlagsLegacyReadOnlyOverride
-      );
-    },
     modalTitle() {
       return sprintf(s__('FeatureFlags|Delete %{name}?'), {
         name: this.deleteFeatureFlagName,
@@ -57,18 +50,13 @@ export default {
     modalId() {
       return 'delete-feature-flag';
     },
-    legacyFlagToolTipText() {
-      const { legacyFlagReadOnlyAlert, legacyFlagAlert } = this.$options.translations;
-
-      return this.isLegacyReadOnlyFlagsEnabled ? legacyFlagReadOnlyAlert : legacyFlagAlert;
-    },
   },
   methods: {
     isLegacyFlag(flag) {
       return flag.version !== NEW_VERSION_FLAG;
     },
     statusToggleDisabled(flag) {
-      return this.isLegacyReadOnlyFlagsEnabled && flag.version === LEGACY_FLAG;
+      return flag.version === LEGACY_FLAG;
     },
     scopeTooltipText(scope) {
       return !scope.active
@@ -123,9 +111,7 @@ export default {
 <template>
   <div class="table-holder js-feature-flag-table">
     <div class="gl-responsive-table-row table-row-header" role="row">
-      <div class="table-section section-10">
-        {{ s__('FeatureFlags|ID') }}
-      </div>
+      <div class="table-section section-10">{{ s__('FeatureFlags|ID') }}</div>
       <div class="table-section section-10" role="columnheader">
         {{ s__('FeatureFlags|Status') }}
       </div>
@@ -161,9 +147,8 @@ export default {
               v-else-if="featureFlag.active"
               variant="success"
               data-testid="feature-flag-status-badge"
+              >{{ s__('FeatureFlags|Active') }}</gl-badge
             >
-              {{ s__('FeatureFlags|Active') }}
-            </gl-badge>
             <gl-badge v-else variant="danger">{{ s__('FeatureFlags|Inactive') }}</gl-badge>
           </div>
         </div>
@@ -179,7 +164,7 @@ export default {
               </div>
               <gl-icon
                 v-if="isLegacyFlag(featureFlag)"
-                v-gl-tooltip.hover="legacyFlagToolTipText"
+                v-gl-tooltip.hover="$options.translations.legacyFlagReadOnlyAlert"
                 class="gl-ml-3"
                 name="information-o"
               />
@@ -205,9 +190,8 @@ export default {
                 :variant="badgeVariant(scope)"
                 :data-qa-selector="`feature-flag-scope-${badgeVariant(scope)}-badge`"
                 class="gl-mr-3 gl-mt-2"
+                >{{ badgeText(scope) }}</gl-badge
               >
-                {{ badgeText(scope) }}
-              </gl-badge>
             </template>
             <template v-else>
               <gl-badge
@@ -216,9 +200,8 @@ export default {
                 data-testid="strategy-badge"
                 variant="info"
                 class="gl-mr-3 gl-mt-2"
+                >{{ strategyBadgeText(strategy) }}</gl-badge
               >
-                {{ strategyBadgeText(strategy) }}
-              </gl-badge>
             </template>
           </div>
         </div>

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

@@ -143,11 +143,7 @@ export default {
       return this.featureFlagIssuesEndpoint.length > 0;
     },
     readOnly() {
-      return (
-        this.glFeatures.featureFlagsLegacyReadOnly &&
-        !this.glFeatures.featureFlagsLegacyReadOnlyOverride &&
-        this.version === LEGACY_FLAG
-      );
+      return this.version === LEGACY_FLAG;
     },
   },
   methods: {

app/controllers/projects/feature_flags_controller.rb

@@ -10,12 +10,10 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
 
   before_action :feature_flag, only: [:edit, :update, :destroy]
 
-  before_action :ensure_legacy_flags_writable!, only: [:update]
+  before_action :ensure_flag_writable!, only: [:update]
 
   before_action do
     push_frontend_feature_flag(:feature_flag_permissions)
-    push_frontend_feature_flag(:feature_flags_legacy_read_only, project, default_enabled: true)
-    push_frontend_feature_flag(:feature_flags_legacy_read_only_override, project)
   end
 
   feature_category :feature_flags
@@ -103,10 +101,8 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
     @feature_flag ||= @noteable = project.operations_feature_flags.find_by_iid!(params[:iid])
   end
 
-  def ensure_legacy_flags_writable!
-    if ::Feature.enabled?(:feature_flags_legacy_read_only, project, default_enabled: true) &&
-        ::Feature.disabled?(:feature_flags_legacy_read_only_override, project) &&
-        feature_flag.legacy_flag?
+  def ensure_flag_writable!
+    if feature_flag.legacy_flag?
       render_error_json(['Legacy feature flags are read-only'])
     end
   end

config/feature_flags/development/feature_flags_legacy_read_only.yml

----
-name: feature_flags_legacy_read_only
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38353
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/240985
-milestone: '13.3'
-type: development
-group: group::progressive delivery
-default_enabled: true

config/feature_flags/development/feature_flags_legacy_read_only_override.yml

----
-name: feature_flags_legacy_read_only_override
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40431
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/240985
-milestone: '13.4'
-type: development
-group: group::progressive delivery
-default_enabled: false

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

@@ -16,66 +16,6 @@ RSpec.describe 'User sees feature flag list', :js do
     sign_in(user)
   end
 
-  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)
-      end
-      create_flag(project, 'drop_legacy_artifacts', false)
-      create_flag(project, 'mr_train', true).tap do |feature_flag|
-        create_scope(feature_flag, 'production', false)
-      end
-      stub_feature_flags(feature_flags_legacy_read_only_override: false)
-    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
-          status_toggle_button.click
-
-          expect_status_toggle_button_to_be_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
-
-    context 'when legacy feature flags are read-only but the override is active for a project' do
-      before do
-        stub_feature_flags(
-          feature_flags_legacy_read_only: true,
-          feature_flags_legacy_read_only_override: project
-        )
-      end
-
-      it 'user updates the status toggle' do
-        visit(project_feature_flags_path(project))
-
-        within_feature_flag_row(1) do
-          status_toggle_button.click
-
-          expect_status_toggle_button_to_be_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,

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

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe 'User updates feature flag', :js do
-  include FeatureFlagHelpers
-
-  let_it_be(:user) { create(:user) }
-  let_it_be(:project) { create(:project, namespace: user.namespace) }
-
-  before_all do
-    project.add_developer(user)
-  end
-
-  before do
-    stub_feature_flags(
-      feature_flag_permissions: false,
-      feature_flags_legacy_read_only_override: false
-    )
-    sign_in(user)
-  end
-
-  context 'with a legacy feature flag' do
-    let!(:feature_flag) do
-      create_flag(project, 'ci_live_trace', true,
-                  description: 'For live trace feature')
-    end
-
-    let!(:scope) { create_scope(feature_flag, 'review/*', true) }
-
-    context 'when legacy flags are editable' do
-      before do
-        stub_feature_flags(feature_flags_legacy_read_only: false)
-
-        visit(edit_project_feature_flag_path(project, feature_flag))
-      end
-
-      context 'when user updates the 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
-
-        it 'records audit event' do
-          visit(project_audit_events_path(project))
-
-          expect(page).to(
-            have_text("Updated feature flag ci_live_trace. Updated rule review/* active state from true to false.")
-          )
-        end
-      end
-
-      context 'when user adds a new scope' do
-        before do
-          within_scope_row(3) do
-            within_environment_spec do
-              find('.js-env-search > input').set('production')
-              find('.js-create-button').click
-            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))
-
-          expect(page).to have_text "Updated feature flag ci_live_trace"
-        end
-      end
-
-      context 'when user deletes a scope' do
-        before do
-          within_scope_row(2) do
-            within_delete { find('.js-delete-scope').click }
-          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))
-
-          expect(page).to have_text "Updated feature flag ci_live_trace"
-        end
-      end
-    end
-  end
-end

locale/gitlab.pot

@@ -12113,9 +12113,6 @@ msgstr ""
 msgid "FeatureFlags|Feature flags limit reached (%{featureFlagsLimit}). Delete one or more feature flags before adding new ones."
 msgstr ""
 
-msgid "FeatureFlags|Flag becomes read only soon"
-msgstr ""
-
 msgid "FeatureFlags|Flag is read-only"
 msgstr ""
 
@@ -12125,9 +12122,6 @@ msgstr ""
 msgid "FeatureFlags|Get started with user lists"
 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 ""
 

spec/features/projects/feature_flags/user_sees_feature_flag_list_spec.rb

@@ -25,7 +25,6 @@ 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
-      stub_feature_flags(feature_flags_legacy_read_only_override: false)
     end
 
     it 'user sees the first flag' do
@@ -79,41 +78,6 @@ RSpec.describe 'User sees feature flag list', :js do
         expect_status_toggle_button_to_be_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
-          status_toggle_button.click
-
-          expect_status_toggle_button_to_be_checked
-        end
-      end
-    end
-
-    context 'when legacy feature flags are read-only but the override is active for a project' do
-      before do
-        stub_feature_flags(
-          feature_flags_legacy_read_only: true,
-          feature_flags_legacy_read_only_override: project
-        )
-      end
-
-      it 'user updates the status toggle' do
-        visit(project_feature_flags_path(project))
-
-        within_feature_flag_row(1) do
-          status_toggle_button.click
-
-          expect_status_toggle_button_to_be_checked
-        end
-      end
-    end
   end
 
   context 'with new version flags' do

spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb

@@ -14,8 +14,7 @@ RSpec.describe 'User updates feature flag', :js do
 
   before do
     stub_feature_flags(
-      feature_flag_permissions: false,
-      feature_flags_legacy_read_only_override: false
+      feature_flag_permissions: false
     )
     sign_in(user)
   end
@@ -79,96 +78,6 @@ RSpec.describe 'User updates feature flag', :js do
 
     let!(:scope) { create_scope(feature_flag, 'review/*', true) }
 
-    context 'when legacy flags are editable' do
-      before do
-        stub_feature_flags(feature_flags_legacy_read_only: false)
-
-        visit(edit_project_feature_flag_path(project, feature_flag))
-      end
-
-      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
-
-      context 'when user updates the 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
-
-        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_status_toggle_button_to_be_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
-      end
-
-      context 'when user adds a new scope' do
-        before do
-          within_scope_row(3) do
-            within_environment_spec do
-              find('.js-env-search > input').set('production')
-              find('.js-create-button').click
-            end
-          end
-
-          click_button 'Save changes'
-          expect(page).to have_current_path(project_feature_flags_path(project))
-        end
-
-        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
-      end
-
-      context 'when user deletes a scope' do
-        before do
-          within_scope_row(2) do
-            within_delete { find('.js-delete-scope').click }
-          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
-            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
-      end
-    end
-
-    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))
 
@@ -176,20 +85,4 @@ RSpec.describe 'User updates feature flag', :js do
       expect(page).to have_css('button.js-ff-submit.disabled')
     end
   end
-
-    context 'when legacy flags are read-only, but the override is active for one project' do
-      it 'the user can edit the flag' do
-        stub_feature_flags(feature_flags_legacy_read_only_override: project)
-
-        visit(edit_project_feature_flag_path(project, feature_flag))
-        status_toggle_button.click
-        click_button 'Save changes'
-
-        expect(page).to have_current_path(project_feature_flags_path(project))
-        within_feature_flag_row(1) do
-          expect_status_toggle_button_not_to_be_checked
-        end
-      end
-    end
-  end
 end

spec/frontend/feature_flags/components/feature_flags_table_spec.js

@@ -120,13 +120,11 @@ describe('Feature flag table', () => {
 
   describe('when active and with an update toggle', () => {
     let toggle;
-    let spy;
 
     beforeEach(() => {
       props.featureFlags[0].update_path = props.featureFlags[0].destroy_path;
       createWrapper(props);
       toggle = wrapper.find(GlToggle);
-      spy = mockTracking('_category_', toggle.element, jest.spyOn);
     });
 
     it('should have a toggle', () => {
@@ -142,14 +140,6 @@ describe('Feature flag table', () => {
         expect(wrapper.emitted('toggle-flag')).toEqual([[flag]]);
       });
     });
-
-    it('should track a click', () => {
-      toggle.trigger('click');
-
-      expect(spy).toHaveBeenCalledWith('_category_', 'click_button', {
-        label: 'feature_flag_toggle',
-      });
-    });
   });
 
   describe('with an active scope and a percentage rollout strategy', () => {
@@ -180,6 +170,8 @@ describe('Feature flag table', () => {
   });
 
   describe('with a new version flag', () => {
+    let toggle;
+    let spy;
     let badges;
 
     beforeEach(() => {
@@ -194,6 +186,7 @@ describe('Feature flag table', () => {
             description: 'flag description',
             destroy_path: 'destroy/path',
             edit_path: 'edit/path',
+            update_path: 'update/path',
             version: NEW_VERSION_FLAG,
             scopes: [],
             strategies: [
@@ -226,6 +219,8 @@ describe('Feature flag table', () => {
         provide: { csrfToken: 'fakeToken', glFeatures: { featureFlagsNewVersion: true } },
       });
 
+      toggle = wrapper.find(GlToggle);
+      spy = mockTracking('_category_', toggle.element, jest.spyOn);
       badges = wrapper.findAll('[data-testid="strategy-badge"]');
     });
 
@@ -254,6 +249,14 @@ describe('Feature flag table', () => {
     it('shows the name of a user list for user list', () => {
       expect(badges.at(3).text()).toContain('User List - test list');
     });
+
+    it('tracks a click', () => {
+      toggle.trigger('click');
+
+      expect(spy).toHaveBeenCalledWith('_category_', 'click_button', {
+        label: 'feature_flag_toggle',
+      });
+    });
   });
 
   it('renders a feature flag without an iid', () => {