Merge branch '20378-natural-sort-issue-numbers' into 'master'

Change issues sentence to use natural sorting

Closes #20378

See merge request !7110

5 files changed, 132 insertions, 9 deletions

diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 38be073c8dc..e347f61fb8d 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -56,11 +56,12 @@ module MergeRequestsHelper
   end
 
   def issues_sentence(issues)
-    # Sorting based on the `#123` or `group/project#123` reference will sort
-    # local issues first.
-    issues.map do |issue|
+    # Issuable sorter will sort local issues, then issues from the same
+    # namespace, then all other issues.
+    issues = Gitlab::IssuableSorter.sort(@project, issues).map do |issue|
       issue.to_reference(@project)
-    end.sort.to_sentence
+    end
+    issues.to_sentence
   end
 
   def mr_closes_issues
diff --git a/changelogs/unreleased/20378-natural-sort-issue-numbers.yml b/changelogs/unreleased/20378-natural-sort-issue-numbers.yml
new file mode 100644
index 00000000000..2ebc8485ddf
--- /dev/null
+++ b/changelogs/unreleased/20378-natural-sort-issue-numbers.yml
@@ -0,0 +1,4 @@
+---
+title: Change issues list in MR to natural sorting
+merge_request: 7110
+author: Jeff Stubler
diff --git a/lib/gitlab/issuable_sorter.rb b/lib/gitlab/issuable_sorter.rb
new file mode 100644
index 00000000000..d392214867a
--- /dev/null
+++ b/lib/gitlab/issuable_sorter.rb
@@ -0,0 +1,29 @@
+module Gitlab
+  module IssuableSorter
+    class << self
+      def sort(project, issuables, &sort_key)
+        grouped_items = issuables.group_by do |issuable|
+          if issuable.project.id == project.id
+            :project_ref
+          elsif issuable.project.namespace.id == project.namespace.id
+            :namespace_ref
+          else
+            :full_ref
+          end
+        end
+
+        natural_sort_issuables(grouped_items[:project_ref], project) +
+          natural_sort_issuables(grouped_items[:namespace_ref], project) +
+          natural_sort_issuables(grouped_items[:full_ref], project)
+      end
+
+      private
+
+      def natural_sort_issuables(issuables, project)
+        VersionSorter.sort(issuables || []) do |issuable|
+          issuable.to_reference(project)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb
index 25f23826648..e9037749ef2 100644
--- a/spec/helpers/merge_requests_helper_spec.rb
+++ b/spec/helpers/merge_requests_helper_spec.rb
@@ -22,24 +22,51 @@ describe MergeRequestsHelper do
   end
 
   describe '#issues_sentence' do
+    let(:project) { create :project }
+
     subject { issues_sentence(issues) }
     let(:issues) do
-      [build(:issue, iid: 1), build(:issue, iid: 2), build(:issue, iid: 3)]
+      [build(:issue, iid: 2, project: project),
+       build(:issue, iid: 3, project: project),
+       build(:issue, iid: 1, project: project)]
     end
 
-    it { is_expected.to eq('#1, #2, and #3') }
+    it do
+      @project = project
+
+      is_expected.to eq('#1, #2, and #3')
+    end
 
     context 'for JIRA issues' do
       let(:project) { create(:empty_project) }
       let(:issues) do
         [
-          ExternalIssue.new('JIRA-123', project),
           ExternalIssue.new('JIRA-456', project),
-          ExternalIssue.new('FOOBAR-7890', project)
+          ExternalIssue.new('FOOBAR-7890', project),
+          ExternalIssue.new('JIRA-123', project)
         ]
       end
 
-      it { is_expected.to eq('FOOBAR-7890, JIRA-123, and JIRA-456') }
+      it do
+        @project = project
+        is_expected.to eq('FOOBAR-7890, JIRA-123, and JIRA-456')
+      end
+    end
+
+    context 'for issues from multiple namespaces' do
+      let(:project) { create(:project) }
+      let(:other_project) { create(:project) }
+      let(:issues) do
+        [build(:issue, iid: 2, project: project),
+         build(:issue, iid: 3, project: other_project),
+         build(:issue, iid: 1, project: project)]
+      end
+
+      it do
+        @project = project
+        
+        is_expected.to eq("#1, #2, and #{other_project.namespace.path}/#{other_project.path}#3")
+      end
     end
   end
 
diff --git a/spec/lib/gitlab/issuable_sorter_spec.rb b/spec/lib/gitlab/issuable_sorter_spec.rb
new file mode 100644
index 00000000000..c9a434b2bcf
--- /dev/null
+++ b/spec/lib/gitlab/issuable_sorter_spec.rb
@@ -0,0 +1,62 @@
+require 'spec_helper'
+
+describe Gitlab::IssuableSorter, lib: true do
+  let(:namespace1) { build(:namespace, id: 1) }
+  let(:project1) { build(:project, id: 1, namespace: namespace1) }
+
+  let(:project2) { build(:project, id: 2, path: "a", namespace: project1.namespace) }
+  let(:project3) { build(:project, id: 3, path: "b", namespace: project1.namespace) }
+
+  let(:namespace2) { build(:namespace, id: 2, path: "a") }
+  let(:namespace3) { build(:namespace, id: 3, path: "b") }
+  let(:project4) { build(:project, id: 4, path: "a", namespace: namespace2) }
+  let(:project5) { build(:project, id: 5, path: "b", namespace: namespace2) }
+  let(:project6) { build(:project, id: 6, path: "a", namespace: namespace3) }
+
+  let(:unsorted) { [sorted[2], sorted[3], sorted[0], sorted[1]] }
+
+  let(:sorted) do
+    [build(:issue, iid: 1, project: project1),
+     build(:issue, iid: 2, project: project1),
+     build(:issue, iid: 10, project: project1),
+     build(:issue, iid: 20, project: project1)]
+  end
+
+  it 'sorts references by a given key' do
+    expect(described_class.sort(project1, unsorted)).to eq(sorted)
+  end
+
+  context 'for JIRA issues' do
+    let(:sorted) do
+      [ExternalIssue.new('JIRA-1', project1),
+       ExternalIssue.new('JIRA-2', project1),
+       ExternalIssue.new('JIRA-10', project1),
+       ExternalIssue.new('JIRA-20', project1)]
+    end
+
+    it 'sorts references by a given key' do
+      expect(described_class.sort(project1, unsorted)).to eq(sorted)
+    end
+  end
+
+  context 'for references from multiple projects and namespaces' do
+    let(:sorted) do
+      [build(:issue, iid: 1, project: project1),
+       build(:issue, iid: 2, project: project1),
+       build(:issue, iid: 10, project: project1),
+       build(:issue, iid: 1, project: project2),
+       build(:issue, iid: 1, project: project3),
+       build(:issue, iid: 1, project: project4),
+       build(:issue, iid: 1, project: project5),
+       build(:issue, iid: 1, project: project6)]
+    end
+    let(:unsorted) do
+      [sorted[3], sorted[1], sorted[4], sorted[2],
+       sorted[6], sorted[5], sorted[0], sorted[7]]
+    end
+
+    it 'sorts references by project and then by a given key' do
+      expect(subject.sort(project1, unsorted)).to eq(sorted)
+    end
+  end
+end