Merge branch '357229-return-sorted-labels-in-mutation-responses' into 'master'

Reset labels after issuables updates

See merge request gitlab-org/gitlab!83946

app/services/issuable_base_service.rb

@@ -539,6 +539,9 @@ class IssuableBaseService < ::BaseProjectService
   def handle_label_changes(issuable, old_labels)
     return unless has_label_changes?(issuable, old_labels)
 
+    # reset to preserve the label sort order (title ASC)
+    issuable.labels.reset
+
     GraphqlTriggers.issuable_labels_updated(issuable)
   end
 

ee/spec/requests/api/graphql/mutations/epics/update_spec.rb

@@ -8,9 +8,9 @@ RSpec.describe Mutations::Epics::Update do
   let_it_be(:current_user) { create(:user) }
   let_it_be(:group) { create(:group) }
 
-  let(:label_1) { create(:group_label, group: group) }
-  let(:label_2) { create(:group_label, group: group) }
-  let(:label_3) { create(:group_label, group: group) }
+  let(:label_1) { create(:group_label, title: "a", group: group) }
+  let(:label_2) { create(:group_label, title: "b", group: group) }
+  let(:label_3) { create(:group_label, title: "c", group: group) }
   let(:epic) { create(:epic, group: group, title: 'original title', labels: [label_2]) }
 
   let(:attributes) do
@@ -25,9 +25,8 @@ RSpec.describe Mutations::Epics::Update do
     }
   end
 
+  let(:params) { { group_path: group.full_path, iid: epic.iid.to_s }.merge(attributes) }
   let(:mutation) do
-    params = { group_path: group.full_path, iid: epic.iid.to_s }.merge(attributes)
-
     graphql_mutation(:update_epic, params)
   end
 
@@ -106,12 +105,39 @@ RSpec.describe Mutations::Epics::Update do
 
       context 'when changing labels of the epic' do
         let(:attributes) { { add_label_ids: [label_1.id, label_3.id], remove_label_ids: label_2.id } }
+        let(:mutation) do
+          graphql_mutation(:update_epic, params) do
+            <<~QL
+                epic {
+                   labels {
+                     nodes {
+                       id
+                     }
+                   }
+                }
+                errors
+            QL
+          end
+        end
 
         it 'adds and removes labels correctly' do
           post_graphql_mutation(mutation, current_user: current_user)
 
           expect(epic.reload.labels).to match_array([label_1, label_3])
         end
+
+        context 'when labels are added' do
+          let(:attributes) { { add_label_ids: [label_1.id, label_3.id] } }
+
+          it 'adds labels correctly and keeps the title ordering' do
+            post_graphql_mutation(mutation, current_user: current_user)
+
+            labels_ids = mutation_response['epic']['labels']['nodes'].map { |l| l['id'] }
+            expected_label_ids = [label_1, label_2, label_3].map { |l| l.to_global_id.to_s }
+
+            expect(labels_ids).to eq(expected_label_ids)
+          end
+        end
       end
 
       context 'when there are ActiveRecord validation errors' do

ee/spec/services/epics/update_service_spec.rb

@@ -537,11 +537,14 @@ RSpec.describe Epics::UpdateService do
       let(:parent) { group }
     end
 
-    it_behaves_like 'broadcasting issuable labels updates' do
-      let(:label_a) { create(:group_label, group: group) }
-      let(:label_b) { create(:group_label, group: group) }
+    context 'labels are updated' do
+      let(:label_a) { create(:group_label, title: 'a', group: group) }
+      let(:label_b) { create(:group_label, title: 'b', group: group) }
       let(:issuable) { epic }
 
+      it_behaves_like 'keeps issuable labels sorted after update'
+      it_behaves_like 'broadcasting issuable labels updates'
+
       def update_issuable(update_params)
         update_epic(update_params)
       end

spec/requests/api/graphql/mutations/issues/update_spec.rb

@@ -8,8 +8,8 @@ RSpec.describe 'Update of an existing issue' do
   let_it_be(:current_user) { create(:user) }
   let_it_be(:project) { create(:project, :public) }
   let_it_be(:issue) { create(:issue, project: project) }
