Order milestone issues by position ascending in api

5 files changed, 40 insertions, 6 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 37c727b5d9f..3cf4c67d7e7 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -63,6 +63,7 @@ module Issuable
     scope :authored, ->(user) { where(author_id: user) }
     scope :assigned_to, ->(u) { where(assignee_id: u.id)}
     scope :recent, -> { reorder(id: :desc) }
+    scope :order_position_asc, -> { reorder(position: :asc) }
     scope :assigned, -> { where("assignee_id IS NOT NULL") }
     scope :unassigned, -> { where("assignee_id IS NULL") }
     scope :of_projects, ->(ids) { where(project_id: ids) }
@@ -144,6 +145,7 @@ module Issuable
                when 'downvotes_desc' then order_downvotes_desc
                when 'upvotes_desc' then order_upvotes_desc
                when 'priority' then order_labels_priority(excluded_labels: excluded_labels)
+               when 'position_asc' then  order_position_asc
                else
                  order_by(method)
                end
diff --git a/app/views/shared/milestones/_issuables.html.haml b/app/views/shared/milestones/_issuables.html.haml
index a93cbd1041f..8af3bd597c5 100644
--- a/app/views/shared/milestones/_issuables.html.haml
+++ b/app/views/shared/milestones/_issuables.html.haml
@@ -13,6 +13,6 @@
   - class_prefix = dom_class(issuables).pluralize
   %ul{ class: "well-list #{class_prefix}-sortable-list", id: "#{class_prefix}-list-#{id}", "data-state" => id }
     = render partial: 'shared/milestones/issuable',
-             collection: issuables.sort_by(&:position),
+             collection: issuables.order_position_asc,
              as: :issuable,
              locals: { show_project_name: show_project_name, show_full_project_name: show_full_project_name }
diff --git a/changelogs/unreleased/28874-fix-milestone-issues-position-order-in-api.yml b/changelogs/unreleased/28874-fix-milestone-issues-position-order-in-api.yml
new file mode 100644
index 00000000000..0177394aa0f
--- /dev/null
+++ b/changelogs/unreleased/28874-fix-milestone-issues-position-order-in-api.yml
@@ -0,0 +1,4 @@
+---
+title: Order milestone issues by position ascending in api
+merge_request: 9635
+author: George Andrinopoulos
diff --git a/lib/api/milestones.rb b/lib/api/milestones.rb
index e7f7edd95c7..abd263c1dfc 100644
--- a/lib/api/milestones.rb
+++ b/lib/api/milestones.rb
@@ -116,7 +116,8 @@ module API
 
         finder_params = {
           project_id: user_project.id,
-          milestone_title: milestone.title
+          milestone_title: milestone.title,
+          sort: 'position_asc'
         }
 
         issues = IssuesFinder.new(current_user, finder_params).execute
@@ -138,7 +139,8 @@ module API
 
         finder_params = {
           project_id: user_project.id,
-          milestone_id: milestone.id
+          milestone_id: milestone.id,
+          sort: 'position_asc'
         }
 
         merge_requests = MergeRequestsFinder.new(current_user, finder_params).execute
diff --git a/spec/requests/api/milestones_spec.rb b/spec/requests/api/milestones_spec.rb
index 3bb8b6fdbeb..7fb728fed6f 100644
--- a/spec/requests/api/milestones_spec.rb
+++ b/spec/requests/api/milestones_spec.rb
@@ -243,8 +243,8 @@ describe API::Milestones, api: true  do
     describe 'confidential issues' do
       let(:public_project) { create(:empty_project, :public) }
       let(:milestone) { create(:milestone, project: public_project) }
-      let(:issue) { create(:issue, project: public_project) }
-      let(:confidential_issue) { create(:issue, confidential: true, project: public_project) }
+      let(:issue) { create(:issue, project: public_project, position: 2) }
+      let(:confidential_issue) { create(:issue, confidential: true, project: public_project, position: 1) }
 
       before do
         public_project.team << [user, :developer]
@@ -283,11 +283,24 @@ describe API::Milestones, api: true  do
         expect(json_response.size).to eq(1)
         expect(json_response.map { |issue| issue['id'] }).to include(issue.id)
       end
+
+      it 'returns issues ordered by position asc' do
+        get api("/projects/#{public_project.id}/milestones/#{milestone.id}/issues", user)
+
+        expect(response).to have_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(2)
+        expect(json_response.first['id']).to eq(confidential_issue.id)
+        expect(json_response.second['id']).to eq(issue.id)
+      end
     end
   end
 
   describe 'GET /projects/:id/milestones/:milestone_id/merge_requests' do
-    let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:merge_request) { create(:merge_request, source_project: project, position: 2) }
+    let(:another_merge_request) { create(:merge_request, :simple, source_project: project, position: 1) }
+
     before do
       milestone.merge_requests << merge_request
     end
@@ -320,5 +333,18 @@ describe API::Milestones, api: true  do
 
       expect(response).to have_http_status(401)
     end
+
+    it 'returns merge_requests ordered by position asc' do
+      milestone.merge_requests << another_merge_request
+
+      get api("/projects/#{project.id}/milestones/#{milestone.id}/merge_requests", user)
+
+      expect(response).to have_http_status(200)
+      expect(response).to include_pagination_headers
+      expect(json_response).to be_an Array
+      expect(json_response.size).to eq(2)
+      expect(json_response.first['id']).to eq(another_merge_request.id)
+      expect(json_response.second['id']).to eq(merge_request.id)
+    end
   end
 end