Merge branch 'issue_20900' into 'master'

Remove issuable drag and drop and sorting from milestone view

Closes #20900

See merge request !11950

app/assets/javascripts/milestone.js

@@ -4,87 +4,7 @@
 
 (function() {
   this.Milestone = (function() {
-    Milestone.updateIssue = function(li, issue_url, data) {
-      return $.ajax({
-        type: "PUT",
-        url: issue_url,
-        data: data,
-        success: function(_data) {
-          return Milestone.successCallback(_data, li);
-        },
-        error: function(data) {
-          return new Flash("Issue update failed", 'alert');
-        },
-        dataType: "json"
-      });
-    };
-
-    Milestone.sortIssues = function(url, data) {
-      return $.ajax({
-        type: "PUT",
-        url,
-        data: data,
-        success: function(_data) {
-          return Milestone.successCallback(_data);
-        },
-        error: function() {
-          return new Flash("Issues update failed", 'alert');
-        },
-        dataType: "json"
-      });
-    };
-
-    Milestone.sortMergeRequests = function(url, data) {
-      return $.ajax({
-        type: "PUT",
-        url,
-        data: data,
-        success: function(_data) {
-          return Milestone.successCallback(_data);
-        },
-        error: function(data) {
-          return new Flash("Issue update failed", 'alert');
-        },
-        dataType: "json"
-      });
-    };
-
-    Milestone.updateMergeRequest = function(li, merge_request_url, data) {
-      return $.ajax({
-        type: "PUT",
-        url: merge_request_url,
-        data: data,
-        success: function(_data) {
-          return Milestone.successCallback(_data, li);
-        },
-        error: function(data) {
-          return new Flash("Issue update failed", 'alert');
-        },
-        dataType: "json"
-      });
-    };
-
-    Milestone.successCallback = function(data, element) {
-      const $avatarContainer = $(element).find('.assignee-icon');
-      $avatarContainer.empty();
-
-      if (data.assignees && data.assignees.length > 0) {
-        const $avatars = data.assignees.map((assignee) => {
-          const img_tag = $('<img/>');
-          img_tag.attr('src', assignee.avatar_url);
-          img_tag.addClass('avatar s16');
-          return img_tag;
-        });
-
-        $avatarContainer.append($avatars);
-      }
-    };
-
     function Milestone() {
-      this.issuesSortEndpoint = $('#tab-issues').data('sort-endpoint');
-      this.mergeRequestsSortEndpoint = $('#tab-merge-requests').data('sort-endpoint');
-
-      this.bindIssuesSorting();
       this.bindTabsSwitching();
 
       // Load merge request tab if it is active
@@ -94,22 +14,6 @@
       this.loadInitialTab();
     }
 
-    Milestone.prototype.bindIssuesSorting = function() {
-      if (!this.issuesSortEndpoint) return;
-
-      $('#issues-list-unassigned, #issues-list-ongoing, #issues-list-closed').each(function (i, el) {
-        this.createSortable(el, {
-          group: 'issue-list',
-          listEls: $('.issues-sortable-list'),
-          fieldName: 'issue',
-          sortCallback: (data) => {
-            Milestone.sortIssues(this.issuesSortEndpoint, data);
-          },
-          updateCallback: Milestone.updateIssue,
-        });
-      }.bind(this));
-    };
-
     Milestone.prototype.bindTabsSwitching = function() {
       return $('a[data-toggle="tab"]').on('show.bs.tab', (e) => {
         const $target = $(e.target);
@@ -119,69 +23,6 @@
       });
     };
 
-    Milestone.prototype.bindMergeRequestSorting = function() {
-      if (!this.mergeRequestsSortEndpoint) return;
-
-      $("#merge_requests-list-unassigned, #merge_requests-list-ongoing, #merge_requests-list-closed").each(function (i, el) {
-        this.createSortable(el, {
-          group: 'merge-request-list',
-          listEls: $(".merge_requests-sortable-list:not(#merge_requests-list-merged)"),
-          fieldName: 'merge_request',
-          sortCallback: (data) => {
-            Milestone.sortMergeRequests(this.mergeRequestsSortEndpoint, data);
-          },
-          updateCallback: Milestone.updateMergeRequest,
-        });
-      }.bind(this));
-    };
-
-    Milestone.prototype.createSortable = function(el, opts) {
-      return Sortable.create(el, {
-        group: opts.group,
-        filter: '.is-disabled',
-        forceFallback: true,
-        onStart: function(e) {
-          opts.listEls.css('min-height', e.item.offsetHeight);
-        },
-        onEnd: function () {
-          opts.listEls.css("min-height", "0px");
-        },
-        onUpdate: function(e) {
-          var ids = this.toArray(),
-            data;
-
-          if (ids.length) {
-            data = ids.map(function(id) {
-              return 'sortable_' + opts.fieldName + '[]=' + id;
-            }).join('&');
-
-            opts.sortCallback(data);
-          }
-        },
-        onAdd: function (e) {
-          var data, issuableId, issuableUrl, newState;
-          newState = e.to.dataset.state;
-          issuableUrl = e.item.dataset.url;
-          data = (function() {
-            switch (newState) {
-              case 'ongoing':
-                return `${opts.fieldName}[assignee_ids][]=${gon.current_user_id}`;
-              case 'unassigned':
-                return `${opts.fieldName}[assignee_ids][]=0`;
-              case 'closed':
-                return opts.fieldName + '[state_event]=close';
-            }
-          })();
-          if (e.from.dataset.state === 'closed') {
-            data += '&' + opts.fieldName + '[state_event]=reopen';
-          }
-
-          opts.updateCallback(e.item, issuableUrl, data);
-          this.options.onUpdate.call(this, e);
-        }
-      });
-    };
-
     Milestone.prototype.loadInitialTab = function() {
       const $target = $(`.js-milestone-tabs a[href="${location.hash}"]`);
 
@@ -203,10 +44,6 @@
         .done((data) => {
           $(tabElId).html(data.html);
           $target.addClass('is-loaded');
-
-          if (tabElId === '#tab-merge-requests') {
-            this.bindMergeRequestSorting();
-          }
         });
       }
     };

app/assets/stylesheets/pages/milestone.scss

@@ -111,8 +111,8 @@
   }
 }
 
-.issues-sortable-list,
-.merge_requests-sortable-list {
+.milestone-issues-list,
+.milestone-merge_requests-list {
   .issuable-detail {
     display: block;
     margin-top: 7px;
@@ -197,6 +197,4 @@
 
 .issuable-row {
   background-color: $white-light;
-  cursor: -webkit-grab;
-  cursor: grab;
 }

app/controllers/concerns/milestone_actions.rb

@@ -6,7 +6,7 @@ module MilestoneActions
       format.html { redirect_to milestone_redirect_path }
       format.json do
         render json: tabs_json("shared/milestones/_merge_requests_tab", {
-          merge_requests: @milestone.merge_requests,
+          merge_requests: @milestone.sorted_merge_requests,
           show_project_name: true
         })
       end

app/controllers/projects/milestones_controller.rb

@@ -2,7 +2,7 @@ class Projects::MilestonesController < Projects::ApplicationController
   include MilestoneActions
 
   before_action :module_enabled
-  before_action :milestone, only: [:edit, :update, :destroy, :show, :sort_issues, :sort_merge_requests, :merge_requests, :participants, :labels]
+  before_action :milestone, only: [:edit, :update, :destroy, :show, :merge_requests, :participants, :labels]
 
   # Allow read any milestone
   before_action :authorize_read_milestone!
@@ -85,22 +85,6 @@ class Projects::MilestonesController < Projects::ApplicationController
     end
   end
 
-  def sort_issues
-    @milestone.sort_issues(params['sortable_issue'].map(&:to_i))
-
-    render json: { saved: true }
-  end
-
-  def sort_merge_requests
-    @merge_requests = @milestone.merge_requests.where(id: params['sortable_merge_request'])
-    @merge_requests.each do |merge_request|
-      merge_request.position = params['sortable_merge_request'].index(merge_request.id.to_s) + 1
-      merge_request.save
-    end
-
-    render json: { saved: true }
-  end
-
   protected
 
   def milestone

app/models/concerns/issuable.rb

@@ -67,7 +67,6 @@ module Issuable
 
     scope :authored, ->(user) { where(author_id: user) }
     scope :recent, -> { reorder(id: :desc) }
-    scope :order_position_asc, -> { reorder(position: :asc) }
     scope :of_projects, ->(ids) { where(project_id: ids) }
     scope :of_milestones, ->(ids) { where(milestone_id: ids) }
     scope :with_milestone, ->(title) { left_joins_milestones.where(milestones: { title: title }) }
@@ -139,7 +138,6 @@ module Issuable
                when 'upvotes_desc' then order_upvotes_desc
                when 'label_priority' then order_labels_priority(excluded_labels: excluded_labels)
                when 'priority' then order_due_date_and_labels_priority(excluded_labels: excluded_labels)
-               when 'position_asc' then  order_position_asc
                else
                  order_by(method)
                end

app/models/concerns/milestoneish.rb

@@ -40,10 +40,18 @@ module Milestoneish
   def issues_visible_to_user(user)
     memoize_per_user(user, :issues_visible_to_user) do
       IssuesFinder.new(user, issues_finder_params)
-        .execute.includes(:assignees).where(milestone_id: milestoneish_ids)
+        .execute.preload(:assignees).where(milestone_id: milestoneish_ids)
     end
   end
 
+  def sorted_issues(user)
+    issues_visible_to_user(user).preload_associations.sort('label_priority')
+  end
+
+  def sorted_merge_requests
+    merge_requests.sort('label_priority')
+  end
+
   def upcoming?
     start_date && start_date.future?
   end

app/models/issue.rb

@@ -9,6 +9,9 @@ class Issue < ActiveRecord::Base
   include Spammable
   include FasterCacheKeys
   include RelativePositioning
+  include IgnorableColumn
+
+  ignore_column :position
 
   DueDateStruct = Struct.new(:title, :name).freeze
   NoDueDate     = DueDateStruct.new('No Due Date', '0').freeze
@@ -44,7 +47,7 @@ class Issue < ActiveRecord::Base
 
   scope :created_after, -> (datetime) { where("created_at >= ?", datetime) }
 
-  scope :include_associations, -> { includes(:labels, project: :namespace) }
+  scope :preload_associations, -> { preload(:labels, project: :namespace) }
 
   after_save :expire_etag_cache
 

app/models/merge_request.rb

@@ -4,6 +4,9 @@ class MergeRequest < ActiveRecord::Base
   include Noteable
   include Referable
   include Sortable
+  include IgnorableColumn
+
+  ignore_column :position
 
   belongs_to :target_project, class_name: "Project"
   belongs_to :source_project, class_name: "Project"

app/models/milestone.rb

@@ -164,38 +164,6 @@ class Milestone < ActiveRecord::Base
     write_attribute(:title, sanitize_title(value)) if value.present?
   end
 
-  # Sorts the issues for the given IDs.
-  #
-  # This method runs a single SQL query using a CASE statement to update the
-  # position of all issues in the current milestone (scoped to the list of IDs).
-  #
-  # Given the ids [10, 20, 30] this method produces a SQL query something like
-  # the following:
-  #
-  #     UPDATE issues
-  #     SET position = CASE
-  #       WHEN id = 10 THEN 1
-  #       WHEN id = 20 THEN 2
-  #       WHEN id = 30 THEN 3
-  #       ELSE position
-  #     END
-  #     WHERE id IN (10, 20, 30);
-  #
-  # This method expects that the IDs given in `ids` are already Fixnums.
-  def sort_issues(ids)
-    pairs = []
-
-    ids.each_with_index do |id, index|
-      pairs << id
-      pairs << index + 1
-    end
-
-    conditions = 'WHEN id = ? THEN ? ' * ids.length
-
-    issues.where(id: ids).
-      update_all(["position = CASE #{conditions} ELSE position END", *pairs])
-  end
-
   private
 
   def milestone_format_reference(format = :iid)

app/serializers/issuable_entity.rb

@@ -5,7 +5,6 @@ class IssuableEntity < Grape::Entity
   expose :description
   expose :lock_version
   expose :milestone_id
-  expose :position
   expose :state
   expose :title
   expose :updated_by_id

app/views/shared/milestones/_issuables.html.haml

@@ -8,11 +8,11 @@
       = title
     - if show_counter
       .counter
-        = number_with_delimiter(issuables.size)
+        = number_with_delimiter(issuables.length)
 
   - class_prefix = dom_class(issuables).pluralize
-  %ul{ class: "well-list #{class_prefix}-sortable-list", id: "#{class_prefix}-list-#{id}", "data-state" => id }
+  %ul{ class: "well-list milestone-#{class_prefix}-list", id: "#{class_prefix}-list-#{id}" }
     = render partial: 'shared/milestones/issuable',
-             collection: issuables.order_position_asc,
+             collection: issuables,
              as: :issuable,
              locals: { show_project_name: show_project_name, show_full_project_name: show_full_project_name }

app/views/shared/milestones/_tabs.html.haml

@@ -31,12 +31,12 @@
 .tab-content.milestone-content
   - if milestone.is_a?(GlobalMilestone) || can?(current_user, :read_issue, @project)
     .tab-pane.active#tab-issues{ data: { sort_endpoint: (sort_issues_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } }
-      = render 'shared/milestones/issues_tab', issues: milestone.issues_visible_to_user(current_user).include_associations, show_project_name: show_project_name, show_full_project_name: show_full_project_name
-    .tab-pane#tab-merge-requests{ data: { sort_endpoint: (sort_merge_requests_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } }
+      = render 'shared/milestones/issues_tab', issues: milestone.sorted_issues(current_user), show_project_name: show_project_name, show_full_project_name: show_full_project_name
+    .tab-pane#tab-merge-requests
       -# loaded async
       = render "shared/milestones/tab_loading"
   - else
-    .tab-pane.active#tab-merge-requests{ data: { sort_endpoint: (sort_merge_requests_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } }
+    .tab-pane.active#tab-merge-requests
       -# loaded async
       = render "shared/milestones/tab_loading"
   .tab-pane#tab-participants

changelogs/unreleased/issue_20900.yml

+---
+title: Remove issues/merge requests drag n drop and sorting from milestone view
+merge_request:
+author:

db/post_migrate/20170609183112_remove_position_from_issuables.rb

+class RemovePositionFromIssuables < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def change
+    remove_column :issues, :position, :integer
+    remove_column :merge_requests, :position, :integer
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170607121233) do
+ActiveRecord::Schema.define(version: 20170609183112) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -547,7 +547,6 @@ ActiveRecord::Schema.define(version: 20170607121233) do
     t.integer "project_id"
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.integer "position", default: 0
     t.string "branch_name"
     t.text "description"
     t.integer "milestone_id"
@@ -738,7 +737,6 @@ ActiveRecord::Schema.define(version: 20170607121233) do
     t.integer "target_project_id", null: false
     t.integer "iid"
     t.text "description"
-    t.integer "position", default: 0
     t.datetime "locked_at"
     t.integer "updated_by_id"
     t.text "merge_error"

lib/api/milestones.rb

@@ -117,7 +117,7 @@ module API
         finder_params = {
           project_id: user_project.id,
           milestone_title: milestone.title,
-          sort: 'position_asc'
+          sort: 'label_priority'
         }
 
         issues = IssuesFinder.new(current_user, finder_params).execute
@@ -140,7 +140,7 @@ module API
         finder_params = {
           project_id: user_project.id,
           milestone_title: milestone.title,
-          sort: 'position_asc'
+          sort: 'label_priority'
         }
 
         merge_requests = MergeRequestsFinder.new(current_user, finder_params).execute

spec/features/dashboard/milestone_tabs_spec.rb

@@ -23,7 +23,7 @@ describe 'Dashboard milestone tabs', :js, :feature do
   it 'loads merge requests async' do
     click_link 'Merge Requests'
 
-    expect(page).to have_selector('.merge_requests-sortable-list')
+    expect(page).to have_selector('.milestone-merge_requests-list')
   end
 
   it 'loads participants async' do

spec/features/milestones/milestones_spec.rb

-require 'rails_helper'
+# require 'rails_helper'
 
-describe 'Milestone draggable', feature: true, js: true do
-  include DragTo
+# describe 'Milestone draggable', feature: true, js: true do
+#   include DragTo
 
-  let(:milestone) { create(:milestone, project: project, title: 8.14) }
-  let(:project)   { create(:empty_project, :public) }
-  let(:user)      { create(:user) }
+#   let(:milestone) { create(:milestone, project: project, title: 8.14) }
+#   let(:project)   { create(:empty_project, :public) }
+#   let(:user)      { create(:user) }
 
-  context 'issues' do
-    let(:issue)        { page.find_by_id('issues-list-unassigned').find('li') }
-    let(:issue_target) { page.find_by_id('issues-list-ongoing') }
+#   context 'issues' do
+#     let(:issue)        { page.find_by_id('issues-list-unassigned').find('li') }
+#     let(:issue_target) { page.find_by_id('issues-list-ongoing') }
 
-    it 'does not allow guest to drag issue' do
-      create_and_drag_issue
+#     it 'does not allow guest to drag issue' do
+#       create_and_drag_issue
 
-      expect(issue_target).not_to have_selector('.issuable-row')
-    end
+#       expect(issue_target).not_to have_selector('.issuable-row')
+#     end
 
-    it 'does not allow authorized user to drag issue' do
-      login_as(user)
-      create_and_drag_issue
+#     it 'does not allow authorized user to drag issue' do
+#       login_as(user)
+#       create_and_drag_issue
 
-      expect(issue_target).not_to have_selector('.issuable-row')
-    end
+#       expect(issue_target).not_to have_selector('.issuable-row')
+#     end
 
-    it 'allows author to drag issue' do
-      login_as(user)
-      create_and_drag_issue(author: user)
+#     it 'allows author to drag issue' do
+#       login_as(user)
+#       create_and_drag_issue(author: user)
 
-      expect(issue_target).to have_selector('.issuable-row')
-    end
+#       expect(issue_target).to have_selector('.issuable-row')
+#     end
 
-    it 'allows admin to drag issue' do
-      login_as(:admin)
-      create_and_drag_issue
+#     it 'allows admin to drag issue' do
+#       login_as(:admin)
 
-      expect(issue_target).to have_selector('.issuable-row')
-    end
+#       create_and_drag_issue
 
-    it 'assigns issue when it has been dragged to ongoing list' do
-      login_as(:admin)
-      create_and_drag_issue
+#       expect(issue_target).to have_selector('.issuable-row')
+#     end
+#   end
 
-      expect(@issue.reload.assignees).not_to be_empty
-      expect(page).to have_selector("#sortable_issue_#{@issue.iid} .assignee-icon img", count: 1)
-    end
-  end
+#   context 'merge requests' do
+#     let(:merge_request)        { page.find_by_id('merge_requests-list-unassigned').find('li') }
+#     let(:merge_request_target) { page.find_by_id('merge_requests-list-ongoing') }
 
-  context 'merge requests' do
-    let(:merge_request)        { page.find_by_id('merge_requests-list-unassigned').find('li') }
-    let(:merge_request_target) { page.find_by_id('merge_requests-list-ongoing') }
+#     it 'does not allow guest to drag merge request' do
+#       create_and_drag_merge_request
 
-    it 'does not allow guest to drag merge request' do
-      create_and_drag_merge_request
+#       expect(merge_request_target).not_to have_selector('.issuable-row')
+#     end
 
-      expect(merge_request_target).not_to have_selector('.issuable-row')
-    end
+#     it 'does not allow authorized user to drag merge request' do
+#       login_as(user)
+#       create_and_drag_merge_request
 
-    it 'does not allow authorized user to drag merge request' do
-      login_as(user)
-      create_and_drag_merge_request
+#       expect(merge_request_target).not_to have_selector('.issuable-row')
+#     end
 
-      expect(merge_request_target).not_to have_selector('.issuable-row')
-    end
+#     it 'allows author to drag merge request' do
+#       login_as(user)
+#       create_and_drag_merge_request(author: user)
 
-    it 'allows author to drag merge request' do
-      login_as(user)
-      create_and_drag_merge_request(author: user)
+#       expect(merge_request_target).to have_selector('.issuable-row')
+#     end
 
-      expect(merge_request_target).to have_selector('.issuable-row')
-    end
+#     it 'allows admin to drag merge request' do
+#       login_as(:admin)
+#       create_and_drag_merge_request
 
-    it 'allows admin to drag merge request' do
-      login_as(:admin)
-      create_and_drag_merge_request
+#       expect(merge_request_target).to have_selector('.issuable-row')
+#     end
+#   end
 
-      expect(merge_request_target).to have_selector('.issuable-row')
-    end
-  end
+#   def create_and_drag_issue(params = {})
+#     create(:issue, params.merge(title: 'Foo', project: project, milestone: milestone))
 
-  def create_and_drag_issue(params = {})
-    @issue = create(:issue, params.merge(title: 'Foo', project: project, milestone: milestone))
+#     visit namespace_project_milestone_path(project.namespace, project, milestone)
+#     scroll_into_view('.milestone-content')
+#     drag_to(selector: '.issues-sortable-list', list_to_index: 1)
 
-    visit namespace_project_milestone_path(project.namespace, project, milestone)
-    scroll_into_view('.milestone-content')
-    drag_to(selector: '.issues-sortable-list', list_to_index: 1)
+#     wait_for_requests
+#   end
 
-    wait_for_requests
-  end
+#   def create_and_drag_merge_request(params = {})
+#     create(:merge_request, params.merge(title: 'Foo', source_project: project, target_project: project, milestone: milestone))
 
-  def create_and_drag_merge_request(params = {})
-    create(:merge_request, params.merge(title: 'Foo', source_project: project, target_project: project, milestone: milestone))
+#     visit namespace_project_milestone_path(project.namespace, project, milestone)
+#     page.find("a[href='#tab-merge-requests']").click
 
-    visit namespace_project_milestone_path(project.namespace, project, milestone)
-    page.find("a[href='#tab-merge-requests']").click
+#     wait_for_requests
 
-    wait_for_requests
+#     scroll_into_view('.milestone-content')
+#     drag_to(selector: '.merge_requests-sortable-list', list_to_index: 1)
 
-    scroll_into_view('.milestone-content')
-    drag_to(selector: '.merge_requests-sortable-list', list_to_index: 1)
+#     wait_for_requests
+#   end
 
-    wait_for_requests
-  end
-
-  def scroll_into_view(selector)
-    page.evaluate_script("document.querySelector('#{selector}').scrollIntoView();")
-  end
-end
+#   def scroll_into_view(selector)
+#     page.evaluate_script("document.querySelector('#{selector}').scrollIntoView();")
+#   end
+# end

spec/models/concerns/milestoneish_spec.rb

@@ -19,12 +19,43 @@ describe Milestone, 'Milestoneish' do
   let!(:closed_security_issue_3) { create(:issue, :confidential, :closed, project: project, author: author, milestone: milestone) }
   let!(:closed_security_issue_4) { create(:issue, :confidential, :closed, project: project, assignees: [assignee], milestone: milestone) }
   let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, milestone: milestone) }
+  let(:label_1) { create(:label, title: 'label_1', project: project, priority: 1) }
+  let(:label_2) { create(:label, title: 'label_2', project: project, priority: 2) }
+  let(:label_3) { create(:label, title: 'label_3', project: project) }
 
   before do
     project.team << [member, :developer]
     project.team << [guest, :guest]
   end
 
+  describe '#sorted_issues' do
+    it 'sorts issues by label priority' do
+      issue.labels << label_1
+      security_issue_1.labels << label_2
+      closed_issue_1.labels << label_3
+
+      issues = milestone.sorted_issues(member)
+
+      expect(issues.first).to eq(issue)
+      expect(issues.second).to eq(security_issue_1)
+      expect(issues.third).not_to eq(closed_issue_1)
+    end
+  end
+
+  describe '#sorted_merge_requests' do
+    it 'sorts merge requests by label priority' do
+      merge_request_1 = create(:labeled_merge_request, labels: [label_2], source_project: project, source_branch: 'branch_1', milestone: milestone)
+      merge_request_2 = create(:labeled_merge_request, labels: [label_1], source_project: project, source_branch: 'branch_2', milestone: milestone)
+      merge_request_3 = create(:labeled_merge_request, labels: [label_3], source_project: project, source_branch: 'branch_3', milestone: milestone)
+
+      merge_requests = milestone.sorted_merge_requests
+
+      expect(merge_requests.first).to eq(merge_request_2)
+      expect(merge_requests.second).to eq(merge_request_1)
+      expect(merge_requests.third).to eq(merge_request_3)
+    end
+  end
+
   describe '#closed_items_count' do
     it 'does not count confidential issues for non project members' do
       expect(milestone.closed_items_count(non_member)).to eq 2

spec/models/milestone_spec.rb

@@ -144,35 +144,6 @@ describe Milestone, models: true do
     end
   end
 
-  describe '#sort_issues' do
-    let(:milestone) { create(:milestone) }
-
-    let(:issue1) { create(:issue, milestone: milestone, position: 1) }
-    let(:issue2) { create(:issue, milestone: milestone, position: 2) }
-    let(:issue3) { create(:issue, milestone: milestone, position: 3) }
-    let(:issue4) { create(:issue, position: 42) }
-
-    it 'sorts the given issues' do
-      milestone.sort_issues([issue3.id, issue2.id, issue1.id])
-
-      issue1.reload
-      issue2.reload
-      issue3.reload
-
-      expect(issue1.position).to eq(3)
-      expect(issue2.position).to eq(2)
-      expect(issue3.position).to eq(1)
-    end
-
-    it 'ignores issues not part of the milestone' do
-      milestone.sort_issues([issue3.id, issue2.id, issue1.id, issue4.id])
-
-      issue4.reload
-
-      expect(issue4.position).to eq(42)
-    end
-  end
-
   describe '.search' do
     let(:milestone) { create(:milestone, title: 'foo', description: 'bar') }
 

spec/requests/api/milestones_spec.rb

@@ -5,6 +5,9 @@ describe API::Milestones do
   let!(:project) { create(:empty_project, namespace: user.namespace ) }
   let!(:closed_milestone) { create(:closed_milestone, project: project, title: 'version1', description: 'closed milestone') }
   let!(:milestone) { create(:milestone, project: project, title: 'version2', description: 'open milestone') }
+  let(:label_1) { create(:label, title: 'label_1', project: project, priority: 1) }
+  let(:label_2) { create(:label, title: 'label_2', project: project, priority: 2) }
+  let(:label_3) { create(:label, title: 'label_3', project: project) }
 
   before do
     project.team << [user, :developer]
@@ -228,6 +231,18 @@ describe API::Milestones do
       expect(json_response.first['milestone']['title']).to eq(milestone.title)
     end
 
+    it 'returns project issues sorted by label priority' do
+      issue_1 = create(:labeled_issue, project: project, milestone: milestone, labels: [label_3])
+      issue_2 = create(:labeled_issue, project: project, milestone: milestone, labels: [label_1])
+      issue_3 = create(:labeled_issue, project: project, milestone: milestone, labels: [label_2])
+
+      get api("/projects/#{project.id}/milestones/#{milestone.id}/issues", user)
+
+      expect(json_response.first['id']).to eq(issue_2.id)
+      expect(json_response.second['id']).to eq(issue_3.id)
+      expect(json_response.third['id']).to eq(issue_1.id)
+    end
+
     it 'matches V4 response schema for a list of issues' do
       get api("/projects/#{project.id}/milestones/#{milestone.id}/issues", user)
 
@@ -244,8 +259,8 @@ describe API::Milestones 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, position: 2) }
-      let(:confidential_issue) { create(:issue, confidential: true, project: public_project, position: 1) }
+      let(:issue) { create(:issue, project: public_project) }
+      let(:confidential_issue) { create(:issue, confidential: true, project: public_project) }
 
       before do
         public_project.team << [user, :developer]