-  let_it_be(:label1) { create(:label, project: project) }
-  let_it_be(:label2) { create(:label, project: project) }
+  let_it_be(:label1) { create(:label, title: "a", project: project) }
+  let_it_be(:label2) { create(:label, title: "b", project: project) }
 
   let(:input) do
     {
@@ -124,7 +124,7 @@ RSpec.describe 'Update of an existing issue' do
       context 'add and remove labels' do
         let(:input_params) { input.merge(extra_params).merge({ addLabelIds: [label1.id], removeLabelIds: [label2.id] }) }
 
-        it 'returns error for mutually exclusive arguments' do
+        it 'returns correct labels' do
           post_graphql_mutation(mutation, current_user: current_user)
 
           expect(response).to have_gitlab_http_status(:success)
@@ -132,6 +132,22 @@ RSpec.describe 'Update of an existing issue' do
           expect(mutation_response['issue']['labels']).to include({ "nodes" => [{ "id" => label1.to_global_id.to_s }] })
         end
       end
+
+      context 'add labels' do
+        let(:input_params) { input.merge(extra_params).merge({ addLabelIds: [label1.id] }) }
+
+        before do
+          issue.update!({ labels: [label2] })
+        end
+
+        it 'adds labels and keeps the title ordering' do
+          post_graphql_mutation(mutation, current_user: current_user)
+
+          expect(response).to have_gitlab_http_status(:success)
+          expect(json_response['errors']).to be_nil
+          expect(mutation_response['issue']['labels']['nodes']).to eq([{ "id" => label1.to_global_id.to_s }, { "id" => label2.to_global_id.to_s }])
+        end
+      end
     end
   end
 end

spec/requests/api/graphql/mutations/merge_requests/set_labels_spec.rb

@@ -8,8 +8,8 @@ RSpec.describe 'Setting labels of a merge request' do
   let(:current_user) { create(:user) }
   let(:merge_request) { create(:merge_request) }
   let(:project) { merge_request.project }
-  let(:label) { create(:label, project: project) }
-  let(:label2) { create(:label, project: project) }
+  let(:label) { create(:label, title: "a", project: project) }
+  let(:label2) { create(:label, title: "b", project: project) }
   let(:input) { { label_ids: [GitlabSchema.id_from_object(label).to_s] } }
 
   let(:mutation) do
@@ -81,12 +81,12 @@ RSpec.describe 'Setting labels of a merge request' do
       merge_request.update!(labels: [label2])
     end
 
-    it 'sets the labels, without removing others' do
+    it 'sets the labels and resets labels to keep the title ordering, without removing others' do
       post_graphql_mutation(mutation, current_user: current_user)
 
       expect(response).to have_gitlab_http_status(:success)
       expect(mutation_label_nodes.count).to eq(2)
-      expect(mutation_label_nodes).to contain_exactly({ 'id' => label.to_global_id.to_s }, { 'id' => label2.to_global_id.to_s })
+      expect(mutation_label_nodes).to eq([{ 'id' => label.to_global_id.to_s }, { 'id' => label2.to_global_id.to_s }])
     end
   end
 

spec/services/issues/update_service_spec.rb

@@ -9,8 +9,8 @@ RSpec.describe Issues::UpdateService, :mailer do
   let_it_be(:guest) { create(:user) }
   let_it_be(:group) { create(:group, :public, :crm_enabled) }
   let_it_be(:project, reload: true) { create(:project, :repository, group: group) }
-  let_it_be(:label) { create(:label, project: project) }
-  let_it_be(:label2) { create(:label, project: project) }
+  let_it_be(:label) { create(:label, title: 'a', project: project) }
+  let_it_be(:label2) { create(:label, title: 'b', project: project) }
   let_it_be(:milestone) { create(:milestone, project: project) }
 
   let(:issue) do
@@ -1361,11 +1361,14 @@ RSpec.describe Issues::UpdateService, :mailer do
       end
     end
 
-    it_behaves_like 'broadcasting issuable labels updates' do
+    context 'labels are updated' do
       let(:label_a) { label }
       let(:label_b) { label2 }
       let(:issuable) { issue }
 
+      it_behaves_like 'keeps issuable labels sorted after update'
+      it_behaves_like 'broadcasting issuable labels updates'
+
       def update_issuable(update_params)
         update_issue(update_params)
       end

spec/services/merge_requests/update_service_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
-  let(:label) { create(:label, project: project) }
+  let(:label) { create(:label, title: 'a', project: project) }
   let(:label2) { create(:label) }
   let(:milestone) { create(:milestone, project: project) }
 
@@ -1193,11 +1193,14 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       let(:issuable) { described_class.new(project: project, current_user: user, params: params).execute(existing_merge_request) }
     end
 
-    it_behaves_like 'broadcasting issuable labels updates' do
+    context 'labels are updated' do
       let(:label_a) { label }
-      let(:label_b) { create(:label, project: project) }
+      let(:label_b) { create(:label, title: 'b', project: project) }
       let(:issuable) { merge_request }
 
+      it_behaves_like 'keeps issuable labels sorted after update'
+      it_behaves_like 'broadcasting issuable labels updates'
+
       def update_issuable(update_params)
         update_merge_request(update_params)
       end

spec/support/services/issuable_update_service_shared_examples.rb

@@ -24,6 +24,20 @@ RSpec.shared_examples 'issuable update service' do
   end
 end
 
+RSpec.shared_examples 'keeps issuable labels sorted after update' do
+  before do
+    update_issuable(label_ids: [label_b.id])
+  end
+
+  context 'when label is changed' do
+    it 'keeps the labels sorted by title ASC' do
+      update_issuable({ add_label_ids: [label_a.id] })
+
+      expect(issuable.labels).to eq([label_a, label_b])
+    end
+  end
+end
+
 RSpec.shared_examples 'broadcasting issuable labels updates' do
   before do
     update_issuable(label_ids: [label_a.id])