Merge branch 'fix-scoped-label-application-order' into 'master'

Preserve order when applying multiple scoped labels

See merge request gitlab-org/gitlab!33420

app/services/labels/available_labels_service.rb

@@ -30,11 +30,13 @@ module Labels
     end
 
     def filter_labels_ids_in_param(key)
-      return [] if params[key].to_a.empty?
+      ids = params[key].to_a
+      return [] if ids.empty?
 
       # rubocop:disable CodeReuse/ActiveRecord
-      available_labels.by_ids(params[key]).pluck(:id)
+      existing_ids = available_labels.by_ids(ids).pluck(:id)
       # rubocop:enable CodeReuse/ActiveRecord
+      ids.map(&:to_i) & existing_ids
     end
 
     private

db/fixtures/development/10_merge_requests.rb

@@ -19,7 +19,7 @@ Gitlab::Seeder.quiet do
       target_branch = branches.pop
 
       label_ids = project.labels.pluck(:id).sample(3)
-      label_ids += project.group.labels.sample(3) if project.group
+      label_ids += project.group.labels.sample(3).pluck(:id) if project.group
 
       params = {
         source_branch: source_branch,

ee/app/services/ee/issuable_base_service.rb

@@ -5,10 +5,10 @@ module EE
     extend ::Gitlab::Utils::Override
     include ::Gitlab::Utils::StrongMemoize
 
-    attr_reader :label_ids_ordered_by_selection
-
     private
 
+    attr_reader :label_ids_ordered_by_selection
+
     override :filter_params
     def filter_params(issuable)
       can_admin_issuable = can_admin_issuable?(issuable)
@@ -26,9 +26,9 @@ module EE
 
     override :filter_labels
     def filter_labels
-      @label_ids_ordered_by_selection = params[:add_label_ids].to_a + params[:label_ids].to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
-
       super
+
+      @label_ids_ordered_by_selection = params[:add_label_ids].to_a + params[:label_ids].to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
     end
 
     def update_task_event?

ee/changelogs/unreleased/fix-scoped-label-application-order.yml

+---
+title: Preserve order when applying scoped labels by title
+merge_request: 33420
+author:
+type: fixed

ee/spec/support/shared_examples/services/scoped_label_shared_examples.rb

@@ -16,26 +16,38 @@ end
 
 RSpec.shared_examples 'new issuable with scoped labels' do
   include_context 'exclusive labels creation' do
-    context 'scoped labels are avaialble' do
+    context 'when scoped labels are available' do
       before do
         stub_licensed_features(scoped_labels: true)
       end
 
-      it 'adds only last selected exclusive scoped label' do
-        label1 = create_label('label1')
-        label2 = create_label('key::label1')
-        label3 = create_label('key::label2')
-        label4 = create_label('key::label3')
+      let!(:label1) { create_label('label1') }
+      let!(:label2) { create_label('key::label1') }
+      let!(:label3) { create_label('key::label2') }
+      let!(:label4) { create_label('key::label3') }
 
-        issuable = described_class.new(
-          parent, user, title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id]
-        ).execute
+      context 'when using label_ids parameter' do
+        it 'adds only last selected exclusive scoped label' do
+          issuable = described_class.new(
+            parent, user, title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id]
+          ).execute
 
-        expect(issuable.labels).to match_array([label1, label2])
+          expect(issuable.labels).to match_array([label1, label2])
+        end
+      end
+
+      context 'when using labels parameter' do
+        it 'adds only last selected exclusive scoped label' do
+          issuable = described_class.new(
+            parent, user, title: 'test', labels: [label1.title, label3.title, label4.title, label2.title]
+          ).execute
+
+          expect(issuable.labels).to match_array([label1, label2])
+        end
       end
     end
 
-    context 'scoped labels are not available' do
+    context 'when scoped labels are not available' do
       before do
         stub_licensed_features(scoped_labels: false)
       end
@@ -62,40 +74,59 @@ RSpec.shared_examples 'existing issuable with scoped labels' do
     let(:label2) { create_label('key::label2') }
     let(:label3) { create_label('key::label3') }
 
-    context 'scoped labels are avaialble' do
+    context 'when scoped labels are available' do
       before do
         stub_licensed_features(scoped_labels: true, epics: true)
       end
 
-      it 'adds only last selected exclusive scoped label' do
-        create(:label_link, label: label1, target: issuable)
-        create(:label_link, label: label2, target: issuable)
+      context 'when using label_ids parameter' do
+        it 'adds only last selected exclusive scoped label' do
+          create(:label_link, label: label1, target: issuable)
+          create(:label_link, label: label2, target: issuable)
 
-        issuable.reload
+          issuable.reload
 
-        described_class.new(
-          parent, user, label_ids: [label1.id, label3.id]
-        ).execute(issuable)
+          described_class.new(
+            parent, user, label_ids: [label1.id, label3.id]
+          ).execute(issuable)
 
-        expect(issuable.reload.labels).to match_array([label3])
+          expect(issuable.reload.labels).to match_array([label3])
+        end
       end
 
-      it 'preserves multiple exclusive scoped labels when only removing labels' do
-        create(:label_link, label: label1, target: issuable)
-        create(:label_link, label: label2, target: issuable)
-        create(:label_link, label: label3, target: issuable)
+      context 'when using label_ids parameter' do
+        it 'adds only last selected exclusive scoped label' do
+          create(:label_link, label: label1, target: issuable)
+          create(:label_link, label: label2, target: issuable)
 
-        issuable.reload
+          issuable.reload
 
-        described_class.new(
-          parent, user, label_ids: [label2.id, label3.id]
-        ).execute(issuable)
+          described_class.new(
+            parent, user, labels: [label1.title, label3.title]
+          ).execute(issuable)
+
+          expect(issuable.reload.labels).to match_array([label3])
+        end
+      end
+
+      context 'when only removing labels' do
+        it 'preserves multiple exclusive scoped labels' do
+          create(:label_link, label: label1, target: issuable)
+          create(:label_link, label: label2, target: issuable)
+          create(:label_link, label: label3, target: issuable)
+
+          issuable.reload
+
+          described_class.new(
+            parent, user, label_ids: [label2.id, label3.id]
+          ).execute(issuable)
 
-        expect(issuable.reload.labels).to match_array([label2, label3])
+          expect(issuable.reload.labels).to match_array([label2, label3])
+        end
       end
     end
 
-    context 'scoped labels are not available' do
+    context 'when scoped labels are not available' do
       before do
         stub_licensed_features(scoped_labels: false, epics: true)
       end

spec/services/labels/available_labels_service_spec.rb

@@ -73,6 +73,12 @@ describe Labels::AvailableLabelsService do
 
         expect(result).to match_array([project_label.id, group_label.id])
       end
+
+      it 'returns labels in preserved order' do
+        result = described_class.new(user, project, ids: label_ids.reverse).filter_labels_ids_in_param(:ids)
+
+        expect(result).to eq([group_label.id, project_label.id])
+      end
     end
 
     context 'when parent is a group' do