@@ -285,7 +300,10 @@ describe API::Milestones do
         expect(json_response.map { |issue| issue['id'] }).to include(issue.id)
       end
 
-      it 'returns issues ordered by position asc' do
+      it 'returns issues ordered by label priority' do
+        issue.labels << label_2
+        confidential_issue.labels << label_1
+
         get api("/projects/#{public_project.id}/milestones/#{milestone.id}/issues", user)
 
         expect(response).to have_http_status(200)
@@ -299,8 +317,8 @@ describe API::Milestones do
   end
 
   describe 'GET /projects/:id/milestones/:milestone_id/merge_requests' do
-    let(:merge_request) { create(:merge_request, source_project: project, position: 2) }
-    let(:another_merge_request) { create(:merge_request, :simple, source_project: project, position: 1) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:another_merge_request) { create(:merge_request, :simple, source_project: project) }
 
     before do
       milestone.merge_requests << merge_request
@@ -318,6 +336,18 @@ describe API::Milestones do
       expect(json_response.first['milestone']['title']).to eq(milestone.title)
     end
 
+    it 'returns project merge_requests sorted by label priority' do
+      merge_request_1 = create(:labeled_merge_request, source_branch: 'branch_1', source_project: project, milestone: milestone, labels: [label_2])
+      merge_request_2 = create(:labeled_merge_request, source_branch: 'branch_2', source_project: project, milestone: milestone, labels: [label_1])
+      merge_request_3 = create(:labeled_merge_request, source_branch: 'branch_3', source_project: project, milestone: milestone, labels: [label_3])
+
+      get api("/projects/#{project.id}/milestones/#{milestone.id}/merge_requests", user)
+
+      expect(json_response.first['id']).to eq(merge_request_2.id)
+      expect(json_response.second['id']).to eq(merge_request_1.id)
+      expect(json_response.third['id']).to eq(merge_request_3.id)
+    end
+
     it 'returns a 404 error if milestone id not found' do
       get api("/projects/#{project.id}/milestones/1234/merge_requests", user)
 
@@ -339,6 +369,8 @@ describe API::Milestones do
 
     it 'returns merge_requests ordered by position asc' do
       milestone.merge_requests << another_merge_request
+      another_merge_request.labels << label_1
+      merge_request.labels << label_2
 
       get api("/projects/#{project.id}/milestones/#{milestone.id}/merge_requests", user)