Keep labels sorted by title after mutation

Labels assigned to an issuable should be kept sorted by title
after mutation.

This commit adds '.reset' to issue and merge request labels
in their mutation resolvers so they get reloaded with
the sort order 'title asc'.

For epic, only the spec coverage is added as
the mutation for epic already utilized '.reset'.

Changelog: changed

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])