Merge branch 'disallow-delete-assoced-userlists' into 'master'

Do Not Allow a User List to be Deleted if Associated with a Strategy

See merge request gitlab-org/gitlab!35275

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

@@ -9,6 +9,8 @@ module Operations
       self.table_name = 'operations_user_lists'
 
       belongs_to :project
+      has_many :strategy_user_lists
+      has_many :strategies, through: :strategy_user_lists
 
       has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.operations_feature_flags_user_lists&.maximum(:iid) }, presence: true
 
@@ -18,6 +20,17 @@ module Operations
         uniqueness: { scope: :project_id },
         length: 1..255
       validates :user_xids, feature_flag_user_xids: true
+
+      before_destroy :ensure_no_associated_strategies
+
+      private
+
+      def ensure_no_associated_strategies
+        if strategies.present?
+          errors.add(:base, 'User list is associated with a strategy')
+          throw :abort # rubocop: disable Cop/BanCatchThrow
+        end
+      end
     end
   end
 end

ee/changelogs/unreleased/disallow-delete-assoced-userlists.yml

+---
+title: Forbid deleting a feature flag user list associated with a strategy
+merge_request: 35275
+author:
+type: fixed

ee/lib/api/feature_flags_user_lists.rb

@@ -84,7 +84,9 @@ module API
         end
         delete do
           list = user_project.operations_feature_flags_user_lists.find_by_iid!(params[:iid])
-          list.destroy
+          unless list.destroy
+            render_api_error!(list.errors.full_messages, :conflict)
+          end
         end
       end
     end

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

@@ -67,6 +67,31 @@ RSpec.describe Operations::FeatureFlags::UserList do
     end
   end
 
+  describe '#destroy' do
+    it 'deletes the model if it is not associated with any feature flag strategies' do
+      project = create(:project)
+      user_list = described_class.create(project: project, name: 'My User List', user_xids: 'user1,user2')
+
+      user_list.destroy
+
+      expect(described_class.count).to eq(0)
+    end
+
+    it 'does not delete the model if it is associated with a feature flag strategy' do
+      project = create(:project)
+      user_list = described_class.create(project: project, name: 'My User List', user_xids: 'user1,user2')
+      feature_flag = create(:operations_feature_flag, :new_version_flag, project: project)
+      strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'gitlabUserList', user_list: user_list)
+
+      user_list.destroy
+
+      expect(described_class.count).to eq(1)
+      expect(::Operations::FeatureFlags::StrategyUserList.count).to eq(1)
+      expect(strategy.reload.user_list).to eq(user_list)
+      expect(strategy.valid?).to eq(true)
+    end
+  end
+
   it_behaves_like 'AtomicInternalId' do
     let(:internal_id_attribute) { :iid }
     let(:instance) { build(:operations_feature_flag_user_list) }

ee/spec/requests/api/feature_flags_user_lists_spec.rb

@@ -343,8 +343,21 @@ RSpec.describe API::FeatureFlagsUserLists do
 
       delete api("/projects/#{project.id}/feature_flags_user_lists/#{list.iid}", developer)
 
-      expect(response).to have_gitlab_http_status(:ok)
+      expect(response).to have_gitlab_http_status(:no_content)
+      expect(response.body).to be_blank
       expect(project.operations_feature_flags_user_lists.count).to eq(0)
     end
+
+    it 'does not delete the list if it is associated with a strategy' do
+      list = create_list
+      feature_flag = create(:operations_feature_flag, :new_version_flag, project: project)
+      create(:operations_strategy, feature_flag: feature_flag, name: 'gitlabUserList', user_list: list)
+
+      delete api("/projects/#{project.id}/feature_flags_user_lists/#{list.iid}", developer)
+
+      expect(response).to have_gitlab_http_status(:conflict)
+      expect(json_response).to eq({ 'message' => ['User list is associated with a strategy'] })
+      expect(list.reload).to be_persisted
+    end
   end
 